wypoon commented on code in PR #4588:
URL: https://github.com/apache/iceberg/pull/4588#discussion_r856688857
##########
core/src/main/java/org/apache/iceberg/deletes/BitmapPositionDeleteIndex.java:
##########
@@ -40,7 +46,11 @@ public void delete(long posStart, long posEnd) {
@Override
public boolean isDeleted(long position) {
- return roaring64Bitmap.contains(position);
+ boolean posIsDeleted = roaring64Bitmap.contains(position);
+ if (counter != null && posIsDeleted) {
+ counter.increment();
+ }
Review Comment:
There are existing tests that have no need for counting deletes, that call
some static methods in `Deletes`. I kept those forms of the methods and had
them call new forms that take a `DeleteCounter`, passing a null
`DeleteCounter`. That is the reason why a `DeleteCounter` field might be null.
The way the code is now, if we decide to do without `DeleteCounter` and simply
use an `AtomicLong`, then it can be updated very easily. I do understand your
suggestion for a `NullDeleteCounter`.
You raise a good point about `PositionDeleteIndex#isDeleted` not being
idempotent now. I have checked where it is called, and it is not a concern. We
should document this point though, to prevent it being a problem in future
code. Before I ended up introducing a delete counter into the
`PositionDeleteIndex` implementation and incrementing it when `isDeleted` is
called, I considered other approaches, but there was one read path in
particular where this seemed to the best solution.
--
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]
