andygrove commented on PR #5194: URL: https://github.com/apache/datafusion-comet/pull/5194#issuecomment-5441509609
> **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. Hoisting the comparator out of the per-row loop is a clear win. The old code called `make_comparator` once per row over freshly sliced child arrays, which is a lot of setup to throw away 8192 times a batch. Building it once over the unsliced children and indexing absolutely is the right shape. I checked that `overlap_rows` still supplies the null-to-NULL semantics that `arrays_overlap_list_generic` used to compute inline, so the move does not lose that. Two things. **#5191 may already be solvable with code that just landed** PR #5403 adds a `spark_comparator` in `native/spark-expr/src/array_funcs/array_extrema.rs` that does exactly what #5191 needs: a recursive comparator where signed zeros compare equal, all NaNs compare equal, structs compare lexicographically, and nulls sort first. It handles `List`, `LargeList`, `ListView`, `FixedSizeList`, `Struct`, and `Dictionary`, falling through to `make_comparator` for everything else. If that lands, closing #5191 could be as small as lifting `spark_comparator` into a shared module and swapping it in here, at which point the `Incompatible` marking in this PR could be dropped entirely. Is it worth coordinating with #5403 so that the shared comparator has a home from the start, rather than marking these paths incompatible and then unmarking them? I am not asking you to block on that. But if this merges as-is, users lose native nested `arrays_overlap` and `array_position` in the interim, and it would be good to know that the interim is short. **`hasNestedFloatElements` does not look at map elements** ```scala case ArrayType(elementType: ArrayType, _) => ... case ArrayType(elementType: StructType, _) => ... case _ => false ``` `ArrayType(MapType(_, DoubleType, _))` falls to `false`. I believe Spark's analyzer rejects `arrays_overlap` and `array_position` on map elements because maps are not orderable, so this is probably unreachable. Could you confirm, and if so add a short comment saying maps cannot reach here? Otherwise the omission looks like a gap. **One note on the docs** `expressions.md` says "Nested floating-point signed-zero handling differs". Since these now report `Incompatible`, the default behavior is to route away from the native path, so the user-visible statement is arguably "falls back by default" rather than "differs". Worth aligning the wording with how `array_intersect` and `array_join` are described a few rows above, which say "Routes through the JVM codegen dispatcher by default". -- 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]
