[SPARK-27252][SQL] Make current_date() independent from time zones #24185

MaxGekk · 2019-03-22T21:00:02Z

What changes were proposed in this pull request?

This makes the CurrentDate expression and current_date function independent from time zone settings. New result is number of days since epoch in UTC time zone. Previously, Spark shifted the current date (in UTC time zone) according the session time zone which violets definition of DateType - number of days since epoch (which is an absolute point in time, midnight of Jan 1 1970 in UTC time).

The changes makes CurrentDate consistent to CurrentTimestamp which is independent from time zone too.

How was this patch tested?

The changes were tested by existing test suites like DateExpressionsSuite.

docs/sql-migration-guide-upgrade.md

SparkQA · 2019-03-23T00:32:06Z

Test build #103837 has finished for PR 24185 at commit 7ff29aa.

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

MaxGekk · 2019-03-23T12:41:50Z

jenkins, retest this, please

SparkQA · 2019-03-23T22:19:16Z

Test build #103841 has finished for PR 24185 at commit 7ff29aa.

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

SparkQA · 2019-03-23T22:54:15Z

Test build #103851 has finished for PR 24185 at commit ae918cb.

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

viirya · 2019-03-24T06:08:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

 * There is no code generation since this expression should get constant folded by the optimizer.
 */
 @ExpressionDescription(
  usage = "_FUNC_() - Returns the current date at the start of query evaluation.",


Is it better to specify the time zone in this usage text?

Updated docs (and comments) of the expression and the current_date() function.

SparkQA · 2019-03-24T17:02:40Z

Test build #103872 has finished for PR 24185 at commit 11afacc.

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

MaxGekk · 2019-03-24T17:46:08Z

@srowen @cloud-fan WDYT of the changes?

srowen · 2019-03-24T19:13:35Z

I see your point, in that a date by itself without a time shouldn't be relative to a time zone.current_date seems like it should reflect the date in the session time zone though. I think of it as the date portion of the current time, and time is relative to a time zone. Is this matching some other DB behavior?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala

MaxGekk · 2019-03-24T22:43:34Z

@cloud-fan Just in case, are you ok with the changes in general?

cloud-fan · 2019-03-24T22:45:18Z

According to the SQL standard, timestamp is timezone dependent while date is not. For current_date, it seems the timezone shifting is reasonable. E.g. when I run current_date in a database, I'd expect the result matches my local calendar. Can we check other mainstreaming databases?

MaxGekk · 2019-03-26T11:06:49Z

timestamp is timezone dependent while date is not. For current_date, it seems the timezone shifting is reasonable.

The current_date() returns a value of DateType which is time zone independent, and actually stores number of days since epoch (in UTC time zone). And Spark assumes that in many places. How can it be shifted by time zone offset?

when I run current_date in a database, I'd expect the result matches my local calendar.

I would guess you speak about textual representation of current_date result? If so, conversion DateType -> StringType should care about time zone offset which what the date_format() function does.

srowen · 2019-03-26T12:07:21Z

Maybe I'm confused; DateType isn't logically time-zone-dependent but it's stored in a way that is (time since epoch)? OK this makes more sense. So if I later render this value according to my session time zone, it would already also 'correct' for the timezone?

If that's the case then we just have a bug, sure. After the fix, current_date() formatted for my current time zone would return what I expect today is? would be the current date from midnight to midnight in my timezone?

srowen · 2019-03-26T14:09:03Z

Your argument here is compelling: #24181 (comment)

So, basically we already convert from absolute time to date w.r.t. UTC elsewhere?

MaxGekk · 2019-03-26T14:32:11Z

what the date_format() function does.
... Your argument here is compelling: #24181 (comment)

Yah, I missed that date_format() has the same behavior as other date renders in Spark. Fortunately everything has been already consistent from this side :-)

cloud-fan · 2019-03-26T15:21:38Z

I would guess you speak about textual representation of current_date result

Ah yes, then +1 for this PR

srowen · 2019-03-28T16:21:17Z

@cloud-fan sounds like we're good with this; what do you think about your comment at https://github.com/apache/spark/pull/24185/files#r269162289 ?

MaxGekk · 2019-03-28T16:30:02Z

@cloud-fan sounds like we're good with this; what do you think about your comment at https://github.com/apache/spark/pull/24185/files#r269162289 ?

Sorry, I forget about the comment. I am about to change this.

SparkQA · 2019-03-28T19:46:56Z

Test build #104059 has finished for PR 24185 at commit ec1f7e8.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2019-03-28T20:21:02Z

Just to make consequences of the changes clear:

current_date() will always produce current date as a value of DateType in the UTC time zone.
All available conversions of DateType to StringType is time zone independent in Spark at the moment. See the comment: [SPARK-27242][SQL] Make formatting TIMESTAMP/DATE literals independent from the default time zone #24181 (comment) . Produced strings are always textual representation of dates in the UTC time zone.

To take the current date in local time zone (defined by spark.sql.session.timeZone), DATE column can be casted to TIMESTAMP column, and former one can be converted STRING column. This works because TimestampType to StringType is time zone dependent. For example:

select date_format(cast(current_date as TIMESTAMP), 'yyyy-MM-dd')

or with implicit conversion:

select date_format(current_date, 'yyyy-MM-dd')

SparkQA · 2019-03-29T00:16:48Z

Test build #104060 has finished for PR 24185 at commit cdb24ae.

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

cloud-fan · 2019-03-29T01:44:31Z

thanks, merging to master!

srowen · 2019-03-29T13:50:06Z

@MaxGekk @cloud-fan I'm seeing this failure in master; looks related?

ComputeCurrentTimeSuite:
- analyzer should replace current_timestamp with literals
- analyzer should replace current_date with literals *** FAILED ***
  17984 was greater than or equal to 17983, but 17984 was not less than or equal to 17983 (ComputeCurrentTimeSuite.scala:64)

If it's just the test that needs to be adjusted, maybe just a follow up to patch it, not a revert.

MaxGekk · 2019-03-29T14:25:33Z

@srowen I am fixing the test, and preparing a PR.

MaxGekk · 2019-03-29T15:05:28Z

Here is the PR #24240 which fixes ComputeCurrentTimeSuite.

MaxGekk added 2 commits March 22, 2019 19:07

Make CurrentDate independent from time zone

7f2f69c

Updating the SQL migration guide

7ff29aa

MaxGekk commented Mar 22, 2019

View reviewed changes

docs/sql-migration-guide-upgrade.md Show resolved Hide resolved

Make the test code tolerant to JVM time zone

ae918cb

viirya reviewed Mar 24, 2019

View reviewed changes

MaxGekk added 2 commits March 24, 2019 10:28

Update docs of CurrentDate

9a8e9bc

Update docs of the current_date function

11afacc

cloud-fan reviewed Mar 24, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala Outdated Show resolved Hide resolved

Removing lazy

ec1f7e8

Merge remote-tracking branch 'origin/master' into current-date

2cbe5fa

srowen approved these changes Mar 28, 2019

View reviewed changes

Fix typo: EANBLED

cdb24ae

cloud-fan approved these changes Mar 28, 2019

View reviewed changes

cloud-fan closed this in 06abd06 Mar 29, 2019

MaxGekk mentioned this pull request Apr 10, 2019

[SPARK-27422][SQL] current_date() should return current date in the session time zone #24330

Closed

MaxGekk deleted the current-date branch September 18, 2019 15:57

[SPARK-27252][SQL] Make current_date() independent from time zones #24185

[SPARK-27252][SQL] Make current_date() independent from time zones #24185

Uh oh!

Conversation

MaxGekk commented Mar 22, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Uh oh!

SparkQA commented Mar 23, 2019

Uh oh!

MaxGekk commented Mar 23, 2019

Uh oh!

SparkQA commented Mar 23, 2019

Uh oh!

SparkQA commented Mar 23, 2019

Uh oh!

viirya Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk Mar 24, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 24, 2019

Uh oh!

MaxGekk commented Mar 24, 2019

Uh oh!

srowen commented Mar 24, 2019

Uh oh!

Uh oh!

MaxGekk commented Mar 24, 2019

Uh oh!

cloud-fan commented Mar 24, 2019

Uh oh!

MaxGekk commented Mar 26, 2019

Uh oh!

srowen commented Mar 26, 2019

Uh oh!

srowen commented Mar 26, 2019

Uh oh!

MaxGekk commented Mar 26, 2019

Uh oh!

cloud-fan commented Mar 26, 2019

Uh oh!

srowen commented Mar 28, 2019

Uh oh!

MaxGekk commented Mar 28, 2019

Uh oh!

SparkQA commented Mar 28, 2019

Uh oh!

MaxGekk commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 29, 2019

Uh oh!

cloud-fan commented Mar 29, 2019

Uh oh!

srowen commented Mar 29, 2019

Uh oh!

MaxGekk commented Mar 29, 2019

Uh oh!

MaxGekk commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya Mar 24, 2019 •

edited

Loading

MaxGekk commented Mar 28, 2019 •

edited

Loading