[SPARK-34769][SQL]AnsiTypeCoercion: return closest convertible type among TypeCollection #31859

gengliangwang · 2021-03-17T04:58:20Z

What changes were proposed in this pull request?

Currently, when implicit casting a data type to a TypeCollection, Spark returns the first convertible data type among TypeCollection.
In ANSI mode, we can make the behavior more reasonable by returning the closet convertible data type in TypeCollection.

In details, we first try to find the all the expected types we can implicitly cast:

if there is no convertible data types, return None;
if there is only one convertible data type, cast input as it;
otherwise if there are multiple convertible data types, find the closet data
type among them. If there is no such closet data type, return None.

Note that if the closet type is Float type and the convertible types contains Double type, simply return Double type as the closet type to avoid potential
precision loss on converting the Integral type as Float type.

Why are the changes needed?

Make the type coercion rule for TypeCollection more reasonable and ANSI compatible.
E.g. returning Long instead of Double forimplicast(int, TypeCollect(Double, Long)).

From ANSI SQL Spec section 4.33 "SQL-invoked routines"

Section 9.6 "Subject routine determination"

Section 10.4 "routine invocation"

Does this PR introduce any user-facing change?

Yes, in ANSI mode, implicit casting to a TypeCollection returns the narrowest convertible data type instead of the first convertible one.

How was this patch tested?

Unit tests.

gengliangwang · 2021-03-17T06:08:37Z

BTW I will add Spark document for the ANSI Type Coercion after this one.

SparkQA · 2021-03-17T07:31:06Z

Test build #136138 has finished for PR 31859 at commit 13f774a.

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

maropu · 2021-03-17T07:45:32Z

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

+          val narrowestCommonType = convertibleTypes.find { dt =>
+            convertibleTypes.forall { target =>
+              implicitCast(dt, target, isInputFoldable = false).isDefined
+            }
+          }


It seems there is no explicit definition of narrowest data types. I'm not 100% sure about this logic though, does the find method (finding a first data type) works fine for any input?

@maropu I have updated the related context in PR description. This part is quite complicated.

does the find method (finding a first data type) works fine for any input?

The logic here is to find a data type that can be converted to all other convertible types. If there is one, it is the closest convertible type.

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

cloud-fan · 2021-03-18T13:31:50Z

Shall we simply fail if there are multiple matches in the type collection? It's a bit tricky to define "closest"

gengliangwang · 2021-03-18T14:04:06Z

Shall we simply fail if there are multiple matches in the type collection? It's a bit tricky to define "closest"

That sounds not reasonable. E.g., failing an input of Integer type for TypeCollection[Long, Double].

cloud-fan · 2021-03-18T14:16:50Z

Is there any function using TypeCollection[Long, Double]?

gengliangwang · 2021-03-18T14:30:39Z

Is there any function using TypeCollection[Long, Double]?

There are similar expressions:
Divide: TypeCollection(DoubleType, DecimalType)
IntegralDivide: TypeCollection(LongType, DecimalType)

cloud-fan · 2021-03-18T14:31:19Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercionSuite.scala

+    shouldNotCastStringInput(TypeCollection(NumericType, BinaryType))
+    // When there are multiple convertible types in the `TypeCollection` and there is no closest
+    // convertible data type among the convertible types.
+    shouldNotCastStringLiteral(TypeCollection(NumericType, BinaryType))


This duplicates the previous case.

This one is for String Literal, which can convert to any of (NumericType, BinaryType). The previous one is for String type column, which can't do the conversion.

SparkQA · 2021-03-24T04:55:32Z

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

SparkQA · 2021-03-24T05:02:46Z

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

SparkQA · 2021-03-24T05:05:31Z

Test build #136434 has finished for PR 31859 at commit 60c4684.

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

SparkQA · 2021-03-24T06:15:37Z

Test build #136439 has started for PR 31859 at commit 4cea569.

cloud-fan · 2021-03-24T06:50:32Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercionSuite.scala

+    // When there are multiple convertible types in the `TypeCollection`, use the closet convertible
+    // data type among convertible types.
+    shouldCast(IntegerType, TypeCollection(BinaryType, FloatType, LongType), LongType)
+    shouldCast(IntegerType, TypeCollection(BinaryType, LongType, NumericType), IntegerType)


This doesn't go to the code path to pick the closest convertible type. This hits case _ if expectedType.acceptsType(inType) => Some(inType)

cloud-fan · 2021-03-24T06:50:42Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercionSuite.scala

@@ -377,10 +368,31 @@ class AnsiTypeCoercionSuite extends AnalysisTest {
      ArrayType(StringType, true),
      TypeCollection(ArrayType(StringType), StringType),
      ArrayType(StringType, true))
+
+    // When there are multiple convertible types in the `TypeCollection`, use the closet convertible


nit: closet -> closest

