fix: Make conversion from FileDecryptionProperties to ConfigFileDecryptionProperties fallible#21603
fix: Make conversion from FileDecryptionProperties to ConfigFileDecryptionProperties fallible#21603adamreeve wants to merge 2 commits intoapache:mainfrom
Conversation
github-actions
bot
added
core
adamreeve
force-pushed
the
fallible-decrypt-config-convert
branch
from
8cc9c06 to
6d380a1
Compare
| type Error = DataFusionError; | ||
|
|
||
| fn try_from(f: &Arc<FileDecryptionProperties>) -> Result<Self> { | ||
| let footer_key = f.footer_key(None).map_err(|e| { |
There was a problem hiding this comment.
This still depends on footer_key(None) failing. A key retriever can return a footer key even when it still cannot represent the full decryption config. In that case thisconversion can still succeed, but column_keys() is empty and we silently lose the column decryption info. Can we reject all key-retriever-based FileDecryptionProperties directly and add a test for that case?
There was a problem hiding this comment.
Yes, that's not currently possible with the public API of FileDecryptionProperties but I can follow up and change that too. I think it would still make sense to make this current change and then improve this later once arrow-rs allows it.
There was a problem hiding this comment.
Sure, Please open a follow-up issue and link it here so that it is tracked. Also handle the upgrade note.
| #[cfg(feature = "parquet_encryption")] | ||
| impl From<&Arc<FileDecryptionProperties>> for ConfigFileDecryptionProperties { | ||
| fn from(f: &Arc<FileDecryptionProperties>) -> Self { | ||
| impl TryFrom<&Arc<FileDecryptionProperties>> for ConfigFileDecryptionProperties { |
There was a problem hiding this comment.
This changes a public API from From to TryFrom, so downstream code using (&decrypt).into() or ConfigFileDecryptionProperties::from(&decrypt) will stop compiling. Can we add an upgrade note for this change.
2 participants
Which issue does this PR close?
Rationale for this change
Fail quickly with a helpful error if we're unable to represent a
FileDecryptionPropertiesinstance asConfigFileDecryptionPropertiesWhat changes are included in this PR?
From<&Arc<FileDecryptionProperties>>forConfigFileDecryptionPropertiestoTryFrom.FileDecryptionPropertieswith empty metadataAre these changes tested?
Yes I've added a new unit test.
I also tested this with a branch of delta-rs that uses Datafusion with Parquet encryption, and this required only minor changes to tests and examples: corwinjoy/delta-rs@file_format_options_squashed...adamreeve:delta-rs:test-datafusion-change
Are there any user-facing changes?
Yes, this is a breaking API change.