[SPARK-23264][SQL] Make INTERVAL keyword optional in INTERVAL clauses when ANSI mode enabled #20433

maropu · 2018-01-30T06:25:23Z

What changes were proposed in this pull request?

This pr updated parsing rules in SqlBase.g4 to support a SQL query below when ANSI mode enabled;

SELECT CAST('2017-08-04' AS DATE) + 1 days;

The current master cannot parse it though, other dbms-like systems support the syntax (e.g., hive and mysql). Also, the syntax is frequently used in the official TPC-DS queries.

This pr added new tokens as follows;

YEAR | YEARS | MONTH | MONTHS | WEEK | WEEKS | DAY | DAYS | HOUR | HOURS | MINUTE 
MINUTES | SECOND | SECONDS | MILLISECOND | MILLISECONDS | MICROSECOND | MICROSECONDS

Then, it registered the keywords below as the ANSI reserved (this follows SQL-2011);

 DAY | HOUR | MINUTE | MONTH | SECOND | YEAR

How was this patch tested?

Added tests in SQLQuerySuite, ExpressionParserSuite, and TableIdentifierParserSuite.

SparkQA · 2018-01-30T08:05:01Z

Test build #86799 has finished for PR 20433 at commit 830cf8d.

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

maropu · 2018-01-30T08:33:20Z

retest this please.

SparkQA · 2018-01-30T11:34:02Z

Test build #86810 has finished for PR 20433 at commit 830cf8d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-31T02:52:11Z

Test build #86849 has finished for PR 20433 at commit 4173ff0.

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

jiangxb1987 · 2018-02-01T00:57:53Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala

@@ -561,8 +561,11 @@ class ExpressionParserSuite extends PlanTest {
      Literal(CalendarInterval.fromSingleUnitString(u, s))
    }

-    // Empty interval statement
-    intercept("interval", "at least one time unit should be given for interval literal")


we shall still check the empty interval statement, now it shall produce a different error message?

yea, antlr just throws an exception when hitting this case;

scala> sql("select cast('2018-01-12' as DATE) + 1 days").show +---------------------------------------------------------------------------+ |CAST(CAST(CAST(2018-01-12 AS DATE) AS TIMESTAMP) + interval 1 days AS DATE)| +---------------------------------------------------------------------------+ | 2018-01-13| +---------------------------------------------------------------------------+ scala> sql("select cast('2018-01-12' as DATE) + interval").show org.apache.spark.sql.AnalysisException: cannot resolve '`interval`' given input columns: []; line 1 pos 36; 'Project [unresolvedalias((cast(2018-01-12 as date) + 'interval), None)] +- OneRowRelation

maropu · 2018-03-02T10:06:50Z

@gatorsmile kindly ping

maropu · 2018-03-06T05:40:09Z

ping

gatorsmile · 2018-03-06T07:15:06Z

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

+HOUR: 'HOUR' | 'HOURS';
+MINUTE: 'MINUTE' | 'MINUTES';
+SECOND: 'SECOND' | 'SECONDS';
+MILLISECOND: 'MILLISECOND' | 'MILLISECONDS';


I am wondering which systems support MILLISECOND , MICROSECOND and NANOSECOND ?

nvm, it sounds like we already support them.

gatorsmile · 2018-03-06T07:20:52Z

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

+SECOND: 'SECOND' | 'SECONDS';
+MILLISECOND: 'MILLISECOND' | 'MILLISECONDS';
+MICROSECOND: 'MICROSECOND' | 'MICROSECONDS';
+NANOSECOND: 'NANOSECOND' | 'NANOSECONDS';


We do not support nanosecond.

gatorsmile · 2018-03-06T07:21:50Z

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

@@ -790,6 +796,16 @@ ASC: 'ASC';
 DESC: 'DESC';
 FOR: 'FOR';
 INTERVAL: 'INTERVAL';
+YEAR: 'YEAR' | 'YEARS';


Also update TableIdentifierParserSuite

gatorsmile · 2018-03-06T07:24:50Z

Could you create interval.sql by adding the test cases in https://issues.apache.org/jira/browse/HIVE-13557 ?

BTW, check the discussion in that JIRA?

maropu · 2018-03-06T07:29:59Z

ok, I'll update based on the comments soon

maropu · 2018-03-06T07:37:38Z

