[SPARK-26593][SQL] Use Proleptic Gregorian calendar in casting UTF8String to Date/TimestampType #23512

MaxGekk · 2019-01-10T20:05:05Z

What changes were proposed in this pull request?

In the PR, I propose to use java.time classes in stringToDate and stringToTimestamp. This switches the methods from the hybrid calendar (Gregorian+Julian) to Proleptic Gregorian calendar. And it should make the casting consistent to other Spark classes that converts textual representation of dates/timestamps to DateType/TimestampType.

How was this patch tested?

The changes were tested by existing suites - HashExpressionsSuite, CastSuite and DateTimeUtilsSuite.

SparkQA · 2019-01-10T23:04:05Z

Test build #101034 has finished for PR 23512 at commit 2c811af.

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

SparkQA · 2019-01-11T10:13:50Z

Test build #101073 has finished for PR 23512 at commit b9a52a8.

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

…p-parsing # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateFormatter.scala # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala

SparkQA · 2019-01-11T23:14:08Z

Test build #101097 has finished for PR 23512 at commit c2bc4b0.

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

MaxGekk · 2019-01-11T23:22:20Z

jenkins, retest this, please

SparkQA · 2019-01-12T01:21:35Z

Test build #101108 has finished for PR 23512 at commit c2bc4b0.

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

MaxGekk · 2019-01-12T09:33:26Z

jenkins, retest this, please

SparkQA · 2019-01-12T12:47:10Z

Test build #101122 has finished for PR 23512 at commit c2bc4b0.

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

SparkQA · 2019-01-12T20:46:09Z

Test build #101130 has finished for PR 23512 at commit 0f7ddd2.

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

MaxGekk · 2019-01-12T20:53:16Z

@srowen @cloud-fan Please, review the PR.

srowen

I think you know the change much better than I, and so I defer to your judgment. The actual change looks reasonable and consistent with the others in this effort. (One tiny suggestion, not important.) Are there any further compatibility issues introduced by this change that need a note in the release notes? We can change behavior if needed, and certainly if it's fixing problematic behavior.

srowen · 2019-01-12T21:43:14Z

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


 import java.sql.{Date, Timestamp}
 import java.text.{DateFormat, SimpleDateFormat}
+import java.time._


If there aren't like 10 classes to import, I'd make them explicit. At least, the following line is currently redundant.

yah, IntelliJ IDEA collapses the imports automatically. I will try to revert them back.

The DateTimeUtils uses many classes from java.time. I will keep import java.time._ if you don't mind.

MaxGekk · 2019-01-12T21:56:29Z

Are there any further compatibility issues introduced by this change that need a note in the release notes?

Besides of switching from the hybrid calendar (Julian+Gregorian) to Proleptic Gregorian calendar used by java.time classes, the PR shouldn't introduce any behaviour change. At least our tests did show that.

I will add a note to the migration guide.

MaxGekk · 2019-01-12T22:00:21Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala


-    checkEvaluation(DayOfYear(Literal(new Date(sdf.parse("1582-10-15 13:10:15").getTime))), 288)
-    checkEvaluation(DayOfYear(Literal(new Date(sdf.parse("1582-10-04 13:10:15").getTime))), 277)
+    checkEvaluation(DayOfYear(Cast(Literal("1582-10-15 13:10:15"), DateType)), 288)


This changed to avoid using of SimpleDateFormat

can we remove sdf from this test suite then?

I removed it from the suite

MaxGekk · 2019-01-12T22:01:13Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HashExpressionsSuite.scala


    // boundary cases
-    checkHiveHashForDateType("0000-01-01", -719530)
+    checkHiveHashForDateType("0000-01-01", -719528)


Hash was changed because number of days since epoch is different for the old date.

MaxGekk · 2019-01-12T22:05:57Z

...re/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetQuerySuite.scala

-        Seq(
-          Row(1, Timestamp.valueOf("2016-01-01 10:11:12.123456")),
-          Row(2, null),
-          Row(3, Timestamp.valueOf("1965-01-01 10:11:12.123456"))))


Using Timestamp to hold number of seconds from epoch is ok but any other methods that involve the hybrid calendar can cause calendar related issue. I changed it to cast since it is ported on new Java 8 API (java.time) in the PR.

MaxGekk · 2019-01-12T22:36:35Z

jenkins, retest this, please

SparkQA · 2019-01-13T02:39:49Z

Test build #101132 has finished for PR 23512 at commit 0f7ddd2.

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

SparkQA · 2019-01-13T11:44:13Z

Test build #101140 has finished for PR 23512 at commit 61ad31a.

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

SparkQA · 2019-01-13T16:25:24Z

Test build #101142 has finished for PR 23512 at commit 0a89446.

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

…p-parsing # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

SparkQA · 2019-01-15T02:38:08Z

Test build #101208 has finished for PR 23512 at commit f02c85c.

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

cloud-fan · 2019-01-15T05:54:44Z

LGTM with only one comment: #23512 (comment)

SparkQA · 2019-01-16T19:18:23Z

Test build #101326 has finished for PR 23512 at commit 1af865f.

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

MaxGekk · 2019-01-16T19:26:09Z

jenkins, retest this, please

SparkQA · 2019-01-16T23:05:32Z

Test build #101328 has finished for PR 23512 at commit 1af865f.

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

MaxGekk · 2019-01-16T23:08:43Z

jenkins, retest this, please

