andygrove opened a new pull request, #5055:
URL: https://github.com/apache/datafusion-comet/pull/5055
## Which issue does this PR close?
Closes #5024.
## Rationale for this change
PR #4720 landed the native `collect_list` / `array_agg` implementation and
deferred four docs and test-coverage items to a follow-on so they would not
block the feature. This PR addresses those items.
Two of them close real gaps in the regression net: neither `collect_list`
test toggled the partial/final hash aggregate configs, so the
`hasNativeArrayBufferAgg` + `tagUnsafePartialAggregates` split-execution path
was untested; and the distinct-combined test only compared answers, so it would
have passed even if the #4724 guard stopped firing and the query silently ran
natively.
## What changes are included in this PR?
1. **`collect_set` audit entry.**
`docs/source/contributor-guide/expression-audits/agg_funcs.md` documented
`collect_list` but not `collect_set`, even though #4720 changed
`CometCollectSet` semantically. Adds a `## collect_set` section covering Spark
3.4.3 / 3.5.8 / 4.0.1 / 4.1.1, the `SparkCollectSet` delegation, the
`BinaryType`-vs-list buffer-shape mismatch, and the `NaN`-dedup `Incompatible`
reason on float/double. The `collect_list` entry now links to it instead of
referring to it in prose.
2. **Split-execution cascade regression tests.** Two new tests in
`CometAggregateSuite`, mirroring the existing `mixed engine sum/avg` pair:
- `mixed engine collect_list: Comet partial + Spark final matches Spark`
(`COMET_ENABLE_FINAL_HASH_AGGREGATE=false`)
- `mixed engine collect_list: Spark partial + Comet final matches Spark`
(`COMET_ENABLE_PARTIAL_HASH_AGGREGATE=false`), the case where
`adjustOutputForNativeState` would misinterpret Spark's `BinaryType` buffer as
a list if the final ran natively.
3. **Fallback-reason assertions in the distinct-combined test.**
`checkSparkAnswer` is replaced with `checkSparkAnswerAndFallbackReason(s)`. The
two branches are protected by different guards, so each asserts its own:
- LocalTableScan branch: the Partial was never convertible, so
`tagUnsafePartialAggregates` does not fire and the dependent stages fall back
via the buffer-source check (`Comet aggregate that merges intermediate buffers
requires a Comet child aggregate ... Incompatible aggregate function(s):
collect_list`).
- Parquet branch: the Partial would otherwise convert, so the #4724
tagging reason (`Partial aggregate disabled: part of a multi-stage
CollectList/CollectSet aggregate whose intermediate buffer cannot round-trip in
Comet (issue #4724)`) is asserted alongside the buffer-source reason.
4. **SQL fixture datatype coverage.** `collect_list.sql` gains a
`timestamp_ntz` block (in `QueryPlanSerde.supportedDataType`, so it runs
through the native `SparkCollectList` accumulator, previously untested) and a
year-month / day-time ANSI interval block (not supported natively, so
`spark_answer_only` covers the Spark fallback). No `MinSparkVersion` gate is
needed: both work on 3.4, which is already the file's floor.
## How are these changes tested?
`CometAggregateSuite` and `CometSqlFileTestSuite` were run against both ends
of the supported range, Spark 3.4 and the default Spark 4.1, and pass on both:
```
./mvnw test -Dsuites="org.apache.comet.exec.CometAggregateSuite
collect_list" -Dtest=none
./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite collect_list"
-Dtest=none
```
The `NaN` / `-0.0` deduplication behavior asserted in the new audit entry
was verified against Spark directly rather than inferred: `collect_set` over
`(0.0, -0.0, NaN, NaN, 1.0)` returns `[0.0, 1.0, NaN, NaN]`, confirming Spark
keeps each `NaN` while collapsing `+0.0` and `-0.0`.
--
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]
