[GitHub] [arrow-datafusion] crepererum commented on issue #6116: `common_sub_expression_eliminate` on aggregates is wrong

["crepererum (via GitHub)" <gi...@apache.org>]

crepererum commented on issue #6116:
URL: https://github.com/apache/arrow-datafusion/issues/6116#issuecomment-1523123687

   Some more analysis:
   
   Someone could argue the the above examples are "wrong" in the first place because `LogicalPlan::Aggregate` is only allowed to have:
   
   - Alias
   - AggregateFunction
   - AggregateUdf
   
   I.e. putting other post-aggregate transforms (like binary expressions) is technically not allowed (even though this only seems to fail during physical planning, which is a bit late).
   
   However the following is a totally legal logical plan and still gets misoptimized (this test passes but shouldn't):
   
   ```rust
   #[test]
   fn aggregate2() -> Result<()> {
       let table_scan = test_table_scan()?;
   
       let plan = LogicalPlanBuilder::from(table_scan)
           .aggregate(
               iter::empty::<Expr>(),
               vec![
                   avg(col("a")).alias("first"),
                   avg(col("a")).alias("second"),
               ],
           )?
           .build()?;
   
       let expected = "Aggregate: groupBy=[[]], aggr=[[AVG(test.a)test.a AS AVG(test.a) AS first, AVG(test.a)test.a AS AVG(test.a) AS second]]\
       \n  Projection: AVG(test.a) AS AVG(test.a)test.a, test.a, test.b, test.c\
       \n    TableScan: test";
   
       assert_optimized_plan_eq(expected, &plan);
   
       Ok(())
   }
   ```


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

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