Add PageIndexBuilder and PageIndexProvider for Parquet page indexes - #10842
Conversation
…o page_index_provider
|
Thanks @alamb! I think I've addressed all of your comments, PTAL when you can. I still need to add issues for the serialization gap and memory accounting. I think the former is a pretty easy fix, but the latter will take some thought (we've gone down this rabbit hole before (e.g. #9138) Edit: I have the serialization ready to go once this merges. Non-breaking so it can wait for 60.1.0. |
|
Thanks for the changes @etseidl |
alamb
left a comment
There was a problem hiding this comment.
Thanks @etseidl -- I went through this one again
I filed some follow on tickets
And then I pushed some commits to:
- Update the TODO comments with links to issues
- Fix some other random small cleanups found while reviewing
I think we are ready now. Maybe you can take one last quick peek and then merge it in!
| #[cfg(not(feature = "encryption"))] | ||
| let encryption_size = 0usize; | ||
|
|
||
| // We can only determine the heap size for PageIndex. Custom providers are |
There was a problem hiding this comment.
|
Thanks @alamb, your changes look good. I'll merge once CI finishes. |
|
Amazing to see this land! @etseidl do you plan to use this to work on alternative storage / representation for page indexes (particularly for wide tables with sparse reads)? |
|
We did it! |
Thanks @adriangb. TBH I haven't really thought that far ahead. I was hoping to get more requirements from the Datafusion side before doing too much further engineering. I'm certainly interested in having other providers in the parquet crate. |
…es (apache#10842) # Which issue does this PR close? - Part of apache#7582 - Closes apache#10824. # Rationale for this change This grew out of a discussion in apache#10784 and relates to apache/datafusion#24288 (comment). This PR provides a new builder for creating page index structures, and also adds a new `PageIndexProvider` trait to allow more performant implementations. # What changes are included in this PR? Adds `PageIndexBuilder`, `PageIndexProvider`, implements `PageIndexProvider` for `PageIndex`, and adds `RowGroupPageIndex` as a helper to support fetching page indexes for a specific row group (replaces the `X_index_for_rowgroup()` functions on `PageIndex`). # Are these changes tested? Yes, should be covered by existing tests # Are there any user-facing changes? Yes, this changes the public API for accessing page index information Created with the aid of Claude Code, but I own the changes. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…es (apache#10842) # Which issue does this PR close? - Part of apache#7582 - Closes apache#10824. # Rationale for this change This grew out of a discussion in apache#10784 and relates to apache/datafusion#24288 (comment). This PR provides a new builder for creating page index structures, and also adds a new `PageIndexProvider` trait to allow more performant implementations. # What changes are included in this PR? Adds `PageIndexBuilder`, `PageIndexProvider`, implements `PageIndexProvider` for `PageIndex`, and adds `RowGroupPageIndex` as a helper to support fetching page indexes for a specific row group (replaces the `X_index_for_rowgroup()` functions on `PageIndex`). # Are these changes tested? Yes, should be covered by existing tests # Are there any user-facing changes? Yes, this changes the public API for accessing page index information Created with the aid of Claude Code, but I own the changes. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
PageIndexcannot be constructed outside the crate #10824.Rationale for this change
This grew out of a discussion in #10784 and relates to apache/datafusion#24288 (comment). This PR provides a new builder for creating page index structures, and also adds a new
PageIndexProvidertrait to allow more performant implementations.What changes are included in this PR?
Adds
PageIndexBuilder,PageIndexProvider, implementsPageIndexProviderforPageIndex, and addsRowGroupPageIndexas a helper to support fetching page indexes for a specific row group (replaces theX_index_for_rowgroup()functions onPageIndex).Are these changes tested?
Yes, should be covered by existing tests
Are there any user-facing changes?
Yes, this changes the public API for accessing page index information
Created with the aid of Claude Code, but I own the changes.