feat(clp-s): Add support for specifying authoritative timestamp in kv-ir ingestion flow #728
Conversation
…veWriter so it can be shared between ingestion paths
WalkthroughThe changes introduce enhanced timestamp management across several components. In the ArchiveWriter, new member variables and logic in the open, close, and add_node methods have been added to manage authoritative timestamps. Similarly, adjustments in the JSON parser update the timestamp matching logic, including renaming methods to return both node IDs and timestamp match statuses, as well as updating internal mappings. In addition, a conditional check in the command-line arguments parser has been removed to allow simultaneous use of certain arguments. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant ArchiveWriter
participant JSONParser
Main->>ArchiveWriter: open(options with authoritative_timestamp)
ArchiveWriter-->>ArchiveWriter: Initialize m_authoritative_timestamp
JSONParser->>ArchiveWriter: matches_timestamp(parent_node_id, key)
ArchiveWriter-->>JSONParser: Return timestamp match flag
JSONParser->>ArchiveWriter: add_node(..., matches_timestamp)
ArchiveWriter-->>ArchiveWriter: Update internal timestamp matching variables
Main->>ArchiveWriter: close()
ArchiveWriter-->>ArchiveWriter: Reset timestamp variables
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
components/core/src/clp_s/ArchiveWriter.cpp (1)
118-120: Consider extracting cleanup logic into a separate method.The cleanup of timestamp-related member variables is correct, but could be extracted into a dedicated method like
reset_timestamp_state()to improve maintainability and reusability.void ArchiveWriter::close() { // ... existing code ... - m_authoritative_timestamp.clear(); - m_matched_timestamp_prefix_length = 0ULL; - m_matched_timestamp_prefix_node_id = constants::cRootNodeId; + reset_timestamp_state(); } + +void ArchiveWriter::reset_timestamp_state() { + m_authoritative_timestamp.clear(); + m_matched_timestamp_prefix_length = 0ULL; + m_matched_timestamp_prefix_node_id = constants::cRootNodeId; +}components/core/src/clp_s/JsonParser.hpp (3)
124-139: Documentation needs update for new parameter.The documentation is missing the
@param matches_timestampentry and the return value description could be more specific about returning a node ID.Add the following to the documentation:
* @param parent_node_id ID of the parent of the IR node + * @param matches_timestamp Whether the node matches the timestamp column * @return The ID of the node added to the archive's Schema Tree
142-153: Documentation could be clearer about return value components.The return value description could be more specific about the components of the returned pair.
Suggest updating the return description to:
- * @return The ID of the corresponding node in the archive's schema tree and a flag indicating - * whether the field should be treated as a timestamp. + * @return A pair containing: + * - first: The ID of the corresponding node in the archive's schema tree + * - second: A flag indicating whether the field should be treated as a timestamp
216-217: Consider adding member variable documentation.The member variable
m_ir_node_to_archive_node_id_mappinglacks documentation explaining its purpose and the meaning of its components.Add documentation above the member variable:
/** * Maps IR node ID and type to archive node ID and timestamp flag. * The key is a pair of (IR node ID, node type). * The value is a pair of (archive node ID, is timestamp flag). */ absl::flat_hash_map<std::pair<uint32_t, NodeType>, std::pair<int32_t, bool>> m_ir_node_to_archive_node_id_mapping;components/core/src/clp_s/ArchiveWriter.hpp (4)
28-28: Consider clarifying the usage of multiple timestamps.Storing multiple strings in
authoritative_timestampis perfectly valid. However, if only a single authoritative timestamp is typically required, you may wish to document whether multiple entries are supported or expected.
100-119: Validate prefix matching logic and handle mismatches carefully.The incremental matching approach is logical. However, consider ensuring that
m_matched_timestamp_prefix_lengthis reset or handled explicitly if a mismatch occurs. Additionally, you could usem_authoritative_timestamp.empty()instead ofm_authoritative_timestamp.size() > 0for clarity.
121-136: Mark the method as const and simplify the difference check.The implementation looks correct. You can declare
matches_timestamp(...)as aconstmethod since it does not modify member variables. Also, consider storing the difference(m_authoritative_timestamp.size() - m_matched_timestamp_prefix_length)in a local variable for readability.
273-275: Add explanatory comments for new fields.These new member variables support authoritative timestamp tracking. Adding doxygen or inline comments describing their purpose and usage would improve maintainability.
components/core/src/clp_s/JsonParser.cpp (1)
634-647: Adjust numeric timestamps if applicable
The logic correctly converts string-based node types toDateStringwhenevermatches_timestampis true. If numeric timestamps are intended to be stored in a specialized type (e.g.,Integervs.DateInteger), consider extending this function’s logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/core/src/clp_s/ArchiveWriter.cpp(2 hunks)components/core/src/clp_s/ArchiveWriter.hpp(4 hunks)components/core/src/clp_s/CommandLineArguments.cpp(0 hunks)components/core/src/clp_s/JsonParser.cpp(8 hunks)components/core/src/clp_s/JsonParser.hpp(2 hunks)
💤 Files with no reviewable changes (1)
- components/core/src/clp_s/CommandLineArguments.cpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/ArchiveWriter.hppcomponents/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/JsonParser.hppcomponents/core/src/clp_s/ArchiveWriter.cpp
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (10)
components/core/src/clp_s/ArchiveWriter.cpp (1)
21-21: LGTM! Initialization of authoritative timestamp.The initialization of
m_authoritative_timestampfrom the option parameter aligns with the PR's objective to support authoritative timestamps in the ingestion flow.components/core/src/clp_s/JsonParser.hpp (1)
114-122: LGTM! Well-documented static method addition.The new static method is well-documented and appropriately designed for adjusting node types based on timestamp matching.
components/core/src/clp_s/ArchiveWriter.hpp (1)
11-11: No concerns with this added include.The addition of
archive_constants.hppappears aligned with the new references (e.g.,constants::cRootNodeId), and there are no conflicts with existing imports.components/core/src/clp_s/JsonParser.cpp (7)
106-106: Ensure non-empty timestamp key before assignment
Ifm_timestamp_keyis blank,authoritative_timestampwill be set to an empty descriptor. Confirm that this assignment is only done when a valid timestamp key has been parsed.
360-361: Looks good
The usage ofmatches_timestamphere is clear and consistent.
397-398: No issues detected
This timestamp check aligns with the approach used for numeric values.
456-456: Double-check do-while control flow
This loop’s condition suggests you are relying onhit_endto break out. Confirm thathit_endis correctly set to avoid logical missteps.
649-668: Implementation looks solid
The additional parameter and updated logic for timestamp-aware node creation are well integrated.
670-737: Good extension for timestamp checks
Returning both node ID andmatches_timestamptogether simplifies further logic.
742-832: Timestamp ingestion is properly woven into parse logic
Integratingmatches_timestampensures that relevant fields are recognized and processed. The flow looks consistent and correct.
| @@ -95,8 +97,42 @@ class ArchiveWriter { | |||
| * @param key | |||
| * @return the node id | |||
| */ | |||
| int32_t add_node(int parent_node_id, NodeType type, std::string_view const key) { | |||
| return m_schema_tree.add_node(parent_node_id, type, key); | |||
| int32_t add_node(int parent_node_id, NodeType type, std::string_view key) { | |||
There was a problem hiding this comment.
Are you putting the implementation of those two functions here (not source file) for better performance?
| @@ -95,8 +97,42 @@ class ArchiveWriter { | |||
| * @param key | |||
| * @return the node id | |||
| */ | |||
| int32_t add_node(int parent_node_id, NodeType type, std::string_view const key) { | |||
| return m_schema_tree.add_node(parent_node_id, type, key); | |||
| int32_t add_node(int parent_node_id, NodeType type, std::string_view key) { | |||
There was a problem hiding this comment.
And can you add more description of this method?
| if (NodeType::Object == type) { | ||
| m_matched_timestamp_prefix_node_id = node_id; | ||
| } | ||
| } else if ((m_authoritative_timestamp.size() - m_matched_timestamp_prefix_length) > 1) { |
There was a problem hiding this comment.
We can probably remove a pair of parentheses?
| if (NodeType::Object == type) { | ||
| m_matched_timestamp_prefix_node_id = node_id; | ||
| } | ||
| } else if ((m_authoritative_timestamp.size() - m_matched_timestamp_prefix_length) > 1) { |
There was a problem hiding this comment.
I'm not sure whether the compiler will optimize repeated calls to m_authoritative_timestamp.size(). Since the value remains constant, would it be more efficient to store it in a variable and reuse it?
There was a problem hiding this comment.
Do you have any numbers of the ingestion performance before and after this timestamp matching rewriting?
| return m_schema_tree.add_node(parent_node_id, type, key); | ||
| int32_t add_node(int parent_node_id, NodeType type, std::string_view key) { | ||
| auto const node_id{m_schema_tree.add_node(parent_node_id, type, key)}; | ||
| if (m_matched_timestamp_prefix_node_id == parent_node_id |
There was a problem hiding this comment.
Since we only perform operations when NodeType::Object == type, can we put it here?
| if (m_matched_timestamp_prefix_node_id == parent_node_id | ||
| && m_authoritative_timestamp.size() > 0) | ||
| { | ||
| if (constants::cRootNodeId == parent_node_id) { | ||
| if (NodeType::Object == type) { | ||
| m_matched_timestamp_prefix_node_id = node_id; | ||
| } | ||
| } else if ((m_authoritative_timestamp.size() - m_matched_timestamp_prefix_length) > 1) { | ||
| if (NodeType::Object == type | ||
| && m_authoritative_timestamp.at(m_matched_timestamp_prefix_length) == key) | ||
| { | ||
| m_matched_timestamp_prefix_length += 1; | ||
| m_matched_timestamp_prefix_node_id = node_id; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if we can write it in this way so that we can get rid of a lot of checks for irrelevant nodes.
| if (m_matched_timestamp_prefix_node_id == parent_node_id | |
| && m_authoritative_timestamp.size() > 0) | |
| { | |
| if (constants::cRootNodeId == parent_node_id) { | |
| if (NodeType::Object == type) { | |
| m_matched_timestamp_prefix_node_id = node_id; | |
| } | |
| } else if ((m_authoritative_timestamp.size() - m_matched_timestamp_prefix_length) > 1) { | |
| if (NodeType::Object == type | |
| && m_authoritative_timestamp.at(m_matched_timestamp_prefix_length) == key) | |
| { | |
| m_matched_timestamp_prefix_length += 1; | |
| m_matched_timestamp_prefix_node_id = node_id; | |
| } | |
| } | |
| } | |
| if (NodeType::Object == type && | |
| m_matched_timestamp_prefix_node_id == parent_node_id | |
| && m_authoritative_timestamp.size() > m_matched_timestamp_prefix_length + 1) | |
| { | |
| if (constants::cRootNodeId == parent_node_id) { | |
| m_matched_timestamp_prefix_node_id = node_id; | |
| } else if (m_authoritative_timestamp.at(m_matched_timestamp_prefix_length) == key) { | |
| m_matched_timestamp_prefix_length += 1; | |
| m_matched_timestamp_prefix_node_id = node_id; | |
| } |
Description
This PR adds support for specifying authoritative timestamps in the kv-ir ingestion flow while refactoring authoritative timestamp extraction in the JSON ingestion flow. Logic to match against authoritative timestamps has been moved into ArchiveWriter so that each ingestion pathway wrapping ArchiveWriter doesn't have to implement as much timestamp matching logic on its own. In particular
add_nodenow executes some hooks to progressively match the authoritative timestamp column as the SchemaTree is constructed and a newmatches_timestampfunction has been added to check if a leaf node matches the authoritative timestamp column.Checklist
breaking change.
Validation performed
Summary by CodeRabbit