You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by maryannxue <gi...@git.apache.org> on 2018/05/14 19:04:05 UTC
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
GitHub user maryannxue opened a pull request:
https://github.com/apache/spark/pull/21324
[SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warning
## What changes were proposed in this pull request?
1. Change antlr rule to fix the warning.
2. Add PIVOT/LATERAL check in AstBuilder with a more meaningful error message.
## How was this patch tested?
1. Add a counter case in `PlanParserSuite.test("lateral view")`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maryannxue/spark spark-24035-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21324.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 #21324
----
commit 5c141f09252ab2bb4b10f8ed191cbd73c3039aab
Author: maryannxue <ma...@...>
Date: 2018-05-14T18:58:56Z
[SPARK-24035] SQL syntax for Pivot (fix Antlr warning)
commit ecd37927ef122a75bf87f1de16d6afc80fd0bf61
Author: maryannxue <ma...@...>
Date: 2018-05-14T19:00:15Z
Merge remote-tracking branch 'origin/master' into spark-24035-fix
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21324
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
Posted by maryannxue <gi...@git.apache.org>.
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/spark/pull/21324#discussion_r188079751
--- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
@@ -398,7 +398,7 @@ hintStatement
;
fromClause
- : FROM relation (',' relation)* (pivotClause | lateralView*)?
+ : FROM relation (',' relation)* lateralView* pivotClause?
--- End diff --
Yes, I agree that "(pivotClause | lateralView+)?" would also be a fix for the problem. But by doing a check in a later stage would give a more detailed and meaningful exception.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21324
**[Test build #90607 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90607/testReport)** for PR 21324 at commit [`ecd3792`](https://github.com/apache/spark/commit/ecd37927ef122a75bf87f1de16d6afc80fd0bf61).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/21324#discussion_r188177303
--- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
@@ -398,7 +398,7 @@ hintStatement
;
fromClause
- : FROM relation (',' relation)* (pivotClause | lateralView*)?
+ : FROM relation (',' relation)* lateralView* pivotClause?
--- End diff --
@mgaido91 Better readability is more important to our end users. Currently, we rely on ANTLR4 to issue the error message in most cases. However, ANTLR4 is not good to issue a readable error message, compared with the commercial vendor.
We need to revisit what our current Parser implementation and improve the error handling.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21324#discussion_r188078585
--- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
@@ -398,7 +398,7 @@ hintStatement
;
fromClause
- : FROM relation (',' relation)* (pivotClause | lateralView*)?
+ : FROM relation (',' relation)* lateralView* pivotClause?
--- End diff --
as I said in the e-mail in the dev list, I think that the right fix is `(pivotClause | lateralView+)?`. I have been running the tests and they are successful so far, so I think this is the right approach. It also avoids the additional check needed to throw the ParseException.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21324#discussion_r188080760
--- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
@@ -398,7 +398,7 @@ hintStatement
;
fromClause
- : FROM relation (',' relation)* (pivotClause | lateralView*)?
+ : FROM relation (',' relation)* lateralView* pivotClause?
--- End diff --
I think the more constraints we can express in the parser, the better. @gatorsmile @maropu what do you think?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21324
**[Test build #90607 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90607/testReport)** for PR 21324 at commit [`ecd3792`](https://github.com/apache/spark/commit/ecd37927ef122a75bf87f1de16d6afc80fd0bf61).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
Posted by maryannxue <gi...@git.apache.org>.
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/spark/pull/21324#discussion_r188080851
--- Diff: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 ---
@@ -398,7 +398,7 @@ hintStatement
;
fromClause
- : FROM relation (',' relation)* (pivotClause | lateralView*)?
+ : FROM relation (',' relation)* lateralView* pivotClause?
--- End diff --
FYI, I am doing this based on the ORACLE syntax definition. This "no PIVOT and LATERAL together" thing is listed as a restriction rather than reinforced in the parser rules in ORACLE. That said, I believe I should have done this in my first check-in.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21324
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3209/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by maryannxue <gi...@git.apache.org>.
Github user maryannxue commented on the issue:
https://github.com/apache/spark/pull/21324
@gatorsmile @rxin Could you please review this fix?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21324
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21324
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/90607/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21324
cc @maropu @mgaido91 too
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix ant...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21324
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21324: [SPARK-24035][SQL] SQL syntax for Pivot - fix antlr warn...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21324
Thanks! Merged to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org