components/core/src/clp/ffi/ir_stream/search/ErrorCode.hpp (1)

13-22: Consider future-proofing the enum size

ErrorCodeEnum is constrained to uint8_t. We now have 9 values; still safe, but adding more than 256 codes in the long run will silently wrap. If you expect the set to keep expanding, consider switching to uint16_t early to avoid a breaking change later.

components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)

31-33: Iterator class lacks operator== / operator!=

Providing comparison operators would make the iterator composable with STL algorithms (and future-proofs the design).

components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (5)

113-136: Good use of error handling, but the exception catch is broad

Your implementation properly checks for null query and follows the code style of using nullptr == query instead of !query as per guidelines. However, the catch block at line 133 catches all exceptions and returns a generic error code. Consider catching specific exceptions or at least logging the exception message to aid in debugging.

    } catch (std::exception const& ex) {
-        return ErrorCode{ErrorCodeEnum::QueryTransformationPassFailed};
+        // Log or capture exception details for debugging
+        return ErrorCode{ErrorCodeEnum::QueryTransformationPassFailed, ex.what()};

---

148-164: The duplicate check could be combined with try-emplace

The current implementation uses a separate set to track duplicates, but this could be streamlined using the return value of try_emplace.

-    std::unordered_set<std::string_view> unique_projected_columns;

     for (auto const& [key, types] : projections) {
-        if (unique_projected_columns.contains(key)) {
-            return ErrorCode{ErrorCodeEnum::DuplicateProjectedColumn};
-        }
-        unique_projected_columns.emplace(key);

This is only a suggestion since the current approach with a separate set might be clearer and more maintainable in some contexts.

---

166-182: Consider separating exception handling for different operations

The current implementation has a single try-catch block that encompasses multiple operations: creating a column descriptor, setting matching types, and checking validity. This makes it difficult to provide specific error messages for different types of failures.

Consider separating the try-catch blocks to handle different operations separately, allowing for more specific error codes or messages.

---

202-207: Map insertion could be optimized

The code checks if a key exists before inserting, which is an extra lookup operation. Consider using try_emplace directly, which will only insert if the key doesn't exist.

-        if (false == partial_resolutions.contains(SchemaTree::cRootId)) {
-            partial_resolutions.emplace(
-                    SchemaTree::cRootId,
-                    std::vector<QueryHandlerImpl::ColumnDescriptorTokenIterator>{}
-            );
-        }
+        auto [it, inserted] = partial_resolutions.try_emplace(
+                SchemaTree::cRootId,
+                std::vector<QueryHandlerImpl::ColumnDescriptorTokenIterator>{}
+        );

This approach is actually used later in the code at line 262, so this would make the code more consistent.

---

256-260: Code reuse opportunity for namespace resolution logic

This code duplicates the logic from lines 197-201. Consider extracting this pattern into a small helper function since it's used multiple times.

+    auto& get_resolution_map_for_namespace(
+            std::string_view ns,
+            QueryHandlerImpl::PartialResolutionMap& auto_gen,
+            QueryHandlerImpl::PartialResolutionMap& user_gen) {
+        return (OUTCOME_TRYX(is_auto_generated(ns))) ? auto_gen : user_gen;
+    }

// Then use it like:
-        bool const is_auto_gen{OUTCOME_TRYX(is_auto_generated(col->get_namespace()))};
-        auto& partial_resolutions{
-                is_auto_gen ? auto_gen_namespace_partial_resolutions
-                            : user_gen_namespace_partial_resolutions
-        };
+        auto& partial_resolutions = get_resolution_map_for_namespace(
+                col->get_namespace(),
+                auto_gen_namespace_partial_resolutions,
+                user_gen_namespace_partial_resolutions);

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

components/core/src/clp/ffi/ir_stream/search/ErrorCode.cpp (2)

18-47: Consider compile-time enforcement that all enumerators are mapped.

The switch covers every enumerator that currently exists, but it will silently fall back to "Unknown error code enum." if a future enumerator is added and the author forgets to update this function.
A small compile-time safeguard can avoid that foot-gun, e.g.

static_assert(
    static_cast<int>(ErrorCodeEnum::UnsupportedNamespace) + 1 ==
        ystdlib::enum_size_v<ErrorCodeEnum>,   // or another meta-func you may have
    "ErrorCode::message() is missing cases for some ErrorCodeEnum values"
);

(or generate the table with magic_enum/nameof if available).
This keeps the mapping exhaustive even as the project evolves.

---

20-44: Return std::string_view to avoid per-call heap allocations.

message() currently allocates a fresh std::string on every call.
Because the returned texts are static and immutable, you can safely return
std::string_view (or char const*) instead:

-auto ErrorCategory::message(ErrorCodeEnum error_enum) const -> std::string {
+auto ErrorCategory::message(ErrorCodeEnum error_enum) const -> std::string_view {

and change each branch to return "literal";.
This removes dynamic allocation and slightly shrinks the call-site code size without sacrificing usability.

components/core/src/clp/ffi/ir_stream/search/utils.hpp (2)

48-53: Pass filter by reference (or gsl::not_null) instead of raw pointer.

evaluate_filter_against_literal_type_value_pair takes a raw
FilterExpr const*. Within the project this pointer is expected to be non-null,
because a null pointer already maps to ErrorCodeEnum::QueryExpressionIsNull
elsewhere.

Prefer an explicit contract by:

-auto evaluate_filter_against_literal_type_value_pair(
-        clp_s::search::ast::FilterExpr const* filter,
+auto evaluate_filter_against_literal_type_value_pair(
+        clp_s::search::ast::FilterExpr const& filter,

(or not_null<FilterExpr const*>) to make misuse impossible at compile time
and avoid repeated null checks inside the implementation.

---

4-12: Include guards use relative includes – consider project-wide include paths.

The header reaches into sibling modules via lengthy relative paths:
../../../../clp_s/search/ast/…. This can become fragile when files move.
If possible, expose these headers via the build system’s include-path and switch to
angle-bracket form:

#include <clp_s/search/ast/FilterExpr.hpp>
#include <clp_s/search/ast/Literal.hpp>

Doing so improves readability and portability across IDEs.

components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.hpp (1)

251-251: Unused variable next_token_it_result

auto const next_token_it_result{token_it.next()}; is evaluated but never used, incurring an unnecessary next() call (and potential error handling). Remove the statement or use the result.

components/core/tests/test-ffi_ir_stream_search_utils.cpp (4)

16-29: Prefer project-wide include paths over brittle relative paths

The test currently relies on multi-level ../src/... relative includes.
If the test directory ever moves (or is re-organised by CMake’s add_test), these paths will silently break. Consider exposing src as a public include directory in your build system and switching to angle-bracket includes such as:

#include <clp/ffi/encoding_methods.hpp>
#include <clp/ffi/ir_stream/search/utils.hpp>

This makes the tests location-agnostic and improves maintainability.

---

145-162: Minor efficiency win in get_encoded_text_ast

1. dict_vars.reserve(dict_var_bounds.size() / 2);
   Reserving avoids repeated reallocations while building dict_vars.

2. The inner loop currently constructs substrings via iterator math.
   std::string{text.substr(begin_pos, end_pos - begin_pos)} is clearer and guards against accidental iterator misuse.

Diff sketch:

-    std::vector<std::string> dict_vars;
+    std::vector<std::string> dict_vars;
+    dict_vars.reserve(dict_var_bounds.size() / 2);
...
-        dict_vars.emplace_back(text.cbegin() + begin_pos, text.cbegin() + end_pos);
+        dict_vars.emplace_back(text.substr(begin_pos, end_pos - begin_pos));

---

499-505: Typo: filer_operand_literal_type → filter_operand_literal_type

The variable is misspelled here and in the subsequent call. While harmless, keeping names consistent aids readability and grep-ability.

-auto const [filter_expr, filer_operand_literal_type]{
+auto const [filter_expr, filter_operand_literal_type]{
 ...
-REQUIRE(check_filter_evaluation(filter_to_test.get(), filer_operand_literal_type));
+REQUIRE(check_filter_evaluation(filter_to_test.get(), filter_operand_literal_type));

---

304-335: Constraint duplication can be simplified

The requires clause std::is_same_v<...> is repeated on both the forward declaration and the definition of check_filter_evaluation_against_encoded_text_asts. Keeping it only on the declaration (or on a using concept) reduces verbosity:

template <EncodedVariableT encoded_variable_t>
auto check_filter_evaluation_against_encoded_text_asts(...);

where template<typename T> concept EncodedVariableT = std::same_as<...> || ...;

This keeps the intent in one place and avoids accidental divergence if the constraint list changes.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

components/core/src/clp/ffi/ir_stream/search/test/test_utilis.cpp (3)

272-282: Factor out duplicated “unsupported literal-type” branch

The early-exit path that evaluates the filter against an empty optional and expects false is replicated in every helper (*_integral_values, *_bool_values, …).
A small utility such as:

auto filter_matches_empty_value(FilterExpr const* f,
                                LiteralType filter_literal_type,
                                LiteralType value_literal_type) -> bool;

would eliminate ~25 duplicated lines, improving readability and reducing maintenance overhead.

---

494-499: Typo: filer_operand_literal_type → filter_operand_literal_type

A simple spelling error leaks into the public test output (via CAPTURE).
Renaming improves clarity and avoids confusion for future readers.

-auto const [filter_expr, filer_operand_literal_type]{
+auto const [filter_expr, filter_operand_literal_type]{
         GENERATE_COPY(from_range(filter_and_type_pairs))
 };
 …
-REQUIRE(check_filter_evaluation(filter_to_test.get(), filer_operand_literal_type));
+REQUIRE(check_filter_evaluation(filter_to_test.get(), filter_operand_literal_type));

---

1-13: File name typo (test_utilis.cpp)

Consider renaming the file to test_utils.cpp for consistency with conventional English spelling and to aid discoverability by IDE search tools.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

components/core/src/clp/ffi/ir_stream/search/test/utils.hpp (1)

64-73: Incomplete Doxygen parameter documentation

The function documentation has an empty @param entry for node_id and is missing documentation for projected_key_path.

/**
 * Trivial implementation of `NewProjectedSchemaTreeNodeCallback` that always return success without
 * doing anything.
 * @param is_auto_generated Whether the node belongs to the auto-generated namespace.
-* @param
+* @param node_id The schema tree node ID receiving the projection.
+* @param projected_key_path The full key-path being projected (unescaped).
 */

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.

components/core/src/clp/ffi/ir_stream/search/test/utils.hpp (1)

67-67: Missing parameter documentation

This line contains an empty @param tag.

-* @param
+* @param node_id The ID of the schema tree node receiving the projection
+* @param projected_key_path The full key path being projected (unescaped)

components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp (1)

120-137: Consider flattening chained assignment in if statements

The current implementation combines transformation and null check in single if statements, which couples concerns and makes debugging more challenging.

-if (query = clp_s::search::ast::OrOfAndForm{}.run(query);
-    nullptr != std::dynamic_pointer_cast<EmptyExpr>(query))
-{
-    return query;
-}
+auto transformed{clp_s::search::ast::OrOfAndForm{}.run(query)};
+if (nullptr != std::dynamic_pointer_cast<EmptyExpr>(transformed)) {
+    return transformed;
+}
+query = std::move(transformed);

This pattern also applies to the NarrowTypes transformation below.

components/core/src/clp/ffi/ir_stream/search/test/test_QueryHandlerImpl.cpp (3)

294-296: Add a null check after KQL parsing

The KQL parser can return nullptr if parsing fails. Adding a check ensures the test fails early with a clear error message rather than crashing on a null dereference later.

auto query_stream{std::istringstream{kql_query_str}};
auto query{clp_s::search::kql::parse_kql_expression(query_stream)};
+REQUIRE(query != nullptr);  // Verify that KQL parsing succeeded

---

76-163: Consider refactoring repetitive code blocks in generate_matchable_kql_expressions

The function contains nearly identical code blocks for handling each data type (Int, Float, Bool, Str). Consider using a helper function to reduce duplication:

auto process_type = [&](SchemaTree::Node::Type type, auto ref_value, std::string_view format = "{}") {
    for (auto const& [column_query, possible_matches] : column_query_to_possible_matches) {
        auto const column_query_with_namespace{fmt::format("{}{}", column_namespace, column_query)};
        if (auto const matchable_node_ids{
                    possible_matches.get_matchable_node_ids_from_schema_tree_type(type)
                };
                false == matchable_node_ids.empty())
        {
            matchable_kql_expressions.emplace_back(
                    fmt::format("{}: " + std::string{format}, column_query_with_namespace, ref_value)
            );
            auto [it, inserted] = expected_column_resolutions.try_emplace(
                    column_query_with_namespace,
                    std::set<SchemaTree::Node::id_t>{}
            );
            auto& resolved_node_ids{it->second};
            resolved_node_ids.insert(matchable_node_ids.cbegin(), matchable_node_ids.cend());
        }
    }
};

// Process each type
process_type(SchemaTree::Node::Type::Int, cRefTestInt);
process_type(SchemaTree::Node::Type::Float, cRefTestFloat, "{:.2f}");
process_type(SchemaTree::Node::Type::Bool, cRefTestBool);
process_type(SchemaTree::Node::Type::Str, cRefTestStr);

This reduces duplication and improves maintainability.

---

228-246: Consider extracting duplicated schema tree setup into a helper function

Both test cases duplicate the same schema tree setup code. Consider extracting this into a helper function:

namespace {
// Returns a schema tree with a predefined structure and the corresponding node locators
[[nodiscard]] auto create_test_schema_tree() 
    -> std::pair<std::shared_ptr<SchemaTree>, std::vector<SchemaTree::NodeLocator>> 
{
    auto 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},
            // ... (other locators)
    };
    for (auto const& locator : locators) {
        schema_tree->insert_node(locator);
    }
    return {schema_tree, locators};
}
}

Then in each test case:

auto [schema_tree, locators] = create_test_schema_tree();

Also applies to: 369-387

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.

Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:148-157
Timestamp: 2025-05-02T22:27:59.347Z
Learning: In the `QueryHandlerImpl.cpp` file, the `unique_projected_columns` set (using `std::string_view`) is intentionally designed to only check for duplications within the local scope of the `create_projected_columns_and_projection_map` function. The team decided this is an acceptable use of `std::string_view` in a container since the referenced strings remain valid throughout the function's execution.