goutamadwant commented on PR #24767: URL: https://github.com/apache/datafusion/pull/24767#issuecomment-5466208613
> ### [P2] Handle encoded-array nullability before rebuilding the list > @goutamadwant The new reconstruction at [collect.rs:96–98](https://github.com/apache/datafusion/blob/e3f28e6153e03b807ba636a24b75f417cd3baaa0/datafusion/spark/src/function/aggregate/collect.rs#L96-L98) panics for valid encoded input even when every collected element is logically non-null. > > For example, construct a `Dictionary<Int8, Utf8>` with keys `[0, 0]` and dictionary values `[Some("a"), None]`, using its actual datatype as the aggregate input type. The array passes `ArrayData::validate_full()` and both input rows are `"a"`. Its `logical_null_count()` is zero, but Arrow 59.2's conservative `DictionaryArray::is_nullable()` is true because the dictionary retains an unused null entry. > > The existing logical-null filtering and single-array concatenation retain that entry. The matching-type fast path then reuses the child array, while `.with_field(field)` makes the list item non-nullable. `ListArray::new` rejects the child's `is_nullable()` and panics with: > > ``` > InvalidArgumentError("Non-nullable field of ListArray \"item\" cannot contain nulls") > ``` > > I reproduced this at `e3f28e6` through `evaluate()`, partial `state()`, and public SQL (`SELECT collect_list(x) FROM dictionary_input` after registering the batch and UDAF). The identical three probes pass with merge-base `c4910e0` production code and return `["a", "a"]`. > > Additional head/base controls confirm the same failure for `collect_list` over sparse unions and sliced run-end-encoded arrays, and for `collect_set` over sparse unions. The expanded matrix has 7 head panics versus all 11 cases passing on the merge base; dictionary/run-end-encoded `collect_set` controls pass, so those are not claimed as affected. > > Please reconcile encoded-child nullability with Arrow's construction requirements while preserving Spark's non-nullable element contract, and add regression coverage for both partial state and final output. Merely making the constructor fallible would avoid the panic but still reject these valid inputs. @sunchao addressed this in `b8932a5d8`. The fix now handles Arrow's conservative encoded-array nullability in `SingleRowListArrayBuilder`: for a non-nullable list field, it checks the exact logical null count and preserves the encoded child buffers and datatype when there are no logical nulls. This keeps Spark's `containsNull = false` contract and avoids the panic without decoding the values or making the field nullable. Let me know if you have any other comments or suggestions! -- 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]
