[SPARK-31939][SQL][test-java11] Fix Parsing day of year when year field pattern is missing #28766

yaooqinn · 2020-06-09T09:03:51Z

What changes were proposed in this pull request?

If a datetime pattern contains no year field, the day of year field should not be ignored if exists

e.g.

spark-sql> select to_timestamp('31', 'DD');
1970-01-01 00:00:00
spark-sql> select to_timestamp('31 30', 'DD dd');
1970-01-30 00:00:00

spark.sql.legacy.timeParserPolicy legacy
spark-sql> select to_timestamp('31', 'DD');
1970-01-31 00:00:00
spark-sql> select to_timestamp('31 30', 'DD dd');
NULL

This PR only fixes some corner cases that use 'D' pattern to parse datetimes and there is w/o 'y'.

Why are the changes needed?

fix some corner cases

Does this PR introduce any user-facing change?

yes, the day of year field will not be ignored

How was this patch tested?

add unit tests.

… missing

SparkQA · 2020-06-09T14:38:49Z

Test build #123683 has finished for PR 28766 at commit 2d797e0.

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

yaooqinn · 2020-06-10T02:16:51Z

cc @cloud-fan @maropu @MaxGekk please review this PR, thanks

cloud-fan · 2020-06-10T03:02:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala


+  private def verifyLocalDate(
+      accessor: TemporalAccessor, field: ChronoField, candidate: LocalDate): Unit = {
+    if (accessor.isSupported(field) && candidate.isSupported(field)) {


candidate.isSupported(field) this is always true?

For the time being, yes. I can remove this condition

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/TimestampFormatterSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala

cloud-fan · 2020-06-10T03:06:12Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/TimestampFormatterSuite.scala

+    val f7 = TimestampFormatter("dd-DD", UTC, isParsing = true)
+    assert(f7.parse("28-59") === date(1970, 2, 28))
+    assertParsingError(f7.parse("27-59"))
+  }


can we also add tests in the .sql file?

SparkQA · 2020-06-10T06:55:13Z

Test build #123715 has finished for PR 28766 at commit a11a049.

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DatetimeFormatterSuite.scala

cloud-fan · 2020-06-10T08:18:19Z

sql/core/src/test/resources/sql-tests/inputs/datetime-parsing.sql

@@ -0,0 +1,16 @@
+--- TESTS FOR DATETIME PARSING FUNCTIONS ---


there are a lot more tests should be added here later :)

cloud-fan · 2020-06-10T08:52:03Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DatetimeFormatterSuite.scala

+
+  private def assertError(pattern: String, datetimeStr: String, expectedMsg: String): Unit = {
+    val exception = if (useDateFormatter) {
+      intercept[SparkUpgradeException](dateFormatter(pattern).parse(datetimeStr))


does this mean the legacy formatter won't fail?

the legacy fomatter only fails when datetimeStr contains non-digits which we don't need to test here

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DatetimeFormatterSuite.scala

SparkQA · 2020-06-10T13:05:24Z

Test build #123741 has finished for PR 28766 at commit cd55187.

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

SparkQA · 2020-06-10T13:11:30Z

Test build #123753 has finished for PR 28766 at commit 6b8dec4.

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

cloud-fan · 2020-06-10T14:01:07Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DatetimeFormatterSuite.scala


+  private def dateFormatter(
+      pattern: String,
+      ldf: LegacyDateFormat = FAST_DATE_FORMAT): DateFormatter = {


super nit: we can put the parameters in one line

cloud-fan · 2020-06-10T14:02:09Z

sql/core/src/test/resources/sql-tests/inputs/datetime-parsing-invalid.sql

+select to_timestamp('2020-01-365', 'yyyy-dd-DDD');
+select to_timestamp('2020-10-350', 'yyyy-MM-DDD');
+select to_timestamp('2020-11-31-366', 'yyyy-MM-dd-DDD');
+select from_csv('2018-366', 'date Date', map('dateFormat', 'yyyy-DDD'))


let's add a comment to explain why we need to test csv: because it's lenient and Spark should throw upgrade exception.

cloud-fan · 2020-06-10T14:02:30Z

sql/core/src/test/resources/sql-tests/inputs/datetime-parsing.sql

+select to_timestamp('2020-30-365', 'yyyy-dd-DDD');
+select to_timestamp('2020-12-350', 'yyyy-MM-DDD');
+select to_timestamp('2020-12-31-366', 'yyyy-MM-dd-DDD');
+select from_csv('2018-365', 'date Date', map('dateFormat', 'yyyy-DDD'))


for non-error case, csv has no difference, we don't need to test it.

cloud-fan

LGTM except a few comments for the test

SparkQA · 2020-06-10T15:09:16Z

Test build #123747 has finished for PR 28766 at commit b3cdccf.

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

SparkQA · 2020-06-10T15:55:26Z

Test build #123743 has finished for PR 28766 at commit 20b585d.

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

SparkQA · 2020-06-10T17:21:48Z

Test build #123757 has finished for PR 28766 at commit 04ce7ee.

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

SparkQA · 2020-06-10T18:38:33Z

Test build #123763 has finished for PR 28766 at commit 1899e81.

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

SparkQA · 2020-06-10T19:54:28Z

Test build #123768 has finished for PR 28766 at commit 78c676e.

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

cloud-fan · 2020-06-11T03:29:04Z

thanks, merging to master/3.0!

…ld pattern is missing ### What changes were proposed in this pull request? If a datetime pattern contains no year field, the day of year field should not be ignored if exists e.g. ``` spark-sql> select to_timestamp('31', 'DD'); 1970-01-01 00:00:00 spark-sql> select to_timestamp('31 30', 'DD dd'); 1970-01-30 00:00:00 spark.sql.legacy.timeParserPolicy legacy spark-sql> select to_timestamp('31', 'DD'); 1970-01-31 00:00:00 spark-sql> select to_timestamp('31 30', 'DD dd'); NULL ``` This PR only fixes some corner cases that use 'D' pattern to parse datetimes and there is w/o 'y'. ### Why are the changes needed? fix some corner cases ### Does this PR introduce _any_ user-facing change? yes, the day of year field will not be ignored ### How was this patch tested? add unit tests. Closes #28766 from yaooqinn/SPARK-31939. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 22dda6e) Signed-off-by: Wenchen Fan <[email protected]>

HyukjinKwon · 2020-06-11T13:58:34Z

I reverted this from 3.0 - it causes test failure. @yaooqinn can you create a PR to backport this to branch-3.0?

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123839/testReport/

yaooqinn · 2020-06-11T14:28:00Z

checking on that. @HyukjinKwon, thanks

dongjoon-hyun · 2020-06-11T16:02:09Z

Thank you, @HyukjinKwon !

yaooqinn added 2 commits June 9, 2020 16:57

[SPARK-31939][SQL] Fix Parsing day of year when year field pattern is…

6afd5f7

… missing

nit

2d797e0

probot-autolabeler bot added the SQL label Jun 9, 2020

refine

a11a049

cloud-fan reviewed Jun 10, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/TimestampFormatterSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 10, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala Show resolved Hide resolved

cloud-fan reviewed Jun 10, 2020

View reviewed changes

yaooqinn added 4 commits June 10, 2020 15:33

add tests

f78db47

Merge branch 'master' into SPARK-31939

b7da48e

add tests

cd55187

for java 11

20b585d

yaooqinn changed the title ~~[SPARK-31939][SQL] Fix Parsing day of year when year field pattern is missing~~ [SPARK-31939][SQL][test-java11] Fix Parsing day of year when year field pattern is missing Jun 10, 2020

cloud-fan reviewed Jun 10, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DatetimeFormatterSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 10, 2020

View reviewed changes

address comments

b3cdccf

cloud-fan reviewed Jun 10, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DatetimeFormatterSuite.scala Show resolved Hide resolved

yaooqinn added 4 commits June 10, 2020 17:25

nit

6b8dec4

fix test

52184fc

nit

04ce7ee

more tests

1899e81

cloud-fan reviewed Jun 10, 2020

View reviewed changes

cloud-fan approved these changes Jun 10, 2020

View reviewed changes

adress cloudfan's comments

78c676e

cloud-fan closed this in 22dda6e Jun 11, 2020

		@@ -0,0 +1,16 @@
		--- TESTS FOR DATETIME PARSING FUNCTIONS ---

[SPARK-31939][SQL][test-java11] Fix Parsing day of year when year field pattern is missing #28766

[SPARK-31939][SQL][test-java11] Fix Parsing day of year when year field pattern is missing #28766

Uh oh!

Conversation

yaooqinn commented Jun 9, 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 Jun 9, 2020

Uh oh!

yaooqinn commented Jun 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

SparkQA commented Jun 10, 2020

Uh oh!

cloud-fan commented Jun 11, 2020

Uh oh!

HyukjinKwon commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaooqinn commented Jun 11, 2020

Uh oh!

dongjoon-hyun commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Jun 11, 2020 •

edited

Loading