refactor: add ParquetOpenerBuilder to reduce test code duplication #19405
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the ParquetOpener test suite to eliminate substantial code duplication by introducing a builder pattern. The main improvement is the addition of a test-only ParquetOpenerBuilder that provides sensible defaults and fluent methods for configuring ParquetOpener instances, reducing each test's setup from ~28 lines to ~6-8 lines.
- Added
ParquetOpenerBuilderstruct with default values matching original test patterns - Refactored 8 test functions to use the builder pattern, making test intent clearer
- Maintained test behavior while significantly reducing code duplication and improving maintainability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @ShashidharM0118 -- this looks great. there is just one small suggestion, but otherwise it looks great to me
FYI @xudong963 and @adriangb
| max_predicate_cache_size: None, | ||
| reverse_row_groups: false, | ||
| } | ||
| ParquetOpenerBuilder::new() |
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Small nit about the schemas but otherwise looks good, thank you! I've wanted this every time I looked at these tests but just never had the bandwith to do it.
TableSchema already contains file_schema internally, so storing it separately in the builder was redundant. This simplifies the builder by removing duplicate state and ensures file_schema is always derived from table_schema in build().
shashidhar-bm
force-pushed
the
add-parquet-opener-builder
branch
from
b139c4a to
e6ec5f9
Compare
3 participants
Which issue does this PR close?
Rationale for this change
The test suite for
ParquetOpenerexhibited substantial code duplication across multiple test functions. Each test was constructingParquetOpenerinstances with largely identical field values, resulting in verbose and repetitive code that hindered maintainability and obscured the distinguishing characteristics of each test case.What changes are included in this PR?
ParquetOpenerBuilderstruct in the test module (#[cfg(test)]) with sensible defaults matching the original test code patternstest_prune_on_statisticstest_prune_on_partition_statistics_with_dynamic_expressiontest_prune_on_partition_values_and_file_statisticstest_prune_on_partition_value_and_data_valuetest_opener_pruning_skipped_on_static_filterstest_reverse_scan_row_groupstest_reverse_scan_single_row_grouptest_reverse_scan_with_row_selectionAre these changes tested?
Yes
Are there any user-facing changes?
No