[SPARK-22826][SQL] findWiderTypeForTwo Fails over StructField of Array #20010

bdrillard · 2017-12-18T21:14:08Z

What changes were proposed in this pull request?

Attempting to find the tightest common type over a struct holding fields of ArrayType or MapType will fail if the types of those struct fields are not exactly equal. See the JIRA for an example.

How was this patch tested?

This patch adds a test case for ArrayType and MapType for finding tightest common types.

mgaido91 · 2017-12-19T08:40:46Z

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

why is this if t1.sameType(t2) needed?

This will make sure the later get won't be applied to None

gczsjdy · 2017-12-19T09:06:34Z

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

If t1 and t2 are the sameType, why do we need recursively findTightestCommonType? Can we just make val dataType = pointType1?

I think that the reason is for nested structures, for instance an array of array or array of map

then @bdrillard can we add also some test cases for nested structures?

Let's use this example, if we call findTightestCommonType on 2 array of array of Integer types, then this dataType = findTightest(findTightest(IntegralType, IntegralType)) = IntegralType, so the final answer is Some(ArrayType(IntegralType))?

@mgaido91, @gczsjdy, sure, I'll add a pair of test cases for nested complex structures for ArrayType and MapType

@mgaido91 Thanks, I got it. But the t1.sameType(t2) judgement has already made sure that the 2 types are the same recursively. So it's not necessary to findTightestCommonType recursively again.

@gczsjdy I think it is needed, because IIUC in sameType the nullability is not checked to be the same. So it might vary and calling findTightestCommonType does the trick.

@gczsjdy sameType will ignore the nullability of the two types, only checking if the DataType is the same. We would expect then that the first case of findTightestCommonType would pass the types on through, but during its equality check, it also checks the type nullability, which for ArrayType and MapType, may differ between t1 and t2:

case (t1, t2) if t1 == t1 => Some(t1)

That means we can establish that two StructFields of ArrayType/MapType are the same DataType, but if they have different nullability, then the above case match won't match them, nor will any other case in the match set. So in order to find the tightest common type where nullabilities of the point-types may differ, we'll need to recurse. See a case like:

widenTest( StructType(StructField("a", ArrayType(StringType, containsNull=true)) :: Nil), StructType(StructField("a", ArrayType(StringType, containsNull=false)) :: Nil), Some(StructType(StructField("a", ArrayType(StringType, containsNull=true)) :: Nil)))

At the moment, since we have no case to match ArrayType/MapType where the nullabilities may differ, this case would fail. I can add this test explicitly to the suite.

Thanks for your illustration.

bdrillard · 2017-12-19T15:49:29Z

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

I think if we now check for ArrayTypes (including MapTypes) in findTightestCommonType, the match here becomes redundant. @mgaido91, @gczsjdy, does this thinking make sense to you both?

it makes sense, but I'd love that in your implementation in findTightestCommonType, you would replicate this logic, ie. removing the sameType guard and using findWiderTypeForTwo, in order to allow casting an array of int to an array of long. What do you think?

Sure, I think that's possible. In order to handle instances with and without string promotion, I think it may be necessary to add a boolean parameter, and then to handle the instances where the pointType/keyType and valueType may result in None, see https://github.com/apache/spark/pull/20010/files#diff-383a8cdd0a9c58cae68e0a79295520a3R105

To support the minor change in function signature for findTightestCommonType, I have to do some refactoring in the tests. Let me know if you think there's a cleaner implementation, but this seems to help localize like concerns into findTightestCommonType.

I think it makes sense, but I doubt the solution we call findWiderType.. in findTightest... This seems not reasonable.

My suggestion: we define a new function, like findWiderTypeForArray. This new function can provide 'findWider' functionality compared to the 'findTightestArray' part(which is basically the first commit in your PR). We won't break the original findTightest semantic in this way and the code is clean.

I like your idea @gczsjdy!

@gczsjdy I've taken a shot at implementing your suggestion with findWiderTypeForTwoComplex, which takes as an argument a widerTypeFunc, describing which widening behavior to apply to point types (should they permit promotion to string or not).

Because ArrayType instances that would require widening the type could be nested in StructType and MapType, I think it's necessary to have more case matching than would be in findWiderTypeForArray, hence findWiderTypeForTwoComplex.

I like your implementation, except for the concerns I raised in another thread. Have you tested locally?

