ffi: Add Deserializer class to deserialize log events from key-value pair IR format.#511
Merged
Merged
Conversation
Deserializer class to deserialize log events from key-value pair IR format.Deserializer class to deserialize log events from key-value pair IR format.
kirkrodrigues
requested changes
Aug 19, 2024
kirkrodrigues
left a comment
Member
There was a problem hiding this comment.
Paused my review so you can take a look at the docstrings again and make sure they are complete and follow our conventions.
Comment on lines
+17
to
+19
| * This class: | ||
| * - maintains all the necessary internal data structure to track deserialization state; | ||
| * - provide APIs to deserialize log events from an IR stream. |
Member
There was a problem hiding this comment.
I think this is obvious, right?
Member
Author
There was a problem hiding this comment.
Right. I've added more details to explain the APIs should work as a transaction
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
requested changes
Aug 23, 2024
| [[nodiscard]] auto deserialize_schema( | ||
| ReaderInterface& reader, | ||
| encoded_tag_t& tag, | ||
| std::vector<SchemaTreeNode::id_t>& schema |
Member
There was a problem hiding this comment.
I think for someone new to this code, schema might be a confusing/overloaded term (and it's not something that we've documented well in SchemaTree). So perhaps we should create a type alias and add a docstring for it explaining that it's a collection of schema tree leaf node IDs that represents the schema of a KV-pair log event.
| } | ||
|
|
||
| auto schema_tree_node_tag_to_type(encoded_tag_t tag) -> std::optional<SchemaTreeNode::Type> { | ||
| std::optional<SchemaTreeNode::Type> type; |
Member
There was a problem hiding this comment.
I think SchemaTreeNode::Type is a trivial type, so could we return it directly from the method without triggering a copy?
Comment on lines
+399
to
+410
| encoded_tag_t str_packet_tag{}; | ||
| if (auto const err{deserialize_tag(reader, str_packet_tag)}; | ||
| IRErrorCode::IRErrorCode_Success != err) | ||
| { | ||
| return err; | ||
| } | ||
| std::string key_name; | ||
| if (auto const err{deserialize_string(reader, str_packet_tag, key_name)}; | ||
| IRErrorCode::IRErrorCode_Success != err) | ||
| { | ||
| return err; | ||
| } |
Member
There was a problem hiding this comment.
Can we extract this into a method? Then you also wouldn't need the comment.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
requested changes
Aug 25, 2024
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
approved these changes
Aug 25, 2024
Member
Author
|
The centOS build failed due to #521 and it is irrelevant to this PR. |
jackluo923
pushed a commit
to jackluo923/clp
that referenced
this pull request
Dec 4, 2024
…e pair IR format. (y-scope#511) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
junhaoliao
pushed a commit
to junhaoliao/clp
that referenced
this pull request
May 17, 2026
…e pair IR format. (y-scope#511) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
This PR implements
Deserializerto deserialize log events from a key-value pair IR format.Deserializeris a class that stores all the necessary states of a key-value pair IR stream (currently including the schema tree and the UTC offset). It also provides API to read until the next key-value pair log event. The read operation is designed to be a transaction: if a failure happens during the deserialization, all the states will be reverted.Notice that the class is designed to take any arbitrary
ReadInterfaceto read bytes from a stream. The caller needs to maintain the stream to get the expected deserialization results. Due to the current limitations on our ffi layer design, we can't bind this class to one particular reader until all ffi libraries have implemented their reader wrapper.Validation performed