You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "friendlymatthew (via GitHub)" <gi...@apache.org> on 2026/01/15 20:23:45 UTC
[PR] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
friendlymatthew opened a new pull request, #9199:
URL: https://github.com/apache/arrow-rs/pull/9199
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/9197
# Rationale for this change
This PR deprecates `ArrowReaderOptions::with_page_index(bool)` in favor of `with_page_index_policy(PageIndexPolicy)` to align with the `ParquetMetadataReader` api. The underlying implementation continues to use separate `column_index` and `offset_index` fields
# Are there any user-facing changes?
Yes, some methods are deprecated
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2695845680
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -455,8 +422,10 @@ pub struct ArrowReaderOptions {
///
/// [ARROW_SCHEMA_META_KEY]: crate::arrow::ARROW_SCHEMA_META_KEY
supplied_schema: Option<SchemaRef>,
- /// Policy for reading offset and column indexes.
- pub(crate) page_index_policy: PageIndexPolicy,
+
+ pub(crate) column_index: PageIndexPolicy,
+ pub(crate) offset_index: PageIndexPolicy,
+
Review Comment:
I split this field out to align with `ParquetMetadataReader`'s design:
https://github.com/apache/arrow-rs/blob/1035781d68e384bb305295c38175b4adabc419aa/parquet/src/file/metadata/reader.rs#L73-L74
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2701251412
##########
parquet/tests/arrow_reader/io/async_reader.rs:
##########
@@ -318,7 +319,9 @@ async fn async_builder(
test_file: &TestParquetFile,
options: ArrowReaderOptions,
) -> ParquetRecordBatchStreamBuilder<RecordingAsyncFileReader> {
- let parquet_meta_data = if options.page_index() {
+ let parquet_meta_data = if options.offset_index_policy() != PageIndexPolicy::Skip
+ || options.column_index_policy() != PageIndexPolicy::Skip
Review Comment:
makes sense to me
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "kczimm (via GitHub)" <gi...@apache.org>.
kczimm commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2699717667
##########
parquet/src/arrow/async_reader/store.rs:
##########
@@ -226,8 +226,12 @@ impl AsyncFileReader for ParquetObjectReader {
// When page_index_policy is Optional or Required, override the preload flags
// to ensure the specified policy takes precedence.
if let Some(options) = options {
- if options.page_index_policy != PageIndexPolicy::Skip {
- metadata = metadata.with_page_index_policy(options.page_index_policy);
+ if options.column_index_policy() != PageIndexPolicy::Skip
Review Comment:
Should this be `&&`, also?
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2700104593
##########
parquet/tests/arrow_reader/io/async_reader.rs:
##########
@@ -318,7 +319,9 @@ async fn async_builder(
test_file: &TestParquetFile,
options: ArrowReaderOptions,
) -> ParquetRecordBatchStreamBuilder<RecordingAsyncFileReader> {
- let parquet_meta_data = if options.page_index() {
+ let parquet_meta_data = if options.offset_index_policy() != PageIndexPolicy::Skip
+ || options.column_index_policy() != PageIndexPolicy::Skip
Review Comment:
My reasoning here was the original `page_index()` returned `page_index_policy != PageIndexPolicy::Skip`. Since `page_index_policy` was split into separate `offset_index_policy` and `column_index_policy`, we should keep the page index if *either* policy is not Skip.
In other words, we only strip the page index when *both* are skip.
But I am happy to make the change if this is the incorrect reasoning
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#issuecomment-3756725466
cc @kczimm @etseidl @alamb
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2700104593
##########
parquet/tests/arrow_reader/io/async_reader.rs:
##########
@@ -318,7 +319,9 @@ async fn async_builder(
test_file: &TestParquetFile,
options: ArrowReaderOptions,
) -> ParquetRecordBatchStreamBuilder<RecordingAsyncFileReader> {
- let parquet_meta_data = if options.page_index() {
+ let parquet_meta_data = if options.offset_index_policy() != PageIndexPolicy::Skip
+ || options.column_index_policy() != PageIndexPolicy::Skip
Review Comment:
my reasoning here was the original `page_index()` returned `page_index_policy != PageIndexPolicy::Skip`. Since `page_index_policy` was split into separate `offset_index_policy` and `column_index_policy`, we should keep the page index if *either* policy is not Skip.
In other words, we only strip the page index when *both* are skip
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2699653544
##########
CHANGELOG.md:
##########
@@ -19,6 +19,12 @@
# Changelog
+## [Unreleased]
+
+**Breaking changes:**
Review Comment:
Technically this is not a breaking change -- as you left the backwards compatibility APIs
##########
parquet/tests/arrow_reader/io/async_reader.rs:
##########
@@ -318,7 +319,9 @@ async fn async_builder(
test_file: &TestParquetFile,
options: ArrowReaderOptions,
) -> ParquetRecordBatchStreamBuilder<RecordingAsyncFileReader> {
- let parquet_meta_data = if options.page_index() {
+ let parquet_meta_data = if options.offset_index_policy() != PageIndexPolicy::Skip
+ || options.column_index_policy() != PageIndexPolicy::Skip
Review Comment:
should this be "&&" in stead of "||"?
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#issuecomment-3764066639
Thanks everyone
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#issuecomment-3761835427
> But it does leave me wondering if we've really got the correct set of policies. For instance, it's perfectly valid for column indexes to be missing for certain pages (all NaN for instance), so would one ever want `PageIndexPolicy::Required` for them? Maybe, maybe not. Not for this PR, but something to think about, especially if we ever finish the conversion of the page indexes to `Vec<Vec<Option<>>>` (see #8818).
It is plausble that having just the page offsets is useful (if you only want rows 100-200, for example) even without the page statistics
I am not sure there is any reasonable usecase to only have page statistics without the offset index 🤔
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199
--
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] Deprecate `ArrowReaderOptions::with_page_index` and update API [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #9199:
URL: https://github.com/apache/arrow-rs/pull/9199#discussion_r2699885817
##########
parquet/src/arrow/async_reader/store.rs:
##########
@@ -226,8 +226,12 @@ impl AsyncFileReader for ParquetObjectReader {
// When page_index_policy is Optional or Required, override the preload flags
// to ensure the specified policy takes precedence.
if let Some(options) = options {
- if options.page_index_policy != PageIndexPolicy::Skip {
- metadata = metadata.with_page_index_policy(options.page_index_policy);
+ if options.column_index_policy() != PageIndexPolicy::Skip
Review Comment:
I think `||` is fine, or just split them up and test/set individually.
--
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