Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jul 16, 2023

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"

@github-actions github-actions bot added the SQL label Jul 16, 2023
@MaxGekk MaxGekk changed the title [WIP][SQL] Move WithCTE into command queries [WIP][SPARK-44355][SQL] Move WithCTE into command queries Jul 16, 2023
cloud-fan added a commit that referenced this pull request Jul 25, 2023
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 `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 #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 #42036 from cloud-fan/help.

Lead-authored-by: Max Gekk <[email protected]>
Co-authored-by: Wenchen Fan <[email protected]>
Co-authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@MaxGekk MaxGekk closed this Aug 2, 2023
turboFei pushed a commit to turboFei/spark that referenced this pull request Nov 6, 2025
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 <[email protected]>
Co-authored-by: Wenchen Fan <[email protected]>
Co-authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>

(cherry picked from commit da84f81)

Co-authored-by: Max Gekk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant