zaynt4606 commented on code in PR #3815:
URL: https://github.com/apache/celeborn/pull/3815#discussion_r4236951925
##########
common/src/test/java/org/apache/celeborn/common/util/ShuffleBlockInfoUtilsTest.java:
##########
@@ -143,4 +143,43 @@ public void testSliceWithEmptyMap() {
"Target buffer should remain empty with an empty indexMap",
targetByteBuf.numComponents() == 0);
}
+
+ @Test
+ public void testSliceSortedBufferByMapRangeWithMaxEndIndex() {
+ Map<Integer, List<ShuffleBlockInfo>> indexMap = new HashMap<>();
+
+ indexMap.put(0, Arrays.asList(new ShuffleBlockInfo(0, 50), new
ShuffleBlockInfo(50, 30)));
+ indexMap.put(1, Arrays.asList(new ShuffleBlockInfo(0, 20), new
ShuffleBlockInfo(20, 30)));
+
+ for (ShuffleBlockInfo blockInfo :
+
indexMap.values().stream().flatMap(List::stream).toArray(ShuffleBlockInfo[]::new))
{
+ byte[] data = new byte[(int) blockInfo.length];
+ Arrays.fill(data, (byte) blockInfo.offset); // Fill data with offset
value for simplicity
+ sortedByteBuf.addComponents(Unpooled.wrappedBuffer(data));
+ }
+
+ // A full re-read passes endMapIndex == Integer.MAX_VALUE. It must be
clamped to
+ // maxMapIndex + 1 instead of looping up to Integer.MAX_VALUE, and must
produce
+ // exactly the same result as an explicit full-range read.
+ CompositeByteBuf targetByteBufWithMaxEndIndex = Unpooled.compositeBuffer();
+ ShuffleBlockInfoUtils.sliceSortedBufferByMapRange(
+ 0,
+ Integer.MAX_VALUE,
+ indexMap,
+ sortedByteBuf,
+ targetByteBufWithMaxEndIndex,
+ shuffleChunkSize);
+
+ ShuffleBlockInfoUtils.sliceSortedBufferByMapRange(
+ 0, 2, indexMap, sortedByteBuf, targetByteBuf, shuffleChunkSize);
+
+ Assert.assertEquals(
+ "Unexpected number of components in target buffer",
+ targetByteBuf.numComponents(),
+ targetByteBufWithMaxEndIndex.numComponents());
+ Assert.assertEquals(
Review Comment:
On the current head, this `readableBytes()` assertion is still vacuous:
`addComponent(...)` does not advance either target buffer's writer index, so it
compares `0 == 0` (as noted in the [earlier
review](https://github.com/apache/celeborn/pull/3815#discussion_r4006569905)).
With this fixture, a clamp ending at `maxMapIndex` instead of `maxMapIndex + 1`
would omit map 1 (80 vs. 130 bytes), yet both results would have one component,
so the test would still pass. Could you compare the component lengths or
contents and assert the expected 130 bytes for the full reread?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
