Scan::execute takes an Arc<dyn EngineData> now #553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #553 +/- ##
=======================================
Coverage 80.61% 80.61%
=======================================
Files 67 67
Lines 14278 14278
Branches 14278 14278
=======================================
Hits 11510 11510
Misses 2188 2188
Partials 580 580 ☔ View full report in Codecov by Sentry. |
scovich
commented
Nov 28, 2024
Comment on lines
+494
to
495
| read_with_scan_data(table.location(), engine.as_ref(), &scan, &expected)?; | ||
| read_with_execute(engine, &scan, &expected)?; |
There was a problem hiding this comment.
Note: Order swapped so we don't have to clone the engine that read_with_execute consumes
|
Attn @zachschuermann: something went wrong with the semver check, it didn't flag a change to a pub fn signature in a pub struct in a pub mod as a breaking change?? |
hntd187
approved these changes
Dec 2, 2024
nicklan
approved these changes
Dec 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
A previous change made
Scan::executereturn a lazy iterator instead of a collection, but in doing so it captured a&dyn Enginewhich limits the lifetime of the iterator. This is silly, given that the caller has access to anArc<dyn Engine>precisely to avoid lifetime problems.Update the method to take an Arc and fix the compiler carnage that results.
Fixes #551
This PR affects the following public APIs
Scan::executeis public, as are some of the impacted methods that rely on it.How was this change tested?
Compilation suffices (yay for rust borrow checker!)