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 2026/08/11 21:30:56 UTC
[PR] parquet: Convert page indexes to `Vec>>` [arrow-rs]
etseidl opened a new pull request, #10653:
URL: https://github.com/apache/arrow-rs/pull/10653
# Which issue does this PR close?
- Closes #8818.
# Rationale for this change
See issue, but in short the page indexes should allow for missing column and page indexes in a more natural way.
# What changes are included in this PR?
The major change is converting `ParquetOffsetIndex` from `Vec<Vec<OffsetIndexMetaData>>` to `Vec<Vec<Option<OffsetIndexMetaData>>>` and `ParquetColumnIndex` from `Vec<Vec<ColumnIndexMetaData>>` to `Vec<Vec<Option<ColumnIndexMetaData>>>`.
Then needed changes to tests and places where these indexes are used.
# Are these changes tested?
Should be covered by existing tests
# Are there any user-facing changes?
Yes, the page indexes are public.
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#discussion_r3761966061
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -580,14 +527,8 @@ impl MetadataObjectWriter {
_column_idx: usize,
sink: impl Write,
) -> Result<bool> {
Review Comment:
This and `write_column_index` below could be refactored to return `Result<()>`
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#issuecomment-5348655061
👁️ (so much for being fresh!) - but I am checking this one out 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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#issuecomment-5272446566
> It also might be worth doing this in two PRs...this one to effect the additional Option change, and then a second to introduce a new PageIndex struct.
I like this idea -- and it keeps the changes somewhat scoped and doesn't require a refactor the world type PR)
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#discussion_r3769801874
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter;
/// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a
/// particular column chunk.
///
-/// `column_index[row_group_number][column_number]` holds the
+/// `column_index[row_group_number][column_number]` holds the optional
/// [`ColumnIndex`] corresponding to column `column_number` of row group
-/// `row_group_number`.
+/// `row_group_number`. This will be `None` if no index is present for the given
+/// column chunk.
///
/// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth
/// column in the third row group of the parquet file.
///
/// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
/// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData
-pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>;
+pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>;
Review Comment:
I have always found this structure to be very confusing (as it is a Vec of Vecs). Adding Option makes it even more confusing in my mind.
What would you think about at least encapsulating the PageIndex into a structure of its own (rather than two parallel structure)?
```rust
struct PageIndex {
column_indexes: Vec<Vec<Option<ColumnIndexMetaData>>>,
offset_indexes: Vec<Vec<Option<OffsetIndexMetaData>>>,
}
```
🤔
Then we could add accessors like
```rust
if let Some(offset_index) = page_index.offset_index(rg_idx) {
// use offset index for rg_idx
}
```
That might also allow us to tweak the internal representation of these indexes to support options, etc without breaking the structure again
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter;
/// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a
/// particular column chunk.
///
-/// `column_index[row_group_number][column_number]` holds the
+/// `column_index[row_group_number][column_number]` holds the optional
/// [`ColumnIndex`] corresponding to column `column_number` of row group
-/// `row_group_number`.
+/// `row_group_number`. This will be `None` if no index is present for the given
+/// column chunk.
///
/// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth
/// column in the third row group of the parquet file.
///
/// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
/// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData
-pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>;
+pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>;
-/// [`OffsetIndexMetaData`] for each data page of each row group of each column
+/// [`OffsetIndexMetaData`] for each column chunk of each row group
///
/// This structure is the parsed representation of the [`OffsetIndex`] from the
/// Parquet file footer, as described in the Parquet [PageIndex documentation].
///
/// `offset_index[row_group_number][column_number]` holds
-/// the [`OffsetIndexMetaData`] corresponding to column
-/// `column_number`of row group `row_group_number`.
+/// the optional [`OffsetIndexMetaData`] corresponding to column
+/// `column_number`of row group `row_group_number`. This will be `None` if no index
+/// is present for the given column chunk.
///
/// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
/// [`OffsetIndex`]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
-pub type ParquetOffsetIndex = Vec<Vec<OffsetIndexMetaData>>;
+pub type ParquetOffsetIndex = Vec<Vec<Option<OffsetIndexMetaData>>>;
Review Comment:
If we are going to change the page index representation (and force a breaking change downstream on the users) I wonder if we can think bigger than just adding an Option here and making it align with the parquet-format names
For example what do you think about making it a struct so that we have a better chance of evolving it over time (and make it easier to document)?
For example:
https://github.com/apache/parquet-format/blob/2076361bb64e2de9ca6a8d06eda025a6fa4e9df6/src/main/thrift/parquet.thrift#L1261
```rust
struct ParquetOffsetIndex {
page_locations: Vec<ParquetPageLocation>,
unencoded_byte_array_data_bytes: Option<Vec<i64>,
}
```
And https://github.com/apache/parquet-format/blob/2076361bb64e2de9ca6a8d06eda025a6fa4e9df6/src/main/thrift/parquet.thrift#L1236
```rust
struct ParquetPageLocation {
offset: i64,
compressed_page_size: i32,
first_row_index: i64,
}
```
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter;
/// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a
/// particular column chunk.
///
-/// `column_index[row_group_number][column_number]` holds the
+/// `column_index[row_group_number][column_number]` holds the optional
/// [`ColumnIndex`] corresponding to column `column_number` of row group
-/// `row_group_number`.
+/// `row_group_number`. This will be `None` if no index is present for the given
+/// column chunk.
///
/// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth
/// column in the third row group of the parquet file.
///
/// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
/// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData
-pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>;
+pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>;
-/// [`OffsetIndexMetaData`] for each data page of each row group of each column
+/// [`OffsetIndexMetaData`] for each column chunk of each row group
///
/// This structure is the parsed representation of the [`OffsetIndex`] from the
/// Parquet file footer, as described in the Parquet [PageIndex documentation].
///
/// `offset_index[row_group_number][column_number]` holds
-/// the [`OffsetIndexMetaData`] corresponding to column
-/// `column_number`of row group `row_group_number`.
+/// the optional [`OffsetIndexMetaData`] corresponding to column
+/// `column_number`of row group `row_group_number`. This will be `None` if no index
+/// is present for the given column chunk.
///
/// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
/// [`OffsetIndex`]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
-pub type ParquetOffsetIndex = Vec<Vec<OffsetIndexMetaData>>;
+pub type ParquetOffsetIndex = Vec<Vec<Option<OffsetIndexMetaData>>>;
Review Comment:
We could also just do something slightly more encapsulated rather than a typedef
```rust
struct ParquetOffsetIndex {
inner: Vec<Vec<Option<OffsetIndexMetaData>>>;
}
```
🤔
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#issuecomment-5259363203
@alamb this is me scratching an old itch. I think it's ready to go, but am not sure what the downstream implications are. Let me know if you think this can go into 60.0.0.
Note that most of the changes are either highly repetitive (like in statistics.rs) or changes to make tests work. The actual code changes are pretty small.
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#issuecomment-5272274195
Thanks @alamb. Let me think some about your suggestions. I'm all for further encapsulating things as I too struggle with these two structures. My only hesitation is how that will impact the stats converter, but I think it will be fine so long as the converter has access to the vecs.
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#issuecomment-5266197308
Thanks -- I will check this out later today
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#discussion_r3761972482
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter;
/// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a
/// particular column chunk.
///
-/// `column_index[row_group_number][column_number]` holds the
+/// `column_index[row_group_number][column_number]` holds the optional
/// [`ColumnIndex`] corresponding to column `column_number` of row group
-/// `row_group_number`.
+/// `row_group_number`. This will be `None` if no index is present for the given
+/// column chunk.
///
/// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth
/// column in the third row group of the parquet file.
///
/// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md
/// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData
-pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>;
+pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>;
Review Comment:
The changes here are the big change...the rest is dealing with the consequences
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #10653:
URL: https://github.com/apache/arrow-rs/pull/10653#discussion_r3761977630
##########
parquet/src/arrow/arrow_reader/statistics.rs:
##########
@@ -678,14 +678,14 @@ macro_rules! get_data_page_statistics {
$values_iter: ident,
$page_statistics: ident
) => {{
- let chunks: Vec<(usize, &ColumnIndexMetaData)> = $iterator.collect();
+ let chunks: Vec<(usize, Option<&ColumnIndexMetaData>)> = $iterator.collect();
let capacity: usize = chunks.iter().map(|c| c.0).sum();
match $data_type {
DataType::Boolean => {
let mut b = BooleanBuilder::with_capacity(capacity);
for (len, index) in chunks {
match index {
- ColumnIndexMetaData::BOOLEAN(index) => {
+ Some(ColumnIndexMetaData::BOOLEAN(index)) => {
Review Comment:
There is a great deal of repetition in this module
--
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] parquet: Convert page indexes to `Vec>>` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl closed pull request #10653: parquet: Convert page indexes to `Vec<Vec<Option<T>>>`
URL: https://github.com/apache/arrow-rs/pull/10653
--
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