openinx commented on a change in pull request #2372:
URL: https://github.com/apache/iceberg/pull/2372#discussion_r607683442
##########
File path: core/src/main/java/org/apache/iceberg/deletes/Deletes.java
##########
@@ -233,6 +246,37 @@ public void close() {
}
}
+ static class PositionStreamDeletedRowMarker<T> extends
PositionStreamDeleteFilter<T> {
+ private final Consumer<T> deleteMarker;
+
+ private PositionStreamDeletedRowMarker(CloseableIterable<T> rows,
Function<T, Long> extractPos,
+ CloseableIterable<Long>
deletePositions,
+ Consumer<T> deleteMarker) {
+ super(rows, extractPos, deletePositions);
+ this.deleteMarker = deleteMarker;
+ }
+
+ @Override
+ protected FilterIterator<T> positionIterator(CloseableIterator<T> items,
+ CloseableIterator<Long>
deletePositions) {
+ return new PositionMarkerIterator(items, deletePositions);
+ }
+
+ private class PositionMarkerIterator extends PositionFilterIterator {
+ protected PositionMarkerIterator(CloseableIterator<T> items,
CloseableIterator<Long> deletePositions) {
+ super(items, deletePositions);
+ }
+
+ @Override
+ protected boolean shouldKeep(T row) {
+ if (!super.shouldKeep(row)) {
+ deleteMarker.accept(row);
Review comment:
Personally, it's really strange that we will modify the original row
(set `_deleted` flag to be true) in a `shouldKeep` method because it should be
a pure test method and should not modify certain states of the record.
Otherwise, it is easy to cause confusion: after a record has undergone a
different sequence of shouldKeep tests, the final result is different, and even
the original record is modified.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
