[SPARK-16475][SQL] Broadcast hint for SQL Queries #16925

rxin · 2017-02-14T12:21:53Z

What changes were proposed in this pull request?

This pull request introduces a simple hint infrastructure to SQL and implements broadcast join hint using the infrastructure.

The hint syntax looks like the following:

SELECT /*+ BROADCAST(t) */ * FROM t

For broadcast hint, we accept "BROADCAST", "BROADCASTJOIN", and "MAPJOIN", and a sequence of relation aliases can be specified in the hint. A broadcast hint plan node will be inserted on top of any relation (that is not aliased differently), subquery, or common table expression that match the specified name.

The hint resolution works by recursively traversing down the query plan to find a relation or subquery that matches one of the specified broadcast aliases. The traversal does not go past beyond any existing broadcast hints, subquery aliases. This rule happens before common table expressions.

Note that there was an earlier patch in #14426. This is a rewrite of that patch, with different semantics and simpler test cases.

How was this patch tested?

Added a new unit test suite for the broadcast hint rule (SubstituteHintsSuite) and new test cases for parser change (in PlanParserSuite). Also added end-to-end test case in BroadcastSuite.

[SPARK-16475][SQL] Broadcast Hint for SQL Queries

hvanhovell · 2017-02-14T12:23:16Z

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

+ *
+ * @param enabled whether to overwrite existing data in the table.
+ * @param specificPartition only data in the specified partition will be overwritten.
+ */


What is this?

SparkQA · 2017-02-14T13:59:27Z

Test build #72870 has finished for PR 16925 at commit 318bc03.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Hint(name: String, parameters: Seq[String], child: LogicalPlan) extends UnaryNode
case class OverwriteOptions(

SparkQA · 2017-02-14T14:08:49Z

Test build #72872 has finished for PR 16925 at commit c702e3e.

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

SparkQA · 2017-02-14T14:35:26Z

Test build #72874 has finished for PR 16925 at commit a095df3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SubstituteHints(conf: CatalystConf) extends Rule[LogicalPlan]

rxin · 2017-02-14T16:31:05Z

Actually I'm going to completely rewrite this. I don't think the current implementation makes sense.

dongjoon-hyun · 2017-02-14T16:42:29Z

@rxin . May I update that better or do you wnat to finish here?
Anything is okay for me. Anyway, thank you for everything!

rxin · 2017-02-14T17:08:23Z

cc @dongjoon-hyun, @cloud-fan, @gatorsmile and @hvanhovell This should be ready for review. Note that the semantics is different from the earlier versions.

rxin · 2017-02-14T17:08:37Z

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

+ * Substitute Hints.
+ * - BROADCAST/BROADCASTJOIN/MAPJOIN match the closest table with the given name parameters.
+ *
+ * In the case of broadcast hint, we find the frontier of


this block needs to be updated.

dongjoon-hyun · 2017-02-14T18:25:36Z

Except the doc you mentioned, it looks great to me, @rxin !
It's great to have this in Spark SQL finally.

SparkQA · 2017-02-14T19:29:17Z

Test build #72879 has finished for PR 16925 at commit 617f8a2.

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

cloud-fan · 2017-02-14T19:31:17Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+
 BRACKETED_COMMENT
-    : '/*' .*? '*/' -> channel(HIDDEN)
+    : '/*' ~[+] .*? '*/' -> channel(HIDDEN)


what does ~[+] mean?

It means except +.

/*+ is used hint rule in line 377.

cloud-fan · 2017-02-14T19:35:22Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+        val withWindow = withDistinct.optionalMap(windows)(withWindows)
+
+        // Hint
+        withWindow.optionalMap(ctx.hint)(withHints)


nit: we already import ctx._ at the beginning, to be consistent we can just write hint

cloud-fan · 2017-02-14T19:41:24Z

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

+  }
+
+  /**
+   * Removes all the hints. This must be executed after all the other hint rules are executed.


shall we mention that, this can happen when users specify invalid hints and we will ignore it?

Yea let me add that.

cloud-fan · 2017-02-14T19:41:59Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+
+hintStatement
+    : hintName=identifier
+    | hintName=identifier '(' parameters+=identifier parameters+=identifier ')'


why do we need this?

The purpose of this is to allow SqlBase.g4 accept more general rule for the future.
So, possibly, it's for reducing the change in SqlBase.g4 in order to add new hint rules.

cloud-fan · 2017-02-14T19:46:38Z

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

+      }
+
+      if ((plan fastEquals newNode) && recurse) {
+        newNode.mapChildren(child => applyBroadcastHint(child, toBroadcast))


in the case of self-join, we may broadcast both side, is it expected?

That's fine. Both being broadcastable doesn't mean we broadcast both.

cloud-fan · 2017-02-14T19:51:15Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/SubstituteHintsSuite.scala

+  }
+
+  test("should work for subqueries") {
+    val relation = UnresolvedRelation(TableIdentifier("table"), Some("tableAlias"))


nit: table("table").as("tableAlias")

They do different things.

actually they do the same thing. there's a pattern match in as. let me change it,

cloud-fan · 2017-02-14T19:55:18Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

+      Hint("STREAMTABLE", Seq("a", "b", "c"), table("t").select(star())))
+
+    comparePlans(
+      parsePlan("SELECT /*+ INDEX(t emp_job_ix) */ * FROM t"),


what if users writing /*+ BROADCASTJOIN(a b) */? shall we treat it as /*+ BROADCASTJOIN(a, b) */?

Yes. Both are considered equally now.

@rxin Looks like we already support space as delimiter?

cloud-fan · 2017-02-14T20:45:50Z

LGTM, pending jenkins

SparkQA · 2017-02-14T21:36:38Z

Test build #72888 has finished for PR 16925 at commit 51a73d5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SubstituteBroadcastHints(conf: CatalystConf) extends Rule[LogicalPlan]

cloud-fan · 2017-02-14T22:11:37Z

merging to master!

rxin · 2017-02-14T22:13:47Z

the latest commit hasn't finished running tests yet ... but probably fine given the small change.

SparkQA · 2017-02-14T23:36:07Z

Test build #72893 has finished for PR 16925 at commit 0d42978.

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

gatorsmile · 2017-02-15T08:27:32Z

A late LGTM. : )

Normally, we do not encourage users to define hints inside a view. Users can add a BROADCAST hint when they define a persistent view. That will be stored in the catalog. The hint will take an effect, as long as this view is not cached.

Users can also use a BROADCAST hint for the operators (e.g., INTERSECT) that will be rewritten to a Join in optimizer.

sql("CREATE VIEW v AS SELECT /*+ BROADCAST(u) */ key FROM u")
sql("SELECT /*+ BROADCAST(t) */ * FROM t INTERSECT SELECT * FROM v")

## What changes were proposed in this pull request? This pull request introduces a simple hint infrastructure to SQL and implements broadcast join hint using the infrastructure. The hint syntax looks like the following: ``` SELECT /*+ BROADCAST(t) */ * FROM t ``` For broadcast hint, we accept "BROADCAST", "BROADCASTJOIN", and "MAPJOIN", and a sequence of relation aliases can be specified in the hint. A broadcast hint plan node will be inserted on top of any relation (that is not aliased differently), subquery, or common table expression that match the specified name. The hint resolution works by recursively traversing down the query plan to find a relation or subquery that matches one of the specified broadcast aliases. The traversal does not go past beyond any existing broadcast hints, subquery aliases. This rule happens before common table expressions. Note that there was an earlier patch in apache#14426. This is a rewrite of that patch, with different semantics and simpler test cases. ## How was this patch tested? Added a new unit test suite for the broadcast hint rule (SubstituteHintsSuite) and new test cases for parser change (in PlanParserSuite). Also added end-to-end test case in BroadcastSuite. Author: Reynold Xin <[email protected]> Author: Dongjoon Hyun <[email protected]> Closes apache#16925 from rxin/SPARK-16475-broadcast-hint.

viirya · 2017-02-15T14:22:46Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+
+hintStatement
+    : hintName=identifier
+    | hintName=identifier '(' parameters+=identifier parameters+=identifier ')'


This rule doesn't support like SELECT /*+ INDEX(a b c) */ * FROM t. To support it, it should be:

| hintName=identifier '(' parameters+=identifier parameters+=identifier* ')'

Is it intentional?

What does it support?

INDEX(a, b, c)

?

I think ti's a bit weird to support space as the delimiter.

But we support INDEX(a b)?

do we? we should disallow it. want to submit a pr?

btw this was written by @dongjoon-hyun. I didn't change it :)

ok. let me prepare a tiny one.

## What changes were proposed in this pull request? A small update to apache#16925 1. Rename SubstituteHints -> ResolveHints to be more consistent with rest of the rules. 2. Added more documentation in the rule and be more defensive / future proof to skip views as well as CTEs. ## How was this patch tested? This pull request contains no real logic change and all behavior should be covered by existing tests. Author: Reynold Xin <[email protected]> Closes apache#16939 from rxin/SPARK-16475.

dongjoon-hyun and others added 2 commits November 3, 2016 13:30

[SPARK-16475][SQL] Broadcast Hint for SQL Queries

539782d

Merge pull request apache#14426 from dongjoon-hyun/SPARK-16475-HINT

318bc03

[SPARK-16475][SQL] Broadcast Hint for SQL Queries

hvanhovell reviewed Feb 14, 2017

View reviewed changes

rxin added 2 commits February 14, 2017 13:28

Get rid of the merge leftover.

c702e3e

Move rule out to its own file.

a095df3

rxin changed the title ~~[SPARK-16475][SQL] Broadcast Hint for SQL Queries~~ [SPARK-16475][SQL] Broadcast Hint for SQL Queries - WIP Feb 14, 2017

Rewrote the PR.

617f8a2

rxin changed the title ~~[SPARK-16475][SQL] Broadcast Hint for SQL Queries - WIP~~ [SPARK-16475][SQL] Broadcast Hint for SQL Queries Feb 14, 2017

rxin commented Feb 14, 2017

View reviewed changes

Separate rule.

51a73d5

rxin changed the title ~~[SPARK-16475][SQL] Broadcast Hint for SQL Queries~~ [SPARK-16475][SQL] Broadcast hint for SQL Queries Feb 14, 2017

cloud-fan reviewed Feb 14, 2017

View reviewed changes

CR

0d42978

asfgit closed this in da7aef7 Feb 14, 2017

rxin mentioned this pull request Feb 15, 2017

[SPARK-16475][SQL] broadcast hint for SQL queries - follow up #16939

Closed

viirya reviewed Feb 15, 2017

View reviewed changes

[SPARK-16475][SQL] Broadcast hint for SQL Queries #16925

[SPARK-16475][SQL] Broadcast hint for SQL Queries #16925

Uh oh!

Conversation

rxin commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

rxin commented Feb 14, 2017

Uh oh!

dongjoon-hyun commented Feb 14, 2017

Uh oh!

rxin commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Feb 14, 2017

Uh oh!

SparkQA commented Feb 14, 2017

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!

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!

cloud-fan commented Feb 14, 2017

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

cloud-fan commented Feb 14, 2017

Uh oh!

rxin commented Feb 14, 2017

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

gatorsmile commented Feb 15, 2017

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

rxin commented Feb 14, 2017 •

edited

Loading