wombatu-kun commented on code in PR #17253:
URL: https://github.com/apache/iceberg/pull/17253#discussion_r3610232683
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinReader.java:
##########
@@ -133,8 +133,18 @@ public Iterable<Pair<BlobMetadata, ByteBuffer>>
readAll(List<BlobMetadata> blobs
.map(
(BlobMetadata blobMetadata) -> {
try {
- input.seek(blobMetadata.offset());
- byte[] bytes = new
byte[Math.toIntExact(blobMetadata.length())];
+ long offset = blobMetadata.offset();
+ long length = blobMetadata.length();
+ Preconditions.checkArgument(offset >= 0, "Invalid blob
offset: %s", offset);
Review Comment:
The new blob offset/length bounds checks in readAll have no test, unlike the
footer and bitmap paths this PR covers. Add a case that consumes readAll for a
BlobMetadata whose offset or length exceeds fileSize and asserts the
IllegalArgumentException.
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -49,11 +49,32 @@
class DVUtil {
private DVUtil() {}
+ /**
+ * Validate a DV. Based on the original from BaseDeleteLoader.
+ *
+ * @param dv deletion vector.
+ */
+ private static void validateDV(DeleteFile dv) {
Review Comment:
validateDV duplicates the first three checks of BaseDeleteLoader.validateDV;
consider hoisting the shared shape validation into ContentFileUtil next to
dvDesc so the query and merge paths use one copy. Follow-up, not a blocker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
