[SPARK-26215][SQL][FOLLOW-UP][MINOR] Fix the warning from ANTR4 #23897

dilipbiswal · 2019-02-26T11:28:15Z

What changes were proposed in this pull request?

I see the following new warning from ANTR4 after SPARK-26215 after it added SCHEMA keyword in the reserved/unreserved list. This is a minor PR to cleanup the warning.

WARNING] warning(125): org/apache/spark/sql/catalyst/parser/SqlBase.g4:784:90: implicit definition of token SCHEMA in parser
[WARNING] .../apache/spark/org/apache/spark/sql/catalyst/parser/SqlBase.g4 [784:90]: implicit definition of token SCHEMA in parser

How was this patch tested?

Manually built catalyst after the fix to verify

dilipbiswal · 2019-02-26T11:28:39Z

cc @maropu Kindly check if this looks okay to you.

maropu

Thanks! Looks good to me.

maropu · 2019-02-26T13:13:48Z

Pending, Jenkins

dilipbiswal · 2019-02-26T14:54:29Z

retest this please

viirya · 2019-02-26T14:58:31Z

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

@@ -1014,7 +1014,8 @@ SORTED: 'SORTED';
 PURGE: 'PURGE';
 INPUTFORMAT: 'INPUTFORMAT';
 OUTPUTFORMAT: 'OUTPUTFORMAT';
-DATABASE: 'DATABASE' | 'SCHEMA';
+SCHEMA: 'SCHEMA';
+DATABASE: 'DATABASE';


Previously, token schema is synonymous with token database.

Won't this change drop support of commands like CREATE SCHEMA ..., ALTER SCHEMA ..., DROP SCHEMA ...?

scala> sql("create schema test1") org.apache.spark.sql.catalyst.parser.ParseException: no viable alternative at input 'create schema'(line 1, pos 7) == SQL == create schema test1 -------^^^ at org.apache.spark.sql.catalyst.parser.ParseException.withCommand(ParseDriver.scala:243) at org.apache.spark.sql.catalyst.parser.AbstractSqlParser.parse(ParseDriver.scala:119) at org.apache.spark.sql.execution.SparkSqlParser.parse(SparkSqlParser.scala:48) at org.apache.spark.sql.catalyst.parser.AbstractSqlParser.parsePlan(ParseDriver.scala:69) at org.apache.spark.sql.SparkSession.$anonfun$sql$1(SparkSession.scala:653) at org.apache.spark.sql.catalyst.QueryPlanningTracker.measurePhase(QueryPlanningTracker.scala:111) at org.apache.spark.sql.SparkSession.sql(SparkSession.scala:653) ... 49 elided

@viirya Thanks a lot. Actually i hadn't run the tests locally for this fix and relied on jenkins. Lets me see what to do here.. thanks again.

Do we have a test to capture this?

@gatorsmile Its a compile time warning. Is it possible to test this ?

@gatorsmile Hi Sean, i found out, one of the way we can get this checked is by failing the build on a warning. I tried using treatWarningsAsErrors option. Do you think, this is a acceptable solution ? If so, should this be under a separate ticket as, if something goes wrong with this option, we can rollback that fix. Please let me know what you think.

https://www.antlr.org/api/maven-plugin/latest/antlr4-mojo.html

Here is the error when i used this option.

INFO] [INFO] --- antlr4-maven-plugin:4.7.1:antlr4 (default) @ spark-catalyst_2.12 --- [INFO] ANTLR 4: Processing source directory /Users/dbiswal/mygit/apache/spark/sql/catalyst/src/main/antlr4 [INFO] Processing grammar: org/apache/spark/sql/catalyst/parser/SqlBase.g4 [WARNING] warning(125): org/apache/spark/sql/catalyst/parser/SqlBase.g4:785:90: implicit definition of token SCHEMA in parser [WARNING] /Users/dbiswal/mygit/apache/spark/org/apache/spark/sql/catalyst/parser/SqlBase.g4 [785:90]: implicit definition of token SCHEMA in parser [ERROR] error(10): warning treated as error [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ [INFO] Total time: 9.901 s [INFO] Finished at: 2019-02-27T01:01:27-08:00

@dilipbiswal if it's just for the antlr plugin, yes that's a good idea.

@srowen Yeah.. its for the antlr plugin. Thank you.

SparkQA · 2019-02-27T01:11:19Z

Test build #102791 has finished for PR 23897 at commit ae2adcd.

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

beliefer · 2019-02-27T03:21:33Z

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

@@ -78,11 +78,11 @@ singleTableSchema
 statement
    : query                                                            #statementDefault
    | USE db=identifier                                                #use
-    | CREATE DATABASE (IF NOT EXISTS)? identifier
+    | CREATE database (IF NOT EXISTS)? identifier


Should the keyword database change to databaseTag and reduce confuse?

@beliefer I think database in lower case indicates a parser rule and different from database in upper case which is a lexer rule. We already have a precedence in our grammar file for "identifier". IMHO keeping it as database reads better.

maropu · 2019-03-01T01:09:12Z

LGTM (I checked the console log in Jenkins and I found no warning there)

maropu · 2019-03-01T01:15:17Z

@dilipbiswal Ah, one more. Based on the Xiao's comment, can you add a simple test in PlanParserSuite to check if Schema can be parsed in antlr?

maropu · 2019-03-01T03:29:51Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

+    assertEqual("ALTER DATABASE foo SET DBPROPERTIES ('x' = 'y')",
+      parser.parsePlan("ALTER SCHEMA foo SET DBPROPERTIES ('x' = 'y')"))
+    assertEqual("DESC DATABASE foo", parser.parsePlan("DESC SCHEMA foo"))
+


nit: remove this empty line.

maropu · 2019-03-01T03:36:08Z

LGTM, pending Jenkins.

SparkQA · 2019-03-01T07:37:26Z

Test build #102893 has finished for PR 23897 at commit e3cb947.

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

SparkQA · 2019-03-01T07:58:55Z

Test build #102895 has finished for PR 23897 at commit eb278c9.

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

dongjoon-hyun · 2019-03-01T20:33:48Z

Thank you all. Merged to master.

dilipbiswal · 2019-03-01T20:52:34Z

Thanks a lot @dongjoon-hyun

## What changes were proposed in this pull request? I see the following new warning from ANTR4 after SPARK-26215 after it added `SCHEMA` keyword in the reserved/unreserved list. This is a minor PR to cleanup the warning. ``` WARNING] warning(125): org/apache/spark/sql/catalyst/parser/SqlBase.g4:784:90: implicit definition of token SCHEMA in parser [WARNING] .../apache/spark/org/apache/spark/sql/catalyst/parser/SqlBase.g4 [784:90]: implicit definition of token SCHEMA in parser ``` ## How was this patch tested? Manually built catalyst after the fix to verify Closes apache#23897 from dilipbiswal/minor_parser_token. Authored-by: Dilip Biswal <dbiswal@us.ibm.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

[SPARK-26215][FOLLOW-UP] Fix the warning from ANTR4

c710eb0

dilipbiswal changed the title ~~[SPARK-26215][FOLLOW-UP][MINOR] Fix the warning from ANTR4~~ [SPARK-26215][SQL][FOLLOW-UP][MINOR] Fix the warning from ANTR4 Feb 26, 2019

maropu approved these changes Feb 26, 2019

View reviewed changes

viirya reviewed Feb 26, 2019

View reviewed changes

Code review - thanks simon

ae2adcd

beliefer requested changes Feb 27, 2019

View reviewed changes

dilipbiswal mentioned this pull request Feb 28, 2019

[SPARK-27016][SQL][BUILD] Treat all antlr warnings as errors while generating parser from the sql grammar file. #23925

Closed

code review

e3cb947

maropu reviewed Mar 1, 2019

View reviewed changes

Remove space

eb278c9

dongjoon-hyun approved these changes Mar 1, 2019

View reviewed changes

viirya approved these changes Mar 1, 2019

View reviewed changes

dongjoon-hyun closed this in 5fd62ca Mar 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26215][SQL][FOLLOW-UP][MINOR] Fix the warning from ANTR4 #23897

[SPARK-26215][SQL][FOLLOW-UP][MINOR] Fix the warning from ANTR4 #23897

dilipbiswal commented Feb 26, 2019 •

edited

Loading

dilipbiswal commented Feb 26, 2019

maropu left a comment

maropu commented Feb 26, 2019

dilipbiswal commented Feb 26, 2019

viirya Feb 26, 2019

dilipbiswal Feb 26, 2019

gatorsmile Feb 27, 2019

dilipbiswal Feb 27, 2019

dilipbiswal Feb 27, 2019

srowen Feb 27, 2019

dilipbiswal Feb 27, 2019

gatorsmile Feb 28, 2019

maropu Mar 1, 2019

SparkQA commented Feb 27, 2019

beliefer Feb 27, 2019 •

edited

Loading

dilipbiswal Feb 28, 2019

maropu commented Mar 1, 2019

maropu commented Mar 1, 2019

maropu Mar 1, 2019

maropu commented Mar 1, 2019

SparkQA commented Mar 1, 2019

SparkQA commented Mar 1, 2019

dongjoon-hyun commented Mar 1, 2019

dilipbiswal commented Mar 1, 2019

[SPARK-26215][SQL][FOLLOW-UP][MINOR] Fix the warning from ANTR4 #23897

[SPARK-26215][SQL][FOLLOW-UP][MINOR] Fix the warning from ANTR4 #23897

Conversation

dilipbiswal commented Feb 26, 2019 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

dilipbiswal commented Feb 26, 2019

maropu left a comment

Choose a reason for hiding this comment

maropu commented Feb 26, 2019

dilipbiswal commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 27, 2019

beliefer Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Mar 1, 2019

maropu commented Mar 1, 2019

Choose a reason for hiding this comment

maropu commented Mar 1, 2019

SparkQA commented Mar 1, 2019

SparkQA commented Mar 1, 2019

dongjoon-hyun commented Mar 1, 2019

dilipbiswal commented Mar 1, 2019

dilipbiswal commented Feb 26, 2019 •

edited

Loading

beliefer Feb 27, 2019 •

edited

Loading