alamb commented on code in PR #10767:
URL: https://github.com/apache/datafusion/pull/10767#discussion_r1623495522


##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -162,23 +162,40 @@ impl Unparser<'_> {
                 // A second projection implies a derived tablefactor
                 if !select.already_projected() {
                     // Special handling when projecting an agregation plan
-                    if let Some(agg) = find_agg_node_within_select(plan, true) 
{
-                        let items = p
-                            .expr
-                            .iter()
-                            .map(|proj_expr| {
-                                let unproj = unproject_agg_exprs(proj_expr, 
agg)?;
-                                self.select_item_to_sql(&unproj)
-                            })
-                            .collect::<Result<Vec<_>>>()?;
-
-                        select.projection(items);
-                        select.group_by(ast::GroupByExpr::Expressions(
-                            agg.group_expr
-                                .iter()
-                                .map(|expr| self.expr_to_sql(expr))
-                                .collect::<Result<Vec<_>>>()?,
-                        ));
+                    if let Some(aggvariant) = 
find_agg_node_within_select(plan, true) {

Review Comment:
   I agree this would be a good follow on PR. 



##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -127,7 +127,10 @@ fn roundtrip_statement() -> Result<()> {
             UNION ALL
             SELECT j2_string as string FROM j2
             ORDER BY string DESC
-            LIMIT 10"#
+            LIMIT 10"#,
+            "SELECT id, count(*) over (PARTITION BY first_name ROWS BETWEEN 
UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING), 

Review Comment:
   > While the two plans are not identical p1!=p2, I believe the difference is 
trivial and will actually result in the same computations taking place.
   
   That is my understanding as well
   
   We could plausibly simply the resulting plan of the window bounds are the 
default
   
   Can we also add some tests that have aggregate and window functions? 
Something like 
   
   ```sql
   SELECT id, count(distinct id), sum(id) OVER (PARTITION BY first_name) from 
person
   
   SELECT id, sum(id) OVER (PARTITION BY first_name ROWS 5 PRECEDING ROWS 2 
FOLLOWING) from person
   ```



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to