-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Make parquet_encryption a non-default feature #17137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CI failure tracked by |
|
Updating to get clean CI run |
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me -- thank you @miroim
I think we need to document the new config as well as add a new CI test to make sure the crate works without the feature. For example:
datafusion/.github/workflows/rust.yml
Lines 207 to 208 in 5c370fa
| - name: Check datafusion (parquet) | |
| run: cargo check --profile ci --no-default-features -p datafusion --features=parquet |
And compile datafusion with the parquet and parquet_encryption features.
cc @adamreeve and @corwinjoy I would be interested to hear your opinion on this change
|
This change looks reasonable to me. |
|
👍 makes sense to me too. There is already a datafusion/.github/workflows/rust.yml Line 222 in 5c370fa
I think we need to enable datafusion/.github/workflows/rust.yml Line 297 in 5c370fa
|
|
Should we also enable datafusion/.github/workflows/rust.yml Line 558 in 5c370fa
|
I don't have any strong opinions -- I don't think it is necessary |
I agree this is needed to ensure the feature keeps working We also need to check that the flags work in isolation as described in #17137 (review) |
.github/workflows/rust.yml
Outdated
| - name: Check parquet encryption (parquet, parquet_encryption) | ||
| run: cargo check --profile ci --no-default-features -p datafusion --features=parquet,parquet_encryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be redundant but then I noticed that the parquet_encryption feature doesn't actually depend on the parquet feature. Maybe we should remove this extra check run as the one above already tests the parquet_encryption feature, and update datafusion/core/Cargo.toml so that the parquet_encryption feature depends on the parquet feature rather than just dep:parquet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @adamreeve for review
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you again @miroim and @adamreeve
|
I merged up from main and minimized this diff for clarity about what actually changed. |
|
Thanks again @miroim |
Which issue does this PR close?
Rationale for this change
as discussion from @alamb on: #16649 (comment)
What changes are included in this PR?
parquet_encryptionfeature from the default features.ParquetEncryptionOptionsexplaining that theparquet_encryptionfeature flag must be enabled to use Parquet encryption.parquet_encryptionfeature in thedatafusion-cli.parquet_encryptionfeature in thedatafusion/sqllogictest.parquet_encryptionfeature in thedatafusion-examples.Are these changes tested?
doc test
Are there any user-facing changes?
Change
parquet_encryptionfeature from the default feature to non-default