You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Soham-Bhattacharjee-work (via GitHub)" <gi...@apache.org> on 2026/05/02 13:17:33 UTC
[PR] Making From conversions fallible with `TryFrom` [datafusion]
Soham-Bhattacharjee-work opened a new pull request, #21985:
URL: https://github.com/apache/datafusion/pull/21985
## Which issue does this PR close?
- Closes #21974 .
## Rationale for this change
The conversions from `ConfigFileDecryptionProperties` to `FileDecryptionProperties` and from `ConfigFileEncryptionProperties` to `FileEncryptionProperties` currently 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 return `DataFusionError::Confoguration` errors.
## What changes are included in this PR?
``` rust
impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties
```
becomes
``` rust
impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties
```
Similarly
```rust
impl From<ConfigFileDecryptionProperties> for FileDecryptionProperties
```
becomes
```rust
impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties
```
These are header changes with accompanied implementation changes
## Are these changes tested?
Yes tests are added in `datafusion/common/src/config.rs`
## Are there any user-facing changes?
No according to me, they are not. They are part of `datafusion-common`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal merged PR #21985:
URL: https://github.com/apache/datafusion/pull/21985
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "Soham-Bhattacharjee-work (via GitHub)" <gi...@apache.org>.
Soham-Bhattacharjee-work commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3178405862
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
Review Comment:
Done
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
+ type Error = DataFusionError;
+
+ fn try_from(val: ConfigFileEncryptionProperties) -> Result<Self> {
let mut fep = FileEncryptionProperties::builder(
- hex::decode(val.footer_key_as_hex).unwrap(),
+ hex::decode(val.footer_key_as_hex)
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key from ConfigFileEncryptionProperties: {e}"))
+ })?,
)
.with_plaintext_footer(!val.encrypt_footer)
.with_aad_prefix_storage(val.store_aad_prefix);
if !val.footer_key_metadata_as_hex.is_empty() {
fep = fep.with_footer_key_metadata(
hex::decode(&val.footer_key_metadata_as_hex)
- .expect("Invalid footer key metadata"),
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key metadata from ConfigFileEncryptionProperties: {e}"))
+ })?,
);
}
for (column_name, encryption_props) in val.column_encryption_properties.iter() {
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}"))
Review Comment:
Done
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "Soham-Bhattacharjee-work (via GitHub)" <gi...@apache.org>.
Soham-Bhattacharjee-work commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3178405971
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
+ type Error = DataFusionError;
+
+ fn try_from(val: ConfigFileEncryptionProperties) -> Result<Self> {
let mut fep = FileEncryptionProperties::builder(
- hex::decode(val.footer_key_as_hex).unwrap(),
+ hex::decode(val.footer_key_as_hex)
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key from ConfigFileEncryptionProperties: {e}"))
+ })?,
)
.with_plaintext_footer(!val.encrypt_footer)
.with_aad_prefix_storage(val.store_aad_prefix);
if !val.footer_key_metadata_as_hex.is_empty() {
fep = fep.with_footer_key_metadata(
hex::decode(&val.footer_key_metadata_as_hex)
- .expect("Invalid footer key metadata"),
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key metadata from ConfigFileEncryptionProperties: {e}"))
+ })?,
);
}
for (column_name, encryption_props) in val.column_encryption_properties.iter() {
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}"))
Review Comment:
Oops sorry ! 😄
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
+ type Error = DataFusionError;
+
+ fn try_from(val: ConfigFileEncryptionProperties) -> Result<Self> {
let mut fep = FileEncryptionProperties::builder(
- hex::decode(val.footer_key_as_hex).unwrap(),
+ hex::decode(val.footer_key_as_hex)
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key from ConfigFileEncryptionProperties: {e}"))
+ })?,
)
.with_plaintext_footer(!val.encrypt_footer)
.with_aad_prefix_storage(val.store_aad_prefix);
if !val.footer_key_metadata_as_hex.is_empty() {
fep = fep.with_footer_key_metadata(
hex::decode(&val.footer_key_metadata_as_hex)
- .expect("Invalid footer key metadata"),
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key metadata from ConfigFileEncryptionProperties: {e}"))
+ })?,
);
}
for (column_name, encryption_props) in val.column_encryption_properties.iter() {
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}"))
Review Comment:
Changed
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3176936177
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
Review Comment:
we need entry in upgrade guide.
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
+ type Error = DataFusionError;
+
+ fn try_from(val: ConfigFileEncryptionProperties) -> Result<Self> {
let mut fep = FileEncryptionProperties::builder(
- hex::decode(val.footer_key_as_hex).unwrap(),
+ hex::decode(val.footer_key_as_hex)
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key from ConfigFileEncryptionProperties: {e}"))
+ })?,
)
.with_plaintext_footer(!val.encrypt_footer)
.with_aad_prefix_storage(val.store_aad_prefix);
if !val.footer_key_metadata_as_hex.is_empty() {
fep = fep.with_footer_key_metadata(
hex::decode(&val.footer_key_metadata_as_hex)
- .expect("Invalid footer key metadata"),
+ .map_err(|e| {
+ DataFusionError::Configuration(format!("Unable to decode hex footer key metadata from ConfigFileEncryptionProperties: {e}"))
+ })?,
);
}
for (column_name, encryption_props) in val.column_encryption_properties.iter() {
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}"))
Review Comment:
message says "encryption key metadata", but this is decoding the actual column encryption key. Please change it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "Soham-Bhattacharjee-work (via GitHub)" <gi...@apache.org>.
Soham-Bhattacharjee-work commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3178405862
##########
datafusion/common/src/config.rs:
##########
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
- fn from(val: ConfigFileEncryptionProperties) -> Self {
+impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
Review Comment:
Done @kumarUjjawal
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3183689334
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -447,6 +447,109 @@ let config_decryption_properties = ConfigFileDecryptionProperties::try_from(&dec
See [#21602](https://github.com/apache/datafusion/issues/21602) and
[PR #21603](https://github.com/apache/datafusion/pull/21603) for details.
+### Conversion from `ConfigFileEncryptionProperties` / `ConfigFileDecryptionProperties` is now fallible
+
+Previously, `datafusion_common::config::ConfigFileEncryptionProperties` and
+`datafusion_common::config::ConfigFileDecryptionProperties` implemented infallible
+conversions into Parquet's encryption/decryption types:
+
+- `impl From<ConfigFileEncryptionProperties> for parquet::encryption::encrypt::FileEncryptionProperties`
+- `impl From<ConfigFileDecryptionProperties> for parquet::encryption::decrypt::FileDecryptionProperties`
+
+These conversions may need to decode hex-encoded keys and other configuration values, which can fail.
+They now use `TryFrom` and return a `Result`:
+
+- `impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties`
+- `impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties`
+
+**Migration guide:**
+
+Replace `from()` / `into()` with `try_from()` / `try_into()` and handle the resulting `Result`.
+
+**Before:**
+
+```rust,ignore
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#issuecomment-4385097291
Thank you @Soham-Bhattacharjee-work and @adamreeve
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "Soham-Bhattacharjee-work (via GitHub)" <gi...@apache.org>.
Soham-Bhattacharjee-work commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3183617022
##########
datafusion/common/src/config.rs:
##########
@@ -3019,36 +3040,54 @@ impl ConfigField for ConfigFileDecryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileDecryptionProperties> for FileDecryptionProperties {
- fn from(val: ConfigFileDecryptionProperties) -> Self {
+impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties {
+ type Error = DataFusionError;
+
+ fn try_from(val: ConfigFileDecryptionProperties) -> Result<Self> {
let mut column_names: Vec<&str> = Vec::new();
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| {
Review Comment:
Done
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "Soham-Bhattacharjee-work (via GitHub)" <gi...@apache.org>.
Soham-Bhattacharjee-work commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3184463259
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -447,6 +447,109 @@ let config_decryption_properties = ConfigFileDecryptionProperties::try_from(&dec
See [#21602](https://github.com/apache/datafusion/issues/21602) and
[PR #21603](https://github.com/apache/datafusion/pull/21603) for details.
+### Conversion from `ConfigFileEncryptionProperties` / `ConfigFileDecryptionProperties` is now fallible
+
+Previously, `datafusion_common::config::ConfigFileEncryptionProperties` and
+`datafusion_common::config::ConfigFileDecryptionProperties` implemented infallible
+conversions into Parquet's encryption/decryption types:
+
+- `impl From<ConfigFileEncryptionProperties> for parquet::encryption::encrypt::FileEncryptionProperties`
+- `impl From<ConfigFileDecryptionProperties> for parquet::encryption::decrypt::FileDecryptionProperties`
+
+These conversions may need to decode hex-encoded keys and other configuration values, which can fail.
+They now use `TryFrom` and return a `Result`:
+
+- `impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties`
+- `impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties`
+
+**Migration guide:**
+
+Replace `from()` / `into()` with `try_from()` / `try_into()` and handle the resulting `Result`.
+
+**Before:**
+
+```rust,ignore
Review Comment:
Done.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Making From conversions fallible with `TryFrom` [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #21985:
URL: https://github.com/apache/datafusion/pull/21985#discussion_r3178819327
##########
datafusion/common/src/config.rs:
##########
@@ -3019,36 +3040,54 @@ impl ConfigField for ConfigFileDecryptionProperties {
}
#[cfg(feature = "parquet_encryption")]
-impl From<ConfigFileDecryptionProperties> for FileDecryptionProperties {
- fn from(val: ConfigFileDecryptionProperties) -> Self {
+impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties {
+ type Error = DataFusionError;
+
+ fn try_from(val: ConfigFileDecryptionProperties) -> Result<Self> {
let mut column_names: Vec<&str> = Vec::new();
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| {
Review Comment:
`column_key_as_hex` has been decoded from hex, so this should probably just be called `column_key`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org