Allow expression to report whether they keep the same ordering of the input (`a.cmp(b) == f(a).cmp(f(b))`) opening up for more optimization by rluvaton · Pull Request #23807 · apache/datafusion

rluvaton · 2026-07-22T15:04:48Z

Which issue does this PR close?

Closes ProjectExec does not keep sorting information for passthrough expression #23798

Related to:

[MAJOR] Equivalence System Overhaul #16217

Rationale for this change

To be able to keep the same sorting order allowing for more optimizations

Now comet own cast implementation can be recognized as not modifying sort order in the same cases that datafusion cast does.

What changes are included in this PR?

added strictly_order_preserving property to ExprProperties + varius other places
and replaced the hard coded logic for cast (substitute_cast_ordering) about keeping input order with more generic approach that now any expression can implement and have the same advantage of sort elimination

Are these changes tested?

yes

Are there any user-facing changes?

yes, breaking change, added strictly_order_preserving property to ExprProperties

github-actions · 2026-07-22T15:19:35Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion v54.1.0 (current)
       Built [ 109.639s] (current)
     Parsing datafusion v54.1.0 (current)
      Parsed [   0.033s] (current)
    Building datafusion v54.1.0 (baseline)
       Built [ 106.483s] (baseline)
     Parsing datafusion v54.1.0 (baseline)
      Parsed [   0.035s] (baseline)
    Checking datafusion v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.639s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 218.724s] datafusion
    Building datafusion-expr v54.1.0 (current)
       Built [  28.067s] (current)
     Parsing datafusion-expr v54.1.0 (current)
      Parsed [   0.076s] (current)
    Building datafusion-expr v54.1.0 (baseline)
       Built [  27.658s] (baseline)
     Parsing datafusion-expr v54.1.0 (baseline)
      Parsed [   0.076s] (baseline)
    Checking datafusion-expr v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   1.379s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  58.374s] datafusion-expr
    Building datafusion-expr-common v54.1.0 (current)
       Built [  19.046s] (current)
     Parsing datafusion-expr-common v54.1.0 (current)
      Parsed [   0.018s] (current)
    Building datafusion-expr-common v54.1.0 (baseline)
       Built [  18.935s] (baseline)
     Parsing datafusion-expr-common v54.1.0 (baseline)
      Parsed [   0.019s] (baseline)
    Checking datafusion-expr-common v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.189s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field ExprProperties.strictly_order_preserving in /home/runner/work/datafusion/datafusion/datafusion/expr-common/src/sort_properties.rs:199

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  39.010s] datafusion-expr-common
    Building datafusion-ffi v54.1.0 (current)
       Built [  60.892s] (current)
     Parsing datafusion-ffi v54.1.0 (current)
      Parsed [   0.060s] (current)
    Building datafusion-ffi v54.1.0 (baseline)
       Built [  60.471s] (baseline)
     Parsing datafusion-ffi v54.1.0 (baseline)
      Parsed [   0.062s] (baseline)
    Checking datafusion-ffi v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.240s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 123.189s] datafusion-ffi
    Building datafusion-functions v54.1.0 (current)
       Built [  30.923s] (current)
     Parsing datafusion-functions v54.1.0 (current)
      Parsed [   0.085s] (current)
    Building datafusion-functions v54.1.0 (baseline)
       Built [  30.836s] (baseline)
     Parsing datafusion-functions v54.1.0 (baseline)
      Parsed [   0.086s] (baseline)
    Checking datafusion-functions v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.402s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  63.599s] datafusion-functions
    Building datafusion-physical-expr v54.1.0 (current)
       Built [  29.865s] (current)
     Parsing datafusion-physical-expr v54.1.0 (current)
      Parsed [   0.049s] (current)
    Building datafusion-physical-expr v54.1.0 (baseline)
       Built [  29.643s] (baseline)
     Parsing datafusion-physical-expr v54.1.0 (baseline)
      Parsed [   0.050s] (baseline)
    Checking datafusion-physical-expr v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.428s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  60.992s] datafusion-physical-expr

rluvaton · 2026-07-22T15:20:10Z

Cc @andygrove, @mbutrovich, @ozankabak

getChan · 2026-07-22T16:55:43Z

-    if is_order_preserving_cast_family(&child.range.data_type(), target_type) {
-        Ok(child.clone().with_range(unbounded))
+    let source_type = child.range.data_type();
+    // A widening cast is additionally one-to-one, so it is strictly


I may not have full context, so please feel free to ignore this if it's a misunderstanding.

CastExpr::check_bigger_cast includes Int32 -> Float32 and Int64 -> Float64. Due to float mantissa limits, large integers lose precision and can collapse to the same float value (e.g. 16_777_216_i32 as f32 == 16_777_217_i32 as f32), so they are not strictly 1-to-1

You are right, unfortunately this moved from here (so no logic change has been made):

datafusion/datafusion/physical-expr/src/equivalence/properties/mod.rs

Line 215 in e271f65

&& CastExpr::check_bigger_cast(cast_expr.cast_type(), expr_type))

filed. #23808

rluvaton · 2026-07-22T16:58:53Z

            preserves_lex_ordering: value.preserves_lex_ordering,
+            // Not carried over the FFI boundary (it would break the ABI);
+            // stay conservative.
+            strictly_order_preserving: false,


Should I make breaking change in FFI (by adding the field) while I'm here to avoid 2 breaking changes? I'm not really familiar with this module

rluvaton · 2026-07-22T17:01:12Z

-    if is_order_preserving_cast_family(&child.range.data_type(), target_type) {
-        Ok(child.clone().with_range(unbounded))
+    let source_type = child.range.data_type();
+    // A widening cast is additionally one-to-one, so it is strictly


You are right, unfortunately this moved from here (so no logic change has been made):

datafusion/datafusion/physical-expr/src/equivalence/properties/mod.rs

Line 215 in e271f65

&& CastExpr::check_bigger_cast(cast_expr.cast_type(), expr_type))

codecov-commenter · 2026-07-22T20:34:53Z

Codecov Report

❌ Patch coverage is 88.29787% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (ed22a4a) to head (7305547).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...tafusion/physical-expr/src/equivalence/ordering.rs	83.33%	3 Missing and 2 partials ⚠️
...on/physical-expr/src/equivalence/properties/mod.rs	84.84%	0 Missing and 5 partials ⚠️
datafusion/physical-expr/src/scalar_function.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23807      +/-   ##
==========================================
+ Coverage   80.69%   80.71%   +0.02%     
==========================================
  Files        1089     1089              
  Lines      368447   368979     +532     
  Branches   368447   368979     +532     
==========================================
+ Hits       297332   297836     +504     
+ Misses      53405    53376      -29     
- Partials    17710    17767      +57

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

rluvaton added 4 commits July 22, 2026 14:12

add test that fails

15a6de3

feat: add strictly_order_preserving to ExprProperties

466f279

update doc

d60f288

update sort

9fd3665

github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate functions Changes to functions implementation ffi Changes to the ffi crate labels Jul 22, 2026

update comment

7305547

rluvaton changed the title ~~Debug why sort ordering get messed up~~ Allow expression to report whether they keep the same ordering of the input (a.cmp(b) == f(a).cmp(f(b)) opening up for more optimization Jul 22, 2026

rluvaton force-pushed the debug-why-sort-ordering-get-messed-up branch from e0c40b1 to 7305547 Compare July 22, 2026 15:13

rluvaton added the api change Changes the API exposed to users of the crate label Jul 22, 2026

rluvaton marked this pull request as ready for review July 22, 2026 15:19

github-actions Bot added the auto detected api change Auto detected API change label Jul 22, 2026

getChan reviewed Jul 22, 2026

View reviewed changes

rluvaton commented Jul 22, 2026

View reviewed changes

This was referenced Jul 22, 2026

Fix precision loss in CastExpr::check_bigger_cast for integer-to-float conversions #23808

Open

fix: exclude precision-losing integer-to-float conversions from CastExpr::check_bigger_cast (#23808) #23809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow expression to report whether they keep the same ordering of the input (`a.cmp(b) == f(a).cmp(f(b))`) opening up for more optimization#23807

Allow expression to report whether they keep the same ordering of the input (`a.cmp(b) == f(a).cmp(f(b))`) opening up for more optimization#23807
rluvaton wants to merge 5 commits into
apache:mainfrom
rluvaton:debug-why-sort-ordering-get-messed-up

rluvaton commented Jul 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

rluvaton commented Jul 22, 2026

Uh oh!

getChan Jul 22, 2026 •

edited

Loading

Uh oh!

rluvaton Jul 22, 2026

Uh oh!

getChan Jul 22, 2026

Uh oh!

rluvaton Jul 22, 2026

Uh oh!

rluvaton Jul 22, 2026

Uh oh!

codecov-commenter commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rluvaton commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rluvaton commented Jul 22, 2026

Uh oh!

getChan Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rluvaton Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

getChan Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

rluvaton Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

rluvaton Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 22, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rluvaton commented Jul 22, 2026 •

edited

Loading

github-actions Bot commented Jul 22, 2026 •

edited

Loading

getChan Jul 22, 2026 •

edited

Loading