jayzhan211 commented on code in PR #26983:
URL: https://github.com/apache/datafusion/pull/26983#discussion_r4434325344
##########
datafusion/core/tests/fuzz_cases/sort_query_fuzz.rs:
##########
@@ -210,9 +211,10 @@ impl SortQueryFuzzer {
if expected_results.is_none() {
expected_results = Some(results);
} else if let Some(ref expected) = expected_results {
- // Every projected column is an ORDER BY key, so tied
- // rows have identical output and can be compared in
order.
- assert_eq!(expected, &results);
+ // Check logical equivalence.
+ // `assert_eq!` may report false failures when
logically equivalent
+ // batches have different physical encodings.
+ check_equality_of_batches(expected, &results).unwrap();
Review Comment:
@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:
```diff
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?
--
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]
