Move `sql_compound_identifier_to_expr` to `ExprPlanner` #11487

dharanad · 2024-07-16T05:02:23Z

Which issue does this PR close?

Rationale for this change

Part of #11207 moving the planning of compound_identifier to ExprPlanner, this change does not include planning for compound_identifier in outer query

What changes are included in this PR?

Moved the planning of compound expression to ExprPlanner
Moved testcases in optimizer crate dependent on functions to datafusion/core/tests/optimizer_integration.rs.
Added expression planner field to MockContextProvider to fix the breaking testcases.

Are these changes tested?

Existing test cases

cargo test

Are there any user-facing changes?

No

jayzhan211 · 2024-07-16T09:57:23Z

datafusion/sql/src/expr/identifier.rs

@@ -135,16 +138,17 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                    let nested_name = nested_names[0].to_string();

                    let col = Expr::Column(Column::from((qualifier, field)));
-                    if let Some(udf) =
-                        self.context_provider.get_function_meta("get_field")


I wonder could we plan the whole compound identifier, the benefit of plan_* function is able to customize the operator (in this case is compound identifier i.e. a.b.c) and expressions we get.

Ideally i wanted to do that same, but this function uses IdentNormalizer field of SqlToRel struct. I am just thinking a way around & wanted to discuss the same here.

We can use the result of self.normalizer.normalize() as the planner arguments, similar to the spirit of others function that we take the result of sql_expr_to_logical_expr as the planner arguments

How about this revision ? Do you think we aligned here ? There is still more work to do.

fn sql_compound_identifier_to_expr does many things, handles the logic for variable, compund indentifier & compund indentifier in outer query. In this change we are only supporting compund indentifier & not adding support compund indentifier in outer query.

dharanad · 2024-07-17T17:41:18Z

Weired that checks are failing, same are passing for me locally. Need to investigate

datafusion/sql/examples/sql.rs

datafusion/optimizer/tests/optimizer_integration.rs

dharanad · 2024-07-18T06:33:46Z

@jayzhan211 I apologize for any confusion caused by submitting multiple reviews. I was mistaken in believing I had the latest changes from the main branch. This revision incorporates the most recent updates.

datafusion/sql/tests/cases/plan_to_sql.rs

alamb

Thank you @dharanad and @jayzhan211 -- I think this PR is looking close. I had a few suggestions but I think it is quite close

It will be great to finally complete the user defined extension planner epic 🙏

datafusion-cli/Cargo.lock

datafusion/expr/src/planner.rs

datafusion/sql/src/expr/identifier.rs

datafusion/expr/src/planner.rs

# Conflicts: # datafusion/expr/src/planner.rs

alamb · 2024-07-19T21:33:04Z

datafusion/core/tests/optimizer_integration.rs

@@ -115,6 +117,139 @@ fn concat_ws_literals() -> Result<()> {
    Ok(())
 }

+#[test]
+fn anti_join_with_join_filter() -> Result<()> {


why are these tests moved? Does that mean we can't plan SQL with identifiers like test.col_int32 without an Expr planner?

For the earlier implementation, I also moved plannig of basic SQL like column references to expr planner, we had to move these test cases from optimizer crate to core to avoid optimizer crate dependecy on functions.

Since we have moved that out of planner, ideally i should moved the testcases back. My bad, i missed them. Thanks for catching this. I have fixed this.

alamb

Thanks @dharanad -- other than the one question about why the tests were moved, I think this PR is looking good to me.

jayzhan211

👍

alamb

Thanks @dharanad and @jayzhan211 -- I think this one looks really nice.

* move get_field to expr planner * formatting * formatting * documentation * refactor * documentation & fix's * move optimizer tests to core * fix breaking tc's * cleanup * fix examples * formatting * rm datafusion-functions from optimizer * update compound identifier * update planner * update planner * formatting * reverting optimizer tests * formatting

move get_field to expr planner

ba92a84

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions labels Jul 16, 2024

dharanad added 3 commits July 16, 2024 10:39

formatting

44118b9

formatting

404347e

documentation

1adc9a4

jayzhan211 reviewed Jul 16, 2024

View reviewed changes

refactor

5f4ac70

github-actions bot added the optimizer Optimizer rules label Jul 16, 2024

documentation & fix's

38debfc

dharanad changed the title ~~Move GetField to ExprPlanner~~ Move sql_compound_identifier_to_expr to ExprPlanner Jul 17, 2024

dharanad marked this pull request as ready for review July 17, 2024 17:31

dharanad requested a review from jayzhan211 July 17, 2024 17:38

dharanad commented Jul 17, 2024

View reviewed changes

datafusion/sql/examples/sql.rs Outdated Show resolved Hide resolved

jayzhan211 reviewed Jul 18, 2024

View reviewed changes

datafusion/optimizer/tests/optimizer_integration.rs Outdated Show resolved Hide resolved

move optimizer tests to core

6623219

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

dharanad added 5 commits July 18, 2024 10:05

Merge branch 'refs/heads/main' into move-get-field-expr-planner

040dc3c

fix breaking tc's

73cf5fa

cleanup

e4a7d7c

fix examples

7c52b3d

formatting

2ab18e4

dharanad requested a review from jayzhan211 July 18, 2024 06:31

jayzhan211 reviewed Jul 18, 2024

View reviewed changes

datafusion/sql/tests/cases/plan_to_sql.rs Show resolved Hide resolved

dharanad mentioned this pull request Jul 18, 2024

chore: Add SessionState to MockContextProvider just like SessionContextProvider #11531

Closed

alamb reviewed Jul 18, 2024

View reviewed changes

dharanad added 2 commits July 19, 2024 11:12

rm datafusion-functions from optimizer

9378d15

update compound identifier

1379cd7

dharanad added 4 commits July 19, 2024 11:34

update planner

d6c3590

update planner

d2fa19a

formatting

3076d54

Merge branch 'refs/heads/main' into move-get-field-expr-planner

091260f

# Conflicts: # datafusion/expr/src/planner.rs

dharanad requested a review from alamb July 19, 2024 19:53

alamb reviewed Jul 19, 2024

View reviewed changes

dharanad added 2 commits July 20, 2024 17:10

reverting optimizer tests

40ab63c

formatting

671ee93

github-actions bot removed optimizer Optimizer rules core Core DataFusion crate labels Jul 20, 2024

dharanad requested a review from alamb July 20, 2024 11:48

jayzhan211 approved these changes Jul 20, 2024

View reviewed changes

alamb approved these changes Jul 21, 2024

View reviewed changes

alamb merged commit 36660fe into apache:main Jul 21, 2024
24 checks passed

alamb mentioned this pull request Jul 21, 2024

Move sql_compound_identifier_to_expr to ExprPlanner #11473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `sql_compound_identifier_to_expr` to `ExprPlanner` #11487

Move `sql_compound_identifier_to_expr` to `ExprPlanner` #11487

dharanad commented Jul 16, 2024 •

edited

Loading

jayzhan211 Jul 16, 2024

dharanad Jul 16, 2024

jayzhan211 Jul 16, 2024

dharanad Jul 16, 2024

dharanad Jul 17, 2024

dharanad commented Jul 17, 2024

dharanad commented Jul 18, 2024

alamb left a comment

alamb Jul 19, 2024

dharanad Jul 20, 2024

alamb left a comment

jayzhan211 left a comment

alamb left a comment

Move sql_compound_identifier_to_expr to ExprPlanner #11487

Move sql_compound_identifier_to_expr to ExprPlanner #11487

Conversation

dharanad commented Jul 16, 2024 • edited Loading

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?

jayzhan211 Jul 16, 2024

Choose a reason for hiding this comment

dharanad Jul 16, 2024

Choose a reason for hiding this comment

jayzhan211 Jul 16, 2024

Choose a reason for hiding this comment

dharanad Jul 16, 2024

Choose a reason for hiding this comment

dharanad Jul 17, 2024

Choose a reason for hiding this comment

dharanad commented Jul 17, 2024

dharanad commented Jul 18, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 19, 2024

Choose a reason for hiding this comment

dharanad Jul 20, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Move `sql_compound_identifier_to_expr` to `ExprPlanner` #11487

Move `sql_compound_identifier_to_expr` to `ExprPlanner` #11487

dharanad commented Jul 16, 2024 •

edited

Loading