kosiew commented on code in PR #21375:
URL: https://github.com/apache/datafusion/pull/21375#discussion_r3137144490
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -605,7 +609,76 @@ impl Unparser<'_> {
if self.dialect.unnest_as_lateral_flatten() {
Self::collect_flatten_aliases(p.input.as_ref(), select);
}
- self.reconstruct_select_statement(plan, p, select)?;
+ let found_agg = self.reconstruct_select_statement(plan, p,
select)?;
+
+ // If the Projection claimed an Aggregate by reaching through
+ // a Limit or Sort, fold those clauses into the current query
+ // and skip the node during recursion. Otherwise the Limit/Sort
+ // arm would see `already_projected` and wrap everything in a
+ // spurious derived subquery.
+ if found_agg {
+ if let LogicalPlan::Limit(limit) = p.input.as_ref() {
Review Comment:
I think this currently only folds a single immediate `Limit` or `Sort` child.
A pretty common stacked shape for ordered aggregates is `Projection -> Limit
-> Sort -> Aggregate`. That still appears to fall back into the existing `Sort`
arm with `select.already_projected() == true`, which recreates the derived
subquery this change is trying to avoid.
I reproduced this locally with a manual plan, and the current code emits
something like `SELECT max(...) AS ... FROM (SELECT max(...) AS __agg_0 FROM
... ORDER BY max(...) ...) LIMIT 20`. So the aggregate is still duplicated, and
the sort stays trapped in the inner query.
Could you handle chained `Limit` and `Sort` modifiers together before
recursing, and add a regression test for the combined `LIMIT + ORDER BY` case?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
