feat(kv-ir): Implement QueryHandler's evaluate_kv_pair_log_event.
#882
Conversation
## Walkthrough
This change implements a full-featured AST evaluation framework for search queries in the IR stream search module. It introduces an iterator-based, stack-driven traversal of logical and filter expressions, supports bitmask evaluation results, enhances error handling, and updates the evaluation interface to operate on complete log events rather than separate node-value pairs.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp | Changed `AstEvaluationResult` from a scoped enum class to a plain enum with explicit bitmask values; added a bitmask type alias for easier bitwise operations. |
| components/core/src/clp/ffi/ir_stream/search/ErrorCode.cpp | Added error messages for three new error codes; removed the message for `QueryExpressionIsNull`. |
| components/core/src/clp/ffi/ir_stream/search/ErrorCode.hpp | Added three new error codes (`AstEvaluationInvariantViolation`, `AttemptToIterateAstLeafExpr`, `ExpressionTypeUnexpected`) and removed `QueryExpressionIsNull` from the error code enum. |
| components/core/src/clp/ffi/ir_stream/search/QueryHandler.hpp | Replaced the method for evaluating node-value pairs with a new method that evaluates a full log event; updated documentation and interface accordingly. |
| components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp | Implemented a recursive, stack-based AST evaluation mechanism with iterator support for logical and filter expressions; added helpers for filter evaluation, wildcard handling, and error propagation; updated query preprocessing and initialization logic. |
| components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp | Added `AstExprIterator` nested class for AST traversal; replaced the evaluation interface to accept log events; introduced new private methods and members for stack-based evaluation and error handling; updated member variable naming and method parameters for clarity and correctness. |
| components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp | Added helper functions to generate matchable and unmatchable values for schema node types; added a new test case verifying evaluation of log events against queries with various logical conditions and namespaces; updated existing tests to align with new query handling logic. |
| components/core/src/clp/ffi/ir_stream/search/test/test_utils.cpp | Removed the template function `get_encoded_text_ast` that parsed and encoded strings into `EncodedTextAst` instances; updated includes accordingly. |
| components/core/src/clp/ffi/ir_stream/search/test/utils.hpp | Added a templated utility function `get_encoded_text_ast` constrained to specific encoded variable types; implements encoding of input text into `EncodedTextAst` with assertions for correctness. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant QueryHandler
participant QueryHandlerImpl
participant AST
participant SchemaTree
Client->>QueryHandler: evaluate_kv_pair_log_event(log_event)
QueryHandler->>QueryHandlerImpl: evaluate_kv_pair_log_event(log_event)
QueryHandlerImpl->>AST: Traverse AST with AstExprIterator (DFS)
loop For each AST node
AST-->>QueryHandlerImpl: Node type (And, Or, Filter)
alt FilterExpr
QueryHandlerImpl->>SchemaTree: Resolve node IDs/columns
QueryHandlerImpl->>log_event: Get node-value pairs
QueryHandlerImpl-->>QueryHandlerImpl: Evaluate filter (may use wildcard)
else And/OrExpr
QueryHandlerImpl-->>AST: Traverse child nodes
end
end
QueryHandlerImpl-->>QueryHandler: AstEvaluationResult (bitmask)
QueryHandler-->>Client: AstEvaluationResult (bitmask)Possibly related PRs
Suggested reviewers
|
LinZhihao-723
changed the title
QueryHandler's evaluate_kv_pair_log_event.QueryHandler's evaluate_kv_pair_log_event.
![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
📒 Files selected for processing (7)
components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/search/ErrorCode.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/search/ErrorCode.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandler.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp(5 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp(7 hunks)components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/ErrorCode.cppcomponents/core/src/clp/ffi/ir_stream/search/ErrorCode.hppcomponents/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandler.hppcomponents/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
🧬 Code Graph Analysis (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (4)
create(400-421)create(400-404)create(447-479)create(447-448)
🪛 Cppcheck (2.10-2)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
[style] 423-423: The function 'evaluate_kv_pair_log_event' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (12)
components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp (3)
5-5: New include for underlying type support.Added for the
std::underlying_type_tused in the type alias definition.
11-17: Good bitmask implementation for AST evaluation results.The enum is now implemented as a bitmask with power-of-2 values (1, 2, 4), enabling bitwise operations to combine multiple evaluation states. This is appropriate for the DFS traversal strategy where results might need to be combined with bitwise operations.
20-20: Well-defined type alias for bitmask operations.The
AstEvaluationResultBitmasktype alias provides a clear way to work with the underlying integral type, which is beneficial for performing bitwise operations on the evaluation results.components/core/src/clp/ffi/ir_stream/search/ErrorCode.cpp (3)
22-24: Clear error message for AST evaluation invariant violations.The message clearly communicates that an internal logic bug has been encountered during AST evaluation, which helps with debugging.
25-26: Precise error message for leaf expression iteration attempts.This error message accurately describes a situation where code is attempting to iterate over a leaf expression, which is not allowed.
35-36: Concise error message for unexpected expression types.The message clearly indicates that an unexpected expression type was encountered during AST evaluation.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
399-399: Updated to use nullptr for empty queries.Changed from using
EmptyExpr::create()to a null shared pointer, which aligns with the updatedQueryHandlerImplbehavior that now accepts null queries as valid inputs representing empty queries.
408-408: Consistent update to pass null query to QueryHandlerImpl::create.This change maintains consistency with the updated implementation that now accepts null queries.
components/core/src/clp/ffi/ir_stream/search/ErrorCode.hpp (2)
14-15: New error codes for AST evaluation.Added error codes for handling invariant violations and invalid iteration attempts. These support proper error handling in the new AST evaluation framework.
20-20: Added error code for unexpected expression types.This error code supports the AST evaluation framework by providing a specific error for unexpected expression types encountered during evaluation.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
153-166: Doxygen params no longer match the function signature
user_gen_node_id_value_pairsis listed in the comment, but the function now takes only
log_event. Please prune the stale parameter to keep generated docs accurate.[ suggest_nitpick ]
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (1)
423-445: Empty query handling – verify intended semantics
nullptrqueries short-circuit toTrue, whereas anEmptyExprshort-circuits to
False(lines 425-432). This asymmetry means that serializing an empty string into an
AST yields a non-matching query, whereas omitting the query field entirely yields a
match-all query.If that difference is unintentional, consider normalizing both cases to the same
behaviour, e.g.:- if (m_is_empty_query) { - return AstEvaluationResult::False; - } + if (m_is_empty_query) { + return AstEvaluationResult::True; // treat empty query as match-all + }Please confirm with product requirements before merging.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 423-423: The function 'evaluate_kv_pair_log_event' is never used.
(unusedFunction)
|
@coderabbitai review depth:deep |
|
Tip For best results, initiate chat on the files or code changes. @LinZhihao-723 I've received your request for a deep review of this PR. The system will analyze your implementation of the The deep review will thoroughly examine your AST traversal implementation, evaluate error handling, and assess the logical correctness of your DFS evaluation approach. This may take a few moments to complete, and you'll receive comprehensive feedback once the analysis is finished. (・‿・) |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
🪛 Cppcheck (2.10-2)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
[style] 428-428: The function 'evaluate_kv_pair_log_event' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (1)
345-376: Clarify semantics for unsupported literal-type evaluations
evaluate_filter_against_node_id_value_pairmapsErrorCodeEnum::LiteralTypeUnsupportedtoAstEvaluationResult::False(lines 368-372). From a caller’s point of view this is indistinguishable from “filter evaluated, mismatch found”, while in reality it means “we don’t know how to evaluate this type”. In practice this can silently exclude relevant log events when the schema evolves with a new literal kind.Consider instead:
- Returning
AstEvaluationResult::Pruned, signalling “defer decision – unknown”.- Or bubbling the error up so that the caller can decide (and possibly log a warning).
Example patch for option ①:
- if (ErrorCode{ErrorCodeEnum::LiteralTypeUnsupported} == evaluation_result.error()) { - // Evaluations on unsupported literal types are considered `AstEvaluationResult::False`. - return AstEvaluationResult::False; + if (ErrorCode{ErrorCodeEnum::LiteralTypeUnsupported} == evaluation_result.error()) { + // Defer decision – literal type not evaluatable with current filter implementation. + return AstEvaluationResult::Pruned; }Please verify the intended business semantics before adopting the change.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
🔇 Additional comments (3)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)
16-16: Header change is appropriate with implementation changes.The change from including
EmptyExpr.hpptoExpression.hppproperly aligns with the shift to using a null shared pointer instead of an explicit empty expression instance.
399-400: Good simplification of the code.Replacing the explicit creation of an empty expression with a default-initialized null shared pointer simplifies the code and reduces unnecessary object creation.
408-408: Implementation change is consistent with initialization.The QueryHandlerImpl::create call is properly updated to use the null_query shared pointer.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (1)
588-591: Good implementation of AST evaluation test.The test case correctly exercises the newly implemented
evaluate_kv_pair_log_eventmethod, which was the primary focus of this PR. It verifies that the method accurately evaluates log events against the parsed query.This test addresses the concern raised in the past review comment about incomplete testing of the evaluation functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)components/core/src/clp/ffi/ir_stream/search/test/test_utils.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/search/test/utils.hpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/test_utils.cppcomponents/core/src/clp/ffi/ir_stream/search/test/utils.hppcomponents/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (5)
components/core/src/clp/ffi/ir_stream/search/test/test_utils.cpp (1)
28-28: Good modularization of test utilities.The inclusion of the "utils.hpp" header is appropriate since the
get_encoded_text_astfunction has been moved there. This refactoring improves code organization by centralizing test utilities in a header file for better reuse across test files.components/core/src/clp/ffi/ir_stream/search/test/utils.hpp (1)
93-123: Well-implemented utility function with appropriate constraints.The implementation of
get_encoded_text_astis thorough and robust:
- Proper template constraints ensure type safety
- Clear documentation explains the function's purpose
- Good error checking with assertions verifies encoding success
- Accurate extraction of dictionary variables from the encoded text
This helper function will be valuable for testing the AST evaluation functionality.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)
156-156: Improved string matching with wildcards.Adding wildcards to the string pattern (
*{}*instead of just{}) is appropriate for testing text search functionality, as it properly tests substring matching rather than exact matching.
436-436: Cleaner null expression initialization.Using a default-initialized null shared pointer is more direct than calling a factory method to create an empty expression. This change simplifies the code and makes the intent clearer.
216-241: Good test data generation helper.The
get_matchable_valuesfunction creates appropriate test values for each node type, with thorough coverage for string types by generating both plain strings and encoded ASTs using both encoding formats. This provides comprehensive test coverage for the various ways strings can be represented in the system.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (1)
490-626: Still missing negative/false-match test casesThis new test asserts only the positive path (all evaluations must be
True). It does not verify that non-matching events correctly produceFalse, which would have caught several regressions in the past.
Please add sections that feed deliberately non-matching values (wrong type, wrong text, missing keys, etc.) and assert that(result & AstEvaluationResult::False) != 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
🧠 Learnings (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
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: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`.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (1)
115-170: 🧹 Nitpick (assertive)Consolidate duplicated per-type logic in
generate_matchable_kql_expressionsThe four almost-identical
ifblocks differ only in the node-type enum, the test constant, and the format specifier. Extracting the differing parts into a small lookup table (e.g.,std::arrayof{Type, fmt_string, Value}tuples) would eliminate the repetition, make future maintenance less error-prone, and keep the function short enough to fit on one screen.- if (auto const matchable_node_ids{ - possible_matches.get_matchable_node_ids_from_schema_tree_type( - SchemaTree::Node::Type::Int) - }; false == matchable_node_ids.empty()) { - matchable_kql_expressions.emplace_back( - fmt::format("{}: {}", column_query_with_namespace, cRefTestInt)); - ... - } - // three more nearly-identical blocks + struct Entry { + SchemaTree::Node::Type type; + std::string formatted_literal; + }; + constexpr std::array<Entry, 4> kVariants{{ + {SchemaTree::Node::Type::Int, fmt::format("{}", cRefTestInt)}, + {SchemaTree::Node::Type::Float, fmt::format("{:.2f}", cRefTestFloat)}, + {SchemaTree::Node::Type::Bool, fmt::format("{}", cRefTestBool)}, + {SchemaTree::Node::Type::Str, fmt::format("*{}*", cRefTestStr)} + }}; + for (auto const& [type, literal] : kVariants) { + auto const ids = possible_matches.get_matchable_node_ids_from_schema_tree_type(type); + if (false == ids.empty()) { + matchable_kql_expressions.emplace_back( + fmt::format("{}: {}", column_query_with_namespace, literal)); + expected_column_resolutions[column_query_with_namespace].insert( + ids.cbegin(), ids.cend()); + } + }This also removes the four near-identical
try_emplacecalls by letting theinsertinto themaphappen only once.
♻️ Duplicate comments (2)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
239-257: 🧹 Nitpick (assertive)Guard encoded strings against encoder size limits (repeat from previous review)
Earlier feedback pointed out that
get_encoded_text_astsilently truncates / throws when the string exceeds the encoder’skMaxTextLen. The same safety check was added elsewhere but is still missing here:REQUIRE(long_str.size() <= ir::four_byte_encoded_variable_t::kMaxTextLen); REQUIRE(long_str.size() <= ir::eight_byte_encoded_variable_t::kMaxTextLen);Placing the assertions right before the two
get_encoded_text_astcalls makes the failure mode explicit and documents the invariant.
512-513: 🧹 Nitpick (assertive)Prefer
nullptrliteral over default-constructed shared_ptrUsing
std::shared_ptr<Expression>{nullptr}(or simplynullptrwith CTAD in C++20) is idiomatic and makes the intent—“no query”—immediately obvious.-auto null_query = std::shared_ptr<clp_s::search::ast::Expression>{}; +auto null_query = std::shared_ptr<clp_s::search::ast::Expression>{nullptr};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
🧠 Learnings (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
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: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`.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
LinZhihao-723
changed the title
QueryHandler's evaluate_kv_pair_log_event.QueryHandler's evaluate_kv_pair_log_event.
There was a problem hiding this comment.
Nice work! Just a few questions and suggestions.
Per side-bar I guess we can leave the performance work for follow-up unless you decide some parts are easy enough that they're worth doing now. If we do defer it though we should open github issues for the parts we intend to improve.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
| constexpr std::string_view cExpressionWithInverter{"NOT (NOT {}{} OR NOT {}{})"}; | ||
| constexpr std::string_view cExpressionWithoutInverter{"({}{} AND {}{})"}; |
There was a problem hiding this comment.
Hmm, I think the OrOfAndForm pass might simplify the AST for both of these to the expr AND expr case, so having both of these might just end up testing the OrOfAndForm transformation pass rather than inversion logic in the AST evaluation.
iirc that pass will always when possible simplify to a single FilterExpr, if that isn't possible it will simplify to an AndExpr of FilterExpr, and if that isn't possible it will simplify to OrExpr of FilterExpr and AndExpr.
Maybe we could remove the first NOT and expect the results to be opposites based on with_inverter?
There was a problem hiding this comment.
Right. I doubled checked that these two queries will be the same after applying all transformation passes.
I tried to change inverters to (NOT {}{} OR NOT {}{}) and chain them with AND instead of OR. This would essentially result an inverted expression of the one without any inverter. However, it takes too long to simplify the expression to Or of And form. To solve this problem, I updated this test case to test xor instead. In this way, we have a query that is already in Or of And form, and it still has inverters to test.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
268-288: Misleading local variable name repeats earlier feedbackThe vector is called
matchable_values, yet it deliberately stores unmatchable
values. Readers (and future maintainers) must mentally invert the meaning every
time they skim this code.This point was raised in a previous review of a similar helper – consider renaming
the container (e.g., tounmatchable_values) for clarity.
239-257: 🧹 Nitpick (assertive)Guard against encoder length limits to make failures explicit
get_matchable_valuesfeedslong_strdirectly into both four- and eight-byte encoders.
If the maximum text length for either encoder is shortened in the future (or if the
test string is inadvertently lengthened), the encoder will silently truncate or throw
deep in the bowels of the encoder, producing non-obvious test failures.Add an explicit
REQUIREon the length before calling the helper so that the
failure mode is local and self-documenting.@@ matchable_values.emplace_back(fmt::format("ThisIs{}", cRefTestStr)); auto const long_str{fmt::format("This is {}", cRefTestStr)}; + REQUIRE(long_str.size() + <= ir::four_byte_encoded_variable_t::kMaxTextLen); + REQUIRE(long_str.size() + <= ir::eight_byte_encoded_variable_t::kMaxTextLen); matchable_values.emplace_back( get_encoded_text_ast<ir::four_byte_encoded_variable_t>(long_str) ); matchable_values.emplace_back( get_encoded_text_ast<ir::eight_byte_encoded_variable_t>(long_str) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
🧠 Learnings (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
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: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`.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Show resolved
Hide resolved
Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
378-391: 🧹 Nitpick (assertive)Stateful DFS stack makes the instance non-re-entrant
m_ast_dfs_stackis persisted across calls toevaluate_kv_pair_log_event.
If the sameQueryHandlerImplobject is invoked concurrently from multiple threads, evaluations will corrupt each other. Even single-threaded re-entrancy (calling evaluate from a callback) would break.If thread safety is a desired property, encapsulate the stack in a small POD that is allocated on the caller’s stack (e.g., pass it as a scratch parameter) or protect the member with a mutex.
♻️ Duplicate comments (2)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (1)
512-513: Prefer explicitnullptrfor clarity
auto null_query = std::shared_ptr<clp_s::search::ast::Expression>{};
creates a null pointer, but the intent is clearer and idiomatic when the
nullptrliteral is used:-auto null_query = std::shared_ptr<clp_s::search::ast::Expression>{}; +auto null_query = std::shared_ptr<clp_s::search::ast::Expression>{nullptr};(This suggestion was raised previously and still applies.)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
212-214:⚠️ Potential issue
createstill accepts the wrong base pointer typePrevious feedback noted that
AstExprIterator::createshould takeclp_s::search::ast::Expression*because onlyFilterExpr,AndExpr, andOrExprare supported. The signature is stillValue*, so callers will need an unsafe cast or the code won’t compile ifValueandExpressionare unrelated.- [[nodiscard]] static auto create(clp_s::search::ast::Value* expr) + [[nodiscard]] static auto create(clp_s::search::ast::Expression* expr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp(6 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp(7 hunks)components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)components/core/src/clp/ffi/ir_stream/search/test/utils.hpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/utils.hppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cppcomponents/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
🧠 Learnings (3)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)
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: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#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
🪛 Cppcheck (2.10-2)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
[style] 429-429: The function 'evaluate_kv_pair_log_event' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (1)
440-447: Thread-local stack is cleared unsafely in concurrent contexts
m_ast_dfs_stack.clear()mutates shared state without protection.
Although earlier discussions stated the class isn't thread-safe, a brief comment
(or// Not thread-safe) here would save future readers from misusing the
handler in parallel pipelines.[ suggest_nitpick ]
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (2)
153-160: Doxygen still references removed parameterThe comment block still mentions
user_gen_node_id_value_pairs, which no longer exists in the signature. This was flagged previously and is still unresolved.
Please update the docstring to avoid stale information.
212-214:⚠️ Potential issueWrong pointer type breaks compilation
AstExprIterator::createtakes aclp_s::search::ast::Value*, yet the iterator only supportsFilterExpr,AndExpr, andOrExpr— all derive fromExpression, notValue.
Passing aValue*will never compile and the overload cannot be found by callers.- [[nodiscard]] static auto create(clp_s::search::ast::Value* expr) + [[nodiscard]] static auto create(clp_s::search::ast::Expression* expr)components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (1)
305-316: Avoid unnecessary reference binding
kv_pair_log_eventis taken by reference only to be used immediately. Holding the reference is superfluous and slightly obscures lifetimes.- auto const& kv_pair_log_event{kv_pair_log_event_result.value()}; - auto const evaluation_result{query_handler_impl.evaluate_kv_pair_log_event(kv_pair_log_event)}; + auto const evaluation_result{ + query_handler_impl.evaluate_kv_pair_log_event( + kv_pair_log_event_result.value() + ) + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp(7 hunks)components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
🧠 Learnings (2)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)
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: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#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
🧬 Code Graph Analysis (1)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (4)
components/core/src/clp/ffi/ir_stream/search/test/utils.hpp (9)
nodiscard(35-37)nodiscard(39-41)get_encoded_text_ast(107-108)get_encoded_text_ast(113-128)get_encoded_text_ast(113-113)get_schema_tree_column_queries(90-91)node_id(51-54)node_id(51-51)trivial_new_projected_schema_tree_node_callback(77-81)components/core/src/clp/ffi/ir_stream/search/QueryHandler.hpp (2)
nodiscard(40-57)is_auto_generated(79-83)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (7)
create(406-427)create(406-410)create(453-485)create(453-454)is_auto_generated(115-115)is_auto_generated(336-344)is_auto_generated(336-336)components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (2)
create(528-564)create(528-534)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, false)
| auto const schema_tree{std::make_shared<SchemaTree>()}; | ||
| std::vector<SchemaTree::NodeLocator> const locators{ | ||
| {SchemaTree::cRootId, "a", SchemaTree::Node::Type::Obj}, | ||
| {SchemaTree::cRootId, "b", SchemaTree::Node::Type::Int}, | ||
| {1, "b", SchemaTree::Node::Type::Obj}, | ||
| {3, "c", SchemaTree::Node::Type::Obj}, | ||
| {3, "d", SchemaTree::Node::Type::Str}, | ||
| {3, "d", SchemaTree::Node::Type::Bool}, | ||
| {4, "a", SchemaTree::Node::Type::Str}, | ||
| {4, "d", SchemaTree::Node::Type::Str}, | ||
| {4, "d", SchemaTree::Node::Type::Float}, | ||
| {3, "e", SchemaTree::Node::Type::Obj}, | ||
| {4, "f", SchemaTree::Node::Type::Obj}, | ||
| {SchemaTree::cRootId, "a", SchemaTree::Node::Type::Int}, | ||
| {1, "b", SchemaTree::Node::Type::Bool} | ||
| }; | ||
| for (auto const& locator : locators) { | ||
| REQUIRE_NOTHROW(schema_tree->insert_node(locator)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider factoring out schema-tree construction
The exact same schema tree is rebuilt in each test section, adding ~60 lines of repetitive code.
Extracting a helper like auto make_test_schema_tree() -> std::shared_ptr<SchemaTree> would:
- Reduce duplication and cognitive load.
- Make future modifications to the test schema easier and less error-prone.
| } | ||
|
|
||
| return std::move(query_handler_impl_result.value()); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
std::move not required when returning local
Returning std::move(query_handler_impl_result.value()) inhibits copy-elision and is unnecessary. Simply return the value; modern compilers will move/copy-elide optimally.
- return std::move(query_handler_impl_result.value());
+ return query_handler_impl_result.value();📝 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 std::move(query_handler_impl_result.value()); | |
| }; | |
| } | |
| return query_handler_impl_result.value(); | |
| }; |
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
75-80: 🧹 Nitpick (assertive)Minor: guard against undefined behaviour when advancing past the end token
next()assumes!is_last(); you guard for this, great.
Adding anassert(!is_last())aids debugging builds and documents the invariant without any runtime cost in production.[[nodiscard]] auto next() const -> outcome_v2::std_result<ColumnDescriptorTokenIterator> { + assert(false == is_last() && "Attempted to advance past the last token"); if (is_last()) {
♻️ Duplicate comments (4)
components/core/src/clp/ffi/ir_stream/search/ErrorCode.cpp (1)
35-36: ExpandExpressionTypeUnexpectedmessage for quicker root-cause analysisThe current text
"Unexpected expression type."omits where the unexpected type was encountered. Including the evaluation phase (e.g., “…encountered during AST evaluation”) gives users immediate context and keeps the wording consistent with the neighbouring error messages.components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
642-645:std::moveon a local return value is redundantMoving the
value()out of theresultcontainer manually prevents copy-elision and needlessly obscures intent. Returning the value directly lets the compiler apply NRVO/move-elision.- return std::move(query_handler_impl_result.value()); + return query_handler_impl_result.value();
315-317: Inline the value instead of binding a short-lived referenceThe reference
kv_pair_log_eventis used exactly once. Passing the value directly reduces cognitive load and avoids accidental lifetime extensions.- auto const& kv_pair_log_event{kv_pair_log_event_result.value()}; - auto const evaluation_result{query_handler_impl.evaluate_kv_pair_log_event(kv_pair_log_event)}; + auto const evaluation_result{query_handler_impl.evaluate_kv_pair_log_event( + kv_pair_log_event_result.value() + )};components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
211-213:⚠️ Potential issue
AstExprIterator::createstill accepts the wrong pointer type – compilation will failAs previously observed,
AstExprIteratoronly handlesFilterExpr,AndExpr, andOrExpr, all of which derive fromExpression.
Passing aValue*does not compile and keeps the super-fluous dependency onValue.hpp.- [[nodiscard]] static auto create(clp_s::search::ast::Value* expr) + [[nodiscard]] static auto create(clp_s::search::ast::Expression* expr)After changing the signature, you can drop the include of
ast/Value.hpp.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp/ffi/ir_stream/search/ErrorCode.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandler.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp(7 hunks)components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/ErrorCode.cppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandler.hppcomponents/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp
🧠 Learnings (3)
components/core/src/clp/ffi/ir_stream/search/QueryHandler.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)
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: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#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
🔇 Additional comments (1)
components/core/src/clp/ffi/ir_stream/search/QueryHandler.hpp (1)
92-102: The implementation looks good!The new
evaluate_kv_pair_log_eventmethod correctly replaces the previousevaluate_node_id_value_pairsmethod, improving the interface by accepting a unifiedKeyValuePairLogEventobject rather than separate node ID-value pairs. The method properly delegates to the implementation class and maintains consistency with the class's design pattern.The documentation has been appropriately updated to reflect the change, and the method signature is correctly defined without the trailing comma issue mentioned in previous reviews.
| [[nodiscard]] auto evaluate_kv_pair_log_event(KeyValuePairLogEvent const& log_event) | ||
| -> outcome_v2::std_result<AstEvaluationResult>; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure the AST DFS stack is reset at the start of every evaluation
evaluate_kv_pair_log_event re-uses the member m_ast_dfs_stack for performance reasons, which is fine.
However, if the function exits early (e.g., returns an error) the stack may hold stale state that will poison the next call, producing undefined results or crashes.
Please guarantee the stack is cleared before any new traversal begins:
[[nodiscard]] auto QueryHandlerImpl::evaluate_kv_pair_log_event(
KeyValuePairLogEvent const& log_event)
-> outcome_v2::std_result<AstEvaluationResult> {
+ m_ast_dfs_stack.clear(); // ← defensive reset
...
}If this is already done in the .cpp, consider adding a brief comment here to make the contract explicit.
To verify whether the DFS stack is already being reset, let’s locate the start of evaluate_kv_pair_log_event in the .cpp:
🏁 Script executed:
#!/bin/bash
rg -n "evaluate_kv_pair_log_event" -A10 -B2 components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cppLength of output: 534
Clear AST DFS stack at the start of every evaluation
The implementation of
QueryHandlerImpl::evaluate_kv_pair_log_event
re-uses the member m_ast_dfs_stack for performance, but early returns (e.g. when m_query == nullptr or m_is_empty_query) occur before any reset. This can leave stale entries on the stack and lead to undefined behaviour on the next call.
Please add a defensive clear at the top of the function:
• File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
• Around line 430, immediately after the function signature
auto QueryHandlerImpl::evaluate_kv_pair_log_event(
KeyValuePairLogEvent const& log_event)
-> outcome_v2::std_result<AstEvaluationResult> {
+ m_ast_dfs_stack.clear(); // ← ensure no stale state before any return
if (nullptr == m_query) {
return AstEvaluationResult::True;
}
if (m_is_empty_query) {
return AstEvaluationResult::False;
}
std::optional<AstEvaluationResult> optional_evaluation_result;
…This guarantees a fresh stack even if the method exits early.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[nodiscard]] auto evaluate_kv_pair_log_event(KeyValuePairLogEvent const& log_event) | |
| -> outcome_v2::std_result<AstEvaluationResult>; | |
| auto QueryHandlerImpl::evaluate_kv_pair_log_event( | |
| KeyValuePairLogEvent const& log_event) | |
| -> outcome_v2::std_result<AstEvaluationResult> { | |
| + m_ast_dfs_stack.clear(); // ← ensure no stale state before any return | |
| if (nullptr == m_query) { | |
| return AstEvaluationResult::True; | |
| } | |
| if (m_is_empty_query) { | |
| return AstEvaluationResult::False; | |
| } | |
| std::optional<AstEvaluationResult> optional_evaluation_result; | |
| … | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (2)
644-645: Unnecessarystd::moveinhibits copy-elision – repeat observationReturning a local by
std::move(return std::move(query_handler_impl_result.value());)
forces a move-construction and prevents mandatory RVO. This exact
nitpick was raised in an earlier round and is still unresolved.- return std::move(query_handler_impl_result.value()); + return query_handler_impl_result.value();
606-616:AndExprshort-circuit order still letsPrunedmaskFalseThe evaluator checks
PrunedbeforeFalse. Consequently
False | Prunedis reported as Pruned, contradicting boolean logic
(Falseshould dominate).This issue has been pointed out previously but the logic remains the
same.- if (0 != (evaluation_results & AstEvaluationResult::Pruned)) { + if (0 != (evaluation_results & AstEvaluationResult::False)) { … - } - if (0 != (evaluation_results & AstEvaluationResult::False)) { + } + if (0 != (evaluation_results & AstEvaluationResult::Pruned)) { … }components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (1)
608-622:AndExprevaluation –Falsemust dominate overPrunedSame logical flaw as flagged in the tests: when both
Falseand
Prunedbits are present, the result must beFalse. The current
ordering lets a precedingPrunedbranch override a definitiveFalse.- if (0 != (evaluation_results & AstEvaluationResult::Pruned)) { - … Pruned … - } - if (0 != (evaluation_results & AstEvaluationResult::False)) { - … False … - } + if (0 != (evaluation_results & AstEvaluationResult::False)) { + … False … + } + if (0 != (evaluation_results & AstEvaluationResult::Pruned)) { + … Pruned … + }Please correct the precedence and consider adding a unit test where one
child isFalseand another isPrunedto prevent regressions.components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (2)
163-165: Please ensure DFS stack is cleared at the start of each evaluationBecause
m_ast_dfs_stackis reused across calls for performance, it must be defensively cleared at the very top ofevaluate_kv_pair_log_event.
An early return (e.g.,m_query == nullptr) before the clear will leave stale state that contaminates the next call.#!/bin/bash # Verify that the first non-comment line inside the function body clears the stack rg -n "evaluate_kv_pair_log_event" -A5 components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp | head -n 20
211-213:⚠️ Potential issueIncorrect parameter type breaks compilation – should accept
Expression*, notValue*
AstExprIterator::createis still declared withclp_s::search::ast::Value*.
All valid call-sites passFilterExpr,AndExpr, orOrExpr, which derive fromExpression, notValue.
This keeps the codebase in an uncompilable state exactly as flagged in an earlier review.- [[nodiscard]] static auto create(clp_s::search::ast::Value* expr) + [[nodiscard]] static auto create(clp_s::search::ast::Expression* expr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp(6 hunks)components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp(7 hunks)components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hppcomponents/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hppcomponents/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
🧠 Learnings (3)
components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)
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: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#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp:164-166
Timestamp: 2025-05-11T22:22:49.286Z
Learning: For the `QueryHandlerImpl` class, reusing member variables like `m_ast_dfs_stack` is preferred over making methods `const` and moving state to function scope to avoid allocation/deallocation overhead, especially for frequently called methods like `evaluate_kv_pair_log_event`.
Learnt from: LinZhihao-723
PR: y-scope/clp#882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
🪛 Cppcheck (2.10-2)
components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp
[style] 429-429: The function 'evaluate_kv_pair_log_event' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
| enum AstEvaluationResult : uint8_t { | ||
| True = 1, | ||
| False = 1 << 1, | ||
|
|
||
| // The AST evaluation is intentionally skipped because it belongs to a pruned branch of the | ||
| // parent tree. | ||
| Pruned, | ||
| Pruned = 1 << 2, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Provide type-safe bitwise helpers for the enum bitmask
AstEvaluationResult is now an unscoped enum so that implicit
integral conversions permit | / & operations.
Consider adding constexpr operator overloads instead and keeping a
scoped enum (enum class) – this preserves type-safety while still
allowing the same ergonomics:
enum class AstEvaluationResult : uint8_t {
True = 1,
False = 1 << 1,
Pruned = 1 << 2,
};
constexpr AstEvaluationResult operator|(
AstEvaluationResult lhs, AstEvaluationResult rhs) {
return static_cast<AstEvaluationResult>(
static_cast<uint8_t>(lhs) | static_cast<uint8_t>(rhs));
}
constexpr bool operator&(AstEvaluationResult lhs, AstEvaluationResult rhs) {
return (static_cast<uint8_t>(lhs) & static_cast<uint8_t>(rhs)) != 0;
}This prevents accidental mixing with unrelated bit-fields and removes
the need for ast_evaluation_result_bitmask_t casts in most places.
Also applies to: 20-20
| ast_evaluation_result_bitmask_t evaluation_results{}; | ||
| for (auto const matchable_node_id : matchable_node_ids) { | ||
| if (false == node_id_value_pairs.contains(matchable_node_id)) { | ||
| continue; | ||
| } | ||
| auto const evaluation_result{OUTCOME_TRYX(evaluate_filter_against_node_id_value_pair( | ||
| filter_expr, | ||
| matchable_node_id, | ||
| node_id_value_pairs.at(matchable_node_id), | ||
| schema_tree, | ||
| m_case_sensitive_match | ||
| ))}; | ||
| if (AstEvaluationResult::True == evaluation_result) { | ||
| return AstEvaluationResult::True; | ||
| } | ||
| evaluation_results |= evaluation_result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Potential performance hot-spot – repeated contains/at look-ups
Inside the tight loop over matchable_node_ids, each iteration performs
node_id_value_pairs.contains(id) followed by at(id), resulting in two
hash look-ups. A single find avoids the duplication:
auto it = node_id_value_pairs.find(matchable_node_id);
if (it == node_id_value_pairs.end()) {
continue;
}
auto const& value = it->second;
…This micro-optimisation can matter when evaluating large events against
complex queries.
| auto QueryHandlerImpl::AstExprIterator::create(clp_s::search::ast::Value* expr) | ||
| -> outcome_v2::std_result<AstExprIterator> { | ||
| if (auto* and_expr{dynamic_cast<clp_s::search::ast::AndExpr*>(expr)}; nullptr != and_expr) { | ||
| return AstExprIterator{ | ||
| ExprVariant{and_expr}, | ||
| and_expr->op_begin(), | ||
| and_expr->op_end(), | ||
| and_expr->is_inverted() | ||
| }; | ||
| } | ||
|
|
||
| if (auto* or_expr{dynamic_cast<clp_s::search::ast::OrExpr*>(expr)}; nullptr != or_expr) { | ||
| return AstExprIterator{ | ||
| ExprVariant{or_expr}, | ||
| or_expr->op_begin(), | ||
| or_expr->op_end(), | ||
| or_expr->is_inverted() | ||
| }; | ||
| } | ||
|
|
||
| if (auto* filter_expr{dynamic_cast<clp_s::search::ast::FilterExpr*>(expr)}; | ||
| nullptr != filter_expr) | ||
| { | ||
| return AstExprIterator{ | ||
| ExprVariant{filter_expr}, | ||
| filter_expr->op_begin(), | ||
| filter_expr->op_end(), | ||
| filter_expr->is_inverted() | ||
| }; | ||
| } | ||
|
|
||
| return ErrorCode{ErrorCodeEnum::ExpressionTypeUnexpected}; | ||
| } |
There was a problem hiding this comment.
Parameter type mismatch – cannot legally pass Expression* to Value*
AstExprIterator::create is declared as
create(clp_s::search::ast::Value* expr) yet every call site
(create(m_query.get()), create((it++)->get())) passes a pointer to
Expression, not Value.
A base-to-derived conversion is illegal – this would not compile unless
Value inherits from Expression and Expression* converts to
Value*, which C++ forbids.
Change the signature to accept Expression* (or the common base type):
-auto QueryHandlerImpl::AstExprIterator::create(clp_s::search::ast::Value* expr)
+auto QueryHandlerImpl::AstExprIterator::create(clp_s::search::ast::Expression* expr)and update the header accordingly.
Failing to do so will either not compile or will require an unsafe
static_cast.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto QueryHandlerImpl::AstExprIterator::create(clp_s::search::ast::Value* expr) | |
| -> outcome_v2::std_result<AstExprIterator> { | |
| if (auto* and_expr{dynamic_cast<clp_s::search::ast::AndExpr*>(expr)}; nullptr != and_expr) { | |
| return AstExprIterator{ | |
| ExprVariant{and_expr}, | |
| and_expr->op_begin(), | |
| and_expr->op_end(), | |
| and_expr->is_inverted() | |
| }; | |
| } | |
| if (auto* or_expr{dynamic_cast<clp_s::search::ast::OrExpr*>(expr)}; nullptr != or_expr) { | |
| return AstExprIterator{ | |
| ExprVariant{or_expr}, | |
| or_expr->op_begin(), | |
| or_expr->op_end(), | |
| or_expr->is_inverted() | |
| }; | |
| } | |
| if (auto* filter_expr{dynamic_cast<clp_s::search::ast::FilterExpr*>(expr)}; | |
| nullptr != filter_expr) | |
| { | |
| return AstExprIterator{ | |
| ExprVariant{filter_expr}, | |
| filter_expr->op_begin(), | |
| filter_expr->op_end(), | |
| filter_expr->is_inverted() | |
| }; | |
| } | |
| return ErrorCode{ErrorCodeEnum::ExpressionTypeUnexpected}; | |
| } | |
| auto QueryHandlerImpl::AstExprIterator::create(clp_s::search::ast::Expression* expr) | |
| -> outcome_v2::std_result<AstExprIterator> { | |
| if (auto* and_expr{dynamic_cast<clp_s::search::ast::AndExpr*>(expr)}; nullptr != and_expr) { | |
| return AstExprIterator{ | |
| ExprVariant{and_expr}, | |
| and_expr->op_begin(), | |
| and_expr->op_end(), | |
| and_expr->is_inverted() | |
| }; | |
| } | |
| if (auto* or_expr{dynamic_cast<clp_s::search::ast::OrExpr*>(expr)}; nullptr != or_expr) { | |
| return AstExprIterator{ | |
| ExprVariant{or_expr}, | |
| or_expr->op_begin(), | |
| or_expr->op_end(), | |
| or_expr->is_inverted() | |
| }; | |
| } | |
| if (auto* filter_expr{dynamic_cast<clp_s::search::ast::FilterExpr*>(expr)}; | |
| nullptr != filter_expr) | |
| { | |
| return AstExprIterator{ | |
| ExprVariant{filter_expr}, | |
| filter_expr->op_begin(), | |
| filter_expr->op_end(), | |
| filter_expr->is_inverted() | |
| }; | |
| } | |
| return ErrorCode{ErrorCodeEnum::ExpressionTypeUnexpected}; | |
| } |
| /** | ||
| * Retrieves the next child expression operator to visit as an `AstExprIterator`. | ||
| * @return A result containing the next `AstExprIterator` to visit on success, or an error | ||
| * code indicating the failure: | ||
| * - ErrorCodeEnum::AttemptToIterateAstLeafExpr if the current expression is a leaf | ||
| * expression (`clp_s::search::ast::FilterExpr`) with no child operators. | ||
| * - Forwards `create`'s return values. | ||
| * @return std::nullopt if there are no more child operators to visit. | ||
| */ | ||
| [[nodiscard]] auto next_op() -> std::optional<outcome_v2::std_result<AstExprIterator>>; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicated “@return” section in docstring
The comment for next_op() lists two separate “@return” blocks. This is redundant and may confuse Doxygen / readers.
Consider condensing to a single block that first explains the std::optional wrapper and then enumerates the possible error codes.
3 participants
Description
References
Overview
This PR implements
QueryHandler::evaluate_kv_pair_log_eventmethod to evaluate a kv-pair log event against a search query using iterative DFS on the search AST.With this PR,
QueryHandlerwill be fully implemented and we can proceed to integrate the search feature intoDeserializer.Checklist
breaking change.
Validation performed
Summary by CodeRabbit