feat(ffi): Add support for auto/user-generated KV-pairs in KeyValuePairLogEvent; Detect and invalidate duplicate keys among non-leaf nodes when constructing a KeyValuePairLogEvent.
#558
Conversation
…t unit tests temporarily
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
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: 11
🧹 Outside diff range and nitpick comments (16)
components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (2)
90-96: LGTM! Consider adding comments for clarity.The updated
KeyValuePairLogEvent::createmethod call aligns with the PR objectives by introducing separate schema trees for auto-generated and user-generated key-value pairs. This change enhances the flexibility and clarity of theKeyValuePairLogEventstructure.To improve code readability, consider adding comments to explain the purpose of each argument:
auto test_log_event_result{KeyValuePairLogEvent::create( std::make_shared<SchemaTree>(), // Auto-generated schema tree std::make_shared<SchemaTree>(), // User-generated schema tree {}, // Initial auto-generated key-value pairs {}, // Initial user-generated key-value pairs cTestUtcOffset )};
134-134: LGTM! Consider adding a test for non-empty user-generated pairs.The updated assertion correctly checks for empty user-generated node-ID-value pairs, which aligns with the PR objectives of distinguishing between auto-generated and user-generated key-value pairs.
To enhance test coverage, consider adding a test case that includes non-empty user-generated key-value pairs:
SECTION("Test with non-empty user-generated pairs") { std::vector<std::pair<SchemaTree::NodeId, std::string>> user_pairs = {{1, "value1"}, {2, "value2"}}; auto test_log_event_result{KeyValuePairLogEvent::create( std::make_shared<SchemaTree>(), std::make_shared<SchemaTree>(), {}, user_pairs, cTestUtcOffset )}; // ... (rest of the test setup) REQUIRE( (optional_log_event.has_value() && optional_log_event.value().get_utc_offset() == cTestUtcOffset && optional_log_event.value().get_user_generated_node_id_value_pairs() == user_pairs) ); }This additional test case would verify that the
KeyValuePairLogEventcorrectly handles and stores non-empty user-generated key-value pairs.components/core/src/clp/ffi/SchemaTree.hpp (2)
130-132: LGTM with a minor suggestion.The addition of a default equality operator for the
Nodeclass is appropriate and follows modern C++ practices. The[[nodiscard]]attribute is correctly used.Consider adding a comment explaining why the default implementation is sufficient for this class, as it might not be immediately obvious to future maintainers why a custom implementation isn't necessary.
254-258: LGTM with a minor suggestion for consistency.The implementation of the equality operator for the
SchemaTreeclass is correct and appropriate. It correctly compares them_tree_nodesmember, which is the core of theSchemaTree.For consistency with the
Nodeclass, consider using thedefaultkeyword here as well:[[nodiscard]] auto operator==(SchemaTree const& rhs) const -> bool = default;This would have the same effect as the current implementation and would make the code more consistent across the file.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1)
34-37: Add descriptions to@paramtags for clarityThe
@paramtags for thecreatemethod lack descriptions. Providing brief explanations for each parameter will enhance the documentation and assist users in understanding the purpose of each argument.components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (4)
Line range hint
244-300: Clarify the Logic of Ancestor Key Duplication ChecksThe implementation of the key duplication checks among ancestor nodes is complex and might benefit from additional comments to enhance readability. Consider adding detailed comments explaining the loop's purpose and the conditions under which the loop exits, to aid future maintainability.
Line range hint
426-504: Ensure Exception Details are Logged When JSON Exceptions OccurWhile the
json_exception_handlercaptures exceptions by setting thejson_exception_capturedflag, it would be beneficial to log the exception details for easier debugging and troubleshooting. This way, if an exception occurs during JSON serialization, developers can quickly identify the cause.
529-561: Consider Enhancing Error Reporting inKeyValuePairLogEvent::createIn the
createmethod, validation errors are directly returned asstd::errc. For better error reporting and debugging, consider wrapping these error codes in a custom error type or adding context to the errors, so that callers can distinguish between different validation failures.
563-583: Evaluate Returning a Single JSON Object inserialize_to_jsonThe
serialize_to_jsonmethod now returns astd::pair<nlohmann::json, nlohmann::json>, separating auto-generated and user-generated key-value pairs. Consider combining these into a single JSON object with distinct fields, such as"auto_generated"and"user_generated", to simplify the interface and make it more user-friendly.Would you like assistance in refactoring the method to return a single JSON object?
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (6)
95-103: Correct Indentation and Formatting in Function DeclarationThe function declaration between lines 95-103 has inconsistent indentation. Ensure that parameters are properly aligned for better readability and to comply with the project's coding style guidelines.
291-302: Correct Alignment and Accuracy in Schema Tree DiagramThe ASCII diagram between lines 291-302 has misalignments and might not accurately represent the schema tree structure. Ensure that the diagram correctly reflects the relationships and hierarchy of nodes for better clarity and understanding.
321-322: Check for Successful Node Insertion After Exception HandlingWhile
REQUIRE_NOTHROWensures that no exceptions are thrown when inserting nodes at lines 321-322, it does not verify that the nodes were inserted as expected. Consider adding checks to confirm that the nodes exist in the schema trees after insertion.
327-328: Enhance Comment Clarity Regarding Schema Tree AssumptionsThe comment at lines 325-327 mentions a preventative check for future development but could be clearer. To improve maintainability, consider specifying the exact assumption and how the check enforces it.
393-409: Refine Loop Logic for Testing Invalid Node ID-Value PairsIn the loop starting at line 393, the test cases call
assert_kv_pair_log_event_creation_failureseparately for auto-generated and user-generated node ID-value pairs. To improve code clarity and reduce redundancy, consider combining these into a single test case or adding comments to explain the purpose of separate calls.
618-636: Handle Edge Cases for Out-of-Bounds Node IDsThe test cases between lines 618-636 test out-of-bounds node IDs. While the current implementation checks for operation not permitted errors, consider adding more descriptive assertions or handling to ensure that such edge cases are thoroughly tested and any exceptions provide meaningful feedback.
components/core/tests/test-ir_encoding_methods.cpp (1)
1200-1204: Consider enhancing test coverage for auto-generated key-value pairsCurrently, the test validates that
auto_generatedkey-value pairs are empty. To ensure comprehensive testing, consider adding test cases that include auto-generated key-value pairs to verify their correct handling during serialization and deserialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (8 hunks)
- components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (3 hunks)
- components/core/src/clp/ffi/SchemaTree.hpp (2 hunks)
- components/core/src/clp/ffi/ir_stream/Deserializer.hpp (4 hunks)
- components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (2 hunks)
- components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (2 hunks)
- components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (2 hunks)
- components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (8 hunks)
- components/core/tests/test-ir_encoding_methods.cpp (1 hunks)
🧰 Additional context used
📓 Learnings (2)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723 PR: y-scope/clp#554 File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111 Timestamp: 2024-10-10T15:19:52.408Z Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1)
Learnt from: LinZhihao-723 PR: y-scope/clp#554 File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111 Timestamp: 2024-10-10T15:19:52.408Z Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.
🔇 Additional comments (12)
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (2)
58-61: Documentation update looks good!The updated documentation accurately reflects the changes in the function signature. The descriptions for both
auto_generated_schema_treeanduser_generated_schema_treeare clear and concise, providing a good explanation of their purposes.
75-76: Function signature update looks good, but verify implementation and callers.The updated function signature correctly reflects the distinction between auto-generated and user-generated schema trees, aligning with the PR objectives. The use of
std::shared_ptris consistent with the previous implementation.Please ensure that:
- The function's implementation has been updated to handle both schema trees correctly.
- All callers of this function have been updated to provide both schema trees.
Run the following script to verify the function usage and implementation:
This script will help identify any discrepancies in the implementation or usage of the updated function.
✅ Verification successful
Verification successful: Implementation and usage are correct.
The function
deserialize_ir_unit_kv_pair_log_eventhas been correctly updated to handle bothauto_generated_schema_treeanduser_generated_schema_tree. Additionally, all callers have been updated to provide both schema trees accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of deserialize_ir_unit_kv_pair_log_event # Test 1: Check the function implementation echo "Checking function implementation:" ast-grep --lang cpp --pattern $'auto deserialize_ir_unit_kv_pair_log_event( $_, $_, std::shared_ptr<SchemaTree> auto_generated_schema_tree, std::shared_ptr<SchemaTree> user_generated_schema_tree, $_ ) { $$$ }' # Test 2: Check for function calls with updated signature echo "Checking function calls:" rg --type cpp -A 5 $'deserialize_ir_unit_kv_pair_log_event\s*\('Length of output: 2433
components/core/src/clp/ffi/SchemaTree.hpp (1)
Line range hint
1-1: Consider removing explicit default destructors.The addition of explicit default destructors for both
NodeandSchemaTreeclasses is generally a good practice. However, in this case, both classes already have user-defined move constructors and move assignment operators, which implicitly define a destructor.Given that these classes are not designed for inheritance (they're not marked as
virtual), the explicit default destructors might be unnecessary. Consider removing them to reduce code clutter, unless there's a specific reason for their inclusion that isn't immediately apparent.To verify the current state of the destructors, you can run the following command:
This will show the current declarations of the destructors, if they exist.
✅ Verification successful
Consider removing explicit default destructors.
Both
NodeandSchemaTreeclasses currently have explicit default destructors defined. Since these classes already have user-defined move constructors and move assignment operators, the destructors are implicitly defined. Removing the explicit default destructors can help reduce code clutter without impacting functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type cpp '~(Node|SchemaTree)\(\s*\)\s*=\s*default;' components/core/src/clp/ffi/SchemaTree.hppLength of output: 157
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (3)
541-542: Approval: Function signature change enhances flexibilityThe modification of the
deserialize_ir_unit_kv_pair_log_eventfunction signature to include separate parameters for auto-generated and user-generated schema trees aligns well with the PR objectives. This change allows for more flexible and precise handling of different types of key-value pairs, which is a valuable improvement to the system's architecture.
571-573: Approval: Implementation correctly uses new parametersThe changes to the
KeyValuePairLogEvent::createcall correctly incorporate the newauto_generated_schema_treeanduser_generated_schema_treeparameters, ensuring proper handling of the different schema tree types as intended by the PR objectives.
573-573: Verify: Purpose of empty initializer listAn empty initializer list
{}has been added as the third argument toKeyValuePairLogEvent::create. Please verify the purpose of this argument and ensure it doesn't introduce any unintended side effects.To assist in verifying this change, you can run the following script to check the
KeyValuePairLogEvent::createmethod definition:components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1)
218-218: Ensure thread safety when accessingm_user_generated_schema_treeIf multiple threads may access
m_user_generated_schema_tree, consider adding synchronization to prevent data races when checking for existing nodes withhas_node(node_locator).To verify potential concurrent access, you might examine the codebase for concurrent usage patterns:
✅ Verification successful
It appears the initial script didn’t yield results. To further investigate concurrent accesses to
m_user_generated_schema_tree, please run the following script:
To further ensure thread safety when accessing
m_user_generated_schema_tree, please run the following script to check for mutex protection:
Thread Safety Verified for
m_user_generated_schema_treeNo concurrent accesses to
m_user_generated_schema_treewere found, ensuring thread safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for concurrent accesses to `m_user_generated_schema_tree`. # Test: Look for uses of `m_user_generated_schema_tree` in potentially concurrent contexts. rg --type cpp 'm_user_generated_schema_tree' | rg 'std::thread|std::async|std::future'Length of output: 86
Script:
#!/bin/bash # Description: Search for concurrent accesses to `m_user_generated_schema_tree`. # Test: Look for uses of `m_user_generated_schema_tree` within threading contexts. rg --type cpp 'm_user_generated_schema_tree' -A5 | rg 'std::thread|std::async|std::future'Length of output: 90
Script:
#!/bin/bash # Description: Verify if `m_user_generated_schema_tree` accesses are protected by mutexes. # Search for mutex declarations related to `m_user_generated_schema_tree`. rg --type cpp 'std::mutex\s+\w+_mutex' | rg 'm_user_generated_schema_tree' # Search for accesses to `m_user_generated_schema_tree` within locked scopes. rg --type cpp 'm_user_generated_schema_tree' -C5 | rg 'std::lock_guard|std::unique_lock|std::mutex'Length of output: 174
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (2)
192-203: New Methodserialize_node_id_value_pairs_to_jsonis Well-DefinedThe addition of the
serialize_node_id_value_pairs_to_jsonmethod enhances the JSON serialization functionality. The method signature and documentation are clear and adhere to coding standards.
204-216: New Methodcheck_key_uniqueness_among_sibling_nodesCorrectly Implements Key Uniqueness ChecksThe
check_key_uniqueness_among_sibling_nodesfunction provides necessary validation to ensure unique keys among sibling nodes. The implementation is appropriate and aligns with the intended functionality.components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (2)
316-318: Duplicate Keys with Different Types May Cause Schema ConflictsThe nodes added at lines 316-318 introduce duplicate keys
{"a"}and{"b"}at the same level with different types. This could lead to ambiguity and conflicts in the schema validation. Verify whether this duplication is intentional for testing purposes and if the schema design allows such constructs.Would you like to confirm that the schema permits nodes with duplicate keys but different types? If not, consider adjusting the test data to avoid potential conflicts.
458-478: Validate JSON Serialization Results in TestIn the test section between lines 458-478, the expected JSON is constructed and compared to the serialization result. Ensure that the serialization accurately reflects both auto-generated and user-generated key-value pairs, and consider adding assertions to verify each part of the JSON structure for comprehensive validation.
Would you like to extend the assertions to check individual elements within the serialized JSON to ensure complete correctness?
components/core/tests/test-ir_encoding_methods.cpp (1)
1205-1209: LGTMThe code correctly deserializes the log event and asserts the expected outcomes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (8 hunks)
- components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (3 hunks)
- components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (7 hunks)
🧰 Additional context used
📓 Learnings (4)
📓 Common learnings
Learnt from: LinZhihao-723 PR: y-scope/clp#554 File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111 Timestamp: 2024-10-10T15:19:52.408Z Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723 PR: y-scope/clp#554 File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111 Timestamp: 2024-10-10T15:19:52.408Z Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (2)
Learnt from: LinZhihao-723 PR: y-scope/clp#558 File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68 Timestamp: 2024-10-14T03:38:57.921Z Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.Learnt from: LinZhihao-723 PR: y-scope/clp#554 File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111 Timestamp: 2024-10-10T15:19:52.408Z Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (4)
Learnt from: LinZhihao-723 PR: y-scope/clp#558 File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:331-335 Timestamp: 2024-10-14T03:45:21.323Z Learning: When reviewing code, ensure that suggestions about unnecessary use of `std::move` with `std::shared_ptr` are only made if `std::move` is actually used in the code.Learnt from: LinZhihao-723 PR: y-scope/clp#558 File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:85-103 Timestamp: 2024-10-14T03:42:53.145Z Learning: The function `assert_kv_pair_log_event_creation_failure` is correctly placed within the anonymous namespace in `test-ffi_KeyValuePairLogEvent.cpp`.Learnt from: LinZhihao-723 PR: y-scope/clp#558 File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:228-236 Timestamp: 2024-10-14T03:43:40.364Z Learning: In `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, helper functions like `assert_kv_pair_log_event_creation_failure` return booleans to the caller, and the caller asserts using `REQUIRE` to provide clearer failure information.Learnt from: LinZhihao-723 PR: y-scope/clp#558 File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14 Timestamp: 2024-10-14T03:42:10.355Z Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
🔇 Additional comments (26)
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (9)
14-14: LGTM: Correct include path for nlohmann/json.hppThe include statement for nlohmann/json.hpp is consistent with the project's coding standards.
291-301: LGTM: Updated schema tree structure commentThe comment has been appropriately updated to reflect the new structure of the schema tree, including the additional nodes. This change enhances the clarity of the test cases and helps developers understand the relationships between nodes.
307-307: LGTM: Updated schema tree construction and equality checkThe schema tree construction has been updated to include new nodes, aligning with the modified structure. The added equality check between auto-generated and user-generated schema trees is a good practice to ensure consistency in the test setup.
Also applies to: 316-318, 325-327
331-334: LGTM: Improved KeyValuePairLogEvent creation testsThe test for empty ID-value pairs has been updated, and a new test section for null schema tree pointers has been added. These changes enhance the test coverage by addressing edge cases and potential error conditions, improving the overall robustness of the test suite.
Also applies to: 340-357
364-364: LGTM: Refactored mismatched types test sectionThe mismatched types test section has been updated with improved consistency in test case setup. The refactoring to use the new
assert_kv_pair_log_event_creation_failurefunction enhances readability and maintainability of the error checking logic. These changes contribute to a more robust and easier-to-maintain test suite.Also applies to: 371-371, 378-378, 385-385, 392-392, 399-399, 413-428
433-434: LGTM: Enhanced valid ID-value pairs test sectionThe valid ID-value pairs test section has been significantly improved:
- New constants for JSON array encoding and static text have been added, enhancing test data consistency.
- The setup for valid node ID-value pairs has been updated to reflect the new schema structure.
- A new test section for JSON serialization has been added, ensuring correct output formatting.
- New test sections for duplicated key conflicts have been introduced, verifying the handling of invalid input scenarios.
These changes align well with the PR objectives of enhancing key-value pair support and fixing duplicated key detection issues. The additional test cases improve the overall robustness of the
KeyValuePairLogEventimplementation.Also applies to: 455-466, 477-497, 499-564
589-630: LGTM: Added test cases for invalid sub-treesNew test sections have been added to verify the handling of invalid sub-tree scenarios:
- A test case for an invalid sub-tree on node How to pass custom delimiters, dictionary and non-dictionary schemas #3, where the node is empty but its descendants appear in the schema tree.
- A similar test case for node Centos7.4 build failed #4, where the node is null but its descendants are present.
These additions are crucial for validating the integrity of the schema tree and ensure that the
KeyValuePairLogEventclass correctly handles inconsistent tree structures. This aligns well with the PR objectives of improving the validation of node-ID value pairs within the schema tree.
640-655: LGTM: Refactored out-of-bound node ID testThe out-of-bound node ID test has been updated to use the new
assert_kv_pair_log_event_creation_failurefunction. This change improves consistency with other test sections and simplifies the error checking logic, contributing to the overall maintainability of the test suite.
Line range hint
1-655: Overall assessment: Comprehensive test improvementsThe changes to this test file significantly enhance the test coverage for the
KeyValuePairLogEventclass. Key improvements include:
- Updated schema tree structure and construction.
- Enhanced tests for KeyValuePairLogEvent creation, including edge cases.
- Refactored and expanded mismatched types tests.
- New tests for JSON serialization and duplicated key conflicts.
- Additional tests for invalid sub-tree scenarios.
- Consistent use of a new assertion function for error checking.
These changes align well with the PR objectives of enhancing key-value pair support, improving serialization, and fixing duplicated key detection issues. The expanded test suite will help ensure the robustness and correctness of the
KeyValuePairLogEventimplementation.components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (9)
19-25: Documentation updates accurately reflect new functionalityThe class description now properly details the inclusion of both auto-generated and user-generated key-value pairs, providing clarity on the class's purpose.
34-42: Constructor documentation is comprehensiveThe updated documentation for the
createfactory function thoroughly explains parameter expectations and possible error codes, enhancing usability for developers.
45-48: Factory function signature aligns with new requirementsThe new parameters in the
createfunction accommodate both types of schema trees and node-ID value pairs, correctly reflecting the designed functionality.
64-70: Getter methods provide clear access to schema treesThe addition of
get_auto_generated_schema_tree()andget_user_generated_schema_tree()methods enhances code clarity by distinctly separating the two schema trees.
72-77: Getter methods for node-ID value pairs are appropriateThe methods
get_auto_generated_node_id_value_pairs()andget_user_generated_node_id_value_pairs()provide necessary access to the respective node-ID value pairs.
83-87: Update in serialization method documentation is accurateThe documentation for
serialize_to_json()effectively explains the return value and possible error codes, accurately reflecting the changes in the method.
95-98: Constructor implementation matches updated class designThe constructor correctly initializes the new member variables, reflecting the inclusion of both auto-generated and user-generated data.
101-104: Member initializations in constructor are appropriateThe use of
std::movein the initializer list ensures efficient assignment of member variables.
108-111: Private member variables are correctly definedThe addition of member variables for both schema trees and node-ID value pairs aligns with the updated class requirements.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (8)
199-202: Function Declaration of 'serialize_node_id_value_pairs_to_json' is CorrectThe declaration of
serialize_node_id_value_pairs_to_jsonis clear and follows the coding standards.
211-215: Function Declaration of 'check_key_uniqueness_among_sibling_nodes' is Well-DefinedThe function
check_key_uniqueness_among_sibling_nodesis properly declared with clear parameters and return type.
244-245: Efficient Initialization of 'key_duplication_checked_node_id_bitmap'Initializing
key_duplication_checked_node_id_bitmapwith the schema tree size ensures efficient tracking of checked nodes.
269-300: Validation Logic in 'validate_node_id_value_pairs' is SoundThe added logic for checking key uniqueness among ancestor nodes enhances validation and prevents duplicate keys across the schema hierarchy.
Line range hint
426-504: DFS Traversal in 'serialize_node_id_value_pairs_to_json' Implemented CorrectlyThe depth-first search traversal using
std::stackandJsonSerializationIteratorefficiently serializes the node ID value pairs into JSON.
505-525: Optimized Map Access in 'check_key_uniqueness_among_sibling_nodes'Using
findto retrieve the iterator avoids redundant lookups, enhancing performance when checking for key uniqueness among sibling nodes.
528-564: Constructor 'KeyValuePairLogEvent::create' Handles Inputs AppropriatelyThe method correctly validates both auto-generated and user-generated schema trees and node ID value pairs, ensuring robustness.
566-586: Serialization in 'serialize_to_json' is AccurateThe function accurately serializes auto-generated and user-generated key-value pairs, returning them as a pair of JSON objects.
|
Can you resolve the conflicts? |
LinZhihao-723
changed the title
KeyValuePairLogEvent.KeyValuePairLogEvent.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (2)
583-585: Consider improving code clarity around the empty initializer list.While the empty initializer list for auto-generated pairs aligns with the PR objectives (treating all pairs as user-generated during deserialization), consider improving clarity by either:
- Using a named variable with a descriptive name
- Adding a comment explaining why it's empty
+ // Auto-generated pairs are left unpopulated during deserialization + KeyValuePairLogEvent::NodeIdValuePairs empty_auto_generated_pairs; return KeyValuePairLogEvent::create( std::move(auto_generated_schema_tree), std::move(user_generated_schema_tree), - {}, + std::move(empty_auto_generated_pairs), std::move(node_id_value_pairs), utc_offset );
Line range hint
554-585: Consider tracking the auto-generated keys limitation.The code currently returns
std::errc::protocol_not_supportedfor auto-generated keys, which is documented in comments. Would you like me to create a GitHub issue to track the implementation of full auto-generated keys support in the deserializer?components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (2)
20-25: Enhance documentation with detailed descriptionsConsider providing additional details in the documentation to clarify the roles and interactions of the auto-generated and user-generated schema trees and their node-ID & value pairs within the
KeyValuePairLogEvent.
35-38: Add parameter descriptions in the@paramtagsThe
@paramtags in thecreatemethod list only the parameter names. Including brief descriptions for each parameter will improve code readability and maintainability.components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (4)
156-169: Provide detailed documentation forget_schema_subtree_bitmapThe
get_schema_subtree_bitmapfunction constructs a bitmap representing nodes in the schema tree. Adding more comprehensive comments or documentation explaining how the bitmap is generated and how it is utilized in the context of the schema tree would enhance code readability and maintainability.
273-277: Simplify conditional statement for clarityOn line 273, the condition can be simplified to improve readability:
if (!check_key_uniqueness_among_sibling_nodes(node, parent_node_id_to_key_names)) { return std::errc::protocol_not_supported; }
279-304: Optimize ancestral key duplication checksThe loop from lines 279 to 304 iteratively checks for key duplication among ancestor nodes. To improve performance, especially in deeply nested schema trees, consider implementing a memoization strategy or marking nodes as checked to prevent redundant processing.
[performance_issue]
430-435: Verify schema tree is not empty before serializationBefore proceeding with serialization in
serialize_node_id_value_pairs_to_json, confirm that the schema tree is not empty. This precaution can prevent unnecessary processing and potential errors related to empty data structures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp(9 hunks)components/core/src/clp/ffi/KeyValuePairLogEvent.hpp(3 hunks)components/core/src/clp/ffi/ir_stream/Deserializer.hpp(4 hunks)components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp(2 hunks)components/core/tests/test-ir_encoding_methods.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (5)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:119-120
Timestamp: 2024-11-10T16:46:58.543Z
Learning: `KeyValuePairLogEvent::create` requires `std::shared_ptr<SchemaTree>`, so `m_auto_generated_schema_tree` and `m_user_generated_schema_tree` need to be `std::shared_ptr`.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:119-120
Timestamp: 2024-11-10T16:46:58.543Z
Learning: `KeyValuePairLogEvent::create` requires `std::shared_ptr<SchemaTree>`, so `m_auto_generated_schema_tree` and `m_user_generated_schema_tree` need to be `std::shared_ptr`.
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:119-120
Timestamp: 2024-11-10T16:46:58.543Z
Learning: `KeyValuePairLogEvent::create` requires `std::shared_ptr<SchemaTree>`, so `m_auto_generated_schema_tree` and `m_user_generated_schema_tree` need to be `std::shared_ptr`.
🔇 Additional comments (13)
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (2)
60-63: Documentation looks good, eh!
The documentation clearly distinguishes between auto-generated and user-generated schema trees, maintaining consistency with the codebase's documentation style.
78-79: Parameter changes align with requirements, but let's verify usage
The parameter types correctly use std::shared_ptr<SchemaTree> as required by KeyValuePairLogEvent::create.
Let's verify that all callers have been updated to provide both schema trees:
✅ Verification successful
Let me generate additional scripts to verify the schema tree parameters are properly handled in the implementation:
Schema tree parameters are properly integrated and used throughout the codebase
The verification confirms that:
- The
Deserializerclass correctly initializes both schema trees as member variables - Both schema trees are properly passed to
deserialize_ir_unit_kv_pair_log_event - The schema trees are used appropriately in
KeyValuePairLogEventclass for various operations - All callers have been updated to handle both schema trees correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to deserialize_ir_unit_kv_pair_log_event to ensure they've been updated
# Search for function calls with context
rg -p "deserialize_ir_unit_kv_pair_log_event\s*\(" -A 5
# Search for test cases to ensure proper coverage
ast-grep --pattern 'TEST_CASE($_) {
$$$
deserialize_ir_unit_kv_pair_log_event($$$)
$$$
}'
Length of output: 1211
Script:
#!/bin/bash
# Check the implementation to verify schema tree usage
ast-grep --pattern 'deserialize_ir_unit_kv_pair_log_event($_) {
$$$
}'
# Check Deserializer constructor to verify schema tree initialization
rg -p "Deserializer.*constructor.*" -B 2 -A 10
# Find where schema trees are used within the implementation
rg -p "m_(auto|user)_generated_schema_tree" -A 3
Length of output: 2700
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (1)
554-555: LGTM! The function signature changes support the new schema tree requirements.
The addition of separate parameters for auto-generated and user-generated schema trees aligns with the PR objectives and maintains consistency with the KeyValuePairLogEvent::create requirements.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (5)
65-67: LGTM
The get_auto_generated_schema_tree() method properly returns a reference to the auto-generated schema tree.
69-70: LGTM
The get_user_generated_schema_tree() method properly returns a reference to the user-generated schema tree.
73-75: LGTM
The get_auto_generated_node_id_value_pairs() method correctly returns the auto-generated node-ID and value pairs.
77-79: LGTM
The get_user_generated_node_id_value_pairs() method correctly returns the user-generated node-ID and value pairs.
94-100: Ensure consistency in bitmap methods
Verify that get_user_generated_schema_subtree_bitmap() is implemented consistently with get_auto_generated_schema_subtree_bitmap(), ensuring uniform error handling and functionality.
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (4)
118-119: Initialization of schema trees with shared pointers is appropriate
The use of std::shared_ptr for m_auto_generated_schema_tree and m_user_generated_schema_tree is appropriate since KeyValuePairLogEvent::create requires std::shared_ptr<SchemaTree>.
187-193: Correct inclusion of schema trees in deserialization
The parameters m_auto_generated_schema_tree and m_user_generated_schema_tree are correctly passed to deserialize_ir_unit_kv_pair_log_event, ensuring proper deserialization of key-value pairs.
215-215: Effective duplicate node check in user-generated schema tree
Adding the check m_user_generated_schema_tree->has_node(node_locator) appropriately prevents duplicate nodes from being inserted, maintaining the integrity of the schema tree.
225-225: Previous comment on handling the return value of insert_node still applies
components/core/tests/test-ir_encoding_methods.cpp (1)
1249-1258: Tests correctly validate key-value pair serialization
The added test code appropriately checks that the number of user-generated key-value pairs matches the number of leaves in the JSON object. It also confirms that the auto_generated pairs are empty, which aligns with the expected behaviour at this stage of implementation.
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp
Outdated
Show resolved
Hide resolved
| std::shared_ptr<SchemaTree> auto_generated_schema_tree, | ||
| std::shared_ptr<SchemaTree> user_generated_schema_tree, |
There was a problem hiding this comment.
How about auto_gen_keys_schema_tree and user_gen_keys_schema_tree? The current names sound like the schema trees are auto-generated and user-generated themselves, rather than being based on fields that are auto-generated and user-generated.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1)
105-116: Consider using a custom struct for better readability.While the implementation is correct, using
std::pair<nlohmann::json, nlohmann::json>as the return type makes the code less self-documenting. Consider creating a custom struct with meaningful member names.Example:
struct SerializedKeyValuePairs { nlohmann::json auto_generated; nlohmann::json user_generated; };components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1)
Line range hint
215-225: Consider handling the node insertion result.While the code correctly uses the user-generated schema tree, it ignores the result of
insert_node. Consider handling potential insertion failures.-std::ignore = m_user_gen_keys_schema_tree->insert_node(node_locator); +auto insertion_result = m_user_gen_keys_schema_tree->insert_node(node_locator); +if (!insertion_result) { + return std::errc::protocol_error; +}components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
333-363: Consider optimizing the parent traversal loop.The parent traversal loop could be simplified by using the loop condition directly instead of breaking inside the loop. This would make the code more straightforward and eliminate the need for the comment about clang-tidy.
Apply this diff to optimize the loop:
- while (true) { - // Ideally, we'd use this if statement as the loop condition, but clang-tidy will - // complain about an unchecked `optional` access. - if (false == optional_parent_id.has_value()) { - // Reached the root - break; - } + while (optional_parent_id.has_value()) { auto const parent_id{optional_parent_id.value()}; if (schema_subtree_bitmap[parent_id]) { // Parent already set by other child break; } schema_subtree_bitmap[parent_id] = true; optional_parent_id = schema_tree.get_node(parent_id).get_parent_id(); - } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp(9 hunks)components/core/src/clp/ffi/KeyValuePairLogEvent.hpp(3 hunks)components/core/src/clp/ffi/ir_stream/Deserializer.hpp(4 hunks)components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp(2 hunks)components/core/tests/test-ffi_IrUnitHandlerInterface.cpp(2 hunks)components/core/tests/test-ffi_KeyValuePairLogEvent.cpp(7 hunks)components/core/tests/test-ir_encoding_methods.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp
- components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp
🧰 Additional context used
📓 Path-based instructions (6)
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (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/KeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/ffi/KeyValuePairLogEvent.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>.
📓 Learnings (4)
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (4)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:331-335
Timestamp: 2024-11-10T16:46:58.543Z
Learning: When reviewing code, ensure that suggestions about unnecessary use of `std::move` with `std::shared_ptr` are only made if `std::move` is actually used in the code.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:85-103
Timestamp: 2024-11-10T16:46:53.300Z
Learning: The function `assert_kv_pair_log_event_creation_failure` is correctly placed within the anonymous namespace in `test-ffi_KeyValuePairLogEvent.cpp`.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:228-236
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, helper functions like `assert_kv_pair_log_event_creation_failure` return booleans to the caller, and the caller asserts using `REQUIRE` to provide clearer failure information.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:119-120
Timestamp: 2024-11-10T16:46:58.543Z
Learning: `KeyValuePairLogEvent::create` requires `std::shared_ptr<SchemaTree>`, so `m_auto_generated_schema_tree` and `m_user_generated_schema_tree` need to be `std::shared_ptr`.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (4)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:474-480
Timestamp: 2024-12-09T15:25:14.043Z
Learning: In `components/core/src/clp/ffi/KeyValuePairLogEvent.cpp`, node IDs are validated before accessing `child_schema_tree_node` in the function `serialize_node_id_value_pairs_to_json`, ensuring `get_node` does not throw exceptions due to invalid node IDs.
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:102-102
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, the `get_next_child_schema_tree_node()` method in `JsonSerializationIterator` is always called after checking `has_next_child_schema_tree_node()`, ensuring proper iterator usage.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (3)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:474-480
Timestamp: 2024-12-09T15:25:14.043Z
Learning: In `components/core/src/clp/ffi/KeyValuePairLogEvent.cpp`, node IDs are validated before accessing `child_schema_tree_node` in the function `serialize_node_id_value_pairs_to_json`, ensuring `get_node` does not throw exceptions due to invalid node IDs.
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.
🔇 Additional comments (18)
components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (2)
90-96: LGTM! The test case correctly uses the updated interface.
The KeyValuePairLogEvent::create call has been properly updated to include both auto-generated and user-generated schema trees, along with their respective empty node-ID-value pairs.
134-134: LGTM! The assertion has been updated to match the new interface.
The test case now correctly verifies the emptiness of user-generated node-ID-value pairs.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (5)
20-26: LGTM! Clear and comprehensive documentation.
The class documentation effectively explains the distinction between auto-generated and user-generated components, providing a clear understanding of the class's structure.
35-49: LGTM! Factory function properly updated.
The factory function's documentation and signature have been correctly updated to handle both auto-generated and user-generated components.
65-79: LGTM! Getter methods are properly implemented.
The getter methods correctly provide access to both auto-generated and user-generated components. As confirmed by previous learnings, null checks are not needed since the factory function ensures the pointers are valid.
83-101: LGTM! Schema subtree bitmap methods are well-documented.
The methods are properly documented with clear explanations of their purpose and error handling.
121-130: LGTM! Constructor properly initializes all components.
The constructor correctly initializes both schema trees and their respective node-ID-value pairs in the proper order.
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (2)
118-119: LGTM! Schema trees are properly initialized.
The schema trees are correctly initialized as shared pointers, which is required for KeyValuePairLogEvent::create as confirmed by previous learnings.
187-193: LGTM! Deserialization properly handles both schema trees.
The deserialization logic has been correctly updated to pass both auto-generated and user-generated schema trees while maintaining proper error handling.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (4)
156-169: LGTM! Well-documented function declaration.
The function declaration is well-structured with clear documentation of parameters, return values, and error conditions.
502-522: LGTM! Efficient implementation with proper map access.
The function efficiently checks key uniqueness by:
- Using find instead of contains+at for map access
- Properly handling the case when parent has no existing keys
- Including clear comments about safety assumptions
525-561: LGTM! Robust input validation and proper use of move semantics.
The function implementation is solid with:
- Proper null checks for schema trees
- Validation of both auto-generated and user-generated node ID-value pairs
- Appropriate use of move semantics in construction
573-605: LGTM! Comprehensive error handling and efficient serialization.
The function implementation:
- Properly handles all potential error cases
- Returns early on errors to avoid unnecessary processing
- Uses move semantics appropriately for the JSON objects
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (4)
85-102: LGTM! Well-documented test helper function.
The function declaration follows the project's testing patterns with:
- Clear documentation of parameters and return value
- Appropriate use of boolean return for test assertions
- Consistent parameter types
221-237: LGTM! Efficient test helper implementation.
The implementation follows best practices:
- Uses move semantics appropriately
- Checks both error presence and specific error code
- Returns boolean for flexible assertion in the caller
340-357: LGTM! Comprehensive null pointer testing.
The test section thoroughly validates null pointer handling:
- Tests both auto-generated and user-generated schema trees
- Uses the helper function appropriately
- Has clear test cases
477-496: LGTM! Thorough JSON serialization testing.
The test section effectively validates the serialization:
- Verifies the complete JSON structure
- Checks both auto-generated and user-generated pairs
- Uses appropriate JSON equality assertions
components/core/tests/test-ir_encoding_methods.cpp (1)
1249-1256: LGTM! Comprehensive serialization validation.
The test changes effectively verify the serialization:
- Validates the number of key-value pairs matches JSON leaves
- Checks both auto-generated and user-generated pairs
- Verifies empty auto-generated pairs and correct user-generated JSON
| [[nodiscard]] auto assert_kv_pair_log_event_creation_failure( | ||
| std::shared_ptr<SchemaTree> auto_gen_keys_schema_tree, | ||
| std::shared_ptr<SchemaTree> user_gen_keys_schema_tree, | ||
| KeyValuePairLogEvent::NodeIdValuePairs auto_generated_node_id_value_pairs, |
There was a problem hiding this comment.
You still have a few instances of xxx_generated_ that I guess we want to replace? Perhaps search for _generated_ to ensure you haven't missed any.
There was a problem hiding this comment.
hmmm, I searched under src files. The rest should be kept unchanged.
I didn't search the test files, let me check
There was a problem hiding this comment.
Should be fixed.
Only is_auto_generated left in the src directory. Use _gen_ to replace _generated_ else where
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (2)
207-218: Consider enhancing the documentation forparent_node_id_to_key_names.While the function signature is clear, the documentation could better explain how the map parameter is used to track and validate key uniqueness.
Add more details to the documentation:
/** * @param node A non-root schema tree node. * @param parent_node_id_to_key_names + * Map tracking the set of key names used by siblings under each parent node. + * Updated to include the current node's key if it's unique. * @return true if `node`'s key is unique among its sibling nodes with `parent_node_id_to_key_names` * updated to keep track of this unique key name. * @return false if a sibling of `node` has the same key. */
573-605: Consider breaking down the serialization logic.While the implementation is correct, it could be more maintainable by extracting the repeated serialization pattern into a helper function.
Consider refactoring to reduce duplication:
+[[nodiscard]] auto serialize_schema_tree_to_json( + SchemaTree const& schema_tree, + KeyValuePairLogEvent::NodeIdValuePairs const& node_id_value_pairs +) -> OUTCOME_V2_NAMESPACE::std_result<nlohmann::json> { + auto const schema_subtree_bitmap_result{get_schema_subtree_bitmap(node_id_value_pairs, schema_tree)}; + if (schema_subtree_bitmap_result.has_error()) { + return schema_subtree_bitmap_result.error(); + } + return serialize_node_id_value_pairs_to_json( + schema_tree, + node_id_value_pairs, + schema_subtree_bitmap_result.value() + ); +} + auto KeyValuePairLogEvent::serialize_to_json( ) const -> OUTCOME_V2_NAMESPACE::std_result<std::pair<nlohmann::json, nlohmann::json>> { - auto const auto_gen_keys_schema_subtree_bitmap_result{get_auto_gen_keys_schema_subtree_bitmap()}; - if (auto_gen_keys_schema_subtree_bitmap_result.has_error()) { - return auto_gen_keys_schema_subtree_bitmap_result.error(); - } - auto serialized_auto_gen_kv_pairs_result{serialize_node_id_value_pairs_to_json( - *m_auto_gen_keys_schema_tree, - m_auto_gen_node_id_value_pairs, - auto_gen_keys_schema_subtree_bitmap_result.value() - )}; + auto serialized_auto_gen_kv_pairs_result{ + serialize_schema_tree_to_json(*m_auto_gen_keys_schema_tree, m_auto_gen_node_id_value_pairs)}; if (serialized_auto_gen_kv_pairs_result.has_error()) { return serialized_auto_gen_kv_pairs_result.error(); } - auto const user_gen_keys_schema_subtree_bitmap_result{get_user_gen_keys_schema_subtree_bitmap()}; - if (user_gen_keys_schema_subtree_bitmap_result.has_error()) { - return user_gen_keys_schema_subtree_bitmap_result.error(); - } - auto serialized_user_gen_kv_pairs_result{serialize_node_id_value_pairs_to_json( - *m_user_gen_keys_schema_tree, - m_user_gen_node_id_value_pairs, - user_gen_keys_schema_subtree_bitmap_result.value() - )}; + auto serialized_user_gen_kv_pairs_result{ + serialize_schema_tree_to_json(*m_user_gen_keys_schema_tree, m_user_gen_node_id_value_pairs)}; if (serialized_user_gen_kv_pairs_result.has_error()) { return serialized_user_gen_kv_pairs_result.error(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp(9 hunks)components/core/tests/test-ffi_KeyValuePairLogEvent.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
📓 Learnings (2)
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (4)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:331-335
Timestamp: 2024-11-10T16:46:58.543Z
Learning: When reviewing code, ensure that suggestions about unnecessary use of `std::move` with `std::shared_ptr` are only made if `std::move` is actually used in the code.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:85-103
Timestamp: 2024-11-10T16:46:53.300Z
Learning: The function `assert_kv_pair_log_event_creation_failure` is correctly placed within the anonymous namespace in `test-ffi_KeyValuePairLogEvent.cpp`.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:228-236
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, helper functions like `assert_kv_pair_log_event_creation_failure` return booleans to the caller, and the caller asserts using `REQUIRE` to provide clearer failure information.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (4)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:474-480
Timestamp: 2024-12-09T15:25:14.043Z
Learning: In `components/core/src/clp/ffi/KeyValuePairLogEvent.cpp`, node IDs are validated before accessing `child_schema_tree_node` in the function `serialize_node_id_value_pairs_to_json`, ensuring `get_node` does not throw exceptions due to invalid node IDs.
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:109-111
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, for the class `JsonSerializationIterator`, it's acceptable to use raw pointers for member variables like `m_schema_tree_node`, and there's no need to replace them with references or smart pointers in this use case.
Learnt from: LinZhihao-723
PR: y-scope/clp#554
File: components/core/src/clp/ffi/KeyValuePairLogEvent.cpp:102-102
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.cpp`, the `get_next_child_schema_tree_node()` method in `JsonSerializationIterator` is always called after checking `has_next_child_schema_tree_node()`, ensuring proper iterator usage.
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp:62-68
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In `KeyValuePairLogEvent.hpp`, the methods `get_auto_generated_schema_tree()` and `get_user_generated_schema_tree()` do not need to check for null shared pointers because the factory function ensures they are not null.
🔇 Additional comments (12)
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (6)
156-168: LGTM! Well-documented function declaration.
The function signature and documentation clearly describe the purpose, parameters, and error handling.
192-205: LGTM! Clear and concise function declaration.
The function signature and documentation effectively communicate the purpose, parameters, and error conditions.
333-363: LGTM! Efficient implementation with good error handling.
The implementation effectively:
- Handles out-of-range node IDs
- Uses efficient iteration to mark parent nodes
- Includes clear comments explaining the logic and clang-tidy workaround
502-522: LGTM! Efficient implementation with good comments.
The implementation:
- Uses efficient map operations with find()
- Properly handles both new and existing parent nodes
- Includes clear comments about safety assumptions
525-561: LGTM! Well-structured factory method implementation.
The implementation follows a clear sequence of validation steps with consistent error handling.
563-571: LGTM! Clean and consistent getter implementations.
The getters provide a clean interface to access schema subtree bitmaps.
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (6)
85-102: LGTM! Well-documented test helper function.
The function declaration is clear and properly scoped within the anonymous namespace.
221-237: LGTM! Implementation follows project's testing patterns.
The function correctly returns a boolean to allow the caller to use REQUIRE for clearer failure information, as per project convention.
Line range hint 303-325: LGTM! Clear and well-documented test setup.
The schema tree setup includes:
- Clear visual documentation of the tree structure
- Proper verification that trees are identical
Line range hint 338-494: LGTM! Comprehensive test coverage.
The test cases effectively validate:
- Error handling for null schema trees
- Successful creation with valid ID-value pairs
- Correct JSON serialization of both auto-generated and user-generated pairs
497-584: LGTM! Thorough duplicate key detection tests.
The test cases provide excellent coverage of duplicate key scenarios:
- Duplicate keys under different parent nodes
- Duplicate keys among siblings at different levels
- Clear documentation of which nodes are involved in each test
587-653: LGTM! Comprehensive invalid case handling.
The test cases effectively validate:
- Invalid subtree structures with empty/null nodes having descendants
- Out-of-bound node ID handling
- Appropriate error codes for each failure case
LinZhihao-723
changed the title
KeyValuePairLogEvent.KeyValuePairLogEvent; Detect and invalidate duplicate keys among non-leaf nodes when constructing a KeyValuePairLogEvent.
LinZhihao-723
changed the title
KeyValuePairLogEvent; Detect and invalidate duplicate keys among non-leaf nodes when constructing a KeyValuePairLogEvent.KeyValuePairLogEvent; Detect and invalidate duplicate keys among non-leaf nodes when constructing a KeyValuePairLogEvent.
…airLogEvent`; Detect and invalidate duplicate keys among non-leaf nodes when constructing a `KeyValuePairLogEvent`. (y-scope#558) Co-authored-by: kirkrodrigues <[email protected]>
Description
As planned in #556, we want to differentiate key-value pairs into two categories: auto-generated key-value pairs and user-generated key-value pairs. This PR adds support for auto/user-generated key-value pairs in
KeyValuePairLogEvent,where each set of key-value pairs is stored as a set of node-ID-value pairs with a reference to the associated schema tree.However, this PR does not yet support auto/user-generated kv pairs serialization/deserialization. The deserializer will output new
KeyValuePairLogEventinstances, but with all kv-pairs as user-generated ones, leaving auto-generated pairs empty.This PR also fixes a bug where duplicated keys are not correctly detected. Consider the following schema tree:
Before this PR, if we have node-ID-value pairs
[<2:"Value0">, <3:"Value1">], it will be considered valid with the schema tree above. However, it is actually invalid since there is an implicit duplicated key under the root node: both node 1 and node 2 have the key `"a". The previous implementation doesn't check key duplications among non-leaf nodes' siblings. This PR fixes this bug by checking key duplications among siblings for all the nodes from the leaf up to the root.Validation performed
KeyValuePairLogEventthat supports auto/user-generated kv pairs.KeyValuePairLogEventSummary by CodeRabbit
New Features
SchemaTreeand itsNodeclass for instance comparisons.Bug Fixes
Tests