[#940] feat: Support columnar shuffle with gluten #950

summaryzb · 2023-06-14T06:48:24Z

What changes were proposed in this pull request?

support read and write serialized columnar data

Why are the changes needed?

Fix: #940

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UnitTest, it's covered by the existing test

client-spark/spark3/src/main/java/org/apache/spark/shuffle/reader/RssShuffleReader.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

jerqi · 2023-06-14T07:04:00Z

cc @xianjingfeng

codecov-commenter · 2023-06-14T11:17:46Z

Codecov Report

Merging #950 (54fc5f4) into master (b8f17bf) will increase coverage by 1.13%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##             master     #950      +/-   ##
============================================
+ Coverage     55.72%   56.86%   +1.13%     
- Complexity     2372     2446      +74     
============================================
  Files           357      339      -18     
  Lines         18123    16677    -1446     
  Branches       1419     1566     +147     
============================================
- Hits          10099     9483     -616     
+ Misses         7438     6621     -817     
+ Partials        586      573      -13

Impacted Files	Coverage Δ
...pache/spark/shuffle/writer/WriteBufferManager.java	`79.78% <80.00%> (+0.45%)`	⬆️

... and 24 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

xianjingfeng · 2023-06-14T11:19:11Z

Could you share the changed code in gluten? @summaryzb

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

client-spark/spark3/src/main/java/org/apache/spark/shuffle/reader/RssShuffleReader.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

client-spark/spark3/src/main/java/org/apache/spark/shuffle/reader/RssShuffleReader.java

advancedxy · 2023-06-14T14:23:10Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

@@ -141,7 +140,11 @@ public WriteBufferManager(
    this.requireMemoryInterval = bufferManagerOptions.getRequireMemoryInterval();
    this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
    this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
-    this.serializeStream = instance.serializeStream(arrayOutputStream);
+    // columnar shuffle use the serialized data directly
+    if (serializer != null) {


What if serializer == null? I didn't special code in the addPartitionData part.

When use addPartitionData to add byte already serialized, serializer is never used. What's more use ColumnarSerializer here in instance.serializeStream(arrayOutputStream) cause exception.

didn't special code in the addPartitionData part.

please see here addPartitionData

I see.

would you mind to update the JavaDoc of addPartitionData to reflect this info.

And update the comment of L143 to indicate serializer might never be used when integrating with gluten.

follow suggestion

summaryzb · 2023-06-15T02:38:22Z

Could you share the changed code in gluten? @summaryzb

Yeah, I'll contribute the patch to gluten today and post the link here later

summaryzb · 2023-06-15T05:15:50Z

Could you share the changed code in gluten? @summaryzb
PTAL @xianjingfeng @advancedxy @jerqi
apache/incubator-gluten#1966

xianjingfeng · 2023-06-16T06:05:35Z

Could you share the changed code in gluten? @summaryzb
PTAL @xianjingfeng @advancedxy @jerqi
oap-project/gluten#1966

Is it possible that UniffleShuffleManager extends RssShuffleManager and UniffleColumnarShuffleWriter extends RssShuffleWriter?

jerqi

LGTM, let @xianjingfeng take another look. @xianjingfeng You can @your colleague to ensure that this change is ok for him.

summaryzb · 2023-06-16T07:52:03Z

Is it possible that UniffleShuffleManager extends RssShuffleManager and UniffleColumnarShuffleWriter extends RssShuffleWriter?

That's really good idea, it'll make the gluten side code more simplify.
How about refactor that in next patch, since many fields type need to be changed to protected

xianjingfeng · 2023-06-16T16:40:58Z

Is it possible that UniffleShuffleManager extends RssShuffleManager and UniffleColumnarShuffleWriter extends RssShuffleWriter?

That's really good idea, it'll make the gluten side code more simplify. How about refactor that in next patch, since many fields type need to be changed to protected

It is ok for me.

xianjingfeng

LGTM

loukey-lj · 2023-06-14T07:13:58Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

@@ -128,7 +128,11 @@ public WriteBufferManager(
    super(taskMemoryManager, taskMemoryManager.pageSizeBytes(), MemoryMode.ON_HEAP);
    this.bufferSize = bufferManagerOptions.getBufferSize();
    this.spillSize = bufferManagerOptions.getBufferSpillThreshold();
-    this.instance = serializer.newInstance();
+    // columnar shuffle reader use the serialized data directly
+    if (serializer != null) {


serializer = rssHandle.getDependency().serializer(). Even with gluten, it is not null here，In my gluten 0.5 practice, the error is reported here because the serializeStream method is not implemented

@loukey-lj In writer side the serializer is specify with null directly in gluten side code, serializeStream method is not implemented, because serialize work is done in native engine layer.
In reader side it's taken from rssHandle.getDependency().serializer(), that's why only deserializeStream method is implemented

loukey-lj · 2023-06-14T07:25:08Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

+  /**
+   * add serialized columnar data directly when integrate with gluten
+   */
+  public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData) {


org.apache.spark.shuffle.writer.rssShuffleWriter#writeImpl logic can be divided into three parts: loop start, execution loop, loop end, addPartitionData This method is reserved for gluten to use, gluten's shuffleWrite logic will also have three parts, loop start, loop execution addPartitionData, end of loop, my question is loop start and loop end also need to execute uniffle logic, where is this part of the code?

Wrap uniffle writer in gluten side writer. loop start and loop end is called in it

loukey-lj · 2023-06-14T09:27:24Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

+   * add serialized columnar data directly when integrate with gluten
+   */
+  public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData) {
+    return addPartitionData(partitionId, serializedData, 0, 0L);


Is it possible to replace 0 with serializedData.length

@loukey-lj The record and bytes metric is already counted in gluten side.

support columnar shuffle

684a33d

jerqi changed the title ~~[#940][FEATURE]Support columnar shuffle with gluten~~ [#940] feat: Support columnar shuffle with gluten Jun 14, 2023

merge the empty partion condition

fd57456

jerqi reviewed Jun 14, 2023

View reviewed changes

client-spark/spark3/src/main/java/org/apache/spark/shuffle/reader/RssShuffleReader.java Show resolved Hide resolved

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java Outdated Show resolved Hide resolved

jerqi requested a review from xianjingfeng June 14, 2023 07:04

add test

7e00d63

fix checkstyle

6b4ec01

summaryzb force-pushed the gluten_uniffle branch from 5d4bf13 to 6b4ec01 Compare June 14, 2023 12:05

jerqi reviewed Jun 14, 2023

View reviewed changes

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java Outdated Show resolved Hide resolved

advancedxy reviewed Jun 14, 2023

View reviewed changes

fix test

42702ee

summaryzb added 2 commits June 15, 2023 15:40

remove rssShuffleHandle variable

2b4e868

fix test buffer size

54fc5f4

add some comment

490ee0e

jerqi approved these changes Jun 16, 2023

View reviewed changes

xianjingfeng approved these changes Jun 16, 2023

View reviewed changes

xianjingfeng merged commit bb7f9c3 into apache:master Jun 16, 2023

loukey-lj reviewed Jun 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#940] feat: Support columnar shuffle with gluten #950

[#940] feat: Support columnar shuffle with gluten #950

summaryzb commented Jun 14, 2023 •

edited by jerqi

Loading

jerqi commented Jun 14, 2023

codecov-commenter commented Jun 14, 2023 •

edited

Loading

xianjingfeng commented Jun 14, 2023

advancedxy Jun 14, 2023

summaryzb Jun 15, 2023

summaryzb Jun 15, 2023

advancedxy Jun 15, 2023

summaryzb Jun 16, 2023

summaryzb commented Jun 15, 2023

summaryzb commented Jun 15, 2023 •

edited

Loading

xianjingfeng commented Jun 16, 2023

jerqi left a comment

summaryzb commented Jun 16, 2023

xianjingfeng commented Jun 16, 2023

xianjingfeng left a comment

loukey-lj Jun 14, 2023

summaryzb Oct 16, 2023 •

edited

Loading

loukey-lj Jun 14, 2023

summaryzb Oct 16, 2023

loukey-lj Jun 14, 2023

summaryzb Oct 16, 2023 •

edited

Loading

[#940] feat: Support columnar shuffle with gluten #950

[#940] feat: Support columnar shuffle with gluten #950

Conversation

summaryzb commented Jun 14, 2023 • edited by jerqi Loading

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?

jerqi commented Jun 14, 2023

codecov-commenter commented Jun 14, 2023 • edited Loading

Codecov Report

xianjingfeng commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summaryzb commented Jun 15, 2023

summaryzb commented Jun 15, 2023 • edited Loading

xianjingfeng commented Jun 16, 2023

jerqi left a comment

Choose a reason for hiding this comment

summaryzb commented Jun 16, 2023

xianjingfeng commented Jun 16, 2023

xianjingfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summaryzb Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summaryzb Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

summaryzb commented Jun 14, 2023 •

edited by jerqi

Loading

codecov-commenter commented Jun 14, 2023 •

edited

Loading

summaryzb commented Jun 15, 2023 •

edited

Loading

summaryzb Oct 16, 2023 •

edited

Loading

summaryzb Oct 16, 2023 •

edited

Loading