[SPARK-33496][SQL]Improve error message of ANSI explicit cast #30440

gengliangwang · 2020-11-20T04:35:23Z

What changes were proposed in this pull request?

After #30260, there are some type conversions disallowed under ANSI mode.
We should tell users what they can do if they have to use the disallowed casting.

Why are the changes needed?

Make it more user-friendly.

Does this PR introduce any user-facing change?

Yes, the error message is improved on casting failure when ANSI mode is enabled

How was this patch tested?

Unit tests.

gengliangwang · 2020-11-20T04:36:40Z

Since there is no alternative function for converting timestamp to numeric, I will work on another one before continuing this one.

SparkQA · 2020-11-20T04:42:45Z

Test build #131399 has finished for PR 30440 at commit 18cecbb.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-23T17:36:26Z

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

SparkQA · 2020-11-23T18:08:39Z

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

SparkQA · 2020-11-23T21:59:02Z

Test build #131559 has finished for PR 30440 at commit 1100cc6.

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

SparkQA · 2020-11-24T01:55:26Z

Test build #131593 has finished for PR 30440 at commit 1100cc6.

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

cloud-fan · 2020-11-24T07:16:35Z

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

+      // scalastyle:off line.size.limit
+      s"""
+         | cannot cast ${from.catalogString} to ${to.catalogString},
+         | you can enable the casting by setting ${SQLConf.LEGACY_ALLOW_CAST_NUMERIC_TO_TIMESTAMP.key}


shall we remove LEGACY_ALLOW_CAST_NUMERIC_TO_TIMESTAMP? It's true by default and I don't see why it's needed since we have ansi mode.

+1, we can have another PR for it

Can we do it first? then we don't need to change this part again.

Sure, I have just created #30493

maropu

Minor comments and the other parts look fine.

maropu · 2020-11-24T07:14:53Z

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

+      s"""
+         | cannot cast ${from.catalogString} to ${to.catalogString},
+         | you can enable the casting by setting ${SQLConf.LEGACY_ALLOW_CAST_NUMERIC_TO_TIMESTAMP.key}
+         | to true, but we strongly recommend using function TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS instead.


nit: function -> functions?

maropu · 2020-11-24T07:21:45Z

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

  override def canCast(from: DataType, to: DataType): Boolean = AnsiCast.canCast(from, to)
+
+  // For now, this expression is only used in table insertion.
+  // If there are more scenarios for this expression,  we should update the error message on type


nit: a unnecessary space found in expression, we

maropu · 2020-11-24T07:29:47Z

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

+        // scalastyle:off line.size.limit
+        s"""
+           | cannot cast ${from.catalogString} to ${from.catalogString}.
+           | We strongly recommend using function TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS instead.


How about describing it like To convert values from ${...} to ${...}, you can use functions TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS instead. ?

maropu · 2020-11-24T07:30:05Z

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

+      case (_: NumericType, TimestampType) =>
+        // scalastyle:off line.size.limit
+        s"""
+           | cannot cast ${from.catalogString} to ${from.catalogString}.


from.catalogString -> to.catalogString for the latter one.

gengliangwang · 2020-11-24T11:13:21Z

@cloud-fan @maropu Thanks for the suggestions. I have addressed them.

SparkQA · 2020-11-24T12:53:26Z

Test build #131652 has finished for PR 30440 at commit e762162.

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

gengliangwang · 2020-11-24T13:30:17Z

retest this please.

SparkQA · 2020-11-24T18:06:18Z

Test build #131664 has finished for PR 30440 at commit e762162.

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

gengliangwang · 2020-11-25T02:17:55Z

retest this please.

SparkQA · 2020-11-25T06:10:06Z

Test build #131721 has finished for PR 30440 at commit e762162.

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

HyukjinKwon · 2020-11-25T07:40:12Z

retest this please.

SparkQA · 2020-11-25T11:02:22Z

Test build #131756 has finished for PR 30440 at commit e762162.

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

gengliangwang · 2020-11-25T11:14:58Z

retest this please.

cloud-fan · 2020-11-25T13:06:51Z

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

+      case (_: ArrayType, StringType) =>
+        s"""
+           | cannot cast ${from.catalogString} to ${to.catalogString} with ANSI mode on.
+           | If you have to cast ${from.catalogString} to ${to.catalogString}, you can use the function array_join or set $fallbackConfKey as $fallbackConfValue.


upper case the function name?

SparkQA · 2020-11-25T13:51:45Z

Test build #131764 has finished for PR 30440 at commit bb5b219.

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

gengliangwang · 2020-11-25T15:14:11Z

@maropu @cloud-fan Thanks for the review!
Merging to master

SparkQA · 2020-11-25T15:39:44Z

Test build #131772 has finished for PR 30440 at commit bb5b219.

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

AmplabJenkins · 2020-11-25T16:26:25Z

Build finished.

AmplabJenkins · 2020-11-25T16:26:26Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36375/

SparkQA · 2020-11-25T16:39:56Z

Test build #131777 has finished for PR 30440 at commit acfa5d0.

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

AmplabJenkins · 2020-11-25T17:05:01Z

Build finished.

AmplabJenkins · 2020-11-25T17:05:02Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/131777/

github-actions bot added the SQL label Nov 20, 2020

gengliangwang changed the title ~~[WIP] [SPARK-33496][SQL]Improve error message of ANSI explicit cast~~ [SPARK-33496][SQL]Improve error message of ANSI explicit cast Nov 24, 2020

gengliangwang requested a review from cloud-fan November 24, 2020 03:29

cloud-fan reviewed Nov 24, 2020

View reviewed changes

maropu reviewed Nov 24, 2020

View reviewed changes

improve error msg

bb5b219

gengliangwang force-pushed the improveAnsiCastErrorMSG branch from e762162 to bb5b219 Compare November 25, 2020 09:16

cloud-fan reviewed Nov 25, 2020

View reviewed changes

cloud-fan approved these changes Nov 25, 2020

View reviewed changes

uppercase

acfa5d0

maropu approved these changes Nov 25, 2020

View reviewed changes

gengliangwang closed this in d691d85 Nov 25, 2020

[SPARK-33496][SQL]Improve error message of ANSI explicit cast #30440

[SPARK-33496][SQL]Improve error message of ANSI explicit cast #30440

Uh oh!

Conversation

gengliangwang commented Nov 20, 2020 • 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!

gengliangwang commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

SparkQA commented Nov 23, 2020

Uh oh!

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

maropu left a comment

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 Nov 24, 2020

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

gengliangwang commented Nov 24, 2020

Uh oh!

SparkQA commented Nov 24, 2020

Uh oh!

gengliangwang commented Nov 25, 2020

Uh oh!

SparkQA commented Nov 25, 2020

Uh oh!

HyukjinKwon commented Nov 25, 2020

Uh oh!

SparkQA commented Nov 25, 2020

Uh oh!

gengliangwang commented Nov 25, 2020

Uh oh!

cloud-fan Nov 25, 2020 • 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!

SparkQA commented Nov 25, 2020

Uh oh!

gengliangwang commented Nov 25, 2020

Uh oh!

SparkQA commented Nov 25, 2020

Uh oh!

AmplabJenkins commented Nov 25, 2020

Uh oh!

AmplabJenkins commented Nov 25, 2020

Uh oh!

SparkQA commented Nov 25, 2020

Uh oh!

AmplabJenkins commented Nov 25, 2020

Uh oh!

AmplabJenkins commented Nov 25, 2020

Uh oh!

gengliangwang commented Nov 20, 2020 •

edited

Loading

cloud-fan Nov 25, 2020 •

edited

Loading