[SPARK-31076][SQL][FOLLOWUP] Incapsulate date rebasing to `DaysWritable` #27890

MaxGekk · 2020-03-12T10:22:43Z

What changes were proposed in this pull request?

Move the code related to days rebasing from/to Julian calendar from HiveInspectors to new class DaysWritable.

Why are the changes needed?

To improve maintainability of the HiveInspectors trait which is already pretty complex.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By HiveOrcHadoopFsRelationSuite.

SparkQA · 2020-03-12T12:08:37Z

Test build #119713 has finished for PR 27890 at commit 2000387.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DaysWritable(var gregorianDays: Int = 0) extends DateWritable

SparkQA · 2020-03-12T18:56:34Z

Test build #119720 has finished for PR 27890 at commit af4549e.

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

MaxGekk · 2020-03-12T19:50:32Z

jenkins, retest this, please

SparkQA · 2020-03-12T22:13:54Z

Test build #119725 has finished for PR 27890 at commit af4549e.

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

MaxGekk · 2020-03-13T06:39:32Z

@cloud-fan @HyukjinKwon Please, take a look at this.

cloud-fan · 2020-03-13T06:56:35Z

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

-              } else {
-                daysSinceEpoch
-              }
+              x.getPrimitiveWritableObject(data).getDays


What if the returned DateWritable here is not our custom version?

You are right, DateWritable can come here. I have fixed that by adding new constructor to DaysWritable to construct its instances from DateWritable.

SparkQA · 2020-03-13T10:57:15Z

Test build #119749 has finished for PR 27890 at commit a017fbe.

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

cloud-fan · 2020-03-13T11:03:54Z

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

+    })
+  }
+
+  override def getDays: Int = gregorianDays


should we return julianDays here to conform to the hive API?

cloud-fan · 2020-03-13T11:05:24Z

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

+    // Proleptic Gregorian calendar: 1582-01-01 -> -141714
+    // Julian calendar: 1582-01-01 -> -141704
+    // The code below converts -141714 to -141704.
+    val writeDays = DaysWritable.rebaseGregorianToJulianDays(gregorianDays)


shall we cache the julianDays?

SparkQA · 2020-03-13T15:35:06Z

Test build #119761 has finished for PR 27890 at commit f606899.

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

cloud-fan · 2020-03-16T09:06:31Z

thanks, merging to master/3.0!

HyukjinKwon · 2020-03-16T09:10:15Z

+1 Looks good to me too

### What changes were proposed in this pull request? Move the code related to days rebasing from/to Julian calendar from `HiveInspectors` to new class `DaysWritable`. ### Why are the changes needed? To improve maintainability of the `HiveInspectors` trait which is already pretty complex. ### Does this PR introduce any user-facing change? No ### How was this patch tested? By `HiveOrcHadoopFsRelationSuite`. Closes #27890 from MaxGekk/replace-DateWritable-by-DaysWritable. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 57854c7) Signed-off-by: Wenchen Fan <[email protected]>

