[SPARK-25772][SQL] Fix java map of structs deserialization #22745

vofque · 2018-10-16T11:41:19Z

This is a follow-up PR for #22708. It considers another case of java beans deserialization: java maps with struct keys/values.

When deserializing values of MapType with struct keys/values in java beans, fields of structs get mixed up. I suggest using struct data types retrieved from resolved input data instead of inferring them from java beans.

What changes were proposed in this pull request?

Invocations of "keyArray" and "valueArray" functions are used to extract arrays of keys and values. Struct type of keys or values is also inferred from java bean structure and ends up with mixed up field order.
I created a new UnresolvedInvoke expression as a temporary substitution of Invoke expression while no actual data is available. It allows to provide the resulting data type during analysis based on the resolved input data, not on the java bean (similar to UnresolvedMapObjects).

Key and value arrays are then fed to MapObjects expression which I replaced with UnresolvedMapObjects, just like in case of ArrayType.

Finally I added resolution of UnresolvedInvoke expressions in Analyzer.resolveExpression method as an additional pattern matching case.

How was this patch tested?

Added a test case.
Built complete project on travis.

@viirya @kiszk @cloud-fan @michalsenkyr @marmbrus @liancheng

Synch with apache:master

srowen · 2018-10-17T00:54:32Z

Is this a separate PR because this part is pretty separable, and you think could be considered separately? if it's all part of one logical change that should go in together or not at all, they can be in the original PR.

cloud-fan · 2018-10-17T02:48:23Z

It's a different issue, I think it worth a new ticket

viirya · 2018-10-17T03:07:27Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaBeanWithMapSuite.java

+            return true;
+        }
+
+        private static <K, V> boolean equals(Map<K, V> aMap, Map<K, V> bMap) {


hmm, do we need this specific equals? Can't we use Map.equals?

Yes, sure, that's absolutely redundand code...

Synch with apache:master

cloud-fan · 2018-10-18T02:12:19Z

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

I feel this is too general. Maybe we should just create a new expression GetArrayFromMap and resolve it to Invoke later.

I had such doubts too. OK.

vofque · 2018-10-18T11:58:56Z

Added these classes:

UnresolvedGetArrayFromMap in unresolved.scala - an unresolved substitution.
GetArrayFromMap in complexTypeExtractors.scala - an extraction algorithm (not sure this fits the idea of complexTypeExtractors.scala file)

Tried to follow the example of UnresolvedExtractValue and ExtractValue classes implementation.

dongjoon-hyun · 2018-10-20T19:15:55Z

ok to test.

SparkQA · 2018-10-20T19:21:20Z

Test build #97668 has finished for PR 22745 at commit 835b6f4.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Key() extends Source
case class Value() extends Source

dongjoon-hyun · 2018-10-20T19:26:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala

Hi, @vofque . Could you run the following in your branch and fix the scala style issues like this?

$ dev/scalastyle

OK, sure, @dongjoon-hyun.

@dongjoon-hyun, I've fixed scala style issues in my code.

cloud-fan · 2018-10-22T13:06:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala

              p => deserializerFor(keyType, Some(p)),
-              Invoke(getPath, "keyArray", ArrayType(keyDataType)),
-              keyDataType),
+              UnresolvedGetArrayFromMap(getPath, GetArrayFromMap.Key())),


Seems we don't need to make it unresolved

case class GetArrayFromMap(map: Expression, getKey: Boolean) extends Expression = { override def inputTypes = Seq(MapType) override def dataType = { val MapType(kt, vt) = map.dataType.asInstanceOf[MapType] if (getKey) kv else vt } override def eval... override def doCodegen... }

My idea was to replace this unresolved expression with Invoke.
Do you mean to write eval and doGenCode from scratch?
I don't see a way to wrap Invoke, as doGenCode is protected and can't be delegated to Invoke.

Yea we can write eval and doGenCode from scratch. It's also more efficient since we can omit the useless try-catch in Invoke.

e.g.

// from UnaryExpression override def nullSafeEval(input: Any) = input.asInstanceOf[MapData].keys override def doGenCode = defineCodeGen(ctx, ev, c => s"$c.keys")

SparkQA · 2018-10-22T13:46:02Z

Test build #97813 has finished for PR 22745 at commit 790cfda.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Key() extends Source
case class Value() extends Source

SparkQA · 2018-10-22T13:56:49Z

Test build #97819 has finished for PR 22745 at commit 835b6f4.

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

