kosiew commented on code in PR #23523:
URL: https://github.com/apache/datafusion/pull/23523#discussion_r3643717742
##########
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:
Nice improvement adding recursive reconstruction for `List`, `LargeList`,
`FixedSizeList`, and `Map`. One remaining gap is that `ListView` and
`LargeListView` are still missing, even though `RowsGroupColumn::supports_type`
now explicitly accepts dictionary-containing view-list types.
Arrow 59.1.0's `decode_list_view` applies `corrected_type`, so
`ListView<Dictionary<Int32, Utf8>>` decodes as `ListView<Utf8>`.
`RowsGroupColumn::build` and `take_n` then hit the helper's fallback branch and
return that array unchanged, so the output no longer matches `self.output_type`.
I reproduced this end-to-end in `datafusion-cli`:
```sql
SELECT arrow_cast(a, 'ListView(Dictionary(Int32, Utf8))') AS k, COUNT(*)
FROM (VALUES (['a','b']), (['a','b']), (['c'])) AS t(a)
GROUP BY k;
```
which fails with:
```text
expected ListView(Dictionary(Int32, Utf8)) but found ListView(Utf8)
```
`LargeListView(Dictionary(Int32, Utf8))` fails the same way.
Could we either add recursive reconstruction for `ListViewArray` and
`LargeListViewArray`, preserving offsets, sizes, nulls, and the expected child
field, or reject dictionary-containing view-list shapes in `supports_type`? It
would also be good to add append plus `build` and `take_n` round-trip tests,
since the current tests only verify acceptance.
That should fully address the original nested re-encoding concern. Right now
the `supports_type` documentation also says these view-list forms \"round-trip
cleanly\", which is no longer accurate for dictionary children.
--
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]
