rdblue commented on code in PR #18704:
URL: https://github.com/apache/iceberg/pull/18704#discussion_r4428770453
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmap.java:
##########
@@ -307,9 +312,49 @@
return 0;
}
+
+ private Container overlay(int containerIndex) {
+ Container overlay = overlays.get(containerIndex);
+ if (overlay == null) {
+ overlay = Container.newOverlay(base, containerIndex);
+ overlays.put(containerIndex, overlay);
+ }
+
+ return overlay;
+ }
+
+ private void merge(Mutable other) {
+ int[] descriptors = other.descriptorArray();
+ for (int containerIndex = 0; containerIndex < descriptors.length;
containerIndex += 1) {
+ if (descriptors[containerIndex] != 0) {
+ Container overlay = overlay(containerIndex);
+ this.cardinality += overlay.merge(other.overlay(containerIndex));
Review Comment:
I don't see much benefit to doing it this way. `Mutable.overlay` already
creates or returns an existing overlay. The only benefit to the suggested
change is that it discards any new overlays rather than keeping them. Keeping
the overlay doesn't affect the contract of the mutable bitmap. It might cause
the merged bitmap to take slightly longer to serialize, but it is unlikely that
a bitmap will be used at all once it has been merged into another.
--
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]
