sunchao commented on code in PR #24767:
URL: https://github.com/apache/datafusion/pull/24767#discussion_r3963773684
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -603,9 +601,31 @@ impl SingleRowListArrayBuilder {
/// Build a single element [`LargeListArray`]
pub fn build_large_list_array(self) -> LargeListArray {
+ self.build_generic_list_array()
+ }
+
+ fn build_generic_list_array<OffsetSize: OffsetSizeTrait>(
+ self,
+ ) -> GenericListArray<OffsetSize> {
let (field, arr) = self.into_field_and_arr();
let offsets = OffsetBuffer::from_lengths([arr.len()]);
- LargeListArray::new(field, offsets, arr, None)
+
+ // `is_nullable` is conservative for encoded arrays and can be true
+ // even when the array contains no logical nulls. In that case the
+ // generic constructor rejects a valid non-nullable list child.
+ if !field.is_nullable() && arr.is_nullable() &&
arr.logical_null_count() == 0 {
+ let data = ArrayData::builder(
+ GenericListArray::<OffsetSize>::DATA_TYPE_CONSTRUCTOR(field),
+ )
+ .len(1)
+ .add_buffer(offsets.into_inner().into_inner())
+ .add_child_data(arr.to_data())
+ .build()
+ .expect("single-row list array should contain valid data");
+ return GenericListArray::from(data);
Review Comment:
### [P2] Handle encoded nullability in downstream list operations
This preserves a dictionary child containing unused null entries. The
resulting list passes `ArrayData::validate_full()`, but Arrow's `concat` and
`take` constructors still reject its non-nullable element field.
I reproduced this with `Dictionary<Int8, Utf8>` keys `[0, 0, 0, 0]`,
dictionary values `[Some("a"), None]`, group keys `[0, 0, 1, 1]`, and row IDs
`[0, 1, 2, 3]`. Both queries fail on this head:
```sql
SELECT collect_list(x) FROM t GROUP BY g ORDER BY g;
SELECT collect_list(x) OVER (
ORDER BY id ROWS BETWEEN 1 PRECEDING AND CURRENT ROW
) FROM t ORDER BY id;
```
The error is `Non-nullable field of ListArray "item" cannot contain nulls`.
Grouped/window output concatenates list scalars through
`ScalarValue::iter_to_array`, reaching the conservative constructor again. Both
queries pass with merge-base production code; the corresponding `collect_set`
controls pass on both versions.
Please reconcile encoded-child nullability beyond this constructor and add
grouped/window regression coverage. The existing global-aggregation test
produces only one scalar and misses this path.
##########
datafusion/spark/src/function/aggregate/collect.rs:
##########
@@ -180,27 +234,49 @@ impl<T: Accumulator> NullToEmptyListAccumulator<T> {
pub fn new(inner: T, list_type: DataType) -> Self {
Self { inner, list_type }
}
+
+ fn normalize_input(&self, value: &ArrayRef) -> Result<ArrayRef> {
+ let DataType::List(field) = &self.list_type else {
+ return internal_err!(
+ "collect_list/collect_set expected List return type, got {:?}",
+ self.list_type
+ );
+ };
+ if value.data_type() == field.data_type() {
+ Ok(Arc::clone(value))
+ } else {
+ Ok(cast(value.as_ref(), field.data_type())?)
+ }
+ }
}
impl<T: Accumulator> Accumulator for NullToEmptyListAccumulator<T> {
fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
- self.inner.update_batch(values)
+ let [value] = values else {
+ return self.inner.update_batch(values);
+ };
+ let value = self.normalize_input(value)?;
+ self.inner.update_batch(&[value])
Review Comment:
### [P2] Normalize retractions consistently with updates
For the declared-nonnullable/runtime-nullable nested Struct scenario covered
by this PR, this update path initializes `collect_set`'s `RowConverter` with
the normalized schema. `retract_batch` still forwards the original runtime
array.
Using declared `Struct(required: Int32 non-null)` and runtime
`Struct(required: Int32 nullable)` containing `[1, 2]`, `update_batch` succeeds
but retracting the first original row now fails:
```text
RowConverter column schema mismatch, expected Struct("required": non-null
Int32) got Struct("required": Int32)
```
The identical update/retract probe passes on the merge base. Sliding windows
pass slices of the original input to both operations, so this breaks retraction
when the downstream runtime/declared schema mismatch occurs. This reproduction
is at the accumulator API; I have not established a native SQL producer of that
mismatch.
Please apply equivalent normalization during retraction and add an
update/retract round-trip to the nested-schema regression coverage.
##########
datafusion/spark/src/function/aggregate/collect.rs:
##########
@@ -180,27 +234,49 @@ impl<T: Accumulator> NullToEmptyListAccumulator<T> {
pub fn new(inner: T, list_type: DataType) -> Self {
Self { inner, list_type }
}
+
+ fn normalize_input(&self, value: &ArrayRef) -> Result<ArrayRef> {
+ let DataType::List(field) = &self.list_type else {
+ return internal_err!(
+ "collect_list/collect_set expected List return type, got {:?}",
+ self.list_type
+ );
+ };
+ if value.data_type() == field.data_type() {
+ Ok(Arc::clone(value))
+ } else {
+ Ok(cast(value.as_ref(), field.data_type())?)
Review Comment:
### [P2] Drop ignored null rows before narrowing nested types
This cast processes rows before the inner accumulator drops null inputs. In
the runtime/declared nested-nullability mismatch scenario, it can reject the
backing payload of a row that both collect aggregates must ignore.
A concrete input has declared element type `List<non-null Int32>` and
runtime type `List<nullable Int32>`, with offsets `[0, 1, 2]`, child values
`[NULL, 1]`, and validity `[false, true]`. Its logical rows are `[NULL, [1]]`;
the null list legally retains a null backing child, and the input passes
`ArrayData::validate_full()`.
Both `collect_list` and `collect_set` now fail during `update_batch` with
`Non-nullable field of ListArray "item" cannot contain nulls`. The identical
accumulator probes return `[[1]]` with merge-base production code. There is no
null in the retained nested value.
Please remove ignored null rows before narrowing nested nullability, and
cover this masked-payload case for both aggregates.
--
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]
