RussellSpitzer commented on code in PR #4588:
URL: https://github.com/apache/iceberg/pull/4588#discussion_r877638371
##########
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:
I think having "isDeleted" have a side effect ruins the book keeping
simplicity for me. I'd much rather if we were to do this counter approach we
would the "delete" methods rather than the "isDeleted" method. The delete
method already has a side effect and we would expect it to be the place where
the count would happen as opposed to "isDeleted" which we have to add a JavaDoc
note to for illustrating the side effect behavior.
I still am not really sold on passing through the counter here, it just
seems to me like we are complicating the internals of this class and we don't
really need to since like we've already discussed, the value we are looking for
is already calculated even without us passing through an object. I'm also not
a big fan of behaviors being dependent on objects being null or not.
But if other reviewers feel differently I could go along with this approach
as well.
--
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]
