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

dongjoon-hyun · 2016-07-11T08:34:55Z

What changes were proposed in this pull request?

This PR aims to achieve the following two goals in Spark SQL.

1. Generic Hint Syntax
The generic hints are parsed and transformed into concrete hints by SubstituteHints of Analyzer. The unknown hints are removed, too. For example, Hint("MAPJOIN") is transformed into BroadcastJoin and other hints are removed currently.

SELECT /*+ MAPJOIN(t) */ * FROM t
SELECT /*+ STREAMTABLE(a,b,c) */ * FROM t
SELECT /*+ INDEX(t emp_job_ix) */ * FROM t

Unlink Hive, NEWMAPJOIN(t) is allowed for accepting new Spark Hints.

2. Broadcast Hints
The followings are recognized. Technically, broadcast hints are matched UnresolvedRelation to support Hive MetastoreRelation. The style of database_name.table_name is not allowed in this PR.

SELECT /*+ MAPJOIN(t) */ * FROM t JOIN u ON t.id = u.id
SELECT /*+ BROADCAST(u) */ * FROM t JOIN u ON t.id = u.id
SELECT /*+ BROADCASTJOIN(u) */ * FROM t JOIN u ON t.id = u.id

Examples

scala> spark.range(1000000000).createOrReplaceTempView("t")
scala> spark.range(1000000000).createOrReplaceTempView("u")

