[SPARK-45760][SQL] Add With expression to avoid duplicating expressions #43623

cloud-fan · 2023-11-01T16:03:20Z

What changes were proposed in this pull request?

Sometimes we need to duplicate expressions when rewriting the plan. It's OK for small query, as codegen has common-subexpression-elimination (CSE) to avoid evaluating the same expression. However, when the query is big, duplicating expressions can lead to a very big expression tree and make catalyst rules very slow, or even OOM when updating a leaf node (need to copy all tree nodes).

This PR introduces a new expression to do expression-level CTE: it adds a Project to pre-evaluate the common expressions, so that they appear only once on the query plan tree, and are evaluated only once. NullIf now uses this new expression to avoid duplicating the left child expression.

Why are the changes needed?

make catalyst more efficient.

Does this PR introduce any user-facing change?

No

How was this patch tested?

new test suite

Was this patch authored or co-authored using generative AI tooling?

No

cloud-fan · 2023-11-01T16:03:37Z

cc @viirya @wangyum

viirya · 2023-11-01T19:46:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala

+        }
+
+        var exprsToAdd = commonExprs.toSeq
+        val newChildren = p.children.map { child =>


Shouldn't be?

Suggested change

val newChildren = p.children.map { child =>

val newChildren = newPlan.children.map { child =>

it's the same, the new plan was produced by transformExpressionsDown so the children won't change.

viirya · 2023-11-01T19:46:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala

+        }
+
+        if (exprsToAdd.nonEmpty) {
+          // If we cannot rewrite the common expressions, force to inline them so that the query


When we cannot rewrite them?

viirya

This looks good. It is like an explicit/manual CSE? I wonder if we want to expose this as a expression function?

cloud-fan · 2023-11-02T07:35:16Z

@viirya yes we can add SQL syntax in the future, following https://github.com/google/zetasql/blob/a745bef47b315bb11fecab4eeefa2bcc41be5951/docs/operators.md?plain=1#L2865

beliefer · 2023-11-02T09:25:15Z

Why not add an optimizer rule to find the common expressions and insert a Project to pre-evaluate?
It seems this PR need we update each expression with With expression one by one.

cloud-fan · 2023-11-02T09:30:35Z

Why not add an optimizer rule to find the common expressions and insert a Project to pre-evaluate?

We need this With expression anyway to support the WITH syntax in google bigquery, and it's safer to use it incrementally to avoid expression duplication. It's also easier to build it incrementally as the agg/window function support can be added later.

cloud-fan · 2023-11-02T09:34:47Z

BTW, I feel it's useful to have a way to do explicit/manual CSE, instead of relying on optimizer features or codegen features.

wangyum · 2023-11-02T15:04:52Z

.../connect/server/src/test/scala/org/apache/spark/sql/connect/ProtoToParsedPlanTestSuite.scala

+        object Helper extends RuleExecutor[LogicalPlan] {
+          val batches =
+            Batch("Finish Analysis", Once, ReplaceExpressions) ::
+            Batch("Rewrite With expression", FixedPoint(10), RewriteWithExpression) :: Nil


Please fix the Scala style.

viirya

Looks good. Seems there are still test failures?

peter-toth · 2023-11-05T12:01:47Z

Looks good to me, but I think we can make the new rule idempotent with a small refactor: cloud-fan#19

Kimahriman · 2023-11-05T13:37:01Z

What's the longer term goal with this, especially in the context of all the attempts to add CSE to conditional expressions? Is the idea to make a with Column expression that can be manually used? Honestly when I started with Spark I assumed this was the default behavior if you re-used a Column object multiple times in an expression.

Just out of curiosity, since this is basically manual CSE at the optimizer stage, would it make sense to do the existing fully recursive, automatic CSE at the optimizer stage instead of the physical stage to achieve a similar affect?

cloud-fan · 2023-11-06T04:45:44Z

What's the longer term goal with this, especially in the context of all the attempts to add CSE to conditional expressions?

I think the final state should be implementing CSE at the logical plan level, so that it works for both codegen backend and native (vectorized) backend. But we still have gaps now. The With expression only works for operators that can add extra projects. It doesn't work for join conditions, while codegen is more flexible. Also codegen CSE is more adaptive and can be used for conditional expressions, but With is static. Before filling the gaps, With can't replace the current codegen CSE.

* make RewriteWithExpression idempotent * restore def index usage in alias, minor change to shorten code

cloud-fan · 2023-11-07T16:56:49Z

The doc generation issue is unrelated to my PR

ImportError: Warning: Latest version of pandas (2.1.2) is required to generate the documentation; however, your version was 2.1.1

I think we need to upgrade pandas version on GA machines. cc @HyukjinKwon @LuciferYang

cloud-fan · 2023-11-07T16:57:02Z

I'm merging it to master, thanks for the reviews!

LuciferYang · 2023-11-07T17:00:11Z

The doc generation issue is unrelated to my PR
ImportError: Warning: Latest version of pandas (2.1.2) is required to generate the documentation; however, your version was 2.1.1
I think we need to upgrade pandas version on GA machines. cc @HyukjinKwon @LuciferYang

Already upgrade: #43689 :)

### What changes were proposed in this pull request? This is a followup of #43623 to fix a regression. For `With` inside conditional branches, they may not be evaluated at all and we should not pull out the common expressions into a `Project`, but just inline. ### Why are the changes needed? avoid perf regression ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? No Closes #43978 from cloud-fan/with. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? This is a followup of apache/spark#43623 to fix a regression. For `With` inside conditional branches, they may not be evaluated at all and we should not pull out the common expressions into a `Project`, but just inline. ### Why are the changes needed? avoid perf regression ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? No Closes #43978 from cloud-fan/with. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

github-actions bot added the SQL label Nov 1, 2023

cloud-fan force-pushed the with branch from ea59da0 to 0be561f Compare November 1, 2023 16:19

Add With expression to avoid duplicating expressions

3de0433

cloud-fan force-pushed the with branch from 0be561f to 3de0433 Compare November 1, 2023 16:42

viirya reviewed Nov 1, 2023

View reviewed changes

fix

264cccd

github-actions bot added the CONNECT label Nov 2, 2023

wangyum reviewed Nov 2, 2023

View reviewed changes

fix style

90b424d

wangyum approved these changes Nov 2, 2023

View reviewed changes

viirya approved these changes Nov 3, 2023

View reviewed changes

ulysses-you approved these changes Nov 3, 2023

View reviewed changes

make it stable

2316fac

cloud-fan force-pushed the with branch from 759f4d9 to 2316fac Compare November 3, 2023 13:28

beliefer approved these changes Nov 3, 2023

View reviewed changes

make RewriteWithExpression idempotent (#19)

6cae9f9

* make RewriteWithExpression idempotent * restore def index usage in alias, minor change to shorten code

cloud-fan closed this in 01c294b Nov 7, 2023

cloud-fan mentioned this pull request Nov 23, 2023

[SPARK-45760][SQL][FOLLOWUP] Inline With inside conditional branches #43978

Closed

kazuyukitanimura mentioned this pull request Jun 3, 2024

build: Enable comet tests with spark-4.0 profile apache/datafusion-comet#493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-45760][SQL] Add With expression to avoid duplicating expressions #43623

[SPARK-45760][SQL] Add With expression to avoid duplicating expressions #43623

cloud-fan commented Nov 1, 2023

cloud-fan commented Nov 1, 2023

viirya Nov 1, 2023

cloud-fan Nov 2, 2023

viirya Nov 1, 2023

viirya left a comment

cloud-fan commented Nov 2, 2023

beliefer commented Nov 2, 2023 •

edited

Loading

cloud-fan commented Nov 2, 2023

cloud-fan commented Nov 2, 2023

wangyum Nov 2, 2023

viirya left a comment

peter-toth commented Nov 5, 2023

Kimahriman commented Nov 5, 2023

cloud-fan commented Nov 6, 2023

cloud-fan commented Nov 7, 2023

cloud-fan commented Nov 7, 2023

LuciferYang commented Nov 7, 2023

	val newChildren = p.children.map { child =>
	val newChildren = newPlan.children.map { child =>

[SPARK-45760][SQL] Add With expression to avoid duplicating expressions #43623

[SPARK-45760][SQL] Add With expression to avoid duplicating expressions #43623

Conversation

cloud-fan commented Nov 1, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

cloud-fan commented Nov 1, 2023

viirya Nov 1, 2023

Choose a reason for hiding this comment

cloud-fan Nov 2, 2023

Choose a reason for hiding this comment

viirya Nov 1, 2023

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

cloud-fan commented Nov 2, 2023

beliefer commented Nov 2, 2023 • edited Loading

cloud-fan commented Nov 2, 2023

cloud-fan commented Nov 2, 2023

wangyum Nov 2, 2023

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

peter-toth commented Nov 5, 2023

Kimahriman commented Nov 5, 2023

cloud-fan commented Nov 6, 2023

cloud-fan commented Nov 7, 2023

cloud-fan commented Nov 7, 2023

LuciferYang commented Nov 7, 2023

beliefer commented Nov 2, 2023 •

edited

Loading