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.

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?