Adding complex expressions projections for Subquery #9719

Lordworms · 2024-03-21T01:12:22Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Added a top-down-top tree-node traverse in all plan, find complex_expressions in all LogicalPlan, try to add an extra Projection if this expression is needed in upper layer.

Are these changes tested?

Are there any user-facing changes?

waynexia

Thanks for taking this @Lordworms

One situation I came up with that makes eliminating common expr across plans tricky is how to keep track of column references. Consider a plan like:

Projection: a+1
--Projection: a+1 as a
----TableScan

Where two a+1 are different things actually, since two a are not the same thing. The result is wrong if we replace them with a pre-computed common expr.

datafusion/sqllogictest/test_files/project_complex_sub_query.slt

datafusion/optimizer/src/optimize_projections.rs

Lordworms · 2024-03-24T12:53:56Z

Thanks for taking this @Lordworms

One situation I came up with that makes eliminating common expr across plans tricky is how to keep track of column references. Consider a plan like:
Projection: a+1
--Projection: a+1 as a
----TableScan
Where two a+1 are different things actually, since two a are not the same thing. The result is wrong if we replace them with a pre-computed common expr.

Thanks, I'll record this and fix in the following PR

mustafasrepo · 2024-03-26T07:17:04Z

datafusion/sqllogictest/test_files/project_complex_sub_query.slt

+Projection: t.c3 + t.c4, SUM(t.c3 + t.c4) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
+--WindowAggr: windowExpr=[[SUM(CAST(t.c3 + t.c4 AS Int64)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]]
+----Projection: t.c3 + t.c4 AS t.c3 + t.c4, t.c3, t.c4
+------TableScan: t projection=[c3, c4]


I double checked this plan. It seems that expression t.c3+t.c4 in the top projection still is a binary expression (Unfortunately from display it is not visible whether it is column or binary expression). We should have convert it to its pre-evaluated column version: Col(t.c3+t.c4). If that was the case, I would expect to see following logical plan

Projection: t.c3 + t.c4, SUM(t.c3 + t.c4) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING --WindowAggr: windowExpr=[[SUM(CAST(t.c3 + t.c4 AS Int64)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]] ----Projection: t.c3 + t.c4 AS t.c3 + t.c4 ------TableScan: t projection=[c3, c4]

where columns t.c3 and t.c4 are pruned at the first projection (from bottom). Since they are no longer needed by subsequent stages. I will try to solve this problem. If I can come up with a solution I will let you know.

Sure, in this case, I think the easiest way to do this is to delete the t.c3 and t.c4 columns in the upper projection schema. (which could be done when traversing from top to down). Will you do it or let me try it?

Feel free to go ahead.

I think same problem still occurs in the LogicalPlan::Window where inner expression of the Cast is binary expression. I would expect Projection immediately top of the TableScan to only include expression: t.c3 + t.c4 AS t.c3 + t.c4 because. Subsequent stages doesn't refer t.c3 or t.c4. If you rewrite the expression for window also (as in projection). We should get that plan.
In short we have to rewrite expressions that contribute to common expression count during f_down pass.

mustafasrepo · 2024-03-26T07:20:14Z

datafusion/sqllogictest/test_files/project_complex_sub_query.slt

+)
+STORED AS CSV
+WITH HEADER ROW
+LOCATION '../core/tests/data/demo.csv';


since demo.csv file is not added to this PR. Following tests fails to produce PhysicalPlan. I think, it is better to add this file also. We can still test LogicalPlans in the test by using the flag set datafusion.explain.logical_plan_only = true;

OK, I'll add that

Lordworms · 2024-03-26T19:46:43Z

I think right now it is fine, I'll run the tests

Lordworms · 2024-03-28T01:03:25Z

@mustafasrepo Hi If you have a moment, I would be honored to have your review on my updated one.

mustafasrepo · 2024-03-28T06:14:01Z

@mustafasrepo Hi If you have a moment, I would be honored to have your review on my updated one.

Sure, I will look into this PR.

Feature/pr9719 exp

Lordworms · 2024-03-29T01:03:49Z

Thanks @mustafasrepo , I double-checked the results and it meet the requirements. I actually have one question, why we could still get the proper answer with eliminating the upper ProjectionExec?(I think it is optimized since another logical plan optimization would eliminate the same Projection) I think it is ready to be merged now. Also, I think we could use similar way to track Aggregate, I could make an extra ticket here and solve it in the coming PR.

mustafasrepo · 2024-03-29T06:15:11Z

Thanks @mustafasrepo , I double-checked the results and it meet the requirements. I actually have one question, why we could still get the proper answer with eliminating the upper ProjectionExec?(I think it is optimized since another logical plan optimization would eliminate the same Projection) I think it is ready to be merged now. Also, I think we could use similar way to track Aggregate, I could make an extra ticket here and solve it in the coming PR.

Exactly, the optimize_projections rule (for LogicalPlan) removes the unnecessary LogicalPlan::Projections from the plan if those projections have same input and same output schema. In this example, this is the case also. LogicalPlan::window has already desired schema for the user input.

mustafasrepo

LGTM!. Thanks @Lordworms for this work. It is really nice to collaborate with you. Another look is appreciated though. If any reviewer has time.

Lordworms · 2024-03-29T14:22:01Z

LGTM!. Thanks @Lordworms for this work. It is really nice to collaborate with you. Another look is appreciated though. If any reviewer has time.

Thank you!

andygrove · 2024-04-15T15:04:37Z

datafusion/optimizer/src/common_subexpr_eliminate.rs

@@ -423,6 +417,34 @@ impl OptimizerRule for CommonSubexprEliminate {
            plan => Ok(plan),
        }
    }
