Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dilipbiswal wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c710eb0
[SPARK-26215][FOLLOW-UP] Fix the warning from ANTR4
dilipbiswal Feb 26, 2019
ae2adcd
Code review - thanks simon
dilipbiswal Feb 26, 2019
e3cb947
code review
dilipbiswal Mar 1, 2019
eb278c9
Remove space
dilipbiswal Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ number
| MINUS? BIGDECIMAL_LITERAL #bigDecimalLiteral
;

// NOTE: You must follow a rule below when you add a new ANTLR taken in this file:
// NOTE: You must follow a rule below when you add a new ANTLR token in this file:
// - All the ANTLR tokens = UNION(`ansiReserved`, `ansiNonReserved`) = UNION(`defaultReserved`, `nonReserved`)
//
// Let's say you add a new token `NEWTOKEN` and this is not reserved regardless of a `spark.sql.parser.ansi.enabled`
Expand Down Expand Up @@ -1014,7 +1014,8 @@ SORTED: 'SORTED';
PURGE: 'PURGE';
INPUTFORMAT: 'INPUTFORMAT';
OUTPUTFORMAT: 'OUTPUTFORMAT';
DATABASE: 'DATABASE' | 'SCHEMA';
SCHEMA: 'SCHEMA';
DATABASE: 'DATABASE';

@viirya viirya Feb 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dilipbiswal dilipbiswal Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@gatorsmile gatorsmile Feb 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test to capture this?

@dilipbiswal dilipbiswal Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dilipbiswal dilipbiswal Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@srowen srowen Feb 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dilipbiswal dilipbiswal Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gatorsmile gatorsmile Feb 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@maropu maropu Mar 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

DATABASES: 'DATABASES' | 'SCHEMAS';
DFS: 'DFS';
TRUNCATE: 'TRUNCATE';
Expand Down