[SPARK-24734][SQL] Fix type coercions and nullabilities of nested data types of some functions. #21704

ueshin · 2018-07-03T11:34:42Z

What changes were proposed in this pull request?

We have some functions which need to aware the nullabilities of all children, such as CreateArray, CreateMap, Concat, and so on. Currently we add casts to fix the nullabilities, but the casts might be removed during the optimization phase.
After the discussion, we decided to not add extra casts for just fixing the nullabilities of the nested types, but handle them by functions themselves.

How was this patch tested?

Modified and added some tests.

ueshin · 2018-07-03T12:00:49Z

cc @mn-mikke @gatorsmile @cloud-fan

mn-mikke · 2018-07-03T12:17:38Z

@ueshin Thanks for bringing this topic! This problem with different nullable/containsNull flags seems to be more generic. In 21687, we've touched a similar problem with CaseWhen and If expression. So I think It would nice if we could think together about a generic and consistent solution for all espressions. WDYT?

ueshin · 2018-07-03T12:36:02Z

@mn-mikke Thanks! I'll take a look and join the discussion later.

SparkQA · 2018-07-03T15:16:38Z

Test build #92562 has finished for PR 21704 at commit d87a8c6.

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

maropu · 2018-07-04T02:23:40Z

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

+        ArrayType(et, dataTypes.exists(_.asInstanceOf[ArrayType].containsNull))
+      case dt => dt
+    }.getOrElse(StringType)
+  }


Can't we handle this case in type coercion (analysis phase)?

