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

xuanyuanking · 2020-02-11T04:07:20Z

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.

xuanyuanking · 2020-02-11T04:11:24Z

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

These changes will be reverted after #27524.

SparkQA · 2020-02-11T06:40:13Z

Test build #118205 has finished for PR 27537 at commit 4679600.

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

SparkQA · 2020-02-12T07:13:37Z

Test build #118271 has finished for PR 27537 at commit b9b3c8f.

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

cloud-fan · 2020-02-18T13:14:16Z

@xuanyuanking can you fix the conflicts?

xuanyuanking · 2020-02-18T13:29:47Z

Sure, will also reuse the LegacyBehaviorPolicy added in #27579

SparkQA · 2020-02-21T08:05:02Z

Test build #118754 has finished for PR 27537 at commit c07d09d.

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

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

HyukjinKwon · 2020-02-21T09:55:45Z

cc @MaxGekk

SparkQA · 2020-02-24T08:05:02Z

Test build #118846 has finished for PR 27537 at commit 86313e5.

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

cloud-fan · 2020-02-24T08:48:02Z

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

This is used to create DateFormatter and TimestampFormatter, and I'm pretty sure these 2 formatters are used in many expressions and other places like the json parser.

I think it's better to put the logic in the formatter, not in some expressions.

Thanks, got it. I moved this logic into formatter in cc9fd4f.

SparkQA · 2020-02-24T13:10:07Z

Test build #118856 has finished for PR 27537 at commit 6f51b13.

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

SparkQA · 2020-02-27T10:08:13Z

Test build #119021 has finished for PR 27537 at commit cc9fd4f.

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

cloud-fan · 2020-02-27T15:05:24Z

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

different places can create different legacy formatter. It's better to always create the legacy formatter, and the new formatter just carry the instance of legacy formatter.

Thanks, done in 47d2f80

cloud-fan · 2020-02-27T15:18:02Z

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

IIRC some places just call formatter and catch NonFatal. Can you do some checks and make sure we don't catch non-fatal blindly?

Thanks for reminding, fix for the non-codegen in fbac26d.

SparkQA · 2020-02-27T16:57:07Z

Test build #119034 has finished for PR 27537 at commit addfccc.

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

SparkQA · 2020-02-27T20:12:05Z

Test build #119035 has finished for PR 27537 at commit 5f28f5b.

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/util/DateFormatter.scala

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

core/src/main/scala/org/apache/spark/SparkException.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

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

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

cloud-fan · 2020-03-04T11:10:07Z

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

  }
+
+  // When legacy time parser policy set to EXCEPTION, check whether we will get different results
+  // between legacy format and new format. For legacy parser, DateTimeParseException will not be


format -> parser?

cloud-fan · 2020-03-04T11:12:05Z

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

+
+  // When legacy time parser policy set to EXCEPTION, check whether we will get different results
+  // between legacy format and new format. For legacy parser, DateTimeParseException will not be
+  // thrown. On the contrary, if the legacy policy set to CORRECTED, DateTimeParseException will


For legacy parser, DateTimeParseException will not be thrown
I think it should be
If new parser fails but legacy parser works, throw a SparkUpgradeException.

On the contrary, if the legacy policy set to CORRECTED ...

If the legacy policy set to CORRECTED, do nothing and let the exception propagate.

cloud-fan · 2020-03-04T11:15:38Z

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

+        case _: Throwable => None
+      }
+      if (res.nonEmpty) {
+        throw new SparkUpgradeException("3.0", s"Set ${SQLConf.LEGACY_TIME_PARSER_POLICY.key} to " +


We should explain what happened.

Fail to parse '$s' in the new parser. You can set ... to LEGACY to restore ..., or set it to CORRECTED and treat it as an invalid datetime string.

cloud-fan · 2020-03-04T11:19:34Z

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

+      val msg = intercept[SparkException] {
+        df2.collect()
+      }.getCause.getMessage
+      assert(msg.contains(s"Set ${SQLConf.LEGACY_TIME_PARSER_POLICY.key} to LEGACY to restore " +


I think it's good enough to test if the cause is SparkUpgradeException

cloud-fan · 2020-03-04T11:19:45Z

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

+    val message = intercept[SparkException] {
+      df.collect()
+    }.getCause.getMessage
+    assert(message.contains(s"Set ${SQLConf.LEGACY_TIME_PARSER_POLICY.key} to LEGACY to restore " +


cloud-fan · 2020-03-04T11:20:39Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+      val msg = intercept[SparkException] {
+        csv.collect()
+      }.getCause.getMessage
+      assert(msg.contains(s"Set ${SQLConf.LEGACY_TIME_PARSER_POLICY.key} to LEGACY to restore " +


cloud-fan · 2020-03-04T11:21:20Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala

+      val msg = intercept[SparkException] {
+        json.collect()
+      }.getCause.getMessage
+      assert(msg.contains(s"Set ${SQLConf.LEGACY_TIME_PARSER_POLICY.key} to LEGACY to restore " +


cloud-fan · 2020-03-04T11:23:39Z

core/src/main/scala/org/apache/spark/SparkException.scala

+ * Exception thrown when Spark returns different result after upgrading to a new version.
+ */
+private[spark] class SparkUpgradeException(version: String, message: String, cause: Throwable)
+  extends SparkException(s"Exception for upgrading to Spark $version: $message", cause)


You may get a different result due to the upgrading of Spark $version: $message

SparkQA · 2020-03-04T13:36:22Z

Test build #119299 has finished for PR 27537 at commit 1a92e0f.

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

cloud-fan · 2020-03-04T15:47:48Z

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

+      checkExceptionInExpression[SparkUpgradeException](
+        GetTimestamp(
+          Literal("2020-01-27T20:06:11.847-0800"),
+          Literal("yyyy-MM-dd'T'HH:mm:ss.SSSz")), "Exception for upgrading to Spark 3.0")


"Fail to parse"?

SparkQA · 2020-03-04T15:59:52Z

Test build #119306 has finished for PR 27537 at commit 0e543da.

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

SparkQA · 2020-03-05T04:52:34Z

Test build #119355 has finished for PR 27537 at commit d01c750.

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

cloud-fan · 2020-03-05T07:29:57Z

thanks, merging to master/3.0!

… 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]>

xuanyuanking · 2020-03-05T10:20:18Z

Thanks for the review!

… 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]>

xuanyuanking commented Feb 11, 2020

View reviewed changes

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

Copy link

Member Author

xuanyuanking Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes will be reverted after #27524.

xuanyuanking mentioned this pull request Feb 14, 2020

[SPARK-30829][SQL] Define LegacyBehaviorPolicy enumeration as the common value for result change configs #27579

Closed

xuanyuanking force-pushed the SPARK-30668-follow branch from b9b3c8f to c07d09d Compare February 21, 2020 05:19

xuanyuanking changed the title ~~[WIP][SPARK-30668][SQL][FOLLOWUP] Raise exception instead of silent change for new TimestampFormatter~~ [SPARK-30668][SQL][FOLLOWUP] Raise exception instead of silent change for new TimestampFormatter Feb 21, 2020

cloud-fan reviewed Feb 21, 2020

View reviewed changes

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

xuanyuanking force-pushed the SPARK-30668-follow branch from 86313e5 to 6f51b13 Compare February 24, 2020 08:14

cloud-fan reviewed Feb 24, 2020

View reviewed changes

cloud-fan reviewed Feb 27, 2020

View reviewed changes

dongjoon-hyun added the SQL label Feb 28, 2020

xuanyuanking force-pushed the SPARK-30668-follow branch from 0242629 to fbac26d Compare March 2, 2020 12:15