[SPARK-21402][SQL] Fix java array of structs deserialization by vofque · Pull Request #22708 · apache/spark

vofque · 2018-10-12T11:58:33Z

When deserializing values of ArrayType with struct elements 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?

MapObjects expression is used to map array elements to java beans. Struct type of elements is inferred from java bean structure and ends up with mixed up field order.
I used UnresolvedMapObjects instead of MapObjects, which allows to provide element type for MapObjects during analysis based on the resolved input data, not on the java bean.

How was this patch tested?

Added a test case.
Built complete project on travis.

@michalsenkyr @cloud-fan @marmbrus @liancheng

Synch with apache:master

cloud-fan · 2018-10-15T03:50:49Z

Can you explain how this happens? Why thhe fields of structs get mixed up?

vofque · 2018-10-15T09:21:50Z

The original problem is described here: https://issues.apache.org/jira/browse/SPARK-21402

I'll try to explain what happens in detail.

Let's consider this data structure:

root
|-- intervals: array
| |-- element: struct
| | |-- startTime: long
| | |-- endTime: long

And let's say we have a java bean class with corresponding structure.

When building a deserializer for the field intervals in JavaTypeInference.deserializerFor we construct a MapObjects expression to convert structs to java beans:

case c if listType.isAssignableFrom(typeToken) =>
val et = elementType(typeToken)
MapObjects(
p => deserializerFor(et, Some(p)),
getPath,
inferDataType(et)._1,
customCollectionCls = Some(c))

MapObjects requires DataType of array elements. It is extracted from java element type using JavaTypeInference.inferDataType which gets java bean properties and maps them to StructFields.

case other =>
// some more code goes here
val properties = getJavaBeanReadableProperties(other)
val fields = properties.map { property =>
val returnType = typeToken.method(property.getReadMethod).getReturnType
val (dataType, nullable) = inferDataType(returnType, seenTypeSet + other)
new StructField(property.getName, dataType, nullable)
}

The order of properties in the resulting StructType may not correspond to their declaration order as the declaration order is simply unknown. So the resulting element StructType may look like this:

root
|-- endTime: long
|-- startTime: long

This StructType is passed to MapObjects and then to its loop variable LambdaVariable.

For deserialization of single array elements an InitializeJavaBean expression is created. It contains UnresolvedExtractValue expressions for each field, and these expressions have LambdaVariable as a child. They are resolved during analysis:

case UnresolvedExtractValue(child, fieldName) if child.resolved =>
ExtractValue(child, fieldName, resolver)

For each field startTime and endTime ordinals are calculated. For that child's DataType is used, and in our case this is StructType of LambdaVariable with incorrect field order.
As a result we get GetStructField expressions with ordinal = 0 for endTime and ordinal = 1 for startTime.

vofque · 2018-10-15T09:40:56Z

