Fix wildcard expansion for `HAVING` clause #12046

goldmedal · 2024-08-18T06:57:42Z

Which issue does this PR close?

Closes #12013 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

goldmedal · 2024-08-18T07:00:54Z

datafusion/sqllogictest/test_files/aggregate.slt

+# because v2 is not in the group by clause, the sql is invalid
+query error
+select * from having_test group by v1 having max(v1) = 3


This error won't be detected when planning the aggregation because we won't expand the wildcard there. This error will be thrown when ExpandWildcardRule.

goldmedal · 2024-08-18T07:02:04Z

datafusion/sql/src/select.rs

+        let select_exprs_post_aggr = if having_expr_opt.is_some()
+            && group_by_exprs.is_empty()


Expand the wildcard only when we ensure the query isn't valid.

Instead of expanding wildcard, does it make sense to return the error if neither column is in group by nor is it part of the aggregate function?

D select * from t having max(a) = 1; Binder Error: column "a" must appear in the GROUP BY clause or must be part of an aggregate function. Either add it to the GROUP BY list, or use "ANY_VALUE(a)" if the exact value of "a" is not important.

In this case, if we see wildcard, we can just return the error without actually expanding the actual columns. Maybe modify check_columns_satisfy_exprs

Instead of expanding wildcard, does it make sense to return the error if neither column is in group by nor is it part of the aggregate function?

D select * from t having max(a) = 1; Binder Error: column "a" must appear in the GROUP BY clause or must be part of an aggregate function. Either add it to the GROUP BY list, or use "ANY_VALUE(a)" if the exact value of "a" is not important.

I think it makes sense for the having function. I can't find any case that violates this rule 🤔.

In this case, if we see wildcard, we can just return the error without actually expanding the actual columns. Maybe modify check_columns_satisfy_exprs

Instead of modifying check_columns_satisfy_exprs, I guess it would be checked at the part of the building having expression.

datafusion/datafusion/sql/src/select.rs

Lines 762 to 770 in a91be04