scala> sql("SELECT * FROM t JOIN u ON t.id = u.id").explain
== Physical Plan ==
*SortMergeJoin [id#0L], [id#4L], Inner
:- *Sort [id#0L ASC], false, 0
:  +- Exchange hashpartitioning(id#0L, 200)
:     +- *Range (0, 1000000000, splits=8)
+- *Sort [id#4L ASC], false, 0
   +- ReusedExchange [id#4L], Exchange hashpartitioning(id#0L, 200)

scala> sql("SELECT /*+ MAPJOIN(t) */ * FROM t JOIN u ON t.id = u.id").explain
== Physical Plan ==
*BroadcastHashJoin [id#0L], [id#4L], Inner, BuildLeft
:- BroadcastExchange HashedRelationBroadcastMode(List(input[0, bigint, false]))
:  +- *Range (0, 1000000000, splits=8)
+- *Range (0, 1000000000, splits=8)

scala> sql("SELECT /*+ MAPJOIN(u) */ * FROM t JOIN u ON t.id = u.id").explain
== Physical Plan ==
*BroadcastHashJoin [id#0L], [id#4L], Inner, BuildRight
:- *Range (0, 1000000000, splits=8)
+- BroadcastExchange HashedRelationBroadcastMode(List(input[0, bigint, false]))
   +- *Range (0, 1000000000, splits=8)

scala> sql("CREATE TABLE hive_t(id INT)")
res5: org.apache.spark.sql.DataFrame = []

scala> sql("CREATE TABLE hive_u(id INT)")
res6: org.apache.spark.sql.DataFrame = []

scala> sql("SELECT /*+ MAPJOIN(hive_u) */ * FROM hive_t JOIN hive_u ON hive_t.id = hive_u.id").explain
== Physical Plan ==
*BroadcastHashJoin [id#28], [id#29], Inner, BuildRight
:- *Filter isnotnull(id#28)
:  +- HiveTableScan [id#28], MetastoreRelation default, hive_t
+- BroadcastExchange HashedRelationBroadcastMode(List(cast(input[0, int, false] as bigint)))
   +- *Filter isnotnull(id#29)
      +- HiveTableScan [id#29], MetastoreRelation default, hive_u

scala> sql("SELECT * FROM hive_t JOIN hive_u ON hive_t.id = hive_u.id").explain
== Physical Plan ==
*SortMergeJoin [id#36], [id#37], Inner
:- *Sort [id#36 ASC], false, 0
:  +- Exchange hashpartitioning(id#36, 200)
:     +- *Filter isnotnull(id#36)
:        +- HiveTableScan [id#36], MetastoreRelation default, hive_t
+- *Sort [id#37 ASC], false, 0
   +- Exchange hashpartitioning(id#37, 200)
      +- *Filter isnotnull(id#37)
         +- HiveTableScan [id#37], MetastoreRelation default, hive_u

How was this patch tested?

Pass the Jenkins tests with new testcases.

dongjoon-hyun · 2016-07-11T08:42:00Z

cc @rxin and @hvanhovell .

dongjoon-hyun · 2016-07-11T08:51:49Z

If the direction is right, I can move on adding BROADCAST JOIN syntax.

hvanhovell · 2016-07-11T08:57:54Z

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

Do we need this because the BRACKETED_COMMENT rule is now expecting at least one character?

Yes. Could you give me some workaround advice?

My advice would be to add the HINT_PREFIX rule ('/*+')

Can you try to add this as a case (| '/**/' -> channel(HIDDEN)) to the BRACKETED_COMMENT rule?

Oops. It seems we can not do that due to channel(HIDDEN).

->command in lexer rule BRACKETED_COMMENT must be last element of single outermost alt

hvanhovell · 2016-07-11T09:32:49Z

This looks pretty good!

dongjoon-hyun · 2016-07-11T09:35:28Z

Thank you for quick review! I'll let you know after updating.

SparkQA · 2016-07-11T09:46:51Z

Test build #62082 has finished for PR 14132 at commit 6bd704e.

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

dongjoon-hyun · 2016-07-11T09:59:13Z

Oops. Five errors, too. I'll fix these tomorrow.

hvanhovell · 2016-07-12T12:12:06Z

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

What are we trying to support here?

I think you can also do this (is easier in the AST builder): | hintName=identifier '(' parameters+=identifier parameters+=identifier ')'

It supports 'INDEX(t idx_emp)' style. For example, I included one in the test.

SparkQA · 2016-07-12T13:26:52Z

Test build #62162 has finished for PR 14132 at commit edec2e4.

This patch passes all 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

dongjoon-hyun · 2016-07-12T16:19:14Z

For the HINT_PREFIX, I tried at the first, but still have some problem. So, I couldn't include the very previous commit.

dongjoon-hyun · 2016-07-12T16:55:54Z

Thank you always, @hvanhovell . And, sorry for the delay. Since last Saturday, I need to do some important personal stuff offline, so the time is limited for me. :(

dongjoon-hyun · 2016-07-12T21:42:17Z

I'm back. I'll resolve them.

dongjoon-hyun · 2016-07-12T23:12:15Z

So far, I couldn't do the following two advices. I just inform you that I'm still working these. :)

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

dongjoon-hyun · 2016-07-12T23:54:25Z

Now, only minor HINT_PREFIX remains.

dongjoon-hyun · 2016-07-13T00:05:39Z

Hi, @hvanhovell . So far, I tried in the following way for HINT_PREFIX.
Due to the prefix overlapping, BRACKETED_COMMENT seems to eat the HINT.
Could you give me some advice more?

 hint
-    : '/*+' hintStatement '*/'
+    : HINT_PREFIX hintStatement '*/'
     ;

 hintStatement
@@ -961,12 +961,12 @@ SIMPLE_COMMENT
     : '--' ~[\r\n]* '\r'? '\n'? -> channel(HIDDEN)
     ;

-BRACKETED_EMPTY_COMMENT
-    : '/**/' -> channel(HIDDEN)
+HINT_PREFIX
+    : '/*+'
     ;

 BRACKETED_COMMENT
-    : '/*' ~[+] .*? '*/' -> channel(HIDDEN)
+    : '/*' .*? '*/' -> channel(HIDDEN)             <--- The original one.
     ;

SparkQA · 2016-07-13T01:09:27Z

Test build #62196 has finished for PR 14132 at commit 5c76e25.

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

SparkQA · 2016-07-13T02:14:26Z

Test build #62198 has finished for PR 14132 at commit 61e4b1b.

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

dongjoon-hyun · 2016-07-13T02:52:45Z

Hi, @rxin .
Could you review this PR again when you have some time?
All the features are implemented and Spark Parser accepts general hints now.

dongjoon-hyun · 2016-07-13T10:52:38Z

Hi, @hvanhovell .
In this PR, if possible, could we skip adding HINT_PREFIX rule? I finished all the other things.
Actually, there is no change on the behavior of parser if we can use HINT_PREFIX.

hvanhovell · 2016-07-13T11:13:42Z

@dongjoon-hyun sure. It was merely a suggestion to get rid of the BRACKETED_EMPTY_COMMENT rule.

dongjoon-hyun · 2016-07-13T11:27:06Z

Thank you, @hvanhovell .
For the other, any more comments are welcome!

dongjoon-hyun · 2016-07-13T18:21:17Z

PR description is updated.

…d more description.

dongjoon-hyun · 2016-07-28T17:35:30Z

Oh, the one failure is due to a new MAPJOIN testcase in the master branch (I added yesterday.)
I'll rebase and update that, too.

SparkQA · 2016-07-28T19:41:50Z

Test build #62975 has finished for PR 14132 at commit 4023d97.

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

dongjoon-hyun · 2016-07-29T15:19:29Z

Hi, @cloud-fan .
Could you review this PR again please?

dongjoon-hyun · 2016-07-30T23:59:18Z

This PR grows too much. Sometime, scrolling is too slow. I close this and open a new one #14426 .

dongjoon-hyun changed the title ~~[SPARK-16475][SQL] Broadcast Hint for SQL Queries~~ [SPARK-16475][SQL][WIP] Broadcast Hint for SQL Queries Jul 11, 2016

hvanhovell reviewed Jul 11, 2016
View reviewed changes

hvanhovell reviewed Jul 12, 2016
View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-16475][SQL][WIP] Broadcast Hint for SQL Queries~~ [SPARK-16475][SQL] Broadcast Hint for SQL Queries Jul 12, 2016

dongjoon-hyun added 23 commits July 28, 2016 10:31

Address comments.

63b629c

Add nested broadcast hint testcase.

3b25505

Add description and one more testcase.

f029dac

Add more tests in AnalysisSuite and address various comments.

4fce2ca

Address comments.

16ad3a1

Updat SQLBuilder and LogicalPlanToSQLSuite.

8a407aa

Use DSL

6b20a44

Update testcases.

0f3f53e

Add window/rollup/groupingset testcases.

11e5f76

Fix scalastyle.

7ab0c19

Use checkSQL.

9cf4d65

Abstract rules.

47c3733

Apply SubstituteHints to unresolved stage back.

61f7776

Remove unused imports.

fccf9da

Address comments.

b0b9f61

Fix indentation.

39a3277

Add more description on SubstituteHint.

1b38e55

Simplify the logic by adding Hint(SubqueryAlias(SQLTable)) pattern an…

9bf6fa7

…d more description.

Move check cases into CheckAnalysis.

c8436fd

Hint should wrap the whole plan.

302b9d4

Make BROADCAST_HINT_NAMES set and fix error message.

e7c7943

Update with stable queries.

e6b2409

Don't propagate Hint into subqueries.

9f0b104

Update golden file with newly one having regenerated hint.

4023d97

dongjoon-hyun closed this Jul 30, 2016

dongjoon-hyun mentioned this pull request Aug 6, 2016

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

Closed

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

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

Uh oh!

Conversation

dongjoon-hyun commented Jul 11, 2016 • 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!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

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!

hvanhovell commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

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!

SparkQA commented Jul 12, 2016

Uh oh!

dongjoon-hyun commented Jul 12, 2016

Uh oh!

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

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 12, 2016

Uh oh!

dongjoon-hyun commented Jul 12, 2016

Uh oh!

dongjoon-hyun commented Jul 12, 2016

Uh oh!

dongjoon-hyun commented Jul 13, 2016

Uh oh!

SparkQA commented Jul 13, 2016

Uh oh!

SparkQA commented Jul 13, 2016

Uh oh!

dongjoon-hyun commented Jul 13, 2016

Uh oh!

dongjoon-hyun commented Jul 13, 2016

Uh oh!

hvanhovell commented Jul 13, 2016

Uh oh!

dongjoon-hyun commented Jul 13, 2016

Uh oh!

dongjoon-hyun commented Jul 13, 2016

Uh oh!

dongjoon-hyun commented Jul 28, 2016

Uh oh!

SparkQA commented Jul 28, 2016

Uh oh!

dongjoon-hyun commented Jul 29, 2016

Uh oh!

dongjoon-hyun commented Jul 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

dongjoon-hyun commented Jul 11, 2016 •

edited

Loading

dongjoon-hyun commented Jul 12, 2016 •

edited

Loading

dongjoon-hyun commented Jul 30, 2016 •

edited

Loading