anuragmantri commented on code in PR #17864:
URL: https://github.com/apache/iceberg/pull/17864#discussion_r3906809905
##########
core/src/main/java/org/apache/iceberg/deletes/Deletes.java:
##########
@@ -147,11 +147,15 @@ public static <T extends StructLike>
CharSequenceMap<PositionDeleteIndex> toPosi
CharSequenceMap<PositionDeleteIndex> indexes = CharSequenceMap.create();
try (CloseableIterable<T> deletes = posDeletes) {
+ String lastFilePath = null;
+ PositionDeleteIndex index = null;
for (T delete : deletes) {
CharSequence filePath = (CharSequence) FILENAME_ACCESSOR.get(delete);
long position = (long) POSITION_ACCESSOR.get(delete);
- PositionDeleteIndex index =
- indexes.computeIfAbsent(filePath, key -> new
BitmapPositionDeleteIndex(file));
+ if (lastFilePath == null || !lastFilePath.equals(filePath)) {
Review Comment:
[String.equals(Object)](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#equals(java.lang.Object))
returns `false` if the Object is not a String. In this case, the filePath is a
`CharSequence`. So this comparison returns false. I see that all the format
models return a String today but we may hit this in the future models.
```suggestion
if (lastFilePath == null || !lastFilePath.contentEquals(filePath)) {
```
##########
core/src/main/java/org/apache/iceberg/deletes/Deletes.java:
##########
@@ -147,11 +147,15 @@ public static <T extends StructLike>
CharSequenceMap<PositionDeleteIndex> toPosi
CharSequenceMap<PositionDeleteIndex> indexes = CharSequenceMap.create();
try (CloseableIterable<T> deletes = posDeletes) {
+ String lastFilePath = null;
+ PositionDeleteIndex index = null;
for (T delete : deletes) {
CharSequence filePath = (CharSequence) FILENAME_ACCESSOR.get(delete);
long position = (long) POSITION_ACCESSOR.get(delete);
- PositionDeleteIndex index =
- indexes.computeIfAbsent(filePath, key -> new
BitmapPositionDeleteIndex(file));
+ if (lastFilePath == null || !lastFilePath.equals(filePath)) {
+ lastFilePath = filePath.toString();
+ index = indexes.computeIfAbsent(filePath, key -> new
BitmapPositionDeleteIndex(file));
Review Comment:
It's worth adding a test for `toPositionIndexes`. Maybe something similar to:
```java
@Test
public void testPositionIndexesWithNonStringPaths() {
// paths are typed as CharSequence, so readers are free to return Utf8
or any other
// implementation; PATH_A is revisited after PATH_B to cover unsorted
delete files
List<Pair<CharSequence, Long>> rows =
Lists.newArrayList(
Pair.of(CharBuffer.wrap(PATH_A), 0L),
Pair.of(CharBuffer.wrap(PATH_A), 5L),
Pair.of(CharBuffer.wrap(PATH_B), 1L),
Pair.of(CharBuffer.wrap(PATH_B), 2L),
Pair.of(CharBuffer.wrap(PATH_A), 9L));
CharSequenceMap<PositionDeleteIndex> indexes =
Deletes.toPositionIndexes(CloseableIterable.withNoopClose(toDeleteRows(rows)));
assertThat(indexes).hasSize(2);
assertThat(collect(indexes.get(PATH_A))).containsExactly(0L, 5L, 9L);
assertThat(collect(indexes.get(PATH_B))).containsExactly(1L, 2L);
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
