Address inefficiency related to creating and returning tag values as absl::flat_hash_set by value during predicate evaluation

.devcontainer/run_in_docker.sh

@@ -0,0 +1,144 @@
+#!/bin/bash
+
+# Find the workspace root (parent of .devcontainer)
+SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+WORKSPACE_DIR=$(cd "$SCRIPT_DIR/.." && pwd)
+WORKSPACE_BASENAME=$(basename "$WORKSPACE_DIR")
+CONTAINER_WORKSPACE="/workspaces/$WORKSPACE_BASENAME"
+
+# Get the current user's UID and GID to avoid permission mismatch issues
+USER_UID=$(id -u)
+USER_GID=$(id -g)
+USER_NAME=$(id -un)
+USER_GNAME=$(id -gn)
+
+# Determine the command to run
+COMMAND=("$@")
+if [ ${#COMMAND[@]} -eq 0 ]; then
+  COMMAND=("bash")
+fi
+
+# Detect if running in an interactive terminal (TTY)
+INTERACTIVE_FLAGS=""
+if [ -t 0 ] && [ -t 1 ]; then
+  INTERACTIVE_FLAGS="-it"
+fi
+
+CREATED_CONTAINER=false
+
+cleanup() {
+  HOST_DEPS_DIR="/tmp/valkey-search-deps"
+  if [ -n "$CONTAINER_ID" ]; then
+    mkdir -p "$HOST_DEPS_DIR"
+    docker cp "$CONTAINER_ID:/opt/valkey-search-deps/." "$HOST_DEPS_DIR" 2>/dev/null || true
+  fi
+  for comp_db in "$WORKSPACE_DIR"/.build-*/compile_commands.json "$WORKSPACE_DIR"/.build-*-container/compile_commands.json; do
+    if [ -f "$comp_db" ] && [ -s "$comp_db" ]; then
+      python3 -c '
+import sys
+with open(sys.argv[1], "r") as f:
+    content = f.read()
+content = content.replace(sys.argv[2], sys.argv[3])
+if len(sys.argv) > 5 and sys.argv[4] and sys.argv[5]:
+    content = content.replace(sys.argv[4], sys.argv[5])
+with open(sys.argv[1], "w") as f:
+    f.write(content)
+' "$comp_db" "/workspaces/$WORKSPACE_BASENAME" "$WORKSPACE_DIR" "/opt/valkey-search-deps" "$HOST_DEPS_DIR" 2>/dev/null || true
+      rel_path="${comp_db#$WORKSPACE_DIR/}"
+      ln -sfn "$rel_path" "$WORKSPACE_DIR/compile_commands.json" 2>/dev/null || true
+      break
+    fi
+  done
+  if [ "$CREATED_CONTAINER" = "true" ] && [ -n "$CONTAINER_ID" ]; then
+    docker rm -f "$CONTAINER_ID" 2>/dev/null || true
+  fi
+}
+# Register cleanup trap
+trap cleanup EXIT
+
+# Search for a running devcontainer for this workspace
+CONTAINER_ID=$(docker ps --filter "label=devcontainer.local_folder=$WORKSPACE_DIR" -q | head -n 1)
+
+if [ -n "$CONTAINER_ID" ]; then
+  # Running container found! 
+
+  # Copy .gitconfig from host if it exists to ensure git settings are available inside
+  if [ -f "$HOME/.gitconfig" ]; then
+    docker cp "$HOME/.gitconfig" "$CONTAINER_ID:/home/$USER_NAME/.gitconfig"
+    docker exec -u root "$CONTAINER_ID" chown "$USER_UID:$USER_GID" "/home/$USER_NAME/.gitconfig"
+  fi
+
+  # Copy non-secret SSH configuration from host if it exists (never private keys)
+  if [ -d "$HOME/.ssh" ]; then
+    docker exec -u "$USER_NAME" "$CONTAINER_ID" mkdir -p "/home/$USER_NAME/.ssh" 2>/dev/null || true
+    for item in config known_hosts known_hosts2; do
+      if [ -f "$HOME/.ssh/$item" ]; then
+        docker cp "$HOME/.ssh/$item" "$CONTAINER_ID:/home/$USER_NAME/.ssh/$item"
+        docker exec -u root "$CONTAINER_ID" chown "$USER_UID:$USER_GID" "/home/$USER_NAME/.ssh/$item"
+        docker exec -u "$USER_NAME" "$CONTAINER_ID" chmod 600 "/home/$USER_NAME/.ssh/$item"
+      fi
+    done
+    docker exec -u "$USER_NAME" "$CONTAINER_ID" chmod 700 "/home/$USER_NAME/.ssh" 2>/dev/null || true
+  fi
+
+  # Execute inside the container
+  ENV_FLAGS=("-e" "TERM=$TERM" "-e" "BUILD_DIR_SUFFIX=-container")
+
+  docker exec $INTERACTIVE_FLAGS "${ENV_FLAGS[@]}" -u "$USER_NAME" -w "$CONTAINER_WORKSPACE" "$CONTAINER_ID" "${COMMAND[@]}"
+else
+  # Fallback: Build and run a new container
+  IMAGE_NAME="valkey-search-dev"
+
+  # Build/update the docker image using the Dockerfile from .devcontainer
+  if ! docker build -q -t "$IMAGE_NAME" \
+    --build-arg USER_UID="$USER_UID" \
+    --build-arg USER_NAME="$USER_NAME" \
+    --build-arg USER_GID="$USER_GID" \
+    --build-arg USER_GNAME="$USER_GNAME" \
+    -f "$WORKSPACE_DIR/.devcontainer/Dockerfile" \
+    "$WORKSPACE_DIR/.devcontainer"; then
+    echo "Error: Failed to build Docker image '$IMAGE_NAME'." >&2
+    exit 1
+  fi
+
+  # Mount .gitconfig if it exists on the host
+  GITCONFIG_MOUNT=()
+  if [ -f "$HOME/.gitconfig" ]; then
+    GITCONFIG_MOUNT=("-v" "$HOME/.gitconfig:/home/$USER_NAME/.gitconfig:ro")
+  fi
+
+  # Mount non-secret SSH config files if they exist on the host (never private keys)
+  SSH_MOUNTS=()
+  if [ -f "$HOME/.ssh/config" ]; then
+    SSH_MOUNTS+=("-v" "$HOME/.ssh/config:/home/$USER_NAME/.ssh/config:ro")
+  fi
+  if [ -f "$HOME/.ssh/known_hosts" ]; then
+    SSH_MOUNTS+=("-v" "$HOME/.ssh/known_hosts:/home/$USER_NAME/.ssh/known_hosts:ro")
+  fi
+
+  # Mount .ccache if it exists on the host, or create it
+  CCACHE_MOUNT=()
+  if [ ! -d "$HOME/.ccache-valkey-devcontainer" ]; then
+    mkdir -p "$HOME/.ccache-valkey-devcontainer"
+  fi
+  CCACHE_MOUNT=("-v" "$HOME/.ccache-valkey-devcontainer:/home/$USER_NAME/.ccache")
+
+  ENV_FLAGS=("-e" "TERM=$TERM" "-e" "BUILD_DIR_SUFFIX=-container")
+  CREATED_CONTAINER=true
+  CONTAINER_ID="valkey-search-runner-$$"
+
+  docker run $INTERACTIVE_FLAGS \
+    --name "$CONTAINER_ID" \
+    -v "$WORKSPACE_DIR":"$CONTAINER_WORKSPACE" \
+    -w "$CONTAINER_WORKSPACE" \
+    -u "$USER_UID:$USER_GID" \
+    "${GITCONFIG_MOUNT[@]}" \
+    "${SSH_MOUNTS[@]}" \
+    "${CCACHE_MOUNT[@]}" \
+    --network host \
+    --security-opt seccomp=unconfined \
+    --cap-add SYS_PTRACE \
+    "${ENV_FLAGS[@]}" \
+    "$IMAGE_NAME" \
+    "${COMMAND[@]}"
+fi

.github/workflows/clang_tidy_format.yml

@@ -90,14 +90,12 @@ jobs:
             docker run --rm --privileged -v "$(pwd):/workspace" --user "ubuntu:ubuntu" presubmit-image \
             bash -c "ci/check_clang_format.sh \"$FILE\""
 
-            echo "Running clang-tidy thread-safety check on $FILE"
-            docker run --rm -v "$(pwd):/workspace" --user "ubuntu:ubuntu" \
-              -e TS_FILE="$FILE" -e TS_CONFIG="$TS_CONFIG" presubmit-image \
-              bash -c 'clang-tidy --quiet -p compile_commands.json --config="$TS_CONFIG" "$TS_FILE" 2>&1 | tail -n +3'
-
-            # TODO: Enable full clang-tidy after the existing code is tidied
-            # docker run --rm  -v "$(pwd):/workspace" --user "ubuntu:ubuntu" presubmit-image \
-            # bash -c "clang-tidy --quiet -p compile_commands.json \"$FILE\" 2>&1 | tail -n +3"
+            if [[ "$FILE" == *.cc ]]; then
+              echo "Running clang-tidy thread-safety check on $FILE"
+              docker run --rm --privileged -v "$(pwd):/workspace" --user "ubuntu:ubuntu" \
+                -e TS_FILE="$FILE" -e TS_CONFIG="$TS_CONFIG" presubmit-image \
+                bash -c 'ci/build_ubuntu.sh --no-build --configure >/dev/null 2>&1 && set -o pipefail; clang-tidy --quiet -p . --config="$TS_CONFIG" "$TS_FILE" 2>&1 | tail -n +3'
+            fi
           done
 
       - name: Ensure script execution success

ci/build_ubuntu.sh

@@ -153,7 +153,7 @@ function build_and_run_tests() {
     # enable core dumps
     echo Enabling core dumps
     ulimit -c unlimited
-    echo 'core.%p' | sudo tee /proc/sys/kernel/core_pattern
+    echo 'core.%p' | sudo tee /proc/sys/kernel/core_pattern || true
 
     # Skip building C++ test binaries for integration tests (they only need libsearch.so)
     if [[ "${BUILD_SH_ARGS}" == *"--run-integration-tests"* ]] && [[ "${BUILD_SH_ARGS}" != *"--run-tests"* ]]; then

src/indexes/tag.cc

@@ -315,6 +315,15 @@ std::optional<absl::flat_hash_set<absl::string_view>> Tag::GetValue(
   return std::nullopt;
 }
 
+std::optional<absl::string_view> Tag::GetRawTagString(
+    const InternedStringPtr& key) const {
+  if (auto it = tracked_tags_by_keys_.find(key);
+      it != tracked_tags_by_keys_.end()) {
+    return *it->second.raw_tag_string;
+  }
+  return std::nullopt;
+}
+
 // -- Search / EntriesFetcher / EntriesFetcherIterator --------------------
 
 Tag::EntriesFetcherIterator::EntriesFetcherIterator(
@@ -389,8 +398,9 @@ std::unique_ptr<EntriesFetcherBase> Tag::Search(
   size_t total = 0;
 
   auto collect_slot = [&](void* slot) {
-    if (slot == nullptr) return;
-    if (!seen.insert(slot).second) return;
+    if (slot == nullptr || !seen.insert(slot).second) {
+      return;
+    }
     matched_slots.push_back(slot);
     auto bag = BagOfInternedStringPtrs::Adopt(SlotToStorage(slot));
     total += bag.size();

src/indexes/tag.h

@@ -94,6 +94,10 @@ class Tag : public IndexBase {
       const InternedStringPtr& key,
       bool& case_sensitive) const ABSL_NO_THREAD_SAFETY_ANALYSIS;
 
+  // Returns the raw tag string for `key`, or nullopt if `key` is not tracked.
+  std::optional<absl::string_view> GetRawTagString(
+      const InternedStringPtr& key) const ABSL_NO_THREAD_SAFETY_ANALYSIS;
+
   // Iterator yielded by EntriesFetcher::Begin(). Walks a vector of rax slots
   // (each slot's 8 bytes encode a BagOfInternedStringPtrs); for negated
   // queries, also walks an extras vector of untracked keys.

src/indexes/vector_base.cc

@@ -88,9 +88,16 @@ bool PrefilterEvaluator::Evaluate(const query::Predicate &predicate,
 
 query::EvaluationResult PrefilterEvaluator::EvaluateTags(
     const query::TagPredicate &predicate) {
-  bool case_sensitive = true;
-  auto tags = predicate.GetIndex()->GetValue(*key_, case_sensitive);
-  return predicate.Evaluate(tags ? &*tags : nullptr, case_sensitive);
+  const auto *tag_index = predicate.GetIndex();
+  if (tag_index == nullptr) {
+    return query::EvaluationResult(false);
+  }
+  auto raw_tags = tag_index->GetRawTagString(*key_);
+  if (!raw_tags.has_value()) {
+    return query::EvaluationResult(false);
+  }
+  return predicate.Evaluate(*raw_tags, tag_index->GetSeparator(),
+                            tag_index->IsCaseSensitive());
 }
 
 query::EvaluationResult PrefilterEvaluator::EvaluateNumeric(

src/query/predicate.cc

@@ -13,7 +13,9 @@
 
 #include "absl/container/flat_hash_set.h"
 #include "absl/container/inlined_vector.h"
+#include "absl/strings/ascii.h"
 #include "absl/strings/match.h"
+#include "absl/strings/str_split.h"
 #include "absl/strings/string_view.h"
 #include "src/commands/filter_parser.h"
 #include "src/indexes/numeric.h"
@@ -359,6 +361,31 @@ EvaluationResult TagPredicate::Evaluate(Evaluator &evaluator) const {
   return evaluator.EvaluateTags(*this);
 }
 
+bool TagPredicate::MatchesSingleTag(absl::string_view in_tag,
+                                    bool case_sensitive) const {
+  for (const auto &tag : tags_) {
+    absl::string_view left_hand_side = in_tag;
+    absl::string_view right_hand_side = tag;
+    if (!right_hand_side.empty() && right_hand_side.back() == '*') {
+      if (left_hand_side.length() < right_hand_side.length() - 1) {
+        continue;
+      }
+      left_hand_side = left_hand_side.substr(0, right_hand_side.length() - 1);
+      right_hand_side = right_hand_side.substr(0, right_hand_side.length() - 1);
+    }
+    if (case_sensitive) {
+      if (left_hand_side == right_hand_side) {
+        return true;
+      }
+    } else {
+      if (absl::EqualsIgnoreCase(left_hand_side, right_hand_side)) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 EvaluationResult TagPredicate::Evaluate(
     const absl::flat_hash_set<absl::string_view> *in_tags,
     bool case_sensitive) const {
@@ -367,26 +394,20 @@ EvaluationResult TagPredicate::Evaluate(
   }
 
   for (const auto &in_tag : *in_tags) {
-    for (const auto &tag : tags_) {
-      absl::string_view left_hand_side = in_tag;
-      absl::string_view right_hand_side = tag;
-      if (right_hand_side.back() == '*') {
-        if (left_hand_side.length() < right_hand_side.length() - 1) {
-          continue;
-        }
-        left_hand_side = left_hand_side.substr(0, right_hand_side.length() - 1);
-        right_hand_side =
-            right_hand_side.substr(0, right_hand_side.length() - 1);
-      }
-      if (case_sensitive) {
-        if (left_hand_side == right_hand_side) {
-          return EvaluationResult(true);
-        }
-      } else {
-        if (absl::EqualsIgnoreCase(left_hand_side, right_hand_side)) {
-          return EvaluationResult(true);
-        }
-      }
+    if (MatchesSingleTag(in_tag, case_sensitive)) {
+      return EvaluationResult(true);
+    }
+  }
+  return EvaluationResult(false);
+}
+
+EvaluationResult TagPredicate::Evaluate(absl::string_view raw_tag_string,
+                                        char separator,
+                                        bool case_sensitive) const {
+  for (const auto &part : absl::StrSplit(raw_tag_string, separator)) {
+    absl::string_view in_tag = absl::StripAsciiWhitespace(part);
+    if (!in_tag.empty() && MatchesSingleTag(in_tag, case_sensitive)) {
+      return EvaluationResult(true);
     }
   }
   return EvaluationResult(false);

src/query/predicate.h

@@ -160,6 +160,8 @@ class TagPredicate : public Predicate {
   // Evaluate against tags (string_view set from indexed data or parsed query)
   EvaluationResult Evaluate(const absl::flat_hash_set<absl::string_view>* tags,
                             bool case_sensitive) const;
+  EvaluationResult Evaluate(absl::string_view raw_tag_string, char separator,
+                            bool case_sensitive) const;
   const indexes::Tag* GetIndex() const { return index_; }
   absl::string_view GetAlias() const { return alias_; }
   absl::string_view GetIdentifier() const {
@@ -172,6 +174,8 @@ class TagPredicate : public Predicate {
   const absl::flat_hash_set<std::string>& GetTags() const { return tags_; }
 
  private:
+  bool MatchesSingleTag(absl::string_view in_tag, bool case_sensitive) const;
+
   const indexes::Tag* index_;
   vmsdk::UniqueValkeyString identifier_;
   std::string alias_;