[SPARK-26002][SQL] Fix day of year calculation for Julian calendar days #23000

attilapiros · 2018-11-10T15:25:38Z

What changes were proposed in this pull request?

Fixing leap year calculations for date operators (year/month/dayOfYear) where the Julian calendars are used (before 1582-10-04). In a Julian calendar every years which are multiples of 4 are leap years (there is no extra exception for years multiples of 100).

How was this patch tested?

With a unit test ("SPARK-26002: correct day of year calculations for Julian calendar years") which focuses to these corner cases.

Manually:

scala> sql("select year('1500-01-01')").show()

+------------------------------+
|year(CAST(1500-01-01 AS DATE))|
+------------------------------+
|                          1500|
+------------------------------+

scala> sql("select dayOfYear('1100-01-01')").show()

+-----------------------------------+
|dayofyear(CAST(1100-01-01 AS DATE))|
+-----------------------------------+
|                                  1|
+-----------------------------------+

SparkQA · 2018-11-10T19:01:10Z

Test build #98682 has finished for PR 23000 at commit f2bc0b2.

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

attilapiros · 2018-11-15T12:43:25Z

ping @10110346 @gatorsmile

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

squito · 2018-11-20T15:13:50Z

lgtm

SparkQA · 2018-11-20T16:19:35Z

Test build #99054 has finished for PR 23000 at commit 421673a.

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

dongjoon-hyun · 2018-12-17T03:51:26Z

Retest this please.

SparkQA · 2018-12-17T07:32:01Z

Test build #100216 has finished for PR 23000 at commit 421673a.

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

dongjoon-hyun · 2018-12-17T18:35:00Z

cc @cloud-fan , @gatorsmile , @mgaido91

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

cloud-fan · 2018-12-18T03:57:29Z

what was the behavior before this PR?

attilapiros · 2018-12-18T05:13:22Z

@cloud-fan It was off by one (at least at 1500, with each 100 years back in time the difference increases) as I described in the Jira:

+-----------------------------------+
|dayofyear(CAST(1500-01-02 AS DATE))|
+-----------------------------------+
|                                  1|
+-----------------------------------+

scala> sql("select year('1500-01-01')").show()
+------------------------------+
|year(CAST(1500-01-01 AS DATE))|
+------------------------------+
|                          1499|
+------------------------------+


scala> sql("select month('1500-01-01')").show()
+-------------------------------+
|month(CAST(1500-01-01 AS DATE))|
+-------------------------------+
|                             12|
+-------------------------------+


scala> sql("select dayOfYear('1500-01-01')").show()
+-----------------------------------+
|dayofyear(CAST(1500-01-01 AS DATE))|
+-----------------------------------+
|                                365|
+-----------------------------------+

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

gatorsmile · 2018-12-29T17:03:11Z

cc @MaxGekk who is touching this

MaxGekk · 2018-12-29T17:51:28Z

@gatorsmile Thanx for the ping. Actually current implementation support Proleptic Gregorian calendar even if it is not documented explicitly. Recently we switched on this chronology for parsing/formatting date/timestamps in JSON/CSV datasource (see #23150 and #23196) and in functions (see #23358).

If we support the hybrid calendar (Julian + Gregorian) here, it will introduce additional inconsistency.

@attilapiros If you need the hybrid calendar for some reasons, you should extend the DayOfYear expression by additional argument to set chronology (calendar system) otherwise you can break user's apps potentially.

MaxGekk

Just in case, SQL standard counts dates exclusively in the Gregorian calendar, even for years before that calendar was in use.

attilapiros · 2019-01-02T10:34:19Z

@MaxGekk thanks for the explanation, but I still a bit uncertain.

My main intention was about correcting the dayOfYear/month/year operators as the result in the following example are very strange (and not consistent with PostgreSQL too):

scala> sql("select year('1500-01-01')").show()
+------------------------------+
|year(CAST(1500-01-01 AS DATE))|
+------------------------------+
|                          1499|
+------------------------------+

scala> sql("select month('1500-01-01')").show()
+-------------------------------+
|month(CAST(1500-01-01 AS DATE))|
+-------------------------------+
|                             12|
+-------------------------------+


scala> sql("select dayOfYear('1500-01-01')").show()
+-----------------------------------+
|dayofyear(CAST(1500-01-01 AS DATE))|
+-----------------------------------+
|                                365|
+-----------------------------------+

Running the corresponding expressions in PostgreSQL 9.6:

>    SELECT DATE_PART('year', TIMESTAMP '1500-01-01 0:0:0') as "year";

| year |
| ---- |
| 1500 |

>   SELECT DATE_PART('month', TIMESTAMP '1500-01-01 0:0:0') as "month";

| month |
| ----- |
| 1     |

>  SELECT DATE_PART('doy', TIMESTAMP '1500-01-01 0:0:0') as "doy";

| doy |
| --- |
| 1   |

View on DB Fiddle

Is there any other DBMS I can take as a reference?

MaxGekk · 2019-01-02T22:16:14Z

correcting the dayOfYear/month/year operators as the result in the following example are very strange ...

I see, we still use hybrid calendar in casting UTF8String to timestamps. An alternative solution could be rewriting this code:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

Lines 414 to 430 in 7c7fccf

    
           val c = if (tz.isEmpty) { 
        
             Calendar.getInstance(timeZone) 
        
           } else { 
        
             Calendar.getInstance( 
        
               getTimeZone(f"GMT${tz.get.toChar}${segments(7)}%02d:${segments(8)}%02d")) 
        
           } 
        
           c.set(Calendar.MILLISECOND, 0) 
        
           if (justTime) { 
        
             c.set(Calendar.HOUR_OF_DAY, segments(3)) 
        
             c.set(Calendar.MINUTE, segments(4)) 
        
             c.set(Calendar.SECOND, segments(5)) 
        
           } else { 
        
             c.set(segments(0), segments(1) - 1, segments(2), segments(3), segments(4), segments(5)) 
        
           } 
        
           Some(c.getTimeInMillis * 1000 + segments(6))

by using java.time API based on Proleptic Gregorian Calendar but such changes look like more heavy. I would guess they will be done to follow SQL standard in the end, so, your fix for Julian calendar will be not needed. At the moment, I agree we need your fix to handle results from stringToTimestamp properly.

attilapiros · 2019-01-02T22:28:31Z

Ok. Then tomorrow I will add the requested tests and we can progress with this PR.

SparkQA · 2019-01-03T17:46:07Z

Test build #100692 has finished for PR 23000 at commit 300ec39.

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

attilapiros · 2019-01-08T14:27:15Z

gentle ping @dongjoon-hyun, @cloud-fan, @gatorsmile

cloud-fan · 2019-01-08T17:25:00Z

thanks, merging to master!

## What changes were proposed in this pull request? Fixing leap year calculations for date operators (year/month/dayOfYear) where the Julian calendars are used (before 1582-10-04). In a Julian calendar every years which are multiples of 4 are leap years (there is no extra exception for years multiples of 100). ## How was this patch tested? With a unit test ("SPARK-26002: correct day of year calculations for Julian calendar years") which focuses to these corner cases. Manually: ``` scala> sql("select year('1500-01-01')").show() +------------------------------+ |year(CAST(1500-01-01 AS DATE))| +------------------------------+ | 1500| +------------------------------+ scala> sql("select dayOfYear('1100-01-01')").show() +-----------------------------------+ |dayofyear(CAST(1100-01-01 AS DATE))| +-----------------------------------+ | 1| +-----------------------------------+ ``` Closes apache#23000 from attilapiros/julianOffByDays. Authored-by: “attilapiros” <piros.attila.zsolt@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Fix day of year calculation for Julian calendar days

f2bc0b2

squito reviewed Nov 19, 2018

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Show resolved Hide resolved

bersprockets reviewed Nov 19, 2018

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated Show resolved Hide resolved

review comments 1.0

421673a

cloud-fan reviewed Dec 18, 2018

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Show resolved Hide resolved

viirya reviewed Dec 18, 2018

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated Show resolved Hide resolved

mgaido91 reviewed Dec 18, 2018

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Show resolved Hide resolved

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Show resolved Hide resolved

MaxGekk reviewed Dec 29, 2018

View reviewed changes

Add end2end and leap day tests, fix comment

300ec39

cloud-fan closed this in c101182 Jan 8, 2019

attilapiros deleted the julianOffByDays branch May 8, 2019 00:28

[SPARK-26002][SQL] Fix day of year calculation for Julian calendar days #23000

[SPARK-26002][SQL] Fix day of year calculation for Julian calendar days #23000

Uh oh!

Conversation

attilapiros commented Nov 10, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Nov 10, 2018

Uh oh!

attilapiros commented Nov 15, 2018

Uh oh!

Uh oh!

Uh oh!

squito commented Nov 20, 2018

Uh oh!

SparkQA commented Nov 20, 2018

Uh oh!

dongjoon-hyun commented Dec 17, 2018

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

dongjoon-hyun commented Dec 17, 2018

Uh oh!

Uh oh!

cloud-fan commented Dec 18, 2018

Uh oh!

attilapiros commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gatorsmile commented Dec 29, 2018

Uh oh!

MaxGekk commented Dec 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

attilapiros commented Jan 2, 2019

Uh oh!

MaxGekk commented Jan 2, 2019

Uh oh!

attilapiros commented Jan 2, 2019

Uh oh!

SparkQA commented Jan 3, 2019

Uh oh!

attilapiros commented Jan 8, 2019

Uh oh!

cloud-fan commented Jan 8, 2019

Uh oh!

Uh oh!

attilapiros commented Dec 18, 2018 •

edited

Loading

MaxGekk commented Dec 29, 2018 •

edited

Loading