Skip to content

Conversation

@allisonwang-db
Copy link
Contributor

What changes were proposed in this pull request?

This PR refactors checkCorrelationsInSubquery in CheckAnalysis to use recursion instead of foreachUp.

Why are the changes needed?

Currently, the logic in checkCorrelationsInSubquery is inefficient and difficult to understand. It uses foreachUp to traverse the subquery plan tree, and traverses down an entire subtree of a plan node to check whether it contains any outer references. We can use recursion instead to traverse the plan tree only once to improve the performance and readability.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing unit tests.

@github-actions github-actions bot added the SQL label Oct 12, 2022
@allisonwang-db
Copy link
Contributor Author

cc @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 069967b Oct 13, 2022
Copy link
Contributor

@amaliujia amaliujia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late but LGTM!

SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
### What changes were proposed in this pull request?

This PR refactors `checkCorrelationsInSubquery` in CheckAnalysis to use recursion instead of `foreachUp`.

### Why are the changes needed?

Currently, the logic in `checkCorrelationsInSubquery` is inefficient and difficult to understand. It uses `foreachUp` to traverse the subquery plan tree, and traverses down an entire subtree of a plan node to check whether it contains any outer references. We can use recursion instead to traverse the plan tree only once to improve the performance and readability.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Existing unit tests.

Closes apache#38226 from allisonwang-db/spark-40773-check-subquery.

Authored-by: allisonwang-db <[email protected]>
Signed-off-by: Wenchen Fan <[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.

3 participants