Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jun 13, 2020

What changes were proposed in this pull request?

  1. Set the given time zone as the first parameter of RebaseDateTime.rebaseJulianToGregorianMicros() and rebaseGregorianToJulianMicros() to Java 7 GregorianCalendar.
    val cal = new Calendar.Builder()
      // `gregory` is a hybrid calendar that supports both the Julian and Gregorian calendar systems
      .setCalendarType("gregory")
    ...
      .setTimeZone(tz)
      .build()

This makes the instance of the calendar independent from the default JVM time zone.

  1. Change type of the first parameter from ZoneId to TimeZone. This allows to avoid unnecessary conversion from TimeZone to ZoneId, for example in
  def rebaseJulianToGregorianMicros(micros: Long): Long = {
    ...
      if (rebaseRecord == null || micros < rebaseRecord.switches(0)) {
        rebaseJulianToGregorianMicros(timeZone.toZoneId, micros)

and back to TimeZone inside of rebaseJulianToGregorianMicros(zoneId: ZoneId, ...)

  1. Modify tests in RebaseDateTimeSuite, and set the default JVM time zone only for functions that depend on it.

Why are the changes needed?

  1. Ignoring passed parameter and using a global variable is bad practice.
  2. Dependency from the global state doesn't allow to run the functions in parallel otherwise there is non-zero probability that the functions may return wrong result if the default JVM is changed during their execution.
  3. This open opportunity for parallelisation of JSON files generation gregorian-julian-rebase-micros.json and julian-gregorian-rebase-micros.json. Currently, the tests generate 'gregorian-julian-rebase-micros.json' and generate 'julian-gregorian-rebase-micros.json' generate the JSON files by iterating over all time zones sequentially w/ step of 1 week. Due to the large step, we can miss some spikes in diffs between 2 calendars (Java 8 Gregorian and Java 7 hybrid calendars) as the PR [SPARK-31959][SQL][test-java11] Fix Gregorian-Julian micros rebasing while switching standard time zone offset #28787 fixed and [SPARK-31986][SQL] Fix Julian-Gregorian micros rebasing of overlapping local timestamps #28816 should fix.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running existing tests from RebaseDateTimeSuite.

@SparkQA
Copy link

SparkQA commented Jun 13, 2020

Test build #123982 has finished for PR 28824 at commit c5306ce.

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

@MaxGekk
Copy link
Member Author

MaxGekk commented Jun 14, 2020

@cloud-fan @HyukjinKwon Please, review this PR.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM

MaxGekk added 2 commits June 15, 2020 18:30
…-rebasing

# Conflicts:
#	sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/RebaseDateTimeSuite.scala
@SparkQA
Copy link

SparkQA commented Jun 15, 2020

Test build #124061 has finished for PR 28824 at commit b42d105.

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

MaxGekk added 2 commits June 16, 2020 09:29
…-rebasing

# Conflicts:
#	sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/RebaseDateTime.scala
// Check reverse rebasing
assert(rebaseJulianToGregorianMicros(rebasedEarlierMicros) === earlierMicros)
assert(rebaseJulianToGregorianMicros(rebasedLaterMicros) === laterMicros)
// Check reverse not-optimized rebasing
Copy link
Contributor

Choose a reason for hiding this comment

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

does this rely on JVM timezone?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they don't. Let me move them out of the default JVM time zone. Thanks.

@SparkQA
Copy link

SparkQA commented Jun 16, 2020

Test build #124103 has finished for PR 28824 at commit 381e950.

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

@SparkQA
Copy link

SparkQA commented Jun 16, 2020

Test build #124105 has finished for PR 28824 at commit 9f4f286.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 6e9ff72 Jun 16, 2020
@MaxGekk MaxGekk deleted the pure-micros-rebasing branch December 11, 2020 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants