[SPARK-21811][SQL] Fix the inconsistency behavior when finding the widest common type #21074

jiangxb1987 · 2018-04-15T15:27:12Z

What changes were proposed in this pull request?

Currently we find the wider common type by comparing the two types from left to right, this can be a problem when you have two data types which don't have a common type but each can be promoted to StringType.

For instance, if you have a table with the schema:
[c1: date, c2: string, c3: int]

The following succeeds:
SELECT coalesce(c1, c2, c3) FROM table

While the following produces an exception:
SELECT coalesce(c1, c3, c2) FROM table

This is only a issue when the seq of dataTypes contains StringType and all the types can do string promotion.

close #19033

How was this patch tested?

Add test in TypeCoercionSuite

SparkQA · 2018-04-15T19:21:20Z

Test build #89376 has finished for PR 21074 at commit 803a6a4.

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

jiangxb1987 · 2018-04-16T02:37:08Z

cc @HyukjinKwon @gatorsmile @cloud-fan

cloud-fan · 2018-04-16T04:23:50Z

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

    types.foldLeft[Option[DataType]](Some(NullType))((r, c) => r match {
      case Some(d) => findWiderTypeForTwo(d, c)
-      case None => None
+      // Currently we find the wider common type by comparing the two types from left to right,


The real problem is, findWiderTypeForTwo doesn't satisfy the associative law, i.e. (a op b) op c may not equal to a op (b op c). I think StringType is the only exception here, it's more clear to do

val (stringType, nonStringType) = types.partition(_ == StringType) (stringType.distinct ++ nonStringType).foldLeft...

This is a behavior change. We need to make it configurable. Add a conf and update the migration guide.

cloud-fan · 2018-04-17T14:49:17Z

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

-      case _ => None
-    })
+    // `findWiderTypeForTwo` doesn't satisfy the associative law, i.e. (a op b) op c may not equal
+    // to a op (b op c). This is only a problem when each of the types is StringType or can be


This is only a problem for StringType. Excluding StringType, findWiderTypeForTwo satisfies the associative law

SparkQA · 2018-04-17T17:19:59Z

Test build #89463 has finished for PR 21074 at commit d77f213.

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

SparkQA · 2018-04-17T18:08:51Z

Test build #89459 has finished for PR 21074 at commit 571912f.

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

HyukjinKwon · 2018-04-18T01:11:13Z

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

+    // to a op (b op c). This is only a problem for StringType. Excluding StringType,
+    // findWiderTypeForTwo satisfies the associative law. For instance, (TimestampType,
+    // IntegerType, StringType) should have StringType as the wider common type.
+    val (stringTypes, nonStringTypes) = types.partition(_ == StringType)


Out of curiosity, does this work with array types too (array of string vs array of non string types)?

It's expected to , let me also fix it for array types. Thanks!

cloud-fan · 2018-04-18T03:46:22Z

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

+    // findWiderTypeForTwo satisfies the associative law. For instance, (TimestampType,
+    // IntegerType, StringType) should have StringType as the wider common type.
+    val (stringTypes, nonStringTypes) = types.partition { t =>
+      t == StringType || t == ArrayType(StringType)


we need something like

def hasStringType(dt: DataType): Boolean = dt match { case StringType => true case ArrayType(et, _) => hasStringType(et) // Add StructType if we support string promotion for struct fields in the future. case _ => false }

SparkQA · 2018-04-18T06:21:08Z

Test build #89478 has finished for PR 21074 at commit 4ce5081.

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

cloud-fan · 2018-04-18T14:56:07Z

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

-      case None => None
-    })
+    // findWiderTypeForTwo doesn't satisfy the associative law, i.e. (a op b) op c may not equal
+    // to a op (b op c). This is only a problem for StringType. Excluding StringType,


This is only a problem for StringType or nested StringType in ArrayType. Excluding these types, ...

gatorsmile · 2018-04-18T16:09:14Z

