[SPARK-48016][SQL] Fix a bug in try_divide function when with decimals #46286

gengliangwang · 2024-04-29T19:08:57Z

What changes were proposed in this pull request?

Currently, the following query will throw DIVIDE_BY_ZERO error instead of returning null

SELECT try_divide(1, decimal(0));

This is caused by the rule DecimalPrecision:

case b @ BinaryOperator(left, right) if left.dataType != right.dataType =>
  (left, right) match {
 ...
    case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType] &&
        l.dataType.isInstanceOf[IntegralType] &&
        literalPickMinimumPrecision =>
      b.makeCopy(Array(Cast(l, DataTypeUtils.fromLiteral(l)), r))

The result of the above makeCopy will contain ANSI as the evalMode, instead of TRY.
This PR is to fix this bug by replacing the makeCopy method calls with withNewChildren

Why are the changes needed?

Bug fix in try_* functions.

Does this PR introduce any user-facing change?

Yes, it fixes a long-standing bug in the try_divide function.

How was this patch tested?

New UT

Was this patch authored or co-authored using generative AI tooling?

No

gengliangwang · 2024-04-29T19:11:10Z

There is another approach at #46251, which fails a test case in the CanonicalizeSuite.
Using withNewChildren is better here.

dongjoon-hyun · 2024-04-29T21:07:48Z

cc @yaooqinn too

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2024-04-29T23:23:47Z

BTW, thank you for adding new test coverage, @gengliangwang .

gengliangwang · 2024-04-29T23:39:20Z

@HyukjinKwon @dongjoon-hyun @yaooqinn Thanks for the review. Merging to master/branch-3.5

Currently, the following query will throw DIVIDE_BY_ZERO error instead of returning null ``` SELECT try_divide(1, decimal(0)); ``` This is caused by the rule `DecimalPrecision`: ``` case b BinaryOperator(left, right) if left.dataType != right.dataType => (left, right) match { ... case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType] && l.dataType.isInstanceOf[IntegralType] && literalPickMinimumPrecision => b.makeCopy(Array(Cast(l, DataTypeUtils.fromLiteral(l)), r)) ``` The result of the above makeCopy will contain `ANSI` as the `evalMode`, instead of `TRY`. This PR is to fix this bug by replacing the makeCopy method calls with withNewChildren Bug fix in try_* functions. Yes, it fixes a long-standing bug in the try_divide function. New UT No Closes #46286 from gengliangwang/avoidMakeCopy. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit 3fbcb26) Signed-off-by: Gengliang Wang <gengliang@apache.org>

Currently, the following query will throw DIVIDE_BY_ZERO error instead of returning null ``` SELECT try_divide(1, decimal(0)); ``` This is caused by the rule `DecimalPrecision`: ``` case b BinaryOperator(left, right) if left.dataType != right.dataType => (left, right) match { ... case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType] && l.dataType.isInstanceOf[IntegralType] && literalPickMinimumPrecision => b.makeCopy(Array(Cast(l, DataTypeUtils.fromLiteral(l)), r)) ``` The result of the above makeCopy will contain `ANSI` as the `evalMode`, instead of `TRY`. This PR is to fix this bug by replacing the makeCopy method calls with withNewChildren Bug fix in try_* functions. Yes, it fixes a long-standing bug in the try_divide function. New UT No Closes apache#46286 from gengliangwang/avoidMakeCopy. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit 3fbcb26) Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? This is a follow-up of SPARK-48016 to update the missed Java 21 golden file. - #46286 ### Why are the changes needed? To recover Java 21 CIs: - https://github.com/apache/spark/actions/workflows/build_java21.yml - https://github.com/apache/spark/actions/workflows/build_maven_java21.yml - https://github.com/apache/spark/actions/workflows/build_maven_java21_macos14.yml ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual tests. I regenerated all in Java 21 and this was the only one affected. ``` $ SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46313 from dongjoon-hyun/SPARK-48016. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2024-05-01T18:15:53Z

Hi, @gengliangwang .

It's a little weird because

This PR doesn't contain sql/core/src/test/resources/log4j2.properties.
master and branch-3.4 commit also does.
However, branch-3.5 commit contains sql/core/src/test/resources/log4j2.properties whose content is suspicious.
- e78ee2c

- logger.parquet_outputcommitter.name = org.apache.parquet.hadoop.ParquetOutputCommitter
+ logger.parquet_outputcommitter.name = org.sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scalaapache.parquet.hadoop.ParquetOutputCommitter

And, the above suspicious change broke branch-3.5.

dongjoon-hyun · 2024-05-01T18:17:47Z

Let me revert branch-3.5 commit first to recover the CI. Could you make a PR to branch-3.5, @gengliangwang ?

gengliangwang · 2024-05-01T18:26:35Z

@dongjoon-hyun Thanks a lot. I will create a backport for 3.5.