Yes, this PR should be ready for a Jenkins build.

bdrillard · 2017-12-19T15:49:42Z

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

Same sanity check here, as above.

bdrillard · 2017-12-19T15:50:30Z

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

Here's a test for nested structures where an explicit match case against ArrayType/MapType is necessary due to the difference in nullability between the two structures.

thanks, may you please add also two tests having Map and Array as outer types?

gczsjdy · 2017-12-20T03:09:34Z

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

I think we break the findTightest semantic by withStringPromotion judgement and calling findWiderTypeForTwo, these are what we should add in Case 2 type widening -> findWiderTypeForTwo. I suggest we put these logic in another functionfindWiderTypeForDecimal. Will mention this in the other thread.

gczsjdy · 2017-12-20T03:16:12Z

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

My suggestion: we define a new function, like findWiderTypeForArray. This new function can provide 'findWider' functionality compared to the 'findTightestArray' part(which is basically the first commit in your PR). We won't break the original findTightest semantic in this way and the code is clean.

gczsjdy · 2017-12-22T07:16:03Z

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

Will this cause infinite loop? Calling findWiderTypeForTwo in findWiderTypeForTwo?

It should not. findWiderTypeForTwoComplex will only be called as we operate over "complex" types (i.e arrays, maps, structs), and will only recurse (calling findWiderTypeForTwo) over the child point-types of a complex type, so we ensure the recursive computation gets narrower as it recurses, until eventually terminating at the leaf level of the schema.

… types in a structfield [SPARK-22826] adding nested struct tests [SPARK-22826] removing extraneous test case

[SPARK-22826] adding test for integral type promotion [SPARK-22826] supporting array and map string promotion

This reverts commit da36de8b4035e143efa388b277596dceb185130a.

bdrillard · 2017-12-27T16:29:33Z

@gczsjdy, @mgaido91 If you all are comfortable with it, I think this PR is in a state where we could trigger a build.

mgaido91 · 2017-12-27T17:14:59Z

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

-        case _ => None
-      })
+      .orElse(findWiderTypeForTwoComplex(t1, t2, findWiderTypeForTwo))
+


this line should be removed

Fixed, see dfb4a3b47d88853be348aafd9802f799639693f6

mgaido91 · 2017-12-27T17:17:21Z

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

    widenTest(ArrayType(IntegerType), StructType(Seq()), None)

+    widenTest(
+      ArrayType(StringType, containsNull=true),


please add a whitespace before and after = and the same in the following lines

Fixed, see dfb4a3b47d88853be348aafd9802f799639693f6

mgaido91 · 2017-12-27T17:18:45Z

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

+    widenTest(
+      MapType(StringType, StringType, valueContainsNull=true),
+      MapType(StringType, StringType, valueContainsNull=false),
+      Some(MapType(StringType, StringType, valueContainsNull=true)))


please add a test also for struct

Here are tests for nested complex types, https://github.com/apache/spark/pull/20010/files#diff-01ecdd038c5c2f53f38118912210fef8R405

mgaido91 · 2017-12-27T17:20:13Z

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

+    widenTestWithStringPromotion(
+      StructType(StructField("a", ArrayType(LongType)) :: Nil),
+      StructType(StructField("a", ArrayType(StringType)) :: Nil),
+      Some(StructType(StructField("a", ArrayType(StringType)) :: Nil)))


please add an analogous test also for map and array

I've grouped string promotion tests for arrays and maps, with tests where those types are nested in structs or are fields in structs, https://github.com/apache/spark/pull/20010/files#diff-01ecdd038c5c2f53f38118912210fef8R504

mgaido91 · 2017-12-27T17:20:26Z

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

+    widenTestWithoutStringPromotion(
+      StructType(StructField("a", ArrayType(LongType)) :: Nil),
+      StructType(StructField("a", ArrayType(StringType)) :: Nil),
+      None)


I've grouped non-string promotion tests, and added tests for arrays, maps, and then structures nested in those types or having fields of those types, https://github.com/apache/spark/pull/20010/files#diff-01ecdd038c5c2f53f38118912210fef8R563

mgaido91 · 2017-12-27T17:20:39Z

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

+      StructType(StructField("a", ArrayType(LongType)) :: Nil),
+      StructType(StructField("a", ArrayType(StringType)) :: Nil),
+      Some(StructType(StructField("a", ArrayType(StringType)) :: Nil)))
+


this empty line is not needed

Fixed, see dfb4a3b47d88853be348aafd9802f799639693f6

mgaido91 · 2017-12-27T17:24:40Z

@bdrillard yes, after addressing my last comments I think we can build this. My only concern is that we are not covering the case in which there are nested complex structures in the wider case. But maybe we can address this later.

I don't have permissions to trigger a build. You might check who are the last committers working on this and maybe ask them.
Thanks.

bdrillard · 2017-12-27T18:43:30Z

@mgaido91 Agreed with that concern. I think the last round of tests I've just added covers the permutation of cases well, where we have arrays and maps of structs, and structs of arrays and maps.

cc: @viirya, @gatorsmile, Could perhaps one of you trigger a Jenkins build against this PR?

viirya · 2017-12-27T23:12:08Z

@HyukjinKwon Can you help trigger Jenkins test for this? Thanks.

HyukjinKwon · 2017-12-27T23:35:46Z

ok to test

SparkQA · 2017-12-28T01:55:07Z

Test build #85447 has finished for PR 20010 at commit 8ce3b39.

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

viirya · 2017-12-28T01:56:38Z

Thanks @HyukjinKwon

gczsjdy · 2017-12-29T03:31:41Z

Seems not a regular error?
@bdrillard Maybe you can push a commit and trigger the test again.

SparkQA · 2017-12-29T07:19:49Z

Test build #85497 has finished for PR 20010 at commit 86e1929.

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

mgaido91 · 2017-12-29T09:58:52Z

@gczsjdy @bdrillard the test errors are valid. In some cases an exception is expected to be thrown, but it isn't, due to the fix. So they should be updated accordingly.

SparkQA · 2017-12-29T16:04:18Z

Test build #85514 has finished for PR 20010 at commit 583f674.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-29T17:34:47Z

Test build #85520 has finished for PR 20010 at commit e034bbc.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-29T18:40:21Z

Test build #85515 has finished for PR 20010 at commit 23c83ac.

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

SparkQA · 2017-12-29T19:59:53Z

Test build #85521 has finished for PR 20010 at commit 7d146e3.

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

SparkQA · 2017-12-29T23:53:23Z

Test build #85526 has finished for PR 20010 at commit d42dfa5.

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

mgaido91 · 2017-12-30T09:13:42Z

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

+        }
+
+      case (StructType(fields1), StructType(fields2)) =>
+        val fieldTypes = fields1.zip(fields2).map { case (f1, f2) =>


this requires that fields with the same name must also be in the same position. Is this assumption correct?

@mgaido91 That's seems to be the assumption already made in findTightestCommonType. The sameType function on DataType also requires structfields are ordered the same, else it returns false.

The difference here is that we don't require the structfields strictly have the same type, so we can support widening to LongType, StringType, etc. But we do require the fields 1. have the same order, and 2. have the same name (either with strict case, or ignoring case).

marmbrus · 2018-01-03T20:11:56Z

/cc @cloud-fan @sameeragarwal

gatorsmile · 2018-01-04T01:38:42Z

Will review it today.

cloud-fan · 2018-01-04T03:20:41Z

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

    case (_: TimestampType, _: DateType) | (_: DateType, _: TimestampType) =>
      Some(TimestampType)

+    case (t1 @ ArrayType(pointType1, nullable1), t2 @ ArrayType(pointType2, nullable2))


pointType looks like a weird name, how about elementType or et?

Sure, see 09e49fb

cloud-fan · 2018-01-04T03:26:04Z

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

-      ArrayType(StringType, containsNull = true),
+      ArrayType(ArrayType(IntegerType), containsNull = true),
+      ArrayType(ArrayType(LongType), containsNull = false),
+      Some(ArrayType(ArrayType(LongType), containsNull = true)))


Do we really wanna do this? Is there any other database have this behavior?

I think for complex type, we should ignore the nullable difference, but I'm not sure if we should do type coercion inside complex type.

@mgaido91, thoughts on this?

It's definitely possible for us to revert back to behavior where we don't do IntegerType-to-LongType, xType-to-StringType, etc. promotion inside complex types, which was how a previous form of this PR handled it.

I think this is more coherent with the behavior in other parts and this is the behavior I would expect. But I think that we should follow @gatorsmile's suggestion and check Hive's behavior first.