docs/sql-programming-guide.md

 - Since Spark 2.4, writing a dataframe with an empty or nested empty schema using any file formats (parquet, orc, json, text, csv etc.) is not allowed. An exception is thrown when attempting to write dataframes with empty schema. 
 - Since Spark 2.4, Spark compares a DATE type with a TIMESTAMP type after promotes both sides to TIMESTAMP. To set `false` to `spark.sql.hive.compareDateTimestampInTimestamp` restores the previous behavior. This option will be removed in Spark 3.0.
 - Since Spark 2.4, creating a managed table with nonempty location is not allowed. An exception is thrown when attempting to create a managed table with nonempty location. To set `true` to `spark.sql.allowCreatingManagedTableUsingNonemptyLocation` restores the previous behavior. This option will be removed in Spark 3.0.
+ - Since Spark 2.4, finding the widest common type for the arguments of a variadic function(e.g. IN/COALESCE) should always success when each of the types of arguments is either StringType or can be promoted to StringType. Previously this may throw an exception for some specific arguments ordering.


Since Spark 2.4, the type coercion rules can automatically promote the argument types of the variadic SQL functions (e.g., IN/COALESCE) to the widest common type, no matter how the input arguments order. In prior Spark versions, the promotion could fail in some specific orders (e.g., TimestampType, IntegerType and StringType) and throw an exception.

SparkQA · 2018-04-18T17:46:14Z

Test build #89508 has finished for PR 21074 at commit 55eefe0.

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

SparkQA · 2018-04-18T17:58:50Z

Test build #89519 has finished for PR 21074 at commit ff514af.

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

SparkQA · 2018-04-18T19:44:36Z

Test build #89516 has finished for PR 21074 at commit c2abce2.

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

gatorsmile · 2018-04-18T23:11:53Z

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

+    ruleTest(rule,
+      Coalesce(Seq(tsArrayLit, intArrayLit, strArrayLit)),
+      Coalesce(Seq(Cast(tsArrayLit, ArrayType(StringType)),
+        Cast(intArrayLit, ArrayType(StringType)), Cast(strArrayLit, ArrayType(StringType)))))


Could you add an end to end test case that can trigger this?

We usually don't add end-to-end tests for type coercion changes, as the type coercion logic is pretty isolated, it's very unlikely that we can pass the unit test but not end-to-end test.

cloud-fan · 2018-04-19T07:15:04Z

thanks, merging to master!

HyukjinKwon · 2018-04-19T13:20:57Z

Seems missed out. I merged this to master.

cloud-fan · 2018-04-20T03:31:39Z

@HyukjinKwon thanks! I encountered some network issue and forgot to come back and merge this...

fix type coercion when promting to StringType

803a6a4

cloud-fan reviewed Apr 16, 2018

View reviewed changes

refactor and update migration guide.

571912f

cloud-fan reviewed Apr 17, 2018

View reviewed changes

update comments

d77f213

HyukjinKwon reviewed Apr 18, 2018

View reviewed changes

fix also for array types

4ce5081

cloud-fan reviewed Apr 18, 2018

View reviewed changes

refactor

55eefe0

cloud-fan reviewed Apr 18, 2018

View reviewed changes

update comments

c2abce2

gatorsmile reviewed Apr 18, 2018

View reviewed changes

update migration guide

ff514af

gatorsmile reviewed Apr 18, 2018

View reviewed changes

asfgit closed this in d96c3e3 Apr 19, 2018

jiangxb1987 deleted the typeCoercion branch April 23, 2018 07:25

[SPARK-21811][SQL] Fix the inconsistency behavior when finding the widest common type #21074

[SPARK-21811][SQL] Fix the inconsistency behavior when finding the widest common type #21074

Uh oh!

Conversation

jiangxb1987 commented Apr 15, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 15, 2018

Uh oh!

jiangxb1987 commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 17, 2018 • 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 Apr 17, 2018

Uh oh!

SparkQA commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 18, 2018 • 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 Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 18, 2018

Uh oh!

SparkQA commented Apr 18, 2018

Uh oh!

SparkQA commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 19, 2018

Uh oh!

HyukjinKwon commented Apr 19, 2018

Uh oh!

cloud-fan commented Apr 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cloud-fan Apr 17, 2018 •

edited

Loading

cloud-fan Apr 18, 2018 •

edited

Loading