feat: Making From<ConfigFileDecryptionProperties/ConfigFileEncryptionProperties> conversions fallible with TryFrom#21985
Conversation
github-actions
Bot
added
common
Soham-Bhattacharjee-work
changed the title
TryFromTryFrom
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @Soham-Bhattacharjee-work I have left some comments, also can you update the pr body that this is user facing change since it is API breaking
| let encryption_key = hex::decode(&encryption_props.column_key_as_hex) | ||
| .expect("Invalid column encryption key"); | ||
| .map_err(|e| { | ||
| DataFusionError::Configuration(format!("Unable to decode hex encryption key metadata for column {column_name}: {e}")) |
There was a problem hiding this comment.
message says "encryption key metadata", but this is decoding the actual column encryption key. Please change it.
There was a problem hiding this comment.
Oops sorry ! 😄
| #[cfg(feature = "parquet_encryption")] | ||
| impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties { | ||
| fn from(val: ConfigFileEncryptionProperties) -> Self { | ||
| impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties { |
There was a problem hiding this comment.
we need entry in upgrade guide.
…-to-try_from-for-ConfigEncryptionDecryption
github-actions
Bot
added
the
documentation
adamreeve
left a comment
adamreeve
left a comment
There was a problem hiding this comment.
Thanks for picking this up @Soham-Bhattacharjee-work
| let mut column_keys: Vec<Vec<u8>> = Vec::new(); | ||
|
|
||
| for (col_name, decryption_properties) in val.column_decryption_properties.iter() { | ||
| let column_key_as_hex = hex::decode(&decryption_properties.column_key_as_hex).map_err(|e| { |
There was a problem hiding this comment.
column_key_as_hex has been decoded from hex, so this should probably just be called column_key.
…-to-try_from-for-ConfigEncryptionDecryption
|
|
||
| **Before:** | ||
|
|
||
| ```rust,ignore |
There was a problem hiding this comment.
This upgrade guide section feels too long for the actual API change. The migration is just replacing from/into with try_from/try_into and handling the Result. Could we shorten this to a small before/after snippet instead of showing the full encryption and decryption config setup? Take inspiration from the work just above for the similar change.
…-to-try_from-for-ConfigEncryptionDecryption
3 participants
Which issue does this PR close?
From<ConfigFileDecryptionProperties>/ConfigFileEncryptionPropertiesconversions fallible (TryFrom) #21974 .Rationale for this change
The conversions from
ConfigFileDecryptionPropertiestoFileDecryptionPropertiesand fromConfigFileEncryptionPropertiestoFileEncryptionPropertiescurrently use From, but the implementations contain several unwrap() and expect() calls which can cause panics for invalid or wrong inputs. Instead of panics we can gracefully returnDataFusionError::Confogurationerrors.What changes are included in this PR?
becomes
Similarly
becomes
These are header changes with accompanied implementation changes
Are these changes tested?
Yes tests are added in
datafusion/common/src/config.rsAre there any user-facing changes?
Yes this is an user facing change. And upgradation guide is added in
54.0.0.md