let having_expr_post_aggr = if let Some(having_expr) = having_expr_opt {

let having_expr_post_aggr =

rebase_expr(having_expr, &aggr_projection_exprs, input)?;

check_columns_satisfy_exprs(

&column_exprs_post_aggr,

&[having_expr_post_aggr.clone()],

"HAVING clause references non-aggregate values",

)?;

check_columns_satisfy_exprs is also used to check the normal aggregation. I think the rule can't be shared.

goldmedal · 2024-08-18T07:04:01Z

datafusion/sql/src/select.rs

@@ -214,7 +215,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

        let plan = if let Some(having_expr_post_aggr) = having_expr_post_aggr {
            LogicalPlanBuilder::from(plan)
-                .filter(having_expr_post_aggr)?
+                .having(having_expr_post_aggr)?


It's important to mention the filter is used for the HAVING clause because the wildcard expansion behavior is different.

jayzhan211 · 2024-08-18T08:56:39Z

datafusion/expr/src/logical_plan/builder.rs

+    /// Apply a filter which is used for a having clause
+    pub fn having(self, expr: impl Into<Expr>) -> Result<Self> {
+        let expr = normalize_col(expr.into(), &self.plan)?;
+        Filter::try_new_having(expr, Arc::new(self.plan))


nit: try_new_with_having (?

jayzhan211 · 2024-08-18T08:58:23Z

datafusion/optimizer/src/analyzer/expand_wildcard_rule.rs

@@ -301,4 +302,32 @@ mod tests {

        Ok(())
    }
+
+    #[test]
+    fn plan_having_wildcard_projection() -> Result<()> {


We could test this kind of test in slt with explain. I prefer to add rust test only for non-trivial cases

alamb · 2024-08-19T18:25:58Z

CI appears to be failing so marking this as draft while we work through those details

goldmedal · 2024-08-20T15:23:08Z

datafusion/sql/src/select.rs

@@ -749,11 +748,15 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
            .map(|expr| rebase_expr(expr, &aggr_projection_exprs, input))
            .collect::<Result<Vec<Expr>>>()?;

+        let wildcard_exprs = select_exprs_post_aggr.iter().filter(|expr| matches!(expr, Expr::Wildcard { .. })).collect::<Vec<_>>();
+        let wildcard_fields = exprlist_to_fields(wildcard_exprs, input)?;


I found that there are too many cases to handle, making it difficult to manage them cleanly. This could complicate the codebase. Currently, since we've expanded the wildcard for the schema, we can retrieve what we need based on the expanded schema.
This approach also allows us to provide correct and appropriate error messages for the user

jayzhan211 · 2024-08-21T00:38:43Z

datafusion/sql/src/select.rs

@@ -766,9 +769,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
            check_columns_satisfy_exprs(


We can have check_columns_satisfy_exprs_for_having_clause if the logic is different for projection and having

I think the function can be shared. The difference is that the check of having requires a return value to confirm that the validation has been executed and is valid.

jayzhan211 · 2024-08-21T01:05:11Z

datafusion/sql/src/utils.rs

+
+    wildcard_fields.into_iter().try_for_each(|(table, field)| {
+        let column_name = qualified_name(table, field.name());
+        if !column_names.iter().any(|c| c == &column_name) {


I think there are two valid cases

In group by clause

group by a having a = 1;

Part of an aggregate function
2. select max(a) from t having max(a) = 1;

I think we just need to check group by expressions and both select + having expressions.
If we got wildcard, I think we could ignore it, it doesn't matter 🤔

hmm ok. I added back the checking for the rules. I tried to add more comments to explain why we need those checks.
About the error message, we could enhance it in the future 🤔 .

jayzhan211 · 2024-08-21T00:46:01Z

datafusion/sql/src/utils.rs

@@ -119,9 +121,34 @@ pub(crate) fn check_columns_satisfy_exprs(
            _ => check_column_satisfies_expr(columns, e, message_prefix)?,
        }
    }
+    let column_names = columns
+        .iter()
+        .map(|c| format!("{}", c.schema_name()))


eq to c.schem_name().to_string()

jayzhan211 · 2024-08-22T01:10:41Z

@goldmedal I didn't find any query that go through the additional check, given the current test cases you added they don't require the additional check too. I suggest we start with simple test case and gradually fix more query and error message.

I don't know why can't I send the pull request to your repo, so I create another one in apache/datafusion #12106. This is just a demo for you, not for merging

jayzhan211

Thanks @goldmedal

goldmedal · 2024-08-22T05:08:02Z

Thanks @jayzhan211

alamb · 2024-08-22T10:09:02Z

🎉

goldmedal added 5 commits August 18, 2024 14:45

fix the wildcard expand for filter plan

c443264

expand the wildcard for the error message

fdb850d

add the tests

950472f

fix recompute_schema

7bf37d7

fix clippy

ffedb96

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Aug 18, 2024

goldmedal commented Aug 18, 2024

View reviewed changes

cargo fmt

2392092

jayzhan211 reviewed Aug 18, 2024

View reviewed changes

goldmedal added 3 commits August 19, 2024 00:56

change the check for having clause

9284e87

rename the function and moving the tests

39aa319

fix check

020ee7d

alamb marked this pull request as draft August 19, 2024 18:25

goldmedal added 3 commits August 20, 2024 21:08

Merge branch 'main' into bugfix/12013-fix-having

ccc2507

expand the schema for aggregate plan

ab68aa6

reduce the time to expand wildcard

6cb23da

goldmedal commented Aug 20, 2024

View reviewed changes

clean the testing table after tested

1879329

goldmedal marked this pull request as ready for review August 20, 2024 16:08

goldmedal marked this pull request as draft August 20, 2024 16:08

goldmedal marked this pull request as ready for review August 20, 2024 16:34

jayzhan211 reviewed Aug 21, 2024

View reviewed changes

goldmedal added 2 commits August 21, 2024 22:16

fmt and address review

69109ad

stop expand wildcard and add more check for group-by and selects

15cb062

simplify the having check

17ebb04

jayzhan211 approved these changes Aug 22, 2024

View reviewed changes

jayzhan211 merged commit a50aeef into apache:main Aug 22, 2024
24 checks passed

goldmedal deleted the bugfix/12013-fix-having branch August 22, 2024 05:07

alamb mentioned this pull request Aug 22, 2024

Fix compilation on main #12108

Merged

		let select_exprs_post_aggr = if having_expr_opt.is_some()
		&& group_by_exprs.is_empty()

	let having_expr_post_aggr = if let Some(having_expr) = having_expr_opt {
	let having_expr_post_aggr =
	rebase_expr(having_expr, &aggr_projection_exprs, input)?;

	check_columns_satisfy_exprs(
	&column_exprs_post_aggr,
	&[having_expr_post_aggr.clone()],
	"HAVING clause references non-aggregate values",
	)?;

		@@ -766,9 +769,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
		check_columns_satisfy_exprs(

Fix wildcard expansion for HAVING clause #12046

Fix wildcard expansion for HAVING clause #12046

Uh oh!

Conversation

goldmedal commented Aug 18, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

jayzhan211 Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goldmedal Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goldmedal commented Aug 22, 2024

Uh oh!

alamb commented Aug 22, 2024

Uh oh!

Uh oh!

Fix wildcard expansion for `HAVING` clause #12046

Fix wildcard expansion for `HAVING` clause #12046

jayzhan211 Aug 18, 2024 •

edited

Loading

goldmedal Aug 18, 2024 •

edited

Loading

jayzhan211 Aug 18, 2024 •

edited

Loading

jayzhan211 commented Aug 22, 2024 •

edited

Loading