Skip to content

test: docs and test-coverage hardening for native collect_list / collect_set - #5055

Open
andygrove wants to merge 2 commits into
apache:mainfrom
andygrove:feat/collect-list-followup-5024
Open

test: docs and test-coverage hardening for native collect_list / collect_set#5055
andygrove wants to merge 2 commits into
apache:mainfrom
andygrove:feat/collect-list-followup-5024

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 feat: native collect_list / array_agg aggregate #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 Support fully-native multi-stage (distinct-combined) collect_list / collect_set #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.

Follow-up to PR apache#4720. Adds the missing `collect_set` audit entry, covers
the split-execution cascade and the multi-stage fallback guard with
assertions, and extends the SQL fixture datatype coverage.

- docs: add a `## collect_set` section to the agg_funcs audit page and
  link the `collect_list` entry to it instead of referring to it in prose
- test: add mixed-engine `collect_list` tests for Comet partial + Spark
  final and Spark partial + Comet final, exercising
  `hasNativeArrayBufferAgg` and `tagUnsafePartialAggregates`
- test: assert the exact fallback reason in the distinct-combined test
  instead of only checking the answer
- test: add `timestamp_ntz` (native path) and ANSI interval (Spark
  fallback) blocks to the `collect_list` SQL fixture
@andygrove andygrove added this to the 1.1.0 milestone Jul 27, 2026
- test: assert on short discriminating fallback-reason fragments instead
  of copying the full message text from CometExecRule / operators
- test: collapse the two mixed-engine tests into one parameterized pair
  and pin the Comet aggregate count at zero, so they fail if the
  partial/final cascade regresses instead of passing on any plan shape
- test: drop the dead `ORDER BY`, which the answer helper already handles
- test: fold the timestamp_ntz fixture into the existing timestamp table
  rather than duplicating its rows, matching the file's convention
- test: pin the interval fallback with `expect_fallback` instead of
  `spark_answer_only`, so the block comment's claim is enforced
- docs: move the shared buffer-shape note under collect_list, correct the
  `adjustOutputForNativeState` owner to CometBaseAggregate, and drop the
  redundant containsNull and Spark 4.2 restatements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant