src/vector/predicate.cpp (1)

354-389: Allow single-child logical nodes to evaluate correctly.

The current size checks force any $and/$or node with a single child to return false. $in lowers to an OR, so a filter like {"tags": {"$in": ["foo"]}} always fails. Please treat “no children” as invalid, but allow one child to evaluate normally.

-        if (and_node.children_.size() < 2)
+        if (and_node.children_.empty())
         {
             return false;
         }
…
-        if (or_node.children_.size() < 2)
+        if (or_node.children_.empty())
         {
             return false;
         }

That restores the expected semantics for single-term $and, $or, and $in.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: liunyl
Repo: liunyl/eloqkv PR: 7
File: src/vector/vector_handler.cpp:457-466
Timestamp: 2025-09-16T05:13:52.366Z
Learning: In the EloqVec codebase, VectorIndex::search method should be extended to accept search_params parameter to support per-request search parameters, and the class should support default search/update parameters stored in VectorMetadata and applied during index initialization.

src/vector/predicate.cpp (1)

22-28: Add missing standard library includes.

The code uses standard library features without including their headers:

• std::memcmp requires <cstring>
• std::make_unique requires <memory>
• std::move requires <utility>
• std::vector requires <vector>
• std::string requires <string>

These missing includes were already flagged by cpplint in past reviews.

include/vector/predicate.h (2)

24-28: Add missing vector include.

The header uses std::vector on lines 139, 166-167, 188-189 but doesn't include <vector>. This was already flagged by cpplint in past reviews.

---

195-195: Add newline at end of file.

Missing newline character at the end of the file, as flagged by cpplint in previous reviews.

src/vector/predicate.cpp (3)

147-148: Validate single key-value pair before access.

The code assumes field_value contains exactly one key-value pair and directly accesses .begin() without validation. If a malformed filter provides multiple operators for a field (e.g., {"age": {"$gt": 18, "$lt": 65}}), only the first is processed silently.

Add validation:

+    if (field_value.size() != 1)
+    {
+        return nullptr;
+    }
+
     const std::string &op_str = field_value.begin().key();
     const nlohmann::json &value_json = field_value.begin().value();

---

198-217: Validate non-empty arrays for logical operators.

Empty $and arrays create nodes with no children. While evaluation will return false for such nodes (lines 323-326), it's better to reject invalid filters at parse time.

     if (obj.contains("$and"))
     {
-        if (!obj["$and"].is_array())
+        if (!obj["$and"].is_array() || obj["$and"].empty())
         {
             return nullptr;
         }

Apply the same check to $or at line 221.

---

319-336: Move node structure validation to parse time.

The check and_node.children_.size() < 2 validates tree structure at evaluation time, but structure is fixed after parsing. This check should be enforced when building the tree (in ParseJSONObject) and converted to an assert here for internal consistency.

Same issue applies to OR (lines 341-344) and NOT (lines 359-362) operators.

include/vector/predicate.h (1)

87-87: Consider making op_ protected for encapsulation.

The public op_ member exposes internal state. While this may be intentional for simplicity (similar to children_ in ParentPredNode), a getter would better encapsulate the implementation.

+protected:
     PredicateOp op_;
+
+public:
+    PredicateOp GetOp() const { return op_; }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: liunyl
Repo: liunyl/eloqkv PR: 7
File: src/vector/vector_handler.cpp:457-466
Timestamp: 2025-09-16T05:13:52.366Z
Learning: In the EloqVec codebase, VectorIndex::search method should be extended to accept search_params parameter to support per-request search parameters, and the class should support default search/update parameters stored in VectorMetadata and applied during index initialization.

include/vector/predicate.h (2)

173-176: Guard Root() against an empty expression.

If Parse() has not been called (or failed), root_node_ remains nullptr, so Root() dereferences a null pointer and triggers UB. Returning a pointer avoids the crash and forces callers to check for a parsed tree.

-    const PredicateNode &Root() const
-    {
-        return *root_node_;
-    }
+    const PredicateNode *Root() const
+    {
+        return root_node_.get();
+    }

---

24-29: Include the headers you use.

PredicateNode relies on uint8_t and LeafPredNode stores a std::vector<char>, but this header does not include <cstdint> or <vector>. Any translation unit that includes predicate.h without pulling those headers first will fail to compile. Please add the missing includes.

 #include <list>
 #include <memory>
 #include <string>
 #include <string_view>
+#include <cstdint>
+#include <vector>

src/vector/predicate.cpp (2)

303-309: Don’t crash in release when Evaluate is called too early.

Evaluate still dereferences root_node_ after only an assert, so in release builds (where asserts vanish) calling Evaluate before a successful Parse leads to a null dereference. Return false (or throw) when no predicate has been parsed.

-    assert(root_node_ != nullptr);
-    return EvaluateNode(*root_node_, metadata, offsets, schema);
+    if (root_node_ == nullptr)
+    {
+        return false;
+    }
+    return EvaluateNode(*root_node_, metadata, offsets, schema);

---

24-27: Add the standard headers needed by this TU.

CompareValues calls std::min and std::memcmp, but <algorithm> and <cstring> are not included. Depending on transitive includes makes the build brittle; please include them explicitly.

 #include "predicate.h"

 #include <cassert>
+#include <algorithm>
+#include <cstring>
 #include <nlohmann/json.hpp>

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: liunyl
Repo: liunyl/eloqkv PR: 7
File: src/vector/vector_handler.cpp:457-466
Timestamp: 2025-09-16T05:13:52.366Z
Learning: In the EloqVec codebase, VectorIndex::search method should be extended to accept search_params parameter to support per-request search parameters, and the class should support default search/update parameters stored in VectorMetadata and applied during index initialization.