… for ORC datasource ### What changes were proposed in this pull request? This PR (SPARK-31238) aims the followings. 1. Modified ORC Vectorized Reader, in particular, OrcColumnVector v1.2 and v2.3. After the changes, it uses `DateTimeUtils. rebaseJulianToGregorianDays()` added by #27915 . The method performs rebasing days from the hybrid calendar (Julian + Gregorian) to Proleptic Gregorian calendar. It builds a local date in the original calendar, extracts date fields `year`, `month` and `day` from the local date, and builds another local date in the target calendar. After that, it calculates days from the epoch `1970-01-01` for the resulted local date. 2. Introduced rebasing dates while saving ORC files, in particular, I modified `OrcShimUtils. getDateWritable` v1.2 and v2.3, and returned `DaysWritable` instead of Hive's `DateWritable`. The `DaysWritable` class was added by the PR #27890 (and fixed by #27962). I moved `DaysWritable` from `sql/hive` to `sql/core` to re-use it in ORC datasource. ### Why are the changes needed? For the backward compatibility with Spark 2.4 and earlier versions. The changes allow users to read dates/timestamps saved by previous version, and get the same result. ### Does this PR introduce any user-facing change? Yes. Before the changes, loading the date `1200-01-01` saved by Spark 2.4.5 returns the following: ```scala scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-08| +----------+ ``` After the changes ```scala scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-01| +----------+ ``` ### How was this patch tested? - By running `OrcSourceSuite` and `HiveOrcSourceSuite`. - Add new test `SPARK-31238: compatibility with Spark 2.4 in reading dates` to `OrcSuite` which reads an ORC file saved by Spark 2.4.5 via the commands: ```shell $ export TZ="America/Los_Angeles" ``` ```scala scala> sql("select cast('1200-01-01' as date) dt").write.mode("overwrite").orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc") scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-01| +----------+ ``` - Add round trip test `SPARK-31238: rebasing dates in write`. The test `SPARK-31238: compatibility with Spark 2.4 in reading dates` confirms rebasing in read. So, we can check rebasing in write. Closes #28016 from MaxGekk/rebase-date-orc. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

… for ORC datasource ### What changes were proposed in this pull request? This PR (SPARK-31238) aims the followings. 1. Modified ORC Vectorized Reader, in particular, OrcColumnVector v1.2 and v2.3. After the changes, it uses `DateTimeUtils. rebaseJulianToGregorianDays()` added by #27915 . The method performs rebasing days from the hybrid calendar (Julian + Gregorian) to Proleptic Gregorian calendar. It builds a local date in the original calendar, extracts date fields `year`, `month` and `day` from the local date, and builds another local date in the target calendar. After that, it calculates days from the epoch `1970-01-01` for the resulted local date. 2. Introduced rebasing dates while saving ORC files, in particular, I modified `OrcShimUtils. getDateWritable` v1.2 and v2.3, and returned `DaysWritable` instead of Hive's `DateWritable`. The `DaysWritable` class was added by the PR #27890 (and fixed by #27962). I moved `DaysWritable` from `sql/hive` to `sql/core` to re-use it in ORC datasource. ### Why are the changes needed? For the backward compatibility with Spark 2.4 and earlier versions. The changes allow users to read dates/timestamps saved by previous version, and get the same result. ### Does this PR introduce any user-facing change? Yes. Before the changes, loading the date `1200-01-01` saved by Spark 2.4.5 returns the following: ```scala scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-08| +----------+ ``` After the changes ```scala scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-01| +----------+ ``` ### How was this patch tested? - By running `OrcSourceSuite` and `HiveOrcSourceSuite`. - Add new test `SPARK-31238: compatibility with Spark 2.4 in reading dates` to `OrcSuite` which reads an ORC file saved by Spark 2.4.5 via the commands: ```shell $ export TZ="America/Los_Angeles" ``` ```scala scala> sql("select cast('1200-01-01' as date) dt").write.mode("overwrite").orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc") scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-01| +----------+ ``` - Add round trip test `SPARK-31238: rebasing dates in write`. The test `SPARK-31238: compatibility with Spark 2.4 in reading dates` confirms rebasing in read. So, we can check rebasing in write. Closes #28016 from MaxGekk/rebase-date-orc. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit d72ec85) Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? Move the code related to days rebasing from/to Julian calendar from `HiveInspectors` to new class `DaysWritable`. ### Why are the changes needed? To improve maintainability of the `HiveInspectors` trait which is already pretty complex. ### Does this PR introduce any user-facing change? No ### How was this patch tested? By `HiveOrcHadoopFsRelationSuite`. Closes apache#27890 from MaxGekk/replace-DateWritable-by-DaysWritable. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… for ORC datasource ### What changes were proposed in this pull request? This PR (SPARK-31238) aims the followings. 1. Modified ORC Vectorized Reader, in particular, OrcColumnVector v1.2 and v2.3. After the changes, it uses `DateTimeUtils. rebaseJulianToGregorianDays()` added by apache#27915 . The method performs rebasing days from the hybrid calendar (Julian + Gregorian) to Proleptic Gregorian calendar. It builds a local date in the original calendar, extracts date fields `year`, `month` and `day` from the local date, and builds another local date in the target calendar. After that, it calculates days from the epoch `1970-01-01` for the resulted local date. 2. Introduced rebasing dates while saving ORC files, in particular, I modified `OrcShimUtils. getDateWritable` v1.2 and v2.3, and returned `DaysWritable` instead of Hive's `DateWritable`. The `DaysWritable` class was added by the PR apache#27890 (and fixed by apache#27962). I moved `DaysWritable` from `sql/hive` to `sql/core` to re-use it in ORC datasource. ### Why are the changes needed? For the backward compatibility with Spark 2.4 and earlier versions. The changes allow users to read dates/timestamps saved by previous version, and get the same result. ### Does this PR introduce any user-facing change? Yes. Before the changes, loading the date `1200-01-01` saved by Spark 2.4.5 returns the following: ```scala scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-08| +----------+ ``` After the changes ```scala scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-01| +----------+ ``` ### How was this patch tested? - By running `OrcSourceSuite` and `HiveOrcSourceSuite`. - Add new test `SPARK-31238: compatibility with Spark 2.4 in reading dates` to `OrcSuite` which reads an ORC file saved by Spark 2.4.5 via the commands: ```shell $ export TZ="America/Los_Angeles" ``` ```scala scala> sql("select cast('1200-01-01' as date) dt").write.mode("overwrite").orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc") scala> spark.read.orc("/Users/maxim/tmp/before_1582/2_4_5_date_orc").show(false) +----------+ |dt | +----------+ |1200-01-01| +----------+ ``` - Add round trip test `SPARK-31238: rebasing dates in write`. The test `SPARK-31238: compatibility with Spark 2.4 in reading dates` confirms rebasing in read. So, we can check rebasing in write. Closes apache#28016 from MaxGekk/rebase-date-orc. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Add DaysWritable

2000387

MaxGekk added 2 commits March 12, 2020 19:12

Make DaysWritable private

3ef4265

Fix "HiveSerDeReadWriteSuite.Read/Write Hive PARQUET serde table"

af4549e

cloud-fan reviewed Mar 13, 2020

View reviewed changes

Wrap DateWritable

a017fbe

cloud-fan reviewed Mar 13, 2020

View reviewed changes

Address Wenchen's review comments

f606899

cloud-fan approved these changes Mar 13, 2020

View reviewed changes

cloud-fan closed this in 57854c7 Mar 16, 2020

MaxGekk mentioned this pull request Mar 25, 2020

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

Closed

MaxGekk deleted the replace-DateWritable-by-DaysWritable branch June 5, 2020 19:46

[SPARK-31076][SQL][FOLLOWUP] Incapsulate date rebasing to DaysWritable #27890

[SPARK-31076][SQL][FOLLOWUP] Incapsulate date rebasing to DaysWritable #27890

Uh oh!

Conversation

MaxGekk commented Mar 12, 2020

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 Mar 12, 2020

Uh oh!

SparkQA commented Mar 12, 2020

Uh oh!

MaxGekk commented Mar 12, 2020

Uh oh!

SparkQA commented Mar 12, 2020

Uh oh!

MaxGekk commented Mar 13, 2020

Uh oh!

cloud-fan Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 13, 2020

Uh oh!

cloud-fan Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 13, 2020

Uh oh!

cloud-fan commented Mar 16, 2020

Uh oh!

HyukjinKwon commented Mar 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-31076][SQL][FOLLOWUP] Incapsulate date rebasing to `DaysWritable` #27890

[SPARK-31076][SQL][FOLLOWUP] Incapsulate date rebasing to `DaysWritable` #27890