Minor: refactor ParquetExec roundtrip tests #5260
Conversation
github-actions
bot
added
core
Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
| } | ||
|
|
||
| /// run the test, returning the `RoundTripResult` | ||
| async fn round_trip(self, batches: Vec<RecordBatch>) -> RoundTripResult { |
There was a problem hiding this comment.
I think whitespace blind diff shows the changes more clearly: https://github.com/apache/arrow-datafusion/pull/5260/files?w=1
|
|
||
| // If testing with page_index_predicate, write parquet | ||
| // files with multiple pages | ||
| let multi_page = page_index_predicate; |
There was a problem hiding this comment.
I found this logic slightly confusing so I added some comments and tried to make it clearer -- no functional change is intended
| @@ -1121,8 +1158,12 @@ mod tests { | |||
| let filter = col("c2").eq(lit(2_i64)).or(col("c2").eq(lit(1_i64))); | |||
|
|
|||
| // read/write them files: | |||
| let rt = | |||
| round_trip(vec![batch1, batch2], None, None, Some(filter), true, false).await; | |||
| let rt = RoundTrip::new() | |||
There was a problem hiding this comment.
I think this construction is clearer about what combination of options are being tested
|
Oops I hit merge with a fmt issue. Will fix |
|
Benchmark runs are scheduled for baseline = 00e60f1 and contender = 9710d7c. 9710d7c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
fmt fix in #5263 |
* Minor: refactor ParquetExec roundtrip tests * Apply suggestions from code review Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> --------- Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Which issue does this PR close?
Part of #5104
Rationale for this change
I am trying to write a test for #5104 and getting lost in the maze of parameters passed to
round_tripandround_trip_to_parquetIt is also hard for me to evaluate which combinations of tests are covered
What changes are included in this PR?
Move the parameters to run the roundtrip test into a structure, with better documentation
Are these changes tested?
Yes
Are there any user-facing changes?
no