[SPARK-30869][SQL] Convert dates to/from timestamps in microseconds precision #27618

MaxGekk · 2020-02-18T11:51:23Z

What changes were proposed in this pull request?

In the PR, I propose to replace:

millisToDays() by microsToDays() which accepts microseconds since the epoch and returns days since the epoch in the specified time zone. The last one is the internal representation of Catalyst's DateType.
daysToMillis() by daysToMicros() which accepts days since the epoch in some time zone and returns the number of microseconds since the epoch. The last one is internal representation of Catalyst's TimestampType.
fromMillis() by millisToMicros()
toMillis() by microsToMillis()

Why are the changes needed?

Spark stores timestamps in microseconds precision, so, there is no actual need to convert dates to milliseconds, and then to microseconds. As examples, look at DateTimeUtils functions monthsBetween() and truncTimestamp().

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites UnivocityParserSuite, DateExpressionsSuite, ComputeCurrentTimeSuite, DateTimeUtilsSuite, DateFunctionsSuite, JsonSuite, StreamSuite.

SparkQA · 2020-02-18T16:44:30Z

Test build #118631 has finished for PR 27618 at commit 7d812a3.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

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

cloud-fan

LGTM except some minor comments

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

Test build #118758 has finished for PR 27618 at commit f78d773.

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

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

SparkQA · 2020-02-21T13:47:55Z

Test build #118764 has finished for PR 27618 at commit 29caa4e.

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

cloud-fan · 2020-02-21T14:24:22Z

This is a refactor PR that should go to master only. But we need to fix the usages of utils like MILLISECONDS.toMicros to make them overflow-safe. Since the fix should go to 3.0 as well, shall we merge bug fix PRs before the refactor ones? This can avoid code conflict when working with bug fix PRs.

MaxGekk · 2020-02-23T09:06:55Z

Since the fix should go to 3.0 as well, shall we merge bug fix PRs before the refactor ones? This can avoid code conflict when working with bug fix PRs.

Here is the PR #27676

…lis-by-micros # Conflicts: # 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

SparkQA · 2020-02-24T07:08:51Z

Test build #118852 has finished for PR 27618 at commit 9a4582e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

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

Test build #118854 has finished for PR 27618 at commit 15bedb3.

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

cloud-fan · 2020-02-24T08:37:47Z

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

    }
    // using milliseconds can cause precision loss with more than 8 digits
    // we follow Hive's implementation which uses seconds
-    val secondsInDay1 = MILLISECONDS.toSeconds(millis1 - daysToMillis(date1, zoneId))


shall we call Math.floorDiv?

Highly likely, yes. I will prepare a separate fix.

The error of rounding is invisible in dividing by DAYS.toSeconds(31). At least, I haven't reproduce the issue yet.

ah, this is "seconds in day", so it's always positive.

cloud-fan · 2020-02-24T08:38:33Z

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

    level match {
      case TRUNC_TO_MICROSECOND => t
+      case TRUNC_TO_MILLISECOND =>
+        millisToMicros(microsToMillis(t))


is it faster than t - Math.floorMod(t, MICROS_PER_MILLI)?

I think it is almost the same:

millisToMicros(microsToMillis(t)) ~= multiplyExact + floorDiv

t - Math.floorMod(t, MICROS_PER_MILLI) = floorDiv + *
I can replaced it by floorMod for consistency

Let's pick t - Math.floorMod(t, MICROS_PER_MILLI) for consistency.

MaxGekk · 2020-02-24T12:55:41Z

jenkins, retest this, please

SparkQA · 2020-02-24T17:28:01Z

Test build #118867 has finished for PR 27618 at commit 15bedb3.

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

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

Test build #118896 has finished for PR 27618 at commit 17d9566.

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

cloud-fan · 2020-02-25T08:33:00Z

retest this please

SparkQA · 2020-02-25T14:22:22Z

Test build #118913 has finished for PR 27618 at commit 17d9566.

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

cloud-fan · 2020-02-25T15:05:42Z

thanks, merging to master!

…recision ### What changes were proposed in this pull request? In the PR, I propose to replace: 1. `millisToDays()` by `microsToDays()` which accepts microseconds since the epoch and returns days since the epoch in the specified time zone. The last one is the internal representation of Catalyst's DateType. 2. `daysToMillis()` by `daysToMicros()` which accepts days since the epoch in some time zone and returns the number of microseconds since the epoch. The last one is internal representation of Catalyst's TimestampType. 3. `fromMillis()` by `millisToMicros()` 4. `toMillis()` by `microsToMillis()` ### Why are the changes needed? Spark stores timestamps in microseconds precision, so, there is no actual need to convert dates to milliseconds, and then to microseconds. As examples, look at DateTimeUtils functions `monthsBetween()` and `truncTimestamp()`. ### Does this PR introduce any user-facing change? No ### How was this patch tested? By existing test suites UnivocityParserSuite, DateExpressionsSuite, ComputeCurrentTimeSuite, DateTimeUtilsSuite, DateFunctionsSuite, JsonSuite, StreamSuite. Closes apache#27618 from MaxGekk/replace-millis-by-micros. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

Adjust tests

7d812a3

MaxGekk changed the title ~~[WIP][SQL] Convert dates to/from timestamps in microseconds precision~~ [SPARK-30869][SQL] Convert dates to/from timestamps in microseconds precision Feb 18, 2020

MaxGekk mentioned this pull request Feb 21, 2020

[SPARK-30869][SQL] Convert dates to/from timestamps in microseconds precision #27661

Closed

cloud-fan reviewed Feb 21, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

toMillis -> microsToMillis, fromMillis -> millisToMicros

f78d773

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/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated Show resolved Hide resolved

cloud-fan approved these changes Feb 21, 2020

View reviewed changes

MaxGekk added 3 commits February 21, 2020 11:06

Rename a test

ac444aa

Use millisToMicros and microsToMillis in Java Date conversions

ffaccad

Prevent overflow in millisToMicros

29caa4e

MaxGekk mentioned this pull request Feb 21, 2020

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

Closed

cloud-fan reviewed Feb 21, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala 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 Show resolved Hide resolved

MaxGekk added 2 commits February 24, 2020 09:55

Merge remote-tracking branch 'remotes/origin/master' into replace-mil…

3c7ef73

…lis-by-micros # Conflicts: # 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

Fix build fails

9a4582e

Fix build fails

15bedb3

cloud-fan reviewed Feb 24, 2020

View reviewed changes

Uniform truncTimestamp: use floorMod

17d9566

cloud-fan closed this in ffc0935 Feb 25, 2020

MaxGekk mentioned this pull request Mar 22, 2020

[SPARK-31183][SQL][FOLLOWUP] Move rebase tests to AvroSuite and check the rebase flag out of function bodies #27964

Closed

This was referenced Apr 9, 2020

[SPARK-31359][SQL][3.0] Speed up timestamps rebasing #28163

Closed

[SPARK-31426][SQL] Fix perf regressions of toJavaTimestamp/fromJavaTimestamp #28189

Closed

MaxGekk mentioned this pull request Jun 1, 2020

[SPARK-31885][SQL] Fix filter push down for old millis timestamps to Parquet #28693

Closed

MaxGekk deleted the replace-millis-by-micros branch June 5, 2020 19:46

[SPARK-30869][SQL] Convert dates to/from timestamps in microseconds precision #27618

[SPARK-30869][SQL] Convert dates to/from timestamps in microseconds precision #27618

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Feb 21, 2020

Uh oh!

cloud-fan commented Feb 21, 2020

Uh oh!

MaxGekk commented Feb 23, 2020

Uh oh!

SparkQA commented Feb 24, 2020

Uh oh!

SparkQA commented Feb 24, 2020

Uh oh!

cloud-fan Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Feb 24, 2020

Uh oh!

SparkQA commented Feb 24, 2020

Uh oh!

SparkQA commented Feb 25, 2020

Uh oh!

cloud-fan commented Feb 25, 2020

Uh oh!

SparkQA commented Feb 25, 2020

Uh oh!

cloud-fan commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MaxGekk commented Feb 18, 2020 •

edited

Loading