Add protocol validation, rename mod features to table_features
#454
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
github-actions
bot
added
the
breaking-change
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
==========================================
+ Coverage 78.08% 78.20% +0.11%
==========================================
Files 55 55
Lines 11605 11730 +125
Branches 11605 11730 +125
==========================================
+ Hits 9062 9173 +111
- Misses 2043 2056 +13
- Partials 500 501 +1 ☔ View full report in Codecov by Sentry. |
zachschuermann
commented
Nov 5, 2024
zachschuermann
commented
Nov 5, 2024
zachschuermann
commented
Nov 5, 2024
zachschuermann
commented
Nov 5, 2024
| @@ -481,4 +555,66 @@ mod tests { | |||
| )])); | |||
| assert_eq!(schema, expected); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_validate_protocol() { | |||
There was a problem hiding this comment.
Would like to see the feature subset check get exercised. Would also like to see the min_reader_version == 1 or 2 get checked as well.
scovich
reviewed
Nov 7, 2024
|
|
||
| /// Check if reading a table with this protocol is supported. That is: does the kernel support | ||
| /// the specified protocol reader version and all enabled reader features? | ||
| // TODO: should this return result so we can say _why_ it isn't supported? |
| Some(reader_features) if self.min_reader_version == 3 => { | ||
| let parsed_features: Result<HashSet<_>, _> = reader_features | ||
| .iter() | ||
| .map(|s| s.parse::<ReaderFeatures>()) |
There was a problem hiding this comment.
Maybe this is cleaner?
Suggested change
| .map(|s| s.parse::<ReaderFeatures>()) | |
| .map(ReaderFeatures::from_str) |
| match &self.reader_features { | ||
| // check that all reader features are subset of supported | ||
| Some(reader_features) if self.min_reader_version == 3 => { | ||
| let parsed_features: Result<HashSet<_>, _> = reader_features |
There was a problem hiding this comment.
Is it really an arbitrary Result? Or would this work?
Suggested change
| let parsed_features: Result<HashSet<_>, _> = reader_features | |
| let parsed_features: DeltaResult<HashSet<_>> = reader_features |
| } | ||
|
|
||
| impl Protocol { | ||
| pub fn new( |
There was a problem hiding this comment.
Suggested change
| pub fn new( | |
| pub fn try_new( |
Comment on lines
+588
to
+596
| assert!(matches!( | ||
| Protocol::new( | ||
| min_reader_version, | ||
| min_writer_version, | ||
| reader_features, | ||
| writer_features | ||
| ), | ||
| Err(Error::InvalidProtocol(_)), | ||
| )); |
There was a problem hiding this comment.
If you aren't worried about the specific error, can also do:
Protocol::try_new(...).expect_err("invalid protocol")
Comment on lines
+605
to
+606
| Some(vec![ReaderFeatures::V2Checkpoint.to_string()]), | ||
| Some(vec![ReaderFeatures::V2Checkpoint.to_string()]), |
There was a problem hiding this comment.
Since we anyway have a method now, maybe consider having it take:
reader_features: Option<impl` IntoIterator<Item = impl Into<String>>>,
writer_features: Option<impl` IntoIterator<Item = impl Into<String>>>,... and then this code here simplifies to:
Suggested change
| Some(vec![ReaderFeatures::V2Checkpoint.to_string()]), | |
| Some(vec![ReaderFeatures::V2Checkpoint.to_string()]), | |
| Some([&ReaderFeatures::V2Checkpoint]), | |
| Some([&ReaderFeatures::V2Checkpoint]), |
(again below)
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?
Protocolvalidation (as soon as aProtocolis constructed via our visitor, do the following validation:)a. Check that table min reader/writer version <= kernel reader/writer version
b. Fail if V2Checkpoint reader feature is enabled (since we don't yet implement it)
c. Check that reader features and writer features are present for reader version 3 and writer version 7, respectively
VacuumProtocolCheckreader/writer feature (but this is allowed as reader feature since it's a noop so we trivially support it)featuresmodule totable_features(less confusing with cargo's notion of features)This PR affects the following public APIs
New error classes for invalid protocol and unsupported features. Add
VacuumProtocolCheckreader/writer feature. Fields inProtocolare now private.How was this change tested?
new ut