You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "krinart (via GitHub)" <gi...@apache.org> on 2026/03/03 02:16:38 UTC
[PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
krinart opened a new pull request, #20658:
URL: https://github.com/apache/datafusion/pull/20658
## Which issue does this PR close?
Restore the `already_projected()` guard in the Sort case of `select_to_sql_recursively`. Without it, queries with ORDER BY expressions generate invalid SQL when the Sort node follows an outer Projection.
## Problem ##
Two regressions in the DuckDB federation unparser after upgrading to DataFusion 52:
1. clickbench q25: Queries like `SELECT "SearchPhrase" ... ORDER BY to_timestamp("EventTime") LIMIT 10` produce ORDER BY outside the subquery, referencing table names ("hits") that are out of scope.
2. tpcds q36: ORDER BY with `GROUPING()` expressions loses the `derived_sort` subquery alias, placing sort references outside their valid scope.
## Root Cause ##
DF52's optimizer merges `Limit` into `Sort` as a fetch parameter, changing the plan shape from `Projection → Limit → Sort → ...` to `Projection → Sort(fetch) → ....` The Sort case previously had a guard that wrapped the sort into a `derived_sort` subquery when `already_projected()` was true. This guard was removed in DF52, causing ORDER BY and LIMIT to be placed on the outer query where inner table references are invalid.
## Fix ##
Re-add the guard at the top of the Sort match arm in select_to_sql_recursively:
```rust
if select.already_projected() {
return self.derive_with_dialect_alias("derived_sort", plan, relation, false, vec![]);
}
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "krinart (via GitHub)" <gi...@apache.org>.
krinart commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4094581857
Looking into it. Will try to fix it today
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4054341421
Here is a proposed fix: https://github.com/apache/datafusion/pull/20923
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4046562801
I also filed https://github.com/apache/datafusion/issues/20905 to track this issue so we can merge it back to 52
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4054303781
We are trying to prepare a 52 and 53 release, and since this issue is a regression I would like to fix it today. I can't push to your branch (as it is in the spiceai fork) so I will likely open a new PR to get it ready
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#discussion_r2912686805
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -223,7 +223,15 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
let mut collects = p.expr.clone();
for sort in &sort.expr {
- collects.push(sort.expr.clone());
+ // Strip aliases from sort expressions so the comparison matches
+ // the inner Projection's raw expressions. The optimizer may add
+ // sort expressions to the inner Projection without aliases, while
+ // the Sort node's expressions carry aliases from the original plan.
+ let mut expr = sort.expr.clone();
Review Comment:
I think this is the same as https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html#method.unalias_nested
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4103287007
Thank you @krinart 🙏
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4085840066
@krinart will you be able to look? not wanted to open a PR but if you cant I can open a PR for @alamb to approve and merge
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #20658:
URL: https://github.com/apache/datafusion/pull/20658
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "krinart (via GitHub)" <gi...@apache.org>.
krinart commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4094711088
@alamb @buraksenn Done!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "krinart (via GitHub)" <gi...@apache.org>.
krinart closed pull request #20658: Restore Sort unparser guard for correct ORDER BY placement
URL: https://github.com/apache/datafusion/pull/20658
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4046546793
@krinart -- this PR also has some conflicts that need to be resolved
Maybe you can also address the comment as well? I would like to backport this to the 52 branch for the next patch release
<img width="1060" height="386" alt="Screenshot 2026-03-12 at 8 54 43 AM" src="https://github.com/user-attachments/assets/4360d441-8f97-442e-916c-6752a9b591a6" />
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#discussion_r2930535936
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -223,7 +223,15 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
let mut collects = p.expr.clone();
for sort in &sort.expr {
- collects.push(sort.expr.clone());
+ // Strip aliases from sort expressions so the comparison matches
+ // the inner Projection's raw expressions. The optimizer may add
+ // sort expressions to the inner Projection without aliases, while
+ // the Sort node's expressions carry aliases from the original plan.
+ let mut expr = sort.expr.clone();
Review Comment:
It apparently isn't quite the same, but I am not sure if the different matters. Let's leave it like this
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "krinart (via GitHub)" <gi...@apache.org>.
krinart commented on code in PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#discussion_r2934426916
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -223,7 +223,15 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
let mut collects = p.expr.clone();
for sort in &sort.expr {
- collects.push(sort.expr.clone());
+ // Strip aliases from sort expressions so the comparison matches
+ // the inner Projection's raw expressions. The optimizer may add
+ // sort expressions to the inner Projection without aliases, while
+ // the Sort node's expressions carry aliases from the original plan.
+ let mut expr = sort.expr.clone();
Review Comment:
I tried to use it here - but `test_complex_order_by_with_grouping` test failed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4062751355
No worries, thank you @krinart -- I reopened this PR and as it is approved I'll merge it when it passes
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Restore Sort unparser guard for correct ORDER BY placement [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20658:
URL: https://github.com/apache/datafusion/pull/20658#issuecomment-4066880688
Hi @krinart -- there appears to be a few test failures. Any chance you can resolve them (You can use the changes on https://github.com/apache/datafusion/pull/20923 as a guide)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org