Skip to content

feat: stage ORDER BY expression evaluation - #25358

Open
yashrb24 wants to merge 6 commits into
apache:mainfrom
yashrb24:staged-sort
Open

yashrb24 wants to merge 6 commits into
apache:mainfrom
yashrb24:staged-sort

Conversation

@yashrb24

@yashrb24 yashrb24 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Related to #25033. This PR is mostly an ideation of the way we can bring in this change since it's kind of an invasive change to the sorting mechanics and thus wanted feedback.

Rationale for this change

Today, sorting evaluates all ORDER BY expressions before it compares rows. Some of this work is unnecessary when earlier keys already determine the order.

This change evaluates later expressions only for rows tied on earlier keys. This could help when later expressions are expensive and earlier keys resolve most rows. The drawback is that the extra sorting steps can outweigh these savings when expressions are cheap or many rows remain tied.

What changes are included in this POC PR?

  • Evaluate sort keys in configurable groups.
  • Sort by the first group, then evaluate each later group only for rows that remain tied.

Are there any user-facing changes?

had added these flags for my local testing, can remove if needed

  • datafusion.execution.enable_staged_sort: default false.
  • datafusion.execution.sort_key_group_size: default value is 5

Currently not extending to TopK or spill codepath

@github-actions github-actions Bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) common Related to common crate physical-plan Changes to the physical-plan crate labels Sep 16, 2026
@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

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-common v55.1.0 (current)
       Built [  35.075s] (current)
     Parsing datafusion-common v55.1.0 (current)
      Parsed [   0.062s] (current)
    Building datafusion-common v55.1.0 (baseline)
       Built [  34.511s] (baseline)
     Parsing datafusion-common v55.1.0 (baseline)
      Parsed [   0.065s] (baseline)
    Checking datafusion-common v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.800s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field ExecutionOptions.enable_staged_sort in /home/runner/work/datafusion/datafusion/datafusion/common/src/config.rs:896
  field ExecutionOptions.sort_key_group_size in /home/runner/work/datafusion/datafusion/datafusion/common/src/config.rs:896

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  71.883s] datafusion-common
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  38.693s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.182s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  38.446s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.178s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.668s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  79.412s] datafusion-physical-plan
    Building datafusion-sqllogictest v55.1.0 (current)
       Built [  97.733s] (current)
     Parsing datafusion-sqllogictest v55.1.0 (current)
      Parsed [   0.016s] (current)
    Building datafusion-sqllogictest v55.1.0 (baseline)
       Built [  97.076s] (baseline)
     Parsing datafusion-sqllogictest v55.1.0 (baseline)
      Parsed [   0.017s] (baseline)
    Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.103s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 197.474s] datafusion-sqllogictest

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Sep 16, 2026
@github-actions github-actions Bot removed documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) labels Sep 16, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) labels Sep 16, 2026
@codecov-commenter

codecov-commenter commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 11.53846% with 138 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.61%. Comparing base (22651d2) to head (a266039).
⚠️ Report is 276 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-plan/src/sorts/sort.rs 11.84% 129 Missing and 5 partials ⚠️
datafusion/physical-plan/src/sorts/stream.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25358      +/-   ##
==========================================
+ Coverage   81.92%   82.61%   +0.69%     
==========================================
  Files        1135     1147      +12     
  Lines      427772   445654   +17882     
  Branches   427772   445654   +17882     
==========================================
+ Hits       350456   368190   +17734     
+ Misses      56373    55106    -1267     
- Partials    20943    22358    +1415     

☔ 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.

@yashrb24
yashrb24 marked this pull request as ready for review September 18, 2026 04:38
@kosiew

kosiew commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

@yashrb24
Please amend the PR description to follow the .github/pull_request_template.md template.

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yashrb24,

Thanks for working on this. I found two issues that I think need to be addressed before this is ready to merge.

}
arrow::compute::take_record_batch(batch, &UInt32Array::from(selection))?
};
let sort_columns = keys

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Staged sorting can skip later fallible or volatile ORDER BY expressions when earlier keys already resolve the order, so queries can behave differently depending on tie distribution. Please either document this experimental/error/volatile-evaluation contract and add tests that pin it, including that disabled staging stays eager, or preserve eager evaluation.

}
}

fn staged_sort_batch_chunked(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new staged sorting path does not have focused behavioral tests. Please add coverage for staged versus eager equivalence, successive tie groups, null ordering, chunked output, the zero-tie/error case, and both single-batch and multiple in-memory-run paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change common Related to common crate documentation Improvements or additions to documentation physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants