zhuqi-lucas commented on code in PR #23523:
URL: https://github.com/apache/datafusion/pull/23523#discussion_r3643991275
##########
datafusion/physical-plan/src/aggregates/group_values/row.rs:
##########
@@ -294,13 +304,55 @@ fn dictionary_encode_if_necessary(
Ok(Arc::new(ListArray::try_new(
Arc::<arrow::datatypes::Field>::clone(expected_field),
list.offsets().clone(),
- dictionary_encode_if_necessary(
- list.values(),
- expected_field.data_type(),
- )?,
+ encode_array_if_necessary(list.values(),
expected_field.data_type())?,
list.nulls().cloned(),
)?))
}
+ (DataType::LargeList(expected_field), &DataType::LargeList(_)) => {
Review Comment:
Thanks @kosiew — great catch, and thanks for the clean reproducer!
Fixed in d30bf6e6b:
- Added `ListView` / `LargeListView` reconstruction arms to
`encode_array_if_necessary` (same shape as the `List` arms, plus the `sizes`
buffer view-lists carry).
- Added round-trip tests covering `build`, `take_n`, and group-identity
across distinct dictionary key mappings — all red without the fix (failing with
exactly the flattened-type mismatch you reported), green with it.
- Added the TODO on the FixedSizeList guard pointing at
apache/arrow-rs#10413 / apache/arrow-rs#10414 (merged upstream 2026-07-24, not
yet released) with the removal condition.
- Reworded the `supports_type` doc — you're right that "round-trip cleanly"
was inaccurate; it now says these containers decode without panicking but with
a flattened child that `build` / `take_n` must re-encode.
Longer term: this is the second bug of the same class (arrow-rs#10413 being
the first), and the root cause is that arrow-row's decode output type can
differ from the encode input type with no API to query the mapping. I've filed
apache/arrow-rs#10427 proposing a `decoded_type(&DataType) -> DataType` API
plus a property test asserting `decode(encode(x)).data_type() ==
decoded_type(x)` for every supported shape — that invariant would have caught
both bugs automatically, and lets code like this compute round-trip
expectations mechanically instead of mirroring decoder internals.
--
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]
