andygrove opened a new pull request, #4817: URL: https://github.com/apache/datafusion-comet/pull/4817
## Which issue does this PR close? Part of #3841 (implements `max_by`; `min_by` remains as a follow-on). ## Rationale for this change `max_by(x, y)` returns the value of `x` associated with the maximum value of `y`. It was previously unsupported, so any query using it fell back to Spark. There is no `max_by`/`min_by` aggregate in DataFusion or the `datafusion-spark` crate, so this adds a native implementation. ## What changes are included in this PR? - A native DataFusion aggregate `MaxMinBy` (`native/spark-expr/src/agg_funcs/max_min_by.rs`). It compares the ordering column using Arrow's row format, so any orderable fixed-length type is handled with a single Spark-consistent total order (NaN sorts largest, matching Spark). Null orderings are ignored, the value paired with the maximum ordering is returned (and may itself be null), and an all-null-ordering group yields null. The implementation is parameterized by an `is_max` flag so `min_by` is a small follow-on. - A `MaxBy` protobuf message and wiring in `planner.rs`. - A `CometMaxBy` serde registered in `QueryPlanSerde`. `getSupportLevel` restricts both the value and the ordering to fixed-length types: a variable-length or nested type (string, binary, struct) forces Spark's `SortAggregate`, which Comet does not accelerate, so those cases fall back to Spark. - A `max_by` microbenchmark group in `CometAggregateExpressionBenchmark`. - Documentation updates (expression support status, expression audit notes). The 2-argument `max_by` implementation is identical across Spark 3.4.3, 3.5.8, 4.0.1, and 4.1.1, so no version shim is needed. The 3-argument top-k form `max_by(x, y, k)` exists only on Spark master and is out of scope. This implementation was scaffolded with the `implement-comet-expression` project skill, which also ran the `audit-comet-expression` skill to drive the test-coverage pass. ## How are these changes tested? - Rust unit tests in `max_min_by.rs` covering basic max/min selection, null-ordering handling, null value at the extremum, all-null orderings, NaN ordering, empty groups, and partial/final merge equivalence. - A new SQL file test `spark/src/test/resources/sql-tests/expressions/aggregate/max_by.sql` covering global and grouped aggregation, null handling, literal arguments, negative and boundary orderings, NaN/±Infinity, decimal/date/timestamp/boolean types, multiple `max_by` in one query, and mixing with other aggregates. Each query runs through both Spark and Comet and asserts native execution. -- 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]
