andygrove commented on code in PR #5166:
URL: https://github.com/apache/datafusion-comet/pull/5166#discussion_r3732293876
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -861,7 +872,12 @@ object CometCollectSet extends
CometAggregateExpressionSerde[CollectSet] {
inputs: Seq[Attribute],
binding: Boolean,
conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
- val child = expr.children.head
+ val child = aggExpr.mode match {
+ case Partial | Complete if isSpark42Plus =>
+ CometExecUtils.normalizeFloatingNumbers(expr.children.head)
Review Comment:
One consequence of reusing `normalize` that I would like called out
somewhere.
For any array-typed child it produces
`KnownFloatingPointNormalized(ArrayTransform(expr, lambda))`, and
`CometArrayTransform` is a `CometCodegenDispatch` rather than a native serde.
So on 4.2+, `collect_set` over `array<float>`, `array<double>`,
`array<struct<v:double>>` and `struct<a:array<double>>` now compiles Spark's
`doGenCode` into a per-batch JVM kernel instead of running natively end to end.
Before this PR those ran purely natively, just with the wrong answer, so this
is the right trade. The scalar path is unaffected, since it maps onto the
native `NormalizeNaNAndZero`. The plain struct path is unaffected too, since
`If`, `IsNull`, `CreateNamedStruct` and `GetStructField` all have native
serdes. It is specifically the array shapes.
Two follow-ups. Could the PR description mention this so the execution-mode
change is on the record? And would you file an issue to teach the native
`NormalizeNaNAndZero` in
`native/spark-expr/src/math_funcs/internal/normalize_nan.rs` to recurse into
`List` and `Struct`, so the serde can emit a single native node for the nested
shapes?
Worth noting in that issue that with
`spark.comet.exec.scalaUDF.codegen.enabled=false` the `ArrayTransform`
serializes to `None`, which makes `convert` return `None` and the whole
aggregate fall back to Spark. That degrades safely, which is good, but it is
slower than it needs to be.
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -844,12 +853,14 @@ object CometCollectSet extends
CometAggregateExpressionSerde[CollectSet] {
// analysis time, and CometCollectShim.ignoreNulls hardcodes true, making
this a no-op.
if (!CometCollectShim.ignoreNulls(expr)) {
Unsupported(Some("collect_set with RESPECT NULLS (ignoreNulls = false)
is not supported"))
Review Comment:
Now that `getIncompatibleReasons()` returns `Nil` on 4.2+, there is nothing
left to render on the Spark 4.2 compatibility page, so `collect_set` will show
up there as fully supported with no caveats at all. But `RESPECT NULLS` still
falls back on this line.
Could you add the matching `getUnsupportedReasons()`, gated the same way so
it does not appear on the 3.4 through 4.1 pages where the field does not exist?
```scala
override def getUnsupportedReasons(): Seq[String] =
if (isSpark42Plus) {
Seq("`collect_set` with `RESPECT NULLS` falls back to Spark, since the
native " +
"implementation always drops null inputs.")
} else {
Nil
}
```
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -861,7 +872,12 @@ object CometCollectSet extends
CometAggregateExpressionSerde[CollectSet] {
inputs: Seq[Attribute],
binding: Boolean,
conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
- val child = expr.children.head
+ val child = aggExpr.mode match {
+ case Partial | Complete if isSpark42Plus =>
Review Comment:
Could you say a bit about why the normalization is gated on `isSpark42Plus`?
Normalizing NaN is a no-op for Comet on every version, since the native set
already keys on `to_bits()` and every NaN Spark can hand it is canonical. So
dropping the gate would change exactly one thing on 3.4 through 4.1: `-0.0` and
`0.0` would collapse to a single entry, which is what Spark does on those
versions. That is precisely the divergence your new `getIncompatibleReasons()`
text documents. The NaN difference would remain and `collect_set` would stay
`Incompatible` there, but users would be one divergence away instead of two.
The cost is that array-typed children would start paying the codegen
dispatch from my other comment on the older versions as well, so I can see this
going either way. If you would rather not take it on here, a tracking issue
linked from this PR works. I would just like the reasoning captured somewhere
rather than left implicit in the gate.
--
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]
