alamb commented on code in PR #6637:
URL: https://github.com/apache/arrow-rs/pull/6637#discussion_r1987941721
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -175,6 +234,12 @@ 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 = input
+
.get_metadata_with_encryption(options.file_decryption_properties.clone())
+ .await?;
Review Comment:
I agree this is all akward -- I have a suggestion on how to simplify it above
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -853,6 +929,10 @@ struct InMemoryRowGroup<'a> {
offset_index: Option<&'a [OffsetIndexMetaData]>,
column_chunks: Vec<Option<Arc<ColumnChunkData>>>,
row_count: usize,
+ #[cfg(feature = "encryption")]
+ row_group_ordinal: usize,
Review Comment:
Minor: I think `row_group_idx` would be a name that is more consistent with
the rest of this crate (and it is used as the local variable name above too)
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -0,0 +1,260 @@
+// 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.
+
+use crate::encryption::ciphers::{BlockDecryptor, RingGcmBlockDecryptor};
+use crate::encryption::modules::{create_module_aad, ModuleType};
+use crate::errors::{ParquetError, Result};
+use std::collections::HashMap;
+use std::io::Read;
+use std::sync::Arc;
+
+pub fn read_and_decrypt<T: Read>(
+ decryptor: &Arc<dyn BlockDecryptor>,
+ input: &mut T,
+ aad: &[u8],
+) -> Result<Vec<u8>> {
+ let mut len_bytes = [0; 4];
+ input.read_exact(&mut len_bytes)?;
+ let ciphertext_len = u32::from_le_bytes(len_bytes) as usize;
+ let mut ciphertext = vec![0; 4 + ciphertext_len];
+ input.read_exact(&mut ciphertext[4..])?;
+
+ decryptor.decrypt(&ciphertext, aad.as_ref())
+}
+
+// CryptoContext is a data structure that holds the context required to
+// decrypt parquet modules (data pages, dictionary pages, etc.).
+#[derive(Debug, Clone)]
+pub(crate) struct CryptoContext {
+ pub(crate) row_group_ordinal: usize,
Review Comment:
stylistically, as before I think using `idx` or `index` for row group and
column group would be more consistent with the existing code but it is not a
major item
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -104,6 +107,14 @@ pub trait AsyncFileReader: Send {
/// allowing fine-grained control over how metadata is sourced, in
particular allowing
/// for caching, pre-fetching, catalog metadata, etc...
fn get_metadata(&mut self) -> BoxFuture<'_, Result<Arc<ParquetMetaData>>>;
+
+ /// Provides asynchronous access to the [`ParquetMetaData`] of encrypted
parquet
+ /// files, like get_metadata does for unencrypted ones.
+ #[cfg(feature = "encryption")]
+ fn get_metadata_with_encryption(
+ &mut self,
+ file_decryption_properties: Option<FileDecryptionProperties>,
Review Comment:
I don't understand this new API and it seems overly specific to the
encryption feature.
It seems like the need is to pass `file_decryption_properties` to the
underlying `ParquetMetadataReader` but there are other potential options on
`ArrowReaderOptions`that might be useful.
What if instead we added a new method like this:
```rust
/// Provides asynchronous access to the [`ParquetMetaData`] of a parquet
file,
/// allowing fine-grained control over how metadata is sourced, in
particular allowing
/// for caching, pre-fetching, catalog metadata, etc...
///
/// By default calls `get_metadata()`
fn get_metadata_with_options(&mut self, options: &ArrowReaderOptions) ->
BoxFuture<'_, Result<Arc<ParquetMetaData>>> {
self.get_metadata()
}
```
I think that would
1. Be backwards compatible
2. Permit access to the encryption options necessary to decrype the data
3. Allow us eventually to clean up `ArrowReaderMetadata::load_async`
function (in some other PR)
As a follow on PR we could potentially deprecate `get_metadata` and direct
people to implement `get_metadata_with_options`
##########
parquet/src/arrow/async_reader/store.rs:
##########
@@ -175,6 +177,24 @@ impl AsyncFileReader for ParquetObjectReader {
Ok(Arc::new(metadata))
})
}
+
+ #[cfg(feature = "encryption")]
+ fn get_metadata_with_encryption(
+ &mut self,
+ _file_decryption_properties: Option<FileDecryptionProperties>,
+ ) -> BoxFuture<'_, Result<Arc<ParquetMetaData>>> {
+ Box::pin(async move {
Review Comment:
this implementation doesn't seem to use the `_file_decryption_properties` at
all 🤔
So I think that means that anything reading Parquet files from an
`ObjectStore` implementation wouldn't be able to read encrypted ones
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -853,6 +929,10 @@ struct InMemoryRowGroup<'a> {
offset_index: Option<&'a [OffsetIndexMetaData]>,
column_chunks: Vec<Option<Arc<ColumnChunkData>>>,
row_count: usize,
+ #[cfg(feature = "encryption")]
+ row_group_ordinal: usize,
+ #[cfg(feature = "encryption")]
+ parquet_metadata: Arc<ParquetMetaData>,
Review Comment:
As a way to remove some #cfg
You could potentially remove `metadata` from this struct and instead store
just
```rust
row_group_idx: usize,
metadata: &'a ParquetMetaData,
```
And `metadata` is the equivalent of
```rust
self.metadata.row_group(self.row_group_idx);
```
--
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
