-
Notifications
You must be signed in to change notification settings - Fork 139
refactor!: update ScanMetadata to struct with new FilteredEngineData type
#768
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
refactor!: update ScanMetadata to struct with new FilteredEngineData type
#768
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #768 +/- ##
=======================================
Coverage 84.87% 84.88%
=======================================
Files 83 83
Lines 20316 20311 -5
Branches 20316 20311 -5
=======================================
- Hits 17244 17241 -3
- Misses 2221 2222 +1
+ Partials 851 848 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ScanData struct with new FilteredEngineData typeScanData to struct with new FilteredEngineData type
kernel/src/scan/mod.rs
Outdated
| let scan_data = res?; | ||
| let (data, sel_vec) = scan_data.filtered_data; | ||
| let scan_files = vec![]; | ||
| state::visit_scan_files( |
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.
Should we have a visit_scan_data_files or similar that just takes the ScanData? Then we don't have to do this decomposition all over the place
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.
How about updating visit_scan_files to just take ScanData?
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.
discussed a little offline: i'm kinda partial to a ScanData.visit(callback, context)?
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.
Implemented @zachschuermann 's approach
zachschuermann
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.
few comments. and we need to ensure all the breaking changes are clearly spelled out in PR description
kernel/src/scan/mod.rs
Outdated
| let scan_data = res?; | ||
| let (data, sel_vec) = scan_data.filtered_data; | ||
| let scan_files = vec![]; | ||
| state::visit_scan_files( |
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.
discussed a little offline: i'm kinda partial to a ScanData.visit(callback, context)?
scovich
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.
Approach LGTM. We just need to clean up all the nits.
nicklan
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, just one small nit
zachschuermann
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 after a quick rename
## What changes are proposed in this pull request? Rename `ScanData` to `ScanMetadata` and `Scan::scan_data` to `Scan::scan_metadata` (and corresponding FFI). Additionally, renames `TableChangesScanData` to `TableChangesScanMetadata`. Additional docs/refactor coming in #768 ### This PR affects the following public APIs breaking changes: 1. rename `ScanData` to `ScanMetadata` 2. rename `Scan::scan_data()` to `Scan::scan_metadata()` 3. (ffi) rename `free_kernel_scan_data()` to `free_scan_metadata_iter()` 4. (ffi) rename `kernel_scan_data_next()` to `scan_metadata_next()` 5. (ffi) rename `visit_scan_data()` to `visit_scan_metadata()` 6. (ffi) rename `kernel_scan_data_init()` to `scan_metadata_iter_init()` 7. (ffi) rename `KernelScanDataIterator` to `ScanMetadataIterator` 8. (ffi) rename `SharedScanDataIterator` to `SharedScanMetadataIterator` ## How was this change tested? existing resolves #816
ScanData to struct with new FilteredEngineData typeScanMetadata to struct with new FilteredEngineData type
What changes are proposed in this pull request?
ScanMetatafrom typed tuple to struct. ScanMetadata is now a struct with fields:FilteredEngineDatainstance.Introduction of
FilteredEngineDatatype:Couples
EngineDatawith a selection vector indicating which rows to process.This type is returned from the
scan_metadataAPI and the incomingcheckpointAPIUpdates
visit_scan_filesparameters to acceptScanMetadatato avoid de-structuring.Corresponding FFI changes for
visit_scan_filesto acceptScanMetadataparamAll current tests pass.