You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "summaryzb (via GitHub)" <gi...@apache.org> on 2023/06/20 09:32:22 UTC
[GitHub] [incubator-uniffle] summaryzb opened a new pull request, #958: [#940] improvement: make the shuffle
summaryzb opened a new pull request, #958:
URL: https://github.com/apache/incubator-uniffle/pull/958
### What changes were proposed in this pull request?
1. Make it possible to extend uniffle in spark3.
2. Optimize the shuffleMetric when use columnar shuffle
### Why are the changes needed?
https://github.com/apache/incubator-uniffle/issues/940
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Unit test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241799783
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
> > We can pass a non-serializer and ColumnarBatch to `addRecord` method although we use columnar shuffle. It's not equivalent.
>
> Not only serialization but also partitioning should be handled and are both related with the implementation of the third party columnar framework, we'd better handle them outside of `WriterBufferManager`
For a shuffle, we can handle partitioning by the `partitioner`. You mean that partitioner is null for gluten, won't it?
In another word, we may use null serializer row based shuffle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#issuecomment-1599974733
Should we make all fields as protected variables?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1235485802
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -185,7 +187,10 @@ public List<ShuffleBlockInfo> addPartitionData(
wb.addRecord(serializedData, serializedDataLength);
buffers.put(partitionId, wb);
}
- shuffleWriteMetrics.incRecordsWritten(1L);
+ // records is a row based semantic
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
Review Comment:
Could we add more comments to explain why we need this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241611503
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -110,13 +107,20 @@ public class RssShuffleManager extends RssShuffleManagerBase {
private String user;
private String uuid;
private Set<String> failedTaskIds = Sets.newConcurrentHashSet();
- private DataPusher dataPusher;
private final Map<Integer, Integer> shuffleIdToPartitionNum = Maps.newConcurrentMap();
private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();
private ShuffleManagerGrpcService service;
private GrpcServer shuffleManagerServer;
+ /**
+ * used by columnar rss shuffle writer implementation
+ */
+ protected AtomicReference<String> id = new AtomicReference<>();
+ protected SparkConf sparkConf;
+ protected ShuffleWriteClient shuffleWriteClient;
+ protected DataPusher dataPusher;
Review Comment:
U'r right, `id` and `dataPusher` need to be extract to a method.
but [sparkConf](https://github.com/oap-project/gluten/blob/1f020a8484e23a9ec0ef0351d77652bae303c548/gluten-uniffle/src/main/java/org/apache/spark/shuffle/gluten/uniffle/UniffleShuffleManager.java#L66) and `shuffleWriteClient` are just used in some constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1240965850
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -185,7 +187,10 @@ public List<ShuffleBlockInfo> addPartitionData(
wb.addRecord(serializedData, serializedDataLength);
buffers.put(partitionId, wb);
}
- shuffleWriteMetrics.incRecordsWritten(1L);
+ // records is a row based semantic
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
Review Comment:
Comments are added, Yes it's a common case.
All columnar shuffle use it's own serializer, all the serializer related work that is binded with implementation of the columnar framework not limited to gluten should be handled in the rss columnar shuffle writer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241714439
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/writer/RssShuffleWriter.java:
##########
@@ -127,7 +127,7 @@ public RssShuffleWriter(
ShuffleWriteClient shuffleWriteClient,
RssShuffleHandle<K, V, C> rssHandle,
Function<String, Boolean> taskFailureCallback) {
- LOG.warn("RssShuffle start write taskAttemptId data" + taskAttemptId);
+ LOG.warn("RssShuffleaskAttempt start write taskAttemptId data" + taskAttemptId);
Review Comment:
There is a misspell.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1242108397
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
Could we have null serializer for row based shuffle in the future? I think the answer is yes. Because some spark data don't need serialization although the data is organized by row format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241466450
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
> Can columnar shuffle be supported by adding configuration or adding a new constructor? This judgment seems a bit hard
+1. It seems a bit counterintuitive to rely on `serializer == null` to check whether the buffer writer manager should support columnar shuffle or not.
Is is possible for gluten to pass additional conf items to spark conf and in Uniffle side we can add a `columnarSupport` field in the BufferManagerOptions class.
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -110,13 +107,20 @@ public class RssShuffleManager extends RssShuffleManagerBase {
private String user;
private String uuid;
private Set<String> failedTaskIds = Sets.newConcurrentHashSet();
- private DataPusher dataPusher;
private final Map<Integer, Integer> shuffleIdToPartitionNum = Maps.newConcurrentMap();
private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();
private ShuffleManagerGrpcService service;
private GrpcServer shuffleManagerServer;
+ /**
+ * used by columnar rss shuffle writer implementation
+ */
+ protected AtomicReference<String> id = new AtomicReference<>();
+ protected SparkConf sparkConf;
+ protected ShuffleWriteClient shuffleWriteClient;
+ protected DataPusher dataPusher;
Review Comment:
Could you point to the gluten impl again?
It doesn't feel right to just expose these fields here, especially the id and sparkConf fields, they are not exposed in Spark's original shuffle manager.
I think these fields should be accessed at least via a getter/method, so uniffle is free to change its implementation later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1242108397
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
Could we have null serializer for row based shuffle in the future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241023132
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -185,7 +187,10 @@ public List<ShuffleBlockInfo> addPartitionData(
wb.addRecord(serializedData, serializedDataLength);
buffers.put(partitionId, wb);
}
- shuffleWriteMetrics.incRecordsWritten(1L);
+ // records is a row based semantic
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
Review Comment:
Could we move this code to the method `addRecord`? Columnar shuffle won't call the `addRecord` method, will it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241450000
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -185,7 +187,10 @@ public List<ShuffleBlockInfo> addPartitionData(
wb.addRecord(serializedData, serializedDataLength);
buffers.put(partitionId, wb);
}
- shuffleWriteMetrics.incRecordsWritten(1L);
+ // records is a row based semantic
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
Review Comment:
That's a good idea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241710622
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -221,6 +216,11 @@ public List<ShuffleBlockInfo> addRecord(int partitionId, Object key, Object valu
if (serializedDataLength == 0) {
return null;
}
+ // records is a row based semantic, when in columnar shuffle records num should be taken from ColumnarBatch
+ // that is handled by rss shuffle writer implementation
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
+ }
Review Comment:
How about
```
List<ShuffleBlockInfo> dataPartition = addPartitionData(partitionId, serializedData, serializedDataLength, start);
shuffleWriteMetrics.incRecordsWritten(1L);
return dataPartition;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1240999710
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
We can pass a non-serializer and ColumnarRowBatch to `addRecord` method although we use columnar shuffle. It's not equivalent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241595501
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
> Is is possible for gluten to pass additional conf items to spark conf and in Uniffle side we can add a `columnarSupport` field in the BufferManagerOptions class.
Maybe it's not a good idea to change the spark conf, since the columnar shuffle maybe fallback to row based shuffle in different stage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241710622
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -221,6 +216,11 @@ public List<ShuffleBlockInfo> addRecord(int partitionId, Object key, Object valu
if (serializedDataLength == 0) {
return null;
}
+ // records is a row based semantic, when in columnar shuffle records num should be taken from ColumnarBatch
+ // that is handled by rss shuffle writer implementation
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
+ }
Review Comment:
How about
```
List<ShuffleBlockInfo> dataPartition = addPartitionData(partitionId, serializedData, serializedDataLength, start);
if (isRowBased) {
shuffleWriteMetrics.incRecordsWritten(1L);
}
return dataPartition;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng merged pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng merged PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1243216967
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
How about indicate row based shuffle false in `rssConf` @jerqi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1243221348
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
Ok for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1240968713
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -88,34 +88,34 @@
public class RssShuffleManager extends RssShuffleManagerBase {
private static final Logger LOG = LoggerFactory.getLogger(RssShuffleManager.class);
- private final String clientType;
- private final long heartbeatInterval;
- private final long heartbeatTimeout;
- private AtomicReference<String> id = new AtomicReference<>();
- private SparkConf sparkConf;
- private final int dataReplica;
- private final int dataReplicaWrite;
- private final int dataReplicaRead;
- private final boolean dataReplicaSkipEnabled;
- private final int dataTransferPoolSize;
- private final int dataCommitPoolSize;
- private ShuffleWriteClient shuffleWriteClient;
- private final Map<String, Set<Long>> taskToSuccessBlockIds;
- private final Map<String, Set<Long>> taskToFailedBlockIds;
- private ScheduledExecutorService heartBeatScheduledExecutorService;
- private boolean heartbeatStarted = false;
- private boolean dynamicConfEnabled = false;
- private final ShuffleDataDistributionType dataDistributionType;
- private final int maxConcurrencyPerPartitionToWrite;
- private String user;
- private String uuid;
- private Set<String> failedTaskIds = Sets.newConcurrentHashSet();
- private DataPusher dataPusher;
-
- private final Map<Integer, Integer> shuffleIdToPartitionNum = Maps.newConcurrentMap();
- private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();
- private ShuffleManagerGrpcService service;
- private GrpcServer shuffleManagerServer;
+ protected final String clientType;
Review Comment:
Follow the suggestion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] LuciferYang commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1236250332
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -88,34 +88,34 @@
public class RssShuffleManager extends RssShuffleManagerBase {
private static final Logger LOG = LoggerFactory.getLogger(RssShuffleManager.class);
- private final String clientType;
- private final long heartbeatInterval;
- private final long heartbeatTimeout;
- private AtomicReference<String> id = new AtomicReference<>();
- private SparkConf sparkConf;
- private final int dataReplica;
- private final int dataReplicaWrite;
- private final int dataReplicaRead;
- private final boolean dataReplicaSkipEnabled;
- private final int dataTransferPoolSize;
- private final int dataCommitPoolSize;
- private ShuffleWriteClient shuffleWriteClient;
- private final Map<String, Set<Long>> taskToSuccessBlockIds;
- private final Map<String, Set<Long>> taskToFailedBlockIds;
- private ScheduledExecutorService heartBeatScheduledExecutorService;
- private boolean heartbeatStarted = false;
- private boolean dynamicConfEnabled = false;
- private final ShuffleDataDistributionType dataDistributionType;
- private final int maxConcurrencyPerPartitionToWrite;
- private String user;
- private String uuid;
- private Set<String> failedTaskIds = Sets.newConcurrentHashSet();
- private DataPusher dataPusher;
-
- private final Map<Integer, Integer> shuffleIdToPartitionNum = Maps.newConcurrentMap();
- private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();
- private ShuffleManagerGrpcService service;
- private GrpcServer shuffleManagerServer;
+ protected final String clientType;
Review Comment:
why change all field to `protected`? If this is necessary, it is also need to add some code comments to explain
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -88,34 +88,34 @@
public class RssShuffleManager extends RssShuffleManagerBase {
private static final Logger LOG = LoggerFactory.getLogger(RssShuffleManager.class);
- private final String clientType;
- private final long heartbeatInterval;
- private final long heartbeatTimeout;
- private AtomicReference<String> id = new AtomicReference<>();
- private SparkConf sparkConf;
- private final int dataReplica;
- private final int dataReplicaWrite;
- private final int dataReplicaRead;
- private final boolean dataReplicaSkipEnabled;
- private final int dataTransferPoolSize;
- private final int dataCommitPoolSize;
- private ShuffleWriteClient shuffleWriteClient;
- private final Map<String, Set<Long>> taskToSuccessBlockIds;
- private final Map<String, Set<Long>> taskToFailedBlockIds;
- private ScheduledExecutorService heartBeatScheduledExecutorService;
- private boolean heartbeatStarted = false;
- private boolean dynamicConfEnabled = false;
- private final ShuffleDataDistributionType dataDistributionType;
- private final int maxConcurrencyPerPartitionToWrite;
- private String user;
- private String uuid;
- private Set<String> failedTaskIds = Sets.newConcurrentHashSet();
- private DataPusher dataPusher;
-
- private final Map<Integer, Integer> shuffleIdToPartitionNum = Maps.newConcurrentMap();
- private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();
- private ShuffleManagerGrpcService service;
- private GrpcServer shuffleManagerServer;
+ protected final String clientType;
Review Comment:
why change all fields to `protected`? If this is necessary, it is also need to add some code comments to explain
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] loukey-lj commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "loukey-lj (via GitHub)" <gi...@apache.org>.
loukey-lj commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1236226003
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
Can columnar shuffle be supported by adding configuration or adding a new constructor? This judgment seems a bit hard
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] LuciferYang commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1236244337
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
Good point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241471439
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
> We can pass a non-serializer and ColumnarBatch to `addRecord` method although we use columnar shuffle. It's not equivalent.
Not only serialization but also partitioning should be handled and are both related with the implementation of the third party columnar framework, we'd better handle them outside of `WriterBufferManager`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1242101654
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
@jerqi When in columnar shuffle, an element of the iterator is a `ColumnarBatch`, it's consists of many rows which will be partitioned to different partitionIds, current spark partitioner api cannot handle this scenario.
Actually the java partitioner is fake when use columnar shuffle in gluten, partitioner is implemented in cpp layer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1235015194
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -185,7 +187,10 @@ public List<ShuffleBlockInfo> addPartitionData(
wb.addRecord(serializedData, serializedDataLength);
buffers.put(partitionId, wb);
}
- shuffleWriteMetrics.incRecordsWritten(1L);
+ // records is a row based semantic
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
Review Comment:
replace with [this](https://github.com/oap-project/gluten/blob/1f020a8484e23a9ec0ef0351d77652bae303c548/gluten-uniffle/src/main/java/org/apache/spark/shuffle/writer/UniffleColumnarShuffleWriter.java#L169)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#issuecomment-1605771747
> Should we make all fields as protected variables?
No, fix this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1240999710
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
We can pass a non-serializer and ColumnarBatch to `addRecord` method although we use columnar shuffle. It's not equivalent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241772059
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -221,6 +216,11 @@ public List<ShuffleBlockInfo> addRecord(int partitionId, Object key, Object valu
if (serializedDataLength == 0) {
return null;
}
+ // records is a row based semantic, when in columnar shuffle records num should be taken from ColumnarBatch
+ // that is handled by rss shuffle writer implementation
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
+ }
Review Comment:
That's better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1241595501
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
> Is is possible for gluten to pass additional conf items to spark conf and in Uniffle side we can add a `columnarSupport` field in the BufferManagerOptions class.
Maybe it's not a good idea to change the spark conf, since the columnar shuffle maybe fallback to row based shuffle
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1235490599
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
Is it common case? if we support other columnar shuffle like rapids, the other columnar shuffle may use non-null serializer.
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -185,7 +187,10 @@ public List<ShuffleBlockInfo> addPartitionData(
wb.addRecord(serializedData, serializedDataLength);
buffers.put(partitionId, wb);
}
- shuffleWriteMetrics.incRecordsWritten(1L);
+ // records is a row based semantic
+ if (isRowBased) {
+ shuffleWriteMetrics.incRecordsWritten(1L);
Review Comment:
Is it a common case? I think it binded the implement of gluten. Could we have more common interface?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on a diff in pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on code in PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#discussion_r1240968568
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -141,7 +142,8 @@ public WriteBufferManager(
this.requireMemoryRetryMax = bufferManagerOptions.getRequireMemoryRetryMax();
this.arrayOutputStream = new WrappedByteArrayOutputStream(serializerBufferSize);
// in columnar shuffle, the serializer here is never used
- if (serializer != null) {
+ this.isRowBased = (serializer != null);
Review Comment:
In my opinion, all columnar shuffle should pass null serializer to `WriterBufferManager`.
1. `WriterBufferManager` should only do the buffer related work as it's name mean
2. Columnar data framework is various, the integration work should be done in the implementation of `rss columnar shuffle writer`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#issuecomment-1606524775
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/958?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#958](https://app.codecov.io/gh/apache/incubator-uniffle/pull/958?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5326c92) into [master](https://app.codecov.io/gh/apache/incubator-uniffle/commit/8a0ae4bf82a3dcb3ee43da34cd052552aec536b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8a0ae4b) will **increase** coverage by `0.82%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #958 +/- ##
============================================
+ Coverage 55.00% 55.83% +0.82%
+ Complexity 2466 2065 -401
============================================
Files 367 304 -63
Lines 19237 13187 -6050
Branches 1579 1220 -359
============================================
- Hits 10582 7363 -3219
+ Misses 8008 5421 -2587
+ Partials 647 403 -244
```
[see 75 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-uniffle/pull/958/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#issuecomment-1606689235
> Another thing about this integration, do we have any integration tests for this, so that we can caught any incompatibility before releasing.
Since Uniffle is kind of dependency for gluten, the integration tests should be placed in gluten side, this will be done after the release. For example we shall not test spark integration in `hadoop-common` project, acctually we fix the hadoop incompatibility in spark side.
Currently i run the tpcds in our production env to test the integration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] summaryzb commented on pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "summaryzb (via GitHub)" <gi...@apache.org>.
summaryzb commented on PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#issuecomment-1610725698
gentle ping @xianjingfeng PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #958: [#940] improvement: Optimize columnar shuffle integration
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #958:
URL: https://github.com/apache/incubator-uniffle/pull/958#issuecomment-1610803211
Merged. Thanks all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org