[SPARK-30865][SQL][SS] Refactor DateTimeUtils #27617

MaxGekk · 2020-02-18T06:38:00Z

What changes were proposed in this pull request?

Move TimeZoneUTC and TimeZoneGMT to DateTimeTestUtils
Remove TimeZoneGMT
Use ZoneId.systemDefault() instead of defaultTimeZone().toZoneId
Alias SQLDate & SQLTimestamp to internal types of DateType and TimestampType
Avoid one * DateTimeUtils.in fromJulianDay()
Use toTotalMonths in DateTimeUtils.subtractDates()
Remove julianCommonEraStart, timestampToString(), microsToEpochDays(), epochDaysToMicros(), instantToDays() from DateTimeUtils.
Make splitDate() private.
Remove def daysToMicros(days: Int): Long and def microsToDays(micros: Long): Int.

Why are the changes needed?

This simplifies the common code related to date-time operations, and should improve maintainability. In particular:

TimeZoneUTC and TimeZoneGMT are moved to DateTimeTestUtils because they are used only in tests
TimeZoneGMT can be removed because it is equal to TimeZoneUTC
After the PR [SPARK-30760][SQL] Port millisToDays and daysToMillis on Java 8 time API #27494, Spark expressions and DateTimeUtils functions switched to ZoneId instead of TimeZone completely. defaultTimeZone() with TimeZone as return type is not needed anymore.
SQLDate and SQLTimestamp types can be explicitly aliased to internal types of DateType and and TimestampType instead of declaring this in a comment.
Avoid one * DateTimeUtils.in fromJulianDay().
Use toTotalMonths in DateTimeUtils.subtractDates().

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites

SparkQA · 2020-02-18T08:05:02Z

Test build #118615 has finished for PR 27617 at commit 0b5711e.

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

MaxGekk · 2020-02-18T08:15:25Z

jenkins, retest this, please

SparkQA · 2020-02-18T12:49:23Z

Test build #118623 has finished for PR 27617 at commit 0b5711e.

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

MaxGekk · 2020-02-19T19:13:38Z

@cloud-fan @HyukjinKwon Please, take a look at this PR.

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

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

SparkQA · 2020-02-21T08:05:02Z

Test build #118757 has finished for PR 27617 at commit 9ba2d3d.

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

SparkQA · 2020-02-21T08:05:02Z

Test build #118759 has finished for PR 27617 at commit c475f27.

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

MaxGekk · 2020-02-21T08:25:30Z

jenkins, retest this, please

SparkQA · 2020-02-21T13:36:16Z

Test build #118768 has finished for PR 27617 at commit c475f27.

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

SparkQA · 2020-02-22T00:41:52Z

Test build #118804 has finished for PR 27617 at commit cbf69fb.

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

maropu · 2020-02-23T00:43:24Z

cc: @viirya @cloud-fan

viirya · 2020-02-23T00:47:02Z

Move TimeZoneUTC and TimeZoneGMT to DateTimeTestUtils
Remove TimeZoneGMT

A bit confused. TimeZoneGMT was removed why it also moved to DateTimeTestUtils?

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

viirya

looks good except few minor comments.

SparkQA · 2020-06-16T23:56:39Z

Test build #124138 has finished for PR 27617 at commit b18859a.

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

MaxGekk · 2020-06-18T06:08:21Z

@cloud-fan @HyukjinKwon It is ready already. Can we continue with this PR?

cloud-fan · 2020-06-18T11:20:18Z

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

-    tf.format(us)
+  // Converts the `micros` timestamp to string according to Hive TimestampWritable convention.
+  def timestampToString(tf: TimestampFormatter, micros: Long): String = {
+    tf.format(micros)


tf.format(micros) is shorter than timestampToString, do we really need this method?

I removed it

cloud-fan · 2020-06-18T11:22:46Z

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


-  def microsToEpochDays(epochMicros: SQLTimestamp, zoneId: ZoneId): SQLDate = {
-    localDateToDays(microsToInstant(epochMicros).atZone(zoneId).toLocalDate)
+  def microsToEpochDays(micros: Long, zoneId: ZoneId): Int = {


is it the same with microsToDays?

Looks like, yes

can we unify them?

I removed the duplicates

cloud-fan · 2020-06-18T11:23:26Z

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


-  def epochDaysToMicros(epochDays: SQLDate, zoneId: ZoneId): SQLTimestamp = {
-    val localDate = LocalDate.ofEpochDay(epochDays)
+  def epochDaysToMicros(days: Int, zoneId: ZoneId): Long = {


cloud-fan · 2020-06-18T11:27:19Z

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

-  def microsToEpochDays(epochMicros: SQLTimestamp, zoneId: ZoneId): SQLDate = {
-    localDateToDays(microsToInstant(epochMicros).atZone(zoneId).toLocalDate)
+  def microsToEpochDays(micros: Long, zoneId: ZoneId): Int = {
+    localDateToDays(microsToInstant(micros).atZone(zoneId).toLocalDate)


nit: localDateToDays(getLocalDateTime(micros, zoneId))

I removed the function

cloud-fan · 2020-06-18T11:29:53Z

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

-  def convertTz(ts: SQLTimestamp, fromZone: ZoneId, toZone: ZoneId): SQLTimestamp = {
-    val rebasedDateTime = microsToInstant(ts).atZone(toZone).toLocalDateTime.atZone(fromZone)
+  def convertTz(micros: Long, fromZone: ZoneId, toZone: ZoneId): Long = {
+    val rebasedDateTime = microsToInstant(micros).atZone(toZone).toLocalDateTime.atZone(fromZone)


nit: we can reuse getLocalDateTime here.

SparkQA · 2020-06-18T19:20:16Z

Test build #124234 has finished for PR 27617 at commit 025ea80.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-19T01:40:44Z

Test build #124235 has finished for PR 27617 at commit 863d747.

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

cloud-fan · 2020-06-19T05:41:06Z

thanks, merging to master!

MaxGekk added 7 commits February 17, 2020 23:49

Move TimeZoneGMT/UTC to DateTimeTestUtils

ba506a2

Remove TimeZoneGMT

6933070

gmtId -> utcId

e195d25

Remove defaultTimeZone

cf620cb

Use Date/TimestampType.InternalType

b487933

Use TimeZoneUTC

b26730c

Use TimeZoneUTC instead of GMT

0b5711e

cloud-fan reviewed Feb 20, 2020

View reviewed changes

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

cloud-fan reviewed Feb 20, 2020

View reviewed changes

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

MaxGekk added 2 commits February 21, 2020 09:00

Remove SQLDate and SQLTimestamp

9ba2d3d

Improve a comment

c475f27

cloud-fan reviewed Feb 21, 2020

View reviewed changes

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

cloud-fan reviewed Feb 21, 2020

View reviewed changes

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

cloud-fan reviewed Feb 21, 2020

View reviewed changes

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

cloud-fan reviewed Feb 21, 2020

View reviewed changes

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

maropu approved these changes Feb 21, 2020

View reviewed changes

Renaming

cbf69fb

viirya reviewed Feb 23, 2020

View reviewed changes

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

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

viirya approved these changes Feb 23, 2020

View reviewed changes

Resolve merge issue

b18859a

cloud-fan reviewed Jun 18, 2020

View reviewed changes

MaxGekk added 11 commits June 18, 2020 20:24

Remove timestampToString()

f477a71

Remove microsToEpochDays() and epochDaysToMicros()

ec386b7

Re-use getLocalDateTime in convertTz

cf85866

Re-use getLocalDateTime

89d7fad

Remove julianCommonEraStart

d9699c4

Remove instantToDays

d261ba7

Make splitDate private

53a33da

Remove def daysToMicros(days: Int): Long

ee91211

Remove def microsToDays(micros: Long): Int

386f6cf

1.1.1970 -> 1970-01-01

c47d522

1970-01-01 00:00:00 -> 1970-01-01 00:00:00Z

025ea80

probot-autolabeler bot added the STRUCTURED STREAMING label Jun 18, 2020

MaxGekk added 2 commits June 18, 2020 23:16

Make Scala style checker happy

8a7ac3f

Update comments

863d747

MaxGekk changed the title ~~[SPARK-30865][SQL] Refactor DateTimeUtils~~ [SPARK-30865][SQL][SS] Refactor DateTimeUtils Jun 18, 2020

cloud-fan approved these changes Jun 19, 2020

View reviewed changes

cloud-fan closed this in 17a5007 Jun 19, 2020

MaxGekk mentioned this pull request Jun 19, 2020

[SPARK-31982][SQL]Function sequence doesn't handle date increments that cross DST #28856

Closed

MaxGekk deleted the move-time-zone-consts branch December 11, 2020 20:26

[SPARK-30865][SQL][SS] Refactor DateTimeUtils #27617

[SPARK-30865][SQL][SS] Refactor DateTimeUtils #27617

Uh oh!

Conversation

MaxGekk commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Feb 18, 2020

Uh oh!

MaxGekk commented Feb 18, 2020

Uh oh!

SparkQA commented Feb 18, 2020

Uh oh!

MaxGekk commented Feb 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

MaxGekk commented Feb 21, 2020

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

SparkQA commented Feb 22, 2020

Uh oh!

maropu commented Feb 23, 2020

Uh oh!

viirya commented Feb 23, 2020

Uh oh!

Uh oh!

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

MaxGekk commented Jun 18, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

SparkQA commented Jun 18, 2020

Uh oh!

SparkQA commented Jun 19, 2020

Uh oh!

cloud-fan commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

MaxGekk commented Feb 18, 2020 •

edited

Loading

cloud-fan Jun 18, 2020 •

edited

Loading