[SPARK-26840][SQL] Avoid cost-based join reorder in presence of join hints #23759

maryannxue · 2019-02-10T19:33:42Z

What changes were proposed in this pull request?

This is a fix for #23524, which did not stop cost-based join reorder when the CostBasedJoinReorder rule recurses down the tree and applies join reorder for nested joins with hints.

The issue had not been detected by the existing tests because CBO is disabled by default.

How was this patch tested?

Enabled CBO for JoinHintSuite.

dongjoon-hyun · 2019-02-10T20:02:50Z

Hi, @maryannxue .
I made a PR to you, maryannxue#1, which replaces all changes in CostBasedJoinReorder with the following one-liner. Could you review and merge that?

   private def replaceWithOrderedJoin(plan: LogicalPlan): LogicalPlan = plan match {
-    case j @ Join(left, right, jt: InnerLike, Some(cond), _) =>
+    case j @ Join(left, right, jt: InnerLike, Some(cond), JoinHint.NONE) =>

cc @gatorsmile

dongjoon-hyun

I believe the one-liner will achieve the same goal you intended.

SparkQA · 2019-02-10T23:55:07Z

Test build #102157 has finished for PR 23759 at commit 6949db5.

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

viirya · 2019-02-11T00:09:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/CostBasedJoinReorder.scala

  private def extractInnerJoins(plan: LogicalPlan): (Seq[LogicalPlan], Set[Expression]) = {
    plan match {
-      case Join(left, right, _: InnerLike, Some(cond), _) =>
+      case Join(left, right, _: InnerLike, Some(cond), hint) if hint == JoinHint.NONE =>


I think we can use pattern match as @dongjoon-hyun's PR, it will be simpler.

maryannxue · 2019-02-11T03:38:31Z

Thank you, @dongjoon-hyun! I'll also change the file "patterns.scala".

dongjoon-hyun · 2019-02-11T15:29:14Z

@maryannxue . Please don't mix the bug fix PR and simple style improvement PR.

dongjoon-hyun · 2019-02-11T15:30:31Z

I don't think 'patters.scala' is releated the bug which this PR aims.

Update to one-liner

dongjoon-hyun

+1, LGTM. (pending Jenkins)

dongjoon-hyun · 2019-02-11T19:04:29Z

@maryannxue . Do we need this in older branches, too?

This reverts commit eb70456.

maryannxue · 2019-02-11T19:23:36Z

Sorry, @dongjoon-hyun, I did not look carefully when I merged your PR. I thought you were to replace every occurrence of "case Join(..., hint) if hint == JoinHint.NONE" with "case Join(..., JoinHint.NONE)" which makes sense to me, but it turned out you had changed other logic too. Is there any specific reason you would have to revert the changes I made in "ExtractInnerJoins"?
I'll update this PR with an extra test.

dongjoon-hyun

@maryannxue . Do you think it's needed? The following was my reason.

Please don't mix the bug fix PR and simple style improvement PR.

For me, this one-liner prevents the start of the disaster.

If you have more test cases, welcome for adding them. Let's see what is really needed minimally. I'll wait for your updates. I prefer not to change the original code if there are no valid test cases.

maryannxue · 2019-02-11T20:48:25Z

@dongjoon-hyun, I just pushed in the latest changes together with two new tests.
The changes in "extractInnerJoins" are necessary for the first added test. The changes (which you suggested) in "replaceWithOrderedJoins" are necessary for the second test.
Once the join items (with number > 2) are extracted by extractInnerJoins, they will be reordered, thus the hint info will be lost; once reordered, regardless of whether the join order has changed, the join operators should be marked "ordered" (replaced with OrderedJoin) to prevent further transform.
The test I enabled with CBO in JoinHintsSuite did not have more than two extracted join items, so it did not go through the real reorder logic, and that's why you one-liner worked.

maryannxue · 2019-02-11T20:58:16Z

@dongjoon-hyun

For me, this one-liner prevents the start of the disaster.

You probably should take a closer look at the logic :) The place where you applied the one-liner happens after the reordering and is just to stop further transform on already ordered joins.

dongjoon-hyun · 2019-02-11T21:22:40Z

Thanks for the more test cases. I'll review tonight and take a more close look. :)

You probably should take a closer look at the logic :)

SparkQA · 2019-02-11T21:32:05Z

Test build #102203 has finished for PR 23759 at commit 41aa5ba.

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

SparkQA · 2019-02-12T01:03:58Z

Test build #102208 has finished for PR 23759 at commit d7c1567.

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

gatorsmile

LGTM

dongjoon-hyun · 2019-02-12T06:38:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/CostBasedJoinReorder.scala

-        case p @ Project(projectList, Join(_, _, _: InnerLike, Some(cond), hint))
-          if projectList.forall(_.isInstanceOf[Attribute]) && hint == JoinHint.NONE =>
+        case p @ Project(projectList, Join(_, _, _: InnerLike, Some(cond), JoinHint.NONE))
+          if projectList.forall(_.isInstanceOf[Attribute]) =>


Could you revert these aesthetic changes (line 46 ~ 49) back?

This PR is not for fixing a regression. Actually, it is just a follow-up of #23524. Thus, it is like a PR for code cleaning.

dongjoon-hyun · 2019-02-12T06:39:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/CostBasedJoinReorder.scala

        (leftPlans ++ rightPlans, splitConjunctivePredicates(cond).toSet ++
          leftConditions ++ rightConditions)
-      case Project(projectList, j @ Join(_, _, _: InnerLike, Some(cond), _))
+      case Project(projectList, j @ Join(_, _, _: InnerLike, Some(cond), JoinHint.NONE))


Ur, is this required? Eventually, line 80 looks enough for me.

It would still be correct without this line of change, but I think it's more efficient that way.

This is also not a bug fix. Just to improve it and make it more efficient.

dongjoon-hyun · 2019-02-12T06:39:49Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/CostBasedJoinReorder.scala


  private def replaceWithOrderedJoin(plan: LogicalPlan): LogicalPlan = plan match {
-    case j @ Join(left, right, jt: InnerLike, Some(cond), _) =>
+    case j @ Join(left, right, jt: InnerLike, Some(cond), JoinHint.NONE) =>


Yep. This is the same one which I suggested.

dongjoon-hyun · 2019-02-12T06:39:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/CostBasedJoinReorder.scala

      val replacedRight = replaceWithOrderedJoin(right)
      OrderedJoin(replacedLeft, replacedRight, jt, Some(cond))
-    case p @ Project(projectList, j @ Join(_, _, _: InnerLike, Some(cond), _)) =>
+    case p @ Project(projectList, j @ Join(_, _, _: InnerLike, Some(cond), JoinHint.NONE)) =>


This is not needed actually.

Same as above.

dongjoon-hyun

Hi, @maryannxue . I reviewed again as you suggested. I agree that this will become two-liner instead of one-liner.

   private def extractInnerJoins(plan: LogicalPlan): (Seq[LogicalPlan], Set[Expression]) = {
     plan match {
-      case Join(left, right, _: InnerLike, Some(cond), _) =>
+      case Join(left, right, _: InnerLike, Some(cond), JoinHint.NONE) =>

   private def replaceWithOrderedJoin(plan: LogicalPlan): LogicalPlan = plan match {
-    case j @ Join(left, right, jt: InnerLike, Some(cond), _) =>
+    case j @ Join(left, right, jt: InnerLike, Some(cond), JoinHint.NONE) =>

Do you think we need the other lines except the above two lines? Especially, line 46 ~ 49 are not functional changes at all.

maryannxue · 2019-02-12T16:58:27Z

@dongjoon-hyun:

I reviewed again as you suggested.

I did not suggest you review it again. I suggested you understand the logic first. Thank you anyway for reviewing it! BTW, I don't totally agree with you on "don't mix the bug fix PR and simple style improvement PR". Ppl can always clean up a few places related to the issue in their PRs, like removing unused imports, correcting styles, refining code comments, etc. Plus, I don't think those two lines of so-called aesthetic changes would be worth another PR (that's going to waste everybody's time). I think we could together make the Spark community more efficient and more constructive if we can focus on important things rather than tell ppl to revert changes that are nice to have.

dongjoon-hyun · 2019-02-12T19:47:22Z

?

I did not suggest you review it again. I suggested you understand the logic first.

Hey, @maryannxue . This is an open source project.

You probably should take a closer look at the logic :)

If you said like the above, it causes another round of reviews.

I understand your enthusiasm to contribute more. But, please take a step back and look at what happens after your SPARK-26065.

98be895 [SPARK-26065][SQL] Change query hint from a LogicalPlan to a field
2d01bcc [SPARK-26065][FOLLOW-UP][SQL] Fix the Failure when having two Consecutive Hints
985f966 [SPARK-26065][FOLLOW-UP][SQL] Revert hint behavior in join reordering
this PR. [SPARK-26840][SQL] Avoid cost-based join reorder in presence of join hints

Are you 100% sure that there is no more bugs related to this? Until the branch becomes more stable back, please minimize the surface of changes by removing unnecessary refactoring. The unrelated refactoring really hurts the traceability.

maryannxue · 2019-02-12T20:01:57Z

@dongjoon-hyun: Just to be clear: the hint behaviors were not clearly documented or well tested. One of the reasons for this whole hint refactoring was to make optimizer work better when hints are present (rules could otherwise be stopped by this trivial ResolveHint node). I had wanted to make join re-ordering work with hints as well, but then found out we had to hold back given the duplicate expr id issue, and it'd probably be a good idea to define the behavior clearly first.
Back to "important things" I mentioned earlier, I think it would have been great if you could have pointed out the correctness problem in my first check-in 98be895. Assume that's what reviewers should do before all other things. And as to this PR, I think it'd be great to refine the code comments here as well, since it took me a little while to figure out what OrderedJoin is really for ( guess the same for you ;) )

dongjoon-hyun · 2019-02-12T20:58:47Z

@maryannxue . That's the reason why we prefer small steps. It's hard for a concise and succinct patch to become wrong. (Of course sometimes it is).

During review process, all reviewers(including me) always track the commit history around it. If this PR was minimal as I suggested last night, I believe that this PR is already in master branch. Furthermore, it will help the next review precess burdens around this code base.

HyukjinKwon · 2019-02-13T13:43:50Z

BTW, it's important to cooperate and to make other people to understand during review process for the best to the author and reviewers in my experience. A patch can be technically reviewed closely and double checked. FWIW, I also had a doubt about the reverted PR although I didn't express my concern there (at the PR you pointed out in the description).

I think I asked to fill the empty JIRA description as well. It is also important to make PR more readable, title, and description. Like .. as an extreme case, let's say there's someone only who understands a logic in an open source project. No one can touch the codes later if that person somehow happens to leave the community (that's an open source :D anyone can join and leave when they want).

HyukjinKwon · 2019-02-13T13:44:52Z

Also, analuzer is pretty core. I know you're good at here and you have been working on this but it's a fix for a reverted PR. Usually it needs some more details rather than saying back to reread it closely to reviewers.

gatorsmile · 2019-02-13T21:59:51Z

I am the reviewer of the original PRs. It takes me many days to finish the review. It is a little bit hard to refactor the whole Hint framework step by step. The goal of the whole hint refactoring is straightforward. We learned the lessons from AnalysisBarrier. Compared with AnalysisBarrier, Hint node is even worse. Users could see the regressions if they use Hint. Not all the the optimizer rules consider Hint when writing the case matching. Basically, the refactoring fixes various issues in the optimizer. The whole work is a very great and benefit all the users in the long term! In the next PR, we can add a new conf for enabling users to decide whether we should throw an exception when the hint is not applicable. Previously, we always silently drop the hints if they do not work.

gatorsmile · 2019-02-13T22:00:32Z

I will merge it soon, if no any new comment.

gatorsmile · 2019-02-15T00:56:13Z

Thanks! Merged to master.

…hints ## What changes were proposed in this pull request? This is a fix for apache#23524, which did not stop cost-based join reorder when the CostBasedJoinReorder rule recurses down the tree and applies join reorder for nested joins with hints. The issue had not been detected by the existing tests because CBO is disabled by default. ## How was this patch tested? Enabled CBO for JoinHintSuite. Closes apache#23759 from maryannxue/spark-26840. Lead-authored-by: maryannxue <[email protected]> Co-authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: gatorsmile <[email protected]>

maryannxue and others added 2 commits February 10, 2019 13:24

fix

6949db5

Update to one-liner

eb70456

dongjoon-hyun requested changes Feb 10, 2019

View reviewed changes

viirya reviewed Feb 11, 2019

View reviewed changes

Merge pull request #1 from dongjoon-hyun/PR-23759

41aa5ba

Update to one-liner

dongjoon-hyun approved these changes Feb 11, 2019

View reviewed changes

Revert "Update to one-liner"

534602b

This reverts commit eb70456.

dongjoon-hyun requested changes Feb 11, 2019

View reviewed changes

fix

d7c1567

gatorsmile reviewed Feb 12, 2019

View reviewed changes

dongjoon-hyun reviewed Feb 12, 2019

View reviewed changes

asfgit closed this in a7e3da4 Feb 15, 2019

[SPARK-26840][SQL] Avoid cost-based join reorder in presence of join hints #23759

[SPARK-26840][SQL] Avoid cost-based join reorder in presence of join hints #23759

Uh oh!

Conversation

maryannxue commented Feb 10, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryannxue commented Feb 11, 2019

Uh oh!

dongjoon-hyun commented Feb 11, 2019

Uh oh!

dongjoon-hyun commented Feb 11, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Feb 11, 2019

Uh oh!

maryannxue commented Feb 11, 2019

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryannxue commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryannxue commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Feb 11, 2019

Uh oh!

SparkQA commented Feb 12, 2019

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

maryannxue commented Feb 12, 2019

Uh oh!

dongjoon-hyun commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryannxue commented Feb 12, 2019

Uh oh!

dongjoon-hyun commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Feb 10, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

maryannxue commented Feb 11, 2019 •

edited

Loading

maryannxue commented Feb 11, 2019 •

edited

Loading

dongjoon-hyun commented Feb 11, 2019 •

edited

Loading

dongjoon-hyun commented Feb 12, 2019 •

edited

Loading

dongjoon-hyun commented Feb 12, 2019 •

edited

Loading