[GitHub] [arrow-datafusion] jackwener commented on a diff in pull request #4579: API-break: just allow having use expr in `groupby` or `aggr`

[GitBox <gi...@apache.org>]

jackwener commented on code in PR #4579:
URL: https://github.com/apache/arrow-datafusion/pull/4579#discussion_r1045235807


##########
datafusion/sql/src/planner.rs:
##########
@@ -1193,33 +1193,24 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
             .collect::<Result<Vec<Expr>>>()?;
 
         // process group by, aggregation or having
-        let (plan, mut select_exprs_post_aggr, having_expr_post_aggr) =
-            if !group_by_exprs.is_empty() || !aggr_exprs.is_empty() {
-                self.aggregate(
-                    plan,
-                    &select_exprs,
-                    having_expr_opt.as_ref(),
-                    group_by_exprs,
-                    aggr_exprs,
-                )?
-            } else {
-                if let Some(having_expr) = &having_expr_opt {
-                    let available_columns = select_exprs
-                        .iter()
-                        .map(|expr| expr_as_column_expr(expr, &plan))
-                        .collect::<Result<Vec<Expr>>>()?;
-
-                    // Ensure the HAVING expression is using only columns
-                    // provided by the SELECT.
-                    check_columns_satisfy_exprs(
-                        &available_columns,
-                        &[having_expr.clone()],
-                        "HAVING clause references column(s) not provided by the select",
-                    )?;
+        let (plan, mut select_exprs_post_aggr, having_expr_post_aggr) = if !group_by_exprs
+            .is_empty()
+            || !aggr_exprs.is_empty()
+        {
+            self.aggregate(

Review Comment:
   It will be check by `check_columns_satisfy_exprs`.



##########
datafusion/sql/src/planner.rs:
##########
@@ -3641,10 +3632,11 @@ mod tests {
         let sql = "SELECT id, age
                    FROM person
                    HAVING age > 100 AND age < 200";
-        let expected = "Projection: person.id, person.age\
-                        \n  Filter: person.age > Int64(100) AND person.age < Int64(200)\
-                        \n    TableScan: person";
-        quick_test(sql, expected);
+        let err = logical_plan(sql).expect_err("query should have failed");

Review Comment:
   Nice advice❤️
   Added it.



-- 
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