[SPARK-44355][SQL] Move WithCTE into command queries #42036

cloud-fan · 2023-07-17T07:40:34Z

This PR is based on #42022 to fix tests, as the PR author is on vacation.

What changes were proposed in this pull request?

In the PR, I propose to add new trait CTEInChildren and mix it to some commands that should have WithCTE on top of their children (queries) instead of main query. Also I modified the CTESubstitution rule and removed special handling of Commands and similar nodes. After the changes, Command, ParsedStatement and InsertIntoDir are handled in the same way as other queries by referring to CTE Defs. Only the difference is in WithCTE is not not placed on the top of main query but on top of command queries.

Closes #41922

Why are the changes needed?

To improve code maintenance. Right now the CTE resolution code path is diverged: query with commands go into CTE inline code path where non-command queries go into CTEDef code path.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running new test:

$ build/sbt "test:testOnly *InsertSuite"

gengliangwang · 2023-07-21T04:38:41Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala

+        // CTE normally and don't need to force inline.
+        !commands.head.isInstanceOf[CTEInChildren]
+      } else if (commands.length > 1) {
+        // This can happen with the multi-insert statement. We should fall back to


Nit: There is duplicated logic here.
To make the code more readable, we can always collect the commands first. If the length of commands is 1, there is a different behavior based on the legacy conf. Otherwise the logic is determined.

gengliangwang · 2023-07-21T04:39:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala

+   * children. There are two reasons:
+   *  1. Some rules will pattern match the root command nodes, and we should keep command
+   *     as the root node to not break them.
+   *  2. `Dataset` eagerly executes the commands inside a query plan. However, the CTE


Nit: Shall we have an example for the eager execution?

gengliangwang · 2023-07-21T04:41:16Z

sql/core/src/test/resources/sql-tests/inputs/cte-command.sql

@@ -0,0 +1,33 @@
+-- WITH inside CTE


QQ: Is there a case will fail before the code change?

no, but the analyzed plan is different, as we always inline CTE before.

gengliangwang · 2023-07-21T04:43:58Z

LGTM over

peter-toth

Looks good to me, just minor comments.

peter-toth · 2023-07-24T09:01:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

  override def left: LogicalPlan = child
  override def right: LogicalPlan = query
  override protected def withNewChildrenInternal(
      newLeft: LogicalPlan, newRight: LogicalPlan): LogicalPlan =
    copy(child = newLeft, query = newRight)
+
+  override def withCTEDefs(cteDefs: Seq[CTERelationDef]): LogicalPlan = {
+    withNewChildren(Seq(child, WithCTE(query, cteDefs)))


Why not just copy(query = WithCTE(... like at other places?

withNewChildren can copy over the tree node tags.

peter-toth · 2023-07-24T09:01:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      "legacy behavior which may produce incorrect results because Spark may evaluate a CTE " +
+      "relation more than once, even if it's nondeterministic.")
+    .booleanConf
+    .createWithDefault(false)


Do we need version here?

good catch!

peter-toth · 2023-07-24T09:08:59Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+ */
+trait CTEInChildren extends LogicalPlan {
+  def withCTEDefs(cteDefs: Seq[CTERelationDef]): LogicalPlan = {
+    withNewChildren(children.map(WithCTE(_, cteDefs)))


I wonder if it makes sense to assert that we have only 1 child.

It seems fine to have multiple children, we just duplicate the CTE relations. The current code does not allow it though, and go back to inline CTE.

I'm not sure that it is always fine to duplicate CTE relations into multiple childrens.
For example, if we have a non-deterministic relation definition and 1-1 reference to it in 2 childrens of CTEInChildren and then here we duplicate the relations into the 2 childrens then I think the InlineCTE rule will decide to inline the relation 2 times, which is not correct.
But I agree with you, I don't see that this could happen now.

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

…onf.scala

cloud-fan · 2023-07-25T13:52:50Z

the test failure is unrelated, I'm merging it to master, thanks for the review!

This PR is based on apache#42022 to fix tests, as the PR author is on vacation. ### What changes were proposed in this pull request? In the PR, I propose to add new trait `CTEInChildren` and mix it to some commands that should have `WithCTE` on top of their children (queries) instead of main query. Also I modified the `CTESubstitution` rule and removed special handling of `Command`s and similar nodes. After the changes, `Command`, `ParsedStatement` and `InsertIntoDir` are handled in the same way as other queries by referring to CTE Defs. Only the difference is in `WithCTE` is not not placed on the top of main query but on top of command queries. Closes apache#41922 ### Why are the changes needed? To improve code maintenance. Right now the CTE resolution code path is diverged: query with commands go into CTE inline code path where non-command queries go into CTEDef code path. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By running new test: ``` $ build/sbt "test:testOnly *InsertSuite" ``` Closes apache#42036 from cloud-fan/help. Lead-authored-by: Max Gekk <max.gekk@gmail.com> Co-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… original WithCTE node ### What changes were proposed in this pull request? I noticed an outdated comment in the rule `InlineCTE` ``` // CTEs in SQL Commands have been inlined by `CTESubstitution` already, so it is safe to add // WithCTE as top node here. ``` This is not true anymore after #42036 . It's not a big deal as we replace not-inlined CTE relations with `Repartition` during optimization, so it doesn't matter where we put the `WithCTE` node with not-inlined CTE relations, as it will disappear eventually. But it's still better to keep it at its original place, as third-party rules may be sensitive about the plan shape. ### Why are the changes needed? to keep the plan shape as much as can after inlining CTE relations. ### 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 #46617 from cloud-fan/cte. Lead-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

MaxGekk added 6 commits July 16, 2023 12:26

Add WithCTEInChildren

4eabbd8

with WithCTEInChildren

4349764

Rename WithCTEInChildren to CTEInChildren

4f1cbee

Remove isCommand

a6633e4

Add withCTEDefs()

25230f2

Fix coding style issues

58bc261

github-actions bot added the SQL label Jul 17, 2023

fix

fedc9a6

cloud-fan force-pushed the help branch from db671db to fedc9a6 Compare July 17, 2023 08:27

gengliangwang reviewed Jul 21, 2023

View reviewed changes

address comments

45672ac

peter-toth approved these changes Jul 24, 2023

View reviewed changes

gengliangwang approved these changes Jul 24, 2023

View reviewed changes

cloud-fan commented Jul 25, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Show resolved Hide resolved

Update sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLC…

8a36db1

…onf.scala

cloud-fan closed this in da84f81 Jul 25, 2023

peter-toth mentioned this pull request Oct 15, 2023

[SPARK-24497][SQL] Support recursive SQL #40744

Closed

cloud-fan mentioned this pull request May 16, 2024

[SPARK-48307][SQL] InlineCTE should keep not-inlined relations in the original WithCTE node #46617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44355][SQL] Move WithCTE into command queries #42036

[SPARK-44355][SQL] Move WithCTE into command queries #42036

cloud-fan commented Jul 17, 2023

gengliangwang Jul 21, 2023

gengliangwang Jul 21, 2023

gengliangwang Jul 21, 2023

cloud-fan Jul 25, 2023

gengliangwang commented Jul 21, 2023

peter-toth left a comment •

edited

Loading

peter-toth Jul 24, 2023

cloud-fan Jul 25, 2023

peter-toth Jul 24, 2023

cloud-fan Jul 25, 2023

peter-toth Jul 24, 2023

cloud-fan Jul 25, 2023

peter-toth Jul 25, 2023 •

edited

Loading

cloud-fan commented Jul 25, 2023

[SPARK-44355][SQL] Move WithCTE into command queries #42036

[SPARK-44355][SQL] Move WithCTE into command queries #42036

Conversation

cloud-fan commented Jul 17, 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?

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

gengliangwang commented Jul 21, 2023

peter-toth left a comment • edited Loading

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

peter-toth Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

cloud-fan commented Jul 25, 2023

peter-toth left a comment •

edited

Loading

peter-toth Jul 25, 2023 •

edited

Loading