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
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
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
18 changes: 12 additions & 6 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 @@ -78,11 +78,11 @@ singleTableSchema
statement
: query #statementDefault
| USE db=identifier #use
| CREATE DATABASE (IF NOT EXISTS)? identifier
| CREATE database (IF NOT EXISTS)? identifier
Copy link
Contributor

@beliefer beliefer Feb 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should the keyword database change to databaseTag and reduce confuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

(COMMENT comment=STRING)? locationSpec?
(WITH DBPROPERTIES tablePropertyList)? #createDatabase
| ALTER DATABASE identifier SET DBPROPERTIES tablePropertyList #setDatabaseProperties
| DROP DATABASE (IF EXISTS)? identifier (RESTRICT | CASCADE)? #dropDatabase
| ALTER database identifier SET DBPROPERTIES tablePropertyList #setDatabaseProperties
| DROP database (IF EXISTS)? identifier (RESTRICT | CASCADE)? #dropDatabase
| createTableHeader ('(' colTypeList ')')? tableProvider
((OPTIONS options=tablePropertyList) |
(PARTITIONED BY partitionColumnNames=identifierList) |
Expand Down Expand Up @@ -163,7 +163,7 @@ statement
(LIKE? (qualifiedName | pattern=STRING))? #showFunctions
| SHOW CREATE TABLE tableIdentifier #showCreateTable
| (DESC | DESCRIBE) FUNCTION EXTENDED? describeFuncName #describeFunction
| (DESC | DESCRIBE) DATABASE EXTENDED? identifier #describeDatabase
| (DESC | DESCRIBE) database EXTENDED? identifier #describeDatabase
| (DESC | DESCRIBE) TABLE? option=(EXTENDED | FORMATTED)?
tableIdentifier partitionSpec? describeColName? #describeTable
| REFRESH TABLE tableIdentifier #refreshTable
Expand Down Expand Up @@ -274,6 +274,11 @@ partitionVal
: identifier (EQ constant)?
;

database
: DATABASE
| SCHEMA
;

describeFuncName
: qualifiedName
| STRING
Expand Down Expand Up @@ -750,7 +755,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 +1019,8 @@ SORTED: 'SORTED';
PURGE: 'PURGE';
INPUTFORMAT: 'INPUTFORMAT';
OUTPUTFORMAT: 'OUTPUTFORMAT';
DATABASE: 'DATABASE' | 'SCHEMA';
SCHEMA: 'SCHEMA';
DATABASE: 'DATABASE';
Copy link
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

Copy link
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.

Copy link
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?

Copy link
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 ?

Copy link
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

Copy link
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.

Copy link
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.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
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
8 changes: 8 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,4 +387,12 @@ class SparkSqlParserSuite extends AnalysisTest {
"INSERT INTO tbl2 SELECT * WHERE jt.id > 4",
"Operation not allowed: ALTER VIEW ... AS FROM ... [INSERT INTO ...]+")
}

test("database and schema tokens are interchangeable") {
assertEqual("CREATE DATABASE foo", parser.parsePlan("CREATE SCHEMA foo"))
assertEqual("DROP DATABASE foo", parser.parsePlan("DROP SCHEMA foo"))
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"))
}
}