[SPARK-35727][SQL] Return INTERVAL DAY from dates subtraction #32999

Peng-Lei · 2021-06-21T10:00:37Z

What changes were proposed in this pull request?

Change the return value type from DayTimeIntervalType(DAY, SECOND) to DayTimeIntervalType(DAY, DAY) of SubtractDates.

Why are the changes needed?

SPARK-35727

Does this PR introduce any user-facing change?

No

How was this patch tested?

existed ut test

Peng-Lei · 2021-06-21T10:02:53Z

cc @MaxGekk

AmplabJenkins · 2021-06-21T10:24:50Z

Can one of the admins verify this patch?

MaxGekk · 2021-06-21T11:53:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

+    if (legacyInterval) {
+      CalendarIntervalType
+    } else {
+      DayTimeIntervalType(DayTimeIntervalType.DAY, DayTimeIntervalType.DAY)


I have added apply() with single arg recently #32997, you can use it here as:

DayTimeIntervalType(DAY)

MaxGekk · 2021-06-21T11:53:49Z

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

-            checkAnswer(df.select($"end" - $"start"), Row(daysBetween))
+            val r = df.select($"end" - $"start").toDF("diff")
+            checkAnswer(r, Row(daysBetween))
+            assert(r.schema === new StructType().add("diff", DayTimeIntervalType(0, 0)))


Please, replace DayTimeIntervalType(0, 0) by DayTimeIntervalType(DAY)

@Peng-Lei This change seems to include the change not only for Date - Date but also Date + Interval and Interval + Date. If you'd like to include them, could you add tests for them too?

Let's exclude the Date +/- Interval changes from this PR.

could you add tests for them too

@sarutak ,Sorry I am wrong. First I should exclude the Date +/- Interval from this PR. Second Even if I wanted to change Date +/- Interval, I should convert the interval to the number of days ExtractANSIIntervalDays) and then use DateAdd expression as cloud-fan said.

MaxGekk · 2021-06-21T11:54:24Z

Also cc @sarutak @AngersZhuuuu and @cloud-fan

sarutak · 2021-06-21T16:24:34Z

@Peng-Lei
Aside from the problem raised in SPARK-35727, this PR seems to include other fixes which are related to Add/Sub between Date and Interval. Could you file these issues in JIRA?

sarutak · 2021-06-21T16:34:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

@@ -349,8 +349,18 @@ class Analyzer(override val catalogManager: CatalogManager)
      case p: LogicalPlan => p.transformExpressionsUpWithPruning(
        _.containsPattern(BINARY_ARITHMETIC), ruleId) {
        case a @ Add(l, r, f) if a.childrenResolved => (l.dataType, r.dataType) match {
-          case (DateType, _: DayTimeIntervalType) => TimeAdd(Cast(l, TimestampType), r)
-          case (_: DayTimeIntervalType, DateType) => TimeAdd(Cast(r, TimestampType), l)
+          case (DateType, dit: DayTimeIntervalType) =>


The change in Analyzer.scala doesn't seem related to the issue raised in SPARK-35727.
If we define the arithmetic operations between DateType and DayTimeIntervalType, we should define such operators for YearMonthIntervalType right?

I agree with change but I think it's a separate thing and can be done in a new PR.

The impl detail can also be improved: we can convert the interval to the number of days (ExtractANSIIntervalDays) and then use DateAdd expression.

I agree with change but I think it's a separate thing and can be done in a new PR.

The impl detail can also be improved: we can convert the interval to the number of days (ExtractANSIIntervalDays) and then use DateAdd expression.

@cloud-fan Could I propose a ticket for improved for DateType +/- DayTimeIntervalType(DAY) and try to resolve it ?

cloud-fan

LGTM if all checks pass (scalastyle check will fail I think)

Peng-Lei · 2021-06-22T03:31:34Z

@cloud-fan Thank you very much. Your review comments are always inspiring.

MaxGekk · 2021-06-22T05:53:45Z

@Peng-Lei Could you fix markdown in PR's description. Please, keep the original formatting for sections.

MaxGekk · 2021-06-22T05:54:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

+    if (legacyInterval) {
+      CalendarIntervalType
+    } else {
+      DayTimeIntervalType(DAY)
+    }


nit: minimal changes would be:

if (legacyInterval) CalendarIntervalType else DayTimeIntervalType(DAY)

@MaxGekk Thank you very much, I fix it. Thank you.

MaxGekk · 2021-06-22T08:22:26Z

@Peng-Lei The failed tests:

[info] - typeCoercion/native/promoteStrings.sql *** FAILED *** (3 seconds, 735 milliseconds)
[info]   typeCoercion/native/promoteStrings.sql
[info]   Expected "... DATE)):interval day[ to second]>", but got "... DATE)):interval day[]>" Schema did not match for query #24
[info]   SELECT '1' - cast('2017-12-11 09:30:00' as date)        FROM t: -- !query
[info]   SELECT '1' - cast('2017-12-11 09:30:00' as date)        FROM t
[info]   -- !query schema
[info]   struct<(1 - CAST(2017-12-11 09:30:00 AS DATE)):interval day>
[info]   -- !query output

are related to the changes. Could you regenerate the golden files, please.

Peng-Lei · 2021-06-22T08:46:31Z

Could you regenerate the golden files

@MaxGekk ok. I'm working on how to do this,

cloud-fan · 2021-06-22T09:11:47Z

@Peng-Lei you can check the classdoc of SQLQueryTestSuite

cloud-fan · 2021-06-22T13:43:24Z

thanks, merging to master!

MaxGekk · 2021-06-22T16:54:53Z

@Peng-Lei Could you change formatting of PR's description (see #32999 (comment)), please. It has been already late for the commit but in any case.

HyukjinKwon · 2021-06-23T01:12:09Z

Yeah, let's keep the PR description properly.

Peng-Lei · 2021-06-23T09:12:19Z

@Peng-Lei Could you change formatting of PR's description (see #32999 (comment)), please. It has been already late for the commit but in any case.

@MaxGekk Sorry, I missed that comment #32999 (comment), I will pay more attention to PR description and deal with each review comments of PR in the future. Thank you.

cloud-fan · 2021-06-23T09:17:27Z

@Peng-Lei although it's merged, you can still update the PR description, in case other people come to this PR in the future

github-actions bot added the SQL label Jun 21, 2021

MaxGekk reviewed Jun 21, 2021

View reviewed changes

sarutak reviewed Jun 21, 2021

View reviewed changes

Peng-Lei force-pushed the SPARK-35727 branch 3 times, most recently from 734b6e2 to 525eb9e Compare June 22, 2021 03:14

cloud-fan approved these changes Jun 22, 2021

View reviewed changes

Peng-Lei force-pushed the SPARK-35727 branch from 525eb9e to d556dd3 Compare June 22, 2021 05:33

MaxGekk reviewed Jun 22, 2021

View reviewed changes

modify the return vale type of SubtractDates

ee755b7

Peng-Lei force-pushed the SPARK-35727 branch from d556dd3 to ee755b7 Compare June 22, 2021 06:03

Merge branch 'apache:master' into SPARK-35727

a6a4ab9

regenerate the golden files

bd83d9e

Peng-Lei requested a review from cloud-fan June 22, 2021 13:26

cloud-fan approved these changes Jun 22, 2021

View reviewed changes

cloud-fan closed this in bc61b62 Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-35727][SQL] Return INTERVAL DAY from dates subtraction #32999

[SPARK-35727][SQL] Return INTERVAL DAY from dates subtraction #32999

Peng-Lei commented Jun 21, 2021 •

edited

Peng-Lei commented Jun 21, 2021

AmplabJenkins commented Jun 21, 2021

MaxGekk Jun 21, 2021

MaxGekk Jun 21, 2021

sarutak Jun 21, 2021

cloud-fan Jun 21, 2021

Peng-Lei Jun 22, 2021

MaxGekk commented Jun 21, 2021

sarutak commented Jun 21, 2021 •

edited

sarutak Jun 21, 2021

cloud-fan Jun 21, 2021 •

edited

Peng-Lei Jun 22, 2021

cloud-fan Jun 22, 2021

cloud-fan left a comment

Peng-Lei commented Jun 22, 2021

MaxGekk commented Jun 22, 2021

MaxGekk Jun 22, 2021 •

edited

Peng-Lei Jun 22, 2021

MaxGekk commented Jun 22, 2021

Peng-Lei commented Jun 22, 2021

cloud-fan commented Jun 22, 2021

cloud-fan commented Jun 22, 2021

MaxGekk commented Jun 22, 2021

HyukjinKwon commented Jun 23, 2021

Peng-Lei commented Jun 23, 2021

cloud-fan commented Jun 23, 2021

[SPARK-35727][SQL] Return INTERVAL DAY from dates subtraction #32999

[SPARK-35727][SQL] Return INTERVAL DAY from dates subtraction #32999

Conversation

Peng-Lei commented Jun 21, 2021 • edited

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?

Peng-Lei commented Jun 21, 2021

AmplabJenkins commented Jun 21, 2021

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

MaxGekk commented Jun 21, 2021

sarutak commented Jun 21, 2021 • edited

Choose a reason for hiding this comment

cloud-fan Jun 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan left a comment

Choose a reason for hiding this comment

Peng-Lei commented Jun 22, 2021

MaxGekk commented Jun 22, 2021

MaxGekk Jun 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Jun 22, 2021

Peng-Lei commented Jun 22, 2021

cloud-fan commented Jun 22, 2021

cloud-fan commented Jun 22, 2021

MaxGekk commented Jun 22, 2021

HyukjinKwon commented Jun 23, 2021

Peng-Lei commented Jun 23, 2021

cloud-fan commented Jun 23, 2021

Peng-Lei commented Jun 21, 2021 •

edited

sarutak commented Jun 21, 2021 •

edited

cloud-fan Jun 21, 2021 •

edited

MaxGekk Jun 22, 2021 •

edited