You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kazuyukitanimura (via GitHub)" <gi...@apache.org> on 2024/09/23 23:40:01 UTC
[PR] fix: div and rem by negative zero [datafusion-comet]
kazuyukitanimura opened a new pull request, #960:
URL: https://github.com/apache/datafusion-comet/pull/960
## Which issue does this PR close?
Closes https://github.com/apache/datafusion-comet/issues/521 and https://github.com/apache/datafusion-comet/issues/665.
## Rationale for this change
## What changes are included in this PR?
## How are these changes tested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#issuecomment-2372030946
cc @andygrove @comphead @huaxingao @mbutrovich @parthchandra @viirya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#issuecomment-2372269111
Closes 585
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773863372
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -873,12 +873,18 @@ class CometSparkSessionExtensions
LessThan(normalizeNaNAndZero(left), normalizeNaNAndZero(right))
case LessThanOrEqual(left, right) =>
LessThanOrEqual(normalizeNaNAndZero(left), normalizeNaNAndZero(right))
+ case Divide(left, right, evalMode) =>
+ Divide(left, normalizeNaNAndZero(right), evalMode)
+ case Remainder(left, right, evalMode) =>
+ Remainder(left, normalizeNaNAndZero(right), evalMode)
}
}
def normalizeNaNAndZero(expr: Expression): Expression = {
expr match {
case _: KnownFloatingPointNormalized => expr
+ case FloatLiteral(f) if !f.equals(-0.0f) => expr
Review 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
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -873,12 +873,18 @@ class CometSparkSessionExtensions
LessThan(normalizeNaNAndZero(left), normalizeNaNAndZero(right))
case LessThanOrEqual(left, right) =>
LessThanOrEqual(normalizeNaNAndZero(left), normalizeNaNAndZero(right))
+ case Divide(left, right, evalMode) =>
+ Divide(left, normalizeNaNAndZero(right), evalMode)
+ case Remainder(left, right, evalMode) =>
+ Remainder(left, normalizeNaNAndZero(right), evalMode)
}
}
def normalizeNaNAndZero(expr: Expression): Expression = {
expr match {
case _: KnownFloatingPointNormalized => expr
+ case FloatLiteral(f) if !f.equals(-0.0f) => expr
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura merged PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773883891
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -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
Review Comment:
Is this related?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773876030
##########
docs/source/contributor-guide/development.md:
##########
@@ -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
Review Comment:
Why add `-nsu`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773938071
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -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
Review Comment:
Indirectly related, this will avoid to put `EqualTo` for non zero literals for divisions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773922597
##########
docs/source/contributor-guide/development.md:
##########
@@ -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
Review Comment:
It helps for faster turnaround
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773854196
##########
docs/source/contributor-guide/development.md:
##########
@@ -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
Review Comment:
Thanks, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#discussion_r1773845063
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -873,12 +873,18 @@ class CometSparkSessionExtensions
LessThan(normalizeNaNAndZero(left), normalizeNaNAndZero(right))
case LessThanOrEqual(left, right) =>
LessThanOrEqual(normalizeNaNAndZero(left), normalizeNaNAndZero(right))
+ case Divide(left, right, evalMode) =>
+ Divide(left, normalizeNaNAndZero(right), evalMode)
+ case Remainder(left, right, evalMode) =>
+ Remainder(left, normalizeNaNAndZero(right), evalMode)
}
}
def normalizeNaNAndZero(expr: Expression): Expression = {
expr match {
case _: KnownFloatingPointNormalized => expr
+ case FloatLiteral(f) if !f.equals(-0.0f) => expr
Review 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?
##########
docs/source/contributor-guide/development.md:
##########
@@ -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
Review Comment:
Since you added these steps to the testing commands, does it makes sense to duplicate them below for the generation commands?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: div and rem by negative zero [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #960:
URL: https://github.com/apache/datafusion-comet/pull/960#issuecomment-2372289365
Merged Thank you @mbutrovich @viirya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org