Skip to content

fix: do not derive ordering for arithmetic that can overflow #23910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kosiew merged 5 commits into from
Aug 5, 2026
Merged

fix: do not derive ordering for arithmetic that can overflow #23910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e0ccd56
dont remove sort
buraksenn Jul 26, 2026
8c6c14e
chore: get rid of csv
buraksenn Jul 26, 2026
34a473c
fix extended test and edge case
buraksenn Jul 27, 2026
92013b3
Merge branch 'main' into 23902-dont-remove-sort-for-overflow-and-wrap
buraksenn Jul 29, 2026
d31efba
address review
buraksenn Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions datafusion/core/tests/fuzz_cases/equivalence/ordering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
// under the License.

use crate::fuzz_cases::equivalence::utils::{
TestScalarUDF, create_random_schema, create_test_params, create_test_schema_2,
generate_table_for_eq_properties, generate_table_for_orderings,
is_table_same_after_sort,
TestScalarUDF, contains_overflowable_arithmetic, create_random_schema,
create_test_params, create_test_schema_2, generate_table_for_eq_properties,
generate_table_for_orderings, is_table_same_after_sort,
};
use arrow::compute::SortOptions;
use datafusion_common::Result;
Expand Down Expand Up @@ -144,14 +144,27 @@ fn test_ordering_satisfy_with_equivalence_complex_random() -> Result<()> {
let err_msg = format!(
"Error in test case requirement:{ordering:?}, expected: {expected:?}, eq_properties: {eq_properties}",
);
// Check whether ordering_satisfy API result and
// experimental result matches.

assert_eq!(
eq_properties.ordering_satisfy(ordering)?,
(expected | false),
"{err_msg}"
// A rejection turns inconclusive only from the first `+`/`-`
// key onwards, since possible overflow makes an ordering
// underivable even when the sample happens to be sorted. A
// table sorted by the full ordering is sorted by every prefix
// of it, so a rejected arithmetic-free prefix still proves
// the rejection is genuine.
let conclusive_prefix = LexOrdering::new(
ordering
.iter()
.take_while(|sort_expr| {
!contains_overflowable_arithmetic(&sort_expr.expr)
})
.cloned(),
);
if eq_properties.ordering_satisfy(ordering)? {
assert!(expected, "{err_msg}");
} else if let Some(prefix) = conclusive_prefix
&& !eq_properties.ordering_satisfy(prefix)?
{
assert!(!expected, "{err_msg}");
}
}
}
}
Expand Down
32 changes: 24 additions & 8 deletions datafusion/core/tests/fuzz_cases/equivalence/projection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
// under the License.

use crate::fuzz_cases::equivalence::utils::{
TestScalarUDF, apply_projection, create_random_schema,
generate_table_for_eq_properties, is_table_same_after_sort,
TestScalarUDF, apply_projection, contains_overflowable_arithmetic,
create_random_schema, generate_table_for_eq_properties, is_table_same_after_sort,
};
use arrow::compute::SortOptions;
use datafusion_common::Result;
Expand Down Expand Up @@ -179,13 +179,29 @@ fn ordering_satisfy_after_projection_random() -> Result<()> {
let err_msg = format!(
"Error in test case requirement:{ordering:?}, expected: {expected:?}, eq_properties: {eq_properties}, projected_eq: {projected_eq}, projection_mapping: {projection_mapping:?}"
);
// Check whether ordering_satisfy API result and
// experimental result matches.
assert_eq!(
projected_eq.ordering_satisfy(ordering)?,
expected,
"{err_msg}"
// Same reasoning as in `ordering.rs`: only keys from
// the first `+`/`-` source onwards are inconclusive,
// so assert on the longest prefix without one.
let conclusive_prefix = LexOrdering::new(
ordering
.iter()
.take_while(|sort_expr| {
!projection_mapping.iter().any(|(source, targets)| {
targets
.iter()
.any(|(target, _)| target.eq(&sort_expr.expr))
&& contains_overflowable_arithmetic(source)
})
})
.cloned(),
);
if projected_eq.ordering_satisfy(ordering)? {
assert!(expected, "{err_msg}");
} else if let Some(prefix) = conclusive_prefix
&& !projected_eq.ordering_satisfy(prefix)?
{
assert!(!expected, "{err_msg}");
}
}
}
}
Expand Down
19 changes: 17 additions & 2 deletions datafusion/core/tests/fuzz_cases/equivalence/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,20 @@ use std::sync::Arc;
use arrow::array::{ArrayRef, Float32Array, Float64Array, RecordBatch, UInt32Array};
use arrow::compute::{SortColumn, SortOptions, lexsort_to_indices, take_record_batch};
use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
use datafusion_common::tree_node::TreeNode;
use datafusion_common::utils::{compare_rows, get_row_at_idx};
use datafusion_common::{Result, exec_err, internal_datafusion_err, plan_err};
use datafusion_expr::sort_properties::{ExprProperties, SortProperties};
use datafusion_expr::{
ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature, Volatility,
ColumnarValue, Operator, ScalarFunctionArgs, ScalarUDFImpl, Signature, Volatility,
};
use datafusion_physical_expr::equivalence::{
EquivalenceClass, ProjectionMapping, convert_to_orderings,
};
use datafusion_physical_expr::{ConstExpr, EquivalenceProperties};
use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
use datafusion_physical_expr_common::sort_expr::{LexOrdering, PhysicalSortExpr};
use datafusion_physical_plan::expressions::{Column, col};
use datafusion_physical_plan::expressions::{BinaryExpr, Column, col};

