parquet: Add new PageIndex struct to encapsulate column and offset indexes - #10719
parquet: Add new PageIndex struct to encapsulate column and offset indexes#10719etseidl wants to merge 25 commits into
PageIndex struct to encapsulate column and offset indexes#10719Conversation
49cbf86 to
7f47fac
Compare
|
Shal I give this one a look? |
Yes please. I think there's a todo left and it needs some better docs, but if you could take a look and see if you think this is the direction you wanted to head I'd appreciate it. |
|
Thank you -- I will do so but probably not until tomorrow (I need a clear mind) |
| let page_locations = self | ||
| .metadata | ||
| .page_index() | ||
| .map(|i| i.page_locations(rg_idx, self.column_idx).cloned()) |
There was a problem hiding this comment.
this clone is unfortunate (it clones all the page locations into a new Vec) -- I realize it is what the previous code did, but I wonder if there is some way to avoid it
It may also be related to
Where @zhuqi-lucas and others have been looking for a way to load some but not all page indexes (or load them on demand, from a cache, etc).
Maybe it is time to sprinkle on some Arc 🤔
There was a problem hiding this comment.
Let me see what I can do here...tracing the page_locations down it looks like they are copied once again deep down in the page reader. Might be able to pass a reference here instead.
There was a problem hiding this comment.
Where @zhuqi-lucas and others have been looking for a way to load some but not all page indexes (or load them on demand, from a cache, etc).
I think the new form will help with that...I envision a builder that starts out with empty vecs sized with num_row_groups and num_columns. Individual cells can then be populated based on what it needed for a given use. We can pop back and forth between page index and page index builder if need be.
| &self, | ||
| column_page_index: &ParquetColumnIndex, | ||
| column_offset_index: &ParquetOffsetIndex, | ||
| page_index: &PageIndex, |
| /// # Structure | ||
| /// | ||
| /// Both indexes are organized as a two-level structure: | ||
| /// - First level: indexed by row group number |
There was a problem hiding this comment.
👍
(though this might end up being an internal implementation detail that would be better put closer to the field definitions)
| /// # use parquet::errors::Result; | ||
| /// | ||
| /// /// Identifies which pages in a column might contain values >= min_value | ||
| /// fn find_relevant_pages( |
There was a problem hiding this comment.
this is a (very) cool example
| self.column_indexes.is_some() | ||
| } | ||
|
|
||
| /// Returns column indexes for all columns in the specified row group |
There was a problem hiding this comment.
these accessors are (so) much easier to read in my mind
| /// [`OffsetIndex`]: crate::file::page_index::offset_index::OffsetIndexMetaData | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct PageIndex { | ||
| column_indexes: Option<Vec<Vec<Option<ColumnIndexMetaData>>>>, |
There was a problem hiding this comment.
Given the comment above about cloning / handling incremental index loading, we might consider
column_indexes: Option<Vec<Arc<[Option<ColumnIndexMetaData>]>>So that the indexes could be returned as a &Arc<[Option<ColumnIndexMetaData]> (and thus cheaply cloned) as well as potenitally being able to reuse existing column metadata)
However I am not sure it really matters and don't feel super strongly
alamb
added
the
next-major-release
|
Some findings from a test upgrade Specifically this commit: apache/datafusion@419027d This pattern comes up a bunch (to see if the index is "complete" it has to test both column_indexes and offset_indexes. metadata
.page_index()
.is_some_and(|index| index.has_column_indexes() && index.has_offset_indexes())Maybe we could add a helper like PageIndex::is_complete that does the check and we could simplify to metadata
.page_index()
.is_some_and(PageIndex::is_complete)But otherwise the changes needed are pretty strightforward |
That's a good idea...there are a bunch of tests too that do the same thing. I'll get to that |
|
😍 |
|
Thanks for the review @alamb. I think this is ready to merge now. I'm going to get started on the builder to prototype partial population of the indexes. |
2 participants
Which issue does this PR close?
ParquetMetaData#8818.Vec<Vec<Option<T>>>#10653Note: this is stacked on #10653
Rationale for this change
Following up on #10653 (review)
What changes are included in this PR?
Try to hide some of the complexity of the page indexes behind a struct with accessors for access by row group, or row group and column.
Are these changes tested?
Should be covered by existing
Are there any user-facing changes?
Yes, this changes the public interface to the page indexes quite a bit.