[SPARK-28200][SQL] Decimal overflow handling in ExpressionEncoder by mickjermsurawong-stripe · Pull Request #25016 · apache/spark

mickjermsurawong-stripe · 2019-06-30T05:52:36Z

What changes were proposed in this pull request?

Currently, ExpressionEncoder does not handle bigdecimal overflow. Round-tripping overflowing java/scala BigDecimal/BigInteger returns null.
- The serializer encode java/scala BigDecimal to to sql Decimal, which still has the underlying data to the former.
- When writing out to UnsafeRow, changePrecision will be false and row has null value.
  spark/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeRowWriter.java
  Lines 202 to 206 in 24e1e41
  if (input == null || !input.changePrecision(precision, scale)) {
  BitSetMethods.set(getBuffer(), startingOffset, ordinal);
  // keep the offset for future update
  setOffsetAndSize(ordinal, 0);
  } else {
In SPARK-23179, an option to throw exception on decimal overflow was introduced.
This PR adds the option in ExpressionEncoder to throw when detecting overflowing BigDecimal/BigInteger before its corresponding Decimal gets written to Row. This gives a consistent behavior between decimal arithmetic on sql expression (DecimalPrecision), and getting decimal from dataframe (RowEncoder)

Thanks to @mgaido91 for the very first PR SPARK-23179 and follow-up discussion on this change.
Thanks to @JoshRosen for working with me on this.

How was this patch tested?

added unit tests

JoshRosen · 2019-06-30T06:04:38Z

@mickjermsurawong-stripe, I think we also had a test case for RowEncoder? Could you also submit that change (in RowEncoderSuite.scala) as part of this PR? Even though we're not actually changing RowEncoder behavior here, the old code was lacking explicit test coverage for that path, so it'd be good to pull in that change to strengthen the tests

JoshRosen · 2019-06-30T06:08:43Z

jenkins this is ok to test

mgaido91 · 2019-06-30T06:09:23Z

+ testOverflowingBigNumeric(new BigInteger("9" * 100), "java very big int")
+
+ private def testOverflowingBigNumeric[T: TypeTag](bigDecimal: T, testName: String): Unit = {
+ for {


Seq("true", "false").foreach

SparkQA · 2019-06-30T07:05:01Z

Test build #107048 has finished for PR 25016 at commit b3740ba.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-06-30T19:45:17Z

Test build #107052 has finished for PR 25016 at commit 0ed62b3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-07-01T04:25:22Z

+ encodeDecodeTest(new java.math.BigDecimal(("9" * 20) + "." + "9" * 18),
+ "java decimal within precision/scale limit")
+
+ encodeDecodeTest(BigDecimal(("9" * 20) + "." + "9" * 18).unary_-,


shall we use -BigDecimal... instead of .unary_-

mgaido91

mostly LGTM, just some style comments on the tests. Thanks.

Anyway, may you please also update the description of the PR? Because actually, the point here is not that we are not honoring the newly introduced flag, but previously we were not checking/handling at all the overflow in these situations. So I think we best should make this more clear in the PR description. Thanks.

mgaido91 · 2019-07-01T08:33:38Z

+ }
+ }
+
+ withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key -> true.toString) {


Suggested change
withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key ->true.toString) {
withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key ->"true") {

mgaido91 · 2019-07-01T08:34:08Z

+ }
+
+ private def testDecimalOverflow(schema: StructType, row: Row): Unit = {
+ withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key -> false.toString) {


Suggested change
withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key ->false.toString) {
withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key ->"false") {

mgaido91 · 2019-07-01T08:37:11Z

 assert(row.toSeq(schema).head == decimal)
 }

+ test("RowEncoder should respect nullOnOverflow for decimals") {


nit: may you please also add the Jira number to the test case? Thanks.

Updated. thank you Marco!

SparkQA · 2019-07-01T09:37:03Z

Test build #107062 has finished for PR 25016 at commit 2513ea7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-01T19:54:24Z

Test build #107081 has finished for PR 25016 at commit 87073da.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-07-05T14:05:39Z

thanks, merging to master!

mickjermsurawong-stripe added 2 commits June 29, 2019 22:24

add test
414e48f

overflow check at expression encoder
b3740ba

JoshRosen changed the title ~~[SPARK-28200] Decimal overflow handling~~[SPARK-28200] Decimal overflow handling in ExpressionEncoderJun 30, 2019

JoshRosen changed the title ~~[SPARK-28200] Decimal overflow handling in ExpressionEncoder~~[SPARK-28200][SQL] Decimal overflow handling in ExpressionEncoderJun 30, 2019

JoshRosen added the SQL label Jun 30, 2019

mgaido91 reviewed Jun 30, 2019
View reviewed changes

mickjermsurawong-stripe added 2 commits June 30, 2019 09:32

test overflow flag on row encoder
f922ff3

improve test style
0ed62b3

cloud-fan reviewed Jul 1, 2019
View reviewed changes

address style
2513ea7

mgaido91 reviewed Jul 1, 2019
View reviewed changes

improve test style and follow naming convention
87073da

cloud-fan closed this in 683e270Jul 5, 2019

	if (input == null \|\| !input.changePrecision(precision, scale)) {
	BitSetMethods.set(getBuffer(), startingOffset, ordinal);
	// keep the offset for future update
	setOffsetAndSize(ordinal, 0);
	} else {

	withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key ->true.toString) {
	withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key ->"true") {

Uh oh!

Conversation

mickjermsurawong-stripe commented Jun 30, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

JoshRosen commented Jun 30, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshRosen commented Jun 30, 2019

Uh oh!

mgaido91Jun 30, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 30, 2019

Uh oh!

SparkQA commented Jun 30, 2019

Uh oh!

cloud-fanJul 1, 2019

Choose a reason for hiding this comment

Uh oh!

mgaido91 left a comment

Choose a reason for hiding this comment

Uh oh!

mgaido91Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

mgaido91Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

mgaido91Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

mickjermsurawong-stripeJul 1, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 1, 2019

Uh oh!

SparkQA commented Jul 1, 2019

Uh oh!

cloud-fan commented Jul 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mickjermsurawong-stripe commented Jun 30, 2019•
edited
Loading

JoshRosen commented Jun 30, 2019•
edited
Loading