-
Notifications
You must be signed in to change notification settings - Fork 793
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
feat:Add function for row alignment with page mask #1791
Merged
Merged
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
116fcbb
add range and rowRanges
Ted-Jiang 0f23f20
add some tests for range
Ted-Jiang e9c98c0
add filter logic
Ted-Jiang 761c929
fix fmt
Ted-Jiang be38829
fix todo
Ted-Jiang a4c9e60
fix test
Ted-Jiang f57d7e0
Apply suggestions from code review
Ted-Jiang 8329f21
fix compute_row_ranges
Ted-Jiang d84b337
fix annotation
Ted-Jiang adc48cf
change to use std:ops:RangeInclusive
Ted-Jiang fc61155
Apply suggestions from code review
Ted-Jiang ce8aec2
fix
Ted-Jiang 81292d5
Update parquet/src/file/page_index/range.rs
Ted-Jiang 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 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 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.
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.
The more I think about this the more I wonder whether the metadata structs are the right place to put the index information. They're parsed and interpreted separately from the main metadata, and so I think it makes sense for them to be stored separately?
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.
Yes, you are right. page index stored in file-meta level.
My thought is read less pageIndex after rowgroup filter
arrow-rs/parquet/src/file/serialized_reader.rs
Lines 211 to 224 in be38829
arrow-rs/parquet/src/file/serialized_reader.rs
Lines 246 to 249 in be38829
So i want to read index here and insert it into RowGroupMetaData.
It was just a simple idea at first, maybe we can find a better way in the process of implementation
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.
It isn't even file-meta level, it isn't part of the footer but stored as separate pages 😅
Provided we take care to ensure we keep things pub(crate) so we don't break APIs, this seems like a good strategy 👍
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.
yes separately from RowGroup, before the footer !😂