Move wildcard expansions to the analyzer #11681

goldmedal · 2024-07-27T10:07:08Z

Which issue does this PR close?

Closes #11639 .

Rationale for this change

As discussed in #11639 (comment), I tried to solve the to-do issue for moving wildcard expansion to the analyzer.

datafusion/datafusion/expr/src/logical_plan/builder.rs

Lines 1443 to 1458 in 7db4213

    
           // TODO: move it into analyzer 
        
           let input_schema = plan.schema(); 
        
           let mut projected_expr = vec![]; 
        
           for e in expr { 
        
               let e = e.into(); 
        
               match e { 
        
                   Expr::Wildcard { qualifier: None } => { 
        
                       projected_expr.extend(expand_wildcard(input_schema, &plan, None)?) 
        
                   } 
        
                   Expr::Wildcard { 
        
                       qualifier: Some(qualifier), 
        
                   } => projected_expr.extend(expand_qualified_wildcard( 
        
                       &qualifier, 
        
                       input_schema, 
        
                       None, 
        
                   )?),

I keep the wildcard expression when planning the SQL and implementing ExpandWildcardRule.

What changes are included in this PR?

This change impacts many parts. I did the required modifications for them.

Expanding expressions to fields for the schema of the Projection plan

When planning the SQL, we base the schema on a plan to perform multiple validations and optimizations. Even if we move the expansion of Expr::Wildcard to the analyzer, the schema should contain the actual column information instead of the wildcard field.

We should be careful with calc_func_dependencies_for_project. functional_dependencies is related to the implementation of constraints (#7040) and also affects the implicit grouping keys optimization (#11903 (comment)).

Handle `WildacrdAddiotionsOption` for replace, exclude, or expect, ...

Datafusion supports options for wildcard expressions. Due to moving the expansion to the analyzer, we should handle WildcardOptions within Expr::Wildcard. We need to consider these options when expanding the wildcard in the following:

ExpandWildcardRule
utils::exprlist_to_fields
utils::exprlist_len

Type_coercion for union

Previously, we will do the type coercion when planning the union. We should do it again after expanding the wildcard expression.

Unparsing Expr::Wildcard

I also implement the unparsing for the Expr::Wildcard to pass the tests. However, I leave a to-do issue for unparsing WildcardOptions

Are these changes tested?

yes

Are there any user-facing changes?

no.

datafusion/sql/src/select.rs

goldmedal · 2024-08-12T17:44:51Z

We performed many optimizations and validations based on the expanded expression when planning SQL. Ideally, we should move these behind the ExpandWildcardRule to avoid additional expansions for the schema fields.

goldmedal · 2024-08-12T17:46:13Z

datafusion/core/src/datasource/view.rs

+    fn apply_required_rule(logical_plan: LogicalPlan) -> Result<LogicalPlan> {
+        let options = ConfigOptions::default();
+        Analyzer::with_rules(vec![Arc::new(ExpandWildcardRule::new())]).execute_and_check(
+            logical_plan,
+            &options,
+            |_, _| {},
+        )
+    }


To generate the correct schema, we need to apply ExpandWildcardRule for the plan of the view first

goldmedal · 2024-08-12T17:49:37Z

datafusion/sql/src/select.rs

+        if let Some(replace) = options.opt_replace {
+            let replace_expr = replace
+                .items
+                .iter()
+                .map(|item| {
+                    Ok(self.sql_select_to_rex(


We need to plan the expression of replace first and then expand them in the ExpandWildcardRule.

goldmedal · 2024-08-12T17:59:38Z

Hi @jayzhan211 @alamb
I ran into several issues, so it took me longer than expected to finish this PR, and the changes ended up being a lot more extensive than I initially thought. I’ve tried to keep the original SQL behavior as much as possible, but I think we should ideally move more of the logic to the Analyzer.
If you could take a look at this PR, I’d appreciate it. Thanks!

jayzhan211

Overall looks good to me!

jayzhan211 · 2024-08-13T00:34:18Z

datafusion/expr/src/expr.rs

+
+#[derive(Clone, PartialEq, Eq, Hash, Debug, Default)]
+pub struct PlannedReplaceSelectItem {
+    pub items: Vec<Box<ReplaceSelectElement>>,


I don't think we need Box here. Vec<ReplaceSelectElement>

jayzhan211 · 2024-08-13T01:06:59Z

datafusion/expr/src/utils.rs

+                    .map(|c| c.flat_name())
+                    .collect();
+                    Ok::<_, DataFusionError>(
+                        (0..wildcard_schema.fields().len())


I think you can use wildcard_schema.field_names()

jayzhan211 · 2024-08-13T01:16:07Z

datafusion/optimizer/src/analyzer/expand_wildcard_rule.rs

+                .zip(replace.expressions().iter())
+                .find(|(item, _)| item.column_name.value == *name)
+            {
+                *expr = Expr::Alias(Alias {


You can try new_expr.alias(name)

datafusion/sqllogictest/test_files/window.slt

jayzhan211 · 2024-08-13T01:26:08Z

datafusion/sql/src/select.rs

+                items: replace.items,
+                planned_expressions: replace_expr,
+            };
+            Ok(WildcardOptions {


Maybe it is worth to have a with_replace function, so we just need options.with_replace(planned_option)

jayzhan211 · 2024-08-13T01:27:57Z

datafusion/core/src/datasource/view.rs

@@ -61,6 +64,15 @@ impl ViewTable {
        Ok(view)
    }

+    fn apply_required_rule(logical_plan: LogicalPlan) -> Result<LogicalPlan> {


jayzhan211 · 2024-08-13T01:29:20Z

datafusion/expr/src/expr.rs

@@ -970,6 +976,60 @@ impl GroupingSet {
    }
 }

+#[derive(Clone, PartialEq, Eq, Hash, Debug, Default)]
+pub struct WildcardOptions {
+    pub opt_ilike: Option<IlikeSelectItem>,


nit: I think opt_xxx is redundant

alamb

This looks pretty epic -- thank you @goldmedal and @jayzhan211

alamb · 2024-08-13T12:54:38Z

datafusion/expr/src/expr.rs

@@ -970,6 +976,72 @@ impl GroupingSet {
    }
 }

+#[derive(Clone, PartialEq, Eq, Hash, Debug, Default)]
+pub struct WildcardOptions {


as a follow on PR, can we add some doc comments to this struct explaining why this structure is needed and how to interpret it?

It seems like the core rationale is that wildcards have different semantics depending on what part of the query they appear in?

Sure, I added descriptions for the purpose and source of each option.

alamb · 2024-08-13T12:55:15Z

datafusion/expr/src/expr_fn.rs

+/// use datafusion_common::TableReference;
+/// use datafusion_expr::{qualified_wildcard};


If you put a # in front of lines in an example they are not shown but still compiled

Suggested change

/// use datafusion_common::TableReference;

/// use datafusion_expr::{qualified_wildcard};

/// # use datafusion_common::TableReference;

/// # use datafusion_expr::{qualified_wildcard};

alamb · 2024-08-13T18:27:58Z

🚀

Thanks again @goldmedal and @jayzhan211

goldmedal · 2024-08-14T01:46:31Z

Thanks @alamb @jayzhan211 ❤️

alamb · 2024-08-14T21:24:06Z

Man I could spend all day reviewing DataFusion PRs these days. There is so much good stuff going on

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules labels Jul 27, 2024

goldmedal commented Jul 27, 2024

View reviewed changes

datafusion/sql/src/select.rs Outdated Show resolved Hide resolved

goldmedal mentioned this pull request Jul 27, 2024

Allow custom planning behavior for selecting wildcard expression #11639

Closed

github-actions bot added the core Core DataFusion crate label Jul 28, 2024

goldmedal added 7 commits August 4, 2024 23:39

allow qualified wildcard in the logical plan

cee3a11

move wildcard expansions to the analyzer

98ef466

fix fmt

3b1c39a

fix the view tests

f3b642a

expand wildcard for schema

01382f1

fix for union query

63ce513

cargo fmt clippy

59cf620

goldmedal force-pushed the feature/11639-add-expand-rule branch from bdf474e to 59cf620 Compare August 5, 2024 16:06

goldmedal added 4 commits August 6, 2024 00:21

move wildcard expanding tests to expand_wildcard_rule.rs

8775862

coercion the expanded wildcard expression in union

8cc5553

remove debug message

718b720

move wildcard options to logical plan

623777c

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 7, 2024

goldmedal added 5 commits August 7, 2024 21:29

remove unused function

50d2e3e

add the doc for expression function

9410c1b

fix cargo check

0b2bba9

fix cargo fmt

36a2e1c

fix test

dd487e5

goldmedal mentioned this pull request Aug 9, 2024

Non-aggregation expressions should be included in the grouping keys #11903

Closed

goldmedal added 5 commits August 10, 2024 20:34

extract expand_exprlist

f01c0dd

expand wildcard for functional_dependencies

4aa8e64

refine the doc

89c1b3d

fix tests

20479ec

fix expand exclude and except

15dddf6

goldmedal added 7 commits August 11, 2024 22:24

fix clippy and support unparsing wildcard

55f13e3

fix clippy and fmt

7272349

add the doc for util functions

3b55265

fix unique expression check for projection

03b9b19

Merge branch 'main' into feature/11639-add-expand-rule

c3503e7

cargo fmt

d21063a

move test and solve dependency issue

e461492

goldmedal commented Aug 12, 2024

View reviewed changes

goldmedal marked this pull request as ready for review August 12, 2024 17:50

jayzhan211 approved these changes Aug 13, 2024

View reviewed changes

goldmedal added 2 commits August 13, 2024 18:37

Merge branch 'main' into feature/11639-add-expand-rule

28278d1

address review comments

5e24932

github-actions bot added the proto Related to proto crate label Aug 13, 2024

add the fail reason

74cc393

alamb reviewed Aug 13, 2024

View reviewed changes

goldmedal added 2 commits August 13, 2024 21:33

enhance the doc

2324fa3

add more doc

81d66f4

alamb merged commit 3438b35 into apache:main Aug 13, 2024
24 checks passed

goldmedal deleted the feature/11639-add-expand-rule branch August 14, 2024 01:46

goldmedal mentioned this pull request Aug 17, 2024

Invalid aggregate SQL query with HAVING can be executed without error (SQLancer-TLP) #12013

Closed

This was referenced Aug 19, 2024

Bug in Functional Dependency Calculation for Projection #12054

Closed

Fix: Remove Unrelated Fields When Expanding Wildcards in Functional Dependency Projections #12060

Merged

jonahgao mentioned this pull request Aug 22, 2024

Handle downstream impacts to union's behavioral changes. #12105

Closed

ahirner mentioned this pull request Sep 30, 2024

unnest errors in conjunction with SELECT * #12684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move wildcard expansions to the analyzer #11681

Move wildcard expansions to the analyzer #11681

goldmedal commented Jul 27, 2024 •

edited

Loading

goldmedal commented Aug 12, 2024

goldmedal Aug 12, 2024

goldmedal Aug 12, 2024

goldmedal commented Aug 12, 2024

jayzhan211 left a comment

jayzhan211 Aug 13, 2024

jayzhan211 Aug 13, 2024

jayzhan211 Aug 13, 2024

jayzhan211 Aug 13, 2024

jayzhan211 Aug 13, 2024

jayzhan211 Aug 13, 2024

alamb left a comment

alamb Aug 13, 2024

goldmedal Aug 13, 2024

alamb Aug 13, 2024

alamb commented Aug 13, 2024

goldmedal commented Aug 14, 2024

alamb commented Aug 14, 2024 •

edited

Loading

	// TODO: move it into analyzer
	let input_schema = plan.schema();
	let mut projected_expr = vec![];
	for e in expr {
	let e = e.into();
	match e {
	Expr::Wildcard { qualifier: None } => {
	projected_expr.extend(expand_wildcard(input_schema, &plan, None)?)
	}
	Expr::Wildcard {
	qualifier: Some(qualifier),
	} => projected_expr.extend(expand_qualified_wildcard(
	&qualifier,
	input_schema,
	None,
	)?),

		/// use datafusion_common::TableReference;
		/// use datafusion_expr::{qualified_wildcard};

Move wildcard expansions to the analyzer #11681

Move wildcard expansions to the analyzer #11681

Conversation

goldmedal commented Jul 27, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Expanding expressions to fields for the schema of the Projection plan

Handle WildacrdAddiotionsOption for replace, exclude, or expect, ...

Type_coercion for union

Unparsing Expr::Wildcard

Are these changes tested?

Are there any user-facing changes?

goldmedal commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldmedal commented Aug 12, 2024

jayzhan211 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 13, 2024

goldmedal commented Aug 14, 2024

alamb commented Aug 14, 2024 • edited Loading

goldmedal commented Jul 27, 2024 •

edited

Loading

Handle `WildacrdAddiotionsOption` for replace, exclude, or expect, ...

alamb commented Aug 14, 2024 •

edited

Loading