parthchandra commented on PR #6457:
URL:
https://github.com/apache/datafusion-comet/pull/6457#issuecomment-5919313648
Notes on the float-ordering fix (matches Spark's `SQLOrderingUtil`: NaN ==
NaN, NaN is largest, `-0.0` == `0.0`):
-
**`spark/src/test/resources/sql-tests/expressions/aggregate/min_max_floating_point.sql:54`**
(the `WHERE g IN (2, 3)` query) — this result depends on which of `-0.0` /
`0.0` is read first, so it's only stable if Spark and Comet scan rows in the
same order. The `CometAggregateSuite` change forces `repartition(1)` for this
reason. Can you confirm the SQL fixture can't go order-flaky (for example if
the insert ever spans more than one file or row group, or under a parallel
partial aggregate)? A single-file guarantee or a note in the file would make it
robust.
- **`native/spark-expr/src/math_funcs/greatest_least.rs:175`** — when every
argument is scalar you set `rows = 1` and return a Scalar, which is right. Just
confirming the planner never routes a zero-argument call here; the
`exec_err!("requires at least one argument")` guard suggests it can, and
Spark's `greatest`/`least` require at least two.
- **`native/core/src/execution/planner.rs:3527`** — good that the window
path now shares `min_max_udaf` with the aggregate path. Since `is_floating()`
also matches Float16 and `SparkMinMax::accumulator` returns `internal_err` for
it, a Float16 min/max would surface as an execution error rather than a Spark
fallback. Unreachable today (Spark has no Float16), just flagging the
assumption.
- **`native/spark-expr/src/agg_funcs/min_max.rs:276`** — the
`get_unchecked_mut` on the grouped path is justified by the `GroupsAccumulator`
contract, guarded by `debug_assert!`, and matches DataFusion's own grouped max.
No change needed.
Because this touches the planner, a native aggregate, and the serde, and
removes a fallback, worth applying the matching `run-*` label or running
`dev/local-ci.sh` before it hits the merge queue.
--
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]
