[SPARK-27578][SQL] Support INTERVAL ... HOUR TO SECOND syntax #24472

lipzhu · 2019-04-26T12:07:58Z

What changes were proposed in this pull request?

Currently, SparkSQL can support interval format like this.

SELECT INTERVAL '0 23:59:59.155' DAY TO SECOND

Like Presto/Teradata, this PR aims to support grammar like below.

SELECT INTERVAL '23:59:59.155' HOUR TO SECOND

Although we can add a new function for this pattern, we had better extend the existing code to handle a missing day case. So, the following is also supported.

SELECT INTERVAL '23:59:59.155' DAY TO SECOND
SELECT INTERVAL '1 23:59:59.155' HOUR TO SECOND

Currently Vertica/Teradata/Postgresql/SQL Server have fully support of below interval functions.

interval ... year to month
interval ... day to hour
interval ... day to minute
interval ... day to second
interval ... hour to minute
interval ... hour to second
interval ... minute to second

https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Literals/interval-qualifier.htm
https://github.com/postgres/postgres/blob/df1a699e5ba3232f373790b2c9485ddf720c4a70/src/test/regress/sql/interval.sql#L180-L203
https://docs.teradata.com/reader/S0Fw2AVH8ff3MDA0wDOHlQ/KdCtT3pYFo~_enc8~kGKVw
https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/interval-literals?view=sql-server-2017

How was this patch tested?

Pass the Jenkins with the updated test cases.

wangyum · 2019-05-21T15:53:29Z

cc @MaxGekk

srowen

Seems reasonable but I don't know this part well

srowen · 2019-05-22T20:20:21Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

@@ -56,6 +56,9 @@ private static String unitRegex(String unit) {
  private static Pattern dayTimePattern =
    Pattern.compile("^(?:['|\"])?([+|-])?(\\d+) (\\d+):(\\d+):(\\d+)(\\.(\\d+))?(?:['|\"])?$");

+  private static Pattern hourTimePattern =


BTW all of these Patterns could be final.

srowen · 2019-05-22T20:21:11Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

+   *
+   */
+  public static CalendarInterval fromHourTimeString(String s) throws IllegalArgumentException {
+    CalendarInterval result;


result is really superfluous here

lipzhu · 2019-05-24T10:08:33Z

@srowen Thanks for your suggestion.

dongjoon-hyun · 2019-05-26T00:40:33Z

ok to test

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

dongjoon-hyun

Hi, @lipzhu .
Thank you for the contribution. I left a few comments. Could you update the PR. We can proceed the next round review after fixing the basic stuffs. Thanks!

SparkQA · 2019-05-26T03:23:15Z

Test build #105784 has finished for PR 24472 at commit 688c53b.

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

lipzhu · 2019-05-27T10:33:34Z

@dongjoon-hyun Thanks for your review and suggestions. I just did some changes according to your suggestions.

SparkQA · 2019-05-27T12:48:21Z

Test build #105833 has finished for PR 24472 at commit 424fa85.

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

dongjoon-hyun

Thank you for updating, @lipzhu . I did another round of reviews and realized that we don't need to add a new function of 30 lines. What we need is a pattern change. I made a PR to you. Please review and merge that.

fix lipzhu/spark#2

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

fix

lipzhu · 2019-05-28T02:13:32Z

@dongjoon-hyun Thanks for your update to remove the duplicate codes.

dongjoon-hyun · 2019-05-28T04:11:39Z

Hi, @gatorsmile and @cloud-fan .

Could you give us some directional advice, please?

First, this PR wants to support INTERVAL ... HOUR TO SECOND like INTERVAL ... DAY TO SECOND like Presto/Terradata. It looks reasonable to me, too.
Second, originally, this PR added a new pattern and new function (which is similar to the existing one). To avoid maintaining two similar functions, I recommended to extend the existing pattern and handling DAY and HOUR with the same function. To sum up, we will support 2~4 additionally.

SELECT INTERVAL '0 23:59:59.155' DAY TO SECOND (Current Spark)
SELECT INTERVAL '23:59:59.155' HOUR TO SECOND
SELECT INTERVAL '23:59:59.155' DAY TO SECOND
SELECT INTERVAL '1 23:59:59.155' HOUR TO SECOND

If you think these are okay, I want to merge this PR. How do you think about this?

SparkQA · 2019-05-28T04:24:53Z

Test build #105852 has finished for PR 24472 at commit 29fcc08.

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

dongjoon-hyun · 2019-05-30T03:33:48Z

Retest this please.

SparkQA · 2019-05-30T07:05:01Z

Test build #105948 has finished for PR 24472 at commit 0ee3bc9.

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

wangyum · 2019-05-30T09:57:31Z

retest this please

SparkQA · 2019-05-30T11:37:57Z

Test build #105958 has finished for PR 24472 at commit 0ee3bc9.

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

wangyum · 2019-05-31T05:37:47Z

retest this please

SparkQA · 2019-05-31T07:05:02Z

Test build #105993 has finished for PR 24472 at commit 0ee3bc9.

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

wangyum · 2019-05-31T07:10:57Z

retest this please

SparkQA · 2019-05-31T09:47:24Z

Test build #106001 has finished for PR 24472 at commit 0ee3bc9.

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

lipzhu · 2019-05-31T10:02:50Z

List more patterns which are supported in Teradata and PostgreSQL but not supported yet in SparkSQL.

interval ... day to hour
interval ... day to minute
interval ... day to second
interval ... hour to minute
interval ... minute to second
...

dongjoon-hyun · 2019-05-31T16:56:13Z

Yep. I know. @lipzhu :) There is a trade-off always. Although Spark SQL was designed to be compatible with Hive, but we officially have unsupported features, too.

https://spark.apache.org/docs/latest/sql-migration-guide-hive-compatibility.html#unsupported-hive-functionality

Since each SQL engine works differently, Apache Spark adds features based on the trade-off. Apache Spark cannot embrace all SQL engine behavior in terms of compatibility, and it's technically impossible.

dongjoon-hyun

Hi, @cloud-fan and @gatorsmile . As I asked before, I support this feature.

[SPARK-27578][SQL] Support INTERVAL ... HOUR TO SECOND syntax #24472 (comment)

We can accept in this way or the original way (by reverting the last two commit in order to keep two patterns).

For now, I prefer the current one with the minimal changes, but also am open to both ways. Please let us know PMC's opinions (positive or negative). If there is no further comments, I'll proceed to merge this into master branch in the next week.

gatorsmile · 2019-06-03T05:47:34Z

In the next one month, Wenchen might not be able to respond your pings very soon. Normally, when we add a support like this, we need to know whether all the other SQL engines have similar supports?

@lipzhu Could you help do an investigation? For example, Oracle, DB2, SQL Server, MySQL, PostgreSQL, Hive?

gatorsmile · 2019-06-03T05:50:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

@@ -1770,7 +1770,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
   * Create a [[CalendarInterval]] for a unit value pair. Two unit configuration types are
   * supported:
   * - Single unit.
-   * - From-To unit (only 'YEAR TO MONTH' and 'DAY TO SECOND' are supported).
+   * - From-To unit (only 'YEAR TO MONTH' and 'DAY TO SECOND' and 'HOUR to SECOND' are supported).


It sounds like we only added a very specific case. https://docs.teradata.com/reader/S0Fw2AVH8ff3MDA0wDOHlQ/RADkJCor1nDoyeD2T1VX5A

Any reason?

This is because INTERVAL ... HOUR to SECOND are most used in our existing scripts. If needed, maybe other cases like INTERVAL .. X to X can be contributed.

lipzhu · 2019-06-03T06:35:30Z

@gatorsmile
Just as I list before. Postgresql/Teradata/SQL Server have fully support of below interval functions.
Postgresqlhttps://github.com/postgres/postgres/blob/df1a699e5ba3232f373790b2c9485ddf720c4a70/src/test/regress/sql/interval.sql#L180-L203
Teradatahttps://docs.teradata.com/reader/S0Fw2AVH8ff3MDA0wDOHlQ/KdCtT3pYFo~_enc8~kGKVw
SqlServerhttps://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/interval-literals?view=sql-server-2017

interval ... year to month
interval ... day to hour
interval ... day to minute
interval ... day to second
interval ... hour to minute
interval ... minute to second
...

The other DB engines

Hive/ORACLE are partial support the interval functions I list.
DB2/MySQL are totally not supported.

wangyum · 2019-06-04T09:11:18Z

Vertica also support HOUR TO SECOND:
https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Literals/interval-qualifier.htm

@lipzhu Could you add #24472 (comment) and this to PR description?

lipzhu · 2019-06-05T01:58:48Z

Vertica also support HOUR TO SECOND:
https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Literals/interval-qualifier.htm

@lipzhu Could you add #24472 (comment) and this to PR description?

@wangyum Just update the PR description.

dongjoon-hyun · 2019-06-09T02:53:11Z

Retest this please.

dongjoon-hyun · 2019-06-09T02:54:10Z

So, could you give us some further direction, @gatorsmile ?

SparkQA · 2019-06-09T05:18:24Z

Test build #106319 has finished for PR 24472 at commit 0ee3bc9.

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

dongjoon-hyun · 2019-06-09T18:05:03Z

Hi, @gatorsmile . If you want a full support like PostgreSQL as a part of SPARK-27764, please make a decision for @lipzhu to go for it. We don't have much time for Spark 3.0.0. And, 3.1 will be next year.

HyukjinKwon · 2019-06-13T11:53:44Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

@@ -1770,7 +1770,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
   * Create a [[CalendarInterval]] for a unit value pair. Two unit configuration types are
   * supported:
   * - Single unit.
-   * - From-To unit (only 'YEAR TO MONTH' and 'DAY TO SECOND' are supported).
+   * - From-To unit (only 'YEAR TO MONTH' and 'DAY TO SECOND' and 'HOUR to SECOND' are supported).


nit: 'YEAR TO MONTH' and 'DAY TO SECOND' -> 'YEAR TO MONTH', 'DAY TO SECOND'

HyukjinKwon · 2019-06-13T11:57:19Z

Will we add interval ... minute to second support too to match it to PostgreSQL?

HyukjinKwon

change looks good

dongjoon-hyun

Thank you for review and approval, @HyukjinKwon .
I'll merge this to the master to move forward. We can support more syntax gradually.

Thank you, @lipzhu , @gatorsmile , @wangyum , too.

## What changes were proposed in this pull request? Currently, SparkSQL can support interval format like this. ```sql SELECT INTERVAL '0 23:59:59.155' DAY TO SECOND ``` Like Presto/Teradata, this PR aims to support grammar like below. ```sql SELECT INTERVAL '23:59:59.155' HOUR TO SECOND ``` Although we can add a new function for this pattern, we had better extend the existing code to handle a missing day case. So, the following is also supported. ```sql SELECT INTERVAL '23:59:59.155' DAY TO SECOND SELECT INTERVAL '1 23:59:59.155' HOUR TO SECOND ``` Currently Vertica/Teradata/Postgresql/SQL Server have fully support of below interval functions. - interval ... year to month - interval ... day to hour - interval ... day to minute - interval ... day to second - interval ... hour to minute - interval ... hour to second - interval ... minute to second https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Literals/interval-qualifier.htm https://github.com/postgres/postgres/blob/df1a699e5ba3232f373790b2c9485ddf720c4a70/src/test/regress/sql/interval.sql#L180-L203 https://docs.teradata.com/reader/S0Fw2AVH8ff3MDA0wDOHlQ/KdCtT3pYFo~_enc8~kGKVw https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/interval-literals?view=sql-server-2017 ## How was this patch tested? Pass the Jenkins with the updated test cases. Closes apache#24472 from lipzhu/SPARK-27578. Lead-authored-by: Zhu, Lipeng <lipzhu@ebay.com> Co-authored-by: Dongjoon Hyun <dhyun@apple.com> Co-authored-by: Lipeng Zhu <lipzhu@icloud.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

lipzhu · 2019-06-19T12:13:17Z

@dongjoon-hyun , Thanks for your patience on this.

I'll merge this to the master to move forward. We can support more syntax gradually.

According to the 03 ANSI SQL rule
I just create a JIRA to follow the other interval type conversion.

dongjoon-hyun · 2019-06-19T14:58:18Z

Yep. Go for it please, @lipzhu !

add interver hour to second

e4cffdd

srowen reviewed May 22, 2019

View reviewed changes

remove unused variable

688c53b