kosiew commented on code in PR #21375:
URL: https://github.com/apache/datafusion/pull/21375#discussion_r3143219556
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -605,7 +609,83 @@ 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
+ // one or more Limit / Sort nodes, fold their clauses into the
+ // current query and skip those nodes during recursion.
+ // Otherwise the Limit/Sort arm would see `already_projected`
+ // and wrap everything in a spurious derived subquery, emitting
+ // the aggregate twice.
+ //
+ // Handles arbitrary stacks of Limit/Sort (e.g. Projection →
+ // Limit → Sort → Aggregate) by peeling nodes in a loop until
+ // we reach something other than a Limit or Sort. Each clause
+ // is folded at most once; if a second Limit or Sort is
Review Comment:
Nice improvement for the mixed Limit plus Sort case. I think there is still
one edge case here though: repeated modifier nodes still seem to fall back into
the duplicate aggregate shape.
The loop only folds the first Limit and the first Sort because of the `if
!have_limit` and `if !have_order_by` checks. When a second node of the same
type appears, the loop breaks and recursion resumes with
`select.already_projected() == true`, which brings back the derived subquery
pattern this change is trying to avoid.
Could we also keep folding repeated Sort or Limit nodes here, or otherwise
avoid re-entering the already-projected aggregate path in this 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]