SparkQA · 2018-10-22T14:30:54Z

Test build #97848 has started for PR 22745 at commit 6f449c4.

SparkQA · 2018-10-22T14:32:43Z

Test build #97853 has started for PR 22745 at commit 790cfda.

SparkQA · 2018-10-22T15:08:25Z

Test build #97861 has started for PR 22745 at commit 790cfda.

SparkQA · 2018-10-22T16:02:16Z

Test build #97870 has started for PR 22745 at commit 6f449c4.

vofque · 2018-10-23T11:40:54Z

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

Assume that result can't be null if the underlying map is not null.

vofque · 2018-10-23T11:50:14Z

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

Thought, this adds readability. Is a simple boolean param better?

I don't have a strong opinion, but shall we use enum?

Yeah, probably.

cloud-fan · 2018-10-23T11:57:57Z

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

we don't need this check. eval is performance critical and we should assume there is no bug. We don't have this check in other expressions either.

OK, thanks.

cloud-fan · 2018-10-23T12:00:09Z

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

well this does reuse the functionName, but performance is more important here. how about

private lazy val arrayGetter: MapData => ArrayData = if (source) ... def eval... arrayGetter( input.asInstanceOf[MapData])

cloud-fan · 2018-10-23T12:02:19Z

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

make this expression extends UnaryExpression, then we can write

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = { defineCodeGen(ctx, ev, map => s"map.$functionName") }

Fixed that.

vofque · 2018-10-23T13:23:30Z

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

@cloud-fan, what about this approach: to pass function name, type getter and array getter as parameters of a private constructor + add two objects with specific constructors?

cloud-fan · 2018-10-23T13:32:49Z

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

let's do this check in

override def checkInputDataTypes

cloud-fan · 2018-10-23T13:33:05Z

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

what is this doing?

I copied this from Invoke, but it looks like it doesn't really affect anything.

Then let's save it. Otherwise other reviewers may get confused as well.

cloud-fan · 2018-10-23T13:35:17Z

LGTM except 2 minor comments

cloud-fan · 2018-10-23T14:54:04Z

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

case _: MapType

shall we just use if-else?

if (isinstanceOf[MapType]) ... else ...

SparkQA · 2018-10-23T15:05:11Z

Test build #97918 has finished for PR 22745 at commit 076e603.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Key() extends Source
case class Value() extends Source
case class GetArrayFromMap(

SparkQA · 2018-10-23T15:13:27Z

Test build #97919 has finished for PR 22745 at commit d9222d5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Key() extends Source
case class Value() extends Source
case class GetArrayFromMap(

SparkQA · 2018-10-23T17:06:14Z

Test build #97920 has finished for PR 22745 at commit 882fd22.

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

SparkQA · 2018-10-23T18:05:14Z

Test build #97921 has finished for PR 22745 at commit 8b26a5c.

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

SparkQA · 2018-10-23T19:14:54Z

Test build #97927 has finished for PR 22745 at commit 0670bbc.

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

cloud-fan · 2018-10-24T01:29:54Z

thanks, merging to master!

…and product type ## What changes were proposed in this pull request? After #22745 , Dataset encoder supports the combination of java bean and map type. This PR is to fix the Scala side. The reason why it didn't work before is, `CatalystToExternalMap` tries to get the data type of the input map expression, while it can be unresolved and its data type is known. To fix it, we can follow `UnresolvedMapObjects`, to create a `UnresolvedCatalystToExternalMap`, and only create `CatalystToExternalMap` when the input map expression is resolved and the data type is known. ## How was this patch tested? enable a old test case Closes #22812 from cloud-fan/map. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

This is a follow-up PR for apache#22708. It considers another case of java beans deserialization: java maps with struct keys/values. When deserializing values of MapType with struct keys/values in java beans, fields of structs get mixed up. I suggest using struct data types retrieved from resolved input data instead of inferring them from java beans. ## What changes were proposed in this pull request? Invocations of "keyArray" and "valueArray" functions are used to extract arrays of keys and values. Struct type of keys or values is also inferred from java bean structure and ends up with mixed up field order. I created a new UnresolvedInvoke expression as a temporary substitution of Invoke expression while no actual data is available. It allows to provide the resulting data type during analysis based on the resolved input data, not on the java bean (similar to UnresolvedMapObjects). Key and value arrays are then fed to MapObjects expression which I replaced with UnresolvedMapObjects, just like in case of ArrayType. Finally I added resolution of UnresolvedInvoke expressions in Analyzer.resolveExpression method as an additional pattern matching case. ## How was this patch tested? Added a test case. Built complete project on travis. viirya kiszk cloud-fan michalsenkyr marmbrus liancheng Closes apache#22745 from vofque/SPARK-21402-FOLLOWUP. Lead-authored-by: Vladimir Kuriatkov <[email protected]> Co-authored-by: Vladimir Kuriatkov <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? In apache#22745 we introduced the `GetArrayFromMap` expression. Later on I realized this is duplicated as we already have `MapKeys` and `MapValues`. This PR removes `GetArrayFromMap` ## How was this patch tested? existing tests Closes apache#22825 from cloud-fan/minor. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…and product type ## What changes were proposed in this pull request? After apache#22745 , Dataset encoder supports the combination of java bean and map type. This PR is to fix the Scala side. The reason why it didn't work before is, `CatalystToExternalMap` tries to get the data type of the input map expression, while it can be unresolved and its data type is known. To fix it, we can follow `UnresolvedMapObjects`, to create a `UnresolvedCatalystToExternalMap`, and only create `CatalystToExternalMap` when the input map expression is resolved and the data type is known. ## How was this patch tested? enable a old test case Closes apache#22812 from cloud-fan/map. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

vofque added 2 commits October 12, 2018 13:49

Merge pull request #2 from apache/master

d3578bb

Synch with apache:master

Merge pull request #4 from apache/master

bac0ff9

Synch with apache:master

vofque changed the title ~~[SPARK-21402][SQL][FOLLOWUP] Fix java map of structs deserialization~~ [SPARK-21402][SQL][FOLLOW-UP] Fix java map of structs deserialization Oct 16, 2018

viirya reviewed Oct 17, 2018

View reviewed changes

Merge pull request #9 from apache/master

a38b902

Synch with apache:master

vofque force-pushed the SPARK-21402-FOLLOWUP branch from 1896f9a to fa99c2d Compare October 17, 2018 15:26

cloud-fan reviewed Oct 18, 2018

View reviewed changes

vofque changed the title ~~[SPARK-21402][SQL][FOLLOW-UP] Fix java map of structs deserialization~~ [SPARK-25772][SQL] Fix java map of structs deserialization Oct 18, 2018

dongjoon-hyun reviewed Oct 20, 2018

View reviewed changes

vofque force-pushed the SPARK-21402-FOLLOWUP branch 2 times, most recently from 097412a to 790cfda Compare October 22, 2018 07:15

cloud-fan reviewed Oct 22, 2018

View reviewed changes

vofque force-pushed the SPARK-21402-FOLLOWUP branch from 076e603 to d9222d5 Compare October 23, 2018 11:34

vofque commented Oct 23, 2018

View reviewed changes

cloud-fan reviewed Oct 23, 2018

View reviewed changes

vofque commented Oct 23, 2018

View reviewed changes

cloud-fan reviewed Oct 23, 2018

View reviewed changes

Java map of structs deserialization fixed

0670bbc

vofque force-pushed the SPARK-21402-FOLLOWUP branch from 8b26a5c to 0670bbc Compare October 23, 2018 15:29

asfgit closed this in 584e767 Oct 24, 2018

This was referenced Oct 24, 2018

[SPARK-25817][SQL] Dataset encoder should support combination of map and product type #22812

Closed

[SPARK-25772][SQL][FOLLOWUP] remove GetArrayFromMap #22825

Closed

[SPARK-25772][SQL] Fix java map of structs deserialization #22745

[SPARK-25772][SQL] Fix java map of structs deserialization #22745

Uh oh!

Conversation

vofque commented Oct 16, 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!

srowen commented Oct 17, 2018

Uh oh!

cloud-fan commented Oct 17, 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!

vofque commented Oct 18, 2018

Uh oh!

dongjoon-hyun commented Oct 20, 2018

Uh oh!

SparkQA commented Oct 20, 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!

cloud-fan Oct 22, 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!

vofque Oct 22, 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!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 2018

Uh oh!

SparkQA commented Oct 22, 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!

cloud-fan Oct 23, 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!

vofque Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

vofque commented Oct 16, 2018 •

edited

Loading

cloud-fan Oct 22, 2018 •

edited

Loading

vofque Oct 22, 2018 •

edited

Loading

cloud-fan Oct 23, 2018 •

edited

Loading

vofque Oct 23, 2018 •

edited

Loading