[SPARK-35152][SQL] ANSI mode: IntegralDivide throws exception on overflow #32260
Conversation
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
Outdated
Show resolved
Hide resolved
| dataType match { | ||
| case LongType if failOnError => | ||
| (x: Any, y: Any) => { | ||
| if (x == Long.MinValue && y == -1) { |
There was a problem hiding this comment.
Shall we do this in DivModLike.eval?
There was a problem hiding this comment.
This is not happening in Divide or Mod
There was a problem hiding this comment.
yea, similar to the codegen path we can use if (isIntegralDivide).
There was a problem hiding this comment.
Yeah it's tricky. It is hard to reuse thedoGenCode here so I added a new flag isIntegralDivide.
The original idea is to keep the special handling code in IntegralDivide if possible.
But now it seems asymmetric in both sides..
|
Test build #137692 has finished for PR 32260 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Have we considered returning |
|
Test build #137695 has finished for PR 32260 at commit
|
|
Test build #137696 has finished for PR 32260 at commit
|
|
Test build #137697 has finished for PR 32260 at commit
|
|
@arvindsaik I think we can make it in safe version functions: |
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
ANSI interval division by a numeric has the same issue. I opened JIRA SPARK-35169 for that.
| protected def decimalToDataTypeCodeGen(decimalResult: String): String = decimalResult | ||
|
|
||
| // Whether we should check overflow or not in ANSI mode. | ||
| protected def checkDivideOverflow: Boolean = false |
There was a problem hiding this comment.
Is this simply left.dataType == LongType? For Divide, the child is either double or decimal, so won't hit this branch.
Under the hood, what we really want to handle is Long.MinValue / -1, so check left.dataType == LongType looks reasonable.
There was a problem hiding this comment.
No, Remainder's left can be Long as well
|
Merging to master |
5 participants
What changes were proposed in this pull request?
IntegralDivide should throw an exception on overflow in ANSI mode.
There is only one case that can cause that:
Why are the changes needed?
ANSI compliance
Does this PR introduce any user-facing change?
Yes, IntegralDivide throws an exception on overflow in ANSI mode
How was this patch tested?
Unit test