Skip to content

Follow-up: docs and test-coverage hardening for native collect_list / array_agg (#4720) #5024

Description

@andygrove

Follow-up to address review feedback from PR #4720 (native collect_list / array_agg): #4720 (comment)

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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions