[SPARK-31076][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time #27807

MaxGekk · 2020-03-05T11:00:40Z

What changes were proposed in this pull request?

In the PR, I propose to change conversion of java.sql.Timestamp/Date values to/from internal values of Catalyst's TimestampType/DateType before cutover day 1582-10-15 of Gregorian calendar. I propose to construct local date-time from microseconds/days since the epoch. Take each date-time component year, month, day, hour, minute, second and second fraction, and construct java.sql.Timestamp/Date using the extracted components.

Why are the changes needed?

This will rebase underlying time/date offset in the way that collected java.sql.Timestamp/Date values will have the same local time-date component as the original values in Gregorian calendar.

Here is the example which demonstrates the issue:

scala> sql("select date '1100-10-10'").collect()
res1: Array[org.apache.spark.sql.Row] = Array([1100-10-03])

Does this PR introduce any user-facing change?

Yes, after the changes:

scala> sql("select date '1100-10-10'").collect()
res0: Array[org.apache.spark.sql.Row] = Array([1100-10-10])

How was this patch tested?

By running DateTimeUtilsSuite, DateFunctionsSuite and DateExpressionsSuite.

SparkQA · 2020-03-05T11:54:56Z

Test build #119382 has finished for PR 27807 at commit b9532d7.

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

…ian cal

SparkQA · 2020-03-05T20:10:18Z

Test build #119411 has finished for PR 27807 at commit 503e3f0.

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

SparkQA · 2020-03-05T21:30:39Z

Test build #119418 has finished for PR 27807 at commit 4a13df3.

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

SparkQA · 2020-03-05T23:23:21Z

Test build #119419 has finished for PR 27807 at commit 120db04.

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

MaxGekk · 2020-03-06T07:58:54Z

@cloud-fan The rebasing cause a few issues. For example, we cannot use TimestampFormatter/DateFormatter to format java.sql.Timestamp/Date anymore:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/HiveResult.scala

Line 74 in 1fac06c

timestampFormatter.format(DateTimeUtils.fromJavaTimestamp(t))

To format rebased Java Timestamp/Date instances before 1582, need to use Julian based formatter - SimpleDateFormat.

…stamp-before-1582

SparkQA · 2020-03-06T19:42:04Z

Test build #119471 has finished for PR 27807 at commit d2e3a27.

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

MaxGekk · 2020-03-06T20:06:17Z

@cloud-fan @juliuszsompolski Please, take a look at the PR if you have time.

sql/core/src/test/resources/sql-tests/results/postgreSQL/date.sql.out

sql/core/src/test/scala/org/apache/spark/sql/execution/HiveResultSuite.scala

SparkQA · 2020-03-06T23:51:38Z

Test build #119484 has finished for PR 27807 at commit 8e83e1f.

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

SparkQA · 2020-03-07T00:26:07Z

Test build #119486 has finished for PR 27807 at commit fe8c1fb.

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

SparkQA · 2020-03-07T09:31:50Z

Test build #119511 has finished for PR 27807 at commit f4ced32.

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

MaxGekk · 2020-03-07T10:33:39Z

sql/core/v1.2/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcColumnVector.java

@@ -136,7 +138,9 @@ public int getInt(int rowId) {
  public long getLong(int rowId) {
    int index = getRowIndex(rowId);
    if (isTimestamp) {
-      return timestampData.time[index] * 1000 + timestampData.nanos[index] / 1000 % 1000;
+      Timestamp ts = new Timestamp(timestampData.time[index]);


This is slightly orthogonal bug fix. @dongjoon-hyun FYI

does parquet have the same issue?

Hi, @MaxGekk . Could you elaborate a little more what was the existing bug here?

Does this PR add a test coverage for this vectorized code path change?

can we add a round trip test in OrcQuerySuite to read/write date before 1582?

Could you elaborate a little more what was the existing bug here?

ORC writer uses DateTimeUtils.toJavaTimestamp, see

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcSerializer.scala

Lines 144 to 148 in 300ec1a

case TimestampType => (getter, ordinal) =>

val ts = DateTimeUtils.toJavaTimestamp(getter.getLong(ordinal))

val result = new OrcTimestamp(ts.getTime)

result.setNanos(ts.getNanos)

result

but here we don't use opposite function DateTimeUtils.fromJavaTimestamp.

And the replaced hand-written code is not equal to fromJavaTimestamp

Does this PR add a test coverage for this vectorized code path change?

I have to fix this place due test failures, see https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/119486/testReport/

The changes covered by the round trip test

spark/sql/hive/src/test/scala/org/apache/spark/sql/sources/HadoopFsRelationTest.scala

Line 122 in 7782b61

test(s"test all data types") {

which runs by OrcHadoopFsRelationSuite for vectorized code path.

can we add a round trip test in OrcQuerySuite to read/write date before 1582?

@cloud-fan The test I pointed out above generates random dates/timestamps before 1582 with high probability.

Got it. Thanks.

MaxGekk · 2020-03-10T05:16:35Z

sql/core/src/test/scala/org/apache/spark/sql/execution/HiveResultSuite.scala

@@ -36,8 +36,8 @@ class HiveResultSuite extends SharedSparkSession {
  test("timestamp formatting in hive result") {
    val timestamps = Seq(
      "2018-12-28 01:02:03",
-      "1582-10-13 01:02:03",
-      "1582-10-14 01:02:03",
+      "1582-10-03 01:02:03",


Conversions of timestamps in the range 1582-10-04 - 1582-10-15 is implementation specific because of calendar switching.

cloud-fan · 2020-03-10T05:41:14Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala

@@ -618,7 +619,19 @@ private[hive] trait HiveInspectors {
        case x: DateObjectInspector if x.preferWritable() =>
          data: Any => {
            if (data != null) {
-              DateTimeUtils.fromJavaDate(x.getPrimitiveWritableObject(data).get())
+              val millis = Math.multiplyExact(


can we add some comments to explain why we need to do it?

This reverts commit 52cdd20.

cloud-fan · 2020-03-10T11:07:05Z

sql/core/v2.3/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcColumnVector.java

@@ -136,7 +138,9 @@ public int getInt(int rowId) {
  public long getLong(int rowId) {
    int index = getRowIndex(rowId);
    if (isTimestamp) {
-      return timestampData.time[index] * 1000 + timestampData.nanos[index] / 1000 % 1000;


I took a look at ORC type spec but it doesn't mention the calendar. The physical timestamp type looks very similar to Timestamp, so this looks correct to me.

How about the write side?

The write side uses toJavaTimestamp already:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcSerializer.scala

Lines 144 to 148 in 300ec1a

case TimestampType => (getter, ordinal) =>

val ts = DateTimeUtils.toJavaTimestamp(getter.getLong(ordinal))

val result = new OrcTimestamp(ts.getTime)

result.setNanos(ts.getNanos)

result

. So after this PR, it will do rebasing automatically

Yes, @cloud-fan . Apache ORC inherited Apache Hive's original design.

https://github.com/apache/hive/blob/master/storage-api/src/java/org/apache/hadoop/hive/ql/exec/vector/TimestampColumnVector.java#L46-L50

MaxGekk · 2020-03-10T11:30:52Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala

+      // For example:
+      // Proleptic Gregorian calendar: 1582-01-01 -> -141714
+      // Julian calendar: 1582-01-01 -> -141704
+      // The code below converts -141714 to -141704.


Gregorian year is shorter than Julian year: 365.2425 days vs 365.25 days, so the same local date in Gregorian calendar requires less days in Julian calendar.

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

SparkQA · 2020-03-10T12:47:28Z

Test build #119609 has finished for PR 27807 at commit 5f5fdd6.

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

SparkQA · 2020-03-10T14:17:13Z

Test build #119616 has finished for PR 27807 at commit da8de7e.

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

cloud-fan · 2020-03-11T03:48:10Z

sql/core/v2.3/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcColumnVector.java

-      return timestampData.time[index] * 1000 + timestampData.nanos[index] / 1000 % 1000;
+      Timestamp ts = new Timestamp(timestampData.time[index]);
+      ts.setNanos(timestampData.nanos[index]);
+      return DateTimeUtils.fromJavaTimestamp(ts);


We can use a simpler API: DateTimeUtils.fromJavaTimestamp(timestampData.asScratchTimestamp(index))

Under the hood, this re-uses the Timestamp object and should be more efficient.

SparkQA · 2020-03-11T07:05:02Z

Test build #119652 has finished for PR 27807 at commit 407cc1f.

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

MaxGekk · 2020-03-11T07:37:18Z

jenkins, retest this, please

cloud-fan · 2020-03-11T07:39:21Z

retest this please

SparkQA · 2020-03-11T12:28:43Z

Test build #119657 has finished for PR 27807 at commit 407cc1f.

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

cloud-fan · 2020-03-11T12:54:10Z

thanks, merging to master/3.0!

…estamp via local date-time In the PR, I propose to change conversion of java.sql.Timestamp/Date values to/from internal values of Catalyst's TimestampType/DateType before cutover day `1582-10-15` of Gregorian calendar. I propose to construct local date-time from microseconds/days since the epoch. Take each date-time component `year`, `month`, `day`, `hour`, `minute`, `second` and `second fraction`, and construct java.sql.Timestamp/Date using the extracted components. This will rebase underlying time/date offset in the way that collected java.sql.Timestamp/Date values will have the same local time-date component as the original values in Gregorian calendar. Here is the example which demonstrates the issue: ```sql scala> sql("select date '1100-10-10'").collect() res1: Array[org.apache.spark.sql.Row] = Array([1100-10-03]) ``` Yes, after the changes: ```sql scala> sql("select date '1100-10-10'").collect() res0: Array[org.apache.spark.sql.Row] = Array([1100-10-10]) ``` By running `DateTimeUtilsSuite`, `DateFunctionsSuite` and `DateExpressionsSuite`. Closes #27807 from MaxGekk/rebase-timestamp-before-1582. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 3d3e366) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

dongjoon-hyun · 2020-03-11T16:29:41Z

Thank you, @MaxGekk and @cloud-fan .
I marked this as a correctness issue according to @MaxGekk 's comment, #27807 (comment) .

dongjoon-hyun · 2020-03-11T16:37:29Z

BTW, @MaxGekk .
Is this PR only correctness issue in 3.0.0?
Since we touched old codes, could you make another PR for branch-2.4 for the followings with one small specific UT?

[SPARK-31076][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time #27807 (comment)
[SPARK-31076][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time #27807 (comment)

cloud-fan · 2020-03-19T04:36:35Z

It's only a bug in 3.0 as we switch the calendar.

…asource ### What changes were proposed in this pull request? In the PR, I propose to add new benchmark `DateTimeRebaseBenchmark` which should measure the performance of rebasing of dates/timestamps from/to to the hybrid calendar (Julian+Gregorian) to/from Proleptic Gregorian calendar: 1. In write, it saves separately dates and timestamps before and after 1582 year w/ and w/o rebasing. 2. In read, it loads previously saved parquet files by vectorized reader and by regular reader. Here is the summary of benchmarking: - Saving timestamps is **~6 times slower** - Loading timestamps w/ vectorized **off** is **~4 times slower** - Loading timestamps w/ vectorized **on** is **~10 times slower** ### Why are the changes needed? To know the impact of date-time rebasing introduced by #27915, #27953, #27807. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Run the `DateTimeRebaseBenchmark` benchmark using Amazon EC2: | 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 | OpenJDK8/11 | Closes #28057 from MaxGekk/rebase-bechmark. Lead-authored-by: Maxim Gekk <max.gekk@gmail.com> Co-authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…asource ### What changes were proposed in this pull request? In the PR, I propose to add new benchmark `DateTimeRebaseBenchmark` which should measure the performance of rebasing of dates/timestamps from/to to the hybrid calendar (Julian+Gregorian) to/from Proleptic Gregorian calendar: 1. In write, it saves separately dates and timestamps before and after 1582 year w/ and w/o rebasing. 2. In read, it loads previously saved parquet files by vectorized reader and by regular reader. Here is the summary of benchmarking: - Saving timestamps is **~6 times slower** - Loading timestamps w/ vectorized **off** is **~4 times slower** - Loading timestamps w/ vectorized **on** is **~10 times slower** ### Why are the changes needed? To know the impact of date-time rebasing introduced by #27915, #27953, #27807. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Run the `DateTimeRebaseBenchmark` benchmark using Amazon EC2: | 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 | OpenJDK8/11 | Closes #28057 from MaxGekk/rebase-bechmark. Lead-authored-by: Maxim Gekk <max.gekk@gmail.com> Co-authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit a1dbcd1) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…estamp via local date-time ### What changes were proposed in this pull request? In the PR, I propose to change conversion of java.sql.Timestamp/Date values to/from internal values of Catalyst's TimestampType/DateType before cutover day `1582-10-15` of Gregorian calendar. I propose to construct local date-time from microseconds/days since the epoch. Take each date-time component `year`, `month`, `day`, `hour`, `minute`, `second` and `second fraction`, and construct java.sql.Timestamp/Date using the extracted components. ### Why are the changes needed? This will rebase underlying time/date offset in the way that collected java.sql.Timestamp/Date values will have the same local time-date component as the original values in Gregorian calendar. Here is the example which demonstrates the issue: ```sql scala> sql("select date '1100-10-10'").collect() res1: Array[org.apache.spark.sql.Row] = Array([1100-10-03]) ``` ### Does this PR introduce any user-facing change? Yes, after the changes: ```sql scala> sql("select date '1100-10-10'").collect() res0: Array[org.apache.spark.sql.Row] = Array([1100-10-10]) ``` ### How was this patch tested? By running `DateTimeUtilsSuite`, `DateFunctionsSuite` and `DateExpressionsSuite`. Closes apache#27807 from MaxGekk/rebase-timestamp-before-1582. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…asource ### What changes were proposed in this pull request? In the PR, I propose to add new benchmark `DateTimeRebaseBenchmark` which should measure the performance of rebasing of dates/timestamps from/to to the hybrid calendar (Julian+Gregorian) to/from Proleptic Gregorian calendar: 1. In write, it saves separately dates and timestamps before and after 1582 year w/ and w/o rebasing. 2. In read, it loads previously saved parquet files by vectorized reader and by regular reader. Here is the summary of benchmarking: - Saving timestamps is **~6 times slower** - Loading timestamps w/ vectorized **off** is **~4 times slower** - Loading timestamps w/ vectorized **on** is **~10 times slower** ### Why are the changes needed? To know the impact of date-time rebasing introduced by apache#27915, apache#27953, apache#27807. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Run the `DateTimeRebaseBenchmark` benchmark using Amazon EC2: | 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 | OpenJDK8/11 | Closes apache#28057 from MaxGekk/rebase-bechmark. Lead-authored-by: Maxim Gekk <max.gekk@gmail.com> Co-authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

MaxGekk added 3 commits March 5, 2020 13:56

Convert via local date-time

88cf01c

Change tests

c60a255

Uncomment tests

b9532d7

MaxGekk added 2 commits March 5, 2020 21:32

Disable a test

503e3f0

Convert via local date-time only date/timestamp before cutover gregor…

4a13df3

…ian cal

Disable "test all data types"

120db04

MaxGekk added 4 commits March 6, 2020 11:22

Merge remote-tracking branch 'remotes/origin/master' into rebase-time…

1edac2a

…stamp-before-1582

Change year in HiveResultSuite

2ecaf1c

Re-gen date.sql.out

d2e3a27

Enable "test all data types"

8e83e1f

MaxGekk changed the title ~~[WIP][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time~~ [SPARK-31076][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time Mar 6, 2020

MaxGekk commented Mar 6, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/date.sql.out Outdated Show resolved Hide resolved

MaxGekk commented Mar 6, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/HiveResultSuite.scala Outdated Show resolved Hide resolved

MaxGekk added 2 commits March 6, 2020 23:16

Revert some values in a test back

bfb6535

Make the test independent of implementation

fe8c1fb

MaxGekk added 3 commits March 7, 2020 11:08

Bug fix in OrcColumnVector

6c892da

Bug fix in OrcColumnVector v1.2

f4ced32

Bug fix in HiveInspectors

00003dd

Bug fix date ser in HiveInspectors

37bd181

MaxGekk commented Mar 7, 2020

View reviewed changes

MaxGekk commented Mar 10, 2020

View reviewed changes

cloud-fan reviewed Mar 10, 2020

View reviewed changes

MaxGekk added 2 commits March 10, 2020 08:37

Revert "Remove special handling before 1582"

5f5fdd6

This reverts commit 52cdd20.

Put conversion code to separate methods

da8de7e

cloud-fan reviewed Mar 10, 2020

View reviewed changes

MaxGekk commented Mar 10, 2020

View reviewed changes

cloud-fan approved these changes Mar 10, 2020

View reviewed changes

HyukjinKwon reviewed Mar 10, 2020

View reviewed changes

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

cloud-fan reviewed Mar 11, 2020

View reviewed changes

Use asScratchTimestamp

407cc1f

HyukjinKwon approved these changes Mar 11, 2020

View reviewed changes

cloud-fan closed this in 3d3e366 Mar 11, 2020

This was referenced Mar 25, 2020

[SPARK-31238][SQL] Rebase dates to/from Julian calendar in write/read for ORC datasource #28016

Closed

[SPARK-31296][SQL][TESTS] Benchmark date-time rebasing in Parquet datasource #28057

Closed

MaxGekk deleted the rebase-timestamp-before-1582 branch June 5, 2020 19:46

	case TimestampType => (getter, ordinal) =>
	val ts = DateTimeUtils.toJavaTimestamp(getter.getLong(ordinal))
	val result = new OrcTimestamp(ts.getTime)
	result.setNanos(ts.getNanos)
	result

[SPARK-31076][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time #27807

[SPARK-31076][SQL] Convert Catalyst's DATE/TIMESTAMP to Java Date/Timestamp via local date-time #27807

Conversation

MaxGekk commented Mar 5, 2020 • edited Loading

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?

SparkQA commented Mar 5, 2020

SparkQA commented Mar 5, 2020

SparkQA commented Mar 5, 2020

SparkQA commented Mar 5, 2020

MaxGekk commented Mar 6, 2020

SparkQA commented Mar 6, 2020

MaxGekk commented Mar 6, 2020

SparkQA commented Mar 6, 2020

SparkQA commented Mar 7, 2020

SparkQA commented Mar 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Mar 10, 2020

SparkQA commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 11, 2020

MaxGekk commented Mar 11, 2020

cloud-fan commented Mar 11, 2020

SparkQA commented Mar 11, 2020

cloud-fan commented Mar 11, 2020

dongjoon-hyun commented Mar 11, 2020

dongjoon-hyun commented Mar 11, 2020 • edited Loading

cloud-fan commented Mar 19, 2020

MaxGekk commented Mar 5, 2020 •

edited

Loading

MaxGekk Mar 10, 2020 •

edited

Loading

dongjoon-hyun commented Mar 11, 2020 •

edited

Loading