zhuqi-lucas commented on PR #23523:
URL: https://github.com/apache/datafusion/pull/23523#issuecomment-5041558345
Thanks @kosiew — great catch. Pushed the fix in 599084666.
I dug into arrow-row 59.1.0's `list.rs` and want to add one clarification:
the panic is specific to `FixedSizeList` with a `Dictionary` leaf, not the full
set. Here's what I found:
- The generic `decode<L: GenericListArrayOrMap>` path (used by `List`,
`LargeList`, `ListView`, `LargeListView`, and `Map` in 59.1.0) does apply the
dict-flatten correction — see `list.rs:259-320`, the `corrected_type` block
rewrites the outer type to match the decoded child data.
- Only `decode_fixed_size_list` (`list.rs:378-431`) is a separate function
that skipped this pattern. It calls
`FixedSizeListArray::try_new_with_length(element_field, ...)` with the original
`element_field` (Dict) plus a decoded (native-typed) child, and the type check
fires — exactly the panic you reproduced.
So the tightening in this commit is:
- `supports_type` rejects any type that contains `FixedSizeList` with a
`Dictionary` leaf anywhere in the subtree — direct (`FSL<Dict>`), buried under
a struct (`FSL<Struct<Dict>>`), buried under a list (`FSL<List<Dict>>`), or
hidden inside an outer wrapper (`List<FSL<Dict>>`, `Struct<FSL<Dict>>`).
- `List<Dict>`, `LargeList<Dict>`, `ListView<Dict>`, `LargeListView<Dict>`,
and `Map<..., Dict>` continue to be accepted because arrow-row already
round-trips them correctly on 59.1.0.
Tests (18 total in `row_backed::tests`, 12 new):
**Rejection** (all must return `false` from `supports_type`)
- `supports_type_rejects_fixed_size_list_of_dict`
- `supports_type_rejects_fsl_with_dict_nested_in_struct`
- `supports_type_rejects_fsl_with_dict_nested_in_list`
- `supports_type_rejects_fsl_hidden_under_outer_list`
- `supports_type_rejects_fsl_hidden_under_outer_struct`
**Acceptance** (must return `true` — and the FSL cases prove the blacklist
is targeted, not blanket)
- `supports_type_accepts_fsl_of_primitive` — plain `FSL<Int32>`
- `supports_type_accepts_fsl_of_struct_without_dict`
- `supports_type_accepts_large_list_of_dict`
- `supports_type_accepts_list_view_of_dict`
- `supports_type_accepts_large_list_view_of_dict`
- `supports_type_map_agrees_with_row_converter`
**End-to-end round-trip** (must survive `vectorized_append` + `build()`
without panic and preserve the declared type)
- `build_preserves_list_of_dictionary_schema` (existing)
- `build_preserves_large_list_of_dictionary_schema` (new)
- `build_preserves_map_of_dictionary_schema` (new)
I'll also file an upstream arrow-rs issue asking for
`decode_fixed_size_list` to adopt the same `corrected_type` fix so this
blacklist can be lifted in a follow-up.
If you prefer the more conservative "reject all four (`FSL` / `LargeList` /
`ListView` / `Map`) with dict leaves" until we've verified round-trip on the
current arrow-row on your side, happy to widen the check — the round-trip tests
would then need to be gated on the shape being accepted.
--
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]
