expr: Add protobuf for chrono types #11801
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
|
I did not yet change the existing Row encoding. As I split types into Date and Time, it will definitely change their binary encoding. |
aalexandrov
approved these changes
Apr 15, 2022
There was a problem hiding this comment.
@lluki I am going to merge this after squashing the following fixup commits in order to unblock progress on the #11745 sub-issues:
- 49c732d
DateTime<Utc>andNaiveDateTimecan share the same protobuf representation. - c7a0eba Increase the number of test cases to 4096 from the default 256.
- ca219ca + bf7aa09 Move custom
propteststrategy definitions for allchronotypes fromcrate::proto::chronotocrate::chrono(the former is intended to be used only for$T ⇔ Proto$Ttype conversions).
I will keep the commits around in aalexandrov/platform_expr_proto_datetime in case you have objections and want to revert some of these changes when you are back.
aalexandrov
force-pushed
the
platform_expr_proto_datetime
branch
2 times, most recently
from
783116b to
e819d1f
Compare
- Implement `ProtoRepr` for the various `chrono` that are transitively needed for the protobuf-ization of `ComputeCommand`. - Add custom `proptest::strategy::Strategy` implementations for those types. - Add `Proto$T` and conversion trait implementations for some of the types located in `mz_repr::datetime`.
aalexandrov
force-pushed
the
platform_expr_proto_datetime
branch
from
e819d1f to
d02d3fa
Compare
aalexandrov
added
C-feature
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.
Add Protobuf support for chrono::types:
NaiveDate,NaiveTime,NaiveDatetime,DateTime<Utc>,FixedOffsetand,DateTimeUnits.Fixes MaterializeInc/database-issues#3439
Tracked in MaterializeInc/database-issues#3419