-
Notifications
You must be signed in to change notification settings - Fork 924
Move Parquet encryption tests into the arrow_reader integration tests #7279
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
2a0c109
to
9ac075b
Compare
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.
Looks good!
parquet/tests/arrow_reader/mod.rs
Outdated
mod encryption; | ||
#[cfg(all(feature = "encryption", feature = "async"))] | ||
mod encryption_async; | ||
mod encryption_common; |
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.
Nit: how about read_encrypted_with_plaintext_footer
or encryption_agnostic
? encryption_common
slightly implies some sort of shared utility and made me wonder why it's not behind #[cfg(feature = "encryption")]
. Not a big deal, feel free to ignore.
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.
Yeah encryption_agnostic
sounds better, I couldn't think of a good name for this.
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 -- this is great (and thank you @rok for the ping)
I also double checked that the tests are run as part of CI: https://github.com/apache/arrow-rs/actions/runs/13826035766/job/38681041589?pr=7279
assert_eq!(file_metadata.num_rows(), 50); | ||
assert_eq!(file_metadata.schema_descr().num_columns(), 8); | ||
|
||
metadata.metadata.row_groups().iter().for_each(|rg| { | ||
metadata.row_groups().iter().for_each(|rg| { |
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.
that is a nice change
Since these are tests only changes I am going to merge it in to avoid conflicts and get us ready for the next round |
Thanks agian! |
…apache#7279) * Move encryption tests to arrow_reader tests * Move object store tests and simplify verification function * Test reading plaintext footer files with encryption both disabled and enabled * Rename encryption_common module to encryption_agnostic
Rationale for this change
See #6637 (comment)
This ensures that the tests are built with the same visibility restrictions as user programs, and reduces the need for
#[cfg(feature = "encryption")]
on all tests.What changes are included in this PR?
parquet/tests/arrow_reader
verify_encryption_test_data
function for the test that usesobject_store
.Are there any user-facing changes?
No