adamreeve commented on code in PR #6637:
URL: https://github.com/apache/arrow-rs/pull/6637#discussion_r1986431798
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -175,6 +227,15 @@ impl ArrowReaderMetadata {
) -> Result<Self> {
// TODO: this is all rather awkward. It would be nice if
AsyncFileReader::get_metadata
// took an argument to fetch the page indexes.
+ #[cfg(feature = "encryption")]
+ let mut metadata = if options.file_decryption_properties.is_some() {
Review Comment:
With these recent changes it doesn't look like we properly handle when a
file is encrypted but no decryption properties are provided. Here we check that
file decryption properties are set before calling `get_encrypted_metadata`, and
then later in `ParquetMetadataReader::decrypt_metadata` we raise an error if
the footer is encrypted but no decryption properties are set
(https://github.com/apache/arrow-rs/blob/276fc1a153c7a64bdf50401d007224b12ee1ddd8/parquet/src/file/metadata/reader.rs#L730).
The code here also seems to have the same problem:
https://github.com/apache/arrow-rs/blob/276fc1a153c7a64bdf50401d007224b12ee1ddd8/parquet/src/file/metadata/reader.rs#L577
I think we can just remove the `if
options.file_decryption_properties.is_some()` check, and maybe rename
`get_encrypted_metadata` etc to indicate that the metadata isn't necessarily
encrypted. (maybe `get_metadata_with_encryption`?). Plus
`ParquetMetaDataReader::decrypt_metadata` also seems misnamed. There are also
some documentation comments that will need to be fixed to indicate that these
methods aren't only for encrypted files, but can handle encrypted files.
Or alternatively we could move the check for an encrypted footer higher up
and simplify some of the lower down methods to not handle the un-encrypted case?
Can we fix that and add a test case? And also a test case for a file with an
encrypted footer that's run when encryption is disabled to check we get this
error:
https://github.com/apache/arrow-rs/blob/276fc1a153c7a64bdf50401d007224b12ee1ddd8/parquet/src/file/metadata/reader.rs#L571
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
