You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "gaborkaszab (via GitHub)" <gi...@apache.org> on 2026/10/13 12:31:10 UTC
[PR] Core, Spark 4.2: Refactor: Wire in InputFilesDecryptor into Spark readers [iceberg]
gaborkaszab opened a new pull request, #18486:
URL: https://github.com/apache/iceberg/pull/18486
Motivation:
- Use the same decryptior class across all of the readers: Currently, the readers are asymmetrical, because some of them can read encrypted files, some can't , some can read encrypted data files but not delete files. Wiring in the decryption to all of them will make them able to. Relevant PRs: [Flink](https://github.com/apache/iceberg/pull/18198), [GenericReader](https://github.com/apache/iceberg/pull/18481), [IcebergInputFormat](https://github.com/apache/iceberg/pull/18484). `ArrowReader` is on my list too
- Later, extending `InputFilesDecryptor` to decrypt column files too will be available to all the readers out of the box
--
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, Spark 4.2: Refactor: Wire in InputFilesDecryptor into Spark readers [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #18486:
URL: https://github.com/apache/iceberg/pull/18486#issuecomment-6217860436
Thanks for the review, @nastra !
@anuragmantri Do you think you can take a look at this from Spark's perspective?
--
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, Spark 4.2: Refactor: Wire in InputFilesDecryptor into Spark readers [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #18486:
URL: https://github.com/apache/iceberg/pull/18486#discussion_r4297558202
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/BaseReader.java:
##########
@@ -148,12 +148,8 @@ public boolean next() throws IOException {
}
}
} catch (IOException | RuntimeException e) {
- if (currentTask != null && !currentTask.isDataTask()) {
- String filePaths =
- referencedFiles(currentTask)
- .map(ContentFile::location)
- .collect(Collectors.joining(", "));
- LOG.error("Error reading file(s): {}", filePaths, e);
+ if (currentTask instanceof ContentScanTask<?> contentTask && !currentTask.isDataTask()) {
+ LOG.error("Error reading file: {}", contentTask.file().location(), e);
Review Comment:
Before this change a read failure logged every file the task touched, so for a `FileScanTask` you'd see the data file and its delete files. Now only `task.file()` shows up. That last one has no path in its message, and it's exactly the case this encryption work targets. Could we keep logging the delete files too? Keeping `referencedFiles(TaskT)` in `BaseReader` would let this block stay as it was. I added more detail in the top-level comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark 4.2: Refactor: Wire in InputFilesDecryptor into Spark readers [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #18486:
URL: https://github.com/apache/iceberg/pull/18486#discussion_r4294906787
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/BatchDataReader.java:
##########
@@ -106,7 +104,6 @@ protected CloseableIterator<ColumnarBatch> open(FileScanTask task) {
InputFileBlockHolder.set(filePath, task.start(), task.length());
InputFile inputFile = getInputFile(filePath);
- Preconditions.checkNotNull(inputFile, "Could not find InputFile associated with FileScanTask");
Review Comment:
Yes, `InputFilesDecryptor.getInputFiles(String)` has a Precondition to avoid returning null. Making the Precondition pointless here.
--
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, Spark 4.2: Refactor: Wire in InputFilesDecryptor into Spark readers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18486:
URL: https://github.com/apache/iceberg/pull/18486#discussion_r4293204683
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/BatchDataReader.java:
##########
@@ -106,7 +104,6 @@ protected CloseableIterator<ColumnarBatch> open(FileScanTask task) {
InputFileBlockHolder.set(filePath, task.start(), task.length());
InputFile inputFile = getInputFile(filePath);
- Preconditions.checkNotNull(inputFile, "Could not find InputFile associated with FileScanTask");
Review Comment:
do we guarantee that the new impl never returns null?
--
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, Spark 4.2: Refactor: Wire in InputFilesDecryptor into Spark readers [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #18486:
URL: https://github.com/apache/iceberg/pull/18486#discussion_r4323360651
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/BaseReader.java:
##########
@@ -148,12 +148,8 @@ public boolean next() throws IOException {
}
}
} catch (IOException | RuntimeException e) {
- if (currentTask != null && !currentTask.isDataTask()) {
- String filePaths =
- referencedFiles(currentTask)
- .map(ContentFile::location)
- .collect(Collectors.joining(", "));
- LOG.error("Error reading file(s): {}", filePaths, e);
+ if (currentTask instanceof ContentScanTask<?> contentTask && !currentTask.isDataTask()) {
+ LOG.error("Error reading file: {}", contentTask.file().location(), e);
Review Comment:
1. Logging delete files
I'm afraid we don't have a unified interface the get the delete files across the implementations: We have a `ContentScanTask` here that doesn't give an interface for deletes, and I don't think we want to cast to specific types here just for logging.
I agree that we loose some of the previous functionality here, but I believe that since this is logging, we won't sacrifice any actual functionality by simply logging the data file's location.
2. referencedFiles()
One of the main goals of the decryptor call is that once that's wired in, we don;t have to change all the readers to add column files to the referenced files list, we could do that internally in the decryptor making it available to all readers out of the box. I agree that the changeling reader and position delete reader are special in this case constructing their own referenced files list. I made some adjustments to the decryptor to do that internally, moving code out from these readers. WDYT?
--
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