components/core/CMakeLists.txt (1)

610-610: Nit: keep test-file list alphabetically sorted

For long source lists a consistent ordering eases grepping and reduces merge conflicts. Consider inserting test-ffi_ir_stream_search_utils.cpp in lexicographical position.

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

281-297: Obj nodes mapped to NullT may break downstream logic

In schema_tree_node_type_to_literal_types, an object node currently returns LiteralType::NullT.
Down-stream code could interpret this as “the only legal literal is null”, blocking legitimate object literals once they are supported.

If object support is still TODO, returning UnknownT (or at least adding ObjectT behind a feature flag) would avoid silently mis-categorising objects as null.

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

37-43: Visibility note: function returns depend on schema-node semantics

evaluate_filter_against_literal_type_value_pair operates on a literal type determined elsewhere.
For callers to use this API safely, please document that:

1. The caller must supply a literal type consistent with value and the underlying schema node.
2. The function does not attempt any implicit conversions (e.g., string-to-number).

This clarification will prevent accidental misuse and future bugs.

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

146-161: Avoid re-encoding the same string for every test – extract and cache the AST once

get_encoded_text_ast performs a full CLP string encoding every time a value is constructed (lines 314-316).
Given that the reference strings are static test fixtures, the repeated work is unnecessary and slows the test suite.

Consider caching the encoded ASTs once per literal type and re-using them:

-    std::vector<ValueToMatchedFilterOpsPair> const value_to_matched_filter_ops_pairs{
-            {Value{get_encoded_text_ast<encoded_variable_t>(matched)}, {FilterOperation::EQ}},
-            {Value{get_encoded_text_ast<encoded_variable_t>(unmatched)}, {FilterOperation::NEQ}},
-    };
+    static auto const matched_ast   = get_encoded_text_ast<encoded_variable_t>(matched);
+    static auto const unmatched_ast = get_encoded_text_ast<encoded_variable_t>(unmatched);
+
+    std::vector<ValueToMatchedFilterOpsPair> const value_to_matched_filter_ops_pairs{
+            {Value{matched_ast},   {FilterOperation::EQ}},
+            {Value{unmatched_ast}, {FilterOperation::NEQ}},
+    };

This keeps the tests deterministic while shaving noticeable time off larger test suites.

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

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

342-348: Implementation aligns with project's EXISTS/NEXISTS semantics.

As noted in the retrieved learnings, EXISTS always returns true and NEXISTS always returns false in this codebase. This implementation correctly follows that convention rather than checking value.has_value().

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

374-379: Consider adding a comment about literal type support.

Based on the past review comments, it would be helpful to add a comment explaining which literal types are planned for future implementation versus those that are intentionally unsupported.

    case LiteralType::EpochDateT:
    case LiteralType::ArrayT:
+        // ArrayT will be implemented in a future update, while EpochDateT is handled differently
+        // as schema_tree_node_type_value_pair_to_literal_type returns specific string types
        return ErrorCode{ErrorCodeEnum::LiteralTypeUnsupported};
    case LiteralType::NullT:
    case LiteralType::UnknownT:
    default:
+        // NullT should be unreachable (EXISTS/NEXISTS filters only)
+        // UnknownT indicates column resolution failure
        return ErrorCode{ErrorCodeEnum::LiteralTypeUnexpected};

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

Learnt from: LinZhihao-723
PR: y-scope/clp#856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:340-347
Timestamp: 2025-04-26T02:19:33.275Z
Learning: In the CLP codebase, the FilterOperation::EXISTS and FilterOperation::NEXISTS operations have specific semantics that differ from simply checking if a value exists. EXISTS always returns true and NEXISTS always returns false in the evaluate_filter_against_literal_type_value_pair function. The optional::has_value() is used specifically to differentiate empty objects from concrete values, not for existence checks in filter operations.

Learnt from: LinZhihao-723
PR: y-scope/clp#856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:258-266
Timestamp: 2025-04-26T02:21:22.021Z
Learning: In the clp::ffi::ir_stream::search namespace, the design principle is that callers are responsible for type checking, not the called functions. If control flow reaches a function, input types should already be validated by the caller.

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

109-109: Fix the typo in the function comment.

The function comment contains a grammatical error - it should be "Checks filter evaluation against variable strings" instead of "Checks filter evaluation against and variable strings".

- * Checks filter evaluation against and variable strings.
+ * Checks filter evaluation against variable strings.

---

120-120: Fix the typo in the function comment.

Similar to the previous comment, the function comment contains a grammatical error - it should be "Checks filter evaluation against encoded text ASTs" instead of "Checks filter evaluation against and encoded text ASTs".

- * Checks filter evaluation against and encoded text ASTs (Clp strings).
+ * Checks filter evaluation against encoded text ASTs (Clp strings).

---

289-291: Consider optimizing string concatenation.

Creating two temporary strings and then concatenating them could be optimized by creating a single string with the combined content.

- {Value{std::string{cRefTestString}}, {FilterOperation::EQ}},
- {Value{std::string{cRefTestString} + std::string{cRefTestString}},
-  {FilterOperation::EQ}},
+ {Value{std::string{cRefTestString}}, {FilterOperation::EQ}},
+ {Value{std::string{cRefTestString.data(), cRefTestString.size() * 2}},
+  {FilterOperation::EQ}},

---

310-321: Consider reducing code duplication.

This pattern of checking for mismatched literal types and returning a predefined result is duplicated across multiple check functions. Consider extracting it to a helper function.

+ /**
+  * Helper function to check if the literal type matches and return false if not.
+  */
+ [[nodiscard]] auto check_literal_type_mismatch(
+        FilterExpr const* filter_to_test,
+        LiteralType expected_type,
+        LiteralType actual_type
+ ) -> bool {
+     if (expected_type != actual_type) {
+         std::optional<Value> const empty_value_to_evaluate;
+         auto const actual_match_result{evaluate_filter_against_literal_type_value_pair(
+                filter_to_test,
+                actual_type,
+                empty_value_to_evaluate,
+                false
+         )};
+         REQUIRE_FALSE(actual_match_result.has_error());
+         return false == actual_match_result.value();
+     }
+     return true;
+ }

Then use it in each check function:

template <typename encoded_variable_t>
requires std::is_same_v<encoded_variable_t, eight_byte_encoded_variable_t>
         || std::is_same_v<encoded_variable_t, four_byte_encoded_variable_t>
