You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anoopj (via GitHub)" <gi...@apache.org> on 2026/09/16 15:21:44 UTC
[PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
anoopj opened a new pull request, #18138:
URL: https://github.com/apache/iceberg/pull/18138
Replace `ManifestInfo`'s raw `dv()/dvCardinality()` pair with a single `manifestDeletionVector()` returning `ManifestBitmap`, backed by the read-only MumblingBitmap implementation. The dv_cardinality field (523) is dropped since cardinality is carried in the Mumbling bitmap header, and the adapter now delegates instead of wrapping the bytes in a stub that throws on cardinality()/isSet().
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031060008
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Test fixtures for Mumbling bitmaps. */
+public class MumblingBitmapTestUtil {
+ private MumblingBitmapTestUtil() {}
+
+ /** Returns the serialized bytes of an empty Mumbling bitmap (version 1, cardinality 0). */
+ public static byte[] emptyBitmapBytes() {
+ return new byte[] {0x01, 0x00, 0x00, 0x00, 0x00, 0x00};
+ }
+
+ /** Returns a read-only empty Mumbling bitmap (cardinality 0). */
Review Comment:
Fixed. Only retained the essential ones.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030968993
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmaps.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Factory for reading Mumbling bitmaps. */
Review Comment:
True. I deleted the comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030224082
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -31,7 +31,8 @@ class TestManifestInfoStruct {
@Test
void fieldAccess() {
ManifestInfoStruct info =
- new ManifestInfoStruct(10, 20, 3, 2, 1000L, 2000L, 300L, 200L, 5L, new byte[] {0xF}, 1L);
+ new ManifestInfoStruct(
+ 10, 20, 3, 2, 1000L, 2000L, 300L, 200L, 5L, MumblingBitmapTestUtil.emptyBitmapBytes());
Review Comment:
I think this test case is probably invalid. If a bitmap's cardinality is 0, we should always write `null` instead of writing a bitmap. For tests like this, we should use valid values.
This is a good case to note for the write wrapper.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031075812
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -31,7 +31,8 @@ class TestManifestInfoStruct {
@Test
void fieldAccess() {
ManifestInfoStruct info =
- new ManifestInfoStruct(10, 20, 3, 2, 1000L, 2000L, 300L, 200L, 5L, new byte[] {0xF}, 1L);
+ new ManifestInfoStruct(
+ 10, 20, 3, 2, 1000L, 2000L, 300L, 200L, 5L, MumblingBitmapTestUtil.emptyBitmapBytes());
Review Comment:
I chose this because the empty Mumbling bitmap bytes was the easiest to construct, and we can't use a mock here because the class contract is bytes. I have moved to a bitmap with just the first bit set, borrowing some code from the mumbling tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030210290
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Test fixtures for Mumbling bitmaps. */
Review Comment:
Unnecessary.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031838360
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -522,7 +520,9 @@ void manifestFileAdapterDelegation(FileContent contentType) {
assertThat(manifest.deletedRowsCount()).isEqualTo(MANIFEST_INFO.deletedRowsCount());
assertThat(manifest.firstRowId()).isEqualTo(FIRST_ROW_ID);
assertThat(manifest.keyMetadata()).isEqualTo(MANIFEST_KEY_METADATA);
- assertThat(manifest.manifestDeletionVector().buffer()).isEqualTo(ByteBuffer.wrap(MANIFEST_DV));
+ assertThat(manifest.manifestDeletionVector().buffer())
+ .isEqualTo(ByteBuffer.wrap(MumblingBitmapTestUtil.bitmapBytes()));
+ assertThat(manifest.manifestDeletionVector().cardinality()).isEqualTo(1);
Review Comment:
It is not needed. Removed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030203605
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -331,16 +322,9 @@ Builder minSequenceNumber(long sequenceNumber) {
return this;
}
- Builder dv(ByteBuffer buffer) {
- Preconditions.checkArgument(buffer != null, "Invalid DV: null");
- this.dv = ByteBuffers.toByteArray(buffer);
- return this;
- }
-
- Builder dvCardinality(long cardinality) {
- Preconditions.checkArgument(
- cardinality >= 0, "Invalid DV cardinality: %s (must be >= 0)", cardinality);
- this.dvCardinality = cardinality;
+ Builder manifestDeletionVector(ManifestBitmap bitmap) {
Review Comment:
I'm not confident that `ManifestBitmap` is going to be used to set the DV. I'd probably leave this unchanged for now, until we know more about how bitmaps are passed after being constructed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031022058
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmaps.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Factory for reading Mumbling bitmaps. */
+public class MumblingBitmaps {
+ private MumblingBitmaps() {}
+
+ /** Returns a read-only view of the Mumbling bitmap serialized in the buffer. */
Review Comment:
Agree. Reworded to just say it returns a `ManifestBitmap`. Also documented the params
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031435565
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+import org.apache.iceberg.util.ByteBuffers;
+
+public class MumblingBitmapTestUtil {
+ private MumblingBitmapTestUtil() {}
+
+ /** Returns the serialized bytes of a Mumbling bitmap with bit 0 set. */
+ public static byte[] bitmapBytes() {
Review Comment:
I think it should be clear from the helper method name that this is a specific bitmap, not just _any_ bitmap. That could be `onlyFirstBitSet` or just `exampleBitmap`, but I think just calling it `bitmap` is going to make uses of it harder to read.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031458835
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+import org.apache.iceberg.util.ByteBuffers;
+
+public class MumblingBitmapTestUtil {
Review Comment:
Nit: I almost always opt to use the shorter name, like `MumblingTestUtil`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031847286
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+import org.apache.iceberg.util.ByteBuffers;
+
+public class MumblingBitmapTestUtil {
+ private MumblingBitmapTestUtil() {}
+
+ /** Returns the serialized bytes of a Mumbling bitmap with bit 0 set. */
+ public static byte[] bitmapBytes() {
Review Comment:
Agree. Renamed to `onlyFirstBitSetBytes()`. I dropped the generic `bitmap()` accessor since it's not needed anymore
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031508478
##########
core/src/test/java/org/apache/iceberg/V4TestComparators.java:
##########
@@ -119,8 +119,7 @@ private static <T extends Comparable<T>> Comparator<T> natural() {
.thenComparingLong(ManifestInfo::deletedRowsCount)
.thenComparingLong(ManifestInfo::replacedRowsCount)
.thenComparingLong(ManifestInfo::minSequenceNumber)
- .thenComparing(ManifestInfo::dv, BYTES)
- .thenComparing(ManifestInfo::dvCardinality, natural()));
+ .thenComparing(V4TestComparators::manifestDeletionVectorBuffer, BYTES));
Review Comment:
I don't think that this needs to add a custom method. The `Comparator` API can handle this, and then we have a comparator that can be reused for `Tracking` later:
```java
private static final Comparator<ManifestBitmap> BITMAPS =
Comparator.nullsFirst(Comparator.comparing(ManifestBitmap::buffer, BYTES));
...
.thenComparing(ManifestInfo::manifestDeletionVector, BITMAPS));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030146082
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
This is a good thing to think through. Thanks for pointing it out.
I don't think that we want to keep cardinality because it is easy to recover from each bitmap when you're working with a manifest, by calling `manifest.manifestDeletionVector().cardinality()` rather than `manifest.cardinality()` directly (assuming that we're working with `ManfiestFile`). That just reads the first 6 bytes, if a DV is present. Otherwise the DV is lazy.
I think the trade-off is that you have to actually read the DV bytes instead of reading the cardinality and row count when scanning the root manifest. But if you did identify a leaf manifest that needed to be compacted, you'd just go and re-scan the root manifest to recover the DV so you can rewrite the file. So I think the case where you'd scan the root twice isn't very realistic.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030964594
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
Thanks for raising this, @amogh-jahagirdar
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031511022
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -522,7 +520,9 @@ void manifestFileAdapterDelegation(FileContent contentType) {
assertThat(manifest.deletedRowsCount()).isEqualTo(MANIFEST_INFO.deletedRowsCount());
assertThat(manifest.firstRowId()).isEqualTo(FIRST_ROW_ID);
assertThat(manifest.keyMetadata()).isEqualTo(MANIFEST_KEY_METADATA);
- assertThat(manifest.manifestDeletionVector().buffer()).isEqualTo(ByteBuffer.wrap(MANIFEST_DV));
+ assertThat(manifest.manifestDeletionVector().buffer())
+ .isEqualTo(ByteBuffer.wrap(MumblingBitmapTestUtil.bitmapBytes()));
+ assertThat(manifest.manifestDeletionVector().cardinality()).isEqualTo(1);
Review Comment:
Why is cardinality being tested?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #18138:
URL: https://github.com/apache/iceberg/pull/18138
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#issuecomment-5719061023
Merged! Thanks @anoopj!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030213977
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Test fixtures for Mumbling bitmaps. */
+public class MumblingBitmapTestUtil {
+ private MumblingBitmapTestUtil() {}
+
+ /** Returns the serialized bytes of an empty Mumbling bitmap (version 1, cardinality 0). */
+ public static byte[] emptyBitmapBytes() {
+ return new byte[] {0x01, 0x00, 0x00, 0x00, 0x00, 0x00};
+ }
+
+ /** Returns a read-only empty Mumbling bitmap (cardinality 0). */
Review Comment:
Please edit and ensure that all Javadoc is helpful and accurate. If you think it might be unnecessary, remove it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031038727
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -53,7 +53,8 @@ class ManifestInfoStruct extends SupportsIndexProjection implements ManifestInfo
private long replacedRowsCount = -1L;
private long minSequenceNumber = -1L;
private byte[] dv = null;
- private Long dvCardinality = null;
+
+ private transient ManifestBitmap manifestDeletionVector = null;
Review Comment:
Reverted.
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -331,16 +322,9 @@ Builder minSequenceNumber(long sequenceNumber) {
return this;
}
- Builder dv(ByteBuffer buffer) {
- Preconditions.checkArgument(buffer != null, "Invalid DV: null");
- this.dv = ByteBuffers.toByteArray(buffer);
- return this;
- }
-
- Builder dvCardinality(long cardinality) {
- Preconditions.checkArgument(
- cardinality >= 0, "Invalid DV cardinality: %s (must be >= 0)", cardinality);
- this.dvCardinality = cardinality;
+ Builder manifestDeletionVector(ManifestBitmap bitmap) {
Review Comment:
Reverted the change.
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Test fixtures for Mumbling bitmaps. */
Review Comment:
Removed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031082633
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -42,8 +43,20 @@ void fieldAccess() {
assertThat(info.deletedRowsCount()).isEqualTo(300L);
assertThat(info.replacedRowsCount()).isEqualTo(200L);
assertThat(info.minSequenceNumber()).isEqualTo(5L);
- assertThat(info.dv()).isNotNull();
- assertThat(info.dvCardinality()).isEqualTo(1L);
+ assertThat(info.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
+ assertThat(info.manifestDeletionVector().cardinality()).isEqualTo(0);
+ }
+
+ @Test
+ void manifestDeletionVectorIsReused() {
Review Comment:
It was mainly to catch perf regressions. I have removed it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031038727
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -53,7 +53,8 @@ class ManifestInfoStruct extends SupportsIndexProjection implements ManifestInfo
private long replacedRowsCount = -1L;
private long minSequenceNumber = -1L;
private byte[] dv = null;
- private Long dvCardinality = null;
+
+ private transient ManifestBitmap manifestDeletionVector = null;
Review Comment:
Reverted.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030228967
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -42,8 +43,20 @@ void fieldAccess() {
assertThat(info.deletedRowsCount()).isEqualTo(300L);
assertThat(info.replacedRowsCount()).isEqualTo(200L);
assertThat(info.minSequenceNumber()).isEqualTo(5L);
- assertThat(info.dv()).isNotNull();
- assertThat(info.dvCardinality()).isEqualTo(1L);
+ assertThat(info.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
+ assertThat(info.manifestDeletionVector().cardinality()).isEqualTo(0);
+ }
+
+ @Test
+ void manifestDeletionVectorIsReused() {
Review Comment:
What is the value of testing this?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030169946
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmaps.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Factory for reading Mumbling bitmaps. */
+public class MumblingBitmaps {
+ private MumblingBitmaps() {}
+
+ /** Returns a read-only view of the Mumbling bitmap serialized in the buffer. */
Review Comment:
Is the contract of this method to return an immutable or read-only bitmap? I think the only requirement is that this returns a `ManifestBitmap`. Relying on that is the most clear way to document the behavior because we won't come back here and update this if we change `ManifestBitmap`.
Also, Javadoc should document the arguments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030224082
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -31,7 +31,8 @@ class TestManifestInfoStruct {
@Test
void fieldAccess() {
ManifestInfoStruct info =
- new ManifestInfoStruct(10, 20, 3, 2, 1000L, 2000L, 300L, 200L, 5L, new byte[] {0xF}, 1L);
+ new ManifestInfoStruct(
+ 10, 20, 3, 2, 1000L, 2000L, 300L, 200L, 5L, MumblingBitmapTestUtil.emptyBitmapBytes());
Review Comment:
I think this test case is probably invalid. If a bitmap's cardinality is 0, we should always write `null` instead of writing a bitmap. For tests like this, we should use valid values.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031096074
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -149,8 +159,8 @@ void internalSetIgnoresUnknownOrdinal() {
assertThat(info.deletedRowsCount()).isEqualTo(300L);
assertThat(info.replacedRowsCount()).isEqualTo(200L);
assertThat(info.minSequenceNumber()).isEqualTo(5L);
- assertThat(info.dv()).isEqualTo(ByteBuffer.wrap(new byte[] {0xF}));
- assertThat(info.dvCardinality()).isEqualTo(1L);
+ assertThat(info.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
Review Comment:
Did it originally this way because the mumbling implementation doesn't have `equals()`. Addressed it because identity is all we need.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031853705
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+import org.apache.iceberg.util.ByteBuffers;
+
+public class MumblingBitmapTestUtil {
+ private MumblingBitmapTestUtil() {}
+
+ /** Returns the serialized bytes of a Mumbling bitmap with bit 0 set. */
+ public static byte[] bitmapBytes() {
+ byte[] container = {0};
+ int[] descriptors = {container.length};
+
+ ByteBuffer buffer =
+ ByteBuffer.allocate(6 + PFOREncoding.estimateEncodedSize(1) + container.length);
+ buffer.put(0, (byte) 1);
+ buffer.put(1, (byte) 1);
+ buffer.put(2, (byte) 0);
+ buffer.put(3, (byte) 0);
+ buffer.put(4, (byte) 1);
+ buffer.put(5, (byte) 0);
+ int descriptorSize = PFOREncoding.encode(descriptors, 0, buffer, 6, 1);
+ buffer.put(6 + descriptorSize, container);
+ buffer.limit(6 + descriptorSize + container.length);
+
+ return ByteBuffers.toByteArray(buffer);
Review Comment:
Thanks for the suggestion. I moved the code from `TestMumblingBitmap` into this util, and reused them. I think it looks a lot cleaner.
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+import org.apache.iceberg.util.ByteBuffers;
+
+public class MumblingBitmapTestUtil {
Review Comment:
Renamed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031866776
##########
core/src/test/java/org/apache/iceberg/V4TestComparators.java:
##########
@@ -119,8 +119,7 @@ private static <T extends Comparable<T>> Comparator<T> natural() {
.thenComparingLong(ManifestInfo::deletedRowsCount)
.thenComparingLong(ManifestInfo::replacedRowsCount)
.thenComparingLong(ManifestInfo::minSequenceNumber)
- .thenComparing(ManifestInfo::dv, BYTES)
- .thenComparing(ManifestInfo::dvCardinality, natural()));
+ .thenComparing(V4TestComparators::manifestDeletionVectorBuffer, BYTES));
Review Comment:
Thanks for the suggestion. Made exactly this change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4029509134
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
Is this something we want to drop, cc @rdblue ? One consequence of this would be that we wouldn't be able to as easily tell just from manifest metadata which leaf manifests should be compacted/rewritten (e.g. dv_cardinality/number_entries gets you a ratio of how much has been deleted).
Considering we have to read the root and all the inline bitmaps in general, maybe not a huge deal to always get it from the bitmap itself but we're possibly missing a more selective projection to be able to determine if a leaf should be rewritten.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030313390
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
I think I agree with that argument. Actually performing the rewrite requires reading the bitmap anyways, so why do a 2 pass over the root where the first one has a selective projection; doesn't really buy us much unless checking for rewrites happens way more frequently or something. I'll update the spec PR in that case. Thanks @rdblue !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030293551
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -574,7 +538,7 @@ void kryoSerializationRoundTrip() throws IOException {
assertThat(deserialized.deletedRowsCount()).isEqualTo(300L);
assertThat(deserialized.replacedRowsCount()).isEqualTo(200L);
assertThat(deserialized.minSequenceNumber()).isEqualTo(5L);
- assertThat(deserialized.dv()).isEqualTo(ByteBuffer.wrap(new byte[] {0xF}));
- assertThat(deserialized.dvCardinality()).isEqualTo(1L);
+ assertThat(deserialized.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
Review Comment:
Shouldn't this validate using the bytes rather than going through `emptyBitmap`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030313390
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
I think I agree with that argument. Actually performing the rewrite requires reading the bitmap anyways, so why do a 2 pass over the root. I'll update the spec PR in that case. Thanks @rdblue !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031456366
##########
core/src/test/java/org/apache/iceberg/mumbling/MumblingBitmapTestUtil.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+import org.apache.iceberg.util.ByteBuffers;
+
+public class MumblingBitmapTestUtil {
+ private MumblingBitmapTestUtil() {}
+
+ /** Returns the serialized bytes of a Mumbling bitmap with bit 0 set. */
+ public static byte[] bitmapBytes() {
+ byte[] container = {0};
+ int[] descriptors = {container.length};
+
+ ByteBuffer buffer =
+ ByteBuffer.allocate(6 + PFOREncoding.estimateEncodedSize(1) + container.length);
+ buffer.put(0, (byte) 1);
+ buffer.put(1, (byte) 1);
+ buffer.put(2, (byte) 0);
+ buffer.put(3, (byte) 0);
+ buffer.put(4, (byte) 1);
+ buffer.put(5, (byte) 0);
+ int descriptorSize = PFOREncoding.encode(descriptors, 0, buffer, 6, 1);
+ buffer.put(6 + descriptorSize, container);
+ buffer.limit(6 + descriptorSize + container.length);
+
+ return ByteBuffers.toByteArray(buffer);
Review Comment:
I'd expect this to be either a fixed byte array or a more generic implementation, but it appears to mix specific bytes with encoding calls.
Also, there are already utils available (located in `TestMumblingBitmap`) that would make this cleaner:
```java
return build(sparse(1));
. . .
return bitmap(onlyFirstBitSetBuffer());
```
I think it makes sense to move those helper methods to this new util class.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4028441511
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -146,13 +142,8 @@ public long minSequenceNumber() {
}
@Override
- public ByteBuffer dv() {
- return dv != null ? ByteBuffer.wrap(dv) : null;
- }
-
- @Override
- public Long dvCardinality() {
- return dvCardinality;
+ public ManifestBitmap manifestDeletionVector() {
+ return dv != null ? MumblingBitmaps.read(ByteBuffer.wrap(dv)) : null;
Review Comment:
Good point. I added memoization.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4027721145
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmaps.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Factory for reading Mumbling bitmaps. */
+public class MumblingBitmaps {
Review Comment:
Added this factory so the package-private `MumblingBitmap` can be constructed from outside the `mumbling` package. If needed we can widen access and get rid of this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4028136097
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -146,13 +142,8 @@ public long minSequenceNumber() {
}
@Override
- public ByteBuffer dv() {
- return dv != null ? ByteBuffer.wrap(dv) : null;
- }
-
- @Override
- public Long dvCardinality() {
- return dvCardinality;
+ public ManifestBitmap manifestDeletionVector() {
+ return dv != null ? MumblingBitmaps.read(ByteBuffer.wrap(dv)) : null;
Review Comment:
I think it's good that this is created lazily, but I'd like to reuse the result rather than creating more than one because the implementation will deserialize the offset and descriptor arrays.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4029509134
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
Is this something we want to drop, cc @rdblue ? One consequence of this would be that we wouldn't be able to as easily tell just from manifest metadata which leaf manifests should be compacted/rewritten (e.g. dv_cardinality/number_entries gets you a ratio of how much has been deleted).
Considering we have to read the root and all the inline bitmaps in general, maybe not a huge deal but we're possibly missing a more selective projection to be able to determine if a leaf should be rewritten.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030152468
##########
core/src/main/java/org/apache/iceberg/mumbling/MumblingBitmaps.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.mumbling;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.ManifestBitmap;
+
+/** Factory for reading Mumbling bitmaps. */
Review Comment:
This is a utility class with a factory method, not a factory.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030178282
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -53,7 +53,8 @@ class ManifestInfoStruct extends SupportsIndexProjection implements ManifestInfo
private long replacedRowsCount = -1L;
private long minSequenceNumber = -1L;
private byte[] dv = null;
- private Long dvCardinality = null;
+
+ private transient ManifestBitmap manifestDeletionVector = null;
Review Comment:
How about `mdv`? This name is very long.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030228967
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -42,8 +43,20 @@ void fieldAccess() {
assertThat(info.deletedRowsCount()).isEqualTo(300L);
assertThat(info.replacedRowsCount()).isEqualTo(200L);
assertThat(info.minSequenceNumber()).isEqualTo(5L);
- assertThat(info.dv()).isNotNull();
- assertThat(info.dvCardinality()).isEqualTo(1L);
+ assertThat(info.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
+ assertThat(info.manifestDeletionVector().cardinality()).isEqualTo(0);
+ }
+
+ @Test
+ void manifestDeletionVectorIsReused() {
Review Comment:
What is the value of testing this? Is this a contract that `ManifestInfo` *must* follow? If not, then I think it's just extra code to review and maintain that could break.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030313390
##########
core/src/main/java/org/apache/iceberg/ManifestInfo.java:
##########
@@ -56,12 +55,6 @@ interface ManifestInfo {
Types.NestedField DV =
Types.NestedField.optional(
522, "dv", Types.BinaryType.get(), "Deletion vector for manifest entries");
- Types.NestedField DV_CARDINALITY =
Review Comment:
I think I agree with that argument. Actually performing the rewrite requires reading the bitmap anyways, so why do a 2 pass over the root where the first one has a selective projection; doesn't really buy us much unless checking for rewrites happens way more frequently, but I don't think that matters much either. I'll update the spec PR in that case. Thanks @rdblue !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4030287757
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -149,8 +159,8 @@ void internalSetIgnoresUnknownOrdinal() {
assertThat(info.deletedRowsCount()).isEqualTo(300L);
assertThat(info.replacedRowsCount()).isEqualTo(200L);
assertThat(info.minSequenceNumber()).isEqualTo(5L);
- assertThat(info.dv()).isEqualTo(ByteBuffer.wrap(new byte[] {0xF}));
- assertThat(info.dvCardinality()).isEqualTo(1L);
+ assertThat(info.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
Review Comment:
This should be `assertThat(info.manifestDeletionVector()).isEqualTo(MumblingBitmapTestUtil.emptyBitmap())`.
This isn't checking that the values are equivalent. It is checking that the value is unchanged.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031030867
##########
core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java:
##########
@@ -53,7 +53,8 @@ class ManifestInfoStruct extends SupportsIndexProjection implements ManifestInfo
private long replacedRowsCount = -1L;
private long minSequenceNumber = -1L;
private byte[] dv = null;
- private Long dvCardinality = null;
+
+ private transient ManifestBitmap manifestDeletionVector = null;
Review Comment:
Renamed to `mdv`. I chose the original name to be consistent with `Manifestfile.manifestDeletionVector` which is also long. I can send a quick followup rename that one if you prefer.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#discussion_r4031111243
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -574,7 +538,7 @@ void kryoSerializationRoundTrip() throws IOException {
assertThat(deserialized.deletedRowsCount()).isEqualTo(300L);
assertThat(deserialized.replacedRowsCount()).isEqualTo(200L);
assertThat(deserialized.minSequenceNumber()).isEqualTo(5L);
- assertThat(deserialized.dv()).isEqualTo(ByteBuffer.wrap(new byte[] {0xF}));
- assertThat(deserialized.dvCardinality()).isEqualTo(1L);
+ assertThat(deserialized.manifestDeletionVector().buffer())
+ .isEqualTo(MumblingBitmapTestUtil.emptyBitmap().buffer());
Review Comment:
You're right. Now compares against bytes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Use ManifestBitmap for manifest deletion vector in ManifestInfo [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on PR #18138:
URL: https://github.com/apache/iceberg/pull/18138#issuecomment-5706363989
> Mostly looks good, but I think this should use shared bitmap test helpers and there are some test cases to update.
@rdblue Thanks for the thorough review. I have incorporated them all.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org