-
Notifications
You must be signed in to change notification settings - Fork 1.3k
parquet: Add new PageIndex struct to encapsulate column and offset indexes
#10719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
etseidl
wants to merge
25
commits into
apache:main
Choose a base branch
from
etseidl:new_page_index
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
1f5d629
convert page indexes to Vec<Vec<Option<T>>>
etseidl 250644d
remove the NONE variant from ColumnIndexMetaData
etseidl 511ffba
doc fix
etseidl 03cbefc
Merge branch 'main' into opt_page_index
etseidl 2a93a11
Merge remote-tracking branch 'origin/main' into opt_page_index
etseidl 6a45480
Merge remote-tracking branch 'origin/main' into opt_page_index
etseidl 158ab8f
clippy
etseidl 965c6a0
checkpoint
etseidl 80a16ad
checkpoint
etseidl b410401
basic tests pass
etseidl 1794e41
fix test
etseidl b971e38
fix benches and bin
etseidl 78d869a
fix more tests
etseidl a7e424a
fix doc test
etseidl b9bf6e2
fix test for missing col idx
etseidl 39469db
clippy
etseidl c76b016
doc fixes
etseidl c47386d
remove fixmes
etseidl 7f47fac
clippy
etseidl c6edf24
claude some documentation
etseidl 4315c9f
fix test todo
etseidl ae3d0d2
add PageIndex::is_complete per review suggestion
etseidl 50041c8
Merge branch 'main' into new_page_index
etseidl 04b8f67
fix merge
etseidl a4b7303
reword docs to be less implementation specific
etseidl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.