use itertools::izip;
use rand::prelude::*;
Expand Down Expand Up @@ -209,6 +210,20 @@ fn add_equal_conditions_test() -> Result<()> {
Ok(())
}

/// Returns `true` if `expr` contains a `+` or `-` anywhere in its tree.
///
/// The equivalence framework conservatively discards orderings derived from
/// `+`/`-` expressions, because wrapping overflow can break them over the
/// type's full domain even when a finite batch happens to remain sorted.
pub fn contains_overflowable_arithmetic(expr: &Arc<dyn PhysicalExpr>) -> bool {
expr.exists(|e| {
Ok(e.downcast_ref::<BinaryExpr>().is_some_and(|binary| {
matches!(binary.op(), Operator::Plus | Operator::Minus)
}))
})
.unwrap()
}

/// Checks if the table (RecordBatch) remains unchanged when sorted according to the provided `required_ordering`.
///
/// The function works by adding a unique column of ascending integers to the original table. This column ensures
Expand Down
8 changes: 4 additions & 4 deletions datafusion/physical-expr/src/equivalence/ordering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,8 +525,8 @@ mod tests {
vec![col_e],
// requirement [a ASC, c ASC, a+b ASC],
vec![(col_a, options), (col_c, options), (&a_plus_b, options)],
// expected: requirement is satisfied.
true,
// expected: requirement is not satisfied because addition can wrap.
false,
),
// ------------ TEST CASE 4 ------------
(
Expand Down Expand Up @@ -672,8 +672,8 @@ mod tests {
vec![col_e],
// requirement [c ASC, d ASC, a + b ASC],
vec![(col_c, options), (col_d, options), (&a_plus_b, options)],
// expected: requirement is satisfied.
true,
// expected: requirement is not satisfied because addition can wrap.
false,
),
];

Expand Down
8 changes: 4 additions & 4 deletions datafusion/physical-expr/src/equivalence/properties/dependency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,10 +632,10 @@ mod tests {
]);

let test_cases = vec![
// d + b
// d + b can wrap
(
Arc::new(BinaryExpr::new(col_d, Operator::Plus, Arc::clone(&col_b))) as _,
SortProperties::Ordered(option_asc),
SortProperties::Unordered,
),
// b
(col_b, SortProperties::Ordered(option_asc)),
Expand Down Expand Up @@ -717,8 +717,8 @@ mod tests {
(vec![col_b], vec![]),
// TEST CASE 5
(vec![col_d], vec![(col_d, option_asc)]),
// TEST CASE 5
(vec![&a_plus_d], vec![(&a_plus_d, option_asc)]),
// TEST CASE 5: a + d is not ordered because addition can wrap.
(vec![&a_plus_d], vec![]),
// TEST CASE 6
(
vec![col_b, col_d],
Expand Down
Loading
Loading