fix: RESPECT NULLS for Spark collect_list function#16933
Open
yaooqinn wants to merge 2 commits intofacebookincubator:mainfrom
Open
fix: RESPECT NULLS for Spark collect_list function#16933yaooqinn wants to merge 2 commits intofacebookincubator:mainfrom
yaooqinn wants to merge 2 commits intofacebookincubator:mainfrom
Conversation
yaooqinn
requested review from
bdice,
devavret,
jinchengchenghh,
karthikeyann,
mhaseeb123,
rui-mo and
zhli1142015
as code owners
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
Collaborator
There was a problem hiding this comment.
Please move them to a separate PR
yaooqinn
force-pushed
the
feat/spark-collect-list-respect-nulls
branch
from
12a2fca to
8aae7ec
Compare
Contributor
Author
|
Rebased on latest main — removed the unrelated cudf changes from the PR diff. Thanks @jinchengchenghh! |
| std::vector<DecodedVector> inputDecoded_; | ||
| DecodedVector intermediateDecoded_; | ||
|
|
||
| protected: |
Collaborator
There was a problem hiding this comment.
The member order should be protected and then private
| @@ -48,8 +49,9 @@ class CollectListAggregate { | |||
Collaborator
There was a problem hiding this comment.
Remove the initialize function
Remove kSparkCollectListIgnoreNulls QueryConfig and replace with a constant boolean argument, matching the collect_set pattern from facebookincubator#16416. Closes facebookincubator#16839 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yaooqinn
force-pushed
the
feat/spark-collect-list-respect-nulls
branch
from
8aae7ec to
a9de448
Compare
Contributor
Author
|
Addressed comments:
All 8 tests pass. Thanks @jinchengchenghh! |
rui-mo
reviewed
Mar 26, 2026
Collaborator
rui-mo
left a comment
rui-mo
left a comment
There was a problem hiding this comment.
Hi @yaooqinn, could you please open a corresponding PR for collect_set and collect_list in Gluten to verify their functionality? We are currently noticing the fallback issue below, and I suppose compatibility code needs to be added due to the signature change.
E20260323 21:52:57.248056 1774 Exceptions.h:87] Line: /work/cpp/velox/substrait/SubstraitToVeloxPlan.cc:290, Function:toAggregationFunctionName, Expression: signatures.has_value() && signatures.value().size() > 0 Cannot find function signature for collect_set_merge_extract_array_row_VARCHAR_BIGINT_BIGINT_endrow in final aggregation step., Source: RUNTIME, ErrorCode: INVALID_STATE
21:52:57.251 WARN org.apache.spark.sql.execution.GlutenFallbackReporter: Validation failed for plan: SortAggregate[QueryId=405], due to:
- Native validation failed:
|- Validation failed due to exception caught at file:SubstraitToVeloxPlanValidator.cc line:1450 function:validate, thrown from file:SubstraitToVeloxPlan.cc line:290 function:toAggregationFunctionName, reason:Cannot find function signature for collect_set_merge_extract_array_row_VARCHAR_BIGINT_BIGINT_endrow in final aggregation step.
rui-mo
changed the title
rui-mo
changed the title
rui-mo
changed the title
Contributor
Author
|
Created Gluten PR apache/gluten#11837 to test compatibility. It adds |
zhli1142015
reviewed
Mar 27, 2026
Collaborator
zhli1142015
left a comment
zhli1142015
left a comment
There was a problem hiding this comment.
Nice clean follow-up to the collect_set RESPECT NULLS pattern (#16416). The approach is consistent and the config removal is thorough. A few minor items — mainly a stale comment referencing the removed config, and a suggestion about the fn_ visibility change in SimpleAggregateAdapter.
| @@ -44,14 +45,6 @@ class CollectListAggregate { | |||
| // aggregation uses the accumulator path, which correctly respects the config. | |||
Collaborator
There was a problem hiding this comment.
This comment references "config" twice, but ignoreNulls_ is no longer read from QueryConfig — it now comes from the constant boolean argument via setConstantInputs(). Please update the wording, e.g.:
// NOTE: toIntermediate() was intentionally removed because it is static and
// cannot access the runtime ignoreNulls_ flag. Without it, partial
// aggregation uses the accumulator path, which correctly respects the flag.| // Velox registers a 2-arg collect_set(T, boolean) signature that Spark | ||
| // doesn't support. The fuzzer may pick this signature and fail. | ||
| "collect_set", | ||
| // Same as collect_set — 2-arg signature not supported by Spark. |
Collaborator
There was a problem hiding this comment.
Nit: "2-arg signature not supported by Spark" is slightly misleading — Spark 4.0+ does support RESPECT NULLS / IGNORE NULLS for collect_list (SPARK-55256). The real reason for skipping is that the fuzzer can't generate the constant boolean argument. Consider:
// Fuzzer may pick the 2-arg (T, boolean) signature which requires
// a constant boolean that the fuzzer cannot generate.
"collect_list",Same applies to the collect_set comment above.
| } | ||
| } | ||
|
|
||
| protected: |
Collaborator
There was a problem hiding this comment.
Making fn_ protected exposes the raw unique_ptr to all SimpleAggregateAdapter subclasses across the codebase. Currently only CollectListAdapter needs it. Would a protected accessor like FUNC& fn() { return *fn_; } be a tighter API contract? That way subclasses can access the function object without being able to reset/move the unique_ptr itself.
Not a blocker — just a suggestion for encapsulation.
| {"spark_collect_list(c0)"}, | ||
| expectedResult, | ||
| makeConfig(false)); | ||
| testAggregations({input}, {}, {"spark_collect_list(c0, false)"}, {expected}); |
Collaborator
There was a problem hiding this comment.
Consider adding a test that verifies the constant boolean false (RESPECT NULLS) works correctly through partial → intermediate → final aggregation stages. testAggregations() does cover multiple modes internally, but an explicit streaming/split test would increase confidence that setConstantInputs() propagates correctly across stages.
Build Impact AnalysisSelective Build Targets (building these covers all 407 affected)Total affected: 407/556 targets
Affected targets (407)Directly changed (296)
Transitively affected (111)
Fast path • Graph from main@4a966b2effd240f9d9f43e0b9305c0bdd7cd6b39 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #16416 (collect_set RESPECT NULLS). Applies the same pattern to
collect_list:kSparkCollectListIgnoreNullsQueryConfigcollect_list(x [, ignoreNulls])signature with constant booleansetConstantInputsviaCollectListAdaptersubclass ofSimpleAggregateAdapterfn_protected inSimpleAggregateAdapterto enable subclass accessChanges
CollectListAggregate.cpp: Replace config-based with constant-arg approachQueryConfig.h: RemovekSparkCollectListIgnoreNullsSimpleAggregateAdapter.h: Makefn_protectedaggregate.rst: Update doc formatSparkAggregationFuzzerTest.cpp: Add to skipFunctionsTesting
All 8 CollectList tests pass.
Closes #16839