yonatan-sevenai commented on code in PR #21375:
URL: https://github.com/apache/datafusion/pull/21375#discussion_r3137885533
##########
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:
Great catch.
I added two new tests:
`test_unparse_aggregate_with_sort_over_limit_no_inner_proj` and
`test_unparse_aggregate_with_limit_sort_no_inner_proj` and changed the
algorithm to loop through as many limit / sort objects found.
Please take a look. There are theoretical weird pathological cases where
we'd see Projection -> Sort -> Sort -> Limit -> Limit -> Aggregate
(theoretically valid form)?
I think my solution handles them correctly, but wanted a second pair of
eyes.
--
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]
