[SPARK-31150][SQL] Parsing seconds fraction with variable length for timestamp by yaooqinn · Pull Request #27906 · apache/spark

yaooqinn · 2020-03-13T18:20:54Z

What changes were proposed in this pull request?

This PR is to support parsing timestamp values with variable length second fraction parts.

e.g. 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]' can parse timestamp with 0~6 digit-length second fraction but fail >=7

select to_timestamp(v, 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]') from values
 ('2019-10-06 10:11:12.'),
 ('2019-10-06 10:11:12.0'),
 ('2019-10-06 10:11:12.1'),
 ('2019-10-06 10:11:12.12'),
 ('2019-10-06 10:11:12.123UTC'),
 ('2019-10-06 10:11:12.1234'),
 ('2019-10-06 10:11:12.12345CST'),
 ('2019-10-06 10:11:12.123456PST') t(v)
2019-10-06 03:11:12.123
2019-10-06 08:11:12.12345
2019-10-06 10:11:12
2019-10-06 10:11:12
2019-10-06 10:11:12.1
2019-10-06 10:11:12.12
2019-10-06 10:11:12.1234
2019-10-06 10:11:12.123456

select to_timestamp('2019-10-06 10:11:12.1234567PST', 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]')
NULL

Since 3.0, we use java 8 time API to parse and format timestamp values. when we create the DateTimeFormatter, we use appendPattern to create the build first, where the 'S..S' part will be parsed to a fixed-length(= 'S..S'.length). This fits the formatting part but too strict for the parsing part because the trailing zeros are very likely to be truncated.

Why are the changes needed?

improve timestamp parsing and more compatible with 2.4.x

Does this PR introduce any user-facing change?

no, the related changes are newly added

How was this patch tested?

add uts

…timestamp

SparkQA · 2020-03-13T22:05:33Z

Test build #119767 has finished for PR 27906 at commit 3e94191.

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

SparkQA · 2020-03-13T22:11:18Z

Test build #119768 has finished for PR 27906 at commit 8ff8875.

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

SparkQA · 2020-03-13T22:43:17Z

Test build #119771 has finished for PR 27906 at commit c58764d.

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

SparkQA · 2020-03-14T07:05:02Z

Test build #119784 has finished for PR 27906 at commit b135dd5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-03-14T07:20:39Z

retest this please

SparkQA · 2020-03-14T11:30:44Z

Test build #119790 has finished for PR 27906 at commit b135dd5.

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

yaooqinn · 2020-03-16T08:50:34Z

cc @cloud-fan @maropu @MaxGekk @xuanyuanking, thanks

cloud-fan · 2020-03-16T08:57:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVInferSchema.scala

    options.locale,
-    legacyFormat = FAST_DATE_FORMAT)
+    legacyFormat = FAST_DATE_FORMAT,
+    varLenEnabled = options.timestampFormat.contains('S'))


it's only for parsing so we should always set it to true?

I have updated this, and mv the cache optimization to DateTimeFormatterHelper

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

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

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

SparkQA · 2020-03-16T11:26:58Z

Test build #119854 has finished for PR 27906 at commit 2686c2e.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2020-03-16T14:38:05Z

Test build #119855 has finished for PR 27906 at commit a0e449a.

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

SparkQA · 2020-03-16T14:49:02Z

Test build #119860 has finished for PR 27906 at commit 81b69fd.

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

SparkQA · 2020-03-16T14:56:43Z

Test build #119856 has finished for PR 27906 at commit 18338df.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala

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

cloud-fan · 2020-03-16T15:24:24Z

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

