andygrove commented on PR #5227: URL: https://github.com/apache/datafusion-comet/pull/5227#issuecomment-5441713730
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is careful work. The comments citing specific arrow-array and arrow-data line numbers for why a guard is or is not reachable are unusually good, and the "intentionally not `Closes`" scope note with the reason is the right way to handle a partial fix. `test_cast_map_to_map_both_paths_agree` pinning the equivalence between the delegated and hand-built paths is exactly the test I would have asked for. Three questions. **Target field nullability can now make a `TRY_CAST` fail** `StructArray::try_new(to_fields.clone(), vec![cast_keys, cast_values], ...)` builds the entries with the target's declared nullability. If the target key field is `nullable = false` and `cast_array` produced nulls in the keys, `try_new` returns `Err` and the whole query fails. Under `EvalMode::Try` that is reachable: `TRY_CAST(m AS MAP<INT, INT>)` where `m` is `MAP<STRING, INT>` and some key is not parseable as an int would produce a null key. What does Spark do there? If Spark returns a null map for the row, or throws a specific error, the current behavior of surfacing an Arrow `try_new` failure is probably not right. If Spark also fails, it would be worth a test pinning that the error is comparable. **Is a differing `sorted` flag reachable from Spark?** Spark's `MapType` has no sort-order concept, so I would expect `from_sorted` and `to_sorted` to always agree in a Comet-produced plan, which would make the hand-built branch reachable only when a child type changes. Is that right? If it is, the sort-flag half of the fix is defensive rather than a bug users can hit, and saying so would help set expectations. If it is not, an example of where the flags diverge would be valuable in the description, since "cast to a map type with a different `sorted` flag returned the wrong type" is currently the headline bug and there is no repro for it. **The delegated path and eval mode** The rename-only path calls `cast_with_options(array, to_type, &arrow_cast_options(cast_options.eval_mode))`. Since neither child type changes, no value conversion happens and the eval mode should be irrelevant. Is it worth passing `DEFAULT_CAST_OPTIONS` there instead, with a comment saying the mode cannot matter for a pure relabel? Threading the mode through suggests it does something, and the next person will wonder what. -- 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]
