andygrove commented on code in PR #6447:
URL: https://github.com/apache/datafusion-comet/pull/6447#discussion_r4178139408
##########
spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala:
##########
@@ -763,14 +710,12 @@ case class CometExecRule(session: SparkSession)
plan
}
} else {
- val normalizedPlan = normalizePlan(plan)
-
val planWithJoinRewritten = if (CometConf.COMET_FORCE_SHJ.get()) {
- normalizedPlan.transformUp { case p =>
+ plan.transformUp { case p =>
RewriteJoin.rewrite(p)
}
} else {
- normalizedPlan
+ plan
Review Comment:
#6413 has merged, so `hash`, `xxhash64` and the shuffle partitioner
canonicalize NaN, and the main merge in b1cc9fc1e brought it in.
`nan_divisor.sql` hashes `1.0D / (-d)`, `1.0D % (-d)` and `1.0F % (-f)` with
ANSI off and on (864c2b576).
##########
spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala:
##########
@@ -826,14 +773,12 @@ case class CometExecRule(session: SparkSession)
plan
}
} else {
- val normalizedPlan = normalizePlan(plan)
-
val planWithJoinRewritten = if (CometConf.COMET_FORCE_SHJ.get()) {
- normalizedPlan.transformUp { case p =>
+ plan.transformUp { case p =>
RewriteJoin.rewrite(p)
}
} else {
- normalizedPlan
+ plan
Review Comment:
Reproduced, and `max`/`min` of a quotient computed in a projection had the
same exposure. #6457 orders floats the Spark way in `min`, `max`, `greatest`
and `least`, so rather than keep the divisor wrapper I merged its branch into
this one (0aae7f58e), and this PR lands after it. #6457 is in the merge queue;
until it merges, this diff also shows its changes, and I'll merge main again
once it does.
`nan_divisor.sql` now runs your projection, plus `least(1.0D % (-d), 0.0D)`,
`greatest(1.0F % (-f), 0.0F)` and `max`/`min` of a quotient that a `UNION ALL`
keeps in a projection below the aggregate, with ANSI off and on (36546d024).
The new queries fail on 864c2b576 and pass with #6457. A wider probe over `1.0D
/ (-d)` and `1.0D % (-d)`, covering the hashes, the extrema, `array_distinct`,
`array_union`, `sort_array`, `array_max`, comparisons, and sort and grouping
keys, now matches Spark in every case, in both ANSI modes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