MaxGekk · 2019-01-16T23:09:49Z

It seems test failures are not related to this changes.

SparkQA · 2019-01-17T03:17:35Z

Test build #101333 has finished for PR 23512 at commit 1af865f.

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

hvanhovell · 2019-01-17T16:52:01Z

Merging to master. Thanks.

…ower/upper bounds ## What changes were proposed in this pull request? In the PR, I propose using of the `stringToDate` and `stringToTimestamp` methods in parsing JDBC lower/upper bounds of the partition column if it has `DateType` or `TimestampType`. Since those methods have been ported on Proleptic Gregorian calendar by #23512, the PR switches parsing of JDBC bounds of the partition column on the calendar as well. ## How was this patch tested? This was tested by `JDBCSuite`. Closes #23597 from MaxGekk/jdbc-parse-timestamp-bounds. Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com> Co-authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ring to Date/TimestampType ## What changes were proposed in this pull request? In the PR, I propose to use *java.time* classes in `stringToDate` and `stringToTimestamp`. This switches the methods from the hybrid calendar (Gregorian+Julian) to Proleptic Gregorian calendar. And it should make the casting consistent to other Spark classes that converts textual representation of dates/timestamps to `DateType`/`TimestampType`. ## How was this patch tested? The changes were tested by existing suites - `HashExpressionsSuite`, `CastSuite` and `DateTimeUtilsSuite`. Closes apache#23512 from MaxGekk/utf8string-timestamp-parsing. Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com> Co-authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>

…ower/upper bounds ## What changes were proposed in this pull request? In the PR, I propose using of the `stringToDate` and `stringToTimestamp` methods in parsing JDBC lower/upper bounds of the partition column if it has `DateType` or `TimestampType`. Since those methods have been ported on Proleptic Gregorian calendar by apache#23512, the PR switches parsing of JDBC bounds of the partition column on the calendar as well. ## How was this patch tested? This was tested by `JDBCSuite`. Closes apache#23597 from MaxGekk/jdbc-parse-timestamp-bounds. Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com> Co-authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

MaxGekk and others added 6 commits January 10, 2019 00:31

Initial implementation

c5592d8

Fix time only parsing

c22b15c

Fix Hive hash for old date

89be967

Fix coding style

6d9dc96

Using Proleptic Gregorian calendar in stringToDate

0bc1470

Reusing methods from DateTimeUtils

2c811af

Using cast instead of casting Timestamp

b9a52a8

Merge remote-tracking branch 'origin/master' into utf8string-timestam…

c2bc4b0

…p-parsing # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateFormatter.scala # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala

MaxGekk added 3 commits January 12, 2019 16:52

Porting getYear on Proleptic Gregorian cal

00ed08c

Re-implementing splitDate

f794378

Avoid using SimpleDateFormat for old dates

0f7ddd2

srowen approved these changes Jan 12, 2019

View reviewed changes

MaxGekk commented Jan 12, 2019

View reviewed changes

MaxGekk added 3 commits January 13, 2019 09:58

Removing unused stringToTime

70aa674

Fix imports

8ed3f45

Improve getLastDayOfMonth

2ebc272

Using Year.isLeap

61ad31a

Fix getLastDayOfMonth

0a89446

MaxGekk added 3 commits January 14, 2019 23:17

Merge remote-tracking branch 'origin/master' into utf8string-timestam…

336b9ac

…p-parsing # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

Fix merge

f048a3d

Minor refactoring

f02c85c

MaxGekk added 2 commits January 16, 2019 18:02

Avoid SimpleDateFormat in the suite

831cdd1

Remove SimpleDateFormat from the suite

1af865f

asfgit closed this in 6f8c0e5 Jan 17, 2019

MaxGekk mentioned this pull request Jan 20, 2019

[SPARK-26653][SQL] Use Proleptic Gregorian calendar in parsing JDBC lower/upper bounds #23597

Closed

MaxGekk deleted the utf8string-timestamp-parsing branch August 17, 2019 13:36

[SPARK-26593][SQL] Use Proleptic Gregorian calendar in casting UTF8String to Date/TimestampType #23512

[SPARK-26593][SQL] Use Proleptic Gregorian calendar in casting UTF8String to Date/TimestampType #23512

Uh oh!

Conversation

MaxGekk commented Jan 10, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jan 10, 2019

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

MaxGekk commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 12, 2019

Uh oh!

MaxGekk commented Jan 12, 2019

Uh oh!

SparkQA commented Jan 12, 2019

Uh oh!

SparkQA commented Jan 12, 2019

Uh oh!

MaxGekk commented Jan 12, 2019

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jan 12, 2019

Uh oh!

SparkQA commented Jan 13, 2019

Uh oh!

SparkQA commented Jan 13, 2019

Uh oh!

SparkQA commented Jan 13, 2019

Uh oh!

SparkQA commented Jan 15, 2019

Uh oh!

cloud-fan commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jan 16, 2019

Uh oh!

MaxGekk commented Jan 16, 2019

Uh oh!

SparkQA commented Jan 16, 2019

Uh oh!

MaxGekk commented Jan 16, 2019

Uh oh!

MaxGekk commented Jan 16, 2019

Uh oh!

SparkQA commented Jan 17, 2019

Uh oh!

hvanhovell commented Jan 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

MaxGekk commented Jan 12, 2019 •

edited

Loading

cloud-fan commented Jan 15, 2019 •

edited

Loading