[SPARK-27401][SQL] Refactoring conversion of Timestamp to/from java.sql.Timestamp #24311

MaxGekk · 2019-04-06T18:15:59Z

What changes were proposed in this pull request?

In the PR, I propose simpler implementation of toJavaTimestamp()/fromJavaTimestamp() by reusing existing functions of DateTimeUtils. This will allow to:

Simply implementation of toJavaTimestamp(), and handle properly negative inputs.
Detect Long overflow in conversion of milliseconds (java.sql.Timestamp) to microseconds (Catalyst's Timestamp).

How was this patch tested?

By existing test suites DateTimeUtilsSuite, DateFunctionsSuite, DateExpressionsSuite and CastSuite. And by new benchmark for export/import timestamps added to DateTimeBenchmark:

Before:

To/from java.sql.Timestamp:               Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
From java.sql.Timestamp                             290            335          49         17.2          58.0       1.0X
Collect longs                                      1234           1681         487          4.1         246.8       0.2X
Collect timestamps                                 1718           1755          63          2.9         343.7       0.2X

After:

To/from java.sql.Timestamp:               Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
From java.sql.Timestamp                             283            301          19         17.7          56.6       1.0X
Collect longs                                      1048           1087          36          4.8         209.6       0.3X
Collect timestamps                                 1425           1479          56          3.5         285.1       0.2X

SparkQA · 2019-04-06T20:23:19Z

Test build #104344 has finished for PR 24311 at commit b0ced9f.

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

SparkQA · 2019-04-06T20:40:42Z

Test build #104345 has finished for PR 24311 at commit 6129c9a.

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

MaxGekk · 2019-04-06T21:15:38Z

Failed ORC related test which generates random dates - HiveOrcHadoopFsRelationSuite. It seems ORC uses its own methods for converting Catalyst's days since epoch to something like java.sql.Date that is based on Julian calendar for old dates:

!== Correct Answer - 10 ==    == Spark Answer - 10 ==
 struct<index:int,col:date>   struct<index:int,col:date>
 [1,3477-08-12]               [1,3477-08-12]
 [2,7867-09-20]               [2,7867-09-20]
 [3,null]                     [3,null]
 [4,6577-03-26]               [4,6577-03-26]
 [5,8002-07-25]               [5,8002-07-25]
 [6,2154-05-10]               [6,2154-05-10]
 [7,4921-05-03]               [7,4921-05-03]
![8,0647-07-01]               [8,0647-06-28]
 [9,9422-12-25]               [9,9422-12-25]
 [10,2677-06-15]              [10,2677-06-15]

@dongjoon-hyun Am I right? It is strange that only this ORC test failed.

dongjoon-hyun · 2019-04-07T01:02:59Z

Thank you for pinging me, @MaxGekk . I'll take a look today.

BTW, @srowen . Sorry, but do you mind if I ask why you approve this PR suffering from the UT failures? Does this pass in your environment?

srowen · 2019-04-07T01:13:16Z

Oh, really I mean this looks fine pending tests. If they're transient failures, fine. If they're not, obviously they need to be fixed. here I assume they aren't significant failures, just tests that need to be adjusted.

dongjoon-hyun · 2019-04-07T01:13:17Z

Oh, @MaxGekk . According to the last reverting commit, you are not aiming DATE in this PR. Are you going to file a JIRA issue for DATE type?

dongjoon-hyun · 2019-04-07T01:14:37Z

Thank you, @srowen . I see~
It seems that @MaxGekk reduced the scope of this PR first to pass the Jenkins.

dongjoon-hyun · 2019-04-07T01:27:51Z

Before the last reverting, I got the following result. @MaxGekk . If there was an issue, it might be an old Hive 1.2.1 issue which is fixed in Apache ORC. If needed, you can file a JIRA issue. For old Hive 1.2.1 issue, I can dig it for you but it might be difficult to fix it because it seems to be not inside both Apache Spark and Apache ORC.

OrcHadoopFsRelationSuite passed.
HiveOrcHadoopFsRelationSuite failed.

SparkQA · 2019-04-07T01:53:53Z

Test build #104347 has finished for PR 24311 at commit bf4b6b6.

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

dongjoon-hyun · 2019-04-07T02:06:14Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala

+  lazy val timestampLiteralGen: Gen[Literal] = {
+    for { millis <- Gen.choose(yearToMillis(-9999), yearToMillis(10000)) }
+      yield Literal.create(new Timestamp(millis), TimestampType)
+  }


@MaxGekk . In general, we don't change the test cases during refactoring in order to ensure the test coverage. I'm wondering if this is a required change due to the known limitation of the new functions. Although this looks minor, I'd make this as an another PR.

I changed the random generator for timestamps, and limit max/min values for milliseconds since epoch because the long random generator can produce milliseconds that caused Long overflow at conversion of milliseconds to microseconds. Internally as you know we store microseconds since epoch in TimestampType. For example, the old (current) generator can create an instance of java.sql.Timestamp(-3948373668011580000, 570000000). New function fromJavaTimestamp calls instantToMicros, and inside of it we use multiplyExact which can detect Long overflow on multiplication:

def instantToMicros(instant: Instant): Long = { val us = Math.multiplyExact(-3948373668011580, 1000000) ... }

Previous (current) implementation doesn't detect the overflow at all.

The long overflow exception caused failures in

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

Line 202 in 1d20d13

checkConsistencyBetweenInterpretedAndCodegen(

That's the exact reason you should not put that in this PR, @MaxGekk .
Please do that improvement in another PR first. And do the refactoring PR later.

@dongjoon-hyun ok. I will open another PR. Thanks.

MaxGekk · 2019-04-07T07:20:39Z

it because it seems to be not inside both Apache Spark and Apache ORC.

OrcHadoopFsRelationSuite passed.

HiveOrcHadoopFsRelationSuite failed.

Yeh, it seems there could be a calendar incompatibility issue somewhere inside Hive + ORC which could cause the difference ![8,0647-07-01] [8,0647-06-28] for old days before 1582 (I guess). I just thought we switched to Proleptic Gregorian calendar everywhere in Spark (https://issues.apache.org/jira/browse/SPARK-26651).

In any case, I think my date (not timestamp) related changes are potentially more expensive comparing to current implementation because of conversion of java.sql.Date -> java.time.LocalDate. The conversion extracts components like year, month and etc from java.sql.Date that are not cheap but current implementation does time zone shifting which is not necessary too. In any case, I am going to leave Java <-> Catalyst's date conversions as is for now.

SparkQA · 2019-04-07T20:27:53Z

Test build #104360 has finished for PR 24311 at commit 20dfacf.

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

SparkQA · 2019-04-07T23:19:55Z

Test build #104365 has finished for PR 24311 at commit ec15d82.

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

dongjoon-hyun · 2019-04-08T17:00:23Z

Retest this please.

dongjoon-hyun

+1, LGTM (pending Jenkins).
Thank you, @MaxGekk and @srowen .

SparkQA · 2019-04-08T21:21:37Z

Test build #104399 has finished for PR 24311 at commit ec15d82.

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

dongjoon-hyun · 2019-04-09T00:00:23Z

sql/core/benchmarks/DateTimeBenchmark-results.txt

+To/from java.sql.Timestamp:               Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
+------------------------------------------------------------------------------------------------------------------------
+From java.sql.Timestamp                             309            316           8         16.2          61.9       1.0X
+Collect longs                                      1410           2747        1158          3.5         282.0       0.2X


Oh, could you rerun this in the more stable environment?
Stdev looks too high.

SparkQA · 2019-04-09T07:05:02Z

Test build #104421 has finished for PR 24311 at commit b4cade3.

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

MaxGekk · 2019-04-09T08:30:24Z

jenkins, retest this, please

SparkQA · 2019-04-09T12:12:16Z

Test build #104425 has finished for PR 24311 at commit b4cade3.

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

MaxGekk · 2019-04-09T13:04:34Z

jenkins, retest this, please

SparkQA · 2019-04-09T16:51:08Z

Test build #104431 has finished for PR 24311 at commit b4cade3.

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

MaxGekk · 2019-04-09T18:33:42Z

I changed only sql/core/benchmarks/DateTimeBenchmark-results.txt since the last success run #24311 (comment) . @dongjoon-hyun @srowen Does it make sense to re-run this again? It seems there is a permanent problem with python tests.

MaxGekk · 2019-04-09T18:37:46Z

jenkins, retest this, please

SparkQA · 2019-04-09T22:40:04Z

Test build #104449 has finished for PR 24311 at commit b4cade3.

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

dongjoon-hyun · 2019-04-09T22:41:40Z

Finally, it passed! Merged to master. Thank you, @MaxGekk and @srowen !

MaxGekk added 3 commits April 6, 2019 20:07

Refactoring conversion to/from java.sql.Date/Timestamp

bc1fdbf

Generate reasonable literals of Date/Timestamp

b0ced9f

Remove unused millisToDays and daysToMillis

6129c9a

Revert DATE related changes

bf4b6b6

MaxGekk changed the title ~~[SPARK-27401][SQL] Refactoring conversion of Date/Timestamp to/from java.sql.Date/Timestamp~~ [SPARK-27401][SQL] Refactoring conversion of Timestamp to/from java.sql.Timestamp Apr 6, 2019

srowen approved these changes Apr 6, 2019

View reviewed changes

dongjoon-hyun reviewed Apr 7, 2019

View reviewed changes

Add a bechmark for export/import timestamps

20dfacf

Revert timestampLiteralGen

ec15d82

MaxGekk mentioned this pull request Apr 7, 2019

[SPARK-27405][SQL][TEST] Restrict the range of generated random timestamps #24316

Closed

dongjoon-hyun approved these changes Apr 8, 2019

View reviewed changes

dongjoon-hyun reviewed Apr 9, 2019

View reviewed changes

Re-run benchmarks

b4cade3

dongjoon-hyun closed this in 63e4bf4 Apr 9, 2019

MaxGekk deleted the conv-java-sql-date-timestamp branch September 18, 2019 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-27401][SQL] Refactoring conversion of Timestamp to/from java.sql.Timestamp #24311

[SPARK-27401][SQL] Refactoring conversion of Timestamp to/from java.sql.Timestamp #24311

MaxGekk commented Apr 6, 2019 •

edited

Loading

SparkQA commented Apr 6, 2019

SparkQA commented Apr 6, 2019

MaxGekk commented Apr 6, 2019

dongjoon-hyun commented Apr 7, 2019 •

edited

Loading

srowen commented Apr 7, 2019

dongjoon-hyun commented Apr 7, 2019

dongjoon-hyun commented Apr 7, 2019

dongjoon-hyun commented Apr 7, 2019

SparkQA commented Apr 7, 2019

dongjoon-hyun Apr 7, 2019 •

edited

Loading

MaxGekk Apr 7, 2019

MaxGekk Apr 7, 2019

dongjoon-hyun Apr 7, 2019 •

edited

Loading

MaxGekk Apr 7, 2019

MaxGekk commented Apr 7, 2019 •

edited

Loading

SparkQA commented Apr 7, 2019

SparkQA commented Apr 7, 2019

dongjoon-hyun commented Apr 8, 2019

dongjoon-hyun left a comment •

edited

Loading

SparkQA commented Apr 8, 2019

dongjoon-hyun Apr 9, 2019 •

edited

Loading

SparkQA commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

SparkQA commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

SparkQA commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

SparkQA commented Apr 9, 2019

dongjoon-hyun commented Apr 9, 2019

[SPARK-27401][SQL] Refactoring conversion of Timestamp to/from java.sql.Timestamp #24311

[SPARK-27401][SQL] Refactoring conversion of Timestamp to/from java.sql.Timestamp #24311

Conversation

MaxGekk commented Apr 6, 2019 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 6, 2019

SparkQA commented Apr 6, 2019

MaxGekk commented Apr 6, 2019

dongjoon-hyun commented Apr 7, 2019 • edited Loading

srowen commented Apr 7, 2019

dongjoon-hyun commented Apr 7, 2019

dongjoon-hyun commented Apr 7, 2019

dongjoon-hyun commented Apr 7, 2019

SparkQA commented Apr 7, 2019

dongjoon-hyun Apr 7, 2019 • edited Loading

Choose a reason for hiding this comment

MaxGekk Apr 7, 2019

Choose a reason for hiding this comment

MaxGekk Apr 7, 2019

Choose a reason for hiding this comment

dongjoon-hyun Apr 7, 2019 • edited Loading

Choose a reason for hiding this comment

MaxGekk Apr 7, 2019

Choose a reason for hiding this comment

MaxGekk commented Apr 7, 2019 • edited Loading

SparkQA commented Apr 7, 2019

SparkQA commented Apr 7, 2019

dongjoon-hyun commented Apr 8, 2019

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

SparkQA commented Apr 8, 2019

dongjoon-hyun Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

SparkQA commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

SparkQA commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

MaxGekk commented Apr 9, 2019

SparkQA commented Apr 9, 2019

dongjoon-hyun commented Apr 9, 2019

MaxGekk commented Apr 6, 2019 •

edited

Loading

dongjoon-hyun commented Apr 7, 2019 •

edited

Loading

dongjoon-hyun Apr 7, 2019 •

edited

Loading

dongjoon-hyun Apr 7, 2019 •

edited

Loading

MaxGekk commented Apr 7, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun Apr 9, 2019 •

edited

Loading