Skip to content

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

Closed
MaxGekk wants to merge 60 commits into
apache:masterfrom
MaxGekk:move-time-zone-consts
Closed

[SPARK-30865][SQL][SS] Refactor DateTimeUtils#27617
MaxGekk wants to merge 60 commits into
apache:masterfrom
MaxGekk:move-time-zone-consts

Conversation

@MaxGekk
Copy link
Copy Markdown
Member

@MaxGekk MaxGekk commented Feb 18, 2020 •
edited
Loading

What changes were proposed in this pull request?

  1. Move TimeZoneUTC and TimeZoneGMT to DateTimeTestUtils
  2. Remove TimeZoneGMT
  3. Use ZoneId.systemDefault() instead of defaultTimeZone().toZoneId
  4. Alias SQLDate & SQLTimestamp to internal types of DateType and TimestampType
  5. Avoid one * DateTimeUtils.in fromJulianDay()
  6. Use toTotalMonths in DateTimeUtils.subtractDates()
  7. Remove julianCommonEraStart, timestampToString(), microsToEpochDays(), epochDaysToMicros(), instantToDays() from DateTimeUtils.
  8. Make splitDate() private.
  9. 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:

  1. TimeZoneUTC and TimeZoneGMT are moved to DateTimeTestUtils because they are used only in tests
  2. TimeZoneGMT can be removed because it is equal to TimeZoneUTC
  3. 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.
  4. SQLDate and SQLTimestamp types can be explicitly aliased to internal types of DateType and and TimestampType instead of declaring this in a comment.
  5. Avoid one * DateTimeUtils.in fromJulianDay().
  6. Use toTotalMonths in DateTimeUtils.subtractDates().

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 18, 2020

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
Copy link
Copy Markdown
Member Author

MaxGekk commented Feb 18, 2020

jenkins, retest this, please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 18, 2020

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
Copy link
Copy Markdown
Member Author

MaxGekk commented Feb 19, 2020

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

cloud-fan
cloud-fan reviewed Feb 20, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
cloud-fan
cloud-fan reviewed Feb 20, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
cloud-fan
cloud-fan reviewed Feb 21, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
cloud-fan
cloud-fan reviewed Feb 21, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
cloud-fan
cloud-fan reviewed Feb 21, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
cloud-fan
cloud-fan reviewed Feb 21, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala Outdated
@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 21, 2020

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
Copy link
Copy Markdown

SparkQA commented Feb 21, 2020

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
Copy link
Copy Markdown
Member Author

MaxGekk commented Feb 21, 2020

jenkins, retest this, please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Feb 21, 2020

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
Copy link
Copy Markdown

SparkQA commented Feb 22, 2020

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
Copy link
Copy Markdown
Member

maropu commented Feb 23, 2020

cc: @viirya @cloud-fan

@viirya
Copy link
Copy Markdown
Member

viirya commented Feb 23, 2020

Move TimeZoneUTC and TimeZoneGMT to DateTimeTestUtils
Remove TimeZoneGMT

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

viirya
viirya reviewed Feb 23, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
viirya
viirya approved these changes Feb 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good except few minor comments.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 16, 2020

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
Copy link
Copy Markdown
Member Author

MaxGekk commented Jun 18, 2020

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

cloud-fan
cloud-fan reviewed Jun 18, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
tf.format(us)
// Converts the `micros` timestamp to string according to Hive TimestampWritable convention.
def timestampToString(tf: TimestampFormatter, micros: Long): String = {
tf.format(micros)
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@MaxGekk MaxGekk Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it

cloud-fan
cloud-fan reviewed Jun 18, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated

def microsToEpochDays(epochMicros: SQLTimestamp, zoneId: ZoneId): SQLDate = {
localDateToDays(microsToInstant(epochMicros).atZone(zoneId).toLocalDate)
def microsToEpochDays(micros: Long, zoneId: ZoneId): Int = {
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the same with microsToDays?

Copy link
Copy Markdown
Member Author

@MaxGekk MaxGekk Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like, yes

Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we unify them?

Copy link
Copy Markdown
Member Author

@MaxGekk MaxGekk Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the duplicates

cloud-fan
cloud-fan reviewed Jun 18, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated

def epochDaysToMicros(epochDays: SQLDate, zoneId: ZoneId): SQLTimestamp = {
val localDate = LocalDate.ofEpochDay(epochDays)
def epochDaysToMicros(days: Int, zoneId: ZoneId): Long = {
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

cloud-fan
cloud-fan reviewed Jun 18, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
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)
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: localDateToDays(getLocalDateTime(micros, zoneId))

Copy link
Copy Markdown
Member Author

@MaxGekk MaxGekk Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the function

cloud-fan
cloud-fan reviewed Jun 18, 2020
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Outdated
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)
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can reuse getLocalDateTime here.

Copy link
Copy Markdown
Member Author

@MaxGekk MaxGekk Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 18, 2020

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.

@MaxGekk MaxGekk changed the title [SPARK-30865][SQL] Refactor DateTimeUtils [SPARK-30865][SQL][SS] Refactor DateTimeUtils Jun 18, 2020
@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 19, 2020

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
Copy link
Copy Markdown
Contributor

cloud-fan commented Jun 19, 2020

thanks, merging to master!

@cloud-fan cloud-fan closed this in 17a5007 Jun 19, 2020
@MaxGekk MaxGekk mentioned this pull request Jun 19, 2020
Closed
@MaxGekk MaxGekk deleted the move-time-zone-consts branch December 11, 2020 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kiszk kiszk kiszk left review comments

@viirya viirya viirya approved these changes

@maropu maropu maropu approved these changes

@cloud-fan cloud-fan cloud-fan approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees

No one assigned

Labels

SQL STRUCTURED STREAMING

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@MaxGekk @SparkQA @maropu @viirya @cloud-fan @kiszk @HyukjinKwon @dongjoon-hyun