[SPARK-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization … #24357

pengbo · 2019-04-12T10:13:18Z

What changes were proposed in this pull request?

Finish the rest work of #24317, #9030
a. Implement Kryo serialization for UnsafeArrayData
b. fix UnsafeMapData Java/Kryo Serialization issue when two machines have different Oops size
c. Move the duplicate code "getBytes()" to Utils.

How was this patch tested?

According Units has been added & tested

…breaks when two machines have different Oops size

pengbo · 2019-04-12T10:16:08Z

@srowen

I have put Unsafe data class in KryoSerializer.newKryo.
I reopen the PR with only 1 commit for further discussion.

Please recheck when available. Thanks

srowen · 2019-04-12T14:37:49Z

CC @cloud-fan

kiszk · 2019-04-14T18:19:17Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeDataUtils.java

+ */
+final class UnsafeDataUtils {
+
+    private UnsafeDataUtils() {


nit: use 2-indent

Thanks for pointing it out.

kiszk · 2019-04-14T18:43:24Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeMapData.java

 import org.apache.spark.sql.catalyst.util.MapData;
 import org.apache.spark.unsafe.Platform;

+import java.io.Externalizable;


It looks incorrect import order. org.apache should be last.

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeMapSuite.scala

kiszk · 2019-04-14T19:03:50Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeMapSuite.scala

+    val valueArray = new UnsafeArrayData()
+    Platform.putLong(baseObject, offset + 8 + keyArray.getSizeInBytes, 1)
+    valueArray.pointTo(baseObject, offset + 8 + keyArray.getSizeInBytes, keyArraySize)
+    valueArray.setLong(0, 19285)


It may be good to use different values among key and value.

Actually I don't think that makes any different, can you please explain a little more?
Thanks.

We hope so. For testing typical use cases, I think that it is good to allocate a separate array for map and value.

Maybe I am missing sth, the key/value array are currently different arrays. The value set is the same (0 -> 19285).

How about using a different value for key and value, not just 19285 both times? that would make a slightly better test. Otherwise you'd possibly miss a weird bug where key and value arrays are swapped

Good point. Thanks @kiszk @srowen

…7416_new

cloud-fan · 2019-04-15T13:48:39Z

ok to test

kiszk · 2019-04-15T15:26:59Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeDataUtils.java

+    }
+    byte[] bytes = new byte[sizeInBytes];
+    Platform.copyMemory(baseObject, baseOffset, bytes, Platform.BYTE_ARRAY_OFFSET,
+        sizeInBytes);


super nit: probably 2-indent

Done. Thanks

SparkQA · 2019-04-15T18:36:56Z

Test build #104590 has finished for PR 24357 at commit ca2bf06.

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

…map test

pengbo · 2019-04-16T09:06:30Z

retest this please.

SparkQA · 2019-04-16T13:54:11Z

Test build #104621 has finished for PR 24357 at commit 357144c.

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

cloud-fan · 2019-04-16T16:12:34Z

thanks, merging to master!

Finish the rest work of apache#24317, apache#9030 a. Implement Kryo serialization for UnsafeArrayData b. fix UnsafeMapData Java/Kryo Serialization issue when two machines have different Oops size c. Move the duplicate code "getBytes()" to Utils. According Units has been added & tested Closes apache#24357 from pengbo/SPARK-27416_new. Authored-by: pengbo <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…erialization … This is a Spark 2.4.x backport of #24357 by pengbo. Original description follows below: --- ## What changes were proposed in this pull request? Finish the rest work of #24317, #9030 a. Implement Kryo serialization for UnsafeArrayData b. fix UnsafeMapData Java/Kryo Serialization issue when two machines have different Oops size c. Move the duplicate code "getBytes()" to Utils. ## How was this patch tested? According Units has been added & tested Closes #25223 from JoshRosen/SPARK-27416-2.4. Authored-by: pengbo <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…erialization … This is a Spark 2.4.x backport of apache#24357 by pengbo. Original description follows below: --- ## What changes were proposed in this pull request? Finish the rest work of apache#24317, apache#9030 a. Implement Kryo serialization for UnsafeArrayData b. fix UnsafeMapData Java/Kryo Serialization issue when two machines have different Oops size c. Move the duplicate code "getBytes()" to Utils. ## How was this patch tested? According Units has been added & tested Closes apache#25223 from JoshRosen/SPARK-27416-2.4. Authored-by: pengbo <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…erialization This is a Spark 2.3.x backport of a 2.4.x backport of apache#24357 by pengbo. Original description follows below: --- Finish the rest work of apache#24317, apache#9030 a. Implement Kryo serialization for UnsafeArrayData b. fix UnsafeMapData Java/Kryo Serialization issue when two machines have different Oops size c. Move the duplicate code "getBytes()" to Utils. According Units has been added & tested Authored-by: pengbo <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> Signed-off-by: Jason Altekruse <[email protected]>

[Spark-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization …

70de399

…breaks when two machines have different Oops size

kiszk reviewed Apr 14, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/UnsafeMapSuite.scala Show resolved Hide resolved

kiszk reviewed Apr 14, 2019

View reviewed changes

pengbo added 2 commits April 15, 2019 12:07

Merge branch 'master' of https://github.com/apache/spark into SPARK-2…

6b674d3

…7416_new

java style fixing

ca2bf06

kiszk reviewed Apr 15, 2019

View reviewed changes

fixing java indent style issue & make key/value different for unsafe …

357144c

…map test

kiszk approved these changes Apr 16, 2019

View reviewed changes

srowen approved these changes Apr 16, 2019

View reviewed changes

cloud-fan changed the title ~~[Spark-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization …~~ [SPARK-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization … Apr 16, 2019

cloud-fan closed this in 54b0d1e Apr 17, 2019

JoshRosen mentioned this pull request Jul 22, 2019

[SPARK-27416][SQL][BRANCH-2.4] UnsafeMapData & UnsafeArrayData Kryo serialization … #25223

Closed

[SPARK-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization … #24357

[SPARK-27416][SQL]UnsafeMapData & UnsafeArrayData Kryo serialization … #24357

Uh oh!

Conversation

pengbo commented Apr 12, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

pengbo commented Apr 12, 2019

Uh oh!

srowen commented Apr 12, 2019

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!

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 commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 15, 2019

Uh oh!

pengbo commented Apr 16, 2019

Uh oh!

SparkQA commented Apr 16, 2019

Uh oh!

cloud-fan commented Apr 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants