[SPARK-30267][SQL] Avro arrays can be of any List #26907

steven-aerts · 2019-12-16T13:22:21Z

The Deserializer assumed that avro arrays are always of type GenericData$Array which is not the case.
Assuming they are from java.util.List is safer and fixes a ClassCastException in some avro code.

What changes were proposed in this pull request?

Java.util.List has all the necessary methods and is the base class of GenericData$Array.

Why are the changes needed?

To prevent the following exception in more complex avro objects:

java.lang.ClassCastException: java.util.ArrayList cannot be cast to org.apache.avro.generic.GenericData$Array
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$newWriter$19(AvroDeserializer.scala:170)
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$newWriter$19$adapted(AvroDeserializer.scala:169)
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$getRecordWriter$1(AvroDeserializer.scala:314)
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$getRecordWriter$1$adapted(AvroDeserializer.scala:310)
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$getRecordWriter$2(AvroDeserializer.scala:332)
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$getRecordWriter$2$adapted(AvroDeserializer.scala:329)
	at org.apache.spark.sql.avro.AvroDeserializer.$anonfun$converter$3(AvroDeserializer.scala:56)
	at org.apache.spark.sql.avro.AvroDeserializer.deserialize(AvroDeserializer.scala:70)

Does this PR introduce any user-facing change?

No

How was this patch tested?

The current tests already test this behavior. In essesence this patch just changes a type case to a more basic type. So I expect no functional impact.

external/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala

HyukjinKwon · 2019-12-17T12:29:54Z

ok to test

HyukjinKwon · 2019-12-17T12:30:16Z

Added a test for an array containing structs which was not yet covered.

So this case is when it becomes java.util.ArrayList?

HyukjinKwon · 2019-12-17T12:30:23Z

cc @gengliangwang FYI.

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala

SparkQA · 2019-12-17T12:58:12Z

Test build #115453 has finished for PR 26907 at commit fc4693d.

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

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala

SparkQA · 2019-12-17T13:50:22Z

Test build #115458 has finished for PR 26907 at commit 19a821a.

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

gengliangwang · 2019-12-18T00:09:41Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala

@@ -127,6 +127,25 @@ class AvroCatalystDataConversionSuite extends SparkFunSuite
    }
  }

+  test(s"array of nested schema with seed") {


I tried the test case with GenericData.Array and I can't reproduce the error you mentioned.
Do you know when the array type is not GenericData.Array ? You can also create a test case without random schema.

@gengliangwang you are right. This is an extra case which covers an extra scenario, but is was not triggering this issue. (I thought it did but it did not.)

I added an extra test to trigger the issue. I am wondering what you think of it.
Hope it does not feel too artificial. It tries to show that a GenericData can contain different list implementation types.

We hit this issue where we map an RDD[T] to a DataFrame using the AvroDeserializer. Where T is a class generate by the avro code generator. So it extends SpecifiData which extends GenericData. So all the code of the AvroDeserializer works, except of this cast, which casts to a type which is too high in the type hierarchy.

Hope it is more clear now.

Thanks for your support.

SparkQA · 2019-12-18T12:59:45Z

Test build #115511 has finished for PR 26907 at commit 0811ca3.

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

SparkQA · 2019-12-18T13:33:49Z

Test build #115513 has finished for PR 26907 at commit 190b5e8.

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

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala

external/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala

SparkQA · 2019-12-19T09:54:32Z

Test build #115555 has finished for PR 26907 at commit 48680a9.

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

steven-aerts · 2020-01-02T08:18:43Z

@gengliangwang for me this patch is ready. Let me know if I can still do something.

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala

gengliangwang

LGTM except one comment

SparkQA · 2020-01-02T11:07:12Z

Test build #116029 has finished for PR 26907 at commit 9bc6de7.

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

gengliangwang · 2020-01-02T18:47:34Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala

+    val deserializer = new AvroDeserializer(avroSchema, dataType)
+
+    def checkDeserialization(data: GenericData.Record): Unit = {
+      checkResult(


The method checkResult returns a Boolean and won't fail if the result doesn't match the expected answer.

Good catch. Fixed.

HyukjinKwon · 2020-01-03T02:05:29Z

external/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala

-          var i = 0
-          while (i < len) {
-            val element = array.get(i)
+          for ((element, i) <- array.asScala.zipWithIndex) {


Why did we change this? zipWithIndex is discouraged especially in a performance sensitive code path (https://github.com/databricks/scala-style-guide#perf-whileloops)

I kept the old counter.

The Deserializer assumed that avro arrays are always of type GenericData$Array which is not the case. Assuming they are from java.util.List is safer and fixes a ClassCastException.

steven-aerts

I updated the code so it validates the checkResult and removed the zipWithIndex.

SparkQA · 2020-01-03T08:51:13Z

Test build #116086 has finished for PR 26907 at commit e52c1ea.

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

gengliangwang · 2020-01-03T20:44:02Z

Thanks, merging to master

### What changes were proposed in this pull request? This is a follow-up of #26907 It changes the for loop `for (element <- array.asScala)` to while loop ### Why are the changes needed? As per https://github.com/databricks/scala-style-guide#traversal-and-zipwithindex, we should use while loop for the performance-sensitive code. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing tests. Closes #27127 from gengliangwang/SPARK-30267-FollowUp. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: Gengliang Wang <gengliang.wang@databricks.com>

stackfun · 2021-01-12T02:37:27Z

I'm encountering this error on the 2.4 branch. Can we have this merged to that branch as well?

HyukjinKwon reviewed Dec 17, 2019

View reviewed changes

external/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala Outdated Show resolved Hide resolved

steven-aerts force-pushed the spark-30267 branch from 67dbb80 to fc4693d Compare December 17, 2019 07:49

HyukjinKwon reviewed Dec 17, 2019

View reviewed changes

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala Show resolved Hide resolved

steven-aerts force-pushed the spark-30267 branch from fc4693d to 19a821a Compare December 17, 2019 13:13

HyukjinKwon reviewed Dec 17, 2019

View reviewed changes

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala Show resolved Hide resolved

HyukjinKwon reviewed Dec 17, 2019

View reviewed changes

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala Outdated Show resolved Hide resolved

gengliangwang reviewed Dec 18, 2019

View reviewed changes

steven-aerts force-pushed the spark-30267 branch from 19a821a to 0811ca3 Compare December 18, 2019 12:51

steven-aerts force-pushed the spark-30267 branch from 0811ca3 to 190b5e8 Compare December 18, 2019 13:02

gengliangwang reviewed Dec 18, 2019

View reviewed changes

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala Outdated Show resolved Hide resolved

gengliangwang reviewed Dec 18, 2019

View reviewed changes

external/avro/src/main/scala/org/apache/spark/sql/avro/AvroDeserializer.scala Outdated Show resolved Hide resolved

steven-aerts force-pushed the spark-30267 branch from 190b5e8 to 48680a9 Compare December 19, 2019 08:43

gengliangwang reviewed Jan 2, 2020

View reviewed changes

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroCatalystDataConversionSuite.scala Outdated Show resolved Hide resolved

gengliangwang approved these changes Jan 2, 2020

View reviewed changes

steven-aerts force-pushed the spark-30267 branch from 48680a9 to 9bc6de7 Compare January 2, 2020 10:33

gengliangwang reviewed Jan 2, 2020

View reviewed changes

HyukjinKwon reviewed Jan 3, 2020

View reviewed changes

[SPARK-30267][SQL] Avro arrays can be of any List

e52c1ea

The Deserializer assumed that avro arrays are always of type GenericData$Array which is not the case. Assuming they are from java.util.List is safer and fixes a ClassCastException.

steven-aerts force-pushed the spark-30267 branch from 9bc6de7 to e52c1ea Compare January 3, 2020 08:12

steven-aerts commented Jan 3, 2020

View reviewed changes

HyukjinKwon approved these changes Jan 3, 2020

View reviewed changes

gengliangwang closed this in e645125 Jan 3, 2020

steven-aerts deleted the spark-30267 branch January 4, 2020 13:41

gengliangwang mentioned this pull request Jan 8, 2020

[SPARK-30267][SQL][FollowUp] Use while loop in Avro Array Deserializer #27127

Closed

dongjoon-hyun added the SQL label Feb 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30267][SQL] Avro arrays can be of any List #26907

[SPARK-30267][SQL] Avro arrays can be of any List #26907

steven-aerts commented Dec 16, 2019

HyukjinKwon commented Dec 17, 2019

HyukjinKwon commented Dec 17, 2019

HyukjinKwon commented Dec 17, 2019

SparkQA commented Dec 17, 2019

SparkQA commented Dec 17, 2019

gengliangwang Dec 18, 2019 •

edited

Loading

steven-aerts Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 19, 2019

steven-aerts commented Jan 2, 2020

gengliangwang left a comment

SparkQA commented Jan 2, 2020

gengliangwang Jan 2, 2020

steven-aerts Jan 3, 2020

HyukjinKwon Jan 3, 2020

steven-aerts Jan 3, 2020

steven-aerts left a comment

SparkQA commented Jan 3, 2020

gengliangwang commented Jan 3, 2020

stackfun commented Jan 12, 2021

[SPARK-30267][SQL] Avro arrays can be of any List #26907

[SPARK-30267][SQL] Avro arrays can be of any List #26907

Conversation

steven-aerts commented Dec 16, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

HyukjinKwon commented Dec 17, 2019

HyukjinKwon commented Dec 17, 2019

HyukjinKwon commented Dec 17, 2019

SparkQA commented Dec 17, 2019

SparkQA commented Dec 17, 2019

gengliangwang Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

steven-aerts Dec 18, 2019

Choose a reason for hiding this comment

SparkQA commented Dec 18, 2019

SparkQA commented Dec 18, 2019

SparkQA commented Dec 19, 2019

steven-aerts commented Jan 2, 2020

gengliangwang left a comment

Choose a reason for hiding this comment

SparkQA commented Jan 2, 2020

gengliangwang Jan 2, 2020

Choose a reason for hiding this comment

steven-aerts Jan 3, 2020

Choose a reason for hiding this comment

HyukjinKwon Jan 3, 2020

Choose a reason for hiding this comment

steven-aerts Jan 3, 2020

Choose a reason for hiding this comment

steven-aerts left a comment

Choose a reason for hiding this comment

SparkQA commented Jan 3, 2020

gengliangwang commented Jan 3, 2020

stackfun commented Jan 12, 2021

gengliangwang Dec 18, 2019 •

edited

Loading