Skip to content

Conversation

@gengliangwang
Copy link
Member

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.

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 UTs

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

No

with NullIntolerant with SupportQueryContext {

protected val evalMode: EvalMode.Value
protected[sql] val evalMode: EvalMode.Value
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to make sure we can access evalMode in tests.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix the UT failure, @gengliangwang ?

[info] *** 1 TEST FAILED ***
[error] Failed: Total 7392, Failed 1, Errors 0, Passed 7391, Ignored 5
[error] Failed tests:
[error] 	org.apache.spark.sql.catalyst.expressions.CanonicalizeSuite
[error] (catalyst / Test / test) sbt.TestsFailedException: Tests unsuccessful

Copy link
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI

@gengliangwang
Copy link
Member Author

I am starting a new approach in #46286.
Fixing the makeCopy can be tricky. The test failure in CanonicalizeSuite seems a bit complicated to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants