sunchao commented on code in PR #24767:
URL: https://github.com/apache/datafusion/pull/24767#discussion_r3963781756
##########
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 children during scalar compaction too
The constructor workaround preserves a dictionary child's unused null entry,
so the resulting non-nullable list still fails in an ordinary consumer. With
the input from `collect_list_handles_dictionary_with_unused_null` (keys `[0,
0]`, dictionary `[Some("a"), None]`), the source path for
`acc.evaluate()?.compacted()` on the `collect_list` wrapper accumulator reaches
[`compact_view_buffers`'s
`ListArray::new`](https://github.com/apache/datafusion/blob/ddbadd312cee1dd117c4324d6e10305993fbd0dd/datafusion/common/src/scalar/mod.rs#L5011).
[Arrow's dictionary
copy](https://github.com/apache/arrow-rs/blob/59.2.0/arrow-data/src/transform/mod.rs#L614)
retains the unused null, so that constructor sees a non-nullable field and a
conservatively nullable child and panics.
This also affects composition: [ordered `array_agg` compacts each retained
input
scalar](https://github.com/apache/datafusion/blob/ddbadd312cee1dd117c4324d6e10305993fbd0dd/datafusion/functions-aggregate/src/array_agg.rs#L1315),
and therefore fails if it receives this inner `collect_list` result. The
pre-PR nullable result passed this constructor's nullability check.
Please preserve support for these valid encoded children through compaction
as well, and extend the dictionary regression to compact the result or feed it
into an ordered aggregate. This finding is source-traced against Arrow 59.2.0;
I have not executed the proposed reproducer locally.
##########
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)?;
Review Comment:
### [P2] Normalize retracted inputs alongside updated inputs
This changes the type received by the inner accumulator, while
`retract_batch` still forwards the original runtime array at lines 283–284.
Reuse the valid input from `accumulator_state_and_output_preserve_nested_type`:
declared `Struct(required: Int32, nullable=false)`, runtime child
`nullable=true`, and actual values `[1, 2]`. After updating a `collect_set`
accumulator, retracting the first row of that same runtime array reaches
[`RowConverter::append`](https://github.com/apache/datafusion/blob/ddbadd312cee1dd117c4324d6e10305993fbd0dd/datafusion/functions-aggregate/src/array_agg.rs#L1110)
with a different nested type. Arrow's [schema
check](https://github.com/apache/arrow-rs/blob/59.2.0/arrow-row/src/lib.rs#L1069)
compares nested nullability and returns `RowConverter column schema mismatch`.
Sliding windows pass slices of the original input to both operations, so the
valid schema mismatch handled by the update path still causes a query error
when rows leave the frame. Please apply the same normalization in
`retract_batch`, and extend the nested-type test with update → retract →
evaluate.
This is source-traced; I have not executed the reproducer locally or
established that the complete SQL query succeeded on the pre-PR revision.
--
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]
