zhuqi-lucas commented on PR #23523: URL: https://github.com/apache/datafusion/pull/23523#issuecomment-4969872767
Thanks for the thorough review @Rich-T-kid and @copilot-pull-request-reviewer! Addressed everything in [132905ebf](https://github.com/apache/datafusion/pull/23523/commits/132905ebf): **Applied changes** - `row.rs:273` — corrected the doc: `RowConverter` flattens dict/REE at `append()` (during row encoding), not "on the way out". - Copilot `row_backed.rs:123` — real bug. `encode_array_if_necessary` only recursed through `Struct` / `List` / `RunEndEncoded`, so a nested key like `List<Dict<Int32,Utf8>>` emitted with mismatched data type. Extended the walk to also cover `FixedSizeList`, `LargeList`, and `Map`. Added a regression test `build_preserves_list_of_dictionary_schema` that fails without the recursion. - `row.rs:278` — renamed `dictionary_encode_if_necessary` → `encode_array_if_necessary` (it also handles REE and now nested containers around them). - `mod.rs:1414` — expanded the `canonical_grouping` doc to spell out that `GroupValues::intern` deliberately does not fix the order of fresh group-ids. - `row_backed.rs:36` — accepted the suggested rewording (`dragging both columns onto GroupValuesRows`). **Answers** - **`mod.rs:936` — why Float16 / Decimal256 not supported:** `make_group_column` doesn't have a type-specialized `PrimitiveGroupValueBuilder` branch for them (they'd need `Float16Type` / `Decimal256Type` wired through the same pattern as `Float32/Int32/etc`). We could alternatively route them through `RowsGroupColumn` by relaxing the `is_nested()` gate, but I kept this PR scoped to nested types so the biconditional `group_column_supported_type ⇔ make_group_column` invariant stays clean. Happy to file a follow-up either way. - **`row_backed.rs:196` — is `emit(n > num_rows)` guaranteed never to happen?** Yes. In `order/mod.rs:80` DataFusion always calls `EmitTo::First(n.min(max))`, and every existing `GroupColumn::take_n` impl (`bytes.rs`, `bytes_view.rs`, `boolean.rs`, `primitive.rs`) has the same `debug_assert!(self.len() >= n)`. So the assert is defensive documentation of the caller-side invariant, not a runtime check for a case we expect to hit. - **`row.rs:275` — specialized `GroupColumn` for `Dict`/`REE`:** Agreed this is the ideal end state (skips the row-encode → cast round-trip). Kept as a follow-up so this PR stays focused on the "keep mixed nested schemas on the column-wise path" story. - **`row_backed.rs:118` — arrow-rs #10275 / #10319 FYI:** Thanks for the pointer, will track those. They don't block this PR since arrow's current API is enough for what we're doing, but the future improvements would likely simplify `encode_array_if_necessary`. **Benchmarks** Requesting numbers now — see next comment. -- 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]
