[SPARK-33498][SQL] Datetime parsing should fail if the input string can't be parsed, or the pattern string is invalid #30442

leanken-zz · 2020-11-20T09:41:13Z

What changes were proposed in this pull request?

Datetime parsing should fail if the input string can't be parsed, or the pattern string is invalid, when ANSI mode is enable. This patch should update GetTimeStamp, UnixTimeStamp, ToUnixTimeStamp and Cast.

Why are the changes needed?

For ANSI mode.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added UT and Existing UT.

leanken-zz · 2020-11-20T09:42:52Z

@cloud-fan FYI.

SparkQA · 2020-11-20T11:25:39Z

Test build #131418 has finished for PR 30442 at commit e6f5634.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnixTimestamp(

SparkQA · 2020-11-20T11:44:50Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36024/

SparkQA · 2020-11-20T12:09:33Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36024/

HyukjinKwon · 2020-11-20T13:25:56Z

cc @MaxGekk too FYI

HyukjinKwon · 2020-11-20T13:28:59Z

docs/sql-ref-ansi-compliance.md

  - `element_at`: This function throws `ArrayIndexOutOfBoundsException` if using invalid indices. 
  - `element_at`: This function throws `NoSuchElementException` if key does not exist in map. 
  - `elt`: This function throws `ArrayIndexOutOfBoundsException` if using invalid indices.
+  - `to_date` This function should fail with Exception if the input string can't be parsed, or the pattern string is invalid.


I would call it an exception instead of Exception.

SparkQA · 2020-11-23T04:26:31Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36131/

SparkQA · 2020-11-23T04:40:12Z

Test build #131529 has finished for PR 30442 at commit 67f2d41.

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

SparkQA · 2020-11-23T04:50:26Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36131/

cloud-fan · 2020-11-23T09:14:49Z

docs/sql-ref-ansi-compliance.md

 The behavior of some SQL operators can be different under ANSI mode (`spark.sql.ansi.enabled=true`).
  - `array_col[index]`: This operator throws `ArrayIndexOutOfBoundsException` if using invalid indices.
  - `map_col[key]`: This operator throws `NoSuchElementException` if key does not exist in map.
+  - `cast to timestamp`: This operator should fail with an exception if the input string can't be parsed.


nit: CAST(string_col AS TIMESTAMP) to be user-facing.

cloud-fan · 2020-11-23T09:18:25Z

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

    }
  }

+  def stringToTimestamp(s: UTF8String, timeZoneId: ZoneId, ansiEnabled: Boolean): Option[Long] = {


It's better to add a stringToTimestampAnsi method. Non-ansi code path should call the original stringToTimestamp method.

cloud-fan · 2020-11-23T09:28:41Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

+    }
+  }
+
+  test("SPARK-33498: TimestampType cast with parseError") {


is it duplicated with CastSuiteBase?

cloud-fan · 2020-11-23T09:30:48Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

+            UnixTimestamp(Literal("2020-01-27T20:06:11.847"), Literal("yyyy-MM-dd HH:mm:ss.SSS")),
+            UnixTimestamp(Literal("Unparseable"), Literal("yyyy-MM-dd HH:mm:ss.SSS")),
+            ToUnixTimestamp(Literal("2020-01-27T20:06:11.847"), Literal("yyyy-MM-dd HH:mm:ss.SSS")),
+            ToUnixTimestamp(Literal("Unparseable"), Literal("yyyy-MM-dd HH:mm:ss.SSS"))


can we test one more case that throws SparkUpgradeException?

SparkQA · 2020-11-24T01:06:41Z

Test build #131592 has finished for PR 30442 at commit 67f2d41.

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

SparkQA · 2020-11-24T10:14:50Z

Test build #131635 has finished for PR 30442 at commit 0417977.

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

SparkQA · 2020-11-24T14:49:14Z

Test build #131656 has finished for PR 30442 at commit 9b76d6a.

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

cloud-fan · 2020-11-24T15:13:03Z

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

We should call stringToTimestampAnsi when ansi mode is on, and call the original method when ansi mode is off.

… runtime exception when parsing to timestamp fail with ANSI mode on. Change-Id: Ie76d494906c7615871860c89602896c64ed2d9d6

… multi thread and not respect SQLConf.get. Change-Id: I60be4963378de992c763cec952af4c03acbcc99f

Change-Id: Iafb1e1abfbf19e063b44f358be729515bea3a6f0

Change-Id: Iddbbe72295a0db24df4c020e79d74b8bdaa95082

SparkQA · 2020-11-25T08:57:28Z

Test build #131739 has finished for PR 30442 at commit 140081d.

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

SparkQA · 2020-11-25T10:57:38Z

Test build #131744 has finished for PR 30442 at commit 5ab59d4.

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

leanken-zz · 2020-11-25T11:07:36Z

@cloud-fan Test passed.

cloud-fan · 2020-11-25T13:14:19Z

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

    }
  }

+  def stringToTimestampAnsi(s: UTF8String, timeZoneId: ZoneId): Option[Long] = {


does this method still need to return option?

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

cloud-fan · 2020-11-25T13:16:25Z

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

        ctx.addReferenceObj("zoneId", zoneId, zoneIdClass.getName),
        zoneIdClass)
      val longOpt = ctx.freshVariable("longOpt", classOf[Option[Long]])
+      val stringToTimestampFunc = if (ansiEnabled) "stringToTimestampAnsi" else "stringToTimestamp"


If stringToTimestampAnsi returns Long directly, the codegen for it will be largely simplified.

cloud-fan · 2020-11-25T13:18:04Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+          s"Cannot cast $str to TimestampType.")
+      }
+
+      withSQLConf(SQLConf.ANSI_ENABLED.key -> currentAnsiEnabled.toString) {


to be more robust:

val activeConf = conf new ParVector... SQLConf.withExistingConf(activeConf) ...

cloud-fan · 2020-11-25T13:19:04Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+    }
+  }
+
+  test("ANSI mode: timestamp type casting with parse error") {


Why is this a separate test?

merged in one test

cloud-fan · 2020-11-25T13:24:00Z

sql/core/src/test/resources/sql-tests/results/postgreSQL/window_part3.sql.out

 struct<>
 -- !query output
-
+org.apache.spark.sql.AnalysisException


Not related to this PR but it's weird that the data preparation step for a test is broken. We should fix it later.

seems this datatimes table was not used at all. I could refine this test case in new PR.

SparkQA · 2020-11-27T10:12:24Z

Test build #131865 has finished for PR 30442 at commit 4706576.

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

Change-Id: I2bfe150923e5a4d14ac6a44f9a2acff9698d5e5d

cloud-fan · 2020-11-27T13:22:17Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+        checkCastWithParseError("2015-03-18T12:03:17-0:70")
+
+        val input = "abdef"
+        checkExceptionInExpression[DateTimeException](


is it just checkCastWithParseError("abdef")?

cloud-fan · 2020-11-27T13:23:19Z

GA passed, merging to master!

SparkQA · 2020-11-27T14:44:06Z

Test build #131879 has finished for PR 30442 at commit e4fe5ee.

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

github-actions bot added DOCS SQL labels Nov 20, 2020

HyukjinKwon reviewed Nov 20, 2020

View reviewed changes

leanken-zz force-pushed the leanken-SPARK-33498 branch from e6f5634 to 67f2d41 Compare November 23, 2020 03:39

cloud-fan reviewed Nov 23, 2020

View reviewed changes

leanken-zz force-pushed the leanken-SPARK-33498 branch from 67f2d41 to 0417977 Compare November 24, 2020 08:43

leanken-zz force-pushed the leanken-SPARK-33498 branch from 0417977 to 9b76d6a Compare November 24, 2020 11:22

cloud-fan reviewed Nov 24, 2020

View reviewed changes

leanken-zz added 3 commits November 25, 2020 13:16

update GetTimeStamp, UnixTimeStamp, ToUnixTimeStamp and Cast to throw…

a197091

… runtime exception when parsing to timestamp fail with ANSI mode on. Change-Id: Ie76d494906c7615871860c89602896c64ed2d9d6

fix UT failures. remove ParVector in CastSuite.scala since it's using…

24de1dc

… multi thread and not respect SQLConf.get. Change-Id: I60be4963378de992c763cec952af4c03acbcc99f

fix CastSuite UT failures..

140081d

Change-Id: Iafb1e1abfbf19e063b44f358be729515bea3a6f0

leanken-zz force-pushed the leanken-SPARK-33498 branch from 9b76d6a to 140081d Compare November 25, 2020 05:17

code refine.

5ab59d4

Change-Id: Iddbbe72295a0db24df4c020e79d74b8bdaa95082

cloud-fan reviewed Nov 25, 2020

View reviewed changes

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

cloud-fan reviewed Nov 25, 2020

View reviewed changes

cloud-fan approved these changes Nov 27, 2020

View reviewed changes

update nullable in ANSI mode and refine UT..

e4fe5ee

Change-Id: I2bfe150923e5a4d14ac6a44f9a2acff9698d5e5d

leanken-zz force-pushed the leanken-SPARK-33498 branch from 4706576 to e4fe5ee Compare November 27, 2020 10:14

cloud-fan reviewed Nov 27, 2020

View reviewed changes

cloud-fan approved these changes Nov 27, 2020

View reviewed changes

cloud-fan closed this in b9f2f78 Nov 27, 2020

[SPARK-33498][SQL] Datetime parsing should fail if the input string can't be parsed, or the pattern string is invalid #30442

[SPARK-33498][SQL] Datetime parsing should fail if the input string can't be parsed, or the pattern string is invalid #30442

Uh oh!

Conversation

leanken-zz commented Nov 20, 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!

leanken-zz commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

HyukjinKwon commented Nov 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 25, 2020

Uh oh!

SparkQA commented Nov 25, 2020

Uh oh!

leanken-zz commented Nov 25, 2020

Uh oh!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 27, 2020

Uh oh!

SparkQA commented Nov 27, 2020

Uh oh!

Reviewers

Assignees

leanken-zz commented Nov 20, 2020 •

edited

Loading