You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2026/05/21 05:03:18 UTC
[PR] Core, Data: Validate deletion-vector offset and length [iceberg]
wombatu-kun opened a new pull request, #16500:
URL: https://github.com/apache/iceberg/pull/16500
Closes #16475
Hostile or corrupted manifest metadata can claim a negative or near-2 GiB `content_offset` / `content_size_in_bytes` on a deletion-vector `DeleteFile`. The existing precondition in `BaseDeleteLoader.validateDV` only checks non-null and `size <= Integer.MAX_VALUE`, so a negative size slips through to `new byte[length]` and throws `NegativeArraySizeException`; a negative offset reaches `IOUtil.readFully` and produces an invalid seek. The same unguarded allocation pattern lives in `DVUtil.readDV` on the rewrite / DV-merge path used by `mergeAndWriteDVsIfRequired`.
### Changes
- Add `ContentFileUtil.validateDV` as the canonical read-path validator (null offset, null size, `offset >= 0`, `size >= 0`, `size <= Integer.MAX_VALUE`).
- Have both `BaseDeleteLoader.validateDV` (data) and `DVUtil.readDV` (core) delegate to it. `BaseDeleteLoader` keeps its loader-specific `referencedDataFile` check.
- For defense-in-depth, also reject negative `contentOffset` / `contentSizeInBytes` in `FileMetadata.Builder.build()` for `PUFFIN`, mirroring the existing `fileSizeInBytes >= 0` / `recordCount >= 0` checks at the same site. Producers using the builder can no longer accidentally emit bad metadata; the read-side check still defends against corrupted on-disk manifests where `BaseFile.internalSet` hydrates fields directly via Avro reflection.
### Tests
- `TestContentFileUtil` (new) — covers each precondition of the new validator (null offset/size, negative offset/size, oversized size, valid zero / typical / `Integer.MAX_VALUE` values).
- `TestFileMetadata` (new) — verifies the builder rejects negative DV offset/size and still accepts valid values.
- `TestBaseDeleteLoader` (new, in package `org.apache.iceberg` so it can reach the package-private `Delegates.DelegatingDeleteFile`) — constructs a tampered `DeleteFile` that bypasses the builder and asserts `BaseDeleteLoader.loadPositionDeletes` throws before any I/O is attempted.
- Existing `TestRowDelta` (324 tests), `TestDeletionVectorStruct`, `TestContentFileParser`, `TestDeleteFiles` regression suites continue to pass.
### Out of scope
`PuffinReader` / `BlobMetadata` similarly don't range-check blob offsets and lengths today, but Puffin is also used for non-DV blobs (statistics) and "valid" at that layer is a separate discussion of footer-authoritative vs. caller-supplied bounds. Leaving Puffin-level hardening as a follow-up.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-5066608859
@rdblue friendly ping. All your review comments have been addressed (replies inline, plus commits 30256ab67 and 00d778384), and @steveloughran has approved. Could you take another look when you have a moment? Thanks!
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-5610710425
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313204032
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
Review Comment:
I think this is too clever and it's odd that the same PR has multiple ways to inject a delete with an invalid offset or size. I think this should pick one strategy. I think it probably makes more sense to use a mock, but I don't have a strong opinion.
Also, it would be reasonable (and straightforward) to just use the `DeleteFile` constructor and bypass the builder rather than doing things like this. The actual class allows these values because we don't validate and fail while filling the objects with data.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3338249294
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
Review Comment:
Left the five checks inline - each carries a distinct message (offset vs length, null vs negative vs 2GB) and they run through the public validateDV, so factoring wouldn't reduce the test surface. Happy to revisit if others prefer the split.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-5070203418
I'd forgotten about this; once it is in the range check code of #17253 can go, leaving on the roaring bitmap parser logic.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3338241044
##########
core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java:
##########
@@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) {
deleteFile.referencedDataFile());
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
+ dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() <= Integer.MAX_VALUE,
Review Comment:
Opened #16653 to add the no-emdash / ASCII-only rule to AGENTS.md.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315061377
##########
core/src/main/java/org/apache/iceberg/FileMetadata.java:
##########
@@ -263,6 +263,12 @@ public DeleteFile build() {
if (format == FileFormat.PUFFIN) {
Preconditions.checkArgument(contentOffset != null, "Content offset is required for DV");
Preconditions.checkArgument(contentSizeInBytes != null, "Content size is required for DV");
+ Preconditions.checkArgument(
+ contentOffset >= 0, "Content offset must be non-negative for DV: %s", contentOffset);
+ Preconditions.checkArgument(
+ contentSizeInBytes >= 0,
Review Comment:
Done in 30256ab67.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub) via issues" <is...@iceberg.apache.org>.
wombatu-kun commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-6542869336
@amogh-jahagirdar you wrote the DV merge path in `DVUtil` (#15006) - could you take a look at this one? `readDV` and `BaseDeleteLoader` size their read from `content_size_in_bytes` without rejecting negatives, so a corrupt manifest fails with `NegativeArraySizeException` or an invalid seek instead of a clear error. The earlier review comments are addressed, steveloughran approved it, and it lets #17253 drop its own range check.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3336611200
##########
core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java:
##########
@@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) {
deleteFile.referencedDataFile());
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
+ dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() <= Integer.MAX_VALUE,
Review Comment:
FWIW, probably something to add in the AGENTS.md code, no emdash, no non-ascii unicode except for UTF-related experiments. separate PR.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315063058
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
Review Comment:
Done in 30256ab67 (Mockito mock).
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3338242544
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
Review Comment:
Done 00d778384.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315059596
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -52,6 +52,7 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
ContentFileUtil.isDV(deleteFile),
"Cannot read, not a deletion vector: %s",
deleteFile.location());
+ ContentFileUtil.validateDV(deleteFile);
Review Comment:
Done in 30256ab67. DVUtil is now public so BaseDeleteLoader (data module) can call 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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-5633107054
I should write some destaler client to update my prs tagged as stale; merge and push...
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315057274
##########
core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java:
##########
@@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) {
deleteFile.referencedDataFile());
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
+ dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() <= Integer.MAX_VALUE,
Review Comment:
Fixed in eacedd538.
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -52,6 +52,7 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
ContentFileUtil.isDV(deleteFile),
"Cannot read, not a deletion vector: %s",
deleteFile.location());
+ ContentFileUtil.validateDV(deleteFile);
Review Comment:
Done in eacedd538. DVUtil is now public so BaseDeleteLoader (data module) can call it.
##########
core/src/main/java/org/apache/iceberg/FileMetadata.java:
##########
@@ -263,6 +263,12 @@ public DeleteFile build() {
if (format == FileFormat.PUFFIN) {
Preconditions.checkArgument(contentOffset != null, "Content offset is required for DV");
Preconditions.checkArgument(contentSizeInBytes != null, "Content size is required for DV");
+ Preconditions.checkArgument(
+ contentOffset >= 0, "Content offset must be non-negative for DV: %s", contentOffset);
+ Preconditions.checkArgument(
+ contentSizeInBytes >= 0,
Review Comment:
Done in eacedd538.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3645790714
##########
core/src/test/java/org/apache/iceberg/TestDVUtil.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.junit.jupiter.api.Test;
+
+public class TestDVUtil {
+
+ @Test
+ public void validateDVRejectsNullOffset() {
+ DeleteFile dv = dv(null, 10L);
Review Comment:
Done 58ac9d602. Took the parameterized variant for both the reject and accept cases, and applied the same shape to TestBaseDeleteLoader.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315057274
##########
core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java:
##########
@@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) {
deleteFile.referencedDataFile());
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
+ dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() <= Integer.MAX_VALUE,
Review Comment:
Fixed in 30256ab67.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-4893899074
Hi @rdblue, gentle nudge on this one. All five of your comments from May 27 were addressed the next day in `30256ab67`, and each thread has a reply pointing at the fix:
- DV validation moved out of `ContentFileUtil` into a public `DVUtil.validateDV`, with the strict `< Integer.MAX_VALUE` bound you flagged
- `FileMetadata.Builder.build()` now also rejects `contentSizeInBytes >= Integer.MAX_VALUE`
- `TestBaseDeleteLoader` switched to a single Mockito-mock strategy
- Em dashes removed across the diff
@steveloughran's later nits were handled in `00d778384`, and he approved earlier today. Would you mind taking another pass when you have a moment? Thanks!
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3338251006
##########
core/src/test/java/org/apache/iceberg/TestFileMetadata.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.Test;
+
+public class TestFileMetadata {
+
+ @Test
+ public void dvBuilderRejectsNegativeContentOffset() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(-1L)
+ .withContentSizeInBytes(10L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content offset must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsNegativeContentSize() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(-1L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content size must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsContentSizeAtIntegerMax() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(Integer.MAX_VALUE)
+ .build())
Review Comment:
Done 00d778384. Added a validDvBuilder() returning a valid builder; each test now overrides only the single invalid field.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3645570793
##########
core/src/test/java/org/apache/iceberg/TestDVUtil.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.junit.jupiter.api.Test;
+
+public class TestDVUtil {
+
+ @Test
+ public void validateDVRejectsNullOffset() {
+ DeleteFile dv = dv(null, 10L);
Review Comment:
most of these tests can be refactored into an assertRejected(Long, Long, String) method, which is then invoked for all the values.
There's one step more advanced, which is to parameterize, though the way junit5 parameterizes it's more complicated to implement a method supplying a list of argument triples of (Long, Long, String) than it is to have the 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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315063058
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
Review Comment:
Done in eacedd538 (Mockito mock).
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
+ @Override
+ public Long contentOffset() {
+ return offset;
+ }
+
+ @Override
+ public Long contentSizeInBytes() {
+ return size;
+ }
+ };
+ }
+
+ // The validator must fire before any I/O attempt — if it does not, the loader will call this
Review Comment:
Done in eacedd538. Removed across the whole branch diff.
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
Review Comment:
Done 1a0356baf.
##########
core/src/test/java/org/apache/iceberg/TestFileMetadata.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.Test;
+
+public class TestFileMetadata {
+
+ @Test
+ public void dvBuilderRejectsNegativeContentOffset() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(-1L)
+ .withContentSizeInBytes(10L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content offset must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsNegativeContentSize() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(-1L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content size must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsContentSizeAtIntegerMax() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(Integer.MAX_VALUE)
+ .build())
Review Comment:
Done 1a0356baf. Added a validDvBuilder() returning a valid builder; each test now overrides only the single invalid field.
##########
core/src/test/java/org/apache/iceberg/TestDVUtil.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.junit.jupiter.api.Test;
+
+public class TestDVUtil {
+
+ @Test
+ public void validateDVRejectsNullOffset() {
+ DeleteFile dv = dv(null, 10L);
Review Comment:
Done b36c242fa. Took the parameterized variant for both the reject and accept cases, and applied the same shape to TestBaseDeleteLoader.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313089058
##########
core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java:
##########
@@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) {
deleteFile.referencedDataFile());
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
+ dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() <= Integer.MAX_VALUE,
Review Comment:
Doesn't this need to be strictly less than?
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313167899
##########
core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.util;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.apache.iceberg.DeleteFile;
+import org.junit.jupiter.api.Test;
+
+public class TestContentFileUtil {
+
+ @Test
+ public void validateDVRejectsNullOffset() {
+ DeleteFile dv = dv(null, 10L);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset cannot be null");
+ }
+
+ @Test
+ public void validateDVRejectsNullLength() {
+ DeleteFile dv = dv(0L, null);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length cannot be null");
+ }
+
+ @Test
+ public void validateDVRejectsNegativeOffset() {
+ DeleteFile dv = dv(-1L, 10L);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void validateDVRejectsNegativeLength() {
+ DeleteFile dv = dv(0L, -1L);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ @Test
+ public void validateDVRejectsLengthAboveIntegerMax() {
+ DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE + 1);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Can't read DV larger than 2GB");
+ }
+
+ @Test
+ public void validateDVAcceptsZero() {
+ DeleteFile dv = dv(0L, 0L);
+ assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException();
+ }
+
+ @Test
+ public void validateDVAcceptsTypicalValues() {
+ DeleteFile dv = dv(4L, 4096L);
+ assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException();
+ }
+
+ @Test
+ public void validateDVAcceptsMaximumLength() {
+ DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE);
+ assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException();
+ }
+
+ private static DeleteFile dv(Long offset, Long length) {
Review Comment:
There's no need for a mock. Can't this use the builder like the other tests? What was the purpose of using a mock instead of a real implementation?
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313167899
##########
core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.util;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.apache.iceberg.DeleteFile;
+import org.junit.jupiter.api.Test;
+
+public class TestContentFileUtil {
+
+ @Test
+ public void validateDVRejectsNullOffset() {
+ DeleteFile dv = dv(null, 10L);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset cannot be null");
+ }
+
+ @Test
+ public void validateDVRejectsNullLength() {
+ DeleteFile dv = dv(0L, null);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length cannot be null");
+ }
+
+ @Test
+ public void validateDVRejectsNegativeOffset() {
+ DeleteFile dv = dv(-1L, 10L);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void validateDVRejectsNegativeLength() {
+ DeleteFile dv = dv(0L, -1L);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ @Test
+ public void validateDVRejectsLengthAboveIntegerMax() {
+ DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE + 1);
+ assertThatThrownBy(() -> ContentFileUtil.validateDV(dv))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Can't read DV larger than 2GB");
+ }
+
+ @Test
+ public void validateDVAcceptsZero() {
+ DeleteFile dv = dv(0L, 0L);
+ assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException();
+ }
+
+ @Test
+ public void validateDVAcceptsTypicalValues() {
+ DeleteFile dv = dv(4L, 4096L);
+ assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException();
+ }
+
+ @Test
+ public void validateDVAcceptsMaximumLength() {
+ DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE);
+ assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException();
+ }
+
+ private static DeleteFile dv(Long offset, Long length) {
Review Comment:
There's no need for a mock. Can't this use the builder like the other tests? What was the purpose of using a mock instead of a real implementation?
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313208221
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
+ @Override
+ public Long contentOffset() {
+ return offset;
+ }
+
+ @Override
+ public Long contentSizeInBytes() {
+ return size;
+ }
+ };
+ }
+
+ // The validator must fire before any I/O attempt — if it does not, the loader will call this
Review Comment:
Please remove AI artifacts, like `—`
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315057274
##########
core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java:
##########
@@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) {
deleteFile.referencedDataFile());
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
+ dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+ Preconditions.checkArgument(
+ dv.contentSizeInBytes() <= Integer.MAX_VALUE,
Review Comment:
Fixed in f76cef519.
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -52,6 +52,7 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
ContentFileUtil.isDV(deleteFile),
"Cannot read, not a deletion vector: %s",
deleteFile.location());
+ ContentFileUtil.validateDV(deleteFile);
Review Comment:
Done in f76cef519. DVUtil is now public so BaseDeleteLoader (data module) can call it.
##########
core/src/main/java/org/apache/iceberg/FileMetadata.java:
##########
@@ -263,6 +263,12 @@ public DeleteFile build() {
if (format == FileFormat.PUFFIN) {
Preconditions.checkArgument(contentOffset != null, "Content offset is required for DV");
Preconditions.checkArgument(contentSizeInBytes != null, "Content size is required for DV");
+ Preconditions.checkArgument(
+ contentOffset >= 0, "Content offset must be non-negative for DV: %s", contentOffset);
+ Preconditions.checkArgument(
+ contentSizeInBytes >= 0,
Review Comment:
Done in f76cef519.
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
Review Comment:
Done in f76cef519 (Mockito mock).
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
+ @Override
+ public Long contentOffset() {
+ return offset;
+ }
+
+ @Override
+ public Long contentSizeInBytes() {
+ return size;
+ }
+ };
+ }
+
+ // The validator must fire before any I/O attempt — if it does not, the loader will call this
Review Comment:
Done in f76cef519. Removed across the whole branch diff.
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
Review Comment:
Done 0c4b66d75.
##########
core/src/test/java/org/apache/iceberg/TestFileMetadata.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.Test;
+
+public class TestFileMetadata {
+
+ @Test
+ public void dvBuilderRejectsNegativeContentOffset() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(-1L)
+ .withContentSizeInBytes(10L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content offset must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsNegativeContentSize() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(-1L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content size must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsContentSizeAtIntegerMax() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(Integer.MAX_VALUE)
+ .build())
Review Comment:
Done 0c4b66d75. Added a validDvBuilder() returning a valid builder; each test now overrides only the single invalid field.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3336637772
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
Review Comment:
there's enough repetition here to make factoring stuff out something to consider, though I'll leave that to others to consider.
Everything looks for non-null, positive, and sizes must be < max_int.
split it out and you've got less to test
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
Review Comment:
nit, add IllegalArgumentException to the javadocs
##########
core/src/test/java/org/apache/iceberg/TestFileMetadata.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.junit.jupiter.api.Test;
+
+public class TestFileMetadata {
+
+ @Test
+ public void dvBuilderRejectsNegativeContentOffset() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(-1L)
+ .withContentSizeInBytes(10L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content offset must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsNegativeContentSize() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(-1L)
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Content size must be non-negative for DV");
+ }
+
+ @Test
+ public void dvBuilderRejectsContentSizeAtIntegerMax() {
+ assertThatThrownBy(
+ () ->
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile("/tmp/data.parquet")
+ .withContentOffset(0L)
+ .withContentSizeInBytes(Integer.MAX_VALUE)
+ .build())
Review Comment:
given that all `FileMetadata.Builder.Builder()` setters are valid, you could factor these out to have a builder which creates a valid file, then each test overrides with the invalid value, e.g
```
assertThatThrownBy(() ->
deleteFile()
.withContentSizeInBytes(Integer.MAX_VALUE)
.build()))
...
```
It'd make it clearer what is being invalid in each test.
again, personal opinion and I'll yield to others
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3528877741
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -64,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
}
}
+ /**
+ * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed
+ * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise
+ * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation
+ * when the DV blob is read.
+ */
+ public static void validateDV(DeleteFile dv) {
+ Preconditions.checkArgument(
Review Comment:
makes sense. the factoring of the construction works though, really makes clear what it is that you have set.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313154174
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -52,6 +52,7 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) {
ContentFileUtil.isDV(deleteFile),
"Cannot read, not a deletion vector: %s",
deleteFile.location());
+ ContentFileUtil.validateDV(deleteFile);
Review Comment:
Since this class already handles `DeleteFile` instances, I'd probably not modify `ContentFileUtil` and put the new method here. People aren't going to go looking for a DV validation method in `ContentFileUtil`.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3313176773
##########
core/src/main/java/org/apache/iceberg/FileMetadata.java:
##########
@@ -263,6 +263,12 @@ public DeleteFile build() {
if (format == FileFormat.PUFFIN) {
Preconditions.checkArgument(contentOffset != null, "Content offset is required for DV");
Preconditions.checkArgument(contentSizeInBytes != null, "Content size is required for DV");
+ Preconditions.checkArgument(
+ contentOffset >= 0, "Content offset must be non-negative for DV: %s", contentOffset);
+ Preconditions.checkArgument(
+ contentSizeInBytes >= 0,
Review Comment:
Why not check < Integer.MAX_VALUE here as well?
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3315064609
##########
data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseDeleteLoader {
+
+ private static final String DATA_FILE = "/tmp/data.parquet";
+
+ private static final DeleteFile VALID_DV =
+ FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath("/tmp/dv.puffin")
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .withReferencedDataFile(DATA_FILE)
+ .withContentOffset(0L)
+ .withContentSizeInBytes(10L)
+ .build();
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentOffset() {
+ DeleteFile dv = tamperedDV(-1L, 10L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("offset must be non-negative");
+ }
+
+ @Test
+ public void loadPositionDeletesRejectsNegativeContentSize() {
+ DeleteFile dv = tamperedDV(0L, -1L);
+ DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile());
+
+ assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("length must be non-negative");
+ }
+
+ // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest,
+ // since the FileMetadata builder now rejects negative values at construction time.
+ private static DeleteFile tamperedDV(Long offset, Long size) {
+ return new Delegates.DelegatingDeleteFile(VALID_DV) {
+ @Override
+ public Long contentOffset() {
+ return offset;
+ }
+
+ @Override
+ public Long contentSizeInBytes() {
+ return size;
+ }
+ };
+ }
+
+ // The validator must fire before any I/O attempt — if it does not, the loader will call this
Review Comment:
Done in 30256ab67. Removed across the whole branch diff.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#discussion_r3645790714
##########
core/src/test/java/org/apache/iceberg/TestDVUtil.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.junit.jupiter.api.Test;
+
+public class TestDVUtil {
+
+ @Test
+ public void validateDVRejectsNullOffset() {
+ DeleteFile dv = dv(null, 10L);
Review Comment:
Done 36cec1099. Took the parameterized variant for both the reject and accept cases, and applied the same shape to TestBaseDeleteLoader.
--
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, Data: Validate deletion-vector offset and length [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16500:
URL: https://github.com/apache/iceberg/pull/16500#issuecomment-5674519698
@rdblue rebased on main and green again; the only new change is test coverage for the `readDV` path. Could you take another look when you get a chance?
--
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