In a nutshell:
First we calculate field ordinals of array elements using java bean field information (which doesn't guarantee any particular field order).
Then we apply these ordinals to the actual data to retrieve values.

cloud-fan · 2018-10-15T10:44:53Z

can we exclude other changes except this one? This one is very easy to reason about. We did the same thing in ScalaReflection.
We need more time to think about the map case, and fix it in ScalaReflection as well.

Sure. Should I create another pull request with this change only?

yes please, thanks!

Removed other changes from this PR and created a new one with only map case.
As far as I see, everything works fine with scala classes, because StructTypes are generated based on constructor parameters, and they are available in correct order with correct names. Which is hardly achievable with Java beans..

viirya · 2018-10-16T04:11:47Z

Should we move this to unresolved.scala? cc @cloud-fan

Synch with apache:master

kiszk · 2018-10-16T08:21:00Z

nit: it does not seem to be necessary.

Yep, removed it.

viirya · 2018-10-16T08:29:28Z

Please modify the PR title and description accordingly. Thanks.

viirya · 2018-10-16T08:57:29Z

This is also unused now.

viirya · 2018-10-16T08:58:38Z

You need to add the license headers.

viirya · 2018-10-16T09:01:09Z

The import orders here are not compliant with Spark codebase. You can follow the style in other tests like JavaApplySchemaSuite.

viirya · 2018-10-16T09:02:12Z

And please add [SQL] to the PR title. Like [SPARK-21402][SQL]

vofque · 2018-10-16T11:17:59Z

Corrected all issues.

cloud-fan · 2018-10-16T12:26:15Z

ok to test

cloud-fan · 2018-10-16T12:26:22Z

lgtm

SparkQA · 2018-10-16T15:05:55Z

Test build #97454 has finished for PR 22708 at commit 8bc66e6.

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

cloud-fan · 2018-10-16T15:17:55Z

cc @dongjoon-hyun here is another instance of the FileBasedDataSourceSuite flaky test.

cloud-fan · 2018-10-16T15:18:15Z

retest this please

viirya · 2018-10-16T16:01:13Z

LGTM

SparkQA · 2018-10-16T19:11:05Z

Test build #97458 has finished for PR 22708 at commit 8bc66e6.

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

dongjoon-hyun · 2018-10-16T21:30:00Z

Thank you for pinging me, @cloud-fan .

viirya · 2018-10-17T03:08:51Z

+
+ private int id;
+ private List<Interval> intervals;
+ private List<Integer> values;


Will this list of int affect the test? If no, maybe we can get rid of it to simplify the test.

The intention was to test a non struct case too. But I think, it's really not critical and we can get rid of it.

viirya · 2018-10-17T03:14:03Z

+ }
+ }
+
+ public static class Interval {


This is duplicate to your another PR. Maybe we can consider put two tests in one Java file so we don't need to have two Interval.

But, I guess, to do that we need to have these both changes in one PR?
Or correct another PR to add the second test in the same Java file later.

We can have this merged first and rebase another PR to have another test in the same file too.

vofque · 2018-10-17T08:11:28Z

Fixed all issues in test class. Thanks a lot for your help and patience.
Rebase was probably a bad idea, sorry for that..

SparkQA · 2018-10-17T08:50:24Z

Test build #97489 has finished for PR 22708 at commit f6c40b6.

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

viirya · 2018-10-17T08:52:45Z

+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+
+public class JavaBeanWithMapSuite {


Why include this change here? Don't you want to have it in another PR?

Yeah, that's my fault..

SparkQA · 2018-10-17T09:05:11Z

Test build #97490 has finished for PR 22708 at commit b1f74ac.

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

SparkQA · 2018-10-17T11:51:59Z

Test build #97488 has finished for PR 22708 at commit 811e45a.

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

SparkQA · 2018-10-17T12:38:08Z

Test build #97491 has finished for PR 22708 at commit 46e942d.

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

cloud-fan · 2018-10-17T14:13:51Z

thanks, merging to master/2.4!

When deserializing values of ArrayType with struct elements 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? MapObjects expression is used to map array elements to java beans. Struct type of elements is inferred from java bean structure and ends up with mixed up field order. I used UnresolvedMapObjects instead of MapObjects, which allows to provide element type for MapObjects during analysis based on the resolved input data, not on the java bean. ## How was this patch tested? Added a test case. Built complete project on travis. michalsenkyr cloud-fan marmbrus liancheng Closes#22708 from vofque/SPARK-21402. Lead-authored-by: Vladimir Kuriatkov <vofque@gmail.com> Co-authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e5b8136) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

dongjoon-hyun

+1, late LGTM. Thank you, @vofque .

dongjoon-hyun · 2018-10-17T17:24:45Z

@cloud-fan and @vofque .
Can we have this fix in branch-2.3 and branch-2.2, too?
It seems that we need another backport PRs for them.

vofque · 2018-10-17T19:29:31Z

@dongjoon-hyun, sure, I'll create equal pull requests.

dongjoon-hyun · 2018-10-17T19:37:57Z

Thanks!

…ation This PR is to backport #22708 to branch 2.2. ## What changes were proposed in this pull request? MapObjects expression is used to map array elements to java beans. Struct type of elements is inferred from java bean structure and ends up with mixed up field order. I used UnresolvedMapObjects instead of MapObjects, which allows to provide element type for MapObjects during analysis based on the resolved input data, not on the java bean. ## How was this patch tested? Added a test case. Built complete project on travis. dongjoon-hyun cloud-fan Closes#22768 from vofque/SPARK-21402-2.2. Lead-authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Co-authored-by: Vladimir Kuriatkov <vofque@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ation This PR is to backport #22708 to branch 2.3. ## What changes were proposed in this pull request? MapObjects expression is used to map array elements to java beans. Struct type of elements is inferred from java bean structure and ends up with mixed up field order. I used UnresolvedMapObjects instead of MapObjects, which allows to provide element type for MapObjects during analysis based on the resolved input data, not on the java bean. ## How was this patch tested? Added a test case. Built complete project on travis. dongjoon-hyun cloud-fan Closes#22767 from vofque/SPARK-21402-2.3. Authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

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 Closes#22745 from vofque/SPARK-21402-FOLLOWUP. Lead-authored-by: Vladimir Kuriatkov <vofque@gmail.com> Co-authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

When deserializing values of ArrayType with struct elements 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? MapObjects expression is used to map array elements to java beans. Struct type of elements is inferred from java bean structure and ends up with mixed up field order. I used UnresolvedMapObjects instead of MapObjects, which allows to provide element type for MapObjects during analysis based on the resolved input data, not on the java bean. ## How was this patch tested? Added a test case. Built complete project on travis. michalsenkyr cloud-fan marmbrus liancheng Closesapache#22708 from vofque/SPARK-21402. Lead-authored-by: Vladimir Kuriatkov <vofque@gmail.com> Co-authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

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 Closesapache#22745 from vofque/SPARK-21402-FOLLOWUP. Lead-authored-by: Vladimir Kuriatkov <vofque@gmail.com> Co-authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ation This PR is to backport apache#22708 to branch 2.2. ## What changes were proposed in this pull request? MapObjects expression is used to map array elements to java beans. Struct type of elements is inferred from java bean structure and ends up with mixed up field order. I used UnresolvedMapObjects instead of MapObjects, which allows to provide element type for MapObjects during analysis based on the resolved input data, not on the java bean. ## How was this patch tested? Added a test case. Built complete project on travis. dongjoon-hyun cloud-fan Closesapache#22768 from vofque/SPARK-21402-2.2. Lead-authored-by: Vladimir Kuriatkov <Vladimir_Kuriatkov@epam.com> Co-authored-by: Vladimir Kuriatkov <vofque@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

Merge pull request#2from apache/master
d3578bb
Synch with apache:master

vofque changed the title ~~[Spark 21402] Fix java array/map of structs deserialization~~[SPARK-21402] Fix java array/map of structs deserializationOct 12, 2018

cloud-fan reviewed Oct 15, 2018
View reviewed changes

viirya reviewed Oct 16, 2018
View reviewed changes

Merge pull request#4from apache/master
bac0ff9
Synch with apache:master

vofque force-pushed the SPARK-21402 branch 2 times, most recently from 4b5d334 to 4103257Compare October 16, 2018 08:06

kiszk reviewed Oct 16, 2018
View reviewed changes

vofque force-pushed the SPARK-21402 branch from 4103257 to a97ffd6Compare October 16, 2018 08:48

vofque changed the title ~~[SPARK-21402] Fix java array/map of structs deserialization~~[SPARK-21402] Fix java array of structs deserializationOct 16, 2018

viirya reviewed Oct 16, 2018
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/JavaTypeInference.scala Outdated
viiryaOct 16, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also unused now.

viirya reviewed Oct 16, 2018
View reviewed changes

vofque changed the title ~~[SPARK-21402] Fix java array of structs deserialization~~[SPARK-21402][SQL] Fix java array of structs deserializationOct 16, 2018

vofque force-pushed the SPARK-21402 branch from a97ffd6 to 8bc66e6Compare October 16, 2018 09:09

vofque mentioned this pull request Oct 16, 2018
[SPARK-25772][SQL] Fix java map of structs deserialization #22745
Closed

viirya reviewed Oct 17, 2018
View reviewed changes

vofque force-pushed the SPARK-21402 branch from 8bc66e6 to 811e45aCompare October 17, 2018 08:04

viirya reviewed Oct 17, 2018
View reviewed changes

vofque force-pushed the SPARK-21402 branch 2 times, most recently from b1f74ac to 571a0feCompare October 17, 2018 09:03

Java array of structs deserialization fixed
46e942d

vofque force-pushed the SPARK-21402 branch from 571a0fe to 46e942dCompare October 17, 2018 09:04

asfgit closed this in e5b8136Oct 17, 2018

dongjoon-hyun reviewed Oct 17, 2018
View reviewed changes

This was referenced Oct 18, 2018
[SPARK-21402][SQL][BACKPORT-2.3] Fix java array of structs deserialization #22767
Closed
[SPARK-21402][SQL][BACKPORT-2.2] Fix java array of structs deserialization #22768
Closed

Uh oh!

Conversation

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

cloud-fan commented Oct 15, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

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

viirya commented Oct 16, 2018

Uh oh!

vofque commented Oct 16, 2018

Uh oh!

cloud-fan commented Oct 16, 2018

Uh oh!

cloud-fan commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

cloud-fan commented Oct 16, 2018

Uh oh!

cloud-fan commented Oct 16, 2018

Uh oh!

viirya commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

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

vofque commented Oct 17, 2018

Uh oh!

SparkQA commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 17, 2018

Uh oh!

SparkQA commented Oct 17, 2018

Uh oh!

vofque commented Oct 12, 2018•
edited
Loading

vofque commented Oct 15, 2018•
edited
Loading