Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-28375][SQL] Make PullupCorrelatedPredicates idempotent #25145

Closed
wants to merge 1 commit into from

Conversation

mgaido91
Copy link
Contributor

@mgaido91 mgaido91 commented Jul 13, 2019

What changes were proposed in this pull request?

Multiple runs of PullupCorrelatedPredicates can produce invalid plans. Indeed, when there is a Subquery with a Filter a second run removes the conditions pulled by the previous run and overwrites them to nothing.

The PR avoids this issue and makes PullupCorrelatedPredicates idempotent.

How was this patch tested?

added UT

@SparkQA
Copy link

SparkQA commented Jul 13, 2019

Test build #107631 has finished for PR 25145 at commit d0a11d8.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@joshrosen-stripe
Copy link
Contributor

FYI @yeshengm also has a patch for this at #25164; posting this cross-reference here so reviewers are aware of both PRs and can work together to converge on a single fix.

@@ -279,9 +277,9 @@ object PullupCorrelatedPredicates extends Rule[LogicalPlan] with PredicateHelper
case Exists(sub, children, exprId) if children.nonEmpty =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we are not treating these two cases similar to ListQuery?

@@ -264,11 +264,9 @@ object PullupCorrelatedPredicates extends Rule[LogicalPlan] with PredicateHelper
} else {
(transformed, baseConditions)
}
(plan, stripOuterReferences(deDuplicatedConditions))
Copy link
Contributor

Choose a reason for hiding this comment

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

This diff seems dicey... This might lead to unresolved outer reference after this batch.

@mgaido91
Copy link
Contributor Author

@yeshengm we can go ahead with your PR, I hadn't much time in these days to fix this, I'l review it. Thanks.

@mgaido91 mgaido91 closed this Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants