Smallfu666 commented on PR #5227: URL: https://github.com/apache/datafusion-comet/pull/5227#issuecomment-5279439364
Thanks for the review, and for checking out the branch and running it yourself. Your read is right, so I dropped the rejection branch and kept the `*to_sorted` fix, the `to_fields.clone()` metadata preservation and `try_new`. Agreed that turning wrong metadata into a runtime error is a separate call from what the issue asked, and not one worth making on a branch nothing reaches. I confirmed your four sites and then looked for a producer you had not covered. I could not find another production producer. Iceberg goes through the same `serializeDataType` and `convert_spark_types_to_arrow_schema`. Shuffle builds maps only through arrow's `MapBuilder`, which hardcodes false. The FFI import honors `MAP_KEYS_SORTED`, but Comet's only JVM side Arrow map field constructor is `Utils.scala:195`, which passes false. `make_all_fields_nullable` and the nested comparison coercion derive a map target from an input type rather than from protobuf, but both route to DataFusion's `CastExpr` rather than here and both copy the source flag anyway. The only `Map(_, true)` types left in the tree are the ones my tests hand build. One correction on the count. 2 of the 16 tests exercised the rejection, not 5, and those are the 2 I removed. The other 3 sorted tests pin the fix itself, and `test_cast_map_to_map_sorted_true_to_false_allowed` fails on the pre-fix code with `sorted=true` where `false` is expected. Cast options are now built by an `arrow_cast_options(eval_mode)` helper shared with `cast_array`, so the two call sites cannot drift. It is inert today and I checked why rather than assuming. Arrow 58.4's map cast delegates to `cast_with_options` for the key and value arrays, and the fast path requires both child data types to be unchanged, so those child casts take Arrow's same-type early return before any `safe`-dependent conversion. The sliced fast path test now asserts ANSI and legacy give byte identical output. Both coverage tests are in, and I verified with a temporary probe which branch each test actually takes rather than inferring it from the types. The new sliced test hits the fast path, `both_paths_agree` hits each branch exactly once, and the pre-existing `test_cast_map_to_map_sliced` hits the hand built path, which matches your observation. Test count stays at 16. Separately, all of this was pinned only by Rust assertions, which proves agreement with my own expectations rather than with Spark. I added map cases to `cast_complex.sql`: value casts, key casts, and the null map, empty map and null value rows, plus the `DATATYPE_MISMATCH` a key cast that could introduce nulls should raise. `CometSqlFileTestSuite` is 444 of 444 on Spark 3.5. Adding cases to an existing file does not move the suite count, so I broke the new expectation on its own and confirmed the suite goes red on that case specifically rather than assuming it ran. Nothing under `docs/source/user-guide/latest/` claims a support level for map casts, so there is no audit page to update. Rebased onto current main. CI has not started, since fork runs still need a maintainer to approve each one. If you still think the `CometCast.isSupported` divergence is worth tracking separately, please do file it. I am happy to pick it up. -- 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]
