Skip to content

[SPARK-26653][SQL] Use Proleptic Gregorian calendar in parsing JDBC lower/upper bounds - #23597

Closed
MaxGekk wants to merge 11 commits into
apache:masterfrom
MaxGekk:jdbc-parse-timestamp-bounds
Closed

[SPARK-26653][SQL] Use Proleptic Gregorian calendar in parsing JDBC lower/upper bounds#23597
MaxGekk wants to merge 11 commits into
apache:masterfrom
MaxGekk:jdbc-parse-timestamp-bounds

Conversation

@MaxGekk

@MaxGekkMaxGekk commented Jan 20, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the PR, I propose using of the stringToDate and stringToTimestamp methods in parsing JDBC lower/upper bounds of the partition column if it has DateType or TimestampType. Since those methods have been ported on Proleptic Gregorian calendar by #23512, the PR switches parsing of JDBC bounds of the partition column on the calendar as well.

How was this patch tested?

This was tested by JDBCSuite.

@MaxGekk

MaxGekk commented Jan 20, 2019

Copy link
Copy Markdown
MemberAuthor

@maropu Please, take a look at this PR.

@SparkQA

SparkQA commented Jan 20, 2019

Copy link
Copy Markdown

Test build #101449 has finished for PR 23597 at commit 0b61076.

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

maropu
maropu reviewed Jan 21, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala Outdated
@maropu

maropu commented Jan 21, 2019

Copy link
Copy Markdown
Member

LGTM except for one minor comment. cc: @gatorsmile

@SparkQA

SparkQA commented Jan 21, 2019

Copy link
Copy Markdown

Test build #101487 has finished for PR 23597 at commit 8bb4f3a.

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

gatorsmile
gatorsmile reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala Outdated
case _: NumericType => value.toLong
case DateType => parse(stringToDate).toLong
case TimestampType =>
parse(stringToTimestamp(_, getTimeZone(SQLConf.get.sessionLocalTimeZone)))

@gatorsmilegatorsmileJan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We decide to adjust the timestamp constants based on the user-specified local timezone?

@gatorsmilegatorsmileJan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @cloud-fan

@cloud-fancloud-fanJan 22, 2019

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.

Previously we call Timestamp.valueOf(value), which uses JVM local timezone. It seems to me that using Spark session timezone is better.

@cloud-fancloud-fanJan 22, 2019

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.

Actually we have to do it. This is a followup of #23391 , which changed how we turn the timestamp boundaries to string. Here we change hoow we turn string to timestamp.

@gatorsmilegatorsmileJan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a behavior change. We need to clearly document which inputs start respecting our Spark local session timezone?

@HyukjinKwonHyukjinKwonJan 22, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about we mention something like .. all string -> timestamp will respect Session timezone, JDBC lower/upper bounds, blabla, ..., and java 8 time will be consistently used across code base .. after the sub-tasks in the umbrella are resolved?

@cloud-fancloud-fanJan 22, 2019

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.

I agree that we should improve the migration guide. Switching to Proleptic Gregorian calendar is a behavior change to many places, it's better we can list all of them in the migration guide.

cloud-fan
cloud-fan reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala Outdated
case _: NumericType => value.toLong
case DateType => DateTimeUtils.fromJavaDate(Date.valueOf(value)).toLong
case TimestampType => DateTimeUtils.fromJavaTimestamp(Timestamp.valueOf(value))
private def toInternalBoundValue(value: String, columnType: DataType): Long = {

@cloud-fancloud-fanJan 22, 2019

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.

we should pass in the timezone id, just like what we did for toBoundValueInWhereClause

@MaxGekkMaxGekkJan 22, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

done

@SparkQA

SparkQA commented Jan 22, 2019

Copy link
Copy Markdown

Test build #101524 has finished for PR 23597 at commit a0b23ed.

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

HyukjinKwon
HyukjinKwon reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala Outdated
HyukjinKwon
HyukjinKwon reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala Outdated
HyukjinKwon
HyukjinKwon reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala Outdated
HyukjinKwon
HyukjinKwon reviewed Jan 22, 2019
View reviewed changes
Comment threaddocs/sql-migration-guide-upgrade.md
@SparkQA

SparkQA commented Jan 22, 2019

Copy link
Copy Markdown

Test build #101545 has finished for PR 23597 at commit 4dc4a2a.

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

@cloud-fan

cloud-fan commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

LGTM, let's resole the conflict.

@SparkQA

SparkQA commented Jan 23, 2019

Copy link
Copy Markdown

Test build #101579 has finished for PR 23597 at commit af20442.

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

@cloud-fan

cloud-fan commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

thanks, merging to master!

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@MaxGekkJackey Lee
[SPARK-26653][SQL] Use Proleptic Gregorian calendar in parsing JDBC l…
7ce476d
…ower/upper bounds
## What changes were proposed in this pull request?
In the PR, I propose using of the `stringToDate` and `stringToTimestamp` methods in parsing JDBC lower/upper bounds of the partition column if it has `DateType` or `TimestampType`. Since those methods have been ported on Proleptic Gregorian calendar by apache#23512, the PR switches parsing of JDBC bounds of the partition column on the calendar as well.
## How was this patch tested?
This was tested by `JDBCSuite`.
Closesapache#23597 from MaxGekk/jdbc-parse-timestamp-bounds.
Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com>
Co-authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@MaxGekk
MaxGekk deleted the jdbc-parse-timestamp-bounds branch August 17, 2019 13:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropumaropumaropu left review comments
@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes
@gatorsmilegatorsmilegatorsmile left review comments
@cloud-fancloud-fancloud-fan 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.

6 participants

@MaxGekk@SparkQA@maropu@cloud-fan@HyukjinKwon@gatorsmile