feat: Support retrieving CLP IR metadata in Structured and Unstructured IR stream readers (resolves #77). #80
Conversation
…ed IR stream readers.
|
""" WalkthroughThe changes refactor and modularize IR stream metadata handling in the CLP FFI JS bindings. Metadata deserialization is extracted into standalone functions, and Emscripten bindings are expanded to expose metadata to JavaScript. Both structured and unstructured IR stream readers are updated to store and provide parsed metadata, with new methods and type conversions for JavaScript interoperability. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript
participant Emscripten
participant StreamReader
participant ReaderInterface
JS->>Emscripten: Create StreamReader from data
Emscripten->>StreamReader: create(data_array, options)
StreamReader->>ReaderInterface: deserialize_metadata()
ReaderInterface-->>StreamReader: metadata JSON
StreamReader->>StreamReader: parse_metadata_to_ts_type(metadata JSON)
StreamReader-->>Emscripten: StreamReader instance with metadata
Emscripten-->>JS: StreamReader object (with get_metadata())
JS->>StreamReader: get_metadata()
StreamReader-->>JS: MetadataTsType
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
🧠 Learnings (4)src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp (1)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (1)src/clp_ffi_js/ir/StreamReader.cpp (3)🧬 Code Graph Analysis (3)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (3)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (3)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (3)
🔇 Additional comments (27)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
… as a JavaScript object
…type as JavaScript object
| ); | ||
|
|
||
| // JS types used as outputs | ||
| emscripten::register_type<clp_ffi_js::ir::MetadataTsType>("Record<string, any>"); |
| &clp_ffi_js::ir::StreamReader::create, | ||
| emscripten::return_value_policy::take_ownership() | ||
| ) | ||
| .function("getMetadata", &clp_ffi_js::ir::StreamReader::get_metadata) |
| auto get_version(clp::ReaderInterface& reader) -> std::string { | ||
| std::string version; | ||
| try { | ||
| auto const metadata_json = clp_ffi_js::ir::deserialize_metadata(reader); |
There was a problem hiding this comment.
The metadata deserialization logic was extracted as deserialize_metadata.
| @@ -36,15 +37,6 @@ constexpr std::string_view cReaderOptionsTimestampKey{"timestampKey"}; | |||
| constexpr std::string_view cMergedKvPairsAutoGeneratedKey{"auto-generated"}; | |||
| constexpr std::string_view cMergedKvPairsUserGeneratedKey{"user-generated"}; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
moved to json_utils.hpp
There was a problem hiding this comment.
As we discussed offline:
- These utils for rewinding readers are needed because the our deserializer interface doesn't support deserializing IRv1 yet. Hopefully we can get rid of this complicated procedure soon once we have a more complete implementation of the deserializer (this would also avoid deserializing metadata manually. Technically the deserializer API should handle it automatically)
- We should use result-style error handling instead of relying on exceptions, at least for the factory functions. It's meaningless to use a factory function if it just throw on errors, since a constructor can be designed in a same way. The result-style error handling allows us to forward the error code from the core-ffi.
|
|
||
| namespace clp_ffi_js { | ||
| /** | ||
| * @see nlohmann::basic_json::dump |
There was a problem hiding this comment.
| * @see nlohmann::basic_json::dump | |
| * @see nlohmann::basic_json::dump | |
Need an empty line to break the description and @see section, otherwise they will be rendered as a whole.
| * @param json | ||
| * @return Serialized JSON. | ||
| */ | ||
| [[nodiscard]] inline auto dump_json_with_replace(nlohmann::json const& json) -> std::string { |
There was a problem hiding this comment.
Any reason why we use inline instead of putting the implementation into a source file?
| * @param json | ||
| * @return Serialized JSON. | ||
| */ | ||
| [[nodiscard]] inline auto dump_json_with_replace(nlohmann::json const& json) -> std::string { |
There was a problem hiding this comment.
I'd suggest to use json_obj instead of json since we usually use json to refer to the format itself, or a namespace.
| * Serializes a JSON value into a string with invalid UTF-8 sequences replaced rather than | ||
| * throwing an exception. |
There was a problem hiding this comment.
| * Serializes a JSON value into a string with invalid UTF-8 sequences replaced rather than | |
| * throwing an exception. | |
| * Serializes a JSON value into a string with invalid UTF-8 sequences replaced rather than throwing | |
| * an exception. |
| * @throws ClpFfiJsException if the preamble couldn't be deserialized. | ||
| * @return The IR stream's metadata as a JSON object. | ||
| */ | ||
| auto deserialize_metadata(clp::ReaderInterface& reader) -> nlohmann::json; |
There was a problem hiding this comment.
| auto deserialize_metadata(clp::ReaderInterface& reader) -> nlohmann::json; | |
| [[nodiscard]] auto deserialize_metadata(clp::ReaderInterface& reader) -> nlohmann::json; |
| * Serializes a JSON value into a string with invalid UTF-8 sequences replaced rather than | ||
| * throwing an exception. | ||
| * @param json | ||
| * @return Serialized JSON. |
There was a problem hiding this comment.
| * @return Serialized JSON. | |
| * @return The JSON object serialized as a string. |
| ); | ||
|
|
||
| // Variables | ||
| nlohmann::json m_metadata_json; |
| @@ -76,10 +78,12 @@ class UnstructuredIrStreamReader : public StreamReader { | |||
| private: | |||
| // Constructor | |||
| explicit UnstructuredIrStreamReader( | |||
| StreamReaderDataContext<UnstructuredIrDeserializer>&& stream_reader_data_context | |||
| StreamReaderDataContext<UnstructuredIrDeserializer>&& stream_reader_data_context, | |||
| nlohmann::json&& metadata_json | |||
There was a problem hiding this comment.
| nlohmann::json&& metadata_json | |
| nlohmann::json metadata_json |
Do we need an rvalue reference?
| @@ -53,7 +59,11 @@ auto UnstructuredIrStreamReader::create( | |||
| std::move(zstd_decompressor), | |||
| std::move(result.value()) | |||
| ); | |||
| return UnstructuredIrStreamReader(std::move(data_context)); | |||
| return UnstructuredIrStreamReader(std::move(data_context), std::move(metadata_json)); | |||
There was a problem hiding this comment.
| return UnstructuredIrStreamReader(std::move(data_context), std::move(metadata_json)); | |
| return UnstructuredIrStreamReader{std::move(data_context), std::move(metadata_json)}; |
Nit
| @@ -76,10 +78,12 @@ class UnstructuredIrStreamReader : public StreamReader { | |||
| private: | |||
| // Constructor | |||
| explicit UnstructuredIrStreamReader( | |||
| StreamReaderDataContext<UnstructuredIrDeserializer>&& stream_reader_data_context | |||
| StreamReaderDataContext<UnstructuredIrDeserializer>&& stream_reader_data_context, | |||
There was a problem hiding this comment.
Out of this PR's scope: I don't think we need && for stream_reader_data_context either
Description
Support retrieving CLP IR metadata in Structured and Unstructured IR stream readers
Checklist
breaking change.
Validation performed
Test files: test-files.zip
test.mjstest-irv1.clp.zstand observed output:test-irv2.clp.zstand observed output:Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores