feat(ffi): Add support for serializing/deserializing user-defined stream-level metadata. #677
Conversation
WalkthroughThis pull request introduces metadata handling capabilities to the IR stream serialization and deserialization process. The changes span multiple files in the Changes
Possibly Related PRs
Suggested Reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)
552-557: Consider using string_view for key lookup.While the implementation is correct, consider using
string_viewinstead of constructing a temporary string for the metadata key.- metadata.emplace( - string{cProtocol::Metadata::UserDefinedMetadataKey}, - std::move(optional_user_defined_metadata.value()) - ); + metadata.emplace( + cProtocol::Metadata::UserDefinedMetadataKey, + std::move(optional_user_defined_metadata.value()) + );components/core/tests/test-ir_encoding_methods.cpp (1)
1314-1320: Consider adding more test cases for metadata validation.While the basic metadata verification is good, consider adding test cases for:
- Empty metadata
- Metadata with special characters
- Metadata with deeply nested structures
- Metadata size limits
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/core/src/clp/ffi/ir_stream/Deserializer.hpp(2 hunks)components/core/src/clp/ffi/ir_stream/Serializer.cpp(4 hunks)components/core/src/clp/ffi/ir_stream/Serializer.hpp(2 hunks)components/core/src/clp/ffi/ir_stream/protocol_constants.hpp(1 hunks)components/core/tests/test-ir_encoding_methods.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
components/core/src/clp/ffi/ir_stream/protocol_constants.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/test-ir_encoding_methods.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (9)
components/core/src/clp/ffi/ir_stream/protocol_constants.hpp (1)
35-36: LGTM!The new constant follows the established naming convention and uses the appropriate type for string constants.
components/core/src/clp/ffi/ir_stream/Serializer.hpp (2)
5-5: LGTM!The added headers are necessary for the new functionality.
Also applies to: 10-10
44-50: LGTM!The method signature change is well-documented and maintains backward compatibility through the default value.
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (3)
113-116: LGTM!The getter method is well-documented and correctly returns a const reference to prevent modification.
120-122: LGTM!The constructor correctly initializes the metadata member using the member initializer list.
127-127: LGTM!The member variable is appropriately placed with other member variables.
components/core/src/clp/ffi/ir_stream/Serializer.cpp (2)
525-527: LGTM!The method signature matches the declaration in the header.
794-799: LGTM!The template specializations are correctly updated to match the new signature.
components/core/tests/test-ir_encoding_methods.cpp (1)
1226-1228: LGTM! Well-structured test data.The user-defined metadata is well-structured with a good mix of data types (map with nested primitives and array with mixed types) to test serialization/deserialization.
Description
This PR adds support for serializing and deserializing user-defined stream-level metadata.
To support user-defined metadata serialization, we update serializer's factory function to allow users to give the metadata as an optional JSON object.
We already have stream-level deserialization support before this PR. However, we don't store the deserialized metadata. This PR updates the deserializer to store the stream-level metadata as a private member and exposes APIs to get an immutable view of the metadata through the deserializer.
Validation performed
Summary by CodeRabbit
New Features
Improvements
Technical Updates