[2010YOUY01]

Which issue does this PR close?

• Closes #.

Rationale for this change

Sort fuzzer fails in
https://github.com/apache/datafusion/actions/runs/41196052207/job/124522323666

It's a false positive due to fuzzer implementation itself, not the actual execution logic.

What changes are included in this PR?

1. add regression test for above failure
2. Fix fuzzer implementation (see comment for detailed explanation)

What is the testing strategy for this PR?

Added one regression UT

Are there any user-facing changes?

[2010YOUY01]

The only core change is this line, all other diffs are propagated changes.

The essence is, there might be two batches, that are logically the same, but physically different. So comparing formatted string works for assertion, while direct eq on RecordBatch can't do comparison for logical equivalence.

The actual failing case is in one dictionary encoding array, I've let Codex verified it's the above case, and it can be reproduced by regression test added in this PR.

This change was introduced 2 days ago in

• fix: bound parallel batch sorting in SortExec #26242

I guess it's for clean up, this PR reverts it to the original version

[jayzhan211]

@2010YOUY01 thanks for tracking this down! I confirmed the cause on this seed: in the Dictionary(UInt64, Utf8) column, one side has a null key and the other has a valid key pointing to a null dictionary value, and ArrayData == treats those as different.

One concern: check_equality_of_batches sorts the formatted lines before comparing, so the sort fuzzer stops checking order (rows [1, 2, 3] vs [3, 1, 2] pass). #26242 switched to in-order comparison on purpose, to catch the parallel merge returning runs out of order. Comparing the formatted output without sorting fixes this seed and keeps that check. With the change below, the regression test, the determinism test and several sort_query_fuzzer_runner runs all pass for me:

 use arrow::array::RecordBatch;
+use arrow::util::pretty::pretty_format_batches;
 ...
-use crate::fuzz_cases::aggregation_fuzzer::check_equality_of_batches;
 ...
-                        // Check logical equivalence.
-                        // `assert_eq!` may report false failures when logically equivalent
-                        // batches have different physical encodings.
-                        check_equality_of_batches(expected, &results).unwrap();
+                        // Every projected column is an ORDER BY key, so tied
+                        // rows have identical output and can be compared in order.
+                        assert_same_rows_in_order(expected, &results, "");
 ...
-                check_equality_of_batches(&results, &actual).unwrap_or_else(|err| {
-                    panic!("sort_in_place_threshold_bytes={threshold}: {err:?}")
-                });
+                assert_same_rows_in_order(
+                    &results,
+                    &actual,
+                    &format!("sort_in_place_threshold_bytes={threshold}"),
+                );
 ...
+/// Compares rows in order by their displayed values, so logically equal
+/// batches with different physical encodings (e.g. dictionaries) still match.
+fn assert_same_rows_in_order(
+    expected: &[RecordBatch],
+    actual: &[RecordBatch],
+    msg: &str,
+) {
+    let expected = pretty_format_batches(expected).unwrap().to_string();
+    let actual = pretty_format_batches(actual).unwrap().to_string();
+    assert_eq!(expected, actual, "{msg}");
+}
 ...
-        check_equality_of_batches(&res1, &res2).unwrap();
+        assert_same_rows_in_order(&res1, &res2, "");

Could you switch to this so the order check stays?

[2010YOUY01]

Thank you for the review! Addressed in 5d9ccac

[geoffreyclaude]

Thanks @jayzhan211, agreed that we should preserve the ordering check.

Could we use Arrow’s not_distinct in assert_same_rows_in_order instead of comparing formatted output? It compares corresponding values and correctly handles the two dictionary null representations.

I tested this approach locally: the regression seed, determinism test and randomized sort fuzzer all pass. Restoring direct batch equality reproduces the original failure.

Keeping the existing batch concatenation, the helper could take individual batches:

-use arrow::util::pretty::pretty_format_batches;
+use arrow::compute::kernels::cmp::not_distinct;

-/// Compares rows in order by their displayed values, so logically equal
-/// batches with different physical encodings (e.g. dictionaries) still match.
+/// Compares values in row order, treating different physical
+/// encodings of the same logical types (e.g. dictionary nulls) as equal.
 fn assert_same_rows_in_order(
-    expected: &[RecordBatch],
-    actual: &[RecordBatch],
+    expected: &RecordBatch,
+    actual: &RecordBatch,
     msg: &str,
 ) {
-    let expected = pretty_format_batches(expected).unwrap().to_string();
-    let actual = pretty_format_batches(actual).unwrap().to_string();
-    assert_eq!(expected, actual, "{msg}");
+    assert_eq!(expected.schema(), actual.schema(), "{msg}");
+    assert_eq!(expected.num_rows(), actual.num_rows(), "{msg}");
+
+    for (index, (left, right)) in
+        expected.columns().iter().zip(actual.columns()).enumerate()
+    {
+        let equal = not_distinct(left, right).unwrap();
+        assert_eq!(
+            equal.true_count(),
+            expected.num_rows(),
+            "{msg}: column {index}"
+        );
+    }
 }

This would preserve the ordering check without making formatting part of the correctness decision.

[2010YOUY01]

@geoffreyclaude Thanks for the review!

I think the long term goal is a util for 'Logical eq for SQL semantics', the issues are

• not_distinct is not exactly it, it might give us some surprises on float/nested type, see https://arrow.apache.org/rust/arrow/compute/kernels/cmp/fn.not_distinct.html
• and I agree formatting specific issues are also valid concern

So I think we can build some sql_eq util, likely a wrapper on arrow not_distinct in the future, now I prefer to keep it simple and keep using formatting.

[geoffreyclaude]

@geoffreyclaude Thanks for the review!

I think the long term goal is a util for 'Logical eq for SQL semantics', the issues are

• not_distinct is not exactly it, it might give us some surprises on float/nested type, see https://arrow.apache.org/rust/arrow/compute/kernels/cmp/fn.not_distinct.html
• and I agree formatting specific issues are also valid concern

So I think we can build some sql_eq util, likely a wrapper on arrow not_distinct in the future, now I prefer to keep it simple and keep using formatting.

Fair enough, let’s keep the formatting comparison here. It fixes the reported failure and preserves the ordering check, which is what really matters.
Thanks for addressing this!