[SPARK-35737][SQL] Parse day-time interval literals to tightest types by sarutak · Pull Request #32892 · apache/spark

sarutak · 2021-06-12T17:58:07Z

What changes were proposed in this pull request?

This PR add a feature which parse day-time interval literals to tightest type.

Why are the changes needed?

To comply with the ANSI behavior.
For example, INTERVAL '10 20:30' DAY TO MINUTE should be parsed as DayTimeIntervalType(DAY, MINUTE) but not as DayTimeIntervalType(DAY, SECOND).

Does this PR introduce any user-facing change?

No because DayTimeIntervalType will be introduced in 3.2.0.

How was this patch tested?

New tests.

SparkQA · 2021-06-12T18:42:33Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44267/

SparkQA · 2021-06-12T19:03:08Z

Test build #139742 has finished for PR 32892 at commit c6906d8.

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

SparkQA · 2021-06-12T19:16:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44267/

HyukjinKwon · 2021-06-13T00:40:04Z

cc @MaxGekk @yaooqinn FYI

…aytime-interval

SparkQA · 2021-06-13T15:35:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44279/

SparkQA · 2021-06-13T16:06:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44279/

SparkQA · 2021-06-13T16:35:28Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44280/

SparkQA · 2021-06-13T17:09:52Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44280/

SparkQA · 2021-06-13T17:59:14Z

Test build #139753 has finished for PR 32892 at commit 32cce06.

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

SparkQA · 2021-06-13T19:43:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44282/

MaxGekk · 2021-06-13T19:51:28Z

@sarutak We should support single field as well there:

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

Lines 2452 to 2462 in d53831f

    
             IntervalUtils.fromDayTimeString(value, IntervalUnit.DAY, IntervalUnit.HOUR) 
        
           case ("day", "minute") => 
        
             IntervalUtils.fromDayTimeString(value, IntervalUnit.DAY, IntervalUnit.MINUTE) 
        
           case ("day", "second") => 
        
             IntervalUtils.fromDayTimeString(value, IntervalUnit.DAY, IntervalUnit.SECOND) 
        
           case ("hour", "minute") => 
        
             IntervalUtils.fromDayTimeString(value, IntervalUnit.HOUR, IntervalUnit.MINUTE) 
        
           case ("hour", "second") => 
        
             IntervalUtils.fromDayTimeString(value, IntervalUnit.HOUR, IntervalUnit.SECOND) 
        
           case ("minute", "second") => 
        
             IntervalUtils.fromDayTimeString(value, IntervalUnit.MINUTE, IntervalUnit.SECOND)

Please, open a sub-task in JIRA to don't forget about it if it is hard to do in this PR.

SparkQA · 2021-06-13T19:52:08Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44282/

MaxGekk · 2021-06-13T19:54:01Z

        Literal(calendarInterval.months, YearMonthIntervalType)
      } else {
        assert(calendarInterval.months == 0)
+        val strToFieldIndex = DayTimeIntervalType.dayTimeFields.map(i =>


I have seen this already in other PR. Does it make sense to put it to a common place case object DayTimeIntervalType?

Yeah, it seems a common logic. After this and #32893 are merged, I'll open a followup PR.

MaxGekk

LGTM. Waiting for GA.

SparkQA · 2021-06-13T20:01:40Z

Test build #139754 has finished for PR 32892 at commit 6f0c88c.

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

sarutak · 2021-06-13T20:19:57Z

Please, open a sub-task in JIRA to don't forget about it if it is hard to do in this PR.

Yes, I've noticed that we should support single field but single field is currently parsed as CalendarIntervalType so we need change the behavior. So I separated that from this issue.
Sure, I'll open a sub task.

BTW, what do you think of multiple fields like 1 day 2 hours? They should be parsed as DayTimeIntervalType rather than CalendarType?

MaxGekk · 2021-06-14T07:02:36Z

BTW, what do you think of multiple fields like 1 day 2 hours? They should be parsed as DayTimeIntervalType rather than CalendarType?

I think we should take into account the SQL config spark.sql.legacy.interval.enabled. If it is false (default), we should parse 1 day 2 hours as DayTimeIntervalType. There is a problem with mixed intervals like 1 month 2 hours. In that case, we should throw an exception, and ask users either to change the config or interval (1 month 2 hours -> 30 days 2 hours).

MaxGekk · 2021-06-14T07:05:45Z

+1, LGTM. Merging to master.
Thank you, @sarutak .

…imeIntervalType ### What changes were proposed in this pull request? This is a followup PR for SPARK-35736(#32893) and SPARK-35737(#32892). This PR moves a common logic to `object DayTimeIntervalType`. That logic is like `val strToFieldIndex = DayTimeIntervalType.dayTimeFields.map(i => DayTimeIntervalType.fieldToString(i) -> (i).toMap`, a `Map` which maps each time unit to the corresponding day-time field index. ### Why are the changes needed? That logic appeared in the change in SPARK-35736 and SPARK-35737 so it can be a common logic and it's better to avoid the similar logic scattered. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes #32905 from sarutak/followup-SPARK-35736-35737. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

Change the parser to parse day-time interval literals to tightest types.

c6906d8

github-actions Bot added the SQL label Jun 12, 2021

Merge branch 'master' of https://github.com/apache/spark into tight-d…

14e7d5f

…aytime-interval

sarutak changed the title ~~[SPARK-35737][SQL] Parse day-time interval literals to tightest types~~ [WIP][SPARK-35737][SQL] Parse day-time interval literals to tightest types Jun 13, 2021

Change the parser to parse day-time interval literals to tightest types.

32cce06

sarutak force-pushed the tight-daytime-interval branch from b4fdd39 to 32cce06 Compare June 13, 2021 14:35

sarutak changed the title ~~[WIP][SPARK-35737][SQL] Parse day-time interval literals to tightest types~~ [SPARK-35737][SQL] Parse day-time interval literals to tightest types Jun 13, 2021

Modify the golden file.

6f0c88c

Refactoring.

eef88fc

sarutak force-pushed the tight-daytime-interval branch from a41fd2b to eef88fc Compare June 13, 2021 18:53

MaxGekk reviewed Jun 13, 2021

View reviewed changes

MaxGekk approved these changes Jun 13, 2021

View reviewed changes

MaxGekk closed this in 439e94c Jun 14, 2021

sarutak mentioned this pull request Jun 14, 2021

[SPARK-35736][SPARK-35737][SQL][FOLLOWUP] Move a common logic to DayTimeIntervalType #32905

Closed

Conversation

sarutak commented Jun 12, 2021

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!

SparkQA commented Jun 12, 2021

Uh oh!

SparkQA commented Jun 12, 2021

Uh oh!

SparkQA commented Jun 12, 2021

Uh oh!

HyukjinKwon commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

MaxGekk commented Jun 13, 2021

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

MaxGekk Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 13, 2021

Uh oh!

sarutak commented Jun 13, 2021

Uh oh!

MaxGekk commented Jun 14, 2021

Uh oh!

MaxGekk commented Jun 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants