feat(kv-ir): Add support for getting the number of log events read from the deserializer. #1282
Conversation
WalkthroughAdded a new public accessor Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)components/core/src/clp/ffi/ir_stream/Deserializer.hpp (2)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/core/src/clp/ffi/ir_stream/Deserializer.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp/ffi/ir_stream/Deserializer.hpp
🧬 Code graph analysis (1)
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (2)
components/core/src/clp_s/JsonParser.cpp (5)
nodiscard(49-54)nodiscard(56-61)nodiscard(63-69)nodiscard(71-74)nodiscard(76-79)components/core/tests/test-ir_encoding_methods.cpp (11)
nodiscard(88-88)nodiscard(90-90)nodiscard(92-92)nodiscard(106-112)nodiscard(114-119)nodiscard(131-144)nodiscard(146-149)nodiscard(152-152)nodiscard(154-156)nodiscard(158-160)nodiscard(254-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, false)
| /** | ||
| * @return The number of deserialized log events so far. | ||
| */ | ||
| [[nodiscard]] auto get_num_log_events_deserialized() const -> size_t { | ||
| return m_next_log_event_idx; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Accessor implementation LGTM; clarify semantics in docstring
The getter correctly exposes the running count via m_next_log_event_idx and matches the “scanned/deserialized” semantics (it increments before query filtering at Line 316–318). Consider tightening the comment to avoid ambiguity about query filtering and 0-based indexing.
- /**
- * @return The number of deserialized log events so far.
- */
+ /**
+ * @return The number of log events successfully deserialized (scanned) so far,
+ * regardless of query filtering. This equals the next 0-based log event index.
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @return The number of deserialized log events so far. | |
| */ | |
| [[nodiscard]] auto get_num_log_events_deserialized() const -> size_t { | |
| return m_next_log_event_idx; | |
| } | |
| /** | |
| * @return The number of log events successfully deserialized (scanned) so far, | |
| * regardless of query filtering. This equals the next 0-based log event index. | |
| */ | |
| [[nodiscard]] auto get_num_log_events_deserialized() const -> size_t { | |
| return m_next_log_event_idx; | |
| } |
🤖 Prompt for AI Agents
In components/core/src/clp/ffi/ir_stream/Deserializer.hpp around lines 161 to
167, tighten the getter docstring to explicitly state that
get_num_log_events_deserialized() returns the total number of log events
deserialized so far (equal to m_next_log_event_idx), that m_next_log_event_idx
is incremented before any query filtering so the count includes events that may
later be filtered out, and that the value is a 0-based size_t count starting at
0; update the comment accordingly to remove ambiguity about indexing and
filtering semantics.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
|
@coderabbitai y-scope/clp-ffi-js can be updated to replace the implementation of StructuredIrStreamReader::get_num_events_buffered() with this method. Currently, StructuredIrStreamReader has to maintain its own counter. create an issue in y-scope/clp-ffi-js to track the effort to update the implementation |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1114 @junhaoliao I've created issue y-scope/clp-ffi-js#111 in the y-scope/clp-ffi-js repository to track updating the The issue includes:
This will help consolidate the event counting logic and eliminate the need for StructuredIrStreamReader to maintain its own counter. 🐰✨ (thoughtful - adding useful tracking for future improvements) |
anlowee
changed the title
3 participants
Description
This PR added a getter for
Deserializerto get the number of deserialized log events so far. This will be used by Velox CLP-connector to get the scanned log events for batch control (which is consistent to reading archives, which can get the total number of log events in an archive).Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit