andygrove opened a new issue, #5024: URL: https://github.com/apache/datafusion-comet/issues/5024
Follow-up to address review feedback from [PR #4720](https://github.com/apache/datafusion-comet/pull/4720) (native `collect_list` / `array_agg`): https://github.com/apache/datafusion-comet/pull/4720#issuecomment-5071730068 The PR merges the core functionality; the items below are docs and test-coverage hardening deferred to a follow-on so they don't block the feature. All were flagged Medium severity. ### 1. Add a `collect_set` audit entry `docs/source/contributor-guide/expression-audits/agg_funcs.md` documents `## collect_list` but has no `collect_set` entry, even though PR #4720 modifies `CometCollectSet` semantically (new RESPECT NULLS branch via `CometCollectShim.ignoreNulls(CollectSet)`, name-matched in `hasNativeArrayBufferAgg`). Add a `## collect_set` section alongside `## collect_list` (covering Spark 3.4.3 / 3.5.8 / 4.0.1 / 4.1.1, the `SparkCollectSet` delegation, the buffer-shape mismatch, and the `NaN`-dedup `Incompatible` reason on float/double), and replace the "same pattern already in use for collect_set" prose in the `collect_list` entry with a link to the new section. ### 2. Regression-cover the split-execution cascade `spark/src/test/scala/org/apache/comet/exec/CometAggregateSuite.scala` — neither new `collect_list` test toggles `COMET_ENABLE_FINAL_HASH_AGGREGATE` / `COMET_ENABLE_PARTIAL_HASH_AGGREGATE`, so the `hasNativeArrayBufferAgg` + `tagUnsafePartialAggregates` path is untested. Mirror the existing `"mixed engine sum/avg"` tests (lines ~211/224) with: - `mixed engine collect_list: Comet partial + Spark final matches Spark` - `mixed engine collect_list: Spark partial + Comet final matches Spark` (the fallback case where Comet cannot read Spark's `BinaryType` buffer — `adjustOutputForNativeState` would misinterpret Binary state as an Array if it ran natively) ### 3. Assert the fallback reason in the distinct-combined test In the new `"collect_list/collect_set combined with distinct aggregate falls back safely"` test, `checkSparkAnswer` doesn't verify anything ran/fell-back as expected. Use `checkSparkAnswerAndFallbackReason` (`CometTestBase.scala:296`) to assert the exact reason so the #4724 guard is actually protected: ``` Partial aggregate disabled: part of a multi-stage CollectList/CollectSet aggregate whose intermediate buffer cannot round-trip in Comet (issue #4724) ``` Apply to both the `collect_list` and `collect_set` variants, and to both the LocalTableScan and Parquet branches. ### 4. Extend SQL fixture datatype coverage `spark/src/test/resources/sql-tests/expressions/aggregate/collect_list.sql` exercises `timestamp` but not `timestamp_ntz` (which is in `QueryPlanSerde.supportedDataType`, so it flows through the native `SparkCollectList` accumulator — an untested native path), and no year-month / day-time interval types (required by the "every datatype Spark accepts, NULLs in every column" rule; these exercise the Spark fallback). Append `timestamp_ntz` and ANSI interval blocks (both Spark 3.4+, already the file's floor). Gate the interval block with `-- MinSparkVersion: 3.4` if it can't persist to Parquet on some target versions. -- 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]