SparkQA · 2018-01-04T18:24:14Z

Test build #85682 has finished for PR 20010 at commit 09e49fb.

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

gatorsmile · 2018-01-06T02:37:42Z

Overall, it is reasonable. What is the current behavior in Hive?

bdrillard · 2018-01-15T20:18:54Z

Is there a consensus on the preferred behavior here? This issue would also be a blocker to encoders for Spark-Avro in Spark 2.3.0 that @marmbrus mentions in #20085.

mgaido91 · 2018-01-16T09:31:53Z

@bdrillard I think that we need to know Hive's behavior as per #20010 (comment) to state which is the preferred/right behavior here.

HyukjinKwon · 2018-06-09T09:06:04Z

ok to test

SparkQA · 2018-06-09T11:46:11Z

Test build #91613 has finished for PR 20010 at commit 09e49fb.

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

cloud-fan · 2018-07-31T13:19:25Z

I think this is already fixed.

AmplabJenkins · 2019-02-10T22:56:40Z

Can one of the admins verify this patch?

bdrillard force-pushed the SPARK-22826 branch from 1ac4b0b to fb48148 Compare December 18, 2017 21:27

mgaido91 reviewed Dec 19, 2017

View reviewed changes

gczsjdy reviewed Dec 19, 2017

View reviewed changes

bdrillard force-pushed the SPARK-22826 branch from c5bb197 to 98709fd Compare December 19, 2017 15:44

bdrillard commented Dec 19, 2017

View reviewed changes

bdrillard force-pushed the SPARK-22826 branch 2 times, most recently from 5a304e6 to da36de8 Compare December 19, 2017 23:14

gczsjdy reviewed Dec 20, 2017

View reviewed changes

gczsjdy reviewed Dec 22, 2017

View reviewed changes

ALeksander Eskilson added 4 commits December 26, 2017 10:54

[SPARK-22826] adding support for tighest type coercion over array/map…

bbcc7c6

… types in a structfield [SPARK-22826] adding nested struct tests [SPARK-22826] removing extraneous test case

[SPARK-22826] supporting with and without string promition

90c2979

[SPARK-22826] adding test for integral type promotion [SPARK-22826] supporting array and map string promotion

Revert "[SPARK-22826] supporting with and without string promition"

eb52629

This reverts commit da36de8b4035e143efa388b277596dceb185130a.

[SPARK-22826] cleaning up api over wider types

527099d

bdrillard force-pushed the SPARK-22826 branch from 538194b to 527099d Compare December 26, 2017 16:55

mgaido91 reviewed Dec 27, 2017

View reviewed changes

bdrillard force-pushed the SPARK-22826 branch from b7de6dc to 8ce3b39 Compare December 27, 2017 18:34

[SPARK-22826] adding more test cases

86e1929

bdrillard force-pushed the SPARK-22826 branch from 8ce3b39 to 86e1929 Compare December 29, 2017 05:04

[SPARK-22826] checking to sameType on complex types

583f674

ALeksander Eskilson added 3 commits December 29, 2017 10:07

[SPARK-22826] fixup on spacing

23c83ac

[SPARK-22826] reverting use of sameType

a3ed2e0

[SPARK-22826] adding sensitivity to field names and case

e034bbc

[SPARK-22826] style fixup

7d146e3

[SPARK-22826] fixing issue with boolean condition on case sensitivity

d42dfa5

mgaido91 reviewed Dec 30, 2017

View reviewed changes

cloud-fan reviewed Jan 4, 2018

View reviewed changes

[SPARK-22826] changing array type element name

09e49fb

HyukjinKwon closed this Feb 11, 2019

[SPARK-22826][SQL] findWiderTypeForTwo Fails over StructField of Array #20010

[SPARK-22826][SQL] findWiderTypeForTwo Fails over StructField of Array #20010

Uh oh!

Conversation

bdrillard commented Dec 18, 2017

What changes were proposed in this pull request?

How was this patch tested?

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!

gczsjdy Dec 19, 2017 • 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!

gczsjdy Dec 20, 2017 • 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!

bdrillard Dec 19, 2017 • 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!

bdrillard commented Dec 27, 2017

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!

gczsjdy Dec 19, 2017 •

edited

Loading

gczsjdy Dec 20, 2017 •

edited

Loading

bdrillard Dec 19, 2017 •

edited

Loading