Actually, Concat for array type has the type coercion to add casts to make all children the same type, but we also have the optimization SimplifyCasts to remove unnecessary casts which might remove casts from arrays not containing null to arrays containing null (optimizer/expressions.scala#L611).

E.g., concat(array(1,2,3), array(4,null,6)) might generate a wrong data type during the execution.

Added a test to show the wrong nullability.

Aha, I see. But, I just have a hunch that SimplifyCasts cannot simplify array casts in some cases?, e.g., this concat case. Since we basically cannot change plan semantics in optimization phase, I feel a little weird about this simplification. Anyway, I'm ok with your approach because I can't find a better & simpler way to solve this in analysis phase... Thanks!

Hmm, that also makes sense. I'm not sure we can remove the simplification, though. cc @gatorsmile @cloud-fan

Yeah, Coalesce should be fixed, and Least and Greatest are also suspicious.

what about changing SimplifyCasts rule to start replacing Cast with a new dummy cast expression that will hold only a target data type and won't perform any casting?

This can work, but my point is we should not add the cast to change containsNull, as it may not match the underlying child expression and generates unnecessary null check code.

Oh, see. In that case, it would be nice to introduce a method that will resolve the output DataType and merges nullable/containNull flags of non-primitive types recursively for such expressions. For the most cases we could encapsulate the function into a new trait. WDYT?

+1 for the @mn-mikke idea

SparkQA · 2018-07-04T11:01:58Z

Test build #92598 has finished for PR 21704 at commit 30d5aed.

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

cloud-fan · 2018-07-04T12:29:48Z

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

+    val dataTypes = children.map(_.dataType)
+    dataTypes.headOption.map {
+      case ArrayType(et, _) =>
+        ArrayType(et, dataTypes.exists(_.asInstanceOf[ArrayType].containsNull))


do we support array of array in concat?

Yes, it should work (see a test for it). Did we miss anything?

then shall we fix the containNull for the inner array?

yeah, + valueContainsNull for MapType and nullable for StructField

I guess the inner nullabilities are coerced during type-coercion? If the inner nullabilities are different, type coercion adds casts and they will remain.

@ueshin For Concat, Coalesce, etc. it seems to be that case since a coercion rule is executed if there is any nullability difference on any level of nesting. But it's not the case of CaseWhenCoercion rule, since sameType method is used for comparison.

I'm wondering if the goal is to avoid generation of extra Cast expressions, shouldn't other coercion rules utilize sameType method as well? Let's assume that the result of concat is subsequently used by flatten, wouldn't it lead to generation of extra null safe checks as mentioned here?

Please ignore the part of my previous comment regarding flatten function. The output data type of concat, etc. will be the same regardless what resolves null flags.

This reverts commit d87a8c6.

ueshin · 2018-07-09T09:49:32Z

Note: we will be able to use NonPrimitiveTypeMergingExpression in #21687.

SparkQA · 2018-07-09T15:47:38Z

Test build #92756 has finished for PR 21704 at commit 444383d.

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

SparkQA · 2018-07-09T15:59:55Z

Test build #92750 has finished for PR 21704 at commit 3d8891e.

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

SparkQA · 2018-07-10T12:42:02Z

Test build #92805 has finished for PR 21704 at commit 2c54e38.

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

SparkQA · 2018-07-11T14:30:29Z

Test build #92855 has finished for PR 21704 at commit db254e5.

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

cloud-fan · 2018-07-11T14:52:18Z

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

        val types = children.map(_.dataType)
        findWiderCommonType(types) match {
-          case Some(finalDataType) => CreateArray(children.map(Cast(_, finalDataType)))
+          case Some(finalDataType) => CreateArray(children.map(castIfNotSameType(_, finalDataType)))


is it needed? I think optimizer can remove unnecessary cast.

Currently the optimizer doesn't remove the cast when the difference of the finalDataType is only the nullabilities of nested types.

cloud-fan · 2018-07-11T14:53:09Z

what's still WIP for this PR?

cloud-fan · 2018-07-12T03:47:38Z

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

      // we need to truncate, but we should not promote one side to string if the other side is
      // string.g
-      case g @ Greatest(children) if !haveSameType(children) =>
+      case g @ Greatest(children) if !g.areInputTypesForMergingEqual =>


these are not just concat, can you update the PR title?

Updated PR title and description.

cloud-fan · 2018-07-12T03:52:00Z

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

    TypeUtils.checkForSameTypeInputExpr(children.map(_.dataType), s"function $prettyName")
  }

  override def dataType: ArrayType = {


why CreateArray doesn't extend ComplexTypeMergingExpression?

Because the data type of the CreateArray itself is not the merged type but ArrayType.

mn-mikke · 2018-07-12T11:31:21Z

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

          keys.map(_.dataType.simpleString).mkString("[", ", ", "]"))
-    } else if (values.map(_.dataType).distinct.length > 1) {
+    } else if (values.length > 1 &&
+      values.map(_.dataType).sliding(2, 1).exists { case Seq(t1, t2) => !t1.sameType(t2) }) {


The checks for keys and values are very similar. Would it be possible to separate the common logic into a private method?

mn-mikke · 2018-07-12T11:35:18Z

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

+    if (types.isEmpty) {
+      None
+    } else {
+      types.tail.foldLeft(Option(types.head)) {


nit: Some(types.head) to avoid an extra null check?

SparkQA · 2018-07-12T13:07:04Z

Test build #92922 has finished for PR 21704 at commit f701242.

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

cloud-fan · 2018-07-12T14:52:41Z

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


-  private def haveSameType(exprs: Seq[Expression]): Boolean =
-    exprs.map(_.dataType).distinct.length == 1
+  private def haveSameType(exprs: Seq[Expression]): Boolean = {


This is duplicated with ComplexTypeMergingExpression.areInputTypesForMergingEqual, can we unify them? We can

remove hasSameType. Any expression that needs to do this check should extend ComplexTypeMergingExpression and the TypeCoercion rule should call areInputTypesForMergingEqual.

remove areInputTypesForMergingEqual. ComplexTypeMergingExpression should only define the list of data types that need to be merged, and TypeCoercion rule should call hasSameType(e.inputTypesForMerging)

Since we have CreateArray and CreateMap, we can't make all such expressions ComplexTypeMergingExpression.
I'd apply 2) approach.

SparkQA · 2018-07-13T12:37:08Z

Test build #92967 has finished for PR 21704 at commit 5115961.

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

This reverts commit f701242.

cloud-fan · 2018-07-16T06:42:02Z

LGTM

SparkQA · 2018-07-16T07:05:02Z

Test build #93083 has finished for PR 21704 at commit e489e8b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class ArraySetLike extends BinaryArrayExpressionWithImplicitCast

ueshin · 2018-07-16T07:05:42Z

Jenkins, retest this please.

SparkQA · 2018-07-16T11:00:13Z

Test build #93087 has finished for PR 21704 at commit e489e8b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class ArraySetLike extends BinaryArrayExpressionWithImplicitCast

cloud-fan · 2018-07-16T15:17:18Z

thanks, merging to master!

Fix containsNull of Concat for array type.

d87a8c6

maropu reviewed Jul 4, 2018

View reviewed changes

ueshin mentioned this pull request Jul 4, 2018

[SPARK-24165][SQL] Fixing conditional expressions to handle nullability of nested types #21687

Closed

Add a test.

30d5aed

cloud-fan reviewed Jul 4, 2018

View reviewed changes

kiszk mentioned this pull request Jul 6, 2018

[SPARK-23914][SQL] Add array_union function #21061

Closed

ueshin added 8 commits July 9, 2018 18:39

Revert "Fix containsNull of Concat for array type."

2e624df

This reverts commit d87a8c6.

Merge branch 'master' into issues/SPARK-24734/concat_containsnull

fa73b32

Suppress unneeded casts.

da0702b

Fix Concat.

b31e401

Fix CreateArray.

7a838b0

Fix CreateMap.

01c9ff3

Fix Coalesce.

b2ca587

Fix Least and Greatest.

3d8891e

ueshin added 2 commits July 9, 2018 21:38

Merge branch 'master' into issues/SPARK-24734/concat_containsnull

1fa692a

Fix MapConcat.

444383d

ueshin changed the title ~~[SPARK-24734][SQL] Fix containsNull of Concat for array type.~~ [WIP][SPARK-24734][SQL] Fix containsNull of Concat for array type. Jul 10, 2018

ueshin added 2 commits July 10, 2018 17:50

Fix udts.

3e1f7e4

Rename to findCommonTypeDifferentOnlyInNullFlags.

2c54e38

ueshin added 2 commits July 11, 2018 17:25

Merge branch 'master' into issues/SPARK-24734/concat_containsnull

5f1b865

Use ComplexTypeMergingExpression trait.

2ab025f

Fix udts.

db254e5

cloud-fan reviewed Jul 11, 2018

View reviewed changes

ueshin changed the title ~~[WIP][SPARK-24734][SQL] Fix containsNull of Concat for array type.~~ [SPARK-24734][SQL] Fix containsNull of Concat for array type. Jul 12, 2018

cloud-fan reviewed Jul 12, 2018

View reviewed changes

ueshin changed the title ~~[SPARK-24734][SQL] Fix containsNull of Concat for array type.~~ [SPARK-24734][SQL] Fix type coercions and nullabilities of nested data types of some functions. Jul 12, 2018

ueshin added 2 commits July 12, 2018 17:45

Merge branch 'master' into issues/SPARK-24734/concat_containsnull

b412f7b

Make ArraySetLike ComplexTypeMergingExpression.

f701242

mn-mikke reviewed Jul 12, 2018

View reviewed changes

cloud-fan reviewed Jul 12, 2018

View reviewed changes

Use TypeCoercion.haveSameType.

5115961

Revert "Make ArraySetLike ComplexTypeMergingExpression."

e489e8b

This reverts commit f701242.

asfgit closed this in b045315 Jul 16, 2018

[SPARK-24734][SQL] Fix type coercions and nullabilities of nested data types of some functions. #21704

[SPARK-24734][SQL] Fix type coercions and nullabilities of nested data types of some functions. #21704

Uh oh!

Conversation

ueshin commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ueshin commented Jul 3, 2018

Uh oh!

mn-mikke commented Jul 3, 2018

Uh oh!

ueshin commented Jul 3, 2018

Uh oh!

SparkQA commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueshin Jul 4, 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!

Choose a reason for hiding this comment

Uh oh!

maropu Jul 4, 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!

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 4, 2018

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!

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!

ueshin commented Jul 9, 2018

Uh oh!

SparkQA commented Jul 9, 2018

Uh oh!

SparkQA commented Jul 9, 2018

Uh oh!

SparkQA commented Jul 10, 2018

Uh oh!

SparkQA commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 11, 2018

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

ueshin commented Jul 3, 2018 •

edited

Loading

ueshin Jul 4, 2018 •

edited

Loading

maropu Jul 4, 2018 •

edited

Loading

ueshin Jul 13, 2018 •

edited

Loading