-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for serializing/deserializing auto-generated key-value pairs in the new IR format. #127
Conversation
WalkthroughThis pull request introduces comprehensive changes to the log event processing system, focusing on separating auto-generated and user-generated key-value pairs. The modifications span multiple files across the project, including native interface definitions, serialization and deserialization methods, and test cases. The primary enhancement is the ability to handle two distinct types of key-value pairs: auto-generated and user-generated, which requires updates to method signatures, return types, and internal logic in various components. Changes
Sequence DiagramsequenceDiagram
participant Client
participant KeyValuePairLogEvent
participant Serializer
participant Deserializer
Client->>KeyValuePairLogEvent: Create with auto_gen and user_gen dicts
KeyValuePairLogEvent-->>Client: Log Event Object
Client->>Serializer: Serialize with separate msgpack maps
Serializer-->>Client: Serialized Data
Client->>Deserializer: Deserialize with separate msgpack maps
Deserializer-->>Client: Deserialized Log Event
Possibly related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/wrapped_facade_headers/Python.hpp (1)Pattern ⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (2)
Line range hint
324-484
: Refactor to eliminate code duplication in dictionary handlingThe function
convert_py_dict_to_key_value_pair_log_event
contains similar blocks for serializing and unpacking bothauto_gen_kv_pairs_dict
anduser_gen_kv_pairs_dict
. Refactoring the common logic into helper functions would enhance maintainability and readability.
735-789
: Consider refactoringto_dict
method to reduce repetitionThe
to_dict
method processes auto-generated and user-generated key-value pairs with similar logic. Abstracting the shared functionality into a helper function could reduce code duplication and improve maintainability.src/clp_ffi_py/ir/native/PySerializer.hpp (1)
97-105
: LGTM! Consider enhancing the documentation.The separation of auto-generated and user-generated key-value pairs is well-implemented. The parameter names are clear and descriptive.
Consider adding more details to the documentation about:
- The expected format of the msgpack maps
- Whether either map can be empty
- Any size limitations or constraints
tests/test_ir/test_serder.py (2)
43-58
: Enhance the method documentation.While the implementation is clean and correct, the docstring could be more detailed.
Consider adding:
- Example of the expected input format
- Explanation of what constitutes auto-generated vs user-generated data
- Any constraints on the dictionary contents
129-140
: Consider making tests more deterministic.While the test coverage is good, the use of current timestamp makes the tests time-dependent.
Consider using a fixed timestamp for testing to ensure consistent and reproducible results.
tests/test_ir/test_serializer.py (1)
77-80
: Enhance test coverage with different data.Using the same msgpack sequence for both auto_gen and user_gen maps might miss edge cases.
Consider adding test cases where auto_gen and user_gen maps contain different data to ensure proper handling of distinct content in each map.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
clp_ffi_py/ir/native.pyi
(3 hunks)src/clp
(1 hunks)src/clp_ffi_py/ir/native/PyDeserializer.cpp
(1 hunks)src/clp_ffi_py/ir/native/PyDeserializer.hpp
(2 hunks)src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp
(8 hunks)src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp
(1 hunks)src/clp_ffi_py/ir/native/PySerializer.cpp
(4 hunks)src/clp_ffi_py/ir/native/PySerializer.hpp
(1 hunks)src/clp_ffi_py/utils.cpp
(2 hunks)src/clp_ffi_py/utils.hpp
(2 hunks)tests/test_ir/test_key_value_pair_log_event.py
(1 hunks)tests/test_ir/test_serder.py
(5 hunks)tests/test_ir/test_serializer.py
(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/clp
🧰 Additional context used
📓 Path-based instructions (8)
src/clp_ffi_py/utils.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PyDeserializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PyDeserializer.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PySerializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/utils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PySerializer.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
📓 Learnings (1)
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#81
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp:42-44
Timestamp: 2024-11-10T16:46:58.543Z
Learning: In the C++ codebase, for consistency with other classes, methods like `get_kv_pair_log_event()` in `src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp` should return pointers instead of references.
🪛 cppcheck (2.10-2)
src/clp_ffi_py/utils.cpp
[performance] 96-96: Using std
(returnStdMoveLocal)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Build cp312-win_amd64
- GitHub Check: Build cp313-musllinux_aarch64
- GitHub Check: Build cp313-manylinux_x86_64
- GitHub Check: Build cp313-manylinux_i686
- GitHub Check: Build cp313-manylinux_aarch64
- GitHub Check: Build cp312-musllinux_x86_64
- GitHub Check: Build cp312-musllinux_aarch64
- GitHub Check: Build cp312-manylinux_aarch64
- GitHub Check: Build cp311-musllinux_aarch64
- GitHub Check: Build cp311-manylinux_aarch64
- GitHub Check: Build cp310-musllinux_aarch64
- GitHub Check: Build cp310-manylinux_aarch64
- GitHub Check: Build cp313-macosx_universal2
- GitHub Check: Build cp313-macosx_arm64
- GitHub Check: Build cp312-macosx_x86_64
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (22)
src/clp_ffi_py/ir/native/PyDeserializer.hpp (2)
124-127
: LGTM! Well-structured type alias update.The addition of the
is_auto_generated
parameter toSchemaTreeNodeInsertionHandle
effectively supports the distinction between auto-generated and user-generated key-value pairs, which is a core requirement of this PR.
166-169
: LGTM! Consistent implementation of the handler method.The method correctly forwards both the
is_auto_generated
flag and theschema_tree_node_locator
to the handler, maintaining consistency with the type alias update.src/clp_ffi_py/ir/native/PyDeserializer.cpp (1)
195-196
: LGTM! Consistent lambda implementation.The trivial handler correctly implements the updated interface, properly marking unused parameters and maintaining the expected behaviour.
src/clp_ffi_py/ir/native/PySerializer.cpp (4)
55-70
: Documentation updated to reflect new parametersThe docstring for
serialize_log_event_from_msgpack_map
accurately describes the new parametersauto_gen_msgpack_map
anduser_gen_msgpack_map
, including their types and the exceptions that may be raised.
72-76
: Method signature now accepts keyword argumentsThe function
PySerializer_serialize_log_event_from_msgpack_map
has been correctly updated to acceptargs
andkeywords
, allowing for keyword arguments in function calls.
297-325
: Arguments parsed appropriately for new parametersThe implementation correctly parses the two new byte string parameters using
PyArg_ParseTupleAndKeywords
, ensuring that bothauto_gen_msgpack_map
anduser_gen_msgpack_map
are obtained properly.
465-490
: Serialization logic handles separate msgpack maps correctlyThe
serialize_log_event_from_msgpack_map
method appropriately unpacks and serializes the separate auto-generated and user-generated msgpack maps, ensuring that both are processed correctly.src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (2)
Line range hint
69-75
: Handler updated to accommodate additional parameterThe inclusion of
is_auto_generated
inhandle_schema_tree_node_insertion
correctly updates the interface to distinguish between auto-generated and user-generated schema nodes.
243-254
: Constructor documentation reflects new parametersThe docstring for
PyKeyValuePairLogEvent
now accurately details theauto_gen_kv_pairs
anduser_gen_kv_pairs
parameters, including their expected types and usage.tests/test_ir/test_key_value_pair_log_event.py (1)
24-39
: Test updated to validate new parametersThe
test_basic
method now effectively tests theKeyValuePairLogEvent
initialization with bothauto_gen_kv_pairs
anduser_gen_kv_pairs
, and verifies that theto_dict
method returns the expected dictionaries, enhancing test coverage for the new functionality.src/clp_ffi_py/utils.cpp (2)
5-9
: LGTM! Headers are appropriately included.The addition of
<optional>
and<utility>
headers is necessary to support the newunpack_msgpack_map
function.
82-97
: Well-implemented function with proper error handling!The implementation includes:
- Comprehensive error handling with appropriate Python exceptions
- Proper validation of the msgpack type
- Efficient return of the object handle using move semantics
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 96-96: Using std
(returnStdMoveLocal)
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp (1)
88-95
: Documentation and return type changes are well-aligned!The changes properly reflect the new structure returning auto-generated and user-generated key-value pairs as separate dictionaries within a tuple. The return type change from
PyDictObject*
toPyObject*
is appropriate for the new tuple return type.clp_ffi_py/ir/native.pyi (2)
89-90
: LGTM! Type hints are consistent with the C++ implementation.The separation of auto-generated and user-generated key-value pairs in both the constructor and return type is properly reflected in the type hints.
101-103
: LGTM! Method signature properly updated.The separation of auto-generated and user-generated msgpack maps is correctly reflected in the method signature.
src/clp_ffi_py/utils.hpp (2)
9-9
: LGTM! Required header properly included.The addition of
<optional>
header is necessary for the newunpack_msgpack_map
function's return type.
82-91
: LGTM! Function declaration is well-documented and properly structured.The declaration includes:
- Comprehensive documentation of parameters and return values
- Appropriate use of [[nodiscard]] attribute
- Clear error handling documentation
tests/test_ir/test_serder.py (2)
2-2
: LGTM! Type imports are properly updated.The addition of
Tuple
to the imports supports the new tuple-based structure for key-value pairs.
88-96
: LGTM! Well-structured deserialization logic.The implementation correctly handles the unpacking of auto-generated and user-generated dictionaries, with proper type assertions.
tests/test_ir/test_serializer.py (3)
69-69
: LGTM! Clear variable declaration.The msgpack_byte_sequence variable is well-placed and properly typed.
94-97
: Same concern about test data variety applies here.
122-125
: Same concern about test data variety applies here.
@@ -86,8 +86,8 @@ class FourByteDeserializer: | |||
) -> Optional[LogEvent]: ... | |||
|
|||
class KeyValuePairLogEvent: | |||
def __init__(self, dictionary: Dict[Any, Any]): ... | |||
def to_dict(self) -> Dict[Any, Any]: ... | |||
def __init__(self, auto_gen_kv_pairs: Dict[Any, Any], user_gen_kv_pairs: Dict[Any, Any]): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we swap the order of user_gen_kv_pairs
& auto_gen_kv_pairs
and make auto_gen_kv_pairs
optional? That way we can achieve backward compatibility.
(That said, I can see any generic developers might not need to call this constructor.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor is mainly for testing purpose; we usually construct KeyValuePairLogEvent
directly from the deserializer in CPython level.
But I think your comment might make sense in Serializer
's API changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought a little more. I feel like we shouldn't allow optional input even in Serializer
. As this is the low level API, both values should be explicitly given. Also, most users won't use this API directly anyways. They should use Python logging library to log their kv pairs, which will be handled as user-generated by the loglib's code.
def __init__(self, dictionary: Dict[Any, Any]): ... | ||
def to_dict(self) -> Dict[Any, Any]: ... | ||
def __init__(self, auto_gen_kv_pairs: Dict[Any, Any], user_gen_kv_pairs: Dict[Any, Any]): ... | ||
def to_dict(self) -> Tuple[Dict[Any, Any], Dict[Any, Any]]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That whether kv-pairs are auto-generated might not matter to all developers. Can we also provide a helper to return a single dict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, my understanding is this should be the low level API, so we should return whatever given on the serialization end.
If you are suggesting merging two dicts into one, I think it would also be problematic because:
- We can't simply merge them into one since there might be key conflicts in two dicts
- If we add keys on the top level, like
{"auto_gen": auto_gen_dict, "user_gen": user_gen_dict}
, it would be a little confusing since the returned dictionaries aren't exactly what user inputs are and we need more documents to clarify things. It might be better just to return two dictionaries separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, i was proposing that we can simply merge the two dicts into one. That there can be potential key conflicts, though rare, is valid concern.
If we are to keep returning a tuple, having the user-kv-pairs as the first element might be more flexible to developers. e.g., they can write
kv_pairs, = log_event.to_dict()
to only get the user inserted pairs, if they don't care about the auto generated ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, the current ordering strictly follows the underlying ffi core APIs.
If really necessary, we can add APIs like get_user_gen_kv_pairs_as_dict
and get_auto_gen_kv_pairs_as_dict
in future PRs?
* @return nullptr on failure with the relevant Python exception and error set. | ||
*/ | ||
[[nodiscard]] auto to_dict() -> PyDictObject*; | ||
[[nodiscard]] auto to_dict() -> PyObject*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, any reason we don't return PyTupleObject*
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used Py_BuildValue
to create the tuple, which returns PyObject*
.
Reference: https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tuple APIs would also return PyObject*
directly, like this one (which is equivalent to Py_BuildValue
): https://docs.python.org/3/c-api/tuple.html#c.PyTuple_Pack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Right, I read from the https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue docs that the API is not guaranteed to return a tuple, but since we have the format string fixed, it should not return anything else i assume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swith to PyTuple_Pack since it doesn't need argument parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the implementation and docs and didn't find anything too odd. I left some suggestions regarding the Python interfaces. Feel free to object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/clp_ffi_py/utils.cpp (1)
82-82
: Consider adding a docstring to document the function's behaviour.Adding a docstring would improve maintainability by documenting:
- Purpose of the function
- Parameter description
- Return value semantics
- Error conditions
Here's a suggested docstring:
/** * Unpacks a msgpack byte sequence and verifies it contains a map. * @param msgpack_byte_sequence Span containing the msgpack data * @return The unpacked object handle if successful * @return std::nullopt if an error occurs (sets Python exception) */src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
Line range hint
431-484
: Consider optimizing the conversion process.While the implementation is correct and safe, the current approach of serializing to msgpack and then deserializing could be optimized. As noted in the TODO comment, a direct conversion utility would be more efficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp
(8 hunks)src/clp_ffi_py/utils.cpp
(2 hunks)src/clp_ffi_py/utils.hpp
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/clp_ffi_py/utils.hpp
🧰 Additional context used
📓 Path-based instructions (2)
src/clp_ffi_py/utils.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
🪛 cppcheck (2.10-2)
src/clp_ffi_py/utils.cpp
[performance] 96-96: Using std
(returnStdMoveLocal)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Build cp311-win_amd64
- GitHub Check: Build cp310-win_amd64
- GitHub Check: Build cp311-musllinux_x86_64
- GitHub Check: Build cp311-musllinux_i686
- GitHub Check: Build cp311-musllinux_aarch64
- GitHub Check: Build cp311-manylinux_x86_64
- GitHub Check: Build cp311-manylinux_i686
- GitHub Check: Build cp311-manylinux_aarch64
- GitHub Check: Build cp310-musllinux_x86_64
- GitHub Check: Build cp310-musllinux_i686
- GitHub Check: Build cp310-musllinux_aarch64
- GitHub Check: Build cp310-manylinux_x86_64
- GitHub Check: Build cp310-manylinux_i686
- GitHub Check: Build cp310-manylinux_aarch64
- GitHub Check: Build cp311-macosx_arm64
- GitHub Check: Build cp310-macosx_x86_64
- GitHub Check: Build cp310-macosx_universal2
- GitHub Check: Build cp310-macosx_arm64
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (6)
src/clp_ffi_py/utils.cpp (2)
5-9
: LGTM! Headers are properly organized.The new headers are correctly placed and necessary for the added functionality.
82-97
: LGTM! Implementation is correct and handles errors appropriately.The function properly handles error cases and sets appropriate Python exceptions. The use of
std::move
is correct here asmsgpack::object_handle
is not trivially copyable.🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 96-96: Using std
(returnStdMoveLocal)
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (4)
69-69
: LGTM!The addition of the
is_auto_generated
parameter is properly marked as unused and maintains the simple success-returning implementation.
243-254
: LGTM! Documentation is clear and follows conventions.The docstrings are well-written, properly documenting:
- The new constructor parameters with their types
- The updated return type of
to_dict
methodAlso applies to: 268-272
375-414
: LGTM! Constructor implementation is robust and safe.The implementation:
- Properly handles keyword arguments
- Includes thorough type checking
- Provides clear error messages
- Ensures memory safety with proper initialization
735-789
: LGTM! Dictionary serialization is well-implemented.The implementation:
- Cleanly separates auto-generated and user-generated key-value pairs
- Includes comprehensive error handling with detailed messages
- Uses RAII patterns for safe memory management
- Has a clear and logical structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/clp_ffi_py/ir/native/PySerializer.cpp (3)
55-70
: Consider enhancing the documentation with examples.While the documentation clearly describes the parameters, it would be more helpful to include examples of the expected format for both
auto_gen_msgpack_map
anduser_gen_msgpack_map
to guide users in preparing the correct input.
473-479
: Enhance error messages to distinguish between map types.When unpacking fails, the error message doesn't indicate which map (auto-generated or user-generated) caused the failure. Consider customizing the error messages for better debugging.
484-491
: Be more specific with NOLINT directives.The NOLINT directives could be more specific about which exact rules are being suppressed. Consider using specific rule identifiers instead of the broad category.
Replace:
-// NOLINTBEGIN(cppcoreguidelines-pro-type-union-access) +// NOLINTBEGIN(cppcoreguidelines-pro-type-union-access, cppcoreguidelines-pro-type-reinterpret-cast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/clp_ffi_py/ir/native/PySerializer.cpp
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/clp_ffi_py/ir/native/PySerializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}
: - Prefer false == <expression>
rather than !<expression>
.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Build cp311-win_amd64
- GitHub Check: Build cp310-win_amd64
- GitHub Check: Build cp311-musllinux_x86_64
- GitHub Check: Build cp311-musllinux_i686
- GitHub Check: Build cp311-musllinux_aarch64
- GitHub Check: Build cp311-manylinux_x86_64
- GitHub Check: Build cp311-manylinux_i686
- GitHub Check: Build cp311-manylinux_aarch64
- GitHub Check: Build cp310-musllinux_x86_64
- GitHub Check: Build cp310-musllinux_i686
- GitHub Check: Build cp310-musllinux_aarch64
- GitHub Check: Build cp310-manylinux_x86_64
- GitHub Check: Build cp310-manylinux_i686
- GitHub Check: Build cp310-manylinux_aarch64
- GitHub Check: Build cp311-macosx_arm64
- GitHub Check: Build cp310-macosx_x86_64
- GitHub Check: Build cp310-macosx_universal2
- GitHub Check: Build cp310-macosx_arm64
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (3)
src/clp_ffi_py/ir/native/PySerializer.cpp (3)
72-76
: LGTM! Good use of keyword arguments.The change to support keyword arguments makes the API more Pythonic and improves usability.
161-161
: LGTM! Correct method table update.The addition of
METH_KEYWORDS
flag is necessary and correctly implemented to support keyword arguments.
297-325
: Consider adding size validation for input maps.While the parameter parsing is correct, consider adding validation for the size of input maps to prevent potential memory issues with extremely large inputs. You could use
m_buffer_size_limit
as a reference for maximum allowed size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python interfaces lgtm. Code logic matches their descriptions. Preferably someone with more C++ experience should also take a look. Otherwise all is good.
@@ -86,8 +86,8 @@ class FourByteDeserializer: | |||
) -> Optional[LogEvent]: ... | |||
|
|||
class KeyValuePairLogEvent: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too sure how we are going to implement the compatibility layer for CLP Text IR Streams. In the future, will log events also be accessed through this interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, timestamp
will be stored inside the auto-generated dictionary and msg
will be stored inside the user-generated dictionary.
Description
This PR updates to the latest CLP core commit to adapt the feature enhancement introduced in this PR. From this PR, we have the following supports:
Serializer
's APIDeserializer
's implementation to adapt the latest core ffi API changesKeyValuePairLogEvent
's APIKeyValuePairLogEvent
object from auto-gen and user-gen Python dictionariesTo test the new features, this PR:
Validation performed
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests
These updates provide more flexible and precise log event processing, allowing for better separation of automatically generated and user-generated metadata.