You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "etseidl (via GitHub)" <gi...@apache.org> on 2025/10/31 22:38:40 UTC
[PR] Metadata options [arrow-rs]
etseidl opened a new pull request, #8763:
URL: https://github.com/apache/arrow-rs/pull/8763
# Which issue does this PR close?
- Part of #8643.
# Rationale for this change
This is a first attempt at an object to help control the parsing of the Parquet metadata.
# What changes are included in this PR?
Adds a new `MetadataOptions` struct, and plumbs it down into the Thrift decoder code. The only option for now is to pass in a schema, which then causes the decoder to skip decoding the schema contained in the footer.
Also adds to the metadata bench to demonstrate the time savings from reusing the schema.
# Are these changes tested?
Yes, adds a new test.
# Are there any user-facing changes?
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500939718
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,108 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct ParquetMetaDataOptions {
+ schema_descr: Option<SchemaDescPtr>,
+}
+
+impl ParquetMetaDataOptions {
+ /// Return a new default [`ParquetMetaDataOptions`].
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Returns an optional [`SchemaDescPtr`] to use when decoding. If this is not `None` then
+ /// the schema in the footer will be skipped.
+ pub fn schema(&self) -> Option<&SchemaDescPtr> {
+ self.schema_descr.as_ref()
+ }
+
+ /// Provide a schema to use when decoding the metadata.
+ pub fn set_schema(&mut self, val: SchemaDescPtr) {
+ self.schema_descr = Some(val);
+ }
+
+ /// Provide a schema to use when decoding the metadata. Returns `Self` for chaining.
+ pub fn with_schema(mut self, val: SchemaDescPtr) -> Self {
+ self.schema_descr = Some(val);
+ self
+ }
+}
Review Comment:
I'm not sure how much I like having a setter and chaining mutator for the same object. It's easier to use the setter from higher level objects like `ArrowReaderOptions`, but nicer to have the more builder-like form when directly constructing a `ParquetMetaDataOptions` to pass to the metadata decoders.
I'm going to look into a macro to cut down on the visual bloat (if not actual code bloat).
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#issuecomment-3513345755
If there are no objections I'll merge this tomorrow. A follow-on is already in the works to deal with the page encoding statistics (#8797), with other statistics objects to be handled after that. I've also got some speed ups for the thrift skipping code on tap (replace the current version that uses recursion with one that maintains state on the stack).
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl merged PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2495244273
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,96 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct MetadataOptions {
+ schema_descr: Option<SchemaDescPtr>,
Review Comment:
Here is a ticket that explains the use case a bit more;
- https://github.com/apache/arrow-rs/issues/5999
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#issuecomment-3493161508
> (I didn't approve it b/c it is still marked as a draft)
Thanks @alamb. I'm still messing around with the API. I think I like hiding the new options object in the file reader APIs, and just exposing it for the metadata readers. The last wrinkle is figuring out a good way to share across the `ParquetMetaDataReader`, `ParquetMetaDataPushDecoder`, and `MetadataParser`, with an eye towards sucking the page index policy stuff into the new options object.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2483106240
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,96 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct MetadataOptions {
+ schema_descr: Option<SchemaDescPtr>,
Review Comment:
It's (1). Say you have a large number of files that share the same schema, there's no need to decode them all. Just grab the schema from the first file and use it for all the others.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500845981
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,102 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct ParquetMetaDataOptions {
+ schema_descr: Option<SchemaDescPtr>,
+}
+
+impl ParquetMetaDataOptions {
+ /// Return a new default [`ParquetMetaDataOptions`].
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Returns an optional [`SchemaDescPtr`] to use when decoding. If this is not `None` then
+ /// the schema in the footer will be skipped.
+ pub fn schema(&self) -> Option<&SchemaDescPtr> {
+ self.schema_descr.as_ref()
+ }
+
+ /// Provide a schema to use when decoding the metadata.
+ pub fn set_schema(&mut self, val: SchemaDescPtr) {
+ self.schema_descr = Some(val);
+ }
+
+ /// Provide a schema to use when decoding the metadata. Returns `Self` for chaining.
+ pub fn with_schema(mut self, val: SchemaDescPtr) -> Self {
+ self.schema_descr = Some(val);
+ self
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use bytes::Bytes;
+
+ use crate::{
+ DecodeResult,
+ file::metadata::{ParquetMetaDataOptions, ParquetMetaDataPushDecoder},
+ util::test_common::file_util::get_test_file,
+ };
+ use std::{io::Read, sync::Arc};
+
+ #[test]
+ fn test_provide_schema() {
+ let mut buf: Vec<u8> = Vec::new();
+ get_test_file("alltypes_plain.parquet")
+ .read_to_end(&mut buf)
+ .unwrap();
+
+ let footer = Bytes::from(buf);
+ let mut decoder = ParquetMetaDataPushDecoder::try_new(footer.len() as u64).unwrap();
+ decoder
+ .push_range(0..footer.len() as u64, footer.clone())
+ .unwrap();
+
+ let expected = match decoder.try_decode().unwrap() {
+ DecodeResult::Data(m) => m,
+ _ => panic!("could not parse metadata"),
+ };
+
+ let mut options = ParquetMetaDataOptions::new();
+ options.set_schema(expected.file_metadata().schema_descr_ptr());
+ let options = Arc::new(options);
+
+ let mut decoder = ParquetMetaDataPushDecoder::try_new(footer.len() as u64)
+ .unwrap()
+ .with_metadata_options(Some(options));
+ decoder.push_range(0..footer.len() as u64, footer).unwrap();
+ let metadata = match decoder.try_decode().unwrap() {
+ DecodeResult::Data(m) => m,
+ _ => panic!("could not parse metadata"),
+ };
+
+ assert_eq!(expected, metadata);
Review Comment:
good idea
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500849857
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -795,7 +810,24 @@ impl ParquetMetaDataReader {
///
/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
pub fn decode_metadata(buf: &[u8]) -> Result<ParquetMetaData> {
- decode_metadata(buf)
+ decode_metadata(buf, None)
Review Comment:
Yes, that would be nice. Maintaining two public APIs that do pretty much the same thing is a bit too much.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2482979688
##########
parquet/src/file/metadata/parser.rs:
##########
@@ -51,6 +53,8 @@ mod inner {
pub(crate) struct MetadataParser {
// the credentials and keys needed to decrypt metadata
file_decryption_properties: Option<Arc<FileDecryptionProperties>>,
+ // metadata parsing options
+ metadata_options: Option<MetadataOptions>,
Review Comment:
Wondering if this should be `Option<Arc<MetadataOptions>>` everywhere.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#issuecomment-3475271628
This may help with #5999
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2483088184
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,96 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct MetadataOptions {
+ schema_descr: Option<SchemaDescPtr>,
Review Comment:
Does this means (1) User provided schema or (2) only (min, max, etc) columns in `schema_descr` be decoded?
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500815752
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,102 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct ParquetMetaDataOptions {
+ schema_descr: Option<SchemaDescPtr>,
+}
+
+impl ParquetMetaDataOptions {
+ /// Return a new default [`ParquetMetaDataOptions`].
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Returns an optional [`SchemaDescPtr`] to use when decoding. If this is not `None` then
+ /// the schema in the footer will be skipped.
+ pub fn schema(&self) -> Option<&SchemaDescPtr> {
+ self.schema_descr.as_ref()
+ }
+
+ /// Provide a schema to use when decoding the metadata.
+ pub fn set_schema(&mut self, val: SchemaDescPtr) {
+ self.schema_descr = Some(val);
+ }
+
+ /// Provide a schema to use when decoding the metadata. Returns `Self` for chaining.
+ pub fn with_schema(mut self, val: SchemaDescPtr) -> Self {
+ self.schema_descr = Some(val);
+ self
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use bytes::Bytes;
+
+ use crate::{
+ DecodeResult,
+ file::metadata::{ParquetMetaDataOptions, ParquetMetaDataPushDecoder},
+ util::test_common::file_util::get_test_file,
+ };
+ use std::{io::Read, sync::Arc};
+
+ #[test]
+ fn test_provide_schema() {
+ let mut buf: Vec<u8> = Vec::new();
+ get_test_file("alltypes_plain.parquet")
+ .read_to_end(&mut buf)
+ .unwrap();
+
+ let footer = Bytes::from(buf);
Review Comment:
Right you are...likely cut and paste from elsewhere 😅.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500734737
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -504,6 +508,13 @@ impl ArrowReaderOptions {
}
}
+ /// Provide a Parquet schema to use when decoding the metadata. The schema in the Parquet
+ /// footer will be skipped.
Review Comment:
Some more context on usecase might be helpful:
```suggestion
/// Provide a Parquet schema to use when decoding the metadata. The schema in the Parquet
/// footer will be skipped.
///
/// This can be used to avoid reparsing the schema from the file when it is
/// already known.
```
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#issuecomment-3492140315
(I didn't approve it b/c it is still marked as a draft)
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2502999591
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,108 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct ParquetMetaDataOptions {
+ schema_descr: Option<SchemaDescPtr>,
+}
+
+impl ParquetMetaDataOptions {
+ /// Return a new default [`ParquetMetaDataOptions`].
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Returns an optional [`SchemaDescPtr`] to use when decoding. If this is not `None` then
+ /// the schema in the footer will be skipped.
+ pub fn schema(&self) -> Option<&SchemaDescPtr> {
+ self.schema_descr.as_ref()
+ }
+
+ /// Provide a schema to use when decoding the metadata.
+ pub fn set_schema(&mut self, val: SchemaDescPtr) {
+ self.schema_descr = Some(val);
+ }
+
+ /// Provide a schema to use when decoding the metadata. Returns `Self` for chaining.
+ pub fn with_schema(mut self, val: SchemaDescPtr) -> Self {
+ self.schema_descr = Some(val);
+ self
+ }
+}
Review Comment:
> I'm going to look into a macro to cut down on the visual bloat (if not actual code bloat).
I personally think it is ok to have two sets of APIs for setting things -- while it is visual bloat as you say I think the methods are so simple it is fairly easy (if repetitive) to understand
While a macro might be clever I worry it would make the code that much harder to understand 🤷 -- probably a matter of opinion
--
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] Metadata options [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2482867989
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -355,7 +367,8 @@ impl ParquetMetaDataReader {
let push_decoder = ParquetMetaDataPushDecoder::try_new_with_metadata(file_size, metadata)?
.with_offset_index_policy(self.offset_index)
- .with_column_index_policy(self.column_index);
+ .with_column_index_policy(self.column_index)
+ .with_metadata_options(self.metadata_options.clone());
Review Comment:
At some point I could see moving the page index policy into the `MetadataOptions` and then deprecating a bunch of setters.
--
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] Metadata options [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#issuecomment-3475131350
Here's an excerpt from a run of the new benchmark that shows the schema is actually skipped.
```
decode parquet metadata time: [14.401 µs 14.436 µs 14.475 µs]
decode metadata with schema
time: [7.1264 µs 7.1461 µs 7.1677 µs]
decode parquet metadata (wide)
time: [48.440 ms 48.828 ms 49.445 ms]
decode metadata (wide) with schema
time: [43.212 ms 43.452 ms 43.793 ms]
```
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#issuecomment-3493672023
Ok, I think this is ready now.
Right now I'm mildly *against* pulling in the page index policies. They are used at a higher level and I don't think it's worth the thrash to move them. Instead I want to focus on options that impact the `FileMetaData` parsing (skip stats, transform page encoding stats, etc), and then work in the metadata index to further accelerate the decoding.
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500690572
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,102 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct ParquetMetaDataOptions {
+ schema_descr: Option<SchemaDescPtr>,
+}
+
+impl ParquetMetaDataOptions {
+ /// Return a new default [`ParquetMetaDataOptions`].
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Returns an optional [`SchemaDescPtr`] to use when decoding. If this is not `None` then
+ /// the schema in the footer will be skipped.
+ pub fn schema(&self) -> Option<&SchemaDescPtr> {
+ self.schema_descr.as_ref()
+ }
+
+ /// Provide a schema to use when decoding the metadata.
+ pub fn set_schema(&mut self, val: SchemaDescPtr) {
+ self.schema_descr = Some(val);
+ }
+
+ /// Provide a schema to use when decoding the metadata. Returns `Self` for chaining.
+ pub fn with_schema(mut self, val: SchemaDescPtr) -> Self {
+ self.schema_descr = Some(val);
+ self
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use bytes::Bytes;
+
+ use crate::{
+ DecodeResult,
+ file::metadata::{ParquetMetaDataOptions, ParquetMetaDataPushDecoder},
+ util::test_common::file_util::get_test_file,
+ };
+ use std::{io::Read, sync::Arc};
+
+ #[test]
+ fn test_provide_schema() {
+ let mut buf: Vec<u8> = Vec::new();
+ get_test_file("alltypes_plain.parquet")
+ .read_to_end(&mut buf)
+ .unwrap();
+
+ let footer = Bytes::from(buf);
Review Comment:
technically speaking this is the entire file, not just the footer, right? As in this might be clearer if it was named something different
```suggestion
let data = Bytes::from(buf);
```
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -795,7 +810,24 @@ impl ParquetMetaDataReader {
///
/// [Parquet Spec]: https://github.com/apache/parquet-format#metadata
pub fn decode_metadata(buf: &[u8]) -> Result<ParquetMetaData> {
- decode_metadata(buf)
+ decode_metadata(buf, None)
Review Comment:
I wonder if we should start directing people to the push metadata decoder (the metadata reader is getting pretty complicated...)
##########
parquet/src/file/serialized_reader.rs:
##########
@@ -2697,6 +2708,21 @@ mod tests {
}
}
+ #[test]
+ fn test_reuse_schema() {
+ let file = get_test_file("alltypes_plain.parquet");
+ let file_reader = SerializedFileReader::new(file.try_clone().unwrap()).unwrap();
+ let schema = file_reader.metadata().file_metadata().schema_descr_ptr();
+ let expected = file_reader.metadata;
+
+ let options = ReadOptionsBuilder::new()
+ .with_parquet_schema(schema)
+ .build();
+ let file_reader = SerializedFileReader::new_with_options(file, options).unwrap();
+
+ assert_eq!(expected.as_ref(), file_reader.metadata.as_ref());
Review Comment:
I think this should also verify that the pointers are the same here too
```diff
diff --git a/parquet/src/file/serialized_reader.rs b/parquet/src/file/serialized_reader.rs
index ad34b15ab5..941267b452 100644
--- a/parquet/src/file/serialized_reader.rs
+++ b/parquet/src/file/serialized_reader.rs
@@ -2716,11 +2716,16 @@ mod tests {
let expected = file_reader.metadata;
let options = ReadOptionsBuilder::new()
- .with_parquet_schema(schema)
+ .with_parquet_schema(Arc::clone(&schema))
.build();
let file_reader = SerializedFileReader::new_with_options(file, options).unwrap();
assert_eq!(expected.as_ref(), file_reader.metadata.as_ref());
+ // Should have used the same schema instance
+ assert!(Arc::ptr_eq(
+ &schema,
+ &file_reader.metadata.file_metadata().schema_descr_ptr()
+ ));
}
```
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -387,6 +389,8 @@ pub struct ArrowReaderOptions {
supplied_schema: Option<SchemaRef>,
/// Policy for reading offset and column indexes.
pub(crate) page_index_policy: PageIndexPolicy,
+ /// Options to control reading of Parquet metadata
Review Comment:
I reviewed the `ArrowReaderOptions` and `ArrowReaderMetadata` structures and their use, and I agree this is the appropriate structure to add metadata parsing to.
Do you think it eventually makes sense to move the other fields from ArrowReaderOptions to `ParquetMetaDataOptions`? (e.g. `supplied_schema`)
##########
parquet/src/file/metadata/options.rs:
##########
@@ -0,0 +1,102 @@
+// 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.
+
+//! Options used to control metadata parsing
+
+use crate::schema::types::SchemaDescPtr;
+
+/// Options that can be set to control what parts of the Parquet file footer
+/// metadata will be decoded and made present in the [`ParquetMetaData`] returned
+/// by [`ParquetMetaDataReader`] and [`ParquetMetaDataPushDecoder`].
+///
+/// [`ParquetMetaData`]: crate::file::metadata::ParquetMetaData
+/// [`ParquetMetaDataReader`]: crate::file::metadata::ParquetMetaDataReader
+/// [`ParquetMetaDataPushDecoder`]: crate::file::metadata::ParquetMetaDataPushDecoder
+#[derive(Default, Debug, Clone)]
+pub struct ParquetMetaDataOptions {
+ schema_descr: Option<SchemaDescPtr>,
+}
+
+impl ParquetMetaDataOptions {
+ /// Return a new default [`ParquetMetaDataOptions`].
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ /// Returns an optional [`SchemaDescPtr`] to use when decoding. If this is not `None` then
+ /// the schema in the footer will be skipped.
+ pub fn schema(&self) -> Option<&SchemaDescPtr> {
+ self.schema_descr.as_ref()
+ }
+
+ /// Provide a schema to use when decoding the metadata.
+ pub fn set_schema(&mut self, val: SchemaDescPtr) {
+ self.schema_descr = Some(val);
+ }
+
+ /// Provide a schema to use when decoding the metadata. Returns `Self` for chaining.
+ pub fn with_schema(mut self, val: SchemaDescPtr) -> Self {
+ self.schema_descr = Some(val);
+ self
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use bytes::Bytes;
+
+ use crate::{
+ DecodeResult,
+ file::metadata::{ParquetMetaDataOptions, ParquetMetaDataPushDecoder},
+ util::test_common::file_util::get_test_file,
+ };
+ use std::{io::Read, sync::Arc};
+
+ #[test]
+ fn test_provide_schema() {
+ let mut buf: Vec<u8> = Vec::new();
+ get_test_file("alltypes_plain.parquet")
+ .read_to_end(&mut buf)
+ .unwrap();
+
+ let footer = Bytes::from(buf);
+ let mut decoder = ParquetMetaDataPushDecoder::try_new(footer.len() as u64).unwrap();
+ decoder
+ .push_range(0..footer.len() as u64, footer.clone())
+ .unwrap();
+
+ let expected = match decoder.try_decode().unwrap() {
+ DecodeResult::Data(m) => m,
+ _ => panic!("could not parse metadata"),
+ };
+
+ let mut options = ParquetMetaDataOptions::new();
+ options.set_schema(expected.file_metadata().schema_descr_ptr());
+ let options = Arc::new(options);
+
+ let mut decoder = ParquetMetaDataPushDecoder::try_new(footer.len() as u64)
+ .unwrap()
+ .with_metadata_options(Some(options));
+ decoder.push_range(0..footer.len() as u64, footer).unwrap();
+ let metadata = match decoder.try_decode().unwrap() {
+ DecodeResult::Data(m) => m,
+ _ => panic!("could not parse metadata"),
+ };
+
+ assert_eq!(expected, metadata);
Review Comment:
I think another important thing to test here is that the schema are actually the same. I think you can do this with something like
```diff
diff --git a/parquet/src/file/metadata/options.rs b/parquet/src/file/metadata/options.rs
index 6f13134df5..6fa6cc1f06 100644
--- a/parquet/src/file/metadata/options.rs
+++ b/parquet/src/file/metadata/options.rs
@@ -84,8 +84,10 @@ mod tests {
_ => panic!("could not parse metadata"),
};
+ let expected_schema = expected.file_metadata().schema_descr_ptr();
+
let mut options = ParquetMetaDataOptions::new();
- options.set_schema(expected.file_metadata().schema_descr_ptr());
+ options.set_schema(expected_schema);
let options = Arc::new(options);
let mut decoder = ParquetMetaDataPushDecoder::try_new(footer.len() as u64)
@@ -98,5 +100,10 @@ mod tests {
};
assert_eq!(expected, metadata);
+ // the schema pointers should be the same
+ assert!(Arc::ptr_eq(
+ &expected.file_metadata().schema_descr_ptr(),
+ &metadata.file_metadata().schema_descr_ptr()
+ ));
}
}
```
##########
parquet/src/file/metadata/thrift/mod.rs:
##########
@@ -711,10 +744,15 @@ pub(crate) fn parquet_metadata_from_bytes(buf: &[u8]) -> Result<ParquetMetaData>
version = Some(i32::read_thrift(&mut prot)?);
}
2 => {
- // read schema and convert to SchemaDescriptor for use when reading row groups
- let val = read_thrift_vec::<SchemaElement, ThriftSliceInputProtocol>(&mut prot)?;
- let val = parquet_schema_from_array(val)?;
- schema_descr = Some(Arc::new(SchemaDescriptor::new(val)));
+ if schema_descr.is_some() {
Review Comment:
```suggestion
// If schema was passed in, skip parsing it
if schema_descr.is_some() {
```
--
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] Add options to control various aspects of Parquet metadata decoding [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8763:
URL: https://github.com/apache/arrow-rs/pull/8763#discussion_r2500813018
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -387,6 +389,8 @@ pub struct ArrowReaderOptions {
supplied_schema: Option<SchemaRef>,
/// Policy for reading offset and column indexes.
pub(crate) page_index_policy: PageIndexPolicy,
+ /// Options to control reading of Parquet metadata
Review Comment:
I was thinking perhaps the `page_index_policy`, but the other things in `ArrowReaderOptions` are more Arrow specific rather than Parquet. That might get confusing.
--
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