[SPARK-24586][SQL] Upcast should not allow casting from string to other types #21586

cloud-fan · 2018-06-18T23:24:37Z

What changes were proposed in this pull request?

When turning a Dataset to another Dataset, Spark will up cast the fields in the original Dataset to the type of corresponding fields in the target DataSet.

However, the current upcast behavior is a little weird, we don't allow up casting from string to numeric, but allow non-numeric types as the target, like boolean, date, etc.

As a result, Seq("str").toDS.as[Int] fails, but Seq("str").toDS.as[Boolean] works and throw NPE during execution.

The motivation of the up cast is to prevent things like runtime NPE, it's more reasonable to make up cast stricter.

This PR does 2 things:

rename Cast.canSafeCast to Cast.canUpcast, and support complex typres
remove Cast.mayTruncate and replace it with !Cast.canUpcast

Note that, the up cast change also affects persistent view resolution. But since we don't support changing column types of an existing table, there is no behavior change here.

How was this patch tested?

new tests

cloud-fan · 2018-06-18T23:24:54Z

cc @marmbrus @viirya @gatorsmile

SparkQA · 2018-06-19T01:52:28Z

Test build #92052 has finished for PR 21586 at commit c89d12e.

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

maropu · 2018-06-19T02:26:51Z

retest this please

viirya · 2018-06-19T02:49:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

@@ -2299,7 +2299,7 @@ class Analyzer(
        case u @ UpCast(child, _, _) if !child.resolved => u

        case UpCast(child, dataType, walkedTypePath)
-          if Cast.mayTruncate(child.dataType, dataType) =>
+          if !Cast.canSafeCast(child.dataType, dataType) =>
          fail(child, dataType, walkedTypePath)


Is the error message of thrown AnalysisException valid under this change? Seems it is not just because truncating?

maropu · 2018-06-19T02:50:11Z

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

    val fromPrecedence = TypeCoercion.numericPrecedence.indexOf(from)
    val toPrecedence = TypeCoercion.numericPrecedence.indexOf(to)
-    toPrecedence > 0 && fromPrecedence > toPrecedence
+    fromPrecedence >= 0 && fromPrecedence < toPrecedence


super nit: fromPrecedence != -1 && fromPrecedence < toPrecedence is more easy-to-understand?

viirya · 2018-06-19T03:06:34Z

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

-    case (TimestampType, DateType) => true
-    case (StringType, to: NumericType) => true
+  def canSafeCast(from: DataType, to: DataType): Boolean = (from, to) match {
+    case _ if from == to => true


Seems nullable of StructField can affect the casting decision?

added support for complex types

SparkQA · 2018-06-19T04:27:41Z

Test build #92059 has finished for PR 21586 at commit c89d12e.

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

…is wrapped in Cast ## What changes were proposed in this pull request? As mentioned in #21586 , `Cast.mayTruncate` is not 100% safe, string to boolean is allowed. Since changing `Cast.mayTruncate` also changes the behavior of Dataset, here I propose to add a new `Cast.canSafeCast` for partition pruning. ## How was this patch tested? new test cases Author: Wenchen Fan <wenchen@databricks.com> Closes #21712 from cloud-fan/safeCast.

SparkQA · 2018-08-03T07:05:01Z

Test build #94103 has finished for PR 21586 at commit c89d12e.

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

SparkQA · 2019-04-11T14:00:21Z

Test build #104512 has finished for PR 21586 at commit 2a64d59.

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

SparkQA · 2019-04-11T15:16:37Z

Test build #104510 has finished for PR 21586 at commit cde33ca.

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

SparkQA · 2019-04-11T18:00:27Z

Test build #104518 has finished for PR 21586 at commit 93f43ad.

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

SparkQA · 2019-04-15T18:10:20Z

Test build #104589 has finished for PR 21586 at commit 13a7846.

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

cloud-fan · 2019-05-15T06:02:10Z

retest this please

SparkQA · 2019-05-15T07:05:01Z

Test build #105401 has finished for PR 21586 at commit 7c6d9a8.

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

SparkQA · 2019-05-15T07:05:01Z

Test build #105398 has finished for PR 21586 at commit 13a7846.

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

SparkQA · 2019-05-15T07:05:02Z

Test build #105400 has finished for PR 21586 at commit a199906.

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

SparkQA · 2019-05-15T10:44:37Z

Test build #105408 has finished for PR 21586 at commit cf8d05f.

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

gatorsmile · 2019-05-16T18:34:22Z

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

   */
-  def mayTruncate(from: DataType, to: DataType): Boolean = (from, to) match {


I know this will be ugly, but I still prefer to adding a conf for falling back to the original behavior.

SparkQA · 2019-05-17T15:57:26Z

Test build #105490 has finished for PR 21586 at commit 14e3de3.

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

SparkQA · 2019-05-17T17:08:59Z

Test build #105486 has finished for PR 21586 at commit e558ad4.

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

SparkQA · 2019-05-20T07:05:03Z

Test build #105551 has finished for PR 21586 at commit a638a53.

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

cloud-fan · 2019-05-21T06:24:12Z

retest this please

gatorsmile

LGTM

SparkQA · 2019-05-21T07:05:02Z

Test build #105598 has finished for PR 21586 at commit a638a53.

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

cloud-fan · 2019-05-21T13:25:41Z

retest this please

SparkQA · 2019-05-21T16:54:23Z

Test build #105618 has finished for PR 21586 at commit a638a53.

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

cloud-fan · 2019-05-22T03:36:08Z

thanks, merging to master!

dongjoon-hyun · 2019-05-22T04:30:42Z

cc @dbtsai

manuzhang · 2022-02-14T10:23:20Z

docs/sql-migration-guide-upgrade.md

@@ -138,6 +138,8 @@ license: |
    need to specify a value with units like "30s" now, to avoid being interpreted as milliseconds; otherwise, 
    the extremely short interval that results will likely cause applications to fail.

+  - When turning a Dataset to another Dataset, Spark will up cast the fields in the original Dataset to the type of corresponding fields in the target DataSet. In version 2.4 and earlier, this up cast is not very strict, e.g. `Seq("str").toDS.as[Int]` fails, but `Seq("str").toDS.as[Boolean]` works and throw NPE during execution. In Spark 3.0, the up cast is stricter and turning String into something else is not allowed, i.e. `Seq("str").toDS.as[Boolean]` will fail during analysis.


@cloud-fan shall we mention setting spark.sql.legacy.looseUpcast=true to preserve old behavior?

Yeah we should

are you interested in creating a PR (with a new JIRA) to fix the doc? 2.4 is EOL but I think it's good to fix the docs anyway.

Sure. I will create one.

#35519 has been created.

viirya reviewed Jun 19, 2018

View reviewed changes

maropu reviewed Jun 19, 2018

View reviewed changes

viirya reviewed Jun 19, 2018

View reviewed changes

cloud-fan mentioned this pull request Jul 4, 2018

[SPARK-22384][SQL][followup] Refine partition pruning when attribute is wrapped in Cast #21712

Closed

cloud-fan mentioned this pull request Jul 5, 2018

[SPARK-24251][SQL] Add AppendData logical plan. #21305

Closed

cloud-fan force-pushed the cast branch 2 times, most recently from cde33ca to 2a64d59 Compare April 11, 2019 11:39

Upcast should not allow casting from string to other types

93f43ad

cloud-fan force-pushed the cast branch from 2a64d59 to 93f43ad Compare April 11, 2019 14:32

fix a case

13a7846

cloud-fan added 3 commits May 15, 2019 14:11

improve comment

a199906

update view as well

7c6d9a8

support complex types

cf8d05f

gatorsmile reviewed May 16, 2019

View reviewed changes

add a legacy config

e558ad4

Merge branch 'master' into cast

14e3de3

Merge branch 'master' into cast

a638a53

gatorsmile reviewed May 21, 2019

View reviewed changes

cloud-fan closed this in 03c9e8a May 22, 2019

manuzhang reviewed Feb 14, 2022

View reviewed changes

		*/
		def mayTruncate(from: DataType, to: DataType): Boolean = (from, to) match {

[SPARK-24586][SQL] Upcast should not allow casting from string to other types #21586

[SPARK-24586][SQL] Upcast should not allow casting from string to other types #21586

Conversation

cloud-fan commented Jun 18, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

cloud-fan commented Jun 18, 2018

SparkQA commented Jun 19, 2018

maropu commented Jun 19, 2018

viirya Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jun 19, 2018

SparkQA commented Aug 3, 2018

SparkQA commented Apr 11, 2019

SparkQA commented Apr 11, 2019

SparkQA commented Apr 11, 2019

SparkQA commented Apr 15, 2019

cloud-fan commented May 15, 2019

SparkQA commented May 15, 2019

SparkQA commented May 15, 2019

SparkQA commented May 15, 2019

SparkQA commented May 15, 2019

Choose a reason for hiding this comment

SparkQA commented May 17, 2019

SparkQA commented May 17, 2019

SparkQA commented May 20, 2019

cloud-fan commented May 21, 2019

gatorsmile left a comment

Choose a reason for hiding this comment

SparkQA commented May 21, 2019

cloud-fan commented May 21, 2019

SparkQA commented May 21, 2019

cloud-fan commented May 22, 2019

dongjoon-hyun commented May 22, 2019

manuzhang Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jun 18, 2018 •

edited

Loading

viirya Jun 19, 2018 •

edited

Loading

cloud-fan Apr 11, 2019 •

edited

Loading

manuzhang Feb 14, 2022 •

edited

Loading