Cleanup record skipping logic and tests (#2158)#2199
Conversation
| let test_file = File::open(&path).unwrap(); | ||
| /// Given a RecordBatch containing all the column data, return the expected batches given | ||
| /// a `batch_size` and `selection` | ||
| fn get_expected_batches( |
| fn set_column_reader<V, CV>( | ||
| /// Returns the number of records skipped, which can be less than `batch_size` if | ||
| /// pages is exhausted | ||
| fn skip_records<V, CV>( |
There was a problem hiding this comment.
I found this method somewhat confusing, and I think it could potentially get stuck if the column reader was set but exhausted (i.e. on a column chunk boundary). #2198 will add some test coverage of this case, and this just copies the logic from read_records above, which is correct.
There was a problem hiding this comment.
Confirmed this to be a bug fixed by this PR in #2201
Codecov Report
@@ Coverage Diff @@
## master #2199 +/- ##
==========================================
- Coverage 82.60% 82.58% -0.02%
==========================================
Files 239 239
Lines 62255 62188 -67
==========================================
- Hits 51428 51361 -67
Misses 10827 10827
Help us with your feedback. Take ten seconds to tell us how you rate us. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
@Ted-Jiang can you please review this PR?
| ); | ||
| total_row_count += batch_size; | ||
| } else if num == selection_len { | ||
| let do_test = |batch_size: usize, selection_len: usize| { |
Ted-Jiang
left a comment
Ted-Jiang
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup 😊
|
Benchmark runs are scheduled for baseline = 9e47779 and contender = cc96687. cc96687 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
5 participants
Which issue does this PR close?
Follows on from #2158
Part of #2198
Rationale for this change
Some miscellaneous cleanups to make the tests slightly easier to follow, and to allow for future test integration.
What changes are included in this PR?
Are there any user-facing changes?
No