Add support for deserializing and decoding v0.1.0 IR streams, but without log-level parsing and filtering. #30
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
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 (
|
junhaoliao
changed the title
junhaoliao
changed the title
junhaoliao
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
src/clp_ffi_js/ir/StreamReaderDataContext.hpp (1)
47-50: LGTM! Consider enhancing the documentation.The implementation is correct and follows the class's patterns. Consider adding more details to the documentation about the lifetime of the returned reference and its intended usage.
/** - * @return A reference to the reader. + * @return A reference to the underlying reader. The reference remains valid + * as long as this context object exists. */src/clp_ffi_js/ir/StreamReader.hpp (2)
14-14: Please document the ReaderOptions type.Consider adding JSDoc comments to describe the available options and their impact on the reader's behaviour, particularly regarding log level filtering support.
+/** + * Options for configuring the stream reader's behaviour. + * @typedef {Object} ReaderOptions + * @property {boolean} enableLogLevelFiltering - Whether to enable log level filtering + */ EMSCRIPTEN_DECLARE_VAL_TYPE(ReaderOptions);
35-37: Update method documentation to include the reader_options parameter.The documentation block above needs to be updated to describe the new parameter.
/** * Creates a `StreamReader` to read from the given array. * * @param data_array An array containing a Zstandard-compressed IR stream. + * @param reader_options Configuration options for the reader instance. * @return The created instance. * @throw ClpFfiJsException if any error occurs. */src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (5)
76-79: Consider throwing an exception for unsupported functionalityCurrently, the
get_filtered_log_event_mapmethod logs an error and returnsnullwhen log level filtering is not supported. To provide clearer feedback to the caller, consider throwing aClpFfiJsExceptionto indicate that this functionality is not implemented.
93-94: Make buffer reservation size configurableThe constant
cDefaultNumReservedLogEventsis set to reserve space for 500,000 log events. Depending on the typical size of the IR streams, it may be beneficial to make this value configurable or dynamically adjust it based on the stream's size to optimize memory usage.
139-141: Handle invalid indices indecode_rangemethodCurrently, the method returns
nullif the provided indices are invalid. It would be more appropriate to throw an exception, such as astd::out_of_rangeor a custom exception, to clearly indicate the misuse of the function and aid in debugging.
157-167: Optimize extraction of log level informationThe extraction of the log level involves multiple checks and could be streamlined. Consider simplifying the logic to enhance readability and performance. For example, you might use a helper function or employ structured bindings if the compiler supports C++17 or higher.
190-197: Initialize member variables using an initializer listFor improved readability and performance, consider initializing
m_stream_reader_data_contextandm_deserialized_log_eventsdirectly in the constructor's member initializer list rather than within the constructor body.Apply this diff to adjust the constructor:
- StructuredIrStreamReader::StructuredIrStreamReader( - StreamReaderDataContext<StructuredIrDeserializer>&& stream_reader_data_context, - std::shared_ptr<std::vector<clp::ffi::KeyValuePairLogEvent>> deserialized_log_events - ) - : m_stream_reader_data_context{std::make_unique< - StreamReaderDataContext<StructuredIrDeserializer>>( - std::move(stream_reader_data_context) - )}, - m_deserialized_log_events{std::move(deserialized_log_events)} {} + StructuredIrStreamReader::StructuredIrStreamReader( + StreamReaderDataContext<StructuredIrDeserializer>&& stream_reader_data_context, + std::shared_ptr<std::vector<clp::ffi::KeyValuePairLogEvent>> deserialized_log_events + ) + : m_stream_reader_data_context{std::make_unique< + StreamReaderDataContext<StructuredIrDeserializer>>( + std::move(stream_reader_data_context) + )}, + m_deserialized_log_events{std::move(deserialized_log_events)} { + // Empty constructor body + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CMakeLists.txt(1 hunks)src/clp_ffi_js/ir/StreamReader.cpp(5 hunks)src/clp_ffi_js/ir/StreamReader.hpp(2 hunks)src/clp_ffi_js/ir/StreamReaderDataContext.hpp(1 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(1 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp(1 hunks)src/submodules/clp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/submodules/clp
🧰 Additional context used
📓 Learnings (1)
src/clp_ffi_js/ir/StreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
🔇 Additional comments (10)
src/clp_ffi_js/ir/StreamReaderDataContext.hpp (1)
47-50: Verify the usage of get_reader() method.
Let's ensure this new method is being used appropriately in the StructuredIrStreamReader implementation.
✅ Verification successful
Let me analyze the usage of get_reader() further by checking its implementation context.
The get_reader() method is properly implemented and used
The method is used correctly within StructuredIrStreamReader.cpp where it's called to obtain the reader reference for deserializing IR units. The usage is consistent with its intended purpose as shown by:
- It's used in conjunction with the deserializer to process IR units
- The return value is properly captured as a reference
- The context shows appropriate error handling and processing flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of get_reader() to verify proper implementation
# Search for direct method calls
echo "Searching for get_reader() usage:"
rg -p "get_reader\(\)" --type cpp
# Search for StructuredIrStreamReader implementation to verify integration
echo -e "\nChecking StructuredIrStreamReader implementation:"
fd -e cpp -e hpp "StructuredIrStreamReader" --exec rg -p "StreamReaderDataContext"
Length of output: 1110
Script:
#!/bin/bash
# Check the implementation of StructuredIrStreamReader where get_reader() is used
echo "Checking the context of get_reader() usage:"
rg -p -B 5 -A 5 "get_reader\(\)" src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
# Check the full class definition of StreamReaderDataContext
echo -e "\nChecking StreamReaderDataContext class definition:"
ast-grep --pattern 'template <typename DeserializerT>
class StreamReaderDataContext {
$$$
}'
Length of output: 872
src/clp_ffi_js/ir/StreamReader.hpp (1)
35-37: Verify version compatibility handling.
Since this interface will be used by the new StructuredIrStreamReader for v0.1.0 IR streams, consider adding version validation in the create() method.
Consider adding a version compatibility check in the factory method or documenting version support in the class documentation.
CMakeLists.txt (2)
86-88: 💡 Codebase verification
Missing test coverage for Stream Reader implementations
The verification reveals that while there are three reader implementations:
- src/clp_ffi_js/ir/StreamReader.cpp
- src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
- src/clp_ffi_js/ir/UnstructuredIrStreamReader.cpp
There are no corresponding test files in the codebase. This is concerning as these components handle critical IR stream processing functionality.
- Add unit tests for StructuredIrStreamReader to verify v0.1.0 IR stream handling
- Add unit tests for UnstructuredIrStreamReader to ensure proper stream processing
- Consider adding integration tests to verify the interaction between different reader implementations
🔗 Analysis chain
Source files organization looks good, but verify test coverage.
The addition of StructuredIrStreamReader.cpp and UnstructuredIrStreamReader.cpp alongside the existing StreamReader.cpp follows a logical organization pattern. However, ensure that corresponding test files are also added to maintain test coverage.
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for corresponding test files for the new source files
# Expected: Test files should exist for the new reader implementations
echo "Checking for test files..."
fd -g "*test*.cpp" -g "*Test*.cpp" | grep -i "structured.*reader"
Length of output: 228
Script:
#!/bin/bash
# Let's try a different approach to find test files
echo "Searching for test files related to readers..."
rg -l "test|Test" --type cpp | grep -i "reader"
# Also search for the actual implementation files to compare
echo -e "\nSearching for reader implementation files..."
fd ".*Reader\.cpp$"
Length of output: 408
Script:
#!/bin/bash
# Let's check the test directory structure and content
echo "Checking test directory structure..."
fd "test" -t d
echo -e "\nChecking for any test files in the repository..."
fd -e cpp -e h ".*[tT]est.*"
# Let's also check if there are tests in the same directory as the readers
echo -e "\nChecking for test files in the ir directory..."
ls src/clp_ffi_js/ir/*[tT]est*
Length of output: 460
⛔ Skipped due to learnings
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/IrStreamReader.cpp:58-59
Timestamp: 2024-11-04T19:35:32.602Z
Learning: In the `IrStreamReader::filter_log_events` method in `src/clp_ffi_js/ir/IrStreamReader.cpp`, the wrapper code is typed with TypeScript, ensuring that non-array inputs for `log_level_filter` would fail static checks, so additional runtime error handling is unnecessary.
92-95: Verify dependency graph for new core components.
The addition of core CLP components (ir_unit_deserialization_methods.cpp, utils.cpp, KeyValuePairLogEvent.cpp, SchemaTree.cpp) suggests new dependencies. These components appear to support the structured IR stream reading functionality.
Let's verify the dependency relationships:
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (3)
65-67: Correct use of [[maybe_unused]] attribute
The parameters utc_offset_old and utc_offset_new are properly marked with [[maybe_unused]], indicating they are intentionally unused in this function.
82-83: Verify the management of m_current_node_id
Manually incrementing m_current_node_id may lead to inconsistencies if node IDs are managed elsewhere. Please ensure that this approach aligns with the SchemaTree node ID assignment strategy or consider retrieving the node ID directly from schema_tree_node_locator.
134-192: Well-structured class design and resource management
The StructuredIrStreamReader class is well-designed, adhering to RAII principles with appropriate use of constructors and deleted copy and move assignment operators. The resource management appears sound.
src/clp_ffi_js/ir/StreamReader.cpp (3)
120-122: Emscripten type registration updated correctly
The ReaderOptions type is registered appropriately with Emscripten bindings, allowing it to be used effectively in the interface.
198-203: Appropriate exception handling for unsupported versions
Throwing ClpFfiJsException for unsupported IR stream versions provides clear feedback to the user about the inability to create a reader.
149-150:
Update method signature and adjust all invocations
The StreamReader::create method now includes the ReaderOptions parameter. Please ensure that all calls to this method are updated to include the new parameter to maintain consistency and prevent compilation issues.
Run the following script to identify any calls to StreamReader::create that might be missing the ReaderOptions parameter:
| if (clp::ffi::ir_stream::IRProtocolErrorCode::Supported == version_validation_result) { | ||
| zstd_decompressor->seek_from_begin(0); | ||
| return std::make_unique<StructuredIrStreamReader>(StructuredIrStreamReader::create( | ||
| std::move(zstd_decompressor), | ||
| std::move(data_buffer), | ||
| reader_options | ||
| )); | ||
| } | ||
| if (clp::ffi::ir_stream::IRProtocolErrorCode::BackwardCompatible | ||
| == version_validation_result) | ||
| { | ||
| zstd_decompressor->seek_from_begin(pos); | ||
| return std::make_unique<UnstructuredIrStreamReader>(UnstructuredIrStreamReader::create( | ||
| std::move(zstd_decompressor), | ||
| std::move(data_buffer) | ||
| )); | ||
| } | ||
| } catch (ZstdDecompressor::OperationFailed const& e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent use of ReaderOptions
While StructuredIrStreamReader::create is called with reader_options, the UnstructuredIrStreamReader::create is called without it. If ReaderOptions are applicable to both reader types, consider passing reader_options to UnstructuredIrStreamReader::create as well for consistency.
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| std::format("Unable to create reader for IR stream with version {}.", version) | ||
| }; | ||
| } | ||
| auto const version_validation_result{clp::ffi::ir_stream::validate_protocol_version(version)}; |
There was a problem hiding this comment.
💡 Codebase verification
Function validate_protocol_version appears to be missing from the codebase
The function clp::ffi::ir_stream::validate_protocol_version is being called in StreamReader.cpp, but after thorough searching through the codebase:
- No function definition was found
- No header file contains the declaration
- Only usage appears in
StreamReader.cpp
This confirms the original concern that this function is not yet available in the CLP-core library.
🔗 Analysis chain
Verify the existence of validate_protocol_version function
Please confirm that the function clp::ffi::ir_stream::validate_protocol_version is available in the CLP-core library. According to previous learnings, this utility function was not planned to be added soon.
Run the following script to check for the definition of validate_protocol_version:
Let me search for more specific information about this function's definition.
Let me try one more search with different parameters to find any related header files or function definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the definition of `validate_protocol_version` in the codebase.
rg 'validate_protocol_version'
Length of output: 164
Script:
#!/bin/bash
# Search for the function definition using ast-grep
ast-grep --pattern 'validate_protocol_version'
# Also check header files that might contain the declaration
rg -t hpp -t h 'validate_protocol_version'
# Look for any namespace definitions related to ir_stream
rg 'namespace.*ir_stream'
Length of output: 283
Script:
#!/bin/bash
# Search for header files with .h extension
fd -e h -x rg -l "validate_protocol_version" {} \;
# Search for any ir_stream related files
fd -t f -x rg -l "namespace.*ir_stream|ir_stream" {} \;
# Look for the function definition in cpp files
fd -e cpp -x rg -B 2 -A 5 "validate_protocol_version" {} \;
Length of output: 784
There was a problem hiding this comment.
This is imported from a CLP upstream library.
There was a problem hiding this comment.
@junhaoliao, thank you for the clarification. I understand that validate_protocol_version is imported from the CLP upstream library.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StreamReader.cpp:179-179
Timestamp: 2024-11-07T08:38:39.605Z
Learning: The function `clp::ffi::ir_stream::validate_protocol_version` used in `src/clp_ffi_js/ir/StreamReader.cpp` is imported from the CLP upstream library and may not be present in the local codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (log_level_filter.isNull()) { | ||
| return; | ||
| } | ||
| SPDLOG_ERROR(cLogLevelFilteringNotSupportedPrompt); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in filter_log_events method
Instead of logging an error when log level filtering is unsupported, you might throw a ClpFfiJsException. This approach would more effectively notify the caller that the operation is not available, allowing for better error management in the application.
| if (std::errc::result_out_of_range == error) { | ||
| SPDLOG_ERROR("File contains an incomplete IR stream"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Throw exception for incomplete IR streams
When encountering an incomplete IR stream, the code logs an error and exits the loop. Consider throwing a ClpFfiJsException to inform the caller about the incomplete stream explicitly. This change would improve error propagation and allow for more robust error handling upstream.
| EM_ASM( | ||
| { Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); }, | ||
| results.as_handle(), | ||
| json.value().dump().c_str(), | ||
| log_level, | ||
| timestamp, | ||
| log_event_idx + 1 | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using EM_ASM for data aggregation
Using EM_ASM macros can introduce complexity and potential maintenance issues. Consider using emscripten::val functions to manipulate JavaScript arrays directly from C++. This approach would lead to cleaner code and better type safety.
Apply this diff to refactor the code:
- EM_ASM(
- { Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); },
- results.as_handle(),
- json.value().dump().c_str(),
- log_level,
- timestamp,
- log_event_idx + 1
- );
+ auto js_array = emscripten::val::array();
+ js_array.call<void>("push", json.value().dump(), log_level, timestamp, log_event_idx + 1);
+ results.call<void>("push", js_array);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp (1)
59-62: Implementation looks good, but documentation would be helpful, eh?The implementation correctly provides backward compatibility for IR protocol error handling. However, adding documentation would make the intent clearer.
Consider adding documentation like this:
+ /** + * Returns the IR protocol error code for this reader. + * @return BackwardCompatible to indicate support for older IR stream versions + */ [[nodiscard]] auto get_ir_protocol_error_code( ) const -> clp::ffi::ir_stream::IRProtocolErrorCode override { return clp::ffi::ir_stream::IRProtocolErrorCode::BackwardCompatible; }src/clp_ffi_js/ir/StreamReader.hpp (2)
15-15: Add documentation for ReaderOptions type.Please add JSDoc or similar documentation to describe the purpose and structure of the ReaderOptions type, similar to how other TS types are documented in the codebase.
54-55: Add documentation for get_ir_protocol_error_code method.The new pure virtual method requires documentation to:
- Describe its purpose and return value meanings
- Specify when it should be called in the error handling flow
- Document any exceptional conditions
Additionally, consider adding error code constants or enums to make the error handling more explicit.
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (1)
64-71: Consider implementing UTC offset change handlingThe method
handle_utc_offset_changecurrently logs a warning indicating that UTC offset changes are not handled. If UTC offset changes are expected in the IR streams, implementing proper handling would ensure accurate processing of time-sensitive data.Would you like assistance in implementing the logic for handling UTC offset changes?
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (2)
144-146: Remove unused variablemessageto clean up the codeThe variable
messageis declared and reserved but never used. Removing it will simplify the code and eliminate any unnecessary memory allocation.Apply this diff to remove the unused variable:
- std::string message; - constexpr size_t cDefaultReservedMessageLength{512}; - message.reserve(cDefaultReservedMessageLength);
93-94: Consider making the reserved log events count configurableThe default reservation of 500,000 log events might not be optimal for all use cases. Allowing this value to be configurable can help manage memory usage more effectively for different stream sizes.
Apply this diff to introduce configurability:
- constexpr size_t cDefaultNumReservedLogEvents{500'000}; + size_t num_reserved_log_events = reader_options.contains("numReservedLogEvents") + ? reader_options["numReservedLogEvents"].as<size_t>() + : 500'000; - m_deserialized_log_events->reserve(cDefaultNumReservedLogEvents); + m_deserialized_log_events->reserve(num_reserved_log_events);src/clp_ffi_js/ir/StreamReader.cpp (1)
182-199: Usage ofreader_optionswithUnstructuredIrStreamReader
reader_optionsare passed toStructuredIrStreamReader::createbut not toUnstructuredIrStreamReader::create. IfUnstructuredIrStreamReaderdoes not utilizereader_options, this is acceptable. However, for consistency and future extensibility, consider passingreader_optionstoUnstructuredIrStreamReader::createas well.Apply this diff if applicable:
- std::move(data_buffer) + std::move(data_buffer), + reader_options
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/clp_ffi_js/ir/StreamReader.cpp(6 hunks)src/clp_ffi_js/ir/StreamReader.hpp(3 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(1 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(1 hunks)
🧰 Additional context used
📓 Learnings (3)
src/clp_ffi_js/ir/StreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
src/clp_ffi_js/ir/StreamReader.hpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/IrStreamReader.cpp:58-59
Timestamp: 2024-11-04T19:35:32.602Z
Learning: In the `IrStreamReader::filter_log_events` method in `src/clp_ffi_js/ir/IrStreamReader.cpp`, the wrapper code is typed with TypeScript, ensuring that non-array inputs for `log_level_filter` would fail static checks, so additional runtime error handling is unnecessary.
🔇 Additional comments (7)
src/clp_ffi_js/ir/StreamReader.hpp (2)
7-7: LGTM! Header changes improve modularity.
The addition of decoding_methods.hpp is appropriate for the new IR protocol error code functionality, and removing unused headers helps reduce compilation overhead.
36-38: Verify all callers are updated for the new create() signature.
The addition of the reader_options parameter is a breaking change.
✅ Verification successful
Let me generate additional scripts to verify the actual usage of StreamReader::create from JavaScript code since we only see the C++ side so far.
Let me try one more search with a different approach to find JavaScript files and their usage patterns.
Based on the search results, I can now provide a final response:
Breaking change in StreamReader::create is properly handled
The addition of the ReaderOptions parameter to StreamReader::create is correctly propagated through the inheritance hierarchy:
- StreamReader::create is the factory method exposed to JavaScript through Emscripten bindings
- Both derived classes (StructuredIrStreamReader and UnstructuredIrStreamReader) handle the change:
- StructuredIrStreamReader::create accepts and uses the reader_options parameter
- UnstructuredIrStreamReader::create maintains its original signature since it doesn't need the options
The JavaScript interface is properly maintained through the Emscripten bindings which register the ReaderOptions type as "{logLevelKey: string, timestampKey: string} | null".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to StreamReader::create to ensure they're updated
# with the new ReaderOptions parameter
# Search for create() calls in both C++ and JS files
rg -t cpp -t js "StreamReader::create|StreamReader\.create" -A 2
Length of output: 2015
Script:
#!/bin/bash
# Search for JavaScript code that calls StreamReader.create
rg -t js "new StreamReader|StreamReader\.create" -A 3
# Also check for any test files that might use StreamReader
fd -e test.js -e spec.js -X rg "StreamReader" -A 3
Length of output: 255
Script:
#!/bin/bash
# Find all JavaScript files first
fd -e js -e mjs -e jsx -e ts -e tsx | xargs rg "StreamReader" -A 3
# Also look for binding registration to see how StreamReader is exposed to JS
rg "EMSCRIPTEN_BINDINGS|class_<StreamReader>" -A 5
Length of output: 26037
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)
47-48: Ensure reader_options contains valid logLevelKey and timestampKey
Accessing reader_options["logLevelKey"].as<std::string>() without verifying the existence and type of the key may lead to exceptions. It's prudent to validate that these keys are present and contain string values before using them.
Run the following script to check the initialization of reader_options:
src/clp_ffi_js/ir/StreamReader.cpp (4)
120-122: Registration of ReaderOptions Type is Correct
The ReaderOptions type is correctly registered with Emscripten, accurately reflecting the TypeScript type {logLevelKey: string, timestampKey: string} | null. This ensures proper type checking and usage in JavaScript.
125-130: Emscripten Binding for IRProtocolErrorCode Enum is Accurate
The enumeration IRProtocolErrorCode is properly bound to JavaScript using Emscripten. All enum values—SUPPORTED, BACKWARD_COMPATIBLE, UNSUPPORTED, and INVALID—are correctly registered, allowing JavaScript code to utilize these constants effectively.
140-143: Exposed Method getIrProtocolErrorCode Matches Naming Conventions
The addition of getIrProtocolErrorCode to the Emscripten bindings exposes the corresponding C++ method to JavaScript. The method name is consistent with the project's naming conventions and provides necessary functionality to the JavaScript interface.
208-213: Exception Handling Enhances Error Reporting
The thrown ClpFfiJsException provides a clear and informative message when unable to create a reader for the IR stream with an unsupported version. This improves error reporting and aids in debugging.
| explicit StructuredIrStreamReader( | ||
| StreamReaderDataContext<StructuredIrDeserializer>&& stream_reader_data_context, | ||
| std::shared_ptr<std::vector<clp::ffi::KeyValuePairLogEvent>> deserialized_log_events | ||
| ); |
There was a problem hiding this comment.
Resolve mismatch between constructor parameter and member variable types
The constructor accepts stream_reader_data_context as an rvalue reference to StreamReaderDataContext<StructuredIrDeserializer>, but the member variable m_stream_reader_data_context is a std::unique_ptr<StreamReaderDataContext<StructuredIrDeserializer>>. This mismatch may lead to incorrect ownership semantics or compilation errors.
Apply this diff to correct the issue:
explicit StructuredIrStreamReader(
- StreamReaderDataContext<StructuredIrDeserializer>&& stream_reader_data_context,
+ std::unique_ptr<StreamReaderDataContext<StructuredIrDeserializer>>&& stream_reader_data_context,
std::shared_ptr<std::vector<clp::ffi::KeyValuePairLogEvent>> deserialized_log_events
);Then, initialize m_stream_reader_data_context by moving the unique pointer:
m_stream_reader_data_context{std::move(stream_reader_data_context)}There was a problem hiding this comment.
The moved value is already wrapped by a make_unique pointer in the constructor.
There was a problem hiding this comment.
@junhaoliao, thank you for the clarification. Since the stream_reader_data_context is already wrapped by a std::make_unique pointer in the constructor, the ownership semantics are correctly handled.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StructuredIrStreamReader.hpp:187-190
Timestamp: 2024-11-07T08:35:20.081Z
Learning: In the `StructuredIrStreamReader` constructor of `src/clp_ffi_js/ir/StructuredIrStreamReader.hpp`, the `stream_reader_data_context` parameter is already wrapped in a `std::make_unique`, ensuring it matches the member variable `m_stream_reader_data_context` of type `std::unique_ptr<StreamReaderDataContext<StructuredIrDeserializer>>`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| auto const& log_level_pair{id_value_pairs.at(m_level_node_id.value())}; | ||
| log_level = log_level_pair.has_value() | ||
| ? static_cast<LogLevel>( | ||
| log_level_pair.value() | ||
| .get_immutable_view<clp::ffi::value_int_t>() | ||
| ) | ||
| : log_level; | ||
| } |
There was a problem hiding this comment.
Prevent potential exceptions when accessing id_value_pairs with missing keys
Using id_value_pairs.at() can throw an std::out_of_range exception if the key is not found. To ensure robustness, consider checking if the key exists before accessing it.
Apply this diff to safely access the map:
At lines 160-167:
- auto const& log_level_pair{id_value_pairs.at(m_level_node_id.value())};
- log_level = log_level_pair.has_value()
- ? static_cast<LogLevel>(
- log_level_pair.value()
- .get_immutable_view<clp::ffi::value_int_t>()
- )
- : log_level;
+ auto log_level_it = id_value_pairs.find(m_level_node_id.value());
+ if (log_level_it != id_value_pairs.end()) {
+ auto const& log_level_pair = log_level_it->second;
+ if (log_level_pair.has_value()) {
+ log_level = static_cast<LogLevel>(
+ log_level_pair.value().get_immutable_view<clp::ffi::value_int_t>()
+ );
+ }
+ }At lines 170-174:
- auto const& timestamp_pair{id_value_pairs.at(m_timestamp_node_id.value())};
- timestamp = timestamp_pair.has_value()
- ? timestamp_pair.value().get_immutable_view<clp::ffi::value_int_t>()
- : timestamp;
+ auto timestamp_it = id_value_pairs.find(m_timestamp_node_id.value());
+ if (timestamp_it != id_value_pairs.end()) {
+ auto const& timestamp_pair = timestamp_it->second;
+ if (timestamp_pair.has_value()) {
+ timestamp = timestamp_pair.value().get_immutable_view<clp::ffi::value_int_t>();
+ }
+ }Also applies to: 170-174
| auto StreamReader::create(DataArrayTsType const& data_array, ReaderOptions const& reader_options) | ||
| -> std::unique_ptr<StreamReader> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential API Breaking Change in StreamReader::create Signature
The create method of StreamReader now requires an additional parameter, ReaderOptions. This change may break existing code that calls StreamReader::create without the new parameter. Consider providing a default value or an overloaded method to maintain backward compatibility.
Apply this diff to add a default parameter value:
-auto StreamReader::create(DataArrayTsType const& data_array, ReaderOptions const& reader_options)
+auto StreamReader::create(DataArrayTsType const& data_array, ReaderOptions const& reader_options = {})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto StreamReader::create(DataArrayTsType const& data_array, ReaderOptions const& reader_options) | |
| -> std::unique_ptr<StreamReader> { | |
| auto StreamReader::create(DataArrayTsType const& data_array, ReaderOptions const& reader_options = {}) | |
| -> std::unique_ptr<StreamReader> { |
…ions from code review Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/clp_ffi_js/ir/StreamReader.hpp
Outdated
| @@ -52,6 +51,9 @@ class StreamReader { | |||
| auto operator=(StreamReader&&) -> StreamReader& = delete; | |||
|
|
|||
| // Methods | |||
| [[nodiscard]] virtual auto get_ir_protocol_error_code( | |||
There was a problem hiding this comment.
Why do we need to expose this method? If it's just so that the caller can determine what type of IR stream we're reading, then I think it's clearer to return some kind of enum of Structured and Unstructured (and call the method something like get_ir_stream_type).
There was a problem hiding this comment.
@kirkrodrigues this is a hack since we are not decoding the message in C++. The log viewer needs to know now if it is recieving json (irv2), or an irv1 message. If recieving json, it needs to format the string.
There was a problem hiding this comment.
Gotcha. I'd say an enum indicating that is still clearer than returning the protocol error code.
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
|
|
||
| // JS types used as outputs | ||
| emscripten::enum_<clp::ffi::ir_stream::IRProtocolErrorCode>("IRProtocolErrorCode") |
There was a problem hiding this comment.
If we end up exposing IRProtocolErrorCode, let's name it IrProtocolErrorCode. I think it's better to avoid propagating the naming errors from CLP.
src/clp_ffi_js/ir/StreamReader.cpp
Outdated
| std::format("Unable to create reader for IR stream with version {}.", version) | ||
| }; | ||
| } | ||
| auto const version_validation_result{clp::ffi::ir_stream::validate_protocol_version(version)}; |
There was a problem hiding this comment.
Can move this into the try to reduce its scope.
| /** | ||
| * @return A reference to the reader. | ||
| */ |
There was a problem hiding this comment.
Seems unnecessary. Same with the get_deserializer docstring.
| auto const json{log_event.serialize_to_json()}; | ||
| if (false == json.has_value()) { | ||
| SPDLOG_ERROR("Failed to decode message."); | ||
| break; |
There was a problem hiding this comment.
Should we continue instead?
There was a problem hiding this comment.
Let's instead place an empty object.
| LogLevel log_level{LogLevel::NONE}; | ||
| if (m_level_node_id.has_value()) { | ||
| auto const& log_level_pair{id_value_pairs.at(m_level_node_id.value())}; | ||
| log_level = log_level_pair.has_value() |
There was a problem hiding this comment.
- If the log level isn't an int,
get_immutable_viewwill throw, right? We should check that the value's type matches the expected type first (usingis) and then retrieve the value. - If the value's type is a string, we should compare it against the log level strings, right?
- Do we need the ternary? Personally, I would write this as: if has_value, set log_level.
There was a problem hiding this comment.
Sure. As we discussed offline, let's leave log level handling till another PR. Let me apply the changes for timestamp after I remove log level related handling.
| timestamp = timestamp_pair.has_value() | ||
| ? timestamp_pair.value().get_immutable_view<clp::ffi::value_int_t>() | ||
| : timestamp; |
There was a problem hiding this comment.
As mentioned above, we need to check the type and the ternary seems unnecessary.
| StreamReaderDataContext<StructuredIrDeserializer>>( | ||
| std::move(stream_reader_data_context) | ||
| )}, | ||
| m_deserialized_log_events{std::move(deserialized_log_events)} {} |
There was a problem hiding this comment.
Move this first to match the declaration order of member variables.
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
src/clp_ffi_js/ir/StreamReaderDataContext.hpp (1)
44-45: Consider adding documentation for consistency.The implementation looks good, but for consistency with the class-level documentation style, consider adding a brief docstring explaining the purpose and return value of this method.
Apply this diff to add documentation:
+ /** + * Gets the underlying reader interface. + * @return Reference to the ReaderInterface instance. + */ [[nodiscard]] auto get_reader() -> clp::ReaderInterface& { return *m_reader; }src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (2)
36-68: Add validation for reader options.While the implementation is solid, consider validating that the required reader options are present and of the correct type before accessing them.
auto StructuredIrStreamReader::create( std::unique_ptr<ZstdDecompressor>&& zstd_decompressor, clp::Array<char> data_array, ReaderOptions const& reader_options ) -> StructuredIrStreamReader { + if (!reader_options.hasOwnProperty(cReaderOptionsTimestampKey.data()) || + !reader_options[cReaderOptionsTimestampKey.data()].isString()) { + throw ClpFfiJsException{ + clp::ErrorCode::ErrorCode_InvalidArgument, + __FILENAME__, + __LINE__, + "Missing or invalid timestampKey in reader options" + }; + } auto deserialized_log_events{std::make_shared<std::vector<clp::ffi::KeyValuePairLogEvent>>()};
105-108: Enhance error logging for incomplete IR stream.Include more details in the error log to help with debugging.
if (std::errc::result_out_of_range == error) { - SPDLOG_ERROR("File contains an incomplete IR stream"); + SPDLOG_ERROR( + "File contains an incomplete IR stream at position {}", + m_stream_reader_data_context->get_reader().tellg() + ); break; }src/clp_ffi_js/ir/StreamReader.cpp (1)
174-192: Document seek position requirements for different reader types.The code saves the position before version check but only restores it for
UnstructuredIrStreamReader. Consider adding a comment explaining why different reader types require different seek positions to improve maintainability.src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (4)
37-42: Ensure member initializations match declaration orderIn the constructor of
IrUnitHandler, the initializer list initializesm_timestamp_keyandm_deserialized_log_eventsin that order. However, in the class definition,m_deserialized_log_eventsis declared after other member variables. Since C++ initializes member variables in the order of declaration, not the order specified in the initializer list, this may lead to warnings or unintended behaviour. Consider reordering the member variable declarations to match the initializer list or vice versa to maintain consistency.
28-28: Expand abbreviation in documentation for clarityIn the class description, consider expanding "kv-pair" to "key-value pair" for better clarity and to maintain consistent terminology throughout the codebase.
Apply this diff:
- * determine the schema-tree node ID of the timestamp kv-pair. + * determine the schema-tree node ID of the timestamp key-value pair.
34-35: Correct parameter documentation formattingThe description for the
timestamp_keyparameter is split across two lines improperly. Combining them into a single line improves readability and maintains standard documentation formatting.Apply this diff:
- * @param timestamp_key Key name of schema-tree node that contains the authoritative timestamp - * for events. + * @param timestamp_key Key name of the schema-tree node that contains the authoritative timestamp for events.
129-130: Add description forreader_optionsparameterThe
createmethod's documentation lacks a description for thereader_optionsparameter. Providing a clear explanation helps users understand its purpose and how it affects the method's behaviour.Apply this diff:
* @param data_array The array backing `zstd_decompressor`. + * @param reader_options Options to configure the reader's behaviour. * @return The created instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/clp_ffi_js/ir/StreamReader.cpp(6 hunks)src/clp_ffi_js/ir/StreamReader.hpp(3 hunks)src/clp_ffi_js/ir/StreamReaderDataContext.hpp(1 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(1 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/clp_ffi_js/ir/StreamReader.hpp
- src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp
🧰 Additional context used
📓 Learnings (3)
src/clp_ffi_js/ir/StreamReader.cpp (2)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StreamReader.cpp:179-179
Timestamp: 2024-11-07T08:38:39.882Z
Learning: The function `clp::ffi::ir_stream::validate_protocol_version` used in `src/clp_ffi_js/ir/StreamReader.cpp` is imported from the CLP upstream library and may not be present in the local codebase.
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StructuredIrStreamReader.hpp:187-190
Timestamp: 2024-11-07T08:35:20.376Z
Learning: In the `StructuredIrStreamReader` constructor of `src/clp_ffi_js/ir/StructuredIrStreamReader.hpp`, the `stream_reader_data_context` parameter is already wrapped in a `std::make_unique`, ensuring it matches the member variable `m_stream_reader_data_context` of type `std::unique_ptr<StreamReaderDataContext<StructuredIrDeserializer>>`.
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StructuredIrStreamReader.hpp:187-190
Timestamp: 2024-11-07T08:35:20.376Z
Learning: In the `StructuredIrStreamReader` constructor of `src/clp_ffi_js/ir/StructuredIrStreamReader.hpp`, the `stream_reader_data_context` parameter is already wrapped in a `std::make_unique`, ensuring it matches the member variable `m_stream_reader_data_context` of type `std::unique_ptr<StreamReaderDataContext<StructuredIrDeserializer>>`.
🔇 Additional comments (7)
src/clp_ffi_js/ir/StructuredIrStreamReader.cpp (3)
1-27: LGTM! Well-organized includes.
The includes are properly categorized and contain all necessary headers for the implementation.
28-35: LGTM! Well-defined constants.
Constants follow proper naming conventions and are appropriately scoped.
171-179: LGTM! Constructor implementation is correct.
The constructor properly initializes member variables and follows the established patterns.
src/clp_ffi_js/ir/StreamReader.cpp (4)
25-25: LGTM! Include statement follows project conventions.
The new include statement for StructuredIrStreamReader.hpp is correctly placed after system headers and aligns with the file's functionality.
120-127: LGTM! Emscripten bindings are well-structured.
The new bindings for ReaderOptions and IrStreamType are properly registered with appropriate TypeScript-like type annotations. The enum values follow the recommended naming convention from previous feedback.
Also applies to: 137-137
153-154: Skipping comment as this was previously addressed.
A past review comment already suggested adding a default value for the reader_options parameter to maintain backward compatibility.
202-207: LGTM! Clear and informative error handling.
The error handling for unsupported versions follows the project's exception pattern and provides a helpful error message that includes the version information.
| auto StructuredIrStreamReader::get_filtered_log_event_map() const -> FilteredLogEventMapTsType { | ||
| SPDLOG_ERROR(cLogLevelFilteringNotSupportedErrorMsg); | ||
| return FilteredLogEventMapTsType{emscripten::val::null()}; | ||
| } | ||
|
|
||
| void StructuredIrStreamReader::filter_log_events(LogLevelFilterTsType const& log_level_filter) { | ||
| if (log_level_filter.isNull()) { | ||
| return; | ||
| } | ||
| SPDLOG_ERROR(cLogLevelFilteringNotSupportedErrorMsg); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider throwing an exception for unsupported operations.
Instead of logging errors and returning null values, consider throwing a ClpFfiJsException to explicitly indicate that these operations are not supported.
auto StructuredIrStreamReader::get_filtered_log_event_map() const -> FilteredLogEventMapTsType {
- SPDLOG_ERROR(cLogLevelFilteringNotSupportedErrorMsg);
- return FilteredLogEventMapTsType{emscripten::val::null()};
+ throw ClpFfiJsException{
+ clp::ErrorCode::ErrorCode_NotSupported,
+ __FILENAME__,
+ __LINE__,
+ std::string{cLogLevelFilteringNotSupportedErrorMsg}
+ };
}Committable suggestion skipped: line range outside the PR's diff.
| EM_ASM( | ||
| { Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); }, | ||
| results.as_handle(), | ||
| json_result.value().dump().c_str(), | ||
| timestamp, | ||
| LogLevel::NONE, | ||
| log_event_idx + 1 | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace EM_ASM with emscripten::val for better type safety.
Using emscripten::val provides better type safety and maintainability compared to EM_ASM.
- EM_ASM(
- { Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); },
- results.as_handle(),
- json_result.value().dump().c_str(),
- timestamp,
- LogLevel::NONE,
- log_event_idx + 1
- );
+ auto event_array = emscripten::val::array();
+ event_array.set(0, emscripten::val(json_result.value().dump()));
+ event_array.set(1, emscripten::val(timestamp));
+ event_array.set(2, emscripten::val(static_cast<int>(LogLevel::NONE)));
+ event_array.set(3, emscripten::val(log_event_idx + 1));
+ results.call<void>("push", event_array);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EM_ASM( | |
| { Emval.toValue($0).push([UTF8ToString($1), $2, $3, $4]); }, | |
| results.as_handle(), | |
| json_result.value().dump().c_str(), | |
| timestamp, | |
| LogLevel::NONE, | |
| log_event_idx + 1 | |
| ); | |
| auto event_array = emscripten::val::array(); | |
| event_array.set(0, emscripten::val(json_result.value().dump())); | |
| event_array.set(1, emscripten::val(timestamp)); | |
| event_array.set(2, emscripten::val(static_cast<int>(LogLevel::NONE))); | |
| event_array.set(3, emscripten::val(log_event_idx + 1)); | |
| results.call<void>("push", event_array); |
| [[nodiscard]] static auto handle_end_of_stream() -> clp::ffi::ir_stream::IRErrorCode { | ||
| return clp::ffi::ir_stream::IRErrorCode::IRErrorCode_Success; | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove static keyword from handle_end_of_stream method
Similar to handle_utc_offset_change, the handle_end_of_stream method is declared as static but should be an instance method to properly implement the interface clp::ffi::ir_stream::IrUnitHandlerInterface. Removing the static keyword ensures the method can be overridden as intended.
Apply this diff to fix the issue:
- [[nodiscard]] static auto handle_end_of_stream() -> clp::ffi::ir_stream::IRErrorCode {
+ [[nodiscard]] auto handle_end_of_stream() -> clp::ffi::ir_stream::IRErrorCode {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[nodiscard]] static auto handle_end_of_stream() -> clp::ffi::ir_stream::IRErrorCode { | |
| return clp::ffi::ir_stream::IRErrorCode::IRErrorCode_Success; | |
| } | |
| [[nodiscard]] auto handle_end_of_stream() -> clp::ffi::ir_stream::IRErrorCode { | |
| return clp::ffi::ir_stream::IRErrorCode::IRErrorCode_Success; | |
| } | |
| [[nodiscard]] static auto handle_utc_offset_change( | ||
| [[maybe_unused]] clp::UtcOffset utc_offset_old, | ||
| [[maybe_unused]] clp::UtcOffset utc_offset_new | ||
| ) -> clp::ffi::ir_stream::IRErrorCode { | ||
| SPDLOG_WARN("UTC offset change packets aren't handled currently."); | ||
|
|
||
| return clp::ffi::ir_stream::IRErrorCode::IRErrorCode_Success; | ||
| } |
There was a problem hiding this comment.
Remove static keyword from handle_utc_offset_change method
The method handle_utc_offset_change is declared as static, but it implements an interface method from clp::ffi::ir_stream::IrUnitHandlerInterface. Since interface methods are usually virtual and cannot be static, this might prevent proper overriding and cause incorrect behaviour. Removing the static keyword ensures correct interface implementation.
Apply this diff to fix the issue:
- [[nodiscard]] static auto handle_utc_offset_change(
+ [[nodiscard]] auto handle_utc_offset_change(
[[maybe_unused]] clp::UtcOffset utc_offset_old,
[[maybe_unused]] clp::UtcOffset utc_offset_new
) -> clp::ffi::ir_stream::IRErrorCode {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[nodiscard]] static auto handle_utc_offset_change( | |
| [[maybe_unused]] clp::UtcOffset utc_offset_old, | |
| [[maybe_unused]] clp::UtcOffset utc_offset_new | |
| ) -> clp::ffi::ir_stream::IRErrorCode { | |
| SPDLOG_WARN("UTC offset change packets aren't handled currently."); | |
| return clp::ffi::ir_stream::IRErrorCode::IRErrorCode_Success; | |
| } | |
| [[nodiscard]] auto handle_utc_offset_change( | |
| [[maybe_unused]] clp::UtcOffset utc_offset_old, | |
| [[maybe_unused]] clp::UtcOffset utc_offset_new | |
| ) -> clp::ffi::ir_stream::IRErrorCode { | |
| SPDLOG_WARN("UTC offset change packets aren't handled currently."); | |
| return clp::ffi::ir_stream::IRErrorCode::IRErrorCode_Success; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/clp_ffi_js/ir/StreamReader.hpp (3)
16-16: Documentation needed for ReaderOptions typePlease add documentation comments explaining the purpose and expected structure of the ReaderOptions type, following the same pattern as other emscripten val types in this file.
42-44: Update create method documentationThe method documentation needs to be updated to include details about the new
reader_optionsparameter.
60-61: Add documentation for get_ir_stream_type methodThe implementation nicely follows the suggested approach from previous reviews. However, please add documentation comments explaining:
- The method's purpose
- Return value meanings
- Any preconditions or exceptions
src/clp_ffi_js/ir/StreamReader.cpp (2)
122-124: Consider renaming enum values for consistency.Based on previous feedback, we should follow a consistent naming convention. Consider renaming the enum values to match the suggested style:
emscripten::enum_<clp_ffi_js::ir::StreamType>("IrStreamType") - .value("STRUCTURED", clp_ffi_js::ir::StreamType::Structured) - .value("UNSTRUCTURED", clp_ffi_js::ir::StreamType::Unstructured); + .value("Structured", clp_ffi_js::ir::StreamType::Structured) + .value("Unstructured", clp_ffi_js::ir::StreamType::Unstructured);
167-190: Optimize try block scope.The try block could be more focused by moving it to only wrap the operations that might throw ZstdDecompressor::OperationFailed:
auto const version{get_version(*zstd_decompressor)}; + auto const version_validation_result{clp::ffi::ir_stream::validate_protocol_version(version)}; + + if (clp::ffi::ir_stream::IRProtocolErrorCode::Supported == version_validation_result) { try { - auto const version_validation_result{clp::ffi::ir_stream::validate_protocol_version(version)}; - if (clp::ffi::ir_stream::IRProtocolErrorCode::Supported == version_validation_result) { - zstd_decompressor->seek_from_begin(0); - return std::make_unique<StructuredIrStreamReader>(StructuredIrStreamReader::create( - std::move(zstd_decompressor), - std::move(data_buffer), - reader_options - )); - } - if (clp::ffi::ir_stream::IRProtocolErrorCode::BackwardCompatible == version_validation_result) { - zstd_decompressor->seek_from_begin(pos); - return std::make_unique<UnstructuredIrStreamReader>(UnstructuredIrStreamReader::create( - std::move(zstd_decompressor), - std::move(data_buffer) - )); - } + zstd_decompressor->seek_from_begin(0); + return std::make_unique<StructuredIrStreamReader>(StructuredIrStreamReader::create( + std::move(zstd_decompressor), + std::move(data_buffer), + reader_options + )); } catch (ZstdDecompressor::OperationFailed const& e) { throw ClpFfiJsException{ clp::ErrorCode::ErrorCode_Failure, __FILENAME__, __LINE__, std::format("Unable to rewind zstd decompressor: {}", e.what()) }; } + } + + if (clp::ffi::ir_stream::IRProtocolErrorCode::BackwardCompatible == version_validation_result) { + try { + zstd_decompressor->seek_from_begin(pos); + return std::make_unique<UnstructuredIrStreamReader>(UnstructuredIrStreamReader::create( + std::move(zstd_decompressor), + std::move(data_buffer) + )); + } catch (ZstdDecompressor::OperationFailed const& e) { + throw ClpFfiJsException{ + clp::ErrorCode::ErrorCode_Failure, + __FILENAME__, + __LINE__, + std::format("Unable to rewind zstd decompressor: {}", e.what()) + }; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/clp_ffi_js/ir/StreamReader.cpp(6 hunks)src/clp_ffi_js/ir/StreamReader.hpp(3 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(1 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.hpp(1 hunks)src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
- src/clp_ffi_js/ir/UnstructuredIrStreamReader.hpp
🧰 Additional context used
📓 Learnings (2)
src/clp_ffi_js/ir/StreamReader.cpp (2)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StreamReader.cpp:179-179
Timestamp: 2024-11-07T08:38:39.882Z
Learning: The function `clp::ffi::ir_stream::validate_protocol_version` used in `src/clp_ffi_js/ir/StreamReader.cpp` is imported from the CLP upstream library and may not be present in the local codebase.
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (1)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StructuredIrStreamReader.hpp:187-190
Timestamp: 2024-11-07T08:35:20.376Z
Learning: In the `StructuredIrStreamReader` constructor of `src/clp_ffi_js/ir/StructuredIrStreamReader.hpp`, the `stream_reader_data_context` parameter is already wrapped in a `std::make_unique`, ensuring it matches the member variable `m_stream_reader_data_context` of type `std::unique_ptr<StreamReaderDataContext<StructuredIrDeserializer>>`.
🔇 Additional comments (9)
src/clp_ffi_js/ir/StreamReader.hpp (2)
5-5: Header inclusions look good!
The addition of these headers properly supports the new StreamType enum and IR stream decoding functionality.
Also applies to: 8-8
22-25: Excellent implementation of the StreamType enum!
This implementation nicely addresses the previous review feedback by providing a clear, type-safe enum for distinguishing between stream types. The uint8_t base type is an appropriate choice for this use case.
src/clp_ffi_js/ir/StreamReader.cpp (3)
24-24: LGTM! Include statement follows project conventions.
The include statement for StructuredIrStreamReader.hpp is properly placed after system includes and follows the project's include ordering convention.
150-151: Previous review comment about API breaking change still applies.
199-204: LGTM! Error handling is comprehensive.
The error handling provides clear context by including the version information in the error message and uses appropriate error codes.
src/clp_ffi_js/ir/StructuredIrStreamReader.hpp (4)
37-42: Constructor of IrUnitHandler correctly initializes member variables
The constructor effectively uses move semantics to initialize m_timestamp_key and m_deserialized_log_events, ensuring efficient resource management.
50-55: handle_log_event method accurately buffers log events
The implementation successfully moves the log_event into m_deserialized_log_events, maintaining the integrity of the event data.
76-87: handle_schema_tree_node_insertion properly updates node IDs
The method correctly increments the m_current_node_id and assigns m_timestamp_node_id when the key matches m_timestamp_key, ensuring accurate tracking of schema-tree nodes.
134-138: Factory method create in StructuredIrStreamReader is well-defined
The static create method provides a clear and exception-safe way to instantiate StructuredIrStreamReader objects, accommodating different reader options.
…s from code review Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
…age_available` handling in deserialize_stream accordingly.
|
|
||
| // JS types used as outputs | ||
| emscripten::enum_<clp_ffi_js::ir::StreamType>("IrStreamType") |
There was a problem hiding this comment.
Just double-checking, the TS type should remain IrStreamType, right?
There was a problem hiding this comment.
Right, I keep the Ir prefix intentionally.
To be precise, types registered via this method are not TS types. For every enum, emscripten creates an object, which is comparable with any transpiled enum value. For example, Module.IrStreamType.STRUCTURED / Module.IrStreamType.UNSTRUCTURED can be used to check against the return values of reader.getIrStreamType() in our case.
There was a problem hiding this comment.
… strings with internal linkage - Apply suggestions from code review Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/clp_ffi_js/ir/StreamReader.hpp (2)
41-43: Update method documentation to include the new parameter.The documentation block above the create() method should be updated to include details about the reader_options parameter.
/** * Creates a `StreamReader` to read from the given array. * * @param data_array An array containing a Zstandard-compressed IR stream. + * @param reader_options Configuration options for the reader instance. * @return The created instance. * @throw ClpFfiJsException if any error occurs. */
59-60: Add documentation for the new virtual method.The get_ir_stream_type() method should be documented to explain its purpose and return value.
+ /** + * Returns the type of IR stream being processed. + * + * @return The stream type (Structured or Unstructured). + */ [[nodiscard]] virtual auto get_ir_stream_type() const -> StreamType = 0;src/clp_ffi_js/ir/StreamReader.cpp (1)
172-191: Consider expanding the try-catch block scopeThe try-catch block could be moved to encompass the version check as well, since
get_versionalso uses the decompressor.- auto const version{get_version(*zstd_decompressor)}; - try { + try { + auto const version{get_version(*zstd_decompressor)}; auto const version_validation_result{clp::ffi::ir_stream::validate_protocol_version(version) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/clp_ffi_js/ir/StreamReader.cpp(6 hunks)src/clp_ffi_js/ir/StreamReader.hpp(4 hunks)src/clp_ffi_js/ir/StructuredIrStreamReader.cpp(1 hunks)src/submodules/clp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/clp_ffi_js/ir/StructuredIrStreamReader.cpp
- src/submodules/clp
🧰 Additional context used
📓 Learnings (1)
src/clp_ffi_js/ir/StreamReader.cpp (2)
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#26
File: src/clp_ffi_js/ir/StreamReader.cpp:35-45
Timestamp: 2024-11-04T20:55:01.745Z
Learning: An IRv1 version validation utility function is not planned to be added to CLP-core soon.
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#30
File: src/clp_ffi_js/ir/StreamReader.cpp:179-179
Timestamp: 2024-11-07T08:38:39.882Z
Learning: The function `clp::ffi::ir_stream::validate_protocol_version` used in `src/clp_ffi_js/ir/StreamReader.cpp` is imported from the CLP upstream library and may not be present in the local codebase.
🔇 Additional comments (7)
src/clp_ffi_js/ir/StreamReader.hpp (2)
5-5: LGTM! Header and type declarations are well-organized.
The addition of and ReaderOptions type declaration follows the existing patterns and best practices.
Also applies to: 15-15
21-24: Well-implemented enum addressing previous review feedback!
The StreamType enum class effectively addresses the previous review comments by providing a clear type-safe way to distinguish between stream types, rather than using protocol error codes.
src/clp_ffi_js/ir/StreamReader.cpp (5)
25-25: LGTM!
The include statement for StructuredIrStreamReader is properly placed and necessary for the implementation.
120-125: LGTM!
The Emscripten bindings are properly configured:
- The IrStreamType enum follows the correct naming convention
- ReaderOptions type registration matches its usage
- The getIrStreamType method binding is correctly added
Also applies to: 135-135
151-152: Skipping comment about breaking change
This issue was already raised in a previous review.
168-191: LGTM! Version validation and reader creation logic is well-implemented
The code properly handles different IR stream versions by:
- Saving the position before version check
- Creating appropriate reader types based on version validation
- Restoring the position for UnstructuredIrStreamReader
200-205: LGTM! Error handling is clear and informative
The exception provides detailed information about the unsupported version, which will help with debugging.
How about: |
junhaoliao
changed the title
Description
StructuredIrStreamReaderwhich supports reading v0.1.0 IR streams without log level filtering.Validation performed
Extracted and placed sample file at project root: test-irv2.clp.zip
Built the js assets with task and ran below sample code at project root:
Observed below in the console:
Sanitize-tested Zstd-compressed CLP IR stream files with older versions:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
StructuredIrStreamReaderclass for improved handling of structured IR streams.StreamReaderclass.get_readerinStreamReaderDataContextfor accessing the reader interface.UnstructuredIrStreamReaderclass.Bug Fixes
Chores
These changes collectively enhance the project's capability to manage and process structured data streams effectively.