Skip to content

[SPARK-27008][SQL] Support java.time.LocalDate as an external type of DateType - #23913

Closed
MaxGekk wants to merge 9 commits into
apache:masterfrom
MaxGekk:date-localdate
Closed

[SPARK-27008][SQL] Support java.time.LocalDate as an external type of DateType#23913
MaxGekk wants to merge 9 commits into
apache:masterfrom
MaxGekk:date-localdate

Conversation

@MaxGekk

@MaxGekkMaxGekk commented Feb 27, 2019•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the PR, I propose to add new Catalyst type converter for DateType. It should be able to convert java.time.LocalDate to/from DateType.

Main motivations for the changes:

  • Smoothly support Java 8 time API
  • Avoid inconsistency of calendars used inside of Spark 3.0 (Proleptic Gregorian calendar) and java.sql.Date (hybrid calendar - Julian + Gregorian).
  • Make conversion independent from current system timezone.

By default, Spark converts values of DateType to java.sql.Date instances but the SQL config spark.sql.datetime.java8API.enabled can change the behavior. If it is set to true, Spark uses java.time.LocalDate as external type for DateType.

How was this patch tested?

Added new testes to CatalystTypeConvertersSuite to check conversion of DateType to/from java.time.LocalDate, JavaUDFSuite/ UDFSuite to test usage of LocalDate type in Scala/Java UDFs.

@HeartSaVioR

HeartSaVioR commented Feb 27, 2019

Copy link
Copy Markdown
Contributor

Just FYI: in #23908 I'm proposing refactor of serializerFor so either this or #23908 may need to apply other's chance once other one gets merged first.

@MaxGekk

MaxGekk commented Feb 27, 2019

Copy link
Copy Markdown
MemberAuthor

@HeartSaVioR I see, I will rebase.

@HeartSaVioR

HeartSaVioR commented Feb 27, 2019

Copy link
Copy Markdown
Contributor

Yeah if your patch gets in first I'll apply the change too. Thanks!

@SparkQA

SparkQA commented Feb 28, 2019

Copy link
Copy Markdown

Test build #102835 has finished for PR 23913 at commit 3ab2851.

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

@MaxGekk

MaxGekk commented Feb 28, 2019

Copy link
Copy Markdown
MemberAuthor

@cloud-fan@HeartSaVioR Please, review the PR.

cloud-fan
cloud-fan reviewed Feb 28, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
cloud-fan
cloud-fan reviewed Feb 28, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala Outdated
cloud-fan
cloud-fan reviewed Feb 28, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala Outdated
@cloud-fan

cloud-fan commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

LGTM

@SparkQA

SparkQA commented Feb 28, 2019

Copy link
Copy Markdown

Test build #102862 has finished for PR 23913 at commit 86cd4a0.

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

@SparkQA

SparkQA commented Feb 28, 2019

Copy link
Copy Markdown

Test build #102859 has finished for PR 23913 at commit 7fbf674.

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

@MaxGekk

MaxGekk commented Feb 28, 2019

Copy link
Copy Markdown
MemberAuthor

jenkins, retest this, please

HeartSaVioR
HeartSaVioR approved these changes Feb 28, 2019
View reviewed changes

@HeartSaVioRHeartSaVioR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SparkQA

SparkQA commented Feb 28, 2019

Copy link
Copy Markdown

Test build #102870 has finished for PR 23913 at commit 86cd4a0.

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

@cloud-fan

cloud-fan commented Mar 1, 2019

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@HeartSaVioRHeartSaVioR mentioned this pull request Mar 1, 2019
Closed
@HyukjinKwon

HyukjinKwon commented Mar 5, 2019

Copy link
Copy Markdown
Member

Nice, +1!

@MaxGekkMaxGekk mentioned this pull request Mar 20, 2019
Closed
cloud-fan pushed a commit that referenced this pull request Mar 21, 2019
@MaxGekk@cloud-fan
[SPARK-27222][SQL] Support Instant and LocalDate in Literal.apply
0f4f816
## What changes were proposed in this pull request?
In the PR, I propose to extend `Literal.apply` to support constructing literals of `TimestampType` and `DateType` from `java.time.Instant` and `java.time.LocalDate`. The java classes have been already supported as external types for `TimestampType` and `DateType` by the PRs #23811 and #23913.
## How was this patch tested?
Added new tests to `LiteralExpressionSuite`.
Closes#24161 from MaxGekk/literal-instant-localdate.
Authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@MaxGekk
MaxGekk deleted the date-localdate branch September 18, 2019 15:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@HeartSaVioRHeartSaVioRHeartSaVioR approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MaxGekk@HeartSaVioR@SparkQA@cloud-fan@HyukjinKwon