[SPARK-32251][SQL][DOCS][TESTS] Fix SQL keyword document #29055

cloud-fan · 2020-07-09T13:53:24Z

What changes were proposed in this pull request?

This PR improves the test to make sure all the SQL keywords are documented correctly. It fixes several issues:

some keywords are not documented
some keywords are not ANSI SQL keywords but documented as reserved/non-reserved.

Why are the changes needed?

To make sure the implementation matches the doc.

Does this PR introduce any user-facing change?

No

How was this patch tested?

new test

cloud-fan · 2020-07-09T13:54:09Z

cc @maropu @dongjoon-hyun

maropu

Cool, thanks for the updates! LGTM except for the minor comment.

maropu · 2020-07-09T14:06:33Z

docs/sql-ref-ansi-compliance.md

 |DISTINCT|reserved|non-reserved|reserved|
 |DISTRIBUTE|non-reserved|non-reserved|non-reserved|
-|DIV|non-reserved|non-reserved|non-reserved|
+|DIV|non-reserved|non-reserved|not a keyword|


Ah, I see. We cannot remove this line: non-reserved|non-reserved|not a keyword? I think users don't need to care about the words.

But is DIV a keyword?

DIV is a non-reserved keyword in Apache Spark. I guess we need to have this here.

SparkQA · 2020-07-09T15:01:00Z

Test build #125480 has finished for PR 29055 at commit f595689.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait SQLKeywordUtils extends SQLHelper
class SQLKeywordSuite extends SparkFunSuite with SQLKeywordUtils
class TableIdentifierParserSuite extends SparkFunSuite with SQLKeywordUtils

dongjoon-hyun · 2020-07-09T15:17:11Z

Retest this please.

dongjoon-hyun · 2020-07-09T15:18:13Z

Thank you for pinging me, @cloud-fan !

SparkQA · 2020-07-09T15:22:37Z

Test build #125489 has started for PR 29055 at commit f595689.

dongjoon-hyun · 2020-07-09T20:45:22Z

Retest this please

dongjoon-hyun · 2020-07-09T21:24:58Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/SQLKeywordSuite.scala

+        }
+    }
+    // DIV is an operator but is also a keyword
+    keywords + "DIV"


@cloud-fan . Instead of adding exceptions here and line 136, can we switch the original lines in SqlBase.g4? I guess you can move DIV: 'DIV'; into somewhere before //--SPARK-KEYWORD-LIST-END. WDYT?

//--SPARK-KEYWORD-LIST-END //============================ // End of the keywords list //============================ EQ : '=' | '=='; NSEQ: '<=>'; NEQ : '<>'; NEQJ: '!='; LT : '<'; LTE : '<=' | '!>'; GT : '>'; GTE : '>=' | '!<'; PLUS: '+'; MINUS: '-'; ASTERISK: '*'; SLASH: '/'; PERCENT: '%'; DIV: 'DIV';

SparkQA · 2020-07-10T04:50:06Z

Test build #125514 has finished for PR 29055 at commit f595689.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait SQLKeywordUtils extends SQLHelper
class SQLKeywordSuite extends SparkFunSuite with SQLKeywordUtils
class TableIdentifierParserSuite extends SparkFunSuite with SQLKeywordUtils

maropu · 2020-07-10T05:04:09Z

retest this please

SparkQA · 2020-07-10T07:05:02Z

Test build #125559 has finished for PR 29055 at commit 71e0c13.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-10T07:05:02Z

Test build #125557 has finished for PR 29055 at commit f595689.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait SQLKeywordUtils extends SQLHelper
class SQLKeywordSuite extends SparkFunSuite with SQLKeywordUtils
class TableIdentifierParserSuite extends SparkFunSuite with SQLKeywordUtils

yaooqinn · 2020-07-10T08:02:35Z

docs/sql-ref-ansi-compliance.md

 Below is a list of all the keywords in Spark SQL.

-|Keyword|Spark SQL<br/>ANSI Mode|Spark SQL<br/>Default Mode|SQL-2011|
+|Keyword|Spark SQL<br/>ANSI Mode|Spark SQL<br/>Default Mode|SQL-2016|


Is there any consensus to decide which version of ANSI SQL standard that we actually try to comply with?

Or any protocol to define the compatibility between Spark versions and ANSI SQL standards？

I think the latest ANSI SQL standard is preferred.

Also to note that: ANSI compliant doesn't mean to follow everything in ANSI SQL. e.g. a keyword reserved in ANSI SQL can be non-reserved in Spark.

Looking at the history of this standard, the trend of it seems to add more and more keywords and change the non-reserved to reserved. I guess it's ok to follow the latest one here as literally we will add migration guide for every newly added keyword.

dongjoon-hyun · 2020-07-10T15:24:41Z

Retest this please

dongjoon-hyun

+1, LGTM. Thank you, @cloud-fan .

dongjoon-hyun · 2020-07-10T22:09:54Z

Merged to master/3.0 because all Scala/Java UT passed in the current run. PySpark and SparkR UT is irrelevant to this PR.
Thank you, @cloud-fan and all.

### What changes were proposed in this pull request? This PR improves the test to make sure all the SQL keywords are documented correctly. It fixes several issues: 1. some keywords are not documented 2. some keywords are not ANSI SQL keywords but documented as reserved/non-reserved. ### Why are the changes needed? To make sure the implementation matches the doc. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test Closes #29055 from cloud-fan/keyword. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 84db660) Signed-off-by: Dongjoon Hyun <[email protected]>

SparkQA · 2020-07-10T23:05:01Z

Test build #125622 has finished for PR 29055 at commit 71e0c13.

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

fix SQL keyword document

f595689

probot-autolabeler bot added DOCS SQL labels Jul 9, 2020

maropu approved these changes Jul 9, 2020

View reviewed changes

dongjoon-hyun reviewed Jul 9, 2020

View reviewed changes

maropu changed the title ~~[SPARK-32251][SQL][DOC] fix SQL keyword document~~ [SPARK-32251][SQL][DOC][TEST] fix SQL keyword document Jul 9, 2020

HyukjinKwon changed the title ~~[SPARK-32251][SQL][DOC][TEST] fix SQL keyword document~~ [SPARK-32251][SQL][DOCS][TESTS] Fix SQL keyword document Jul 10, 2020

address comment

71e0c13

yaooqinn reviewed Jul 10, 2020

View reviewed changes

dongjoon-hyun approved these changes Jul 10, 2020

View reviewed changes

dongjoon-hyun closed this in 84db660 Jul 10, 2020

[SPARK-32251][SQL][DOCS][TESTS] Fix SQL keyword document #29055

[SPARK-32251][SQL][DOCS][TESTS] Fix SQL keyword document #29055

Uh oh!

Conversation

cloud-fan commented Jul 9, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

cloud-fan commented Jul 9, 2020

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

maropu Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 9, 2020

Uh oh!

dongjoon-hyun commented Jul 9, 2020

Uh oh!

dongjoon-hyun commented Jul 9, 2020

Uh oh!

SparkQA commented Jul 9, 2020

Uh oh!

dongjoon-hyun commented Jul 9, 2020

Uh oh!

dongjoon-hyun Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

maropu commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

yaooqinn Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 10, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maropu Jul 9, 2020 •

edited

Loading

dongjoon-hyun Jul 9, 2020 •

edited

Loading

dongjoon-hyun commented Jul 10, 2020 •

edited

Loading