cloud-fan · 2021-03-24T06:52:10Z

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

@@ -73,8 +73,8 @@ select left(null, -2), left("abcd", -2), left("abcd", 0), left("abcd", 'a')
 -- !query schema
 struct<>
 -- !query output
-java.lang.NumberFormatException
-invalid input syntax for type numeric: a


can we move left(null, -2) out to a new query? otherwise we can't test the error message of left("abcd", 'a')

cloud-fan · 2021-03-24T06:52:31Z

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

@@ -92,7 +92,7 @@ select right(null, -2), right("abcd", -2), right("abcd", 0), right("abcd", 'a')
 struct<>
 -- !query output
 org.apache.spark.sql.AnalysisException
-cannot resolve 'substring('abcd', (- CAST('a' AS DOUBLE)), 2147483647)' due to data type mismatch: argument 2 requires int type, however, '(- CAST('a' AS DOUBLE))' is of double type.; line 1 pos 61
+cannot resolve 'substring(NULL, (- -2), 2147483647)' due to data type mismatch: argument 1 requires (string or binary) type, however, 'NULL' is of null type.; line 1 pos 7


SparkQA · 2021-03-24T07:47:37Z

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

SparkQA · 2021-03-24T07:58:40Z

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

SparkQA · 2021-03-24T11:30:13Z

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

SparkQA · 2021-03-24T11:38:17Z

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

cloud-fan · 2021-03-24T15:04:01Z

thanks, merging to master!

SparkQA · 2021-03-24T15:04:33Z

Test build #136448 has finished for PR 31859 at commit a72dcfb.

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

maropu · 2021-03-24T23:29:29Z

late lgtm.

HyukjinKwon · 2021-03-29T03:22:21Z

I just found out that I mistakenly assigned it to myself .. I removed it back now ..

gengliangwang added 3 commits March 17, 2021 00:04

update handling of typecollection

ae82d44

use the result as representation

f2dc93e

update tests

13f774a

github-actions bot added the SQL label Mar 17, 2021

gengliangwang changed the title ~~[SPARK-34769][SQL]AnsiTypeCoercion: return narrowest convertible type among TypeCollection~~ [SPARK-34769][SQL]AnsiTypeCoercion: return closest convertible type among TypeCollection Mar 17, 2021

revise comments

9900d29

maropu reviewed Mar 17, 2021

View reviewed changes

fix tests

73e7237

maropu reviewed Mar 17, 2021

View reviewed changes

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

HyukjinKwon self-assigned this Mar 18, 2021

gengliangwang requested a review from cloud-fan March 18, 2021 12:40

cloud-fan reviewed Mar 18, 2021

View reviewed changes

add more test cases

60c4684

fix test case

4cea569

cloud-fan reviewed Mar 24, 2021

View reviewed changes

address comments

a72dcfb

cloud-fan closed this in abfd9b2 Mar 24, 2021

HyukjinKwon removed their assignment Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34769][SQL]AnsiTypeCoercion: return closest convertible type among TypeCollection #31859

[SPARK-34769][SQL]AnsiTypeCoercion: return closest convertible type among TypeCollection #31859

gengliangwang commented Mar 17, 2021 •

edited

gengliangwang commented Mar 17, 2021

SparkQA commented Mar 17, 2021

maropu Mar 17, 2021 •

edited

gengliangwang Mar 17, 2021

gengliangwang Mar 17, 2021

cloud-fan commented Mar 18, 2021

gengliangwang commented Mar 18, 2021

cloud-fan commented Mar 18, 2021

gengliangwang commented Mar 18, 2021

cloud-fan Mar 18, 2021

gengliangwang Mar 19, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

cloud-fan Mar 24, 2021

gengliangwang Mar 24, 2021

cloud-fan Mar 24, 2021

cloud-fan Mar 24, 2021

cloud-fan Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

cloud-fan commented Mar 24, 2021

SparkQA commented Mar 24, 2021

maropu commented Mar 24, 2021

HyukjinKwon commented Mar 29, 2021

[SPARK-34769][SQL]AnsiTypeCoercion: return closest convertible type among TypeCollection #31859

[SPARK-34769][SQL]AnsiTypeCoercion: return closest convertible type among TypeCollection #31859

Conversation

gengliangwang commented Mar 17, 2021 • edited

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?

gengliangwang commented Mar 17, 2021

SparkQA commented Mar 17, 2021

maropu Mar 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Mar 18, 2021

gengliangwang commented Mar 18, 2021

cloud-fan commented Mar 18, 2021

gengliangwang commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

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

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

SparkQA commented Mar 24, 2021

cloud-fan commented Mar 24, 2021

SparkQA commented Mar 24, 2021

maropu commented Mar 24, 2021

HyukjinKwon commented Mar 29, 2021

gengliangwang commented Mar 17, 2021 •

edited

maropu Mar 17, 2021 •

edited