rdblue commented on code in PR #18704:
URL: https://github.com/apache/iceberg/pull/18704#discussion_r4428847953
##########
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:
Maybe it's me, but I don't think that is much more clear. It doesn't state
that `other` was sparse, only when `other.dense` may be present for a sparse
array. Does this need to state how other could be sparse but still have a dense
array? That is stated above, "other may be sparse and have a dense array", so
it seems to be covered.
--
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]
