fix: div and rem by negative zero #960
Conversation
| @@ -91,13 +91,15 @@ Comet has a plan stability testing framework that can be used to test the stabil | |||
| The plan stability testing framework is located in the `spark` module and can be run using the following command: | |||
|
|
|||
| ```sh | |||
| ./mvnw -pl spark -Dsuites="org.apache.spark.sql.comet.CometTPCDSV1_4_PlanStabilitySuite" test | |||
| ./mvnw -pl spark -Dsuites="org.apache.spark.sql.comet.CometTPCDSV1_4_PlanStabilitySuite" -nsu test | |||
| ./mvnw -pl spark -Dsuites="org.apache.spark.sql.comet.CometTPCDSV1_4_PlanStabilitySuite" -Pspark-3.5 -nsu test | |||
There was a problem hiding this comment.
Since you added these steps to the testing commands, does it makes sense to duplicate them below for the generation commands?
There was a problem hiding this comment.
Thanks, updated.
| } | ||
| } | ||
|
|
||
| def normalizeNaNAndZero(expr: Expression): Expression = { | ||
| expr match { | ||
| case _: KnownFloatingPointNormalized => expr | ||
| case FloatLiteral(f) if !f.equals(-0.0f) => expr |
There was a problem hiding this comment.
Just clarifying: this is saying that as long as the literal isn't -0.0, we don't have to normalize it? And it doesn't look like there's any way to define a literal NaN, so we don't handle that case?
There was a problem hiding this comment.
Just clarifying: this is saying that as long as the literal isn't -0.0, we don't have to normalize it?
Exactly
And it doesn't look like there's any way to define a literal NaN, so we don't handle that case?
We can create a literal NaN, but Spark NormalizeNaNAndZero is not doing much for literal NaN. I think NaN normalization matters only when some expression results are NaN https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala#L194
| @@ -91,27 +91,31 @@ Comet has a plan stability testing framework that can be used to test the stabil | |||
| The plan stability testing framework is located in the `spark` module and can be run using the following command: | |||
|
|
|||
| ```sh | |||
| ./mvnw -pl spark -Dsuites="org.apache.spark.sql.comet.CometTPCDSV1_4_PlanStabilitySuite" test | |||
| ./mvnw -pl spark -Dsuites="org.apache.spark.sql.comet.CometTPCDSV1_4_PlanStabilitySuite" -nsu test | |||
There was a problem hiding this comment.
It helps for faster turnaround
| @@ -2636,7 +2636,11 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim | |||
|
|
|||
| def nullIfWhenPrimitive(expression: Expression): Expression = if (isPrimitive(expression)) { | |||
| val zero = Literal.default(expression.dataType) | |||
| If(EqualTo(expression, zero), Literal.create(null, expression.dataType), expression) | |||
| expression match { | |||
| case _: Literal if expression != zero => expression | |||
There was a problem hiding this comment.
Indirectly related, this will avoid to put EqualTo for non zero literals for divisions
|
Closes #585 |
|
Merged Thank you @mbutrovich @viirya |
Which issue does this PR close?
Closes #521 and #665.
Rationale for this change
What changes are included in this PR?
This PR normalize the right hand of div and rem
How are these changes tested?
Test updated