[SPARK-38929][SQL] Improve error messages for cast failures in ANSI #36241

anchovYu · 2022-04-18T04:57:36Z

What changes were proposed in this pull request?

Improve the error messages for cast failures in ANSI.
As mentioned in https://issues.apache.org/jira/browse/SPARK-38929, this PR targets two cast-to types: numeric types and date types.

For numeric(int, smallint, double, float, decimal ..) types, it embeds the cast-to types in the error message. For example,
```
Invalid input value for type INT: '1.0'. To return NULL instead, use 'try_cast'. If necessary set %s to false to bypass this error.
```
It uses the toSQLType and toSQLValue to wrap the corresponding types and literals.

For date types, it does similarly as above. For example,

Invalid input value for type TIMESTAMP: 'a'. To return NULL instead, use 'try_cast'. If necessary set spark.sql.ansi.enabled to false to bypass this error.

Why are the changes needed?

To improve the error message in general.

Does this PR introduce any user-facing change?

It changes the error messages.

How was this patch tested?

The related unit tests are updated.

anchovYu · 2022-04-18T04:58:33Z

The tests running: https://github.com/anchovYu/spark/actions/runs/2182280611

anchovYu · 2022-04-18T05:00:49Z

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

    } catch {
      case e: NumberFormatException =>
-        throw QueryExecutionErrors.invalidInputSyntaxForNumericError(e, errorContext)
+        throw QueryExecutionErrors.invalidInputSyntaxForNumericError(to, s, errorContext)


Here it doesn't use the error message in e coming from toLongExact, but uses the error message in error-classes.json for better error message organization and grouping.

But the side effect is, it throws the org.apache.spark.SparkNumberFormatException instead of the java.lang.NumberFormatException. The former is a subclass of the latter, so the code catching the latter will still work with the updated version. Will it be a problem?

This is fine

anchovYu · 2022-04-18T05:02:55Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

  }

  def invalidInputSyntaxForNumericError(
+      to: AbstractDataType,


I don't know whether there is a better way than this AbstractDataType and corresponding tosimpleString method, considering the DecimalType. Since the cast fails, we can't get a concrete DecimalType class with scale and precision, so I just can't use the DataType here.

Problem solved if we use the DecimalType from the caller of fromStringANSI:
https://github.com/apache/spark/pull/36244/files#diff-393a0ad195d66fb094f2dca3669c1317b7f57a6637f6e95241b57bc6550ce49aR819

anchovYu · 2022-04-18T05:09:57Z

Hi @cloud-fan and @MaxGekk , could you take a look at this one? Thank you!

cloud-fan · 2022-04-18T05:39:01Z

core/src/main/resources/error/error-classes.json

    "message" : [ "Input schema %s can only contain StringType as a key type for a MapType." ]
  },
+  "INVALID_LITERAL_FORMAT_FOR_CAST" : {
+    "message" : [ "Invalid %s literal: %s. To return NULL instead, use 'try_cast'. If necessary set %s to false to bypass this error.%s" ],


Is cast input always a literal? I think Invalid input value for type %s: %s. To return NULL ... is better

cloud-fan · 2022-04-18T05:40:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

    } catch {
      case _: NumberFormatException =>
-        throw QueryExecutionErrors.invalidInputSyntaxForNumericError(str, errorContext)
+        throw QueryExecutionErrors.invalidInputSyntaxForNumericError(DecimalType, str, errorContext)


Where do we call fromStringANSI? There should be a concrete DecimalType available

I don't think we can get that concrete Decimal type because the cast fails: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala#L629

We can get it from the caller:
https://github.com/apache/spark/pull/36244/files#diff-393a0ad195d66fb094f2dca3669c1317b7f57a6637f6e95241b57bc6550ce49aR819

I didn't do it initially, because I think fromStringANSI and changePrecision are two separate processes and has its corresponding exceptions. The former has an exception to cast from a string to a BigDecimal, and at this step, the target DecimalType doesn't appear. While the latter has exception to change precision, where target starts to take place.

But maybe even though these are two steps, they are always called together serially. Thus, having the target information in the first stage seems fine. It also saves a lot of trouble in AbstractDataType etc. Will update the PR.

gengliangwang · 2022-04-18T15:07:06Z

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

      case StringType =>
        val doubleStr = ctx.freshVariable("doubleStr", StringType)
        (c, evPrim, evNull) =>
+          val dt = ctx.addReferenceObj("doubleType", DoubleType, DoubleType.getClass.getName)


This is overkill. Let's simply hardcode the data type name in the method invalidInputSyntaxForNumericError like #36244

Sorry I forget to remove this line .. the dt is not used.

BTW according to the comment from Maxim below, I believe we still need to accept a DataType or AbstractDataType instead of a pure string as a parameter to invalidInputSyntaxForNumericError, so that we can utilize the toSQLType to format

gengliangwang · 2022-04-18T15:21:05Z

@anchovYu I am sorry that I didn't notice you start this one. I realize it after I created #36244.
This PR should not limit to the literals. I suggest changing the scope of casting strings to datetime types and follow #36244 in implementation.
I have also added you as co-author in #36244.

It would be great if you ping me in ANSI-related PRs, thanks!

MaxGekk · 2022-04-18T16:32:56Z

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

      formatter.parse("x123")
    }.getMessage
-    assert(errMsg.contains("Cannot cast x123 to DateType"))
+    assert(errMsg.contains("Invalid `date` literal: 'x123'"))


Please, use toSQLType to output types, see #36233

Will do, thanks for the source!

AmplabJenkins · 2022-04-18T21:37:30Z

Can one of the admins verify this patch?

anchovYu · 2022-04-19T01:09:33Z

The latest tests running: https://github.com/anchovYu/spark/actions/runs/2186916595

gengliangwang · 2022-04-19T01:24:08Z

core/src/main/resources/error/error-classes.json

    "sqlState" : "42000"
  },
+  "INVALID_FORMAT_FOR_CAST" : {
+    "message" : [ "Invalid input value for type %s: %s. To return NULL instead, use 'try_cast'. If necessary set %s to false to bypass this error.%s" ],


Shall we keep the previous wording

Invalid input syntax for type ...

It is from PostgreSQL

gengliangwang · 2022-04-19T01:24:37Z

core/src/main/resources/error/error-classes.json

    "message" : [ "Field name %s is invalid: %s is not a struct." ],
    "sqlState" : "42000"
  },
+  "INVALID_FORMAT_FOR_CAST" : {


How about INVALID_SYNTAX_FOR_CAST

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryErrorsBase.scala

anchovYu · 2022-04-19T06:57:23Z

Latest test: https://github.com/anchovYu/spark/actions/runs/2188016147

gengliangwang · 2022-04-19T07:04:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

-    new DateTimeException(s"Cannot cast $value to $to. To return NULL instead, use 'try_cast'. " +
-      s"If necessary set ${SQLConf.ANSI_ENABLED.key} to false to bypass this error." + errorContext)
+    new DateTimeException(s"Invalid input syntax for type ${toSQLType(to)}: " +
+      s"${if (value.isInstanceOf[UTF8String]) {


nit: let's save the string value to a val before line 1016

gengliangwang · 2022-04-19T07:05:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

  }

-  def fromStringANSI(str: UTF8String, errorContext: String = ""): Decimal = {
+  def fromStringANSI(str: UTF8String, to: DecimalType = DecimalType.USER_DEFAULT,


Suggested change

def fromStringANSI(str: UTF8String, to: DecimalType = DecimalType.USER_DEFAULT,

def fromStringANSI(

str: UTF8String,

to: DecimalType = DecimalType.USER_DEFAULT,

gengliangwang

LGTM pending tests

anchovYu · 2022-04-19T17:07:22Z

Tests: https://github.com/anchovYu/spark/actions/runs/2191065157

MaxGekk · 2022-04-19T18:04:48Z

sql/core/src/test/resources/sql-tests/results/string-functions.sql.out

@@ -1,5 +1,5 @@
 -- Automatically generated by SQLQueryTestSuite
-- Number of queries: 142
+-- Number of queries: 143


hmm, only increased the number?

This is the generated results, and did a check that 143 is the right number

MaxGekk · 2022-04-19T19:24:46Z

+1, LGTM. Merging to master.
Thank you, @anchovYu.

MaxGekk · 2022-04-19T19:25:57Z

@anchovYu Could you backport the changes to branch-3.3, please.

### What changes were proposed in this pull request? Improve the error messages for cast failures in ANSI. As mentioned in https://issues.apache.org/jira/browse/SPARK-38929, this PR targets two cast-to types: numeric types and date types. * For numeric(`int`, `smallint`, `double`, `float`, `decimal` ..) types, it embeds the cast-to types in the error message. For example, ``` Invalid input value for type INT: '1.0'. To return NULL instead, use 'try_cast'. If necessary set %s to false to bypass this error. ``` It uses the `toSQLType` and `toSQLValue` to wrap the corresponding types and literals. * For date types, it does similarly as above. For example, ``` Invalid input value for type TIMESTAMP: 'a'. To return NULL instead, use 'try_cast'. If necessary set spark.sql.ansi.enabled to false to bypass this error. ``` ### Why are the changes needed? To improve the error message in general. ### Does this PR introduce _any_ user-facing change? It changes the error messages. ### How was this patch tested? The related unit tests are updated. Closes apache#36241 from anchovYu/ansi-error-improve. Authored-by: Xinyi Yu <xinyi.yu@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit f76b3e7)

anchovYu · 2022-04-20T03:46:06Z

The cherrypick PR to 3.3: #36275

anchovYu added 4 commits April 17, 2022 18:33

update the error class and related code

9f5bd0d

Merge remote-tracking branch 'apache/master' into ansi-error-improve

622c372

use the SQLid function to wrap type with

3d4d791

date cast ansi error improvement

e592b43

github-actions bot added CORE SQL labels Apr 18, 2022

anchovYu commented Apr 18, 2022

View reviewed changes

cloud-fan reviewed Apr 18, 2022

View reviewed changes

gengliangwang reviewed Apr 18, 2022

View reviewed changes

gengliangwang mentioned this pull request Apr 18, 2022

[WIP][SPARK-38935][SQL] Improve the exception type and message of casting string to numbers #36244

Closed

MaxGekk reviewed Apr 18, 2022

View reviewed changes

anchovYu added 3 commits April 18, 2022 16:38

Merge remote-tracking branch 'apache/master' into ansi-error-improve

0f14052

address comments; use toSQLType

b45f9cb

update the error-class position

d28fb91

anchovYu requested review from MaxGekk, cloud-fan and gengliangwang April 19, 2022 01:09

gengliangwang reviewed Apr 19, 2022

View reviewed changes

address comments

45e3b81

anchovYu requested a review from gengliangwang April 19, 2022 05:36

gengliangwang reviewed Apr 19, 2022

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryErrorsBase.scala Outdated Show resolved Hide resolved

address comment

30a3b82

add several more tests

691e1e3

gengliangwang reviewed Apr 19, 2022

View reviewed changes

gengliangwang approved these changes Apr 19, 2022

View reviewed changes

address final comments

ae43c19

MaxGekk reviewed Apr 19, 2022

View reviewed changes

MaxGekk approved these changes Apr 19, 2022

View reviewed changes

MaxGekk closed this in f76b3e7 Apr 19, 2022

anchovYu mentioned this pull request Apr 20, 2022

[SPARK-38929][SQL][3.3] Improve error messages for cast failures in ANSI #36275

Closed

LuciferYang mentioned this pull request Sep 1, 2022

[SPARK-40283][INFRA] Make MiMa check default exclude private object and bump previousSparkVersion to 3.3.0 #37741

Closed

[SPARK-38929][SQL] Improve error messages for cast failures in ANSI #36241

[SPARK-38929][SQL] Improve error messages for cast failures in ANSI #36241

Uh oh!

Conversation

anchovYu commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

anchovYu commented Apr 18, 2022

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!

anchovYu commented Apr 18, 2022

Uh oh!

cloud-fan Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gengliangwang commented Apr 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 18, 2022

Uh oh!

anchovYu commented Apr 19, 2022

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!

anchovYu commented Apr 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

anchovYu commented Apr 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Apr 19, 2022

Uh oh!

MaxGekk commented Apr 19, 2022

Uh oh!

anchovYu commented Apr 20, 2022

anchovYu commented Apr 18, 2022 •

edited

Loading

cloud-fan Apr 18, 2022 •

edited

Loading