Skip to content
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

ffi: Add support for serializing a KeyValuePairLogEvent as a nlohmann::json object. #512

Merged
merged 34 commits into from
Sep 9, 2024

Conversation

LinZhihao-723
Copy link
Member

Description

Note: this PR should not be merged before #507 and #511
This PR implements a method in KeyValuePairLogEvent to serialize it into a nlohamnn::json object. KeyValuePairLogEvent, as the output of Deserializer in key-value pair IR format, will be processed by the ffi libraries. Each ffi library has to turn this C++ object into some dynamic data structure in their language. Ideally, KeyValuePairLogEvent can be serialized into msgpack byte sequence. However, there are two problems we need to solve:

  1. In the current format, arrays are stored as an unstructured JSON string. We need to parse the JSON array to build the msgpack array.
  2. It isn't that straightforward to use the official C++ msgpack library to construct an in-memory map and then serialize it into bytes.

Therefore, we decided to serialize KeyValuePairLogEvent into nlohmann::json object instead for now. nlohmann::json provides a better support to construct an in-memory map, and also provides methods to serialize the result into either msgpack byte sequence or JSON string.

With this PR, we should have all the necessary pieces to integrate serialization/deserialization features of the key-value pair IR format into our ffi libraries.

Validation performed

  • Add unit test to ensure the deserialized KeyValuePairLogEvent can be successfully serialized into nlohmann::json objects, and be identical with the objects before serialization.

@LinZhihao-723 LinZhihao-723 changed the title [WIP] ffi: Add support to serialize KeyValuePairLogEvent to nlohmann::json object. ffi: Add support to serialize KeyValuePairLogEvent to nlohmann::json object. Sep 3, 2024
Copy link
Contributor

@gibber9809 gibber9809 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me -- just a few minor changes that could help make this code easier to understand.

Per previous discussions this PR is focused on finishing a functional implementation, so discussion about performance is left out of this review.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some renaming suggestions to be more explicit and avoid confusion between the schema tree node and corresponding JSON object. We can discuss it if you think it's too explicit, but I don't think the long names make the code significantly less readable.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the PR title, how about:

ffi: Add support for serializing a KeyValuePairLogEvent as a nlohmann::json object.

@LinZhihao-723 LinZhihao-723 changed the title ffi: Add support to serialize KeyValuePairLogEvent to nlohmann::json object. ffi: Add support for serializing a KeyValuePairLogEvent as a nlohmann::json object. Sep 9, 2024
@LinZhihao-723 LinZhihao-723 merged commit 5b7ab18 into y-scope:main Sep 9, 2024
17 checks passed
gibber9809 added a commit to gibber9809/clp that referenced this pull request Oct 25, 2024
…ann::json` object. (y-scope#512)

Co-authored-by: Devin Gibson <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
…ann::json` object. (y-scope#512)

Co-authored-by: Devin Gibson <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants