nastra commented on code in PR #18027:
URL: https://github.com/apache/iceberg/pull/18027#discussion_r4035815628
##########
core/src/main/java/org/apache/iceberg/deletes/PositionDeleteIndex.java:
##########
@@ -79,6 +80,32 @@ default void forEach(LongConsumer consumer) {
}
}
+ /**
+ * Traverses the deleted positions within the given range in ascending
order, applying the
+ * provided consumer.
+ *
+ * <p>Callers that test a contiguous range of positions should prefer this
method over calling
+ * {@link #isDeleted(long)} once per position.
+ *
+ * @param posStart inclusive beginning of position range
+ * @param posEnd exclusive ending of position range
+ * @param consumer a consumer for the deleted positions in the range
+ * @throws IllegalArgumentException if posStart > posEnd
+ */
+ default void forEachInRange(long posStart, long posEnd, LongConsumer
consumer) {
Review Comment:
```suggestion
default void forEachInRange(long posStartInclusive, long posEndExclusive,
LongConsumer consumer) {
```
we use similar naming in RoaringPositionBitmap.setRange(..), so I think
being explicit about the ranges is helpful. Please also update the implementing
methods
##########
core/src/main/java/org/apache/iceberg/deletes/PositionDeleteIndex.java:
##########
@@ -79,6 +80,32 @@ default void forEach(LongConsumer consumer) {
}
}
+ /**
+ * Traverses the deleted positions within the given range in ascending
order, applying the
+ * provided consumer.
+ *
+ * <p>Callers that test a contiguous range of positions should prefer this
method over calling
Review Comment:
not sure if that's helpful, maybe just remove this
##########
core/src/test/java/org/apache/iceberg/deletes/TestBitmapPositionDeleteIndex.java:
##########
@@ -74,6 +78,116 @@ public void testForEachEmptyIndex() {
assertThat(positions).isEmpty();
}
+ @Test
+ public void testForEachInRange() {
+ PositionDeleteIndex index = indexOf(10L, 11L, 12L, 13L);
+
+ // the beginning is inclusive and the end is exclusive
+ assertThat(collect(index, 11L, 13L)).containsExactly(11L, 12L);
+ assertThat(collect(index, 11L, 12L)).containsExactly(11L);
+
+ // a range that ends where the deletes start
+ assertThat(collect(index, 0L, 10L)).isEmpty();
+
+ // a range that starts after the deletes end
+ assertThat(collect(index, 14L, 24L)).isEmpty();
+ }
+
+ @Test
+ public void testForEachInRangeAscendingOrder() {
+ PositionDeleteIndex index = indexOf(9L, 3L, 7L, 1L, 5L);
+ assertThat(collect(index, 0L, 20L)).containsExactly(1L, 3L, 5L, 7L, 9L);
+ }
+
+ @Test
+ public void testForEachInRangeEmptyRange() {
+ PositionDeleteIndex index = indexOf(0L, 1L, 2L);
+ assertThat(collect(index, 0L, 0L)).isEmpty();
Review Comment:
we also need tests where end < start
##########
core/src/main/java/org/apache/iceberg/deletes/EmptyPositionDeleteIndex.java:
##########
@@ -48,6 +51,15 @@ public boolean isEmpty() {
return true;
}
+ @Override
+ public void forEachInRange(long posStart, long posEnd, LongConsumer
consumer) {
+ Preconditions.checkArgument(
Review Comment:
what about just throwing an UOE stating that this isn't supported? Not sure
we're adding any value by checking the start/end but essentially not doing
anything
##########
core/src/main/java/org/apache/iceberg/deletes/PositionDeleteIndex.java:
##########
@@ -79,6 +80,32 @@ default void forEach(LongConsumer consumer) {
}
}
+ /**
+ * Traverses the deleted positions within the given range in ascending
order, applying the
+ * provided consumer.
+ *
+ * <p>Callers that test a contiguous range of positions should prefer this
method over calling
+ * {@link #isDeleted(long)} once per position.
+ *
+ * @param posStart inclusive beginning of position range
+ * @param posEnd exclusive ending of position range
+ * @param consumer a consumer for the deleted positions in the range
+ * @throws IllegalArgumentException if posStart > posEnd
+ */
+ default void forEachInRange(long posStart, long posEnd, LongConsumer
consumer) {
Review Comment:
maybe we should make the predicate part of the method parameters
##########
core/src/main/java/org/apache/iceberg/deletes/PositionDeleteIndex.java:
##########
@@ -79,6 +80,32 @@ default void forEach(LongConsumer consumer) {
}
}
+ /**
+ * Traverses the deleted positions within the given range in ascending
order, applying the
+ * provided consumer.
+ *
+ * <p>Callers that test a contiguous range of positions should prefer this
method over calling
+ * {@link #isDeleted(long)} once per position.
+ *
+ * @param posStart inclusive beginning of position range
+ * @param posEnd exclusive ending of position range
+ * @param consumer a consumer for the deleted positions in the range
+ * @throws IllegalArgumentException if posStart > posEnd
+ */
+ default void forEachInRange(long posStart, long posEnd, LongConsumer
consumer) {
Review Comment:
also I think it's not entirely clear from the method naming that this will
only look at deleted positions in the range. `forEach` itself iterates over all
positions, so one would assume that `forEachInRange` would do the same without
checked for deleted positions
--
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]