You meant the HIVE jira? If so, no (I'm checking now). Any point I should know?

maropu · 2018-03-06T09:55:55Z

better to port all the related tests in Hive clientpositive into the Spark sql query tests?

# hive-master/ql/src/test/queries/clientpositive: (master=)$ls | grep interval
interval_1.q
interval_2.q
interval_3.q
interval_alt.q
interval_arithmetic.q
interval_comparison.q
...

maropu · 2018-03-13T04:00:47Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+      None
+    }
+  }
+


This is not related to this pr though, I think it is some useful to run tests selectively in SQLQueryTestSuite (cuz the number of tests there grows recently...). If possibly, could we add this feature in a separate pr? Otherwise, I'll drop this.

Let us create a separate PR?

ok, I'll do later.

SparkQA · 2018-03-13T04:02:40Z

Test build #88191 has finished for PR 20433 at commit c0710d6.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-03-13T04:50:44Z

This sounds good to me

maropu · 2018-03-13T04:53:08Z

ok, I added related tests from hive clientpositive in interval.sql.

SparkQA · 2018-03-13T06:35:21Z

Test build #88192 has finished for PR 20433 at commit 1eec819.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-13T07:05:01Z

Test build #88198 has finished for PR 20433 at commit 39fe5dc.

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

SparkQA · 2018-03-13T12:54:21Z

Test build #88204 has finished for PR 20433 at commit ba6cce5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-13T18:18:24Z

Test build #88209 has finished for PR 20433 at commit f6210a2.

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

gatorsmile · 2018-03-19T05:52:44Z

sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala

