gaborkaszab commented on PR #17545: URL: https://github.com/apache/iceberg/pull/17545#issuecomment-6779284432
Hey @danielcweeks @RussellSpitzer @rdblue @stevenzwu I explored a way to add a common interface, `EncryptedFile` on top of `DataFile`, `DeleteFile`, `ManifestFile` (covering V4 `RootManifestFile`), and `ColumnFile`, and since these behave very similarly, the PR seems simple. I changed the implementation in `FileIO` for them to call the overload for the new interface. For `ManifestListFile` I found that it's not that straightforward to implement this common interface, though. It's feasible but not entirely comfortable because it already has a public API and also behaves somewhat differently: - Doesn't have length: In order to use them in a uniform `newInputFile(EncryptedFile)` path we have to branch on whether length is set or not. For this we need some auxiliary value to indicate "not set", like -1. - Custom implementations: `encryptionKeyId()` and `decryptKeyMetadata(EncryptionManager)` are public APIs now, we have to keep an `newInputFile` implementation that uses these methods not to break potential (but not likely) custom implementations. We need a new major version to entirely move to using the new API For exploration [this is how it would look like](https://github.com/apache/iceberg/commit/e8a14da76ab4fd3ee9c1b859686a975229d7b047) to make `ManifestListFile` implement `EncryptedFile`. -- 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]
