[SPARK-10887] [SQL] Build HashedRelation outside of HashJoinNode. #8953

yhuai · 2015-09-30T23:24:26Z

This PR refactors HashJoinNode to take a existing HashedRelation. So, we can reuse this node for both ShuffledHashJoin and BroadcastHashJoin.

https://issues.apache.org/jira/browse/SPARK-10887

SparkQA · 2015-10-01T01:30:19Z

Test build #43140 has finished for PR 8953 at commit 0cf8d44.

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

yhuai · 2015-10-02T15:25:33Z

I think we still need to have a 2-way hash join node that takes two LocalNodes as children. It will internally use the refactored hash join node.

SparkQA · 2015-10-02T18:11:58Z

Test build #43187 has finished for PR 8953 at commit f262b36.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinarydHashJoinNode(
- case class BroadcastHashJoinNode(

yhuai · 2015-10-02T18:23:59Z

test this please

SparkQA · 2015-10-02T18:29:35Z

Test build #43189 has started for PR 8953 at commit f262b36.

yhuai · 2015-10-02T22:29:01Z

test this please

SparkQA · 2015-10-03T00:24:22Z

Test build #43201 has finished for PR 8953 at commit f262b36.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinarydHashJoinNode(
- case class BroadcastHashJoinNode(

…de at runtime.

SparkQA · 2015-10-04T06:05:34Z

Test build #43223 has finished for PR 8953 at commit b9d5bc9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinaryHashJoinNode(
- case class BroadcastHashJoinNode(

SparkQA · 2015-10-06T17:39:57Z

Test build #43282 has finished for PR 8953 at commit e8a3d17.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinaryHashJoinNode(
- case class BroadcastHashJoinNode(

JoshRosen · 2015-10-06T19:04:10Z

sql/core/src/main/scala/org/apache/spark/sql/execution/local/BinaryHashJoinNode.scala

+    left: LocalNode,
+    right: LocalNode) extends BinaryLocalNode(conf) {
+
+  private[this] lazy val (buildNode, buildKeys, streamedNode, streamedKeys) = buildSide match {


Why is this a lazy val?

agreed, doesn't need to be

Removed lazy.

SparkQA · 2015-10-06T19:36:19Z

Test build #43283 has finished for PR 8953 at commit 65c6a50.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinaryHashJoinNode(
- case class BroadcastHashJoinNode(

andrewor14 · 2015-10-07T01:05:58Z

...main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala

+ * input [[InternalRow]] for a fixed set of [[Expression Expressions]]. It exposes a `target`
+ * method. This method is used to set the row that will be updated. So, when `target` is used, the
+ * [[MutableRow]] object created internally will not be used. If `target` is not used, the
+ * [[MutableRow]] object created internally will be used.


I don't understand this comment. Where is target? I can't find it.

See public ${classOf[BaseMutableProjection].getName} target($mutableRowType row) {.

andrewor14 · 2015-10-07T02:40:37Z

As discussed offline, we should explore alternatives where we don't have to wrap local nodes to avoid the wrapping logic. Right now we need to worry about the initialization / prepare / close order and it's easy to miss something there.

yhuai · 2015-10-07T20:09:03Z

sql/core/src/test/scala/org/apache/spark/sql/execution/local/HashJoinNodeSuite.scala

+        val resolvedBuildNode = resolveExpressions(buildNode)
+        val resolvedBuildKeys = resolveExpressions(buildKeys, resolvedBuildNode)
+        val hashedRelation = buildHashedRelation(conf, resolvedBuildKeys, resolvedBuildNode)
+        val broadcastHashedRelation = sqlContext.sparkContext.broadcast(hashedRelation)


@andrewor14 This is the place where I need sqlcontext.

can't you just create a broadcast variable directly? You might have to do some mocking for that, e.g.

import org.mockito.Mockito.{mock, when} val hashedRelation = ... val broadcastHashedRelation = mock(classOf[TorrentBroadcast[HashedRelation]]) when(broadcastHashedRelation.value).thenReturn(hashedRelation)

would that work? Then we won't have to rely on SparkContext.

yhuai · 2015-10-07T20:11:03Z

@andrewor14 I have made HashJoinNode a trait.

SparkQA · 2015-10-07T21:58:22Z

Test build #43340 has finished for PR 8953 at commit ecbd1d0.

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

andrewor14 · 2015-10-07T22:08:40Z

...main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateMutableProjection.scala

- * input [[InternalRow]] for a fixed set of [[Expression Expressions]].
+ * input [[InternalRow]] for a fixed set of [[Expression Expressions]]. It exposes a `target`
+ * method. This method is used to set the row that will be updated. So, when `target` is used, the
+ * [[MutableRow]] object created internally will not be used. If `target` is not used, the


This comment is quite verbose. I think you can just say

It exposes a `target` method, which is used to set the row that will be updated. The internal [[MutableRow]] object created internally is used only when `target` is not used.

andrewor14 · 2015-10-07T22:11:11Z

sql/core/src/main/scala/org/apache/spark/sql/execution/local/HashJoinNode.scala


 /**
+ * A node for inner hash equi-join. [[BinaryHashJoinNode]] and [[BroadcastHashJoinNode]]


An abstract node for sharing common functionality among different implementations of inner hash equi-join, notably [[BinaryHashJoinNode]] and [[BroadcastHashJoinNode]].

andrewor14 · 2015-10-07T22:25:50Z

@yhuai thanks, this looks much simpler. My remaining comments are mainly concerned with comments and tests.

andrewor14 · 2015-10-07T22:34:24Z

LGTM will merge pending tasks. Thanks for addressing the comments so quickly.

SparkQA · 2015-10-08T00:34:13Z

Test build #43352 has finished for PR 8953 at commit 081e331.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinaryHashJoinNode(
- case class BroadcastHashJoinNode(
- trait HashJoinNode

andrewor14 · 2015-10-08T00:35:30Z

retest this please

SparkQA · 2015-10-08T02:29:44Z

Test build #43366 has finished for PR 8953 at commit 081e331.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BinaryHashJoinNode(
- case class BroadcastHashJoinNode(
- trait HashJoinNode

Build HashedRelation outside of HashJoinNode.

0cf8d44

yhuai changed the title ~~[SPARK-10887] [sql] Build HashedRelation outside of HashJoinNode.~~ [SPARK-10887] [SQL] [WIP] Build HashedRelation outside of HashJoinNode. Oct 2, 2015

Add BinarydHashJoinNode and BroadcastHashJoinNode.

f262b36

yhuai changed the title ~~[SPARK-10887] [SQL] [WIP] Build HashedRelation outside of HashJoinNode.~~ [SPARK-10887] [SQL] Build HashedRelation outside of HashJoinNode. Oct 2, 2015

Use withHashedRelation to set the HashedRelation used by a HashJoinNo…

b9d5bc9

…de at runtime.

Add comments to GenerateMutableProjection and GenerateSafeProjection.

65c6a50

JoshRosen reviewed Oct 6, 2015
View reviewed changes

andrewor14 reviewed Oct 7, 2015
View reviewed changes

yhuai reviewed Oct 7, 2015
View reviewed changes

Address comments.

ecbd1d0

andrewor14 reviewed Oct 7, 2015
View reviewed changes

Address comments.

081e331

asfgit closed this in 82d275f Oct 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-10887] [SQL] Build HashedRelation outside of HashJoinNode. #8953

[SPARK-10887] [SQL] Build HashedRelation outside of HashJoinNode. #8953

yhuai commented Sep 30, 2015

SparkQA commented Oct 1, 2015

yhuai commented Oct 2, 2015

SparkQA commented Oct 2, 2015

yhuai commented Oct 2, 2015

SparkQA commented Oct 2, 2015

yhuai commented Oct 2, 2015

SparkQA commented Oct 3, 2015

SparkQA commented Oct 4, 2015

SparkQA commented Oct 6, 2015

JoshRosen Oct 6, 2015

andrewor14 Oct 6, 2015

yhuai Oct 7, 2015

SparkQA commented Oct 6, 2015

andrewor14 Oct 7, 2015

yhuai Oct 7, 2015

andrewor14 Oct 7, 2015

andrewor14 commented Oct 7, 2015

yhuai Oct 7, 2015

andrewor14 Oct 7, 2015

yhuai commented Oct 7, 2015

SparkQA commented Oct 7, 2015

andrewor14 Oct 7, 2015

andrewor14 Oct 7, 2015

andrewor14 commented Oct 7, 2015

andrewor14 commented Oct 7, 2015

SparkQA commented Oct 8, 2015

andrewor14 commented Oct 8, 2015

SparkQA commented Oct 8, 2015


		/**
		* A node for inner hash equi-join. [[BinaryHashJoinNode]] and [[BroadcastHashJoinNode]]

[SPARK-10887] [SQL] Build HashedRelation outside of HashJoinNode. #8953

[SPARK-10887] [SQL] Build HashedRelation outside of HashJoinNode. #8953

Conversation

yhuai commented Sep 30, 2015

SparkQA commented Oct 1, 2015

yhuai commented Oct 2, 2015

SparkQA commented Oct 2, 2015

yhuai commented Oct 2, 2015

SparkQA commented Oct 2, 2015

yhuai commented Oct 2, 2015

SparkQA commented Oct 3, 2015

SparkQA commented Oct 4, 2015

SparkQA commented Oct 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhuai commented Oct 7, 2015

SparkQA commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Oct 7, 2015

andrewor14 commented Oct 7, 2015

SparkQA commented Oct 8, 2015

andrewor14 commented Oct 8, 2015

SparkQA commented Oct 8, 2015