@@ -155,6 +155,7 @@ class QueryExecution(val sparkSession: SparkSession, val logical: LogicalPlan) {
      case (null, _) => "null"
      case (s: String, StringType) => "\"" + s + "\""
      case (decimal, DecimalType()) => decimal.toString
+      case (interval, CalendarIntervalType) => interval.toString


Do we have a test case to capture this change?

ok, I'll try to add tests for that.

Added: https://github.com/apache/spark/pull/20433/files#diff-b608c1c9b761ffb3a19bf5c0cd971e43R43

gatorsmile · 2018-03-19T06:22:05Z

sql/core/src/test/resources/sql-tests/results/interval.sql.out

+struct<>
+-- !query 30 output
+org.apache.spark.sql.AnalysisException
+cannot resolve '(DATE '2012-01-01' + (t.`a` + 1))' due to data type mismatch: differing types in '(DATE '2012-01-01' + (t.`a` + 1))' (date and int).; line 1 pos 7


How about the columns having the matched data type?

~~ok, I'll add tests to check~~
You mean interval-interval cases? If so, this one? https://github.com/apache/spark/pull/20433/files#diff-24539a8bfdac0a14cec58755dd6565dbR237

gatorsmile · 2018-03-19T06:26:56Z

Change the PR title to [SPARK-23264][SQL] Make INTERVAL keyword optional in INTERVAL clauses?

maropu · 2019-01-30T02:48:30Z

aha, got it.
Based on the ANSI one below, I think the the plural from is not reserved:
https://developer.mimer.com/wp-content/uploads/2018/05/Standard-SQL-Reserved-Words-Summary.pdf
So, to comply with the ANSI SQL, we need to reserve year, month, week, day, hour, minute, second, millisecond, and microsecond for the interval.

Ah,... I just noticed the parser regards 2 seconds as a interval is not a ANSI behaviour because seconds is not reserved...? Postgre/Hive just regards 2 seconds as a interval though.

Anyway, we should include the fix to reserve these keywords in this pr?
To do so, we need to port some logics from #23259(https://github.com/apache/spark/pull/23259/files#diff-9847f5cef7cf7fbc5830fbc6b779ee10R1405) to here, and then I feel this pr gets a little complicated?

cloud-fan · 2019-01-30T02:59:29Z

My understanding is, this PR is blocked by #23259. Without a well-defined list of keywords, we can't simply change the parser rule of interval literal.

To unblock it, we want to build a framework to optionally(controlled by a config) define something as keyword, and then define the interval related keywords first.

I just noticed the parser regards 2 seconds as a interval is not a ANSI behaviour because seconds is not reserved

Can you check some mainstream databases like postgres, sql server, oracle?

maropu · 2019-01-30T03:34:37Z

Can you check some mainstream databases like postgres, sql server, oracle?

ok, I'll check.

To unblock it, we want to build a framework to optionally(controlled by a config) define something as > keyword, and then define the interval related keywords first.

ok, I'll simplify #23259 first to build the framework, then I'll revisit this.

maropu · 2019-02-17T12:30:17Z

I checked if the time unit keywords (YEAR, MONTH, DAY, HOUR, MINUTE, SECOND, MILLISECOND, WEEK, MILLISECOND, MICROSECOND) are reserved in SQL-2011/database implementations:

SQL-2011

reserved: YEAR, MONTH, DAY, HOUR, MINUTE, SECOND
non-reserved: WEEK, MILLISECOND, MICROSECOND

PostgreSQL, MySQL, Oracle, and SQL Server

reserved:
non-reserved: YEAR, MONTH, WEEK, DAY, HOUR, MINUTE, SECOND, MILLISECOND, MICROSECOND

Also, all the the plural forms (YEARS, MONTHS, ...) are not reserved in all the cases.
I'm not sure about the historical reason why these databases don't handle them as reserved though, IMHO we would be better to follow SQL-2011; YEAR, MONTH, DAY, HOUR, MINUTE, and SECOND are reserved if ansi=true.

As for SQL server, these time unit keywords are listed in future keywords:
https://docs.microsoft.com/ja-jp/sql/t-sql/language-elements/reserved-keywords-transact-sql?view=sql-server-2017#future-keywords

SparkQA · 2019-02-17T16:36:39Z

Test build #102428 has finished for PR 20433 at commit e11cb2a.

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

maropu · 2019-02-24T01:00:32Z

sql/core/src/test/resources/sql-tests/inputs/interval.sql

@@ -0,0 +1,188 @@
+-- Turns on ANSI mode
+SET spark.sql.parser.ansi.enabled=true;


How about moving these kinds of ANSI-related tests into a new dir sql-tests/inputs/ansi/?

SparkQA · 2019-02-24T04:51:35Z

Test build #102714 has finished for PR 20433 at commit 41645a8.

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

SparkQA · 2019-02-24T05:00:06Z

Test build #102715 has finished for PR 20433 at commit 5699547.

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

maropu · 2019-03-08T12:14:53Z

ping @cloud-fan

SparkQA · 2019-03-11T04:14:27Z

Test build #103287 has finished for PR 20433 at commit a76f8a9.

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

SparkQA · 2019-03-13T07:05:02Z

Test build #103413 has finished for PR 20433 at commit feccbc8.

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

maropu · 2019-03-13T07:06:06Z

retest this please

SparkQA · 2019-03-13T10:10:38Z

Test build #103421 has finished for PR 20433 at commit feccbc8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-03-13T12:36:52Z

LGTM except one comment: #20433 (comment)

SparkQA · 2019-03-13T22:02:58Z

Test build #103445 has finished for PR 20433 at commit 8e414ac.

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

maropu · 2019-03-14T01:47:37Z

Thanks! Merged to master.

… when ANSI mode enabled ## What changes were proposed in this pull request? This pr updated parsing rules in `SqlBase.g4` to support a SQL query below when ANSI mode enabled; ``` SELECT CAST('2017-08-04' AS DATE) + 1 days; ``` The current master cannot parse it though, other dbms-like systems support the syntax (e.g., hive and mysql). Also, the syntax is frequently used in the official TPC-DS queries. This pr added new tokens as follows; ``` YEAR | YEARS | MONTH | MONTHS | WEEK | WEEKS | DAY | DAYS | HOUR | HOURS | MINUTE MINUTES | SECOND | SECONDS | MILLISECOND | MILLISECONDS | MICROSECOND | MICROSECONDS ``` Then, it registered the keywords below as the ANSI reserved (this follows SQL-2011); ``` DAY | HOUR | MINUTE | MONTH | SECOND | YEAR ``` ## How was this patch tested? Added tests in `SQLQuerySuite`, `ExpressionParserSuite`, and `TableIdentifierParserSuite`. Closes #20433 from maropu/SPARK-23264. Authored-by: Takeshi Yamamuro <yamamuro@apache.org> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

…nabled" ### What changes were proposed in this pull request? Revert #20433 . ### Why are the changes needed? According to the SQL standard, the INTERVAL prefix is required: ``` <interval literal> ::= INTERVAL [ <sign> ] <interval string> <interval qualifier> <interval string> ::= <quote> <unquoted interval string> <quote> ``` ### Does this PR introduce any user-facing change? yes, but omitting the INTERVAL prefix is a new feature in 3.0 ### How was this patch tested? existing tests Closes #27080 from cloud-fan/interval. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Xiao Li <gatorsmile@gmail.com>

maropu mentioned this pull request Jan 30, 2018

[SPARK-23167][SQL] Add TPCDS queries v2.7 in TPCDSQuerySuite #20343

Closed

jiangxb1987 reviewed Feb 1, 2018

View reviewed changes

gatorsmile reviewed Mar 6, 2018

View reviewed changes

maropu force-pushed the SPARK-23264 branch from 4173ff0 to c0710d6 Compare March 13, 2018 03:56

maropu commented Mar 13, 2018

View reviewed changes

maropu force-pushed the SPARK-23264 branch from c0710d6 to 1eec819 Compare March 13, 2018 04:09

maropu force-pushed the SPARK-23264 branch from ba6cce5 to f6210a2 Compare March 13, 2018 14:52

maropu mentioned this pull request Mar 14, 2018

[SPARK-23662][SQL] Support selective tests in SQLQueryTestSuite #20808

Closed

gatorsmile reviewed Mar 19, 2018

View reviewed changes

maropu force-pushed the SPARK-23264 branch from 7cb8f87 to e11cb2a Compare February 17, 2019 12:17

maropu force-pushed the SPARK-23264 branch 2 times, most recently from 41645a8 to 5699547 Compare February 24, 2019 00:48

maropu commented Feb 24, 2019

View reviewed changes

maropu force-pushed the SPARK-23264 branch from 5699547 to a76f8a9 Compare March 11, 2019 00:10

maropu added 2 commits March 13, 2019 11:29

Fix

0743ac3

Fix

feccbc8

maropu force-pushed the SPARK-23264 branch from a76f8a9 to feccbc8 Compare March 13, 2019 05:56

maropu changed the title ~~[SPARK-23264][SQL] Make INTERVAL keyword optional in INTERVAL clauses~~ [SPARK-23264][SQL] Make INTERVAL keyword optional in INTERVAL clauses when ANSI mode enabled Mar 13, 2019

Fix

8e414ac

maropu closed this Mar 14, 2019

cloud-fan mentioned this pull request Jan 2, 2020

Revert "[SPARK-23264][SQL] Make INTERVAL keyword optional when ANSI enabled" #27080

Closed

		@@ -0,0 +1,188 @@
		-- Turns on ANSI mode
		SET spark.sql.parser.ansi.enabled=true;

[SPARK-23264][SQL] Make INTERVAL keyword optional in INTERVAL clauses when ANSI mode enabled #20433

[SPARK-23264][SQL] Make INTERVAL keyword optional in INTERVAL clauses when ANSI mode enabled #20433

Conversation

maropu commented Jan 30, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jan 30, 2018

maropu commented Jan 30, 2018

SparkQA commented Jan 30, 2018

SparkQA commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Mar 2, 2018 • edited Loading

maropu commented Mar 6, 2018

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

gatorsmile commented Mar 6, 2018 • edited Loading

maropu commented Mar 6, 2018

maropu commented Mar 6, 2018 • edited Loading

maropu commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 13, 2018

gatorsmile commented Mar 13, 2018

maropu commented Mar 13, 2018 • edited Loading

SparkQA commented Mar 13, 2018

SparkQA commented Mar 13, 2018

SparkQA commented Mar 13, 2018

SparkQA commented Mar 13, 2018

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

maropu Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

gatorsmile commented Mar 19, 2018

maropu commented Jan 30, 2019

cloud-fan commented Jan 30, 2019

maropu commented Jan 30, 2019 • edited Loading

maropu commented Feb 17, 2019

SparkQA commented Feb 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 24, 2019

SparkQA commented Feb 24, 2019

maropu commented Mar 8, 2019

SparkQA commented Mar 11, 2019

SparkQA commented Mar 13, 2019

maropu commented Mar 13, 2019

SparkQA commented Mar 13, 2019

cloud-fan commented Mar 13, 2019

SparkQA commented Mar 13, 2019

maropu commented Mar 14, 2019

maropu commented Jan 30, 2018 •

edited

Loading

maropu commented Mar 2, 2018 •

edited

Loading

gatorsmile commented Mar 6, 2018 •

edited

Loading

maropu commented Mar 6, 2018 •

edited

Loading

maropu commented Mar 13, 2018 •

edited

Loading

maropu Mar 19, 2018 •

edited

Loading

maropu commented Jan 30, 2019 •

edited

Loading