-
Notifications
You must be signed in to change notification settings - Fork 1k
Clean up predicate_cache tests #8755
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
Conversation
| let test = ParquetPredicateCacheTest::new().with_expected_records_read_from_cache(49); | ||
| let async_builder = test.async_builder().await; | ||
| let async_builder = test.add_project_ab_and_filter_b(async_builder); | ||
| let async_builder = test.async_builder().await.add_project_ab_and_filter_b(); |
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.
This is the key change -- move the logic to add specific filters to an extension trait rather than a methog on the ParquetPredicateCacheTest test fixture
185af9f to
fbfc545
Compare
| .unwrap() | ||
| } | ||
|
|
||
| /// Return a [`ParquetRecordBatchReaderBuilder`] for reading the file with |
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.
this code was moved to a new trait -- but the logic is the same
etseidl
left a comment
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.
LGTM
|
Thank you @etseidl |
Which issue does this PR close?
test_cache_projection_excludes_nested_columnsto use high level APIs #8754Rationale for this change
While working on #8754 I found the current formulation a bit akward so let's fix that
What changes are included in this PR?
Move the builder configuration to a trait so the tests read more fluently.
This is totally unecessary, it just makes me feel better about the tests
Are these changes tested?
Yes by CI
Are there any user-facing changes?
No this is test only