You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kczimm (via GitHub)" <gi...@apache.org> on 2025/08/06 20:53:48 UTC
[PR] create PageIndexPolicy to allow optional indexes [arrow-rs]
kczimm opened a new pull request, #8071:
URL: https://github.com/apache/arrow-rs/pull/8071
# Which issue does this PR close?
- Closes #8070.
# Rationale for this change
This change introduces a more flexible way to handle page indexes (column and offset indexes) in Parquet files. Previously, the reading of these indexes was controlled by boolean flags, which indicated read required or do not read. The new `PageIndexPolicy` enum (`Off`, `Optional`, `Required`) provides finer control, allowing users to specify whether an index is not read, read if present (without error if missing), or strictly required (error if missing).
# What changes are included in this PR?
- Introduced a new `PageIndexPolicy` enum with `Off`, `Optional`, and `Required` variants.
- Replaced the boolean `column_index` and `offset_index` fields in `ParquetMetaDataReader` with the new `PageIndexPolicy` enum.
- Updated the `ParquetMetaDataReader::new()` function to initialize page index policies to `Off`, preserving previous defaults.
- Modified existing `with_page_indexes`, `with_column_indexes`, and `with_offset_indexes` methods to utilize the new `PageIndexPolicy`, defaulting to `Required` when enabling indexes.
- Added new methods: `with_page_index_policy`, `with_column_index_policy`, and `with_offset_index_policy` to allow direct setting of the page index policy.
- Adjusted the internal logic for parsing column and offset indexes to respect the specified `PageIndexPolicy`, including returning an error if a `Required` index is not found.
# Are these changes tested?
Yes, a new test file `parquet/tests/page_index.rs` has been added to cover the functionality of the new `PageIndexPolicy` and its integration with `ParquetMetaDataReader`.
# Are there any user-facing changes?
Yes, there are user-facing changes to the `ParquetMetaDataReader` API. The `with_column_indexes` and `with_offset_indexes` methods now implicitly use `PageIndexPolicy::Required` when enabling page indexes. New methods `with_page_index_policy`, `with_column_index_policy`, and `with_offset_index_policy` have been added.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2274301022
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,46 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
pub fn with_page_indexes(self, val: bool) -> Self {
Review Comment:
maybe we should deprecate these methods too to guide people to the new APIs
Following this model perhaps: https://github.com/apache/arrow-rs?tab=readme-ov-file#deprecation-guidelines
##########
parquet/tests/page_index.rs:
##########
@@ -0,0 +1,73 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::fs::File;
Review Comment:
Rather than making a new test binary, could you please put these tests near the existing tests for the reader in https://github.com/apache/arrow-rs/blob/a6d6e353acbb8f9ac792bf58e908ee4de5846b1d/parquet/src/file/metadata/reader.rs#L1616-L1615 ?
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2274871459
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
Review Comment:
```suggestion
#[deprecated(since = "56.1.0", note = "Use `with_page_index_policy` instead")]
```
These changes will go into ~56.0.1~ 56.1.0
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2274872787
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
pub fn with_page_indexes(self, val: bool) -> Self {
- self.with_column_indexes(val).with_offset_indexes(val)
+ let policy = PageIndexPolicy::from(val);
+ self.with_column_index_policy(policy)
+ .with_offset_index_policy(policy)
}
/// Enable or disable reading the Parquet [ColumnIndex] structure.
///
/// [ColumnIndex]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
- pub fn with_column_indexes(mut self, val: bool) -> Self {
- self.column_index = val;
- self
+ #[deprecated(since = "56.0.0", note = "Use `with_column_index_policy` instead")]
+ pub fn with_column_indexes(self, val: bool) -> Self {
+ let policy = PageIndexPolicy::from(val);
+ self.with_column_index_policy(policy)
}
/// Enable or disable reading the Parquet [OffsetIndex] structure.
///
/// [OffsetIndex]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
- pub fn with_offset_indexes(mut self, val: bool) -> Self {
- self.offset_index = val;
+ #[deprecated(since = "56.0.0", note = "Use `with_offset_index_policy` instead")]
Review Comment:
```suggestion
#[deprecated(since = "56.1.0", note = "Use `with_offset_index_policy` instead")]
```
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2276446549
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
Review Comment:
I think technically it will be `56.1.0` (not `56.0.1`)
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2261725971
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -105,7 +126,15 @@ impl FooterTail {
impl ParquetMetaDataReader {
/// Create a new [`ParquetMetaDataReader`]
pub fn new() -> Self {
- Default::default()
+ Self {
Review Comment:
Thank you, you're right. This isn't how it started but it's how it ended up and I didn't catch it. I agree it shouldn't be changed.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3165664290
I think this is a good idea, FWIW and a nice change. Is this PR ready for review @kczimm (it is currently 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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl merged PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2276560580
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
Review Comment:
D'oh 😅
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "Jeadie (via GitHub)" <gi...@apache.org>.
Jeadie commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2258651235
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -593,7 +642,15 @@ impl ParquetMetaDataReader {
col_idx,
)
}
- None => Err(general_err!("missing offset index")),
+ None => {
+ if self.offset_index == PageIndexPolicy::Required {
+ return Err(general_err!("missing offset index"));
+ }
+ Ok(OffsetIndexMetaData {
Review Comment:
A question from naivetaty. What are the implication of `page_locations` being empty? i.e. What behaviour is assumed if this is empty? possibly
1. there are no associated pages
2. we have no pre-indexed idea about which pages are associated so we must calculate it ourselves.
I've started looking at this, but it is convoluted.
I feel the most correct approach would be to change the `ParquetOffsetIndex` to have Options, i.e.
```diff
- pub type ParquetOffsetIndex = Vec<Vec<OffsetIndexMetaData>>;
+ pub type ParquetOffsetIndex = Vec<Vec<Option<OffsetIndexMetaData>>>;
```
This is a bit more involved, but semantically more correct (again from my understanding).
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2274351145
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,46 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
pub fn with_page_indexes(self, val: bool) -> Self {
Review Comment:
I agree! I had the same thought but didn't feel comfortable suggesting deprecation of APIs created by others, especially as a new contributor to this project. (and I didn't see there were guidelines). I'll go ahead and read the guidelines and update this PR accordingly.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3192179912
🚀 thank you @kczimm -- this is a much better API in my mind 👍
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3191922297
Thank you @alamb and @etseidl for your feedback and time.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2274872787
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
pub fn with_page_indexes(self, val: bool) -> Self {
- self.with_column_indexes(val).with_offset_indexes(val)
+ let policy = PageIndexPolicy::from(val);
+ self.with_column_index_policy(policy)
+ .with_offset_index_policy(policy)
}
/// Enable or disable reading the Parquet [ColumnIndex] structure.
///
/// [ColumnIndex]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
- pub fn with_column_indexes(mut self, val: bool) -> Self {
- self.column_index = val;
- self
+ #[deprecated(since = "56.0.0", note = "Use `with_column_index_policy` instead")]
+ pub fn with_column_indexes(self, val: bool) -> Self {
+ let policy = PageIndexPolicy::from(val);
+ self.with_column_index_policy(policy)
}
/// Enable or disable reading the Parquet [OffsetIndex] structure.
///
/// [OffsetIndex]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
- pub fn with_offset_indexes(mut self, val: bool) -> Self {
- self.offset_index = val;
+ #[deprecated(since = "56.0.0", note = "Use `with_offset_index_policy` instead")]
Review Comment:
```suggestion
#[deprecated(since = "56.0.1", note = "Use `with_offset_index_policy` instead")]
```
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
Review Comment:
```suggestion
#[deprecated(since = "56.0.1", note = "Use `with_page_index_policy` instead")]
```
These changes will go into 56.0.1
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
pub fn with_page_indexes(self, val: bool) -> Self {
- self.with_column_indexes(val).with_offset_indexes(val)
+ let policy = PageIndexPolicy::from(val);
+ self.with_column_index_policy(policy)
+ .with_offset_index_policy(policy)
}
/// Enable or disable reading the Parquet [ColumnIndex] structure.
///
/// [ColumnIndex]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
- pub fn with_column_indexes(mut self, val: bool) -> Self {
- self.column_index = val;
- self
+ #[deprecated(since = "56.0.0", note = "Use `with_column_index_policy` instead")]
Review Comment:
```suggestion
#[deprecated(since = "56.0.1", note = "Use `with_column_index_policy` instead")]
```
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3185687736
> I've been somewhat taken over by thrift
That is what we like to hear ! I love fostering the ability to obsess over some low level technical thing to make it really cool!
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3185664981
Sorry, I've been somewhat taken over by thrift and life 😬. I'll try to take another look today, but don't hold this up for me either. My main concern was addressed.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2264388593
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -593,7 +642,15 @@ impl ParquetMetaDataReader {
col_idx,
)
}
- None => Err(general_err!("missing offset index")),
+ None => {
Review Comment:
Thanks @kczimm. I'll try to review some time in the next few days. First glance looks good.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3185728850
working on the CI issues...
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2274871773
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -118,27 +139,49 @@ impl ParquetMetaDataReader {
}
/// Enable or disable reading the page index structures described in
- /// "[Parquet page index]: Layout to Support Page Skipping". Equivalent to:
- /// `self.with_column_indexes(val).with_offset_indexes(val)`
+ /// "[Parquet page index]: Layout to Support Page Skipping".
///
/// [Parquet page index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
+ #[deprecated(since = "56.0.0", note = "Use `with_page_index_policy` instead")]
pub fn with_page_indexes(self, val: bool) -> Self {
- self.with_column_indexes(val).with_offset_indexes(val)
+ let policy = PageIndexPolicy::from(val);
+ self.with_column_index_policy(policy)
+ .with_offset_index_policy(policy)
}
/// Enable or disable reading the Parquet [ColumnIndex] structure.
///
/// [ColumnIndex]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
- pub fn with_column_indexes(mut self, val: bool) -> Self {
- self.column_index = val;
- self
+ #[deprecated(since = "56.0.0", note = "Use `with_column_index_policy` instead")]
Review Comment:
```suggestion
#[deprecated(since = "56.1.0", note = "Use `with_column_index_policy` instead")]
```
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2263332718
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -593,7 +642,15 @@ impl ParquetMetaDataReader {
col_idx,
)
}
- None => Err(general_err!("missing offset index")),
+ None => {
Review Comment:
Thanks for the thoughtful feedback, @etseidl. I see what you mean. I pushed a commit that was an attempt to implement your desire.
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2263824008
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -593,7 +642,15 @@ impl ParquetMetaDataReader {
col_idx,
)
}
- None => Err(general_err!("missing offset index")),
+ None => {
+ if self.offset_index == PageIndexPolicy::Required {
+ return Err(general_err!("missing offset index"));
+ }
+ Ok(OffsetIndexMetaData {
Review Comment:
I've changed it based on @etseidl feedback. Now if we just set the column and offset index to None and return. What do you think about this approach?
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#discussion_r2261631007
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -105,7 +126,15 @@ impl FooterTail {
impl ParquetMetaDataReader {
/// Create a new [`ParquetMetaDataReader`]
pub fn new() -> Self {
- Default::default()
+ Self {
Review Comment:
Is this change necessary? Won't the policies default to `Skip` anyway?
##########
parquet/src/file/metadata/reader.rs:
##########
@@ -593,7 +642,15 @@ impl ParquetMetaDataReader {
col_idx,
)
}
- None => Err(general_err!("missing offset index")),
+ None => {
Review Comment:
This change here is the heart of the PR. The thing that gives me pause is simply replacing missing indexes with empty vectors doesn't let the user know that the indexes are in a potentially unusable state. Are the indexes missing for a single column chunk? An entire row group? We can't really tell without doing a validation step after decoding is complete.
I think if we move forward with this, I'd prefer rather than inserting invalid indexes, we instead invalidate the entire page index (i.e. set column and offset index back to `None` in the `ParquetMetaData`).
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3191664454
> @alamb any last words before merging this?
Nope. DO IT!
--
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] create PageIndexPolicy to allow optional indexes [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8071:
URL: https://github.com/apache/arrow-rs/pull/8071#issuecomment-3191639463
@alamb any last words before merging 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