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?

fn nested_row_overlap<'a>(
    left: &'a ArrayRef,
    right: &'a ArrayRef,
    comparator: &'a dyn Fn(usize, usize) -> Ordering,
) -> impl FnMut(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;
                }
                if comparator(li, ri) == Ordering::Equal {
                    return true;
                }
            }
        }
        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
CREATE TABLE test_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

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".

[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.

[sunchao]

Summary

• Prior state and problem: Nested arrays_overlap constructed a comparator for each row. Nested floating-point comparisons also had an existing signed-zero incompatibility.
• Design approach: Build one comparator over the child arrays and reuse it with absolute offsets.
• Correctness / compatibility analysis: Null handling and sliced offsets are preserved, but removing comparator handling from the generic fallback introduces one P2 failure for nested types with different nullability metadata.
• Key design decisions: Reuse overlap_rows, keep a direct left-to-right scan, and make incompatible nested floating-point execution opt-in.
• Implementation sketch: Update comparator dispatch, share the Scala nested-float check, and extend documentation, regression coverage, and benchmarks.
• Behavioral changes worth calling out: Nested floating-point inputs now fall back by default. Two isolated release-mode measurements across all five nested benchmark shapes found no slowdown, so the previously reported long-struct regression was not reproduced.
• Suggested improvements: Preserve comparator handling for compatible nested types whose metadata differs, with a regression covering mixed array constructors.

Reviewed all six changed files in the full diff from 0690d38d3cc8f634dffb5341783f4b5da180eceb to b4d539403f11c96869bf2427d71191d9185371bc. Read the existing review and discussion. Routed skills: review-comet-pr, audit-comet-expression, and optimize-comet-expression.

Exact-head CI: 53 successful checks and 10 skipped, with no failures. Successful checks include Rust tests, Linux expression tests across Spark 3.4–4.2, and Spark 4.1 SQL tests. macOS and the PR benchmark check were skipped.

Validation: All 25 focused existing Rust tests passed. Three disposable native probes demonstrated base-success/head-failure, including actual expression producers. Spark 4.1.3 confirmed the SQL results. Relevant Spark implementations were compared across supported versions and the audit skill’s reference versions. Performance measurements used extracted kernels, not the full Criterion or end-to-end workloads. The Comet JVM suite was not run locally. Disposable project tests were removed, the checkout is clean, and no GitHub state was changed.

[sunchao]

[P2] Could we preserve nested comparator handling in this fallback? The caller still routes unequal Arrow child types here, including types that differ only in nullability. For a Parquet table t(i INT), SELECT arrays_overlap(array_repeat(named_struct('x',1,'y',i),1), array(named_struct('x',1,'y',i))) FROM t should return true. array_repeat preserves the non-nullable x field, while CometCreateArray widens it to nullable. The previous make_comparator handled this difference, but the unconditional find_in_array_flat now raises Nested comparison ... (hint: use make_comparator instead), aborting a previously working query. Keep comparator support in the fallback or dispatch compatible nested types before the strict metadata-equality guard, and cover this mixed-constructor case.

Evidence: Using exact base and head implementations with locked Arrow 59.3.0, disposable Rust probes compared identical List and Struct values whose child nullability differed. Both returned true on the base and Arrow errors on the head. A third probe using CreateNamedStruct, Spark's native array_repeat, Comet's spark_cast, and make_array produced the SQL input types and reproduced the same regression. Spark 4.1.3 returned [true, true, true] for i = 1, 2, NULL. Reproduction source and output are retained at /tmp/comet-5194-reproduction.rs and /tmp/comet-5194-repro.log.

[sunchao]

Summary

• Prior state and problem: Nested arrays_overlap constructed a comparator for each row, repeating setup and slicing.
• Design approach: Build one comparator over the child arrays and reuse it through absolute offsets and overlap_rows.
• Correctness / compatibility analysis: Null handling, sliced offsets, and inherited floating-point normalization are preserved in the tested cases. The existing P2 fallback finding remains unresolved. Three probes reproduced base-success/head-error for nested types with different nullability metadata.
• Key design decisions: Sharing the row driver centralizes null handling and reduces allocation. Changing traversal to unconditional left-first order introduces a reproducible performance regression for unequal-length arrays.
• Implementation sketch: The full base-relative diff changes two Rust files, adding comparator dispatch, a nested scan helper, a sliced-offset regression, and five benchmark shapes.
• Behavioral changes worth calling out: The five added benchmark shapes improved locally, but an unequal-length early-match case became approximately 50× slower. Signed-zero normalization is already present in both base and head.
• Suggested improvements: Preserve shorter-side probing while keeping comparator arguments in their original order, add the unequal-length benchmark, and restore nested comparator support in the generic fallback covered by the existing thread.

Reviewed the entire diff from 81f2574d5e92028d6a0c8aebd39e6e0b5b4c2fd2 to b64149ebb033aa6d6faf28de587774942f044eb7. Confirmed the PR is not a draft. Read all supplied reviews, comments, and threads. Routed skills: review-comet-pr, review-comet-expression-pr, audit-comet-expression, and optimize-comet-expression.

Exact-head CI: 23 successful checks, 14 skipped, no failures. Linux Spark 4.1 expression logs confirm arrays_overlap.sql passed. Spark’s own SQL suites, macOS, and the benchmark job were skipped.

Validation: All 27 focused Rust tests passed. Three disposable native probes confirmed the existing correctness blocker. Compared Spark sources across supported versions 3.4.3–4.2.0 and the audit reference versions. Two independent release-mode measurements and comparison counts confirmed the new performance finding. Measurements used verified extracted kernels with Arrow 59.3.0, not end-to-end Spark workloads or the full Criterion suite. The Comet JVM suite was not run locally. Disposable project tests were removed, the checkout is clean, and no GitHub state was changed.

[sunchao]

[P2] Could we preserve shorter-side probing while hoisting the comparator? For arrays_overlap(a, b) on nested integer columns where a=[[0],...,[127]] and b=[[127],...,[190]], both versions correctly return true, but the base probes the shorter right side and finds the match after 128 comparisons. This unconditional left-first loop performs 8,129 comparisons. Two independent release-mode measurements reproduced approximately 50× higher kernel latency, so this materially slows previously efficient overlapping-array workloads. Branch once per row to preserve the previous probe order, keeping comparator arguments in (left_index, right_index) order, and cover unequal lengths in the benchmark.

Evidence: Verified the extracted base/head kernel bodies against the requested Git commits. With locked Arrow 59.3.0, 128 rows, and every twentieth outer row null, alternating base/head measurements produced 0.148 ms versus 7.380 ms and 0.148 ms versus 7.406 ms. Outputs matched. A separately instrumented single-row probe counted exactly 128 versus 8,129 comparator calls. Reproduce with cargo run --release --offline --locked --manifest-path /tmp/comet-5194-b641-kernel/Cargo.toml --bin comet-5194-b641-kernel and --bin count. Timing logs are /tmp/comet-5194-final-bench1.log and /tmp/comet-5194-final-bench2.log.

[sunchao]

Summary

• Prior state and problem: Nested arrays_overlap repeatedly constructed comparators and sliced child arrays for each row.
• Design approach: Build one comparator over the full child arrays and reuse it with absolute offsets.
• Correctness / compatibility analysis: The previously reported mixed-nullability failure is fixed. Null handling, sliced offsets, and nested floating-point equality remain correct in the tested cases. No additional introduced P1/P2 issues found within this review.
• Key design decisions: Reusing overlap_rows centralizes null handling without substantial abstraction overhead. The unconditional left-first traversal still causes the existing performance regression.
• Implementation sketch: The full three-file diff changes comparator dispatch, adds a nested scan helper, and extends Rust tests, SQL coverage, and benchmarks. Signed-zero normalization already exists in the supplied base.
• Behavioral changes worth calling out: The existing P2 performance blocker remains unresolved. For left [[0],...,[127]] and right [[127],...,[190]], comparison counts rise from 128 to 8,129. Two independent release-mode samples measured approximately 0.147 ms on the base versus 7.38 ms on this head, with identical results.
• Suggested improvements: Address that existing thread by preserving shorter-side probing while keeping comparator arguments in (left_index, right_index) order, and cover unequal lengths in the benchmark.

Reviewed the entire diff from 81f2574d5e92028d6a0c8aebd39e6e0b5b4c2fd2 to 619fc4252752d89179c4d1418c58ab4a06f18ca4. Confirmed the PR is not a draft and read existing reviews, discussion, inline comments, and threads. Routed skills: review-comet-pr, review-comet-expression-pr, audit-comet-expression, and optimize-comet-expression.

Exact-head CI: 23 successful checks, 14 skipped, no failures. Linux Spark 4.1 expression logs confirm the updated arrays_overlap.sql passed. Spark’s own SQL suites, macOS, and the benchmark job were skipped.

Validation: All 28 focused Rust tests passed locally. Compared Spark sources across supported versions 3.4.3–4.2.0 and the audit reference versions. Performance measurements used extracted kernels verified against the exact base and head with Arrow 59.3.0. They were not end-to-end Spark measurements or full Criterion runs. The Comet JVM suite was not run locally. The checkout is clean and no GitHub state was changed.