You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by viirya <gi...@git.apache.org> on 2017/02/15 14:55:00 UTC
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
GitHub user viirya opened a pull request:
https://github.com/apache/spark/pull/16941
[SPARK-16475][SQL] broadcast hint for SQL queries - disallow space as the delimiter
## What changes were proposed in this pull request?
A follow-up to disallow space as the delimiter in broadcast hint.
## How was this patch tested?
Jenkins test.
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viirya/spark-1 disallow-space-delimiter
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16941.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #16941
----
commit 23982b196543bce1f8f0f6f24b44c1e90c14e92e
Author: Liang-Chi Hsieh <vi...@gmail.com>
Date: 2017-02-15T14:51:54Z
Disallow space as the delimiter.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16941
**[Test build #72940 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/72940/testReport)** for PR 16941 at commit [`23982b1`](https://github.com/apache/spark/commit/23982b196543bce1f8f0f6f24b44c1e90c14e92e).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/16941#discussion_r101435067
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala ---
@@ -524,7 +530,7 @@ class PlanParserSuite extends PlanTest {
Hint("STREAMTABLE", Seq("a", "b", "c"), table("t").select(star())))
comparePlans(
- parsePlan("SELECT /*+ INDEX(t emp_job_ix) */ * FROM t"),
+ parsePlan("SELECT /*+ INDEX(t, emp_job_ix) */ * FROM t"),
--- End diff --
@dongjoon-hyun Thanks for explanation. Looks like the syntax you referred only supports space as the delimiter?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/16941#discussion_r101313928
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala ---
@@ -505,7 +505,13 @@ class PlanParserSuite extends PlanTest {
val m2 = intercept[ParseException] {
parsePlan("SELECT /*+ MAPJOIN(default.t) */ * from default.t")
}.getMessage
- assert(m2.contains("no viable alternative at input"))
+ assert(m2.contains("mismatched input '.' expecting {')', ','}"))
+
+ // Disallow space as the delimiter.
+ val m3 = intercept[ParseException] {
--- End diff --
NIT: You can use the `PlanParserSuite.intercept` method. That saves some typing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/16941#discussion_r101328119
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala ---
@@ -524,7 +530,7 @@ class PlanParserSuite extends PlanTest {
Hint("STREAMTABLE", Seq("a", "b", "c"), table("t").select(star())))
comparePlans(
- parsePlan("SELECT /*+ INDEX(t emp_job_ix) */ * FROM t"),
+ parsePlan("SELECT /*+ INDEX(t, emp_job_ix) */ * FROM t"),
--- End diff --
Hi, @viirya and @cloud-fan @hvanhovell @rxin @gatorsmile .
Just for explanation, it was originally designed to support other syntax like the followings. Except query block parts (which we don't support), this case means `tableSpec` and `indexSpec`.
https://docs.oracle.com/cd/B12037_01/server.101/b10752/hintsref.htm#21629
At that time, there was a request to provide more general syntax to prevent future changes on `SqlBase.g4` layer.
Anyway, I have no objection on the current approach since I also read the comments between @rxin and @viirya on the previous @rxin 's commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/16941#discussion_r101315401
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala ---
@@ -524,7 +530,7 @@ class PlanParserSuite extends PlanTest {
Hint("STREAMTABLE", Seq("a", "b", "c"), table("t").select(star())))
comparePlans(
- parsePlan("SELECT /*+ INDEX(t emp_job_ix) */ * FROM t"),
+ parsePlan("SELECT /*+ INDEX(t, emp_job_ix) */ * FROM t"),
--- End diff --
we can remove this test now, as it's duplicated if we use `,` as delimiter
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/16941
Merging in master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/16941
LGTM pending jenkins
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16941
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/72940/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/16941
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16941: [SPARK-16475][SQL] broadcast hint for SQL queries...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/16941#discussion_r101329235
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala ---
@@ -524,7 +530,7 @@ class PlanParserSuite extends PlanTest {
Hint("STREAMTABLE", Seq("a", "b", "c"), table("t").select(star())))
comparePlans(
- parsePlan("SELECT /*+ INDEX(t emp_job_ix) */ * FROM t"),
+ parsePlan("SELECT /*+ INDEX(t, emp_job_ix) */ * FROM t"),
--- End diff --
Yea we can generalize it later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16941
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/16941
cc @rxin @dongjoon-hyun @cloud-fan
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16941: [SPARK-16475][SQL] broadcast hint for SQL queries - disa...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16941
**[Test build #72940 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/72940/testReport)** for PR 16941 at commit [`23982b1`](https://github.com/apache/spark/commit/23982b196543bce1f8f0f6f24b44c1e90c14e92e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org