rdblue commented on code in PR #18704:
URL: https://github.com/apache/iceberg/pull/18704#discussion_r4428808435
##########
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:
I agree here and I'll add this.
Tests are independent of the internal state of the object. Cases that will
take this path are tested already and this won't change behavior. The benefit
of doing this is that it maintains an invariant in `Container`, which is that
the `sparse` array is non-null only when the container is sparse. (The dense
array may also exist when the container is sparse.)
--
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]