+    /// currently the implemention is not optimal, Basically I just do a top-down iteration over all the


This comment seems to be incomplete?

Sorry for that, I'll complete it

andygrove · 2024-04-15T15:06:02Z

datafusion/optimizer/src/common_subexpr_eliminate.rs

+}
+
+impl ProjectionAdder {
+    // TODO: adding more expressions for sub query, currently only support for Simple Binary Expressions


Is there a tracking issue for this that we could refer to in the comment?

I'll add that

alamb · 2024-04-29T19:11:59Z

I am trying to clean up our old PRs -- it looks to me like @mustafasrepo approved and asked for another reviewer #9719 (review). However it isn't clear to me if this PR needs another review before merge or if that is a nice to have.

I don't think we can merge this PR without some work to resolve the conflicts, but I don't want to waste people's time resolving conflicts if it isn't ready to merge.

Please let us know your thoughts

mustafasrepo · 2024-04-30T05:51:24Z

However it isn't clear to me if this PR needs another review before merge or if that is a nice to have.

While asking for another review, What I had in mind was "nice to have". I think, once conflicts are resolved we can merge this PR as is.

waynexia

Thank you all for working on this ❤️

I've reviewed it again, it's mostly LGTM. +1 for merging this once the conflict and remaining comments are solved.

waynexia · 2024-05-01T08:33:13Z

datafusion/optimizer/src/common_subexpr_eliminate.rs

+}
+pub fn is_not_complex(op: &Operator) -> bool {
+    matches!(
+        op,


Sugg:

Operator::LtEq and Operator::GtEq can also be sonsidered not complex

Operator is Copy, so changing the function parameter to op: Operator seems better

waynexia · 2024-05-01T08:56:23Z

datafusion/optimizer/src/common_subexpr_eliminate.rs

+                    let f =
+                        DFField::new_unqualified(&expr.to_string(), dtype.clone(), true);
+                    field_set.insert(f.name().to_owned());


Since only field name is used, we can skip constructing the DFField?

Suggested change

let f =

DFField::new_unqualified(&expr.to_string(), dtype.clone(), true);

field_set.insert(f.name().to_owned());

field_set.insert(expr.to_string());

alamb · 2024-05-17T00:34:34Z

Marking this one as a draft as I think it needs to have conflicts resolved before we can merge it

alamb · 2024-05-17T00:34:37Z

Marking this one as a draft as I think it needs to have conflicts resolved before we can merge it

github-actions · 2024-07-16T01:52:00Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

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

Lordworms added 3 commits March 21, 2024 20:15

for debug

f495921

adding projections for subquery

cc1d412

refactor code and add tests

1c64d23

Lordworms force-pushed the issue_9576 branch from a92072f to 1c64d23 Compare March 22, 2024 01:18

optimize code

3e96839

github-actions bot added the core Core DataFusion crate label Mar 22, 2024

remove useless print

71b5bd7

github-actions bot removed the core Core DataFusion crate label Mar 22, 2024

remove print

30d29a0

github-actions bot removed the sql SQL Planner label Mar 22, 2024

Lordworms added 4 commits March 21, 2024 21:32

optimize code

81523d1

optimize code

1c4010b

adding test

4add9cb

fix clippy

251c48d

Lordworms marked this pull request as ready for review March 22, 2024 15:31

alamb requested review from waynexia and mustafasrepo March 22, 2024 18:46

waynexia reviewed Mar 24, 2024

View reviewed changes

datafusion/sqllogictest/test_files/project_complex_sub_query.slt Outdated Show resolved Hide resolved

datafusion/optimizer/src/optimize_projections.rs Outdated Show resolved Hide resolved

Lordworms mentioned this pull request Mar 24, 2024

track of column references for common expr #9776

Open

optimize code

117e490

mustafasrepo reviewed Mar 26, 2024

View reviewed changes

optimize

113e232

mustafasrepo added 6 commits March 28, 2024 11:24

Tmp

b77efab

slt tests pass

59e0385

Remove unnecessary test

c1a16f9

Remove unnecessary changes

fb9e4d6

Resolve linter errors

bdf626a

Merge branch 'issue_9576' into feature/pr9719_exp

2c70a15

mustafasrepo mentioned this pull request Mar 28, 2024

Feature/pr9719 exp Lordworms/arrow-datafusion#1

Merged

mustafasrepo and others added 5 commits March 28, 2024 14:20

Remove left over file

31cc2d7

Minor changes

e7f4b49

Remove unnecessary changes

313cccd

Update comment

759226b

Merge pull request #1 from synnada-ai/feature/pr9719_exp

4596f90

Feature/pr9719 exp

mustafasrepo approved these changes Mar 29, 2024

View reviewed changes

waynexia mentioned this pull request Mar 31, 2024

fix(9870): common expression elimination optimization, should always re-find the correct expression during re-write. #9871

Merged

andygrove reviewed Apr 15, 2024

View reviewed changes

waynexia approved these changes May 1, 2024

View reviewed changes

alamb marked this pull request as draft May 17, 2024 00:34

github-actions bot added the Stale PR has not had any activity for some time label Jul 16, 2024

github-actions bot closed this Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding complex expressions projections for Subquery #9719

Adding complex expressions projections for Subquery #9719

Lordworms commented Mar 21, 2024 •

edited

Loading

waynexia left a comment

Lordworms commented Mar 24, 2024

mustafasrepo Mar 26, 2024

Lordworms Mar 26, 2024

mustafasrepo Mar 26, 2024

Lordworms Mar 26, 2024

mustafasrepo Mar 28, 2024

mustafasrepo Mar 26, 2024

Lordworms Mar 26, 2024

Lordworms commented Mar 26, 2024

Lordworms commented Mar 28, 2024

mustafasrepo commented Mar 28, 2024

Lordworms commented Mar 29, 2024 •

edited

Loading

mustafasrepo commented Mar 29, 2024

mustafasrepo left a comment

Lordworms commented Mar 29, 2024

andygrove Apr 15, 2024

Lordworms Apr 15, 2024

andygrove Apr 15, 2024

Lordworms Apr 15, 2024

alamb commented Apr 29, 2024

mustafasrepo commented Apr 30, 2024 •

edited

Loading

waynexia left a comment

waynexia May 1, 2024

waynexia May 1, 2024

alamb commented May 17, 2024

alamb commented May 17, 2024

github-actions bot commented Jul 16, 2024

Adding complex expressions projections for Subquery #9719

Adding complex expressions projections for Subquery #9719

Conversation

Lordworms commented Mar 21, 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?

waynexia left a comment

Choose a reason for hiding this comment

Lordworms commented Mar 24, 2024

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

Lordworms commented Mar 26, 2024

Lordworms commented Mar 28, 2024

mustafasrepo commented Mar 28, 2024

Lordworms commented Mar 29, 2024 • edited Loading

mustafasrepo commented Mar 29, 2024

mustafasrepo left a comment

Choose a reason for hiding this comment

Lordworms commented Mar 29, 2024

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 Apr 29, 2024

mustafasrepo commented Apr 30, 2024 • edited Loading

waynexia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 17, 2024

alamb commented May 17, 2024

github-actions bot commented Jul 16, 2024

Lordworms commented Mar 21, 2024 •

edited

Loading

Lordworms commented Mar 29, 2024 •

edited

Loading

mustafasrepo commented Apr 30, 2024 •

edited

Loading