Skip to content

[SPARK-27824][SQL] Make rule EliminateResolvedHint idempotent#24692

Closed
maryannxue wants to merge 1 commit intoapache:masterfrom
maryannxue:eliminatehint-bug
Closed

[SPARK-27824][SQL] Make rule EliminateResolvedHint idempotent#24692
maryannxue wants to merge 1 commit intoapache:masterfrom
maryannxue:eliminatehint-bug

Conversation

@maryannxue
Copy link
Contributor

@maryannxue maryannxue commented May 23, 2019

What changes were proposed in this pull request?

This fix prevents the rule EliminateResolvedHint from being applied again if it's already applied.

How was this patch tested?

Added new UT.

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

def apply(plan: LogicalPlan): LogicalPlan = {
val pulledUp = plan transformUp {
case j: Join =>
case j: Join if j.hint == JoinHint.NONE =>
Copy link
Contributor

@dilipbiswal dilipbiswal May 24, 2019

Choose a reason for hiding this comment

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

@maryannxue If we don't do this transformation, we still require the traversal to remove ResolvedHint ?

Copy link
Contributor Author

@maryannxue maryannxue May 24, 2019

Choose a reason for hiding this comment

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

I think the "best" way to fix it is to merge the new hints into the existing JoinHint, but the question is which one should take precedence, the new one or the existing one?
So far, if things all work right, we should never come to a point where there is a non-empty new hint and a non-empty existing hint, that is, the hint has either been pulled into the join node or it hasn't been processed it. In either case, this fix is sufficient.
The removal of the ResolvedHint is taken care of by the following lines and should not happen at all if the rule has been applied once.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maryannxue Thanks for explanation.

@SparkQA
Copy link

SparkQA commented May 24, 2019

Test build #105739 has finished for PR 24692 at commit 88ac655.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

j-baker pushed a commit to palantir/spark that referenced this pull request Jan 25, 2020
## What changes were proposed in this pull request?

This fix prevents the rule EliminateResolvedHint from being applied again if it's already applied.

## How was this patch tested?

Added new UT.

Closes apache#24692 from maryannxue/eliminatehint-bug.

Authored-by: maryannxue <maryannxue@apache.org>
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants