You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/09/12 19:46:54 UTC
[PR] Alamb/refactor push decoder [arrow-rs]
alamb opened a new pull request, #8340:
URL: https://github.com/apache/arrow-rs/pull/8340
# Which issue does this PR close?
- part of #8000
- Follow on to https://github.com/apache/arrow-rs/pull/8080
# Rationale for this change
The current ParquetMetadataDecoder intermixes three things:
1. The state machine for decoding parquet metadata (footer, then metadata, then (optional) indexes)
2. orchestrating IO (aka calling read, etc)
3. Decoding thrift encoded byte into objets
This makes it almost impossible to add features like "only decode a subset of the columns in the ColumnIndex" and other potentially advanced usecases
Now that we have a "push" style API for metadata decoding that avoids IO, the next step is to extract out the actual work into this API so that the existing ParquetMetadataDecoder just calls into the PushDecoder
# What changes are included in this PR?
1. Extract decoding state machine into PushMetadataDecoder
2. Update ParquetMetadataDecoder to use the PushMetadataDecoder
3. Extract the bytes --> object code into its own module
This almost certainly will conflict with @etseidl 's plans in thrift-remodel.
# Are these changes tested?
by existing tests
# Are there any user-facing changes?
Not really -- this is an internal change that will make it easier to add features like "only decode a subset of the columns in the ColumnIndex, for example
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3330155120
Ok, I am now pretty happy with this PR and how it looks. I broke it up into a few PRs to make reviews easier
- https://github.com/apache/arrow-rs/pull/8437
- https://github.com/apache/arrow-rs/pull/8436
You can see the results in this PR as the last commit
If/when those PRs are merged I'll rebase this one and mark it as ready for review
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3348769159
> 🤖: Benchmark completed
🤔 [those benchmark](https://github.com/apache/arrow-rs/pull/8340#issuecomment-3346335136) results look really bad. I will investigate
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3339061565
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group alamb_refactor_push_decoder main
----- --------------------------- ----
decode parquet metadata 1.00 24.5±0.04µs ? ?/sec 1.09 26.7±0.10µs ? ?/sec
decode parquet metadata (wide) 1.00 157.9±7.66ms ? ?/sec 1.03 162.1±3.69ms ? ?/sec
decode thrift file metadata 1.00 16.7±0.12µs ? ?/sec 1.06 17.6±0.13µs ? ?/sec
decode thrift file metadata (wide) 1.00 106.1±0.32ms ? ?/sec 1.04 110.3±0.32ms ? ?/sec
open(default) 1.00 23.9±0.05µs ? ?/sec 1.02 24.3±0.13µs ? ?/sec
open(page index) 1.02 1296.4±4.15µs ? ?/sec 1.00 1272.8±1.78µs ? ?/sec
page headers 1.00 7.5±0.03µs ? ?/sec 1.03 7.7±0.01µs ? ?/sec
```
</p>
</details>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3351101161
> 🤖: Benchmark completed
😅 that looks much better. Let's do this!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2365583592
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -261,68 +269,152 @@ impl ParquetMetaDataPushDecoder {
&mut self,
ranges: Vec<std::ops::Range<u64>>,
buffers: Vec<bytes::Bytes>,
- ) -> std::result::Result<(), String> {
- if self.done {
- return Err(
+ ) -> Result<(), ParquetError> {
+ if matches!(&self.state, DecodeState::Finished) {
+ return Err(general_err!(
"ParquetMetaDataPushDecoder: cannot push data after decoding is finished"
- .to_string(),
- );
+ ));
}
self.buffers.push_ranges(ranges, buffers);
Ok(())
}
/// Try to decode the metadata from the pushed data, returning the
/// decoded metadata or an error if not enough data is available.
- pub fn try_decode(
- &mut self,
- ) -> std::result::Result<DecodeResult<ParquetMetaData>, ParquetError> {
- if self.done {
- return Ok(DecodeResult::Finished);
- }
-
- // need to have the last 8 bytes of the file to decode the metadata
+ pub fn try_decode(&mut self) -> Result<DecodeResult<ParquetMetaData>, ParquetError> {
let file_len = self.buffers.file_len();
- if !self.buffers.has_range(&(file_len - 8..file_len)) {
- #[expect(clippy::single_range_in_vec_init)]
- return Ok(DecodeResult::NeedsData(vec![file_len - 8..file_len]));
+ let footer_len = FOOTER_SIZE as u64;
+ loop {
+ match std::mem::replace(&mut self.state, DecodeState::Intermediate) {
Review Comment:
Here is the core state machine that makes it very clear, in my mind, what is happening.
I am quite pleased with how this decoder state machine is looking
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2383112611
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -43,6 +43,86 @@ use crate::encryption::{
#[cfg(feature = "encryption")]
use crate::format::EncryptionAlgorithm;
+/// Helper struct for metadata parsing
+///
+/// This structure parses thrift-encoded bytes into the correct Rust structs,
+/// such as [`ParquetMetaData`], handling decryption if necessary.
+//
+// Note this structure is used to minimize the number of
+// places need to add `#[cfg(feature = "encryption")]` checks.
+pub(crate) use inner::MetadataParser;
+
+#[cfg(feature = "encryption")]
+mod inner {
+ use super::*;
+ use crate::encryption::decrypt::FileDecryptionProperties;
+ use crate::errors::Result;
+
+ /// API for decoding metadata that may be encrypted
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser {
+ // the credentials and keys needed to decrypt metadata
+ file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ }
+
+ impl MetadataParser {
+ pub(crate) fn new() -> Self {
+ MetadataParser::default()
+ }
+
+ pub(crate) fn with_file_decryption_properties(
+ mut self,
+ file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ ) -> Self {
+ self.file_decryption_properties = file_decryption_properties;
+ self
+ }
+
+ pub(crate) fn decode_metadata(
+ &self,
+ buf: &[u8],
+ encrypted_footer: bool,
+ ) -> Result<ParquetMetaData> {
+ decode_metadata_with_encryption(
+ buf,
+ encrypted_footer,
+ self.file_decryption_properties.as_deref(),
+ )
+ }
+ }
+}
+
+#[cfg(not(feature = "encryption"))]
+mod inner {
+ use super::*;
+ use crate::errors::Result;
+ /// parallel implementation when encryption feature is not enabled
+ ///
+ /// This has the same API as the encryption-enabled version
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser;
+
+ impl MetadataParser {
+ pub(crate) fn new() -> Self {
+ MetadataParser
+ }
+
+ pub(crate) fn decode_metadata(
+ &self,
+ buf: &[u8],
+ encrypted_footer: bool,
+ ) -> Result<ParquetMetaData> {
+ if encrypted_footer {
+ Err(general_err!(
+ "Parquet file has an encrypted footer but the encryption feature is disabled"
+ ))
+ } else {
+ decode_metadata(buf)
Review Comment:
I don't fully understand your description of the problem. Do you mean you inlined the contents of decode_metadata or something?
Is there anything I can do to make the pattern more amenable to the thrift-remodel branch?
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -211,16 +232,38 @@ impl ParquetMetaDataPushDecoder {
)));
};
- let metadata_reader =
- ParquetMetaDataReader::new().with_page_index_policy(PageIndexPolicy::Optional);
-
Ok(Self {
- done: false,
- metadata_reader,
+ state: DecodeState::ReadingFooter,
+ column_index_policy: PageIndexPolicy::Optional,
+ offset_index_policy: PageIndexPolicy::Optional,
buffers: crate::util::push_buffers::PushBuffers::new(file_len),
+ metadata_parser: MetadataParser::new(),
})
}
+ /// Begin decoding from the given footer tail.
+ pub(crate) fn try_new_with_footer_tail(
+ file_len: u64,
+ footer_tail: FooterTail,
+ ) -> Result<Self, ParquetError> {
Review Comment:
Oh, now I see what you are asking -- I will update this to use crate::errors::Result (which is what I thought I had used)!
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -259,70 +325,160 @@ impl ParquetMetaDataPushDecoder {
/// example on [`Self`]
pub fn push_ranges(
&mut self,
- ranges: Vec<std::ops::Range<u64>>,
- buffers: Vec<bytes::Bytes>,
- ) -> std::result::Result<(), String> {
- if self.done {
- return Err(
+ ranges: Vec<Range<u64>>,
+ buffers: Vec<Bytes>,
+ ) -> Result<(), ParquetError> {
+ if matches!(&self.state, DecodeState::Finished) {
+ return Err(general_err!(
"ParquetMetaDataPushDecoder: cannot push data after decoding is finished"
- .to_string(),
- );
+ ));
}
self.buffers.push_ranges(ranges, buffers);
Ok(())
}
- /// Try to decode the metadata from the pushed data, returning the
- /// decoded metadata or an error if not enough data is available.
- pub fn try_decode(
- &mut self,
- ) -> std::result::Result<DecodeResult<ParquetMetaData>, ParquetError> {
- if self.done {
- return Ok(DecodeResult::Finished);
+ /// Pushes a single range of data into the decoder's buffer.
+ pub fn push_range(&mut self, range: Range<u64>, buffer: Bytes) -> Result<(), ParquetError> {
+ if matches!(&self.state, DecodeState::Finished) {
+ return Err(general_err!(
+ "ParquetMetaDataPushDecoder: cannot push data after decoding is finished"
+ ));
}
+ self.buffers.push_range(range, buffer);
+ Ok(())
+ }
- // need to have the last 8 bytes of the file to decode the metadata
+ /// Try to decode the metadata from the pushed data, returning the
+ /// decoded metadata or an error if not enough data is available.
+ pub fn try_decode(&mut self) -> Result<DecodeResult<ParquetMetaData>, ParquetError> {
let file_len = self.buffers.file_len();
- if !self.buffers.has_range(&(file_len - 8..file_len)) {
- #[expect(clippy::single_range_in_vec_init)]
- return Ok(DecodeResult::NeedsData(vec![file_len - 8..file_len]));
+ let footer_len = FOOTER_SIZE as u64;
+ loop {
+ match std::mem::replace(&mut self.state, DecodeState::Intermediate) {
+ DecodeState::ReadingFooter => {
+ // need to have the last 8 bytes of the file to decode the metadata
+ let footer_start = file_len.saturating_sub(footer_len);
+ let footer_range = footer_start..file_len;
+
+ if !self.buffers.has_range(&footer_range) {
+ self.state = DecodeState::ReadingFooter;
+ return Ok(needs_range(footer_range));
+ }
+ let footer_bytes = self.get_bytes(&footer_range)?;
+ let footer_tail = FooterTail::try_from(footer_bytes.as_ref())?;
+
+ self.state = DecodeState::ReadingMetadata(footer_tail);
+ continue;
+ }
+
+ DecodeState::ReadingMetadata(footer_tail) => {
+ let metadata_len: u64 = footer_tail.metadata_length() as u64;
+ let metadata_start = file_len - footer_len - metadata_len;
+ let metadata_end = metadata_start + metadata_len;
+ let metadata_range = metadata_start..metadata_end;
+
+ if !self.buffers.has_range(&metadata_range) {
+ self.state = DecodeState::ReadingMetadata(footer_tail);
+ return Ok(needs_range(metadata_range));
+ }
+
+ let metadata = self.metadata_parser.decode_metadata(
+ &self.get_bytes(&metadata_range)?,
+ footer_tail.is_encrypted_footer(),
+ )?;
+ self.state = DecodeState::ReadingPageIndex(Box::new(metadata));
+ continue;
+ }
+
+ DecodeState::ReadingPageIndex(mut metadata) => {
Review Comment:
Good call -- I tried to clarify in 12bca808bf.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3339016198
This PR is now ready for review
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3351101620
Thanks again @etseidl
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3346335136
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing alamb/refactor_push_decoder (12bca808bfc0eafb419f7b753f083324fdcfaa61) to 4d184015d7ec2b76d1b3c7a34305da9b9d2d1d15 [diff](https://github.com/apache/arrow-rs/compare/4d184015d7ec2b76d1b3c7a34305da9b9d2d1d15..12bca808bfc0eafb419f7b753f083324fdcfaa61)
BENCH_NAME=metadata
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench metadata
BENCH_FILTER=
BENCH_BRANCH_NAME=alamb_refactor_push_decoder
Results will be posted here when complete
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3286702622
> > This almost certainly will conflict with @etseidl 's plans in thrift-remodel.
>
> I took a quick peak and I think it won't be too hard to merge this into what I'm doing. It will likely help if I can merge this without any other changes from main to contend with. We can coordinate that pas de deux when the time comes 😅
Cool -- I likewise don't think it will conflict too badly logically, though I think it may generate lots of textual diffs that we'll have to be careful with.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3339047617
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing alamb/refactor_push_decoder (fdc9f3f28b9d7bb44a85ad68f7d457c0a9daf928) to 4d184015d7ec2b76d1b3c7a34305da9b9d2d1d15 [diff](https://github.com/apache/arrow-rs/compare/4d184015d7ec2b76d1b3c7a34305da9b9d2d1d15..fdc9f3f28b9d7bb44a85ad68f7d457c0a9daf928)
BENCH_NAME=metadata
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench metadata
BENCH_FILTER=
BENCH_BRANCH_NAME=alamb_refactor_push_decoder
Results will be posted here when complete
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3346357081
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group alamb_refactor_push_decoder main
----- --------------------------- ----
decode parquet metadata 1.53 40.3±5.80µs ? ?/sec 1.00 26.2±0.08µs ? ?/sec
decode parquet metadata (wide) 1.47 263.9±9.26ms ? ?/sec 1.00 179.2±39.86ms ? ?/sec
decode thrift file metadata 1.63 27.8±1.42µs ? ?/sec 1.00 17.1±0.10µs ? ?/sec
decode thrift file metadata (wide) 1.14 131.8±30.54ms ? ?/sec 1.00 115.5±20.20ms ? ?/sec
open(default) 1.71 40.7±2.89µs ? ?/sec 1.00 23.8±0.13µs ? ?/sec
open(page index) 1.21 1969.7±93.64µs ? ?/sec 1.00 1623.4±353.49µs ? ?/sec
page headers 1.00 7.4±0.24µs ? ?/sec 1.15 8.6±1.56µs ? ?/sec
```
</p>
</details>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2373568434
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,467 @@
+// 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.
+
+//! Internal metadata parsing routines
+//!
+//! In general these functions parse thrift-encoded metadata from a byte slice
+//! into the corresponding Rust structures
+
+use crate::basic::ColumnOrder;
+use crate::errors::ParquetError;
+use crate::file::metadata::{
+ ColumnChunkMetaData, FileMetaData, PageIndexPolicy, ParquetMetaData, RowGroupMetaData,
+};
+use crate::file::page_index::index::Index;
+use crate::file::page_index::index_reader::{decode_column_index, decode_offset_index};
+use crate::file::page_index::offset_index::OffsetIndexMetaData;
+use crate::schema::types;
+use crate::schema::types::SchemaDescriptor;
+use crate::thrift::TCompactSliceInputProtocol;
+use crate::thrift::TSerializable;
+use bytes::Bytes;
+use std::sync::Arc;
+
+#[cfg(feature = "encryption")]
+use crate::encryption::{
+ decrypt::{FileDecryptionProperties, FileDecryptor},
+ modules::create_footer_aad,
+};
+#[cfg(feature = "encryption")]
+use crate::format::EncryptionAlgorithm;
+
+/// Decodes [`ParquetMetaData`] from the provided bytes.
+///
+/// Typically this is used to decode the metadata from the end of a parquet
+/// file. The format of `buf` is the Thrift compact binary protocol, as specified
+/// by the [Parquet Spec].
+///
+/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
+pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaData> {
+ let mut prot = TCompactSliceInputProtocol::new(buf);
+
+ let t_file_metadata: crate::format::FileMetaData =
+ crate::format::FileMetaData::read_from_in_protocol(&mut prot)
+ .map_err(|e| general_err!("Could not parse metadata: {}", e))?;
+ let schema = types::from_thrift(&t_file_metadata.schema)?;
+ let schema_descr = Arc::new(SchemaDescriptor::new(schema));
+
+ let mut row_groups = Vec::new();
+ for rg in t_file_metadata.row_groups {
+ row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
+ }
+ let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr)?;
+
+ let file_metadata = FileMetaData::new(
+ t_file_metadata.version,
+ t_file_metadata.num_rows,
+ t_file_metadata.created_by,
+ t_file_metadata.key_value_metadata,
+ schema_descr,
+ column_orders,
+ );
+
+ Ok(ParquetMetaData::new(file_metadata, row_groups))
+}
+
+/// Parses column orders from Thrift definition.
+/// If no column orders are defined, returns `None`.
+pub(crate) fn parse_column_orders(
+ t_column_orders: Option<Vec<crate::format::ColumnOrder>>,
+ schema_descr: &SchemaDescriptor,
+) -> crate::errors::Result<Option<Vec<ColumnOrder>>> {
+ match t_column_orders {
+ Some(orders) => {
+ // Should always be the case
+ if orders.len() != schema_descr.num_columns() {
+ return Err(general_err!("Column order length mismatch"));
+ };
+ let mut res = Vec::new();
+ for (i, column) in schema_descr.columns().iter().enumerate() {
+ match orders[i] {
+ crate::format::ColumnOrder::TYPEORDER(_) => {
+ let sort_order = ColumnOrder::get_sort_order(
+ column.logical_type(),
+ column.converted_type(),
+ column.physical_type(),
+ );
+ res.push(ColumnOrder::TYPE_DEFINED_ORDER(sort_order));
+ }
+ }
+ }
+ Ok(Some(res))
+ }
+ None => Ok(None),
+ }
+}
+
+pub(crate) fn parse_column_index(
Review Comment:
Somewhat parallel, but they for the most part converge eventually (except for the main `ParquetMetaData` parser).
Do you think encryption support will always be feature gated? Or will there eventually be just the one path? I wouldn't want to get too fancy if the latter.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2383143528
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -43,6 +43,86 @@ use crate::encryption::{
#[cfg(feature = "encryption")]
use crate::format::EncryptionAlgorithm;
+/// Helper struct for metadata parsing
+///
+/// This structure parses thrift-encoded bytes into the correct Rust structs,
+/// such as [`ParquetMetaData`], handling decryption if necessary.
+//
+// Note this structure is used to minimize the number of
+// places need to add `#[cfg(feature = "encryption")]` checks.
+pub(crate) use inner::MetadataParser;
+
+#[cfg(feature = "encryption")]
+mod inner {
+ use super::*;
+ use crate::encryption::decrypt::FileDecryptionProperties;
+ use crate::errors::Result;
+
+ /// API for decoding metadata that may be encrypted
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser {
+ // the credentials and keys needed to decrypt metadata
+ file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ }
+
+ impl MetadataParser {
+ pub(crate) fn new() -> Self {
+ MetadataParser::default()
+ }
+
+ pub(crate) fn with_file_decryption_properties(
+ mut self,
+ file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ ) -> Self {
+ self.file_decryption_properties = file_decryption_properties;
+ self
+ }
+
+ pub(crate) fn decode_metadata(
+ &self,
+ buf: &[u8],
+ encrypted_footer: bool,
+ ) -> Result<ParquetMetaData> {
+ decode_metadata_with_encryption(
+ buf,
+ encrypted_footer,
+ self.file_decryption_properties.as_deref(),
+ )
+ }
+ }
+}
+
+#[cfg(not(feature = "encryption"))]
+mod inner {
+ use super::*;
+ use crate::errors::Result;
+ /// parallel implementation when encryption feature is not enabled
+ ///
+ /// This has the same API as the encryption-enabled version
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser;
+
+ impl MetadataParser {
+ pub(crate) fn new() -> Self {
+ MetadataParser
+ }
+
+ pub(crate) fn decode_metadata(
+ &self,
+ buf: &[u8],
+ encrypted_footer: bool,
+ ) -> Result<ParquetMetaData> {
+ if encrypted_footer {
+ Err(general_err!(
+ "Parquet file has an encrypted footer but the encryption feature is disabled"
+ ))
+ } else {
+ decode_metadata(buf)
Review Comment:
Sorry, this was mostly a note to myself. When I did the merge I changed the `decode_metadata` call to
```rust
let mut prot = ThriftSliceInputProtocol::new(buf);
ParquetMetaData::read_thrift(&mut prot)
```
Instead I should do the same in `parser::decode_metadata`.
No changes on your end are necessary 😄
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2383107089
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -211,16 +232,38 @@ impl ParquetMetaDataPushDecoder {
)));
};
- let metadata_reader =
- ParquetMetaDataReader::new().with_page_index_policy(PageIndexPolicy::Optional);
-
Ok(Self {
- done: false,
- metadata_reader,
+ state: DecodeState::ReadingFooter,
+ column_index_policy: PageIndexPolicy::Optional,
+ offset_index_policy: PageIndexPolicy::Optional,
buffers: crate::util::push_buffers::PushBuffers::new(file_len),
+ metadata_parser: MetadataParser::new(),
})
}
+ /// Begin decoding from the given footer tail.
+ pub(crate) fn try_new_with_footer_tail(
+ file_len: u64,
+ footer_tail: FooterTail,
+ ) -> Result<Self, ParquetError> {
Review Comment:
I don't really have a preference, I was just trying to keep the code consistent -- I think (??) this crate's convention was to typedef the `Result` that has a specific error, but now that I am typing this, maybe I am getting confused with other crates 🤔 😬
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2349982873
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,467 @@
+// 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.
+
+//! Internal metadata parsing routines
+//!
+//! In general these functions parse thrift-encoded metadata from a byte slice
+//! into the corresponding Rust structures
+
+use crate::basic::ColumnOrder;
+use crate::errors::ParquetError;
+use crate::file::metadata::{
+ ColumnChunkMetaData, FileMetaData, PageIndexPolicy, ParquetMetaData, RowGroupMetaData,
+};
+use crate::file::page_index::index::Index;
+use crate::file::page_index::index_reader::{decode_column_index, decode_offset_index};
+use crate::file::page_index::offset_index::OffsetIndexMetaData;
+use crate::schema::types;
+use crate::schema::types::SchemaDescriptor;
+use crate::thrift::TCompactSliceInputProtocol;
+use crate::thrift::TSerializable;
+use bytes::Bytes;
+use std::sync::Arc;
+
+#[cfg(feature = "encryption")]
+use crate::encryption::{
+ decrypt::{FileDecryptionProperties, FileDecryptor},
+ modules::create_footer_aad,
+};
+#[cfg(feature = "encryption")]
+use crate::format::EncryptionAlgorithm;
+
+/// Decodes [`ParquetMetaData`] from the provided bytes.
+///
+/// Typically this is used to decode the metadata from the end of a parquet
+/// file. The format of `buf` is the Thrift compact binary protocol, as specified
+/// by the [Parquet Spec].
+///
+/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
+pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaData> {
+ let mut prot = TCompactSliceInputProtocol::new(buf);
+
+ let t_file_metadata: crate::format::FileMetaData =
+ crate::format::FileMetaData::read_from_in_protocol(&mut prot)
+ .map_err(|e| general_err!("Could not parse metadata: {}", e))?;
+ let schema = types::from_thrift(&t_file_metadata.schema)?;
+ let schema_descr = Arc::new(SchemaDescriptor::new(schema));
+
+ let mut row_groups = Vec::new();
+ for rg in t_file_metadata.row_groups {
+ row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
+ }
+ let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr)?;
+
+ let file_metadata = FileMetaData::new(
+ t_file_metadata.version,
+ t_file_metadata.num_rows,
+ t_file_metadata.created_by,
+ t_file_metadata.key_value_metadata,
+ schema_descr,
+ column_orders,
+ );
+
+ Ok(ParquetMetaData::new(file_metadata, row_groups))
+}
+
+/// Parses column orders from Thrift definition.
+/// If no column orders are defined, returns `None`.
+pub(crate) fn parse_column_orders(
+ t_column_orders: Option<Vec<crate::format::ColumnOrder>>,
+ schema_descr: &SchemaDescriptor,
+) -> crate::errors::Result<Option<Vec<ColumnOrder>>> {
+ match t_column_orders {
+ Some(orders) => {
+ // Should always be the case
+ if orders.len() != schema_descr.num_columns() {
+ return Err(general_err!("Column order length mismatch"));
+ };
+ let mut res = Vec::new();
+ for (i, column) in schema_descr.columns().iter().enumerate() {
+ match orders[i] {
+ crate::format::ColumnOrder::TYPEORDER(_) => {
+ let sort_order = ColumnOrder::get_sort_order(
+ column.logical_type(),
+ column.converted_type(),
+ column.physical_type(),
+ );
+ res.push(ColumnOrder::TYPE_DEFINED_ORDER(sort_order));
+ }
+ }
+ }
+ Ok(Some(res))
+ }
+ None => Ok(None),
+ }
+}
+
+pub(crate) fn parse_column_index(
Review Comment:
Hmm, there are details (the `parse_xxx_index` methods need access to private fields in the metadata). I guess leave it here for now.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3350696189
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing alamb/refactor_push_decoder (512195bad42c514feccd98047b552f2bd7dc1b45) to 422da15fa58447702e6186b0cc7361f3cb4a7aa5 [diff](https://github.com/apache/arrow-rs/compare/422da15fa58447702e6186b0cc7361f3cb4a7aa5..512195bad42c514feccd98047b552f2bd7dc1b45)
BENCH_NAME=metadata
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench metadata
BENCH_FILTER=
BENCH_BRANCH_NAME=alamb_refactor_push_decoder
Results will be posted here when complete
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3286712087
> Cool -- I likewise don't think it will conflict too badly logically, though I think it may generate lots of textual diffs that we'll have to be careful with.
Agreed. Looking forward to this one. I'm hoping for a much more flexible metadata parsing regime after the dust settles.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3338752127
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing alamb/refactor_push_decoder (4fb5ce56ccd9b3e260d4e6b79187ec8e0c7af3b7) to 6ecbd623c046c081c898d5b3b2417b453c1db8db [diff](https://github.com/apache/arrow-rs/compare/6ecbd623c046c081c898d5b3b2417b453c1db8db..4fb5ce56ccd9b3e260d4e6b79187ec8e0c7af3b7)
BENCH_NAME=metadata
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench metadata
BENCH_FILTER=
BENCH_BRANCH_NAME=alamb_refactor_push_decoder
Results will be posted here when complete
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2349883002
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,467 @@
+// 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.
+
+//! Internal metadata parsing routines
+//!
+//! In general these functions parse thrift-encoded metadata from a byte slice
+//! into the corresponding Rust structures
+
+use crate::basic::ColumnOrder;
+use crate::errors::ParquetError;
+use crate::file::metadata::{
+ ColumnChunkMetaData, FileMetaData, PageIndexPolicy, ParquetMetaData, RowGroupMetaData,
+};
+use crate::file::page_index::index::Index;
+use crate::file::page_index::index_reader::{decode_column_index, decode_offset_index};
+use crate::file::page_index::offset_index::OffsetIndexMetaData;
+use crate::schema::types;
+use crate::schema::types::SchemaDescriptor;
+use crate::thrift::TCompactSliceInputProtocol;
+use crate::thrift::TSerializable;
+use bytes::Bytes;
+use std::sync::Arc;
+
+#[cfg(feature = "encryption")]
+use crate::encryption::{
+ decrypt::{FileDecryptionProperties, FileDecryptor},
+ modules::create_footer_aad,
+};
+#[cfg(feature = "encryption")]
+use crate::format::EncryptionAlgorithm;
+
+/// Decodes [`ParquetMetaData`] from the provided bytes.
+///
+/// Typically this is used to decode the metadata from the end of a parquet
+/// file. The format of `buf` is the Thrift compact binary protocol, as specified
+/// by the [Parquet Spec].
+///
+/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
+pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaData> {
+ let mut prot = TCompactSliceInputProtocol::new(buf);
+
+ let t_file_metadata: crate::format::FileMetaData =
+ crate::format::FileMetaData::read_from_in_protocol(&mut prot)
+ .map_err(|e| general_err!("Could not parse metadata: {}", e))?;
+ let schema = types::from_thrift(&t_file_metadata.schema)?;
+ let schema_descr = Arc::new(SchemaDescriptor::new(schema));
+
+ let mut row_groups = Vec::new();
+ for rg in t_file_metadata.row_groups {
+ row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
+ }
+ let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr)?;
+
+ let file_metadata = FileMetaData::new(
+ t_file_metadata.version,
+ t_file_metadata.num_rows,
+ t_file_metadata.created_by,
+ t_file_metadata.key_value_metadata,
+ schema_descr,
+ column_orders,
+ );
+
+ Ok(ParquetMetaData::new(file_metadata, row_groups))
+}
+
+/// Parses column orders from Thrift definition.
+/// If no column orders are defined, returns `None`.
+pub(crate) fn parse_column_orders(
+ t_column_orders: Option<Vec<crate::format::ColumnOrder>>,
+ schema_descr: &SchemaDescriptor,
+) -> crate::errors::Result<Option<Vec<ColumnOrder>>> {
+ match t_column_orders {
+ Some(orders) => {
+ // Should always be the case
+ if orders.len() != schema_descr.num_columns() {
+ return Err(general_err!("Column order length mismatch"));
+ };
+ let mut res = Vec::new();
+ for (i, column) in schema_descr.columns().iter().enumerate() {
+ match orders[i] {
+ crate::format::ColumnOrder::TYPEORDER(_) => {
+ let sort_order = ColumnOrder::get_sort_order(
+ column.logical_type(),
+ column.converted_type(),
+ column.physical_type(),
+ );
+ res.push(ColumnOrder::TYPE_DEFINED_ORDER(sort_order));
+ }
+ }
+ }
+ Ok(Some(res))
+ }
+ None => Ok(None),
+ }
+}
+
+pub(crate) fn parse_column_index(
Review Comment:
That would be great -- I don't know why it is here, but I am quite happy to put it elsewhere if that makes sense
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3292911267
I just did a test merge of this branch with the head of my remodel branch and it went pretty smoothly. The few conflicts were easily resolved. 🚀
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2375362706
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,467 @@
+// 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.
+
+//! Internal metadata parsing routines
+//!
+//! In general these functions parse thrift-encoded metadata from a byte slice
+//! into the corresponding Rust structures
+
+use crate::basic::ColumnOrder;
+use crate::errors::ParquetError;
+use crate::file::metadata::{
+ ColumnChunkMetaData, FileMetaData, PageIndexPolicy, ParquetMetaData, RowGroupMetaData,
+};
+use crate::file::page_index::index::Index;
+use crate::file::page_index::index_reader::{decode_column_index, decode_offset_index};
+use crate::file::page_index::offset_index::OffsetIndexMetaData;
+use crate::schema::types;
+use crate::schema::types::SchemaDescriptor;
+use crate::thrift::TCompactSliceInputProtocol;
+use crate::thrift::TSerializable;
+use bytes::Bytes;
+use std::sync::Arc;
+
+#[cfg(feature = "encryption")]
+use crate::encryption::{
+ decrypt::{FileDecryptionProperties, FileDecryptor},
+ modules::create_footer_aad,
+};
+#[cfg(feature = "encryption")]
+use crate::format::EncryptionAlgorithm;
+
+/// Decodes [`ParquetMetaData`] from the provided bytes.
+///
+/// Typically this is used to decode the metadata from the end of a parquet
+/// file. The format of `buf` is the Thrift compact binary protocol, as specified
+/// by the [Parquet Spec].
+///
+/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
+pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaData> {
+ let mut prot = TCompactSliceInputProtocol::new(buf);
+
+ let t_file_metadata: crate::format::FileMetaData =
+ crate::format::FileMetaData::read_from_in_protocol(&mut prot)
+ .map_err(|e| general_err!("Could not parse metadata: {}", e))?;
+ let schema = types::from_thrift(&t_file_metadata.schema)?;
+ let schema_descr = Arc::new(SchemaDescriptor::new(schema));
+
+ let mut row_groups = Vec::new();
+ for rg in t_file_metadata.row_groups {
+ row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
+ }
+ let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr)?;
+
+ let file_metadata = FileMetaData::new(
+ t_file_metadata.version,
+ t_file_metadata.num_rows,
+ t_file_metadata.created_by,
+ t_file_metadata.key_value_metadata,
+ schema_descr,
+ column_orders,
+ );
+
+ Ok(ParquetMetaData::new(file_metadata, row_groups))
+}
+
+/// Parses column orders from Thrift definition.
+/// If no column orders are defined, returns `None`.
+pub(crate) fn parse_column_orders(
+ t_column_orders: Option<Vec<crate::format::ColumnOrder>>,
+ schema_descr: &SchemaDescriptor,
+) -> crate::errors::Result<Option<Vec<ColumnOrder>>> {
+ match t_column_orders {
+ Some(orders) => {
+ // Should always be the case
+ if orders.len() != schema_descr.num_columns() {
+ return Err(general_err!("Column order length mismatch"));
+ };
+ let mut res = Vec::new();
+ for (i, column) in schema_descr.columns().iter().enumerate() {
+ match orders[i] {
+ crate::format::ColumnOrder::TYPEORDER(_) => {
+ let sort_order = ColumnOrder::get_sort_order(
+ column.logical_type(),
+ column.converted_type(),
+ column.physical_type(),
+ );
+ res.push(ColumnOrder::TYPE_DEFINED_ORDER(sort_order));
+ }
+ }
+ }
+ Ok(Some(res))
+ }
+ None => Ok(None),
+ }
+}
+
+pub(crate) fn parse_column_index(
Review Comment:
I think part of it will always be feature gated, because of the additional dependencies encryption brings in. However, we could potentially feature gate less of the actual code (maybe only feature gate the actual decryption calls 🤔 )
I was also thinking having some sort of stateful struct while decoding metadata could be useful for other reasons (for example, specifying for which columns to decode the statistics)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2383115155
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -211,16 +232,38 @@ impl ParquetMetaDataPushDecoder {
)));
};
- let metadata_reader =
- ParquetMetaDataReader::new().with_page_index_policy(PageIndexPolicy::Optional);
-
Ok(Self {
- done: false,
- metadata_reader,
+ state: DecodeState::ReadingFooter,
+ column_index_policy: PageIndexPolicy::Optional,
+ offset_index_policy: PageIndexPolicy::Optional,
buffers: crate::util::push_buffers::PushBuffers::new(file_len),
+ metadata_parser: MetadataParser::new(),
})
}
+ /// Begin decoding from the given footer tail.
+ pub(crate) fn try_new_with_footer_tail(
+ file_len: u64,
+ footer_tail: FooterTail,
+ ) -> Result<Self, ParquetError> {
Review Comment:
Oh, now I see what you are asking -- I will update this to use crate::errors::Result (which is what I thought I had used)!
Edit: in https://github.com/apache/arrow-rs/pull/8340/commits/95fffc53f0a3ba775cc0cd95cb5ee205851288d6
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3286696297
> This almost certainly will conflict with @etseidl 's plans in thrift-remodel.
I took a quick peak and I think it won't be too hard to merge this into what I'm doing. It will likely help if I can merge this without any other changes from main to contend with. We can coordinate that pas de deux when the time comes 😅
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3338713172
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing alamb/refactor_push_decoder (12bccecce68ffef6eabb8c8039680b77e09df8d0) to 6ecbd623c046c081c898d5b3b2417b453c1db8db [diff](https://github.com/apache/arrow-rs/compare/6ecbd623c046c081c898d5b3b2417b453c1db8db..12bccecce68ffef6eabb8c8039680b77e09df8d0)
BENCH_NAME=metadata
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench metadata
BENCH_FILTER=
BENCH_BRANCH_NAME=alamb_refactor_push_decoder
Results will be posted here when complete
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3334371814
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing alamb/refactor_push_decoder (fc2fd81c053c026dba217fbc1f50f5f52250cf7b) to 3027dbc595819763dc2bff74b024ce943c82ca06 [diff](https://github.com/apache/arrow-rs/compare/3027dbc595819763dc2bff74b024ce943c82ca06..fc2fd81c053c026dba217fbc1f50f5f52250cf7b)
BENCH_NAME=metadata
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench metadata
BENCH_FILTER=
BENCH_BRANCH_NAME=alamb_refactor_push_decoder
Results will be posted here when complete
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3339216099
I'm going to try merging this into my remodel branch and see what comes up.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3339078496
Looks like the benchmark differences are noise. I have an idea to reduce some allocations though, which I will push up here
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2365583592
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -261,68 +269,152 @@ impl ParquetMetaDataPushDecoder {
&mut self,
ranges: Vec<std::ops::Range<u64>>,
buffers: Vec<bytes::Bytes>,
- ) -> std::result::Result<(), String> {
- if self.done {
- return Err(
+ ) -> Result<(), ParquetError> {
+ if matches!(&self.state, DecodeState::Finished) {
+ return Err(general_err!(
"ParquetMetaDataPushDecoder: cannot push data after decoding is finished"
- .to_string(),
- );
+ ));
}
self.buffers.push_ranges(ranges, buffers);
Ok(())
}
/// Try to decode the metadata from the pushed data, returning the
/// decoded metadata or an error if not enough data is available.
- pub fn try_decode(
- &mut self,
- ) -> std::result::Result<DecodeResult<ParquetMetaData>, ParquetError> {
- if self.done {
- return Ok(DecodeResult::Finished);
- }
-
- // need to have the last 8 bytes of the file to decode the metadata
+ pub fn try_decode(&mut self) -> Result<DecodeResult<ParquetMetaData>, ParquetError> {
let file_len = self.buffers.file_len();
- if !self.buffers.has_range(&(file_len - 8..file_len)) {
- #[expect(clippy::single_range_in_vec_init)]
- return Ok(DecodeResult::NeedsData(vec![file_len - 8..file_len]));
+ let footer_len = FOOTER_SIZE as u64;
+ loop {
+ match std::mem::replace(&mut self.state, DecodeState::Intermediate) {
Review Comment:
I am quite pleased with how this decoder state machine is looking
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3350715034
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group alamb_refactor_push_decoder main
----- --------------------------- ----
decode parquet metadata 1.00 24.7±0.11µs ? ?/sec 1.02 25.3±0.09µs ? ?/sec
decode parquet metadata (wide) 1.00 146.7±4.81ms ? ?/sec 1.01 147.5±4.27ms ? ?/sec
open(default) 1.01 23.9±0.10µs ? ?/sec 1.00 23.6±0.30µs ? ?/sec
open(page index) 1.01 1291.8±4.23µs ? ?/sec 1.00 1275.6±3.97µs ? ?/sec
```
</p>
</details>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2349540828
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,467 @@
+// 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.
+
+//! Internal metadata parsing routines
+//!
+//! In general these functions parse thrift-encoded metadata from a byte slice
+//! into the corresponding Rust structures
+
+use crate::basic::ColumnOrder;
+use crate::errors::ParquetError;
+use crate::file::metadata::{
+ ColumnChunkMetaData, FileMetaData, PageIndexPolicy, ParquetMetaData, RowGroupMetaData,
+};
+use crate::file::page_index::index::Index;
+use crate::file::page_index::index_reader::{decode_column_index, decode_offset_index};
+use crate::file::page_index::offset_index::OffsetIndexMetaData;
+use crate::schema::types;
+use crate::schema::types::SchemaDescriptor;
+use crate::thrift::TCompactSliceInputProtocol;
+use crate::thrift::TSerializable;
+use bytes::Bytes;
+use std::sync::Arc;
+
+#[cfg(feature = "encryption")]
+use crate::encryption::{
+ decrypt::{FileDecryptionProperties, FileDecryptor},
+ modules::create_footer_aad,
+};
+#[cfg(feature = "encryption")]
+use crate::format::EncryptionAlgorithm;
+
+/// Decodes [`ParquetMetaData`] from the provided bytes.
+///
+/// Typically this is used to decode the metadata from the end of a parquet
+/// file. The format of `buf` is the Thrift compact binary protocol, as specified
+/// by the [Parquet Spec].
+///
+/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
+pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaData> {
+ let mut prot = TCompactSliceInputProtocol::new(buf);
+
+ let t_file_metadata: crate::format::FileMetaData =
+ crate::format::FileMetaData::read_from_in_protocol(&mut prot)
+ .map_err(|e| general_err!("Could not parse metadata: {}", e))?;
+ let schema = types::from_thrift(&t_file_metadata.schema)?;
+ let schema_descr = Arc::new(SchemaDescriptor::new(schema));
+
+ let mut row_groups = Vec::new();
+ for rg in t_file_metadata.row_groups {
+ row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
+ }
+ let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr)?;
+
+ let file_metadata = FileMetaData::new(
+ t_file_metadata.version,
+ t_file_metadata.num_rows,
+ t_file_metadata.created_by,
+ t_file_metadata.key_value_metadata,
+ schema_descr,
+ column_orders,
+ );
+
+ Ok(ParquetMetaData::new(file_metadata, row_groups))
+}
+
+/// Parses column orders from Thrift definition.
+/// If no column orders are defined, returns `None`.
+pub(crate) fn parse_column_orders(
+ t_column_orders: Option<Vec<crate::format::ColumnOrder>>,
+ schema_descr: &SchemaDescriptor,
+) -> crate::errors::Result<Option<Vec<ColumnOrder>>> {
+ match t_column_orders {
+ Some(orders) => {
+ // Should always be the case
+ if orders.len() != schema_descr.num_columns() {
+ return Err(general_err!("Column order length mismatch"));
+ };
+ let mut res = Vec::new();
+ for (i, column) in schema_descr.columns().iter().enumerate() {
+ match orders[i] {
+ crate::format::ColumnOrder::TYPEORDER(_) => {
+ let sort_order = ColumnOrder::get_sort_order(
+ column.logical_type(),
+ column.converted_type(),
+ column.physical_type(),
+ );
+ res.push(ColumnOrder::TYPE_DEFINED_ORDER(sort_order));
+ }
+ }
+ }
+ Ok(Some(res))
+ }
+ None => Ok(None),
+ }
+}
+
+pub(crate) fn parse_column_index(
Review Comment:
One option to consider is to move the column and offset index handling to `file/metadata/page_index/index_reader.rs`. Or I can do that later as part of the thrift remodel. That would keep all the page index parsing in one place.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2373441064
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,467 @@
+// 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.
+
+//! Internal metadata parsing routines
+//!
+//! In general these functions parse thrift-encoded metadata from a byte slice
+//! into the corresponding Rust structures
+
+use crate::basic::ColumnOrder;
+use crate::errors::ParquetError;
+use crate::file::metadata::{
+ ColumnChunkMetaData, FileMetaData, PageIndexPolicy, ParquetMetaData, RowGroupMetaData,
+};
+use crate::file::page_index::index::Index;
+use crate::file::page_index::index_reader::{decode_column_index, decode_offset_index};
+use crate::file::page_index::offset_index::OffsetIndexMetaData;
+use crate::schema::types;
+use crate::schema::types::SchemaDescriptor;
+use crate::thrift::TCompactSliceInputProtocol;
+use crate::thrift::TSerializable;
+use bytes::Bytes;
+use std::sync::Arc;
+
+#[cfg(feature = "encryption")]
+use crate::encryption::{
+ decrypt::{FileDecryptionProperties, FileDecryptor},
+ modules::create_footer_aad,
+};
+#[cfg(feature = "encryption")]
+use crate::format::EncryptionAlgorithm;
+
+/// Decodes [`ParquetMetaData`] from the provided bytes.
+///
+/// Typically this is used to decode the metadata from the end of a parquet
+/// file. The format of `buf` is the Thrift compact binary protocol, as specified
+/// by the [Parquet Spec].
+///
+/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
+pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaData> {
+ let mut prot = TCompactSliceInputProtocol::new(buf);
+
+ let t_file_metadata: crate::format::FileMetaData =
+ crate::format::FileMetaData::read_from_in_protocol(&mut prot)
+ .map_err(|e| general_err!("Could not parse metadata: {}", e))?;
+ let schema = types::from_thrift(&t_file_metadata.schema)?;
+ let schema_descr = Arc::new(SchemaDescriptor::new(schema));
+
+ let mut row_groups = Vec::new();
+ for rg in t_file_metadata.row_groups {
+ row_groups.push(RowGroupMetaData::from_thrift(schema_descr.clone(), rg)?);
+ }
+ let column_orders = parse_column_orders(t_file_metadata.column_orders, &schema_descr)?;
+
+ let file_metadata = FileMetaData::new(
+ t_file_metadata.version,
+ t_file_metadata.num_rows,
+ t_file_metadata.created_by,
+ t_file_metadata.key_value_metadata,
+ schema_descr,
+ column_orders,
+ );
+
+ Ok(ParquetMetaData::new(file_metadata, row_groups))
+}
+
+/// Parses column orders from Thrift definition.
+/// If no column orders are defined, returns `None`.
+pub(crate) fn parse_column_orders(
+ t_column_orders: Option<Vec<crate::format::ColumnOrder>>,
+ schema_descr: &SchemaDescriptor,
+) -> crate::errors::Result<Option<Vec<ColumnOrder>>> {
+ match t_column_orders {
+ Some(orders) => {
+ // Should always be the case
+ if orders.len() != schema_descr.num_columns() {
+ return Err(general_err!("Column order length mismatch"));
+ };
+ let mut res = Vec::new();
+ for (i, column) in schema_descr.columns().iter().enumerate() {
+ match orders[i] {
+ crate::format::ColumnOrder::TYPEORDER(_) => {
+ let sort_order = ColumnOrder::get_sort_order(
+ column.logical_type(),
+ column.converted_type(),
+ column.physical_type(),
+ );
+ res.push(ColumnOrder::TYPE_DEFINED_ORDER(sort_order));
+ }
+ }
+ }
+ Ok(Some(res))
+ }
+ None => Ok(None),
+ }
+}
+
+pub(crate) fn parse_column_index(
Review Comment:
@etseidl -- I spent a bit more time on this one, and I encountered a challenge with encryption -- namely there are now 2 parallel codepaths -- one for encrypted data and one for non encrypted data.
The only way I can come up with to avoid replicating the same pattern (🤮 ) is to change the free functions into some sort of struct that can pass the state along with self
Something like intead of
```rust
pub(crate) fn parse_column_index(
metadata: &mut ParquetMetaData,
column_index_policy: PageIndexPolicy,
bytes: &Bytes,
start_offset: u64,
) -> crate::errors::Result<()> {
```
like
```rust
struct MetadataParser {
// page index policy:
page_index_policy: PageIndexPolicy,
// ... other state fields like crypto policies
}
...
impl MetadataParser {
fn parse_column_index(
metadata: &mut ParquetMetaData,
bytes: &Bytes,
start_offset: u64,
) -> crate::errors::Result<()> {
```
However, I want to coordinate to avoid a massive merge conflict.
For now, I plan to just keep the same pattern
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#issuecomment-3338766443
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group alamb_refactor_push_decoder main
----- --------------------------- ----
decode parquet metadata 1.01 25.7±0.30µs ? ?/sec 1.00 25.5±0.22µs ? ?/sec
decode parquet metadata (wide) 1.03 154.5±3.73ms ? ?/sec 1.00 149.7±2.00ms ? ?/sec
decode thrift file metadata 1.01 16.9±0.05µs ? ?/sec 1.00 16.8±0.10µs ? ?/sec
decode thrift file metadata (wide) 1.01 107.1±0.52ms ? ?/sec 1.00 106.4±0.30ms ? ?/sec
open(default) 1.04 24.3±0.09µs ? ?/sec 1.00 23.4±0.28µs ? ?/sec
open(page index) 1.01 1297.6±2.40µs ? ?/sec 1.00 1279.8±10.47µs ? ?/sec
page headers 1.00 7.4±0.05µs ? ?/sec 1.00 7.3±0.02µs ? ?/sec
```
</p>
</details>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2382462026
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -492,15 +490,27 @@ impl ParquetMetaDataReader {
async fn load_page_index_with_remainder<F: MetadataFetch>(
&mut self,
mut fetch: F,
- remainder: Option<(usize, bytes::Bytes)>,
+ remainder: Option<(usize, Bytes)>,
) -> Result<()> {
- // Get bounds needed for page indexes (if any are present in the file).
- let Some(range) = self.range_for_page_index() else {
- return Ok(());
+ let Some(metadata) = self.metadata.take() else {
Review Comment:
I had hoped we would be able to remove more of the logic from `ParquetMetadataReader` but I couldn't figure out how to do so given the somewhat complex way it supports reading metadata even when the file length isn't known
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -0,0 +1,555 @@
+// 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.
+
+//! Internal metadata parsing routines
Review Comment:
Most of this module is just moving the the parsing functions (that take `&[u8]` and convert them to the relevant rust structure), I made no changes to the actual logic
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -79,7 +74,7 @@ pub struct ParquetMetaDataReader {
// `self.parse_metadata` is called.
metadata_size: Option<usize>,
#[cfg(feature = "encryption")]
- file_decryption_properties: Option<FileDecryptionProperties>,
+ file_decryption_properties: Option<std::sync::Arc<FileDecryptionProperties>>,
Review Comment:
The `FileDecryptionProperties` is currently copied, which is unfortunately.
As a follow on PR, I plan to update the options elsewhere to use a `Arc<FileDecryptonProperties>` to avoid copies
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -211,16 +232,38 @@ impl ParquetMetaDataPushDecoder {
)));
};
- let metadata_reader =
- ParquetMetaDataReader::new().with_page_index_policy(PageIndexPolicy::Optional);
-
Ok(Self {
- done: false,
- metadata_reader,
+ state: DecodeState::ReadingFooter,
+ column_index_policy: PageIndexPolicy::Optional,
+ offset_index_policy: PageIndexPolicy::Optional,
buffers: crate::util::push_buffers::PushBuffers::new(file_len),
+ metadata_parser: MetadataParser::new(),
})
}
+ /// Begin decoding from the given footer tail.
+ pub(crate) fn try_new_with_footer_tail(
+ file_len: u64,
+ footer_tail: FooterTail,
+ ) -> Result<Self, ParquetError> {
+ let mut new_self = Self::try_new(file_len)?;
+ new_self.state = DecodeState::ReadingMetadata(footer_tail);
+ Ok(new_self)
+ }
+
+ /// Create a decoder with the given `ParquetMetaData` already known.
+ ///
+ /// This can be used to parse and populate the page index structures
Review Comment:
I think this is now a nice API to load/decode PageIndexes -- provide an existing ParquetMetadata and then this decoder figures out what bytes are needed and parses them. If we ever want to extend ParquetMetadata to include, for example, BloomFilters, we could use the same basic idea
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -43,6 +43,86 @@ use crate::encryption::{
#[cfg(feature = "encryption")]
use crate::format::EncryptionAlgorithm;
+/// Helper struct for metadata parsing
+///
+/// This structure parses thrift-encoded bytes into the correct Rust structs,
+/// such as [`ParquetMetaData`], handling decryption if necessary.
+//
+// Note this structure is used to minimize the number of
+// places need to add `#[cfg(feature = "encryption")]` checks.
+pub(crate) use inner::MetadataParser;
+
+#[cfg(feature = "encryption")]
+mod inner {
+ use super::*;
+ use crate::encryption::decrypt::FileDecryptionProperties;
+ use crate::errors::Result;
+
+ /// API for decoding metadata that may be encrypted
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser {
Review Comment:
I am thinking we can eventually use this structure as the place to hang more detailed decoding instructions (like "only decode statistics for column `A`" on)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move ParquetMetadata decoder state machine into ParquetMetadataPushDecoder [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8340:
URL: https://github.com/apache/arrow-rs/pull/8340#discussion_r2382839671
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -211,16 +232,38 @@ impl ParquetMetaDataPushDecoder {
)));
};
- let metadata_reader =
- ParquetMetaDataReader::new().with_page_index_policy(PageIndexPolicy::Optional);
-
Ok(Self {
- done: false,
- metadata_reader,
+ state: DecodeState::ReadingFooter,
+ column_index_policy: PageIndexPolicy::Optional,
+ offset_index_policy: PageIndexPolicy::Optional,
buffers: crate::util::push_buffers::PushBuffers::new(file_len),
+ metadata_parser: MetadataParser::new(),
})
}
+ /// Begin decoding from the given footer tail.
+ pub(crate) fn try_new_with_footer_tail(
+ file_len: u64,
+ footer_tail: FooterTail,
+ ) -> Result<Self, ParquetError> {
Review Comment:
Just curious, but do you dislike use of `crate::errors::Result`?
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -79,7 +159,7 @@ pub(crate) fn decode_metadata(buf: &[u8]) -> crate::errors::Result<ParquetMetaDa
/// Parses column orders from Thrift definition.
/// If no column orders are defined, returns `None`.
-pub(crate) fn parse_column_orders(
+fn parse_column_orders(
Review Comment:
This will go away, btw.
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -43,6 +43,86 @@ use crate::encryption::{
#[cfg(feature = "encryption")]
use crate::format::EncryptionAlgorithm;
+/// Helper struct for metadata parsing
+///
+/// This structure parses thrift-encoded bytes into the correct Rust structs,
+/// such as [`ParquetMetaData`], handling decryption if necessary.
+//
+// Note this structure is used to minimize the number of
+// places need to add `#[cfg(feature = "encryption")]` checks.
+pub(crate) use inner::MetadataParser;
+
+#[cfg(feature = "encryption")]
+mod inner {
+ use super::*;
+ use crate::encryption::decrypt::FileDecryptionProperties;
+ use crate::errors::Result;
+
+ /// API for decoding metadata that may be encrypted
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser {
+ // the credentials and keys needed to decrypt metadata
+ file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ }
+
+ impl MetadataParser {
+ pub(crate) fn new() -> Self {
+ MetadataParser::default()
+ }
+
+ pub(crate) fn with_file_decryption_properties(
+ mut self,
+ file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ ) -> Self {
+ self.file_decryption_properties = file_decryption_properties;
+ self
+ }
+
+ pub(crate) fn decode_metadata(
+ &self,
+ buf: &[u8],
+ encrypted_footer: bool,
+ ) -> Result<ParquetMetaData> {
+ decode_metadata_with_encryption(
+ buf,
+ encrypted_footer,
+ self.file_decryption_properties.as_deref(),
+ )
+ }
+ }
+}
+
+#[cfg(not(feature = "encryption"))]
+mod inner {
+ use super::*;
+ use crate::errors::Result;
+ /// parallel implementation when encryption feature is not enabled
+ ///
+ /// This has the same API as the encryption-enabled version
+ #[derive(Debug, Default)]
+ pub(crate) struct MetadataParser;
+
+ impl MetadataParser {
+ pub(crate) fn new() -> Self {
+ MetadataParser
+ }
+
+ pub(crate) fn decode_metadata(
+ &self,
+ buf: &[u8],
+ encrypted_footer: bool,
+ ) -> Result<ParquetMetaData> {
+ if encrypted_footer {
+ Err(general_err!(
+ "Parquet file has an encrypted footer but the encryption feature is disabled"
+ ))
+ } else {
+ decode_metadata(buf)
Review Comment:
This is the only problematic line for the merge. For my initial pass I replaced this call with the thrift decode, but on second thought I should just change the implementation of `decode_metadata` below to use the new structs.
##########
parquet/src/file/metadata/push_decoder.rs:
##########
@@ -259,70 +325,160 @@ impl ParquetMetaDataPushDecoder {
/// example on [`Self`]
pub fn push_ranges(
&mut self,
- ranges: Vec<std::ops::Range<u64>>,
- buffers: Vec<bytes::Bytes>,
- ) -> std::result::Result<(), String> {
- if self.done {
- return Err(
+ ranges: Vec<Range<u64>>,
+ buffers: Vec<Bytes>,
+ ) -> Result<(), ParquetError> {
+ if matches!(&self.state, DecodeState::Finished) {
+ return Err(general_err!(
"ParquetMetaDataPushDecoder: cannot push data after decoding is finished"
- .to_string(),
- );
+ ));
}
self.buffers.push_ranges(ranges, buffers);
Ok(())
}
- /// Try to decode the metadata from the pushed data, returning the
- /// decoded metadata or an error if not enough data is available.
- pub fn try_decode(
- &mut self,
- ) -> std::result::Result<DecodeResult<ParquetMetaData>, ParquetError> {
- if self.done {
- return Ok(DecodeResult::Finished);
+ /// Pushes a single range of data into the decoder's buffer.
+ pub fn push_range(&mut self, range: Range<u64>, buffer: Bytes) -> Result<(), ParquetError> {
+ if matches!(&self.state, DecodeState::Finished) {
+ return Err(general_err!(
+ "ParquetMetaDataPushDecoder: cannot push data after decoding is finished"
+ ));
}
+ self.buffers.push_range(range, buffer);
+ Ok(())
+ }
- // need to have the last 8 bytes of the file to decode the metadata
+ /// Try to decode the metadata from the pushed data, returning the
+ /// decoded metadata or an error if not enough data is available.
+ pub fn try_decode(&mut self) -> Result<DecodeResult<ParquetMetaData>, ParquetError> {
let file_len = self.buffers.file_len();
- if !self.buffers.has_range(&(file_len - 8..file_len)) {
- #[expect(clippy::single_range_in_vec_init)]
- return Ok(DecodeResult::NeedsData(vec![file_len - 8..file_len]));
+ let footer_len = FOOTER_SIZE as u64;
+ loop {
+ match std::mem::replace(&mut self.state, DecodeState::Intermediate) {
+ DecodeState::ReadingFooter => {
+ // need to have the last 8 bytes of the file to decode the metadata
+ let footer_start = file_len.saturating_sub(footer_len);
+ let footer_range = footer_start..file_len;
+
+ if !self.buffers.has_range(&footer_range) {
+ self.state = DecodeState::ReadingFooter;
+ return Ok(needs_range(footer_range));
+ }
+ let footer_bytes = self.get_bytes(&footer_range)?;
+ let footer_tail = FooterTail::try_from(footer_bytes.as_ref())?;
+
+ self.state = DecodeState::ReadingMetadata(footer_tail);
+ continue;
+ }
+
+ DecodeState::ReadingMetadata(footer_tail) => {
+ let metadata_len: u64 = footer_tail.metadata_length() as u64;
+ let metadata_start = file_len - footer_len - metadata_len;
+ let metadata_end = metadata_start + metadata_len;
+ let metadata_range = metadata_start..metadata_end;
+
+ if !self.buffers.has_range(&metadata_range) {
+ self.state = DecodeState::ReadingMetadata(footer_tail);
+ return Ok(needs_range(metadata_range));
+ }
+
+ let metadata = self.metadata_parser.decode_metadata(
+ &self.get_bytes(&metadata_range)?,
+ footer_tail.is_encrypted_footer(),
+ )?;
+ self.state = DecodeState::ReadingPageIndex(Box::new(metadata));
+ continue;
+ }
+
+ DecodeState::ReadingPageIndex(mut metadata) => {
Review Comment:
A little documentation of the state change would be nice here. It's not immediately obvious that `range_for_page_index` will return `None` if page indexes were not requested. I was thinking the check for the latter would be done above while deciding the next state.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org