Simplify parquet filter predicate test, fix Page Filtering Incorrectly Handles Pages with Different Row Counts #4743
Conversation
|
|
||
| let mut set = tokio::task::JoinSet::new(); |
There was a problem hiding this comment.
I reverted the JoinSet change as the test now runs in under 2 seconds, and it made debugging test failures difficult as the stack trace pointed to where the JoinHandle is unwrapped, not the case that actually failed. It also made the failures non-deterministic, which was also annoying
There was a problem hiding this comment.
Looks good to me. It also takes me some time to figure out which sub-case fails. If the time is greatly shortened, I agree we need not pay for the parallel
tustvold
changed the title
| @@ -251,7 +251,7 @@ fn prune_pages_in_one_row_group( | |||
| let mut sum_row = *row_vec.first().unwrap(); | |||
| let mut selected = *values.first().unwrap(); | |||
| trace!("Pruned to to {:?} using {:?}", values, pruning_stats); | |||
| for (i, &f) in values.iter().skip(1).enumerate() { | |||
| for (i, &f) in values.iter().enumerate().skip(1) { | |||
There was a problem hiding this comment.
This is the cause of #4744
tustvold
force-pushed
the
simplify-filter-predicate-test
branch
from
36dcfd0 to
6654e44
Compare
alamb
changed the title
|
|
||
| let mut set = tokio::task::JoinSet::new(); |
| .with_filter( | ||
| conjunction(vec
|
I'm looking into the other test failures, I'm hoping the tests are wrong |
| // vec.push(RowSelector::skip(894)); | ||
| // vec.push(RowSelector::select(339)); | ||
| // vec.push(RowSelector::skip(3330)); | ||
| // `month = 1` or `month = 2` from the page index should create below RowSelection |
There was a problem hiding this comment.
These tests were just wrong, I have fixed them using apache/arrow-rs#3405 to verify that they are correct
|
Benchmark runs are scheduled for baseline = 3abbffb and contender = 40bf559. 40bf559 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4744
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?