You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "szlta (via GitHub)" <gi...@apache.org> on 2025/11/20 15:12:54 UTC
[PR] Core: Wrong reported length of encrypted Puffin files [iceberg]
szlta opened a new pull request, #14645:
URL: https://github.com/apache/iceberg/pull/14645
Manifest files made for puffin files track the
length of new files as `PuffinWriter#length()`.
The underlying PositionOutputStream provides this
as `getPos()`, but for encrypted files this is not
the true file length, rather the unencrypted
content length.
Tested locally and with UT.
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2550155898
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,36 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testLengthCalculation(boolean isEncrypted) throws Exception {
Review Comment:
I opted for `testFileSizeCalculation` - since all the other test cases here are prepended with `test-`
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2549245467
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +91,21 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @Test
+ public void testEncryptedLength() throws Exception {
Review Comment:
Sure, adding an encrypted and an unencrypted case makes sense. Thanks for pointing to `TestGcmStreams`, I got some insipration from there on creating real encrypted stream instead of a mock one.
Now I have a parameterized test that has no mock and tests both cases. I don't think there's a need to add anything into `TestGcmStreams` though, we're not testing any stream here, the class under test is just `PuffinWriter`.
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2549993144
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,36 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testLengthCalculation(boolean isEncrypted) throws Exception {
Review Comment:
```suggestion
public void fileSizeCalculation(boolean isEncrypted) throws Exception {
```
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2555421378
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,37 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
Review Comment:
This looks nice, thanks for the suggestion
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#issuecomment-3558592270
cc: @ggershinsky @huaxingao
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#issuecomment-3569729723
> I think we need to back-port the fix to 1.10.x. I will add this to 1.10.x milestone.
1.10.x doesn't have manifest list encryption (and quite few related) commits backported either. I'm not sure backporting the Puffin writer fix makes much sense without 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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2546589803
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +91,21 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @Test
+ public void testEncryptedLength() throws Exception {
Review Comment:
can we add the same test for an unencrypted stream?
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2550152595
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,36 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testLengthCalculation(boolean isEncrypted) throws Exception {
+ final OutputFile outputFile;
+
+ if (isEncrypted) {
+ File testFile = temp.resolve("test" + System.nanoTime()).toFile();
+ Random random = new Random();
+ byte[] key = new byte[16];
+ random.nextBytes(key);
+ byte[] aadPrefix = new byte[16];
+ outputFile = new AesGcmOutputFile(Files.localOutput(testFile), key, aadPrefix);
Review Comment:
Fixed
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,36 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testLengthCalculation(boolean isEncrypted) throws Exception {
+ final OutputFile outputFile;
+
+ if (isEncrypted) {
+ File testFile = temp.resolve("test" + System.nanoTime()).toFile();
+ Random random = new Random();
+ byte[] key = new byte[16];
+ random.nextBytes(key);
+ byte[] aadPrefix = new byte[16];
+ outputFile = new AesGcmOutputFile(Files.localOutput(testFile), key, aadPrefix);
+ } else {
+ outputFile = new InMemoryOutputFile();
+ }
+
+ PuffinWriter writer = Puffin.write(outputFile).build();
+ writer.write(
+ new Blob(
+ "blob",
+ ImmutableList.of(1),
+ 2,
+ 1,
+ ByteBuffer.wrap("blob".getBytes()),
+ null,
+ ImmutableMap.of()));
+ writer.close();
+ assertThat(writer.length()).isEqualTo(isEncrypted ? 158 : 122);
Review Comment:
Fixed
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2550267037
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,37 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testFileSizeCalculation(boolean isEncrypted) throws Exception {
+ final OutputFile outputFile;
+
+ if (isEncrypted) {
+ File testFile = temp.resolve("test" + System.nanoTime()).toFile();
+ Random random = new Random();
+ byte[] key = new byte[16];
+ random.nextBytes(key);
+ byte[] aadPrefix = new byte[16];
+ random.nextBytes(aadPrefix);
+ outputFile = new AesGcmOutputFile(Files.localOutput(testFile), key, aadPrefix);
+ } else {
+ outputFile = new InMemoryOutputFile();
+ }
+
+ PuffinWriter writer = Puffin.write(outputFile).build();
+ writer.write(
+ new Blob(
+ "blob",
+ ImmutableList.of(1),
+ 2,
+ 1,
+ ByteBuffer.wrap("blob".getBytes()),
+ null,
+ ImmutableMap.of()));
+ writer.close();
+ assertThat(writer.length()).isEqualTo(isEncrypted ? 158L : 122L);
Review Comment:
It would be nice (if not covered already) that we can check we can read this file post write
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,37 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
Review Comment:
minor : can avoid if checks on length
```
@CsvSource({
"true, 156L", // Test case 1: isEncrypted=true, expectedSize=156L
"false, 122L" // Test case 2: isEncrypted=false, expectedSize=122L
})
```
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinWriter.java:
##########
@@ -144,7 +142,8 @@ public void finish() throws IOException {
long footerOffset = outputStream.getPos();
writeFooter();
this.footerSize = Optional.of(Math.toIntExact(outputStream.getPos() - footerOffset));
- this.fileSize = Optional.of(outputStream.getPos());
+ outputStream.close();
Review Comment:
seems like the close() can trigger a write to the outputStream and post writer wrote the footer
https://github.com/apache/iceberg/blob/fb52fdef4fdcaa3cb5779e4d5b1b9f1ea7522e73/core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java#L162
wondering we should update the description that this patch additionally includes this fix too ?
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#issuecomment-3571108828
thanks for fixing this @szlta and thanks to @huaxingao and @singhpk234 for the additional reviews
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2551438577
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinWriter.java:
##########
@@ -144,7 +142,8 @@ public void finish() throws IOException {
long footerOffset = outputStream.getPos();
writeFooter();
this.footerSize = Optional.of(Math.toIntExact(outputStream.getPos() - footerOffset));
- this.fileSize = Optional.of(outputStream.getPos());
+ outputStream.close();
Review Comment:
nit: should we add a short comment here explaining why we call `close()` before computing `fileSize` and use `storedLength()` instead of `getPos()`?
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#issuecomment-3565132612
I think we need to back-port the fix to 1.10.x. I will add this to 1.10.x milestone.
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2555418606
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,37 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testFileSizeCalculation(boolean isEncrypted) throws Exception {
+ final OutputFile outputFile;
+
+ if (isEncrypted) {
+ File testFile = temp.resolve("test" + System.nanoTime()).toFile();
+ Random random = new Random();
+ byte[] key = new byte[16];
+ random.nextBytes(key);
+ byte[] aadPrefix = new byte[16];
+ random.nextBytes(aadPrefix);
+ outputFile = new AesGcmOutputFile(Files.localOutput(testFile), key, aadPrefix);
+ } else {
+ outputFile = new InMemoryOutputFile();
+ }
+
+ PuffinWriter writer = Puffin.write(outputFile).build();
+ writer.write(
+ new Blob(
+ "blob",
+ ImmutableList.of(1),
+ 2,
+ 1,
+ ByteBuffer.wrap("blob".getBytes()),
+ null,
+ ImmutableMap.of()));
+ writer.close();
+ assertThat(writer.length()).isEqualTo(isEncrypted ? 158L : 122L);
Review Comment:
I don't think it's in the scope of this change. Sounds rather as a new IT test to be made that covers writing Puffin delete files and then reading it back (along with the respective manifest writes and reads where this fix is actually reflected)
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2546612332
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +91,21 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @Test
+ public void testEncryptedLength() throws Exception {
Review Comment:
also it would be good to have an actual real test without mocking, which should probably go into `TestGcmStreams`
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2548325519
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +91,21 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @Test
+ public void testEncryptedLength() throws Exception {
Review Comment:
+1 for both 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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2549989762
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,36 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testLengthCalculation(boolean isEncrypted) throws Exception {
+ final OutputFile outputFile;
+
+ if (isEncrypted) {
+ File testFile = temp.resolve("test" + System.nanoTime()).toFile();
+ Random random = new Random();
+ byte[] key = new byte[16];
+ random.nextBytes(key);
+ byte[] aadPrefix = new byte[16];
+ outputFile = new AesGcmOutputFile(Files.localOutput(testFile), key, aadPrefix);
+ } else {
+ outputFile = new InMemoryOutputFile();
+ }
+
+ PuffinWriter writer = Puffin.write(outputFile).build();
+ writer.write(
+ new Blob(
+ "blob",
+ ImmutableList.of(1),
+ 2,
+ 1,
+ ByteBuffer.wrap("blob".getBytes()),
+ null,
+ ImmutableMap.of()));
+ writer.close();
+ assertThat(writer.length()).isEqualTo(isEncrypted ? 158 : 122);
Review Comment:
```suggestion
assertThat(writer.length()).isEqualTo(isEncrypted ? 158L : 122L);
```
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2549989151
##########
core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java:
##########
@@ -86,6 +98,36 @@ public void testWriteMetricDataCompressedZstd() throws Exception {
testWriteMetric(ZSTD, "v1/sample-metric-data-compressed-zstd.bin");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testLengthCalculation(boolean isEncrypted) throws Exception {
+ final OutputFile outputFile;
+
+ if (isEncrypted) {
+ File testFile = temp.resolve("test" + System.nanoTime()).toFile();
+ Random random = new Random();
+ byte[] key = new byte[16];
+ random.nextBytes(key);
+ byte[] aadPrefix = new byte[16];
+ outputFile = new AesGcmOutputFile(Files.localOutput(testFile), key, aadPrefix);
Review Comment:
I think this is missing `random.nextBytes(aadPrefix);`
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #14645:
URL: https://github.com/apache/iceberg/pull/14645
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#discussion_r2555405692
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinWriter.java:
##########
@@ -144,7 +142,8 @@ public void finish() throws IOException {
long footerOffset = outputStream.getPos();
writeFooter();
this.footerSize = Optional.of(Math.toIntExact(outputStream.getPos() - footerOffset));
- this.fileSize = Optional.of(outputStream.getPos());
+ outputStream.close();
Review Comment:
@singhpk234 makes sense, I've updated the description.
--
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: Wrong reported length of encrypted Puffin files [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on PR #14645:
URL: https://github.com/apache/iceberg/pull/14645#issuecomment-3571589297
Thanks everyone for the reviews
--
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