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

MaxGekk · 2020-04-11T09:49:12Z

What changes were proposed in this pull request?

Reuse the rebaseGregorianToJulianMicros() and rebaseJulianToGregorianMicros() functions introduced by the PR #28119 in DateTimeUtils.toJavaTimestamp() and fromJavaTimestamp(). Actually, new implementation is derived from Spark 2.4 + rebasing via pre-calculated rebasing maps.

Why are the changes needed?

The changes speed up conversions to/from java.sql.Timestamp, and as a consequence the PR improve performance of ORC datasource in loading/saving timestamps:

Saving ~ x2.8 faster in master, and -11% against Spark 2.4.6
Loading - x3.2-4.5 faster in master, -5% against Spark 2.4.6

Before:

Save timestamps to ORC:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
after 1582                                        59877          59877           0          1.7         598.8       0.0X
before 1582                                       61361          61361           0          1.6         613.6       0.0X

Load timestamps from ORC:                 Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
after 1582, vec off                               48197          48288         118          2.1         482.0       1.0X
after 1582, vec on                                38247          38351         128          2.6         382.5       1.3X
before 1582, vec off                              53179          53359         249          1.9         531.8       0.9X
before 1582, vec on                               44076          44268         269          2.3         440.8       1.1X

After:

Save timestamps to ORC:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
after 1582                                        21250          21250           0          4.7         212.5       0.1X
before 1582                                       22105          22105           0          4.5         221.0       0.1X

Load timestamps from ORC:                 Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
after 1582, vec off                               14903          14933          40          6.7         149.0       1.0X
after 1582, vec on                                 8342           8426          73         12.0          83.4       1.8X
before 1582, vec off                              15528          15575          76          6.4         155.3       1.0X
before 1582, vec on                                9025           9075          61         11.1          90.2       1.7X

Spark 2.4.6-SNAPSHOT:

Save timestamps to ORC:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
after 1582                                        18858          18858           0          5.3         188.6       1.0X
before 1582                                       18508          18508           0          5.4         185.1       1.0X

Load timestamps from ORC:                 Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
after 1582, vec off                               14063          14177         143          7.1         140.6       1.0X
after 1582, vec on                                 5955           6029         100         16.8          59.5       2.4X
before 1582, vec off                              14119          14126           7          7.1         141.2       1.0X
before 1582, vec on                                5991           6007          25         16.7          59.9       2.3X

Does this PR introduce any user-facing change?

Yes, the to_utc_timestamp function returns the later local timestamp in the case of overlapping local timestamps at daylight saving time. it's changed back to the 2.4 behavior.

How was this patch tested?

By existing test suite DateTimeUtilsSuite, RebaseDateTimeSuite, DateFunctionsSuite, DateExpressionsSuites, ParquetIOSuite, OrcHadoopFsRelationSuite.
Re-generating results of the benchmarks DateTimeBenchmark and DateTimeRebaseBenchmark in the environment:

Item	Description
Region	us-west-2 (Oregon)
Instance	r3.xlarge
AMI	ubuntu/images/hvm-ssd/ubuntu-bionic-18.04-amd64-server-20190722.1 (ami-06f2f779464715dc5)
Java	OpenJDK 64-Bit Server VM 1.8.0_242 and OpenJDK 64-Bit Server VM 11.0.6+10

SparkQA · 2020-04-11T14:05:28Z

Test build #121130 has finished for PR 28189 at commit 794cc5e.

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

MaxGekk · 2020-04-11T16:02:45Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

      test("2016-11-06 00:59:59", tz, "2016-11-06 07:59:59.0")
-      test("2016-11-06 01:00:00", tz, "2016-11-06 08:00:00.0")
-      test("2016-11-06 01:59:59", tz, "2016-11-06 08:59:59.0")
+      test("2016-11-06 01:00:00", tz, "2016-11-06 09:00:00.0")


what's the behavior of Spark 2.4?

See the same test in 2.4 https://github.com/apache/spark/blob/branch-2.4/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala#L580-L581 but it sets PST time zone.

as we have looked into the JDK code before, PST is treated as LA. So the behavior is the same as 2.4, and it's great!

can we highlight it in the Does this PR introduce any user-facing change? section? Say it's changed back to the 2.4 behavior.

SparkQA · 2020-04-11T17:59:06Z

Test build #121132 has finished for PR 28189 at commit 94dc31e.

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

MaxGekk · 2020-04-12T16:29:44Z

@cloud-fan @HyukjinKwon @dongjoon-hyun Please, have a look at the PR.

…va-timestamp

SparkQA · 2020-04-13T12:30:45Z

Test build #121192 has finished for PR 28189 at commit a28f2a1.

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

cloud-fan · 2020-04-13T12:37:04Z

retest this please

SparkQA · 2020-04-13T14:42:34Z

Test build #121209 has finished for PR 28189 at commit a28f2a1.

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

MaxGekk · 2020-04-13T16:55:33Z

jenkins, retest this, please

SparkQA · 2020-04-13T21:33:24Z

Test build #121218 has finished for PR 28189 at commit a28f2a1.

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

cloud-fan · 2020-04-14T04:50:36Z

thanks, merging to master/3.0!

…mestamp ### What changes were proposed in this pull request? Reuse the `rebaseGregorianToJulianMicros()` and `rebaseJulianToGregorianMicros()` functions introduced by the PR #28119 in `DateTimeUtils`.`toJavaTimestamp()` and `fromJavaTimestamp()`. Actually, new implementation is derived from Spark 2.4 + rebasing via pre-calculated rebasing maps. ### Why are the changes needed? The changes speed up conversions to/from java.sql.Timestamp, and as a consequence the PR improve performance of ORC datasource in loading/saving timestamps: - Saving ~ **x2.8 faster** in master, and -11% against Spark 2.4.6 - Loading - **x3.2-4.5 faster** in master, -5% against Spark 2.4.6 Before: ``` Save timestamps to ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582 59877 59877 0 1.7 598.8 0.0X before 1582 61361 61361 0 1.6 613.6 0.0X Load timestamps from ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582, vec off 48197 48288 118 2.1 482.0 1.0X after 1582, vec on 38247 38351 128 2.6 382.5 1.3X before 1582, vec off 53179 53359 249 1.9 531.8 0.9X before 1582, vec on 44076 44268 269 2.3 440.8 1.1X ``` After: ``` Save timestamps to ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582 21250 21250 0 4.7 212.5 0.1X before 1582 22105 22105 0 4.5 221.0 0.1X Load timestamps from ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582, vec off 14903 14933 40 6.7 149.0 1.0X after 1582, vec on 8342 8426 73 12.0 83.4 1.8X before 1582, vec off 15528 15575 76 6.4 155.3 1.0X before 1582, vec on 9025 9075 61 11.1 90.2 1.7X ``` Spark 2.4.6-SNAPSHOT: ``` Save timestamps to ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582 18858 18858 0 5.3 188.6 1.0X before 1582 18508 18508 0 5.4 185.1 1.0X Load timestamps from ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582, vec off 14063 14177 143 7.1 140.6 1.0X after 1582, vec on 5955 6029 100 16.8 59.5 2.4X before 1582, vec off 14119 14126 7 7.1 141.2 1.0X before 1582, vec on 5991 6007 25 16.7 59.9 2.3X ``` ### Does this PR introduce any user-facing change? Yes, the `to_utc_timestamp` function returns the later local timestamp in the case of overlapping local timestamps at daylight saving time. it's changed back to the 2.4 behavior. ### How was this patch tested? - By existing test suite `DateTimeUtilsSuite`, `RebaseDateTimeSuite`, `DateFunctionsSuite`, `DateExpressionsSuites`, `ParquetIOSuite`, `OrcHadoopFsRelationSuite`. - Re-generating results of the benchmarks `DateTimeBenchmark` and `DateTimeRebaseBenchmark` in the environment: | Item | Description | | ---- | ----| | Region | us-west-2 (Oregon) | | Instance | r3.xlarge | | AMI | ubuntu/images/hvm-ssd/ubuntu-bionic-18.04-amd64-server-20190722.1 (ami-06f2f779464715dc5) | | Java | OpenJDK 64-Bit Server VM 1.8.0_242 and OpenJDK 64-Bit Server VM 11.0.6+10 | Closes #28189 from MaxGekk/optimize-to-from-java-timestamp. Authored-by: Max Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a0f8cc0) Signed-off-by: Wenchen Fan <[email protected]>

dongjoon-hyun · 2020-04-14T05:24:59Z

Hi, @cloud-fan .
This seems to break branch-3.0. Could you take a look at that once more?

[ERROR] [Error] /home/runner/work/spark/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:181: not found: value millisToMicros
700
[ERROR] one error found

dongjoon-hyun · 2020-04-14T05:25:56Z

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

-      // year in Proleptic Gregorian calendar. And 1000-02-29 doesn't exist in it.
-      .plusDays(t.getDate - 1) // Returns the next valid date after `date.getDate - 1` days
-    instantToMicros(localDateTime.atZone(ZoneId.systemDefault).toInstant)
+    val micros = millisToMicros(t.getTime) + (t.getNanos / NANOS_PER_MICROS) % MICROS_PER_MILLIS


This seems to be missing in branch-3.0.

$ git grep millisToMicros sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala: val micros = millisToMicros(t.getTime) + (t.getNanos / NANOS_PER_MICROS) % MICROS_PER_MILLIS

cloud-fan · 2020-04-14T05:41:12Z

Sorry I didn't run tests when backporting as it only has a very small conflict. I've fixed it in branch-3.0 after local tests pass. Thanks for catching @dongjoon-hyun !

MaxGekk · 2020-04-14T06:06:38Z

WDYT of merging #27618 to branch-3.0? Missing the changes in branch-3.0 causes problems already twice, see #27964 (comment)

dongjoon-hyun · 2020-04-14T06:23:55Z

Thank you so much, @cloud-fan .

…mestamp ### What changes were proposed in this pull request? Reuse the `rebaseGregorianToJulianMicros()` and `rebaseJulianToGregorianMicros()` functions introduced by the PR apache#28119 in `DateTimeUtils`.`toJavaTimestamp()` and `fromJavaTimestamp()`. Actually, new implementation is derived from Spark 2.4 + rebasing via pre-calculated rebasing maps. ### Why are the changes needed? The changes speed up conversions to/from java.sql.Timestamp, and as a consequence the PR improve performance of ORC datasource in loading/saving timestamps: - Saving ~ **x2.8 faster** in master, and -11% against Spark 2.4.6 - Loading - **x3.2-4.5 faster** in master, -5% against Spark 2.4.6 Before: ``` Save timestamps to ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582 59877 59877 0 1.7 598.8 0.0X before 1582 61361 61361 0 1.6 613.6 0.0X Load timestamps from ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582, vec off 48197 48288 118 2.1 482.0 1.0X after 1582, vec on 38247 38351 128 2.6 382.5 1.3X before 1582, vec off 53179 53359 249 1.9 531.8 0.9X before 1582, vec on 44076 44268 269 2.3 440.8 1.1X ``` After: ``` Save timestamps to ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582 21250 21250 0 4.7 212.5 0.1X before 1582 22105 22105 0 4.5 221.0 0.1X Load timestamps from ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582, vec off 14903 14933 40 6.7 149.0 1.0X after 1582, vec on 8342 8426 73 12.0 83.4 1.8X before 1582, vec off 15528 15575 76 6.4 155.3 1.0X before 1582, vec on 9025 9075 61 11.1 90.2 1.7X ``` Spark 2.4.6-SNAPSHOT: ``` Save timestamps to ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582 18858 18858 0 5.3 188.6 1.0X before 1582 18508 18508 0 5.4 185.1 1.0X Load timestamps from ORC: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ after 1582, vec off 14063 14177 143 7.1 140.6 1.0X after 1582, vec on 5955 6029 100 16.8 59.5 2.4X before 1582, vec off 14119 14126 7 7.1 141.2 1.0X before 1582, vec on 5991 6007 25 16.7 59.9 2.3X ``` ### Does this PR introduce any user-facing change? Yes, the `to_utc_timestamp` function returns the later local timestamp in the case of overlapping local timestamps at daylight saving time. it's changed back to the 2.4 behavior. ### How was this patch tested? - By existing test suite `DateTimeUtilsSuite`, `RebaseDateTimeSuite`, `DateFunctionsSuite`, `DateExpressionsSuites`, `ParquetIOSuite`, `OrcHadoopFsRelationSuite`. - Re-generating results of the benchmarks `DateTimeBenchmark` and `DateTimeRebaseBenchmark` in the environment: | Item | Description | | ---- | ----| | Region | us-west-2 (Oregon) | | Instance | r3.xlarge | | AMI | ubuntu/images/hvm-ssd/ubuntu-bionic-18.04-amd64-server-20190722.1 (ami-06f2f779464715dc5) | | Java | OpenJDK 64-Bit Server VM 1.8.0_242 and OpenJDK 64-Bit Server VM 11.0.6+10 | Closes apache#28189 from MaxGekk/optimize-to-from-java-timestamp. Authored-by: Max Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

MaxGekk added 4 commits April 11, 2020 11:35

Rewrite toJavaTimestamp/fromJavaTimestamp

a56c752

Re-gen DateTimeRebaseBenchmark results on JDK 8

794cc5e

Re-gen DateTimeBenchmark results on JDK 8

fd06ecc

Re-gen benchmark results on JDK 11

94dc31e

MaxGekk commented Apr 11, 2020

View reviewed changes

MaxGekk changed the title ~~[WIP][SQL] Fix regressions of toJavaTimestamp/fromJavaTimestamp~~ [SPARK-31426][SQL] Fix regressions of toJavaTimestamp/fromJavaTimestamp Apr 12, 2020

MaxGekk added 4 commits April 13, 2020 06:03

Merge remote-tracking branch 'origin/master' into optimize-to-from-ja…

055ac97

…va-timestamp

Merge remote-tracking branch 'origin/master' into optimize-to-from-ja…

b331ad8

…va-timestamp

Re-gen DateTimeRebaseBenchmark results on JDK 11

5dc9a3a

Re-gen DateTimeRebaseBenchmark results on JDK 8

a28f2a1

probot-autolabeler bot added the SQL label Apr 13, 2020

cloud-fan approved these changes Apr 13, 2020

View reviewed changes

cloud-fan changed the title ~~[SPARK-31426][SQL] Fix regressions of toJavaTimestamp/fromJavaTimestamp~~ [SPARK-31426][SQL] Fix perf regressions of toJavaTimestamp/fromJavaTimestamp Apr 14, 2020

cloud-fan closed this in a0f8cc0 Apr 14, 2020

dongjoon-hyun reviewed Apr 14, 2020

View reviewed changes

MaxGekk deleted the optimize-to-from-java-timestamp branch June 5, 2020 19:47

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

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

Uh oh!

Conversation

MaxGekk commented Apr 11, 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 Apr 11, 2020

Uh oh!

MaxGekk Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 11, 2020

Uh oh!

MaxGekk commented Apr 12, 2020

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

cloud-fan commented Apr 13, 2020

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

MaxGekk commented Apr 13, 2020

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

cloud-fan commented Apr 14, 2020

Uh oh!

dongjoon-hyun commented Apr 14, 2020

Uh oh!

dongjoon-hyun Apr 14, 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!

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

Uh oh!

Uh oh!

MaxGekk commented Apr 14, 2020

Uh oh!

dongjoon-hyun commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaxGekk commented Apr 11, 2020 •

edited

Loading

dongjoon-hyun Apr 14, 2020 •

edited

Loading

cloud-fan commented Apr 14, 2020 •

edited

Loading