You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "corwinjoy (via GitHub)" <gi...@apache.org> on 2025/07/02 02:42:06 UTC
[PR] feat: Add a configuration to make parquet encryption optional [datafusion]
corwinjoy opened a new pull request, #16649:
URL: https://github.com/apache/datafusion/pull/16649
## Which issue does this PR close?
This is a followup to PR https://github.com/apache/datafusion/pull/16351 . In that review, it was suggested that we add a configuration to make encryption optional.
## Rationale for this change
Users may want to minimize crate dependencies.
## What changes are included in this PR?
Will add summary below.
## Are these changes tested?
Covered by existing tests.
## Are there any user-facing changes?
Shouldn't affect the API. If encryption is turned off, the encryption settings will not do anything.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196363819
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
Review Comment:
Would it be better to use `#[cfg(feature = "parquet_encryption")]` on this parameter so this isn't a breaking change unless the encryption feature is enabled? (assuming this PR goes in before the next release)
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
) -> Result<ParquetMetaData> {
let file_size = meta.size;
let fetch = ObjectStoreFetch::new(store, meta);
- ParquetMetaDataReader::new()
- .with_prefetch_hint(size_hint)
- .with_decryption_properties(decryption_properties)
+ #[allow(unused_mut)]
+ let mut reader = ParquetMetaDataReader::new().with_prefetch_hint(size_hint);
Review Comment:
I don't think this mut is needed even with `parquet_encryption` enabled, as `with_decryption_properties` takes ownership of the reader.
```suggestion
let reader = ParquetMetaDataReader::new().with_prefetch_hint(size_hint);
```
##########
datafusion/common/src/encryption.rs:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Support optional features for encryption in Parquet files.
+//! This module provides types and functions related to encryption in Parquet files.
+
+#[cfg(feature = "parquet_encryption")]
Review Comment:
Rather than having so may `cfg` lines in this file, you could have the module implemented in two separate files, one for when encryption is enabled and one for when it's disabled.
Eg. see https://github.com/apache/arrow-rs/blob/ff3a2f2c59f0355f8afedb3e9258e1d6307f21ae/parquet/src/column/mod.rs#L121-L125
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2198712011
##########
datafusion/common/src/encryption.rs:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Support optional features for encryption in Parquet files.
+//! This module provides types and functions related to encryption in Parquet files.
+
+#[cfg(feature = "parquet_encryption")]
Review Comment:
Thanks. That's an interesting idea, but I will stick with the current approach for now.
1. The file and functions are relatively simple, at under 100 lines long.
2. Having two separate files, which may or may not be used, is somewhat confusing.
3. I don't like the idea of having to maintain the same API in two separate places/files. Here, the two versions are displayed side by side, making it straightforward to update them simultaneously.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196418502
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I'm not sure. I set it to the default so that users can have this feature enabled as per the updated docs and available in the CLI.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #16649:
URL: https://github.com/apache/datafusion/pull/16649
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196417839
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
) -> Result<ParquetMetaData> {
let file_size = meta.size;
let fetch = ObjectStoreFetch::new(store, meta);
- ParquetMetaDataReader::new()
- .with_prefetch_hint(size_hint)
- .with_decryption_properties(decryption_properties)
+ #[allow(unused_mut)]
+ let mut reader = ParquetMetaDataReader::new().with_prefetch_hint(size_hint);
Review Comment:
Good catch. Updated.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196418502
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I'm not sure. I set it to the default so that users can have this feature enabled as per the updated docs.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2180912655
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -350,15 +352,17 @@ impl FileFormat for ParquetFormat {
Some(time_unit) => Some(parse_coerce_int96_string(time_unit.as_str())?),
None => None,
};
- let config_file_decryption_properties = &self.options.crypto.file_decryption;
+ #[cfg(feature = "parquet_encryption")]
Review Comment:
It would be great if there wa ssome way to encapsulate this somehow into a function or something that could be cfg'd rather than sprinking them around
For example, something like
```
#[cfg(..)]
fn add_crypto_properties(...)
```
##########
datafusion/common/src/config.rs:
##########
@@ -29,12 +29,16 @@ use std::error::Error;
use std::fmt::{self, Display};
use std::str::FromStr;
-#[cfg(feature = "parquet")]
+#[cfg(feature = "parquet_encryption")]
Review Comment:
I wonder if we can move all the crypto code into its own module, so we only have to `#cfg` the module rather than so many individual lines 🤔
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2184109462
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -350,15 +352,17 @@ impl FileFormat for ParquetFormat {
Some(time_unit) => Some(parse_coerce_int96_string(time_unit.as_str())?),
None => None,
};
- let config_file_decryption_properties = &self.options.crypto.file_decryption;
+ #[cfg(feature = "parquet_encryption")]
Review Comment:
As mentioned above, I wasn't able to do this entirely, but I have refactored most of it into methods in encryption.rs, which cleans this up further.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196388182
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
Should this really be a default feature? It's still marked experimental in arrow-rs.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2183847859
##########
datafusion/core/src/test/mod.rs:
##########
@@ -38,6 +38,7 @@ use crate::test_util::{aggr_test_schema, arrow_test_data};
use arrow::array::{self, Array, ArrayRef, Decimal128Builder, Int32Array};
use arrow::datatypes::{DataType, Field, Schema};
use arrow::record_batch::RecordBatch;
+#[cfg(feature = "compression")]
use datafusion_common::DataFusionError;
Review Comment:
This is only used by compression. Got warning via
```
cargo check --profile ci --no-default-features -p datafusion --features=parquet_encryption
```
Figured I may as well fix it while I am here.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2206008277
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I think this would make for a good followup PR. Thanks for the helpful notes.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2206231243
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
Review Comment:
One nice effect of this would be that it would allow removing the need to define `FileDecryptionProperties` as an empty struct when the encryption feature is disabled, as I think this is the only place it appears in a public API that isn't an encryption specific method.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2206010727
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
Review Comment:
That would probably be a nice long-term change. My feeling is that this is likely a lower priority since this set of functions seems to be mostly just for internal use.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#issuecomment-3026163921
## Pull Request Overview from Copilot
This PR introduces an optional `parquet_encryption` feature that wires encryption/decryption properties through the Parquet reader and writer layers and updates workspace dependencies.
- Gate encryption/decryption logic behind `parquet_encryption` in `source.rs`, `opener.rs`, and `file_format.rs`
- Expose and stub `FileEncryptionProperties`/`FileDecryptionProperties` types in `common/src/config.rs`
- Add optional `hex` crate and define the `parquet_encryption` feature in multiple `Cargo.toml` files
<details>
<summary>Show a summary per file</summary>
| File | Description |
| ------------------------------------------------------------ | ---------------------------------------------------------------- |
| datafusion/datasource-parquet/src/source.rs | Fallback `file_decryption_properties` when encryption is off |
| datafusion/datasource-parquet/src/opener.rs | Conditionally pass decryption props to `ArrowReaderOptions` |
| datafusion/datasource-parquet/src/file_format.rs | Apply cfg guards around decryption props for schema/statistics |
| datafusion/datasource-parquet/Cargo.toml | Add optional `hex` and define `parquet_encryption` feature |
| datafusion/core/tests/parquet/encryption.rs | Gate round-trip encryption test behind feature flag |
| datafusion/core/src/dataframe/parquet.rs | Gate dataframe encryption test behind feature flag |
| datafusion/core/Cargo.toml | Enable `parquet_encryption` in workspace default features |
| datafusion/common/src/file_options/parquet_writer.rs | Conditionally add encryption props in writer builder |
| datafusion/common/src/config.rs | Publicly expose and stub encryption types based on cfg |
| datafusion/common/Cargo.toml | Add optional `hex` under `parquet_encryption` |
| Cargo.toml | Introduce `hex` dependency (always enabled) |
</details>
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2183972933
##########
datafusion/common/src/config.rs:
##########
@@ -29,12 +29,16 @@ use std::error::Error;
use std::fmt::{self, Display};
use std::str::FromStr;
-#[cfg(feature = "parquet")]
+#[cfg(feature = "parquet_encryption")]
Review Comment:
I wasn't able to do this entirely, but I have refactored most of this into methods in encryption.rs, which cleans this up more.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2202528175
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
Review Comment:
A nicer API over the long term would be to move the arguments of `fetch_parquet_metadata` to a struct, and then have a field in that struct be different if the feature was enabled.
That could also be a different / follow on PR if we wanted to pursue it
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I would personally prefer if it was not part of the default features but given it is already on by default on main, I think it is fine to leave it on by default in this PR
We can discuss disabling the feature by default as a follow on ticket / PR perhaps
Here are some notes I wrote about how to make it a non default feature:
1. Don't add it to the default features
1. Update the docs to mention the config setting requires the `parquet_encryption` feature -- https://github.com/apache/datafusion/blob/d66d6b9eb83a4643bc8d1ee9c2da7f0fa644c0cf/datafusion/common/src/config.rs#L1839
2. Enable it in datafusion-cli (add `parquet_encryption` in https://github.com/apache/datafusion/blob/ca16255e725bf6676a8ddd4b9948496d99b5bd88/datafusion-cli/Cargo.toml#L45-L44)
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#issuecomment-3070584588
Thanks again everyone
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2206008277
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I think this would make for a good followup PR. Thanks for the helpful notes.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2206009134
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I think this would make for a good follow-up PR. Thanks for the helpful notes.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2206050403
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
I've made an issue for this for further discussion: #16777
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#issuecomment-3026209585
@alamb
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#issuecomment-3033798159
> Thanks @corwinjoy ! I have a few suggestions on how to maybe make this code a bit easier to deal with -- let me know what you think
>
> I think this would benefit from:
>
> 1. A test that all the cfg'ing is working by adding to the CI here: https://github.com/apache/datafusion/blob/e063a8d95841aadcb337b99e6c2b4581a811a318/.github/workflows/rust.yml#L204
> 2. Documentation about this rust feature: https://github.com/apache/datafusion/blob/56c1a09bff1eae188486c747404f163353c1bfd4/README.md#L111-L110
Sounds good! I have added this.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196392195
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
Review Comment:
Hmm yeah OK I agree. It would be nice to not need to change the API, but I guess this might be fine here as it's documented as "exposed for low level integrations".
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "corwinjoy (via GitHub)" <gi...@apache.org>.
corwinjoy commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196387219
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -959,14 +953,18 @@ pub async fn fetch_parquet_metadata(
store: &dyn ObjectStore,
meta: &ObjectMeta,
size_hint: Option<usize>,
- decryption_properties: Option<&FileDecryptionProperties>,
+ #[allow(unused)] decryption_properties: Option<&FileDecryptionProperties>,
Review Comment:
I don't really think so. I don't like the idea of the API changing depending on the cfg setting.
--
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: Add a configuration to make parquet encryption optional [datafusion]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #16649:
URL: https://github.com/apache/datafusion/pull/16649#discussion_r2196420000
##########
datafusion/core/Cargo.toml:
##########
@@ -61,13 +61,21 @@ default = [
"unicode_expressions",
"compression",
"parquet",
+ "parquet_encryption",
Review Comment:
Probably a question for @alamb
--
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