+        var rest = pattenPart
+        while (rest.nonEmpty) {
+          rest match {
+            case extractor(prefix, sss, suffix) =>


nit: sss -> secondFraction

cloud-fan · 2020-03-16T15:25:44Z

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

+                builder.appendFraction(ChronoField.NANO_OF_SECOND, 0, sss.length, false)
+              }
+              rest = suffix
+            case _ => // never reach


it's reachable. The outer-most is pattern.split("'"), so it's possible that a section of pattern string doesn't have S.

the extractor here can match anything, so this is unreachable. If a pattern split contains no S, it goes the prefix

then let's throw IllegalStateException as it's un-reachable

cloud-fan · 2020-03-16T15:26:52Z

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

+  /**
+   * Building a formatter for parsing seconds fraction with variable length
+   */
+  def appendPattern(


the name is bad. How about createBuilderWithVarLengthSecondFraction?

SparkQA · 2020-03-16T18:27:13Z

Test build #119877 has finished for PR 27906 at commit 64fa825.

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

SparkQA · 2020-03-16T19:28:50Z

Test build #119872 has finished for PR 27906 at commit 234f7c8.

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

SparkQA · 2020-03-16T19:30:42Z

Test build #119880 has finished for PR 27906 at commit d4aea29.

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

SparkQA · 2020-03-16T21:13:20Z

Test build #119882 has finished for PR 27906 at commit 18de1b7.

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

dongjoon-hyun · 2020-03-16T22:42:11Z

Hi, @yaooqinn and @cloud-fan .
This is created an improvement JIRA. Is this a regression in 3.0.0 or an improvement in 3.1.0?

SparkQA · 2020-03-16T23:08:13Z

Test build #119886 has finished for PR 27906 at commit e550f16.

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

SparkQA · 2020-03-16T23:33:30Z

Test build #119888 has finished for PR 27906 at commit bb1a580.

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

yaooqinn · 2020-03-17T03:30:14Z

Hi @dongjoon-hyun, thanks for pointing that out. I'd say that this is a regression of 3.0.0.

cloud-fan · 2020-03-17T04:09:36Z

Yea the JIRA should be reported as bug. The same query works in 2.4 but returns null in 3.0.

cloud-fan · 2020-03-17T04:17:37Z

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

        "yyyy-MM-dd'T'HH:mm:ss.SSSSSS",
-        DateTimeUtils.getZoneId(zoneId))
+        DateTimeUtils.getZoneId(zoneId),
+        needVarLengthSecondFraction = true)


is this really needed? The test input is fixed: 2018-12-02T10:11:12.001234

We are not going to use fixed-length formatter for parsing right? So I guess test that one is not needed anymore. the var-length formatter should work fine with fixed inputs.

cloud-fan · 2020-03-17T04:17:47Z

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

        "yyyy-MM-dd'T'HH:mm:ss.SSSSSS",
-        DateTimeUtils.getZoneId(zoneId))
+        DateTimeUtils.getZoneId(zoneId),
+        needVarLengthSecondFraction = true)


this one is not needed

really? this test is for formatting, and the actual formatting doesn't do padding, so this flag should be false.

oh. I mean this should be false too.

cloud-fan · 2020-03-17T04:18:06Z

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

-          val parsed = formatter.parse(timestamp)
+          val timestamp = TimestampFormatter(pattern, zoneId).format(micros)
+          val parsed = TimestampFormatter(
+            pattern, zoneId, needVarLengthSecondFraction = true).parse(timestamp)


this change is needed because it is how the actual roundtrip goes no matter what the input is

ok makes sense.

cloud-fan · 2020-03-17T04:21:22Z

sql/core/src/test/resources/sql-tests/results/datetime.sql.out

+
+
+-- !query
+select to_timestamp('2019-10-06 10:11:12.123', 'yyyy-MM-dd HH:mm:ss[.SSSSSS]')


can we test the optional case? to_timestamp('2019-10-06 10:11:12', 'yyyy-MM-dd HH:mm:ss[.SSSSSS]')

cloud-fan

LGTM except a few comments about tests

…s[.SSSSSS]')

cloud-fan · 2020-03-17T05:51:10Z

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


  test("SPARK-30958: parse timestamp with negative year") {
-    val formatter1 = TimestampFormatter("yyyy-MM-dd HH:mm:ss", ZoneOffset.UTC)
+    val formatter1 = TimestampFormatter("yyyy-MM-dd HH:mm:ss", ZoneOffset.UTC, true)


is the point to test the actual parser? with needVarLengthSecondFraction = true.

yes, the actual parser should work with patterns w/o SSS too. I don't add new tests in TimestampFormatterSuite but modify the existing ones because we have enough e2e tests in date_time.sql

xuanyuanking

+1, only few comments for code structure, please add more comments for the util functions.

xuanyuanking · 2020-03-17T06:32:59Z

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

+  /**
+   * Building a formatter for parsing seconds fraction with variable length
+   */
+  def createBuilderWithVarLengthSecondFraction(


How about move this method into DateTimeUtils, together with convertIncompatiblePattern?
I think we need to collect all these kind of util functions analyzing pattern string together.

This is very specific to datetime parsing. How about we move convertIncompatiblePattern to object DateTimeFormatterHelper?

Then, I will not do this refactoring in this PR, not related

xuanyuanking · 2020-03-17T06:35:18Z

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

    val newPattern = DateTimeUtils.convertIncompatiblePattern(pattern)
-    val key = (newPattern, locale)
+    val useVarLen = needVarLengthSecondFraction && newPattern.split("'").zipWithIndex
+      .exists { case (p, idx) => idx % 2 == 0 && p.contains('S') }


Also for this one, although it's only one line, maybe it still worth having a function in DateTimeUtils and more comments.

S in literal is very rare, maybe newPattern.contains('S') is good enough as it's only for performance.

SparkQA · 2020-03-17T07:05:01Z

Test build #119909 has finished for PR 27906 at commit d1ab415.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-17T07:05:02Z

Test build #119914 has finished for PR 27906 at commit 27095da.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-17T12:41:18Z

Test build #119921 has finished for PR 27906 at commit ec44083.

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

cloud-fan · 2020-03-17T13:54:01Z

thanks, merging to master/3.0!

…timestamp ### What changes were proposed in this pull request? This PR is to support parsing timestamp values with variable length second fraction parts. e.g. 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]' can parse timestamp with 0~6 digit-length second fraction but fail >=7 ```sql select to_timestamp(v, 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]') from values ('2019-10-06 10:11:12.'), ('2019-10-06 10:11:12.0'), ('2019-10-06 10:11:12.1'), ('2019-10-06 10:11:12.12'), ('2019-10-06 10:11:12.123UTC'), ('2019-10-06 10:11:12.1234'), ('2019-10-06 10:11:12.12345CST'), ('2019-10-06 10:11:12.123456PST') t(v) 2019-10-06 03:11:12.123 2019-10-06 08:11:12.12345 2019-10-06 10:11:12 2019-10-06 10:11:12 2019-10-06 10:11:12.1 2019-10-06 10:11:12.12 2019-10-06 10:11:12.1234 2019-10-06 10:11:12.123456 select to_timestamp('2019-10-06 10:11:12.1234567PST', 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]') NULL ``` Since 3.0, we use java 8 time API to parse and format timestamp values. when we create the `DateTimeFormatter`, we use `appendPattern` to create the build first, where the 'S..S' part will be parsed to a fixed-length(= `'S..S'.length`). This fits the formatting part but too strict for the parsing part because the trailing zeros are very likely to be truncated. ### Why are the changes needed? improve timestamp parsing and more compatible with 2.4.x ### Does this PR introduce any user-facing change? no, the related changes are newly added ### How was this patch tested? add uts Closes #27906 from yaooqinn/SPARK-31150. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 0946a95) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…timestamp ### What changes were proposed in this pull request? This PR is to support parsing timestamp values with variable length second fraction parts. e.g. 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]' can parse timestamp with 0~6 digit-length second fraction but fail >=7 ```sql select to_timestamp(v, 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]') from values ('2019-10-06 10:11:12.'), ('2019-10-06 10:11:12.0'), ('2019-10-06 10:11:12.1'), ('2019-10-06 10:11:12.12'), ('2019-10-06 10:11:12.123UTC'), ('2019-10-06 10:11:12.1234'), ('2019-10-06 10:11:12.12345CST'), ('2019-10-06 10:11:12.123456PST') t(v) 2019-10-06 03:11:12.123 2019-10-06 08:11:12.12345 2019-10-06 10:11:12 2019-10-06 10:11:12 2019-10-06 10:11:12.1 2019-10-06 10:11:12.12 2019-10-06 10:11:12.1234 2019-10-06 10:11:12.123456 select to_timestamp('2019-10-06 10:11:12.1234567PST', 'yyyy-MM-dd HH:mm:ss.SSSSSS[zzz]') NULL ``` Since 3.0, we use java 8 time API to parse and format timestamp values. when we create the `DateTimeFormatter`, we use `appendPattern` to create the build first, where the 'S..S' part will be parsed to a fixed-length(= `'S..S'.length`). This fits the formatting part but too strict for the parsing part because the trailing zeros are very likely to be truncated. ### Why are the changes needed? improve timestamp parsing and more compatible with 2.4.x ### Does this PR introduce any user-facing change? no, the related changes are newly added ### How was this patch tested? add uts Closes apache#27906 from yaooqinn/SPARK-31150. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

yaooqinn added 3 commits March 14, 2020 02:03

[SPARK-31150][SQL] Parsing seconds fraction with variable length for …

3e94191

…timestamp

add more tests

8ff8875

restore the behavior of DateTimeBenchmark

c58764d

fix tests

b135dd5

yaooqinn added 2 commits March 16, 2020 16:41

handle escaped literal S

2686c2e

Merge branch 'master' into SPARK-31150

a0e449a