auto check_filter_evaluation_against_encoded_text_asts(
        FilterExpr const* filter_to_test,
        LiteralType filter_operand_literal_type
) -> bool {
-    if (LiteralType::ClpStringT != filter_operand_literal_type) {
-        std::optional<Value> const empty_value_to_evaluate;
-        auto const actual_match_result{evaluate_filter_against_literal_type_value_pair(
-                filter_to_test,
-                filter_operand_literal_type,
-                empty_value_to_evaluate,
-                false
-        )};
-        REQUIRE_FALSE(actual_match_result.has_error());
-        return false
-               == actual_match_result.value();
+    if (!check_literal_type_mismatch(filter_to_test, LiteralType::ClpStringT, filter_operand_literal_type)) {
+        return false;
    }

---

367-419: Consider simplifying validation logic pattern.

The repeated pattern of checking return values against false followed by a WARN message and return statement could be simplified using a helper function or macro to reduce repetition and improve readability.

+ /**
+  * Helper macro to check function result and issue warning on failure.
+  */
+ #define CHECK_EVAL_OR_WARN(evalFunc, failMsg) \
+     if (false == (evalFunc)) { \
+         WARN(failMsg); \
+         return false; \
+     }

auto
check_filter_evaluation(FilterExpr const* filter_to_test, LiteralType filter_operand_literal_type)
        -> bool {
    // ... existing code ...

-    if (false
-        == check_filter_evaluation_against_integral_values<value_int_t>(
-                filter_to_test,
-                filter_operand_literal_type
-        ))
-    {
-        WARN("Failed to evaluate filter against integers.");
-        return false;
-    }
+    CHECK_EVAL_OR_WARN(
+        check_filter_evaluation_against_integral_values<value_int_t>(
+            filter_to_test, 
+            filter_operand_literal_type
+        ),
+        "Failed to evaluate filter against integers."
+    );

    // ... apply to other similar blocks ...

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

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

1-1: Typo in file name reduces discoverability

The file is named test_utilis.cpp; consider renaming to test_utils.cpp (or test_utilities.cpp) to match the common spelling of “utilities”. This improves searchability and consistency with the rest of the codebase.

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

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

429-432: Prefer direct boolean assertions for pointer checks

The current code uses REQUIRE_FALSE((nullptr == ref_ptr)) pattern for multiple pointer validity checks. While this works, consider using a more direct approach:

-REQUIRE_FALSE((nullptr == ref_str_literal));
-REQUIRE_FALSE((nullptr == ref_bool_literal));
-REQUIRE_FALSE((nullptr == ref_int_literal));
-REQUIRE_FALSE((nullptr == ref_float_literal));
+REQUIRE(nullptr != ref_str_literal);
+REQUIRE(nullptr != ref_bool_literal);
+REQUIRE(nullptr != ref_int_literal);
+REQUIRE(nullptr != ref_float_literal);

This makes the assertion's intent clearer and eliminates unnecessary parentheses.

---

437-438: Consistent style with other pointer validity checks

Apply the same suggestion as with the other pointer checks:

-REQUIRE_FALSE((nullptr == column_descriptor));
+REQUIRE(nullptr != column_descriptor);

---

496-497: Consistent style with other pointer validity checks

Apply the same suggestion as with the other pointer checks:

-REQUIRE_FALSE((nullptr == filter_to_test));
+REQUIRE(nullptr != filter_to_test);

---

152-159: Consider adding additional bounds validation

The code uses dict_var_bounds indices directly for string slicing. While there's a check that ensures there's an even number of bounds, consider adding a range check to ensure the bounds are within the text's limits before accessing them:

REQUIRE(((dict_var_bounds.size() % 2) == 0));

std::vector<std::string> dict_vars;
for (size_t i{0}; i < dict_var_bounds.size(); i += 2) {
    auto const begin_pos{static_cast<size_t>(dict_var_bounds[i])};
    auto const end_pos{static_cast<size_t>(dict_var_bounds[i + 1])};
+   REQUIRE(begin_pos <= end_pos);
+   REQUIRE(end_pos <= text.size());
    dict_vars.emplace_back(text.cbegin() + begin_pos, text.cbegin() + end_pos);
}

This would catch potential issues with invalid boundaries.

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

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

61-64: Introduce a reusable concept to avoid duplicated requires clauses

The same std::is_same_v<encoded_variable_t, …> constraint appears twice (lines 61-64 and 145-146). Extracting it into a named concept simplifies the declaration and keeps the intent DRY.

+#include <concepts>   // already present
+
+template<typename T>
+concept EncodedVariable =
+       std::same_as<T, eight_byte_encoded_variable_t>
+    || std::same_as<T, four_byte_encoded_variable_t>;
 
-template <typename encoded_variable_t>
-requires std::is_same_v<encoded_variable_t, eight_byte_encoded_variable_t>
-         || std::is_same_v<encoded_variable_t, four_byte_encoded_variable_t>
+template <EncodedVariable encoded_variable_t>

Apply the same EncodedVariable concept where the second duplicate constraint occurs.
This removes boiler-plate and makes future maintenance easier.

Also applies to: 145-146

---

283-289: Consider widening negative-test coverage for var-strings

At the moment only a prefix substring ("te") is evaluated against the NEQ operation. Adding a couple of additional non-matching scenarios (e.g., an empty string and a completely unrelated token) would strengthen the assertion that wildcard handling behaves as expected for all corner cases.

This is strictly optional, but helps future contributors understand the intended semantics.

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