[SPARK-13383][SQL] Keep broadcast hint after column pruning #11260

viirya · 2016-02-19T06:35:04Z

JIRA: https://issues.apache.org/jira/browse/SPARK-13383

What changes were proposed in this pull request?

When we do column pruning in Optimizer, we put additional Project on top of a logical plan. However, when we already wrap a BroadcastHint on a logical plan, the added Project will hide BroadcastHint after later execution.

We should take care of BroadcastHint when we do column pruning.

How was the this patch tested?

Unit test is added.

viirya · 2016-02-19T06:42:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

@@ -432,7 +432,8 @@ class Analyzer(
            case r if r == oldRelation => newRelation
          } transformUp {
            case other => other transformExpressions {
-              case a: Attribute => attributeRewrites.get(a).getOrElse(a)
+              case a: Attribute =>
+                attributeRewrites.get(a).getOrElse(a).withQualifiers(a.qualifiers)


This issue is proposed to fix in #11261. I included this change here is because without this the following unit test can not pass.

SparkQA · 2016-02-19T08:17:26Z

Test build #51527 has finished for PR 11260 at commit 9639223.

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

viirya · 2016-02-20T12:13:56Z

ping @marmbrus @liancheng @rxin @davies

davies · 2016-02-20T16:15:17Z

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

-      Project(allReferences.filter(c.outputSet.contains).toSeq, c)
+      c match {
+        case BroadcastHint(p) =>
+          BroadcastHint(Project(allReferences.filter(c.outputSet.contains).toSeq, p))


Is there any other operator will be inserted between Join and BroadcastHint?

Maybe we could have a rule to pull up BroadcastHint until Join.

One way to do it is to define the hint in the join operator, and then it is safe (can never be pushed anywhere else)

SparkQA · 2016-02-21T12:01:20Z

Test build #51625 has finished for PR 11260 at commit 37c3523.

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

rxin · 2016-02-21T22:22:22Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicOperators.scala

@@ -260,6 +260,20 @@ case class Join(
    condition: Option[Expression])
  extends BinaryNode with PredicateHelper {

+  private def isBrocastHint(plan: LogicalPlan): Boolean = {


what i meant was making the broadcast hint just a property on the logical plan, so it can never be pushed away from it.

yeah, I see. But as we need to add two broadcast hint for left and right plans. Considering Join is broadly used in many places as pattern matching usage, I am afraid that we need to change too many places for this change.

E.g. each case Join(left, right, joinType, condition) => would need to change to case Join(left, right, joinType, condition, leftBroadcastHint, rightBroadcastHint).

Is it ok for you? If so, I will make the change as that.

well you don't need to make the hint a parameter of the case class, but just some field you can set ...

OK. I've made it as variables to set & copy from other Join operator. Please take a look if this update is good. Thanks.

viirya · 2016-02-22T04:37:41Z

retest this please.

SparkQA · 2016-02-22T04:57:12Z

Test build #51659 has finished for PR 11260 at commit 03dac45.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2016-02-22T13:09:59Z

retest this please.

viirya · 2016-02-22T14:01:22Z

Why jenkins can't retest?

gatorsmile · 2016-02-22T16:13:49Z

It could be caused by the build failure. The shade JAR is too large now. After merging the latest build, I hit an issue when using mvn.

marmbrus · 2016-02-22T18:27:05Z

This feels kind of hacky to me. @rxin why doesn't the hint just change the statistics again?

SparkQA · 2016-02-22T23:07:43Z

Test build #51672 has finished for PR 11260 at commit ad63367.

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

rxin · 2016-02-22T23:10:38Z

I still don't like this approach and find it too hacky.

I talked to @marmbrus more offline, and maybe it's easiest to just rely on stats overriding, e.g. when broadcast hint is declared, we set the size of the relation (just in the hint operator) to the smallest possible number (1?). Then it should be robust to pushdowns.

viirya · 2016-02-23T01:23:38Z

@rxin @marmbrus that is good idea. Let me update this.

viirya · 2016-02-23T04:09:27Z

retest this please.

rxin · 2016-02-23T04:51:23Z

We can remove the old broadcast hint matching in strategy now, can't we?

viirya · 2016-02-23T05:02:55Z

Indeed. We can now. Will update it.

rxin · 2016-02-23T05:07:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

@@ -82,7 +82,6 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
   */
  object CanBroadcast {
    def unapply(plan: LogicalPlan): Option[LogicalPlan] = plan match {


we should probably turn this patter matching into just an if/else statement now

SparkQA · 2016-02-23T06:55:42Z

Test build #51731 has finished for PR 11260 at commit a3cadbb.

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

rxin · 2016-02-23T07:10:53Z

sql/core/src/test/scala/org/apache/spark/sql/JoinSuite.scala

@@ -137,6 +139,22 @@ class JoinSuite extends QueryTest with SharedSQLContext {
    assert(planned.size === 1)
  }

+  test("broadcasthint sets relation statistics to smallest value") {


sorry would be great to put this in the optimizer suites, rather than this file which is an end-to-end suite.

I just took a look at the suites available. I'd rename JoinOrderSuite to JoinOptimizationSuite, and then put this case there.

yeah, I just can't find appropriate suite for it. I'd use JoinOptimizationSuite. Thanks.

SparkQA · 2016-02-23T07:23:30Z

Test build #51738 has finished for PR 11260 at commit 3360007.

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

SparkQA · 2016-02-23T07:34:23Z

Test build #51743 has finished for PR 11260 at commit 1e35aa3.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2016-02-23T10:00:25Z

retest this please.

SparkQA · 2016-02-23T11:54:12Z

Test build #51763 has finished for PR 11260 at commit 01dfad4.

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

viirya · 2016-02-23T11:58:50Z

@rxin I've addressed your comments. Please see if this is appropriate. Thanks.

marmbrus · 2016-02-23T18:28:51Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/JoinOptimizationSuite.scala

+    comparePlans(optimized, expected)
+
+    assert(optimized.collect {
+      case b @ BroadcastHint(_) if b.statistics.sizeInBytes == 1 => 1


This is a bit of a nit, but I think what you really want to test here is something like:

val broadcastChildren = optimized.collect { case Join(_, CanBroadcast(r), _, _) => r } assert(broadcastChildren == 1)

With the current test something could break in Project (for example) that would prevent the broadcast from actually happening.

Because it seems we can't import CanBroadcast into this test, I do update according to your comment with a little change (just check its statistics.sizeInBytes). Please see if it is appropriate now. Thanks.

marmbrus · 2016-02-23T18:30:34Z

Implementation LGTM overall, minor comment on tests.

marmbrus · 2016-02-24T03:28:50Z

test this please

SparkQA · 2016-02-24T04:46:55Z

Test build #51842 has finished for PR 11260 at commit b58710f.

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

viirya · 2016-02-24T05:13:46Z

I think the failed test is caused by updated column pruning rule.

SparkQA · 2016-02-24T09:18:41Z

Test build #51858 has finished for PR 11260 at commit 88535ee.

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

viirya · 2016-02-24T09:54:57Z

retest this please.

viirya · 2016-02-24T09:55:29Z

Failure (at test_mllib.R#133): kmeans ...

SparkQA · 2016-02-24T11:36:35Z

Test build #51868 has finished for PR 11260 at commit 88535ee.

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

viirya · 2016-02-24T13:37:32Z

It is weird. Some other PRs like #11344 (just document change) also failed at this SparkR unit tests.

viirya · 2016-02-24T15:08:39Z

retest this please.

SparkQA · 2016-02-24T16:40:10Z

Test build #51882 has finished for PR 11260 at commit 88535ee.

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

marmbrus · 2016-02-24T18:20:38Z

Looks good, merging to master.

Keep broadcast hint after column pruning.

9639223

viirya reviewed Feb 19, 2016
View reviewed changes

viirya mentioned this pull request Feb 19, 2016

[SPARK-13213][SQL] Use CartisianProduct instead of BroacastNestedLoopJoin if possibly #11251

Closed

davies reviewed Feb 20, 2016
View reviewed changes

viirya added 3 commits February 21, 2016 04:03

Merge remote-tracking branch 'upstream/master' into keep-broadcasthint

4c4c833

Track broadcast hint info in Join operator.

c682aaa

Fix typo.

37c3523

rxin reviewed Feb 21, 2016
View reviewed changes

viirya added 2 commits February 22, 2016 03:01

Merge remote-tracking branch 'upstream/master' into keep-broadcasthint

eeb6c1f

Keep broadhint hint in Join operator.

03dac45

Fix scala style.

ad63367

viirya added 2 commits February 23, 2016 03:51

Address comments.

0f9978f

Revert previous change.

3fbdab9

Remove broadcast hint match.

a3cadbb

rxin reviewed Feb 23, 2016
View reviewed changes

viirya force-pushed the keep-broadcasthint branch from 1e35aa3 to 9998d95 Compare February 23, 2016 07:21

viirya added 2 commits February 23, 2016 07:21

Move test.

9998d95

For comment.

01dfad4

marmbrus reviewed Feb 23, 2016
View reviewed changes

Fix test.

b58710f

Merge remote-tracking branch 'upstream/master' into keep-broadcasthint

349d5d9

Fix test according to new column pruning rule.

88535ee

viirya mentioned this pull request Feb 24, 2016

[SPARK-13472][SparkR] Fix unstable Kmeans test in R #11345

Closed

asfgit closed this in f373986 Feb 24, 2016

viirya deleted the keep-broadcasthint branch December 27, 2023 18:33

[SPARK-13383][SQL] Keep broadcast hint after column pruning #11260

[SPARK-13383][SQL] Keep broadcast hint after column pruning #11260

Conversation

viirya commented Feb 19, 2016

What changes were proposed in this pull request?

How was the this patch tested?

Choose a reason for hiding this comment

SparkQA commented Feb 19, 2016

viirya commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Feb 22, 2016

SparkQA commented Feb 22, 2016

viirya commented Feb 22, 2016

viirya commented Feb 22, 2016

gatorsmile commented Feb 22, 2016

marmbrus commented Feb 22, 2016

SparkQA commented Feb 22, 2016

rxin commented Feb 22, 2016

viirya commented Feb 23, 2016

viirya commented Feb 23, 2016

rxin commented Feb 23, 2016

viirya commented Feb 23, 2016

Choose a reason for hiding this comment

SparkQA commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 23, 2016

SparkQA commented Feb 23, 2016

viirya commented Feb 23, 2016

SparkQA commented Feb 23, 2016

viirya commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmbrus commented Feb 23, 2016

marmbrus commented Feb 24, 2016

SparkQA commented Feb 24, 2016

viirya commented Feb 24, 2016

SparkQA commented Feb 24, 2016

viirya commented Feb 24, 2016

viirya commented Feb 24, 2016

SparkQA commented Feb 24, 2016

viirya commented Feb 24, 2016

viirya commented Feb 24, 2016

SparkQA commented Feb 24, 2016

marmbrus commented Feb 24, 2016