Encapsulate encryption code more in readers #7337
Conversation
alamb
force-pushed
the
alamb/encapsulate_encryption
branch
from
0a5e90f to
c1dbf29
Compare
| @@ -698,32 +696,25 @@ impl<T: ChunkReader + 'static> Iterator for ReaderPageIterator<T> { | |||
| let total_rows = rg.num_rows() as usize; | |||
| let reader = self.reader.clone(); | |||
|
|
|||
| #[cfg(feature = "encryption")] | |||
There was a problem hiding this comment.
this code I found especially hard to work with, so I was able to move it into a method called add_crypto_context and then introduce a stub that doesn't do anything by default
|
|
||
| fn next(&mut self) -> Option<Self::Item> { | ||
| let rg_idx = self.row_groups.next()?; | ||
| impl<T: ChunkReader + 'static> ReaderPageIterator<T> { |
There was a problem hiding this comment.
I moved the code that gets the next reader into its own function that returns Result<> so that I could use ? to check for errors. In the Iterator implementation, returning Option<Result<..>> meant it was akward to return the errors
|
|
||
| #[cfg(feature = "encryption")] | ||
| let page_reader = page_reader.with_crypto_context(crypto_context); | ||
| let page_reader = page_reader.add_crypto_context( |
There was a problem hiding this comment.
same thing here, I removed the duplicated setup of the CryptoContext and put it in a method on the page reader
| @@ -571,22 +571,51 @@ impl<R: ChunkReader> SerializedPageReader<R> { | |||
| /// Creates a new serialized page reader from a chunk reader and metadata | |||
| pub fn new( | |||
| reader: Arc<R>, | |||
| meta: &ColumnChunkMetaData, | |||
| column_chunk_metadata: &ColumnChunkMetaData, | |||
There was a problem hiding this comment.
I renamed this variable for clarity (there are too many types of metadata floating around -- at least ParquetMetaData and ColumnChunkMetadata)
There was a problem hiding this comment.
Pull Request Overview
This PR encapsulates the encryption context setup within the SerializedPageReader to reduce code duplication and improve maintainability for encryption-related code. Key changes include:
- Renaming the metadata parameter from "meta" to "column_chunk_metadata" for clarity.
- Consolidating the encryption context logic into a single method named add_crypto_context with conditional compilation.
- Updating references to the metadata parameter across asynchronous and arrow reader modules.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| parquet/src/file/serialized_reader.rs | Refactored functions to set up encryption context and renamed parameter. |
| parquet/src/arrow/async_reader/mod.rs | Updated page reader construction and streamlined encryption context setup. |
| parquet/src/arrow/arrow_reader/mod.rs | Adjusted metadata reference and removed unused encryption imports. |
|
Thank you for the review @etseidl and @adamreeve |
Which issue does this PR close?
Rationale for this change
While trying to get the filter pushdown code in #6921 working, I keep hitting merge conflicts with encryption changes related to
#cfg[encryption]Also I had an idea described in #7111 (comment) but I don't think I did a good job explaining it and it didn't seem to work for @adamreeve (see #7111 (comment))
Thus I would like to propose a pattern of how to reduce some duplication and keep the encryption related code out of the main deocder logic
What changes are included in this PR?
Move adding
CryptoContextinto a method onSerializedPageReaderAre there any user-facing changes?
No, this is all internal code reorganization