Currently, the following query will throw DIVIDE_BY_ZERO error instead of returning null ``` SELECT try_divide(1, decimal(0)); ``` This is caused by the rule `DecimalPrecision`: ``` case b BinaryOperator(left, right) if left.dataType != right.dataType => (left, right) match { ... case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType] && l.dataType.isInstanceOf[IntegralType] && literalPickMinimumPrecision => b.makeCopy(Array(Cast(l, DataTypeUtils.fromLiteral(l)), r)) ``` The result of the above makeCopy will contain `ANSI` as the `evalMode`, instead of `TRY`. This PR is to fix this bug by replacing the makeCopy method calls with withNewChildren Bug fix in try_* functions. Yes, it fixes a long-standing bug in the try_divide function. New UT No Closes apache#46286 from gengliangwang/avoidMakeCopy. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit 3fbcb26) Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? Currently, the following query will throw DIVIDE_BY_ZERO error instead of returning null ``` SELECT try_divide(1, decimal(0)); ``` This is caused by the rule `DecimalPrecision`: ``` case b BinaryOperator(left, right) if left.dataType != right.dataType => (left, right) match { ... case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType] && l.dataType.isInstanceOf[IntegralType] && literalPickMinimumPrecision => b.makeCopy(Array(Cast(l, DataTypeUtils.fromLiteral(l)), r)) ``` The result of the above makeCopy will contain `ANSI` as the `evalMode`, instead of `TRY`. This PR is to fix this bug by replacing the makeCopy method calls with withNewChildren ### Why are the changes needed? Bug fix in try_* functions. ### Does this PR introduce _any_ user-facing change? Yes, it fixes a long-standing bug in the try_divide function. ### How was this patch tested? New UT ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#46286 from gengliangwang/avoidMakeCopy. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? This is a follow-up of SPARK-48016 to update the missed Java 21 golden file. - apache#46286 ### Why are the changes needed? To recover Java 21 CIs: - https://github.com/apache/spark/actions/workflows/build_java21.yml - https://github.com/apache/spark/actions/workflows/build_maven_java21.yml - https://github.com/apache/spark/actions/workflows/build_maven_java21_macos14.yml ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual tests. I regenerated all in Java 21 and this was the only one affected. ``` $ SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46313 from dongjoon-hyun/SPARK-48016. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

gengliangwang added 2 commits April 29, 2024 11:56

makeCopy => withNewChildren

fe0c4c7

add tests

271c387

github-actions bot added the SQL label Apr 29, 2024

gengliangwang mentioned this pull request Apr 29, 2024

[SPARK-48016][SQL] Binary Arithmetic operators should include the evalMode when makeCopy #46251

Closed

gengliangwang requested review from cloud-fan, HyukjinKwon and dongjoon-hyun April 29, 2024 19:12

HyukjinKwon approved these changes Apr 29, 2024

View reviewed changes

dongjoon-hyun approved these changes Apr 29, 2024

View reviewed changes

yaooqinn approved these changes Apr 29, 2024

View reviewed changes

gengliangwang closed this in 3fbcb26 Apr 29, 2024

gengliangwang mentioned this pull request Apr 30, 2024

[SPARK-48016][SQL][3.4] Fix a bug in try_divide function when with decimals #46289

Closed

dongjoon-hyun mentioned this pull request May 1, 2024

[SPARK-48016][SQL][TESTS][FOLLOWUP] Update Java 21 golden file #46313

Closed

gengliangwang mentioned this pull request May 8, 2024

[SPARK-41794][SQL] Add try_remainder function and re-enable column tests #46434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48016][SQL] Fix a bug in try_divide function when with decimals #46286

[SPARK-48016][SQL] Fix a bug in try_divide function when with decimals #46286

gengliangwang commented Apr 29, 2024

gengliangwang commented Apr 29, 2024

dongjoon-hyun commented Apr 29, 2024

dongjoon-hyun left a comment

dongjoon-hyun commented Apr 29, 2024

gengliangwang commented Apr 29, 2024 •

edited

Loading

dongjoon-hyun commented May 1, 2024 •

edited

Loading

dongjoon-hyun commented May 1, 2024

gengliangwang commented May 1, 2024

[SPARK-48016][SQL] Fix a bug in try_divide function when with decimals #46286

[SPARK-48016][SQL] Fix a bug in try_divide function when with decimals #46286

Conversation

gengliangwang commented Apr 29, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

gengliangwang commented Apr 29, 2024

dongjoon-hyun commented Apr 29, 2024

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Apr 29, 2024

gengliangwang commented Apr 29, 2024 • edited Loading

dongjoon-hyun commented May 1, 2024 • edited Loading

dongjoon-hyun commented May 1, 2024

gengliangwang commented May 1, 2024

gengliangwang commented Apr 29, 2024 •

edited

Loading

dongjoon-hyun commented May 1, 2024 •

edited

Loading