Skip to content

Support ORDER BY ALL for projected expressions - #25243

Open
osipovartem wants to merge 2 commits into
apache:mainfrom
Embucket:upstream-order-by-all-expressions
Open

Support ORDER BY ALL for projected expressions#25243
osipovartem wants to merge 2 commits into
apache:mainfrom
Embucket:upstream-order-by-all-expressions

Conversation

@osipovartem

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes a planner gap where ORDER BY ALL only accepted raw column projections and was silently ignored for set-operation outputs.

Rationale for this change

ORDER BY ALL means sorting by every output column in select-list order. Expanding it to the equivalent 1-based ordinal keys supports computed expressions, aliases, aggregate outputs, wildcard-expanded projections, and set-operation outputs through one planner path.

The ordinal expansion also sorts already-projected columns rather than evaluating computed expressions again. Physical execution remains on DataFusion's existing vectorized SortExec; this adds no row-wise conversion or custom physical operator.

What changes are included?

  • expand OrderByKind::All to ordinal OrderByExprs from the output width
  • provide output width for non-SELECT set expressions instead of dropping ORDER BY ALL
  • add focused execution coverage for computed expressions, null-order options, set operations, and aggregate output

Are these changes tested?

  • cargo +1.95.0 test -p datafusion-sqllogictest --test sqllogictests -- order_by_all.slt --test-threads 1
  • cargo +1.95.0 test -p datafusion-sql --test sql_integration -- --test-threads 8 (591 passed)
  • cargo +1.95.0 clippy -p datafusion-sql --all-targets -- -D warnings
  • cargo +1.95.0 fmt --all -- --check

Snowflake dialect parsing support is proposed independently in apache/datafusion-sqlparser-rs#2502; this planner change is generic to every dialect that already emits OrderByKind::All.

@github-actions github-actions Bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Sep 12, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.94%. Comparing base (6079a1e) to head (b5fedbc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/sql/src/query.rs 89.47% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25243      +/-   ##
==========================================
- Coverage   81.95%   81.94%   -0.01%     
==========================================
  Files        1133     1133              
  Lines      423799   423793       -6     
  Branches   423799   423793       -6     
==========================================
- Hits       347307   347294      -13     
- Misses      55899    55907       +8     
+ Partials    20593    20592       -1     

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

@jayzhan211 jayzhan211 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.

Thanks @osipovartem , LGTM!

# specific language governing permissions and limitations
# under the License.

# ORDER BY ALL expands computed expressions and aliases in select-list order.

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.

It would be nice to add

SELECT b AS a, a AS b FROM t ORDER BY ALL, plus optionally a self-join with duplicate output names.

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

Labels

sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants