andygrove commented on PR #5227: URL: https://github.com/apache/datafusion-comet/pull/5227#issuecomment-5257140378
*Review assisted by an LLM (Claude Code). I checked the branch out, ran the tests, and verified the claims below myself.* Thanks for taking this on. The two core fixes are right. Using `to_fields.clone()` instead of rebuilding fields with `Field::new` does preserve metadata, and switching `*from_sorted` to `*to_sorted` is clearly correct. Moving to `try_new` is a real improvement over `new` as well. What I ran locally on the branch: - The 16 new Rust tests pass. - `cargo clippy --all-targets` and `cargo fmt --check` are clean. - The three existing map cast tests in `CometCastSuite` pass. - I added a scratch test for a sliced input through the rename-only fast path and it passes too. A few things I would like to resolve before merge. ### The sorted rejection Issue #5097 asked for the target `sorted` flag to be honored, and this does that. The rejection branch at `cast_map_to_map` is separate policy on top of that, and it turns what used to be wrong metadata into a runtime `Internal` error. The code comment points to the PR description for planner reachability, but I could not find that discussion there. When I traced it, `native/core/src/execution/serde.rs:151` hardcodes `ArrowDataType::Map(Arc::new(struct_field), false)` for every map type built from protobuf. `native/core/src/execution/planner.rs:200` propagates the flag unchanged. The Parquet reader derives `sorted` from the requested arrow type, which comes from that same `false`-producing path. And `spark_map_sort` deliberately preserves the input flag rather than setting it true. That suggests no Comet plan ever asks for a `sorted = true` map target. If that is right, would it be simpler to drop the rejection and keep just the `*to_sorted` fix? Five of the sixteen tests exercise a branch that cannot fire. If there is a path I missed, could you add it to the PR description? That would also make the case that a hard error is the behavior we want here. ### Cast options in the fast path The fast path passes the static `CAST_OPTIONS`, which hardcodes `safe: true`. A little above, `cast_array` builds `native_cast_options` specifically so that ANSI mode gets `safe: false`. The fast path does not transform any values today, so this is inert. But if the condition ever widens it would silently swallow ANSI errors. Could you use `native_cast_options` here, or add a comment explaining why the static is fine? ### Test coverage `test_cast_map_to_map_sliced` casts Int32 to Int64, so it goes through the hand-built path rather than the fast path. Since there are now two independent implementations selected by a condition, it might be worth adding a sliced test where the value type is unchanged so the arrow delegation is covered too. I tried it locally and it passes, so this is about pinning the behavior rather than a suspected bug. Along the same lines, a test that casts the same input through both paths and compares results would guard against the two drifting. ### One thing I checked that is fine Casting `Map<Utf8, Int32>` to `Map<Int32, Int32>` with a key like `"abc"` now returns `Found unmasked nulls for non-nullable StructArray field "key"` from `StructArray::try_new`. Before this PR the same input would have panicked in `StructArray::new`, so this is strictly better. It is also unreachable from a Spark plan, since Spark's `canCast` guards map casts with `(!forceNullable(fromKey, toKey))` and `forceNullable` is true for any string source, so the analyzer rejects it first. I confirmed that. Separately, Comet's `CometCast.isSupported` map arm only recurses into key and value support and does not replicate that `forceNullable` guard. Harmless today because the analyzer runs first, but worth a tracking issue so the two do not drift. Happy to file that if you would rather keep it out of this PR. ### CI There are no CI results on this yet and the branch is 35 commits behind main. Could you rebase so a full run can be triggered? My local runs only covered the Rust tests and the three map cast suites. -- 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]
