stevenzwu commented on code in PR #18704:
URL: https://github.com/apache/iceberg/pull/18704#discussion_r4419024649
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmap.java:
##########
@@ -54,7 +55,7 @@ class MumblingBitmap implements ManifestBitmap {
private static final int MAX_CONTAINER_COUNT = 8_192;
private static final int MAX_CARDINALITY = 2_097_152;
- static MutableBitmap newMutableBitmap() {
+ static Mutable newMutableBitmap() {
Review Comment:
why do we change the return type from interface to the impl class?
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmap.java:
##########
@@ -307,9 +312,49 @@ private int copyOriginalContainers(
return 0;
}
+
+ private Container overlay(int containerIndex) {
+ Container overlay = overlays.get(containerIndex);
+ if (overlay == null) {
+ overlay = Container.newOverlay(base, containerIndex);
+ overlays.put(containerIndex, overlay);
Review Comment:
I was expecting this method to be a pure getter without state mutation side
effect.
updating the `overlays` map probably is better done by the caller side.
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmap.java:
##########
@@ -371,6 +416,40 @@ int writeTo(ByteBuffer buffer, int offset) {
}
}
+ private int merge(Container other) {
+ int initialCardinality = cardinality;
+
+ if (other.dense != null) {
+ // other may be sparse and have a dense array
+ if (dense != null) {
+ // OR the dense bytes together
+ for (int i = 0; i < dense.length; i += 1) {
+ this.dense[i] |= other.dense[i];
+ }
+ } else {
+ // Copy the dense bytes and set the positions from this container
+ this.dense = Arrays.copyOf(other.dense, other.dense.length);
+ for (int i = 0; i < cardinality; i += 1) {
+ denseSet(dense, (sparse[i] & 0xFF));
+ }
+ }
+
+ this.cardinality = denseCardinality(dense);
+
+ // copy the sparse array if needed; other was sparse and merge set no
new bits
Review Comment:
This `if` condition is a bit tricky to understand. More details comment can
help. Also if we adopt the `else` block comment below, we should move this
comment inside the `if` block.
```
// other.dense on a sparse container means cardinality ==
MAX_SPARSE_CARDINALITY.
// Merged cardinality still below DENSE_CONTAINER_SIZE means this
added no bits,
// so other.sparse is the merged sparse array.
```
##########
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:
Use the argument's overlay when it already exists, which can save one copy.
Otherwise build a temporary container from `other.base`, as
`merge(MumblingBitmap)` does.
```suggestion
Container source = other.overlays.get(containerIndex);
if (source == null) {
source = Container.newOverlay(other.base, containerIndex);
}
this.cardinality += overlay.merge(source);
```
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmap.java:
##########
@@ -371,6 +416,40 @@ int writeTo(ByteBuffer buffer, int offset) {
}
}
+ private int merge(Container other) {
+ int initialCardinality = cardinality;
+
+ if (other.dense != null) {
+ // other may be sparse and have a dense array
+ if (dense != null) {
+ // OR the dense bytes together
+ for (int i = 0; i < dense.length; i += 1) {
+ this.dense[i] |= other.dense[i];
+ }
+ } else {
+ // Copy the dense bytes and set the positions from this container
+ this.dense = Arrays.copyOf(other.dense, other.dense.length);
+ for (int i = 0; i < cardinality; i += 1) {
+ denseSet(dense, (sparse[i] & 0xFF));
+ }
+ }
+
+ this.cardinality = denseCardinality(dense);
+
+ // copy the sparse array if needed; other was sparse and merge set no
new bits
+ if (cardinality < DENSE_CONTAINER_SIZE) {
+ this.sparse = Arrays.copyOf(other.sparse, other.sparse.length);
+ }
+ } else {
+ // Set the positions from the other container in this container
+ for (int i = 0; i < other.cardinality; i += 1) {
+ set(other.sparse[i] & 0xFF);
+ }
+ }
+
+ return cardinality - initialCardinality;
Review Comment:
nit: This returns the cardinality increase. Callers add it to the bitmap
cardinality. Would be good to add Javadoc on the return value.
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmap.java:
##########
@@ -371,6 +416,40 @@
}
}
+ private int merge(Container other) {
+ int initialCardinality = cardinality;
+
+ if (other.dense != null) {
+ // other may be sparse and have a dense array
+ if (dense != null) {
+ // OR the dense bytes together
+ for (int i = 0; i < dense.length; i += 1) {
+ this.dense[i] |= other.dense[i];
+ }
+ } else {
+ // Copy the dense bytes and set the positions from this container
+ this.dense = Arrays.copyOf(other.dense, other.dense.length);
+ for (int i = 0; i < cardinality; i += 1) {
+ denseSet(dense, (sparse[i] & 0xFF));
+ }
+ }
+
+ this.cardinality = denseCardinality(dense);
+
+ // copy the sparse array if needed; other was sparse and merge set no
new bits
+ if (cardinality < DENSE_CONTAINER_SIZE) {
+ this.sparse = Arrays.copyOf(other.sparse, other.sparse.length);
Review Comment:
Once the union is dense, clear `sparse`. Leaving the old array means a dense
merged container can hold sparse bytes that are not its set positions. maybe
also add a test coverage for this scenario.
```suggestion
this.sparse = Arrays.copyOf(other.sparse, other.sparse.length);
} else {
this.sparse = null;
```
--
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]
