[SPARK-30668][SQL] Support `SimpleDateFormat` patterns in parsing timestamps/dates strings #27441

MaxGekk · 2020-02-03T10:59:10Z

What changes were proposed in this pull request?

In the PR, I propose to partially revert the commit 51a6ba0, and provide a legacy parser based on FastDateFormat which is compatible to SimpleDateFormat.

To enable the legacy parser, set spark.sql.legacy.timeParser.enabled to true.

Why are the changes needed?

To allow users to restore old behavior in parsing timestamps/dates using SimpleDateFormat patterns. The main reason for restoring is DateTimeFormatter's patterns are not fully compatible to SimpleDateFormat patterns, see https://issues.apache.org/jira/browse/SPARK-30668

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Added new test to DateFunctionsSuite
Restored additional test cases in JsonInferSchemaSuite.

MaxGekk · 2020-02-03T11:03:53Z

I will update the SQL migration guide soon.

MaxGekk · 2020-02-03T11:04:04Z

jenkins, retest this, please

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

cloud-fan · 2020-02-03T12:01:12Z

This patch LGTM as it is (pending migration guide updates). Another concern is: since we use the new formatter by default, shall we try the old formatter if the new formatter doesn't work? Then users don't need to enable the legacy config for some cases.

SparkQA · 2020-02-03T15:08:02Z

Test build #117770 has finished for PR 27441 at commit af38b87.

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

sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala

dongjoon-hyun · 2020-02-03T17:20:15Z

Hi, @gatorsmile . Since you are the issue reporter, could you confirm that you are okay with this solution?

MaxGekk · 2020-02-03T17:22:02Z

since we use the new formatter by default, shall we try the old formatter if the new formatter doesn't work? Then users don't need to enable the legacy config for some cases.

This may have some downsides like some values in a column could be parsed by old parser which is based on combined calendar (Julian + Gregorian) but another values in the same column by new one which uses Proleptic Gregorian calendar. This could make user life harder.

MaxGekk · 2020-02-03T18:18:25Z

There are some problems with jenkins:

ERROR: [WS-CLEANUP] Cannot delete workspace: remote file operation failed: /home/jenkins/workspace/SparkPullRequestBuilder at hudson.remoting.Channel@35d245c0:amp-jenkins-worker-02: java.nio.file.FileSystemException: /home/jenkins/workspace/SparkPullRequestBuilder: Read-only file system
ERROR: Cannot delete workspace: remote file operation failed: /home/jenkins/workspace/SparkPullRequestBuilder at hudson.remoting.Channel@35d245c0:amp-jenkins-worker-02: java.nio.file.FileSystemException: /home/jenkins/workspace/SparkPullRequestBuilder: Read-only file system

MaxGekk · 2020-02-03T18:32:01Z

I will update the SQL migration guide soon.
pending migration guide updates

I am considering 2 options for updating the SQL migration guide:

Write a separate item regarding SimpleDateFormat and DateTimeFomatter in parsing timestamps/dates using a pattern
Or to modify existing items https://github.com/apache/spark/blame/cd5f03a3ba18ae455f93abc5e5d04f098fa8f046/docs/sql-migration-guide.md#L70-L72

The first option will clearly highlight the changes. @cloud-fan @dongjoon-hyun WDYT?

MaxGekk · 2020-02-03T18:37:06Z

TODO: Need to update all docs like

spark/python/pyspark/sql/readwriter.py

Line 224 in e5abbab

follow the formats at ``java.time.format.DateTimeFormatter``. This

cloud-fan · 2020-02-04T03:38:11Z

a separate item sounds good, but don't forget to remove the existing item.

TODO: Need to update all docs like

I don't think so. Legacy configs are interval and are not expected to be turned on in most cases. We don't need to mention the legacy config in user-facing documents.

MaxGekk · 2020-02-04T07:50:55Z

Legacy configs are interval ...

At the moment, the config is not internal. I will make it as internal one. Also I reviewed other legacy configs, and made some of them internal: #27448

…pledateformat

MaxGekk · 2020-02-04T10:52:34Z

jenkins, retest this, please

MaxGekk · 2020-02-04T21:50:47Z

jenkins, retest this, please

SparkQA · 2020-02-05T01:57:50Z

Test build #117861 has finished for PR 27441 at commit 19a47c1.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PlanAdaptiveSubqueries(subqueryMap: Map[Long, SubqueryExec]) extends Rule[SparkPlan]

MaxGekk · 2020-02-05T07:55:36Z

jenkins, retest this, please

cloud-fan · 2020-02-05T08:43:43Z

docs/sql-migration-guide.md

