-
Notifications
You must be signed in to change notification settings - Fork 807
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
Support peek_next_page() and skip_next_page in serialized_reader. #2044
Conversation
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.
Looking really good only some minor nits.
// total row count. | ||
num_rows: i64, | ||
|
||
//Page offset index. |
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.
Perhaps we could have something like
enum SerializedPages<T: Read>
/// Read entire chunk
Chunk {
buf: T,
total_num_values: i64,
},
Pages {
index: Vec<PageLocation>,
seen_num_data_pages: usize,
has_dictionary_page_to_read: bool,
page_bufs: VecDeque<T>
}
To make it clear what is present in what mode
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.
Good advice! i will try in this mode.👍
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.
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2044 +/- ##
==========================================
+ Coverage 83.54% 83.57% +0.02%
==========================================
Files 222 223 +1
Lines 58186 58357 +171
==========================================
+ Hits 48612 48769 +157
- Misses 9574 9588 +14
Continue to review full report at Codecov.
|
@@ -481,6 +513,18 @@ pub struct SerializedPageReader<T: Read> { | |||
|
|||
// Column chunk type. | |||
physical_type: Type, | |||
// //Page offset index. |
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.
Do you want to remove these lines?
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.
Thanks!
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.
Nice 👍
Benchmark runs are scheduled for baseline = 9116297 and contender = 5e520bb. 5e520bb is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Thanks ❤️ |
Which issue does this PR close?
Closes #2043.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?