refactor: use arrow make_comparator for nested structural equality in arrays_overlap and array_position [2/2]

[peterxcli]

Which issue does this PR close?

Closes#5101.
Follow-up to #5176. Related to #5191.

Rationale for this change

PR #5176 was merged only after I filed #5191, but the changes requested in its final review had not been pushed yet. This follow-up publishes those review changes on top of the merged upstream main.

#5191 tracks a pre-existing signed-zero mismatch in nested comparisons. Spark treats -0.0 and 0.0 as equal inside nested arrays or structs, while Arrow's total-order comparator distinguishes them. This affects nested arrays_overlap and the nested fallback of array_position; flat arrays_overlap intentionally continues to distinguish signed zero. This PR does not implement the normalization fix: it marks the affected native paths as incompatible by default and adds ignored SQL coverage for the follow-up.

What changes are included in this PR?

• Build one nested comparator over the unsliced child arrays and reuse it for every row, using absolute offsets.
• Keep the nested loop in left-then-right order and move comparator dispatch into the typed match.
• Mark nested floating-point arrays as incompatible for arrays_overlap and array_position because of Nested array comparison does not match Spark for signed zero #5191, with SQL and user-guide coverage.
• Extend the sliced-offset regression to cover null results.
• Add a nested-list benchmark whose match occurs partway through the row.

How are these changes tested?

• cargo test -p datafusion-comet-spark-expr --lib (620 passed)
• cargo clippy -p datafusion-comet-spark-expr --lib --tests --benches -- -D warnings
• cargo fmt --all -- --check
• ./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite arrays_overlap"
• cargo bench -p datafusion-comet-spark-expr --bench arrays_overlap --no-run

Benchmark medians compare the PR merge base b54d9dc40 (upstream main after #5176) with this PR:

Benchmark	Base	PR	Change
nested int32 early match	1.427 ms	0.909 ms	-36.3%
nested int32 long lists	1.595 ms	1.513 ms	-5.1%
nested int32 short lists	2.158 ms	1.686 ms	-21.9%
nested struct long lists	0.920 ms	1.078 ms	+17.2%
nested struct short lists	2.110 ms	1.156 ms	-45.2%

[peterxcli]

@andygrove this is the PR as followup for your review in #5176. this PR shows a very good speedup, around 60~80x. please take a look whenever you have time. thanks!

[andygrove]

Thanks for splitting this out. Hoisting make_comparator out of the per-row loop is clearly the right move, and the speedup is impressive.

I checked the new code against Spark's ArraysOverlap in collectionOperations.scala. Spark picks bruteForceEval with ordering.equiv whenever TypeUtils.typeWithProperEquals(elementType) is false, which is exactly the nested and binary cases, and the three-valued logic is hasNull set from either side with an early return true on a definite match. overlap_rows plus range_has_null reproduces that faithfully, including Spark's if (smaller.numElements() > 0) guard that makes an empty side yield false rather than null.

I also read Arrow's make_comparator in arrow-ord/src/ord.rs to confirm what the hoist relies on. compare() captures logical_nulls() at construction time and compare_impl maps (true, true) => Ordering::Equal, so inner nulls compare equal, matching ordering.equiv. That is why nested_row_overlap has to skip outer element nulls itself, and it does. (Null, Null) => Ordering::Equal also means a List<List<Null>> child does not error.

I built the branch and ran the tests locally. All 24 arrays_overlap tests pass. I added two throwaway tests to probe the cases I was worried about and both behave correctly. A sliced nested case that should return null under three-valued logic does return null, and arrays_overlap(array(array(NULL)), array(array(NULL))) returns true, which is what Spark gives.

I also verified the new regression test earns its place. On row 1 the right side is smaller so the swap branch fires, and without the (li, ri) fix the comparator would be handed left_values[4] against right_values[2..5] and return false where true is expected. Good test.

A few things I would like to see addressed.

The probe-side swap in nested_row_overlap costs more than it saves

In flat_row_overlap the swap matters because it keeps the hash table small. This path has no hash table, so the scan is O(n×m) whichever side is on the outside, and the swap only reorders the early exit while adding a probe_is_left branch to the innermost comparison. It is also what forced the (li, ri) argument-order fix in the first place.

I tried dropping it and looping left then right directly. Tests still pass, since equality and null skipping are both symmetric, and the nested benchmarks got faster. nested int32 long improved by about 2.5% and nested struct short by about 11%, with the other two inside the noise. Would you consider removing it?

fnnested_row_overlap<'a>(left:&'aArrayRef,right:&'aArrayRef,comparator:&'adynFn(usize,usize) -> Ordering,) -> implFnMut(Range<usize>,Range<usize>) -> bool + 'a{move |left_range, right_range| {for li in left_range {if left.is_null(li){continue;}for ri in right_range.clone(){if right.is_null(ri){continue;}ifcomparator(li, ri) == Ordering::Equal{returntrue;}}}false}}

arrays_overlap_list_generic is no longer just a fallback

The doc comment still says "Fallback for nested and otherwise unhandled element types", but nested is now the fast path at the top of the same function and the loop below only handles the leftovers such as binary, mismatched child types, and a Null child. Would it read better to move the comparator branch into the _ => arm of the match in arrays_overlap_list, so this function stays a true fallback? That would also drop the left_values.data_type() == right_values.data_type() re-check, which duplicates the guard the caller already applied.

The signed-zero mismatch is still invisible to users

Referencing #5191 from the tests is a good change. The user-facing docs still present both expressions as fully compatible though. docs/source/user-guide/latest/compatibility/expressions/array.md lists arrays_overlap as ✅ with no caveat, because CometArraysOverlap has no getSupportLevel override, and the array_position row only mentions the type fallback. Given #5191 is labeled correctness and priority:high, could we surface it? Adding getSupportLevel and getIncompatibleReasons with something like "nested float elements distinguish -0.0 from 0.0, unlike Spark" would let GenerateDocs pick it up. If you would rather keep this PR narrow, expanding the scope of #5191 to cover the serde and docs and noting that on the issue works too, but I lean toward doing it here since it is only a few lines.

I confirmed #5191's scope is accurate, incidentally. position_float uses v == search_val plus an explicit NaN branch, so the flat array_position path already matches ordering.equiv. Only the nested fallback differs.

Nested float SQL coverage

The nested and struct coverage added in #5176 is thorough. The one case missing is nested floats, which is where #5191 lives. Could you add something like this to arrays_overlap.sql so CI picks the fix up when it lands?

statement
CREATETABLEtest_overlap_nested_dbl(a array<array<double>>, b array<array<double>>) USING parquet
statement
INSERT INTO test_overlap_nested_dbl VALUES (array(array(0.0D)), array(array(-0.0D))), (array(array(double('NaN'))), array(array(double('NaN'))))
query ignore(https://github.com/apache/datafusion-comet/issues/5191)
SELECT a, b, arrays_overlap(a, b) FROM test_overlap_nested_dbl

Note the -0.0D rather than -0.0, otherwise the literal parses as a decimal and the case is vacuous.

Extending the new regression test

overlap_rows now derives null bookkeeping from range_has_null over absolute offsets. Would it be worth extending test_nested_array_sliced_offsets_and_probe_swap with a row that should come back null, something like [[10], NULL] against [[20]] inside the sliced region? I tried it locally and it does return null, so this is about pinning the behavior down rather than a suspected bug. That branch looks like the one most likely to regress if the offset handling gets touched again.

Benchmark data never overlaps

nested_int_lists and struct_lists build the two sides from disjoint ranges, so no row ever overlaps and every row pays the full n×m scan. That is the right worst case to have, but it means nothing here exercises the early exit. Would you consider adding one nested variant where a match is found partway through, the way int_lists uses offset to make the flat cases overlap?

[peterxcli]

@andygrove thanks for another round of review! addressed all your latest review in 5c73049

1. “Would you consider removing [the probe-side swap]?”

Done. nested_row_overlap now always iterates left then right, removing the swap and the probe_is_left branch from the inner loop. The corrected benchmarks compare against b54d9dc40, which already contains #5176. Four cases improved, although nested struct long regressed by about 17%; the PR description reports the complete results.

2. “Would it read better to move the comparator branch into the _ => arm of the match in arrays_overlap_list, so this function stays a true fallback?”

Done. Comparator dispatch now occurs directly in the guarded arrays_overlap_list match arm. arrays_overlap_list_generic handles only otherwise-unhandled types, and the redundant child-type equality check was removed.

3. “Given Nested array comparison does not match Spark for signed zero #5191 is labeled correctness and priority:high, could we surface it?”

Done for both arrays_overlap and array_position. Their serdes now report nested floating-point inputs as Incompatible, with getIncompatibleReasons supplying the explanation for generated compatibility documentation. The main expression table also links to #5191.

4. “Could you add something like this to arrays_overlap.sql so CI picks the fix up when it lands?”

Done. The SQL file now covers nested doubles containing 0.0D, -0.0D, and NaN, with query ignore(https://github.com/apache/datafusion-comet/issues/5191). The explicit D suffix ensures signed zero is parsed as a double rather than a decimal.

5. “Would it be worth extending test_nested_array_sliced_offsets_and_probe_swap with a row that should come back null?”

Done. The renamed sliced-offset regression now covers three results within the sliced region: false, null, and true. The null row uses [[10], NULL] against [[20]], pinning the absolute-offset null bookkeeping.

6. “Would you consider adding one nested variant where a match is found partway through?”

Done. nested_int_lists now accepts an offset, and the new nested int32 early match benchmark uses an offset of four. This finds a match partway through each eight-element row instead of always paying the complete O(n x m) scan.

[peterxcli]

@andygrove I've addressed your review, would appreciate it if you could take a look at the update and see if we can get this merged.

[andygrove]

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

caseArrayType(elementType: ArrayType, _) => ...
caseArrayType(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".

[peterxcli]

Thanks for the follow-up pass! Addressed in 390fc91.

On #5403 / spark_comparator: agreed that's the right endgame for #5191 — it's exactly the comparator these paths need. Since #5403 hasn't merged yet, I'd rather not couple the two PRs: I'll keep the Incompatible marking here and, once #5403 lands, follow up in #5191 by lifting spark_comparator into a shared module, swapping it into arrays_overlap/array_position, and dropping the Incompatible markings. On the interim cost: only arrays with nested float elements route away from the native path — that's exactly the path affected by the correctness bug — and spark.comet.expression.allowIncompatible opts back in for users who accept the difference. I'll note this plan on #5191.

On map elements in hasNestedFloatElements: confirmed unreachable. Both ArraysOverlap.checkInputDataTypes and ArrayPosition.checkInputDataTypes call TypeUtils.checkForOrderingExpr on the element type, and MapType is not orderable, so the analyzer rejects array<map<...>> inputs before planning. Added a comment saying so.

On the docs wording: good catch — updated both rows in expressions.md to say the nested-float case "falls back to Spark by default, and the incompatible native path is opt-in via allowIncompatible". I deliberately didn't copy the array_intersect/array_join phrasing: those route through the JVM codegen dispatcher (CodegenDispatchFallback), whereas these two serdes fall back to Spark entirely, so "codegen dispatcher" would be inaccurate here.