-    - CSV/JSON datasources use java.time API for parsing and generating CSV/JSON content. In Spark version 2.4 and earlier, java.text.SimpleDateFormat is used for the same purpose with fallbacks to the parsing mechanisms of Spark 2.0 and 1.x. For example, `2018-12-08 10:39:21.123` with the pattern `yyyy-MM-dd'T'HH:mm:ss.SSS` cannot be parsed since Spark 3.0 because the timestamp does not match to the pattern but it can be parsed by earlier Spark versions due to a fallback to `Timestamp.valueOf`. To parse the same timestamp since Spark 3.0, the pattern should be `yyyy-MM-dd HH:mm:ss.SSS`.
-
-    - The `unix_timestamp`, `date_format`, `to_unix_timestamp`, `from_unixtime`, `to_date`, `to_timestamp` functions. New implementation supports pattern formats as described here https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html and performs strict checking of its input. For example, the `2015-07-22 10:00:00` timestamp cannot be parse if pattern is `yyyy-MM-dd` because the parser does not consume whole input. Another example is the `31/01/2015 00:00` input cannot be parsed by the `dd/MM/yyyy hh:mm` pattern because `hh` supposes hours in the range `1-12`.
+    - Parsing/formatting of timestamp/date strings. This effects on CSV/JSON datasources and on the `unix_timestamp`, `date_format`, `to_unix_timestamp`, `from_unixtime`, `to_date`, `to_timestamp` functions when patterns specified by users is used for parsing and formatting. Since Spark 3.0, the conversions are based on `java.time.format.DateTimeFormatter`, see https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html. New implementation performs strict checking of its input. For example, the `2015-07-22 10:00:00` timestamp cannot be parse if pattern is `yyyy-MM-dd` because the parser does not consume whole input. Another example is the `31/01/2015 00:00` input cannot be parsed by the `dd/MM/yyyy hh:mm` pattern because `hh` supposes hours in the range `1-12`. In Spark version 2.4 and earlier, `java.text.SimpleDateFormat` is used for timestamp/date string conversions, and the supported patterns are described in https://docs.oracle.com/javase/7/docs/api/java/text/SimpleDateFormat.html. The old behavior can be restored by setting `spark.sql.legacy.timeParser.enabled` to `true`.


is this really related to the Proleptic Gregorian calendar switch? It looks to me that we just switch to a better pattern string implementation.

Yes, it is related because SimpleDateFormat and DateTimeFormatter use different calendars underneath. Slightly different patterns are just a consequence of switching.

cloud-fan · 2020-02-05T08:44:33Z

LGTM except one question.

cloud-fan · 2020-02-05T10:49:00Z

thanks, merging to master/3.0!

…estamps/dates strings ### What changes were proposed in this pull request? In the PR, I propose to partially revert the commit 51a6ba0, and provide a legacy parser based on `FastDateFormat` which is compatible to `SimpleDateFormat`. To enable the legacy parser, set `spark.sql.legacy.timeParser.enabled` to `true`. ### Why are the changes needed? To allow users to restore old behavior in parsing timestamps/dates using `SimpleDateFormat` patterns. The main reason for restoring is `DateTimeFormatter`'s patterns are not fully compatible to `SimpleDateFormat` patterns, see https://issues.apache.org/jira/browse/SPARK-30668 ### Does this PR introduce any user-facing change? Yes ### How was this patch tested? - Added new test to `DateFunctionsSuite` - Restored additional test cases in `JsonInferSchemaSuite`. Closes #27441 from MaxGekk/support-simpledateformat. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 459e757) Signed-off-by: Wenchen Fan <[email protected]>

SparkQA · 2020-02-05T12:42:46Z

Test build #117909 has finished for PR 27441 at commit 19a47c1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class PlanAdaptiveSubqueries(subqueryMap: Map[Long, SubqueryExec]) extends Rule[SparkPlan]

gatorsmile · 2020-02-06T18:58:39Z

sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala

+      checkTimeZoneParsing(Timestamp.valueOf("2020-01-27 20:06:11.847"))
+    }
+    withSQLConf(SQLConf.LEGACY_TIME_PARSER_ENABLED.key -> "false") {
+      checkTimeZoneParsing(null)


fallback to the old parser?

Silent fallback to old parser can lead to mixed values in the same column - some in combined calendar Julian+Gregorian another in Proleptic Gregorian calendar.

Strings parsed in different calendars may have difference of dozen days.

… for new DateFormatter ### What changes were proposed in this pull request? This is a follow-up work for #27441. For the cases of new TimestampFormatter return null while legacy formatter can return a value, we need to throw an exception instead of silent change. The legacy config will be referenced in the error message. ### Why are the changes needed? Avoid silent result change for new behavior in 3.0. ### Does this PR introduce any user-facing change? Yes, an exception is thrown when we detect legacy formatter can parse the string and the new formatter return null. ### How was this patch tested? Extend existing UT. Closes #27537 from xuanyuanking/SPARK-30668-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… for new DateFormatter This is a follow-up work for #27441. For the cases of new TimestampFormatter return null while legacy formatter can return a value, we need to throw an exception instead of silent change. The legacy config will be referenced in the error message. Avoid silent result change for new behavior in 3.0. Yes, an exception is thrown when we detect legacy formatter can parse the string and the new formatter return null. Extend existing UT. Closes #27537 from xuanyuanking/SPARK-30668-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 7db0af5) Signed-off-by: Wenchen Fan <[email protected]>

… for new DateFormatter ### What changes were proposed in this pull request? This is a follow-up work for apache#27441. For the cases of new TimestampFormatter return null while legacy formatter can return a value, we need to throw an exception instead of silent change. The legacy config will be referenced in the error message. ### Why are the changes needed? Avoid silent result change for new behavior in 3.0. ### Does this PR introduce any user-facing change? Yes, an exception is thrown when we detect legacy formatter can parse the string and the new formatter return null. ### How was this patch tested? Extend existing UT. Closes apache#27537 from xuanyuanking/SPARK-30668-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

MaxGekk added 5 commits February 3, 2020 12:45

Add SQL config spark.sql.legacy.timeParser.enabled

68f6c65

Add LegacyTimestampFormatter

13d0ba1

Add LegacyDateFormatter

3d335ff

Check legacy parser in JsonInferSchemaSuite

c8bc585

Add a test for to_timestamp

af38b87

MaxGekk requested review from cloud-fan, dongjoon-hyun, gatorsmile, hvanhovell and srowen February 3, 2020 11:04

cloud-fan reviewed Feb 3, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

dongjoon-hyun added the SQL label Feb 3, 2020

dongjoon-hyun reviewed Feb 3, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala Show resolved Hide resolved

dongjoon-hyun reviewed Feb 3, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala Show resolved Hide resolved

MaxGekk added 2 commits February 3, 2020 21:07

Check results of new parser

03cec7b

Remove extra space

31675fa

MaxGekk added 3 commits February 4, 2020 11:19

Make SQL config as internal one

8ecb318

Update the SQL migration guide

fc25dc7

Merge remote-tracking branch 'remotes/origin/master' into support-sim…

19a47c1

…pledateformat

cloud-fan reviewed Feb 5, 2020

View reviewed changes

cloud-fan closed this in 459e757 Feb 5, 2020

gatorsmile reviewed Feb 6, 2020

View reviewed changes

xuanyuanking mentioned this pull request Feb 11, 2020

[SPARK-31410][SPARK-30668][SQL][FOLLOWUP] Raise exception instead of silent change for new DateFormatter #27537

Closed

MaxGekk deleted the support-simpledateformat branch June 5, 2020 19:43

[SPARK-30668][SQL] Support SimpleDateFormat patterns in parsing timestamps/dates strings #27441

[SPARK-30668][SQL] Support SimpleDateFormat patterns in parsing timestamps/dates strings #27441

Uh oh!

Conversation

MaxGekk commented Feb 3, 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!

MaxGekk commented Feb 3, 2020

Uh oh!

MaxGekk commented Feb 3, 2020

Uh oh!

Uh oh!

cloud-fan commented Feb 3, 2020

Uh oh!

SparkQA commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Feb 3, 2020

Uh oh!

MaxGekk commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk commented Feb 3, 2020

Uh oh!

MaxGekk commented Feb 3, 2020

Uh oh!

MaxGekk commented Feb 3, 2020

Uh oh!

cloud-fan commented Feb 4, 2020

Uh oh!

MaxGekk commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk commented Feb 4, 2020

Uh oh!

MaxGekk commented Feb 4, 2020

Uh oh!

SparkQA commented Feb 5, 2020

Uh oh!

MaxGekk commented Feb 5, 2020

Uh oh!

cloud-fan Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Feb 5, 2020

Uh oh!

cloud-fan commented Feb 5, 2020

Uh oh!

SparkQA commented Feb 5, 2020

Uh oh!

gatorsmile Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[SPARK-30668][SQL] Support `SimpleDateFormat` patterns in parsing timestamps/dates strings #27441

[SPARK-30668][SQL] Support `SimpleDateFormat` patterns in parsing timestamps/dates strings #27441

MaxGekk commented Feb 3, 2020 •

edited

Loading

MaxGekk commented Feb 4, 2020 •

edited

Loading