gaborkaszab commented on code in PR #17545:
URL: https://github.com/apache/iceberg/pull/17545#discussion_r4052646273
##########
api/src/main/java/org/apache/iceberg/io/FileIO.java:
##########
@@ -80,6 +85,13 @@ default InputFile newInputFile(ManifestListFile
manifestList) {
return newInputFile(manifestList.location());
}
+ default InputFile newInputFile(String location, String keyId) {
Review Comment:
On the general case what I have in mind is that we don't want the callers to
branch on whether keyId is null or not. It's a cleaner implementation to call
`newInputFile(location, keyId)` unconditionally for files that might be
encrypted with the key-id based mechanism and then internally we can decide
which path to go.
One such implementation would look like this:
```
StatisticsFile statFile = // get the stats file from table metadata
EncryptingFileIO io = EncryptingFileIO.combine(table.io(),
table.encryption());
InputFile inputFile = io.newInputFile(statFile.path(), statFile.keyId());
```
This way the caller code is clean enough, no need to branch on keyId being
null, we can fallback to the unencrypted case inside `newInputFile`.
This is what `BaseSnapshot.cacheManifests()` ->
`ManifestLists.newInputFile()` -> io.newInputFile()` path does now.
About other such APIs, I'm not sure about other APIs, but the `keyMetadata`
based ones seem to do the same here, and for me this seems to give a nice
flexibility: even if we have an EncryptionFileIO there might be unencrypted
files without keyMetadata or keyId and then we can silently fallback to the
unencrypted case without making the caller to make this decision.
##########
api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java:
##########
@@ -140,6 +145,16 @@ public InputFile newInputFile(ManifestListFile
manifestList) {
}
}
+ @Override
+ public InputFile newInputFile(String location, String keyId) {
+ if (keyId != null) {
Review Comment:
I think having a length param could make sense as you described above to be
consistent with other function and to have a length available in the "fallback
to unencrypted read" case.
My understanding might not be that strong here, but I think for manifests
list files we are in a weird situation because we don't directly keep the
length of the physical encrypted file. We do keep the location and keyId on the
snapshot, and in turn we can get the keyMetadata based on the keyId. When it
comes to reading we assume that the length is integrated into keyMetadata. So
for manifest list files I don't think there is a straightforward way to use the
proposed `newInputFile(path, length, keyMetadata or keyId)`.
Just a general comment, that with the proposed new function, we could
technically wipe out all the variations of `newInputFile(DataFile)`,
`newInputFile(DeleteFile)`, `newInputFile(ManifestFile)` because all they need
is a path, a length and keyMetadata. Unfortunately, we keep no length for
manifest list files as described above.
Should we proposed keeping the length in `Snapshot` for the new V4 root
manifest since we are actively working on it and then we can avoid relying on
length being embedded into keyMetadata and could use length as a param for
`newInputFile`? cc @amogh-jahagirdar @stevenzwu
On the question of should we fail if keyMetadata is null or fallback to the
unencrypted path, I think the latter is cleaner on the caller side. See my
example on the other 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: [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]
