Preserve input field nullability in ArrayAgg return field #19868
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR attempts to preserve the input field's nullability in the ArrayAgg aggregate function's return field. When the input field is non-nullable, the result field would be marked as non-nullable; when the input is nullable, the result field would remain nullable.
Changes:
- Added a
return_fieldmethod override to theArrayAggimplementation that sets the result field's nullability to match the input field's nullability - Added a unit test to verify that the schema metadata correctly reflects the input field's nullability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GaneshPatil7517
force-pushed
the
feature/arrayagg-preserve-nullability
branch
from
668ce8c to
b833445
Compare
GaneshPatil7517
force-pushed
the
feature/arrayagg-preserve-nullability
branch
from
114f5bc to
2b2a41b
Compare
Jefffrey
reviewed
Jan 17, 2026
- Override return_field() to preserve input field nullability in list elements - Add input_nullable field to all ArrayAgg accumulators - List itself remains nullable (NULL for empty groups) - Inner elements nullability now matches input field nullability
GaneshPatil7517
force-pushed
the
feature/arrayagg-preserve-nullability
branch
from
024785b to
24e57cb
Compare
- Separate state() from evaluate() in all three accumulators - state() always uses nullable=true for inner elements (matches state_fields()) - evaluate() uses input_nullable to preserve nullability for final output - Fix clippy: use std::slice::from_ref() instead of .clone() on FieldRef
- Fix array_sort return_type() to preserve input inner field nullability - Fix clippy: use Arc::clone() instead of .clone() on FieldRef - Update unnest_with_redundant_columns snapshot for non-nullable inner
Jefffrey
reviewed
Jan 19, 2026
- Make return_type() return error since return_field() is implemented - Simplify comments in return_field() - Revert array_sort changes (unrelated to original issue) - Revert spark/collect.rs changes (UDAF return_field not fixed)
GaneshPatil7517
force-pushed
the
feature/arrayagg-preserve-nullability
branch
from
84c066f to
402927a
Compare
Contributor
Author
|
ok ill update this all |
Contributor
|
@GaneshPatil7517 Please refrain from closing a discussion when no action has been taken or no comment was left. My comments regarding the state related functions don't seem to have been addressed. |
- Update state_fields() to preserve input nullability (same as return_field) - Simplify state() to just call evaluate() since they now share the same nullability - Remove duplicate handling code in all three accumulators
GaneshPatil7517
force-pushed
the
feature/arrayagg-preserve-nullability
branch
from
fce0b8b to
f7c3206
Compare
github-actions
bot
added
the
documentation
array_sort must preserve the input list's inner field nullability to match the type returned by array_agg which now preserves input nullability.
GaneshPatil7517
force-pushed
the
feature/arrayagg-preserve-nullability
branch
from
e744abe to
4d0118d
Compare
Jefffrey
requested changes
Jan 20, 2026
Contributor
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
@GaneshPatil7517 I strongly urge you to do a self-review on PRs; I'm seeing an unrelated file be accidentally committed, as well as the array-sort changes coming back without any reason given
| } | ||
|
|
||
| fn state(&mut self) -> Result<Vec<ScalarValue>> { | ||
| if !self.is_input_pre_ordered { |
Contributor
There was a problem hiding this comment.
Why are we removing this line?
| DataType::Null => Ok(DataType::Null), | ||
| DataType::List(field) => { | ||
| Ok(DataType::new_list(field.data_type().clone(), true)) | ||
| // Preserve the inner field's nullability from input |
Contributor
There was a problem hiding this comment.
Why are these changes coming back again?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Which issue does this PR close?
Fixes #19768
Rationale for this change
Currently,
ArrayAggalways produces an output with data type: