[SPARK-30292][SQL]Throw Exception when invalid string is cast to numeric type in ANSI mode #26933

iRakson · 2019-12-18T06:14:43Z

What changes were proposed in this pull request?

If spark.sql.ansi.enabled is set,
throw exception when cast to any numeric type do not follow the ANSI SQL standards.

Why are the changes needed?

ANSI SQL standards do not allow invalid strings to get casted into numeric types and throw exception for that. Currently spark sql gives NULL in such cases.

Before:
select cast('str' as decimal) => NULL

After :
select cast('str' as decimal) => invalid input syntax for type numeric: str

These results are after setting spark.sql.ansi.enabled=true

Does this PR introduce any user-facing change?

Yes. Now when ansi mode is on users will get arithmetic exception for invalid strings.

How was this patch tested?

Unit Tests Added.

iRakson · 2019-12-18T06:16:09Z

cc @cloud-fan #26518

maropu · 2019-12-18T08:03:06Z

also cc: @gengliangwang

cloud-fan · 2019-12-18T12:23:11Z

ok to test

cloud-fan · 2019-12-18T12:24:22Z

we already have an AnsiCast expression, we can implement it there.

SparkQA · 2019-12-18T14:25:38Z

Test build #115509 has finished for PR 26933 at commit 3ed7795.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AnsiSqlCastToDecimal(child: Expression, timeZoneId: Option[String])

iRakson · 2019-12-18T15:35:00Z

I will move them to AnsiCast and also look into test failures.

gengliangwang · 2019-12-18T22:31:11Z

Well, I think the issue exists in converting a String to any Numeric types (short/int/long/float/double..)
Could you fix all of them in one PR?

iRakson · 2019-12-19T04:52:32Z

Okk. I will include all of them in this PR only.

iRakson · 2019-12-19T12:23:14Z

I have moved all rules to AnsiCast for all the numeric types(short,long,byte,decimal,double,float & int).
@cloud-fan @gengliangwang Please review the code.

SparkQA · 2019-12-19T13:12:44Z

Test build #115570 has finished for PR 26933 at commit 1686c6a.

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

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

maropu · 2019-12-20T00:00:07Z

(Also, you need to add tests...)

SparkQA · 2019-12-27T14:16:12Z

Test build #115864 has finished for PR 26933 at commit 74809d0.

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

SparkQA · 2019-12-27T14:19:02Z

Test build #115862 has finished for PR 26933 at commit 69ee231.

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

iRakson · 2019-12-27T15:08:50Z

@cloud-fan @gengliangwang @maropu Kindly review the changes.

Everything is moved inside CastBase. Some test cases have been moved under CastSuite as these will fail when ansiEnabled=true. New Test cases are added under AnsiCastSuite to check this PR.

SparkQA · 2019-12-27T17:03:02Z

Test build #115866 has finished for PR 26933 at commit a336084.

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

SparkQA · 2019-12-30T14:55:10Z

Test build #115952 has finished for PR 26933 at commit c0f8baf.

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

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/float8.sql

iRakson · 2020-01-02T05:20:31Z

@cloud-fan @maropu @gengliangwang @HyukjinKwon Kindly review the changes.

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

SparkQA · 2020-01-06T15:01:23Z

Test build #116166 has finished for PR 26933 at commit 7d0faa6.

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/expressions/Cast.scala

cloud-fan · 2020-01-06T15:50:08Z

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

+        try floatStr.toFloat catch {
+          case _: NumberFormatException =>
+            val f = Cast.processFloatingPointSpecialLiterals(floatStr, true)
+            if (f == null) {


this is too much code duplication. How about unifying these 2 cases?

val f = Cast.processFloatingPointSpecialLiterals(floatStr, true) if (f == null && ansiEnabled) { throw ... } else { f }

cloud-fan · 2020-01-06T15:57:17Z

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

+            $evPrim = Float.valueOf($floatStr);
+          } catch (java.lang.NumberFormatException e) {
+            final Float f = (Float) Cast.processFloatingPointSpecialLiterals($floatStr, true);
+            if (f == null) {


nit: we can unify the code a little bit

val handleNull = if (ansiEnabled) { s"throw ..." } else { s"$evNull = true;" } ... code""" ... if (f == null) { $handleNull } else ... """

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

SparkQA · 2020-01-06T16:46:40Z

Test build #116175 has finished for PR 26933 at commit d454452.

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

cloud-fan · 2020-01-08T09:28:42Z

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

+          """
+        }
        code"""
          UTF8String.IntWrapper $wrapper = new UTF8String.IntWrapper();


ah we don't need to create int wrapper at all for ansi mode

case StringType if ansi => (c, evPrim, evNull) => s"$evPrim = $c.toByteExact();" case StringType => // the original code

iRakson · 2020-01-08T11:13:52Z

@cloud-fan Please review this once. I have made all the required changes.

SparkQA · 2020-01-08T12:06:11Z

Test build #116291 has finished for PR 26933 at commit 4b0149c.

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

cloud-fan · 2020-01-08T12:14:42Z

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

              ${changePrecision(tmp, target, evPrim, evNull, canNullSafeCast)}
            } catch (java.lang.NumberFormatException e) {
-              $evNull = true;
+              if ($ansiEnabled) {


nit: this will generate java code with if-else, we can do better

val handleException = if (ansiEnabled) { s"throw new NumberFormatException("invalid input syntax for type numeric: $c");" } else { s"$evNull =true;" } code""" ... } catch (java.lang.NumberFormatException e) { $handleException } """

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

cloud-fan

LGTM except one comment, thanks for your patience!

iRakson · 2020-01-08T13:38:19Z

LGTM except one comment, thanks for your patience!

Thanks for the all the code reviews and suggestions. :)

SparkQA · 2020-01-08T13:49:17Z

Test build #116296 has finished for PR 26933 at commit 40afc54.

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

SparkQA · 2020-01-08T13:56:01Z

Test build #116297 has finished for PR 26933 at commit 2f845c3.

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

SparkQA · 2020-01-08T16:16:47Z

Test build #116304 has finished for PR 26933 at commit 0cb4edc.

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

iRakson · 2020-01-08T16:37:46Z

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

    }
    Seq("nan", "nAn", " nan ").foreach { value =>
      checkEvaluation(cast(value, DoubleType), Double.NaN)
    }


cast('nan' as float) is working fine.
Although CAST(CAST(nan AS DECIMAL(10,0)) AS DOUBLE) is returning NaN in pgsql.

cloud-fan · 2020-01-14T09:03:26Z

thanks, merging to master!

…rs (byte/short/int/long) should fail with fraction ### What changes were proposed in this pull request? This is a followup of #26933 Fraction string like "1.23" is definitely not a valid integral format and we should fail to do the cast under the ANSI mode. ### Why are the changes needed? correct the ANSI cast behavior from string to integral ### Does this PR introduce any user-facing change? Yes under ANSI mode, but ANSI mode is off by default. ### How was this patch tested? new test Closes #27957 from cloud-fan/ansi. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>

…rs (byte/short/int/long) should fail with fraction ### What changes were proposed in this pull request? This is a followup of #26933 Fraction string like "1.23" is definitely not a valid integral format and we should fail to do the cast under the ANSI mode. ### Why are the changes needed? correct the ANSI cast behavior from string to integral ### Does this PR introduce any user-facing change? Yes under ANSI mode, but ANSI mode is off by default. ### How was this patch tested? new test Closes #27957 from cloud-fan/ansi. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]> (cherry picked from commit ac262cb) Signed-off-by: Takeshi Yamamuro <[email protected]>

…rs (byte/short/int/long) should fail with fraction ### What changes were proposed in this pull request? This is a followup of apache#26933 Fraction string like "1.23" is definitely not a valid integral format and we should fail to do the cast under the ANSI mode. ### Why are the changes needed? correct the ANSI cast behavior from string to integral ### Does this PR introduce any user-facing change? Yes under ANSI mode, but ANSI mode is off by default. ### How was this patch tested? new test Closes apache#27957 from cloud-fan/ansi. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>

Cast to Decimal

3ed7795

iRakson changed the title ~~Throw Exception when invalid string is cast to decimal in ANSI mode~~ [SPARK-30292][SQL]Throw Exception when invalid string is cast to decimal in ANSI mode Dec 18, 2019

Fix Review Comments

1686c6a

cloud-fan reviewed Dec 19, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ANSISQLStandard.scala Outdated Show resolved Hide resolved

dongjoon-hyun added the SQL label Dec 19, 2019

iRakson added 2 commits December 27, 2019 19:13

Fix

69ee231

Fix

74809d0

Fix

a336084

iRakson requested a review from cloud-fan December 27, 2019 15:03

Test Cases Fix.

c0f8baf

iRakson commented Dec 30, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/float8.sql Outdated Show resolved Hide resolved

iRakson changed the title ~~[SPARK-30292][SQL]Throw Exception when invalid string is cast to decimal in ANSI mode~~ [SPARK-30292][SQL]Throw Exception when invalid string is cast to numeric type in ANSI mode Dec 31, 2019

cloud-fan reviewed Jan 2, 2020

View reviewed changes

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

cloud-fan reviewed Jan 6, 2020

View reviewed changes

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

cloud-fan reviewed Jan 6, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/float4.sql.out Show resolved Hide resolved

Fix

4b0149c

cloud-fan reviewed Jan 8, 2020

View reviewed changes

iRakson added 2 commits January 8, 2020 16:35

Fix

40afc54

Fix

2f845c3

iRakson requested a review from cloud-fan January 8, 2020 11:13

cloud-fan reviewed Jan 8, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/window_part4.sql.out Show resolved Hide resolved

cloud-fan approved these changes Jan 8, 2020

View reviewed changes

Fix

0cb4edc

iRakson commented Jan 8, 2020

View reviewed changes

iRakson mentioned this pull request Jan 9, 2020

[SPARK-30472][SQL] ANSI SQL: Throw exception on format invalid and overflow when casting String to IntegerType. #27152

Closed

cloud-fan closed this in e0efd21 Jan 14, 2020

cloud-fan mentioned this pull request Mar 19, 2020

[SPARK-30292][SQL][FOLLOWUP] ansi cast from strings to integral numbers (byte/short/int/long) should fail with fraction #27957

Closed

[SPARK-30292][SQL]Throw Exception when invalid string is cast to numeric type in ANSI mode #26933

[SPARK-30292][SQL]Throw Exception when invalid string is cast to numeric type in ANSI mode #26933

Uh oh!

Conversation

iRakson commented Dec 18, 2019 • 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!

iRakson commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Dec 18, 2019

Uh oh!

cloud-fan commented Dec 18, 2019

Uh oh!

cloud-fan commented Dec 18, 2019

Uh oh!

SparkQA commented Dec 18, 2019

Uh oh!

iRakson commented Dec 18, 2019

Uh oh!

gengliangwang commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iRakson commented Dec 19, 2019

Uh oh!

iRakson commented Dec 19, 2019

Uh oh!

SparkQA commented Dec 19, 2019

Uh oh!

Uh oh!

maropu commented Dec 20, 2019

Uh oh!

SparkQA commented Dec 27, 2019

Uh oh!

SparkQA commented Dec 27, 2019

Uh oh!

iRakson commented Dec 27, 2019

Uh oh!

SparkQA commented Dec 27, 2019

Uh oh!

SparkQA commented Dec 30, 2019

Uh oh!

Uh oh!

iRakson commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jan 6, 2020

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!

Uh oh!

SparkQA commented Jan 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iRakson commented Jan 8, 2020

Uh oh!

SparkQA commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

iRakson commented Dec 18, 2019 •

edited

Loading

iRakson commented Dec 18, 2019 •

edited

Loading

gengliangwang commented Dec 18, 2019 •

edited

Loading

iRakson commented Jan 2, 2020 •

edited

Loading