Address inefficiency related to creating and returning tag values as absl::flat_hash_set by value during predicate evaluation - #1296
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change exposes raw tag values, evaluates separator-aware predicates in vector prefilters, adds a devcontainer command runner, and updates CI validation behavior. ChangesRaw tag matching
Devcontainer runtime
CI validation updates
Sequence Diagram(s)sequenceDiagram
participant PrefilterEvaluator
participant Tag
participant TagPredicate
PrefilterEvaluator->>Tag: GetRawTagString(key)
Tag-->>PrefilterEvaluator: Raw tag string or nullopt
PrefilterEvaluator->>TagPredicate: Evaluate(raw tag string, separator, case sensitivity)
TagPredicate-->>PrefilterEvaluator: Evaluation result
Suggested reviewers: Merge Risk: 🔵 Low · up to The development-container setup reuses a fixed temporary dependency directory, which can cause stale data or collisions between runs. The PR is otherwise mergeable, with owner awareness needed to make the export directory unique per run. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
2115810 to
c990f6b
Compare
yairgott
changed the title
absl::flat_hash_set by value during predicate evaluation
yairgott
changed the title
absl::flat_hash_set by value during predicate evaluationabsl::flat_hash_set by value during predicate evaluation
5e46f16 to
cdb7014
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.devcontainer/run_in_docker.sh:
- Around line 32-39: Replace the fixed sleep in the SSH agent setup around
socket_proxy.py with a bounded readiness loop that waits for $PROXY_SOCKET to
appear, checks whether PROXY_PID has exited, and fails with a clear error if the
proxy exits or the socket is not created before the timeout; export
SSH_AUTH_SOCK only after successful readiness verification.
- Around line 84-90: Restrict SSH exposure in .devcontainer/run_in_docker.sh: at
lines 84-90, update the .ssh copy logic to transfer only required non-secret
files such as config and known_hosts, never private keys; at lines 122-126,
remove the full .ssh bind mount and mount only explicitly selected non-secret
configuration files if needed. Preserve SSH-agent proxy access for signing.
- Around line 108-114: Make the Docker image build step in the script fail fast
by enabling appropriate shell error handling or explicitly checking the docker
build status before proceeding. Ensure docker run is skipped when the build
fails, preventing an existing valkey-search-dev image from being used.
- Around line 43-58: Update the dependency extraction flow around CONTAINER_ID
to create a unique per-run temporary directory with mktemp -d, use that
directory as the docker cp destination, and substitute the same path when
rewriting compile_commands.json. Avoid the fixed /tmp/valkey-search-deps path so
existing directories cannot cause nested copies or stale references.
In @.devcontainer/socket_proxy.py:
- Around line 15-25: Update the proxy’s socket, file, and cleanup exception
handling to catch only expected OSError failures instead of broad Exception
clauses. Ensure initialization and accept() failures are reported before
exiting, and route those diagnostics through a visible channel rather than the
launcher’s /dev/null output redirection.
In @.github/workflows/clang_tidy_format.yml:
- Line 97: Update the clang-tidy command in the workflow’s inner bash invocation
to enable pipefail, or otherwise explicitly preserve clang-tidy’s exit status
through the tail pipeline. Ensure the workflow fails whenever clang-tidy exits
non-zero while retaining the existing output filtering.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c19e3c8b-e794-4b6e-89d4-f702ca51c150
📒 Files selected for processing (5)
.devcontainer/run_in_docker.sh.devcontainer/socket_proxy.py.github/workflows/clang_tidy_format.ymlsrc/indexes/tag.ccsrc/query/predicate.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- src/indexes/tag.cc
- src/query/predicate.cc
9250929 to
7408a37
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.devcontainer/run_in_docker.sh:
- Around line 127-155: The fresh-container execution path around the bash and
command branches must assign a unique CONTAINER_ID before starting Docker, use
that name without --rm, and ensure cleanup copies /opt/valkey-search-deps before
removing the container. Update cleanup to remove only containers created by this
runner while preserving compile_commands.json rewriting and existing container
behavior.
- Around line 15-19: Update the command setup in the run script to preserve
argument boundaries by storing the incoming arguments in an array such as
COMMAND, defaulting that array to bash when no arguments are provided. Replace
every CMD-based bash -c execution with direct "${COMMAND[@]}" execution, and do
not add implicit shell evaluation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb8f6c86-779c-487d-81a2-feab14a683e1
📒 Files selected for processing (7)
.devcontainer/run_in_docker.sh.github/workflows/clang_tidy_format.ymlci/build_ubuntu.shsrc/indexes/tag.ccsrc/indexes/tag.hsrc/indexes/vector_base.cctesting/tag_index_test.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/clang_tidy_format.yml
- src/indexes/tag.h
- src/indexes/vector_base.cc
… predicate evaluation Signed-off-by: Yair Gottdenker <yairg@google.com>
7408a37 to
425bfe3
Compare
|
/reviewer mnunberg1 |
|
/reviewer KarthikSubbarao |
|
Reviewers for this PR
|
|
This should be split into different commits, I'm two files deep and there's nothing related to the actual title of the PR |
|
Hi @yairgott 👋 — this is tracked as a P2 for valkey-search 1.3. P2s aren't RC1 blockers, but we'd love to land them for GA. First-pass reviewer: @mnunberg1 — if your first-pass review is already done, please ignore this message; otherwise, please prioritize getting this PR reviewed. Second-pass reviewer: @KarthikSubbarao — please take a look/followup with the final review and merge once everything looks good. If it's close to ready, getting it merged soon keeps it comfortably ahead of GA. Board: #1346. Thanks! 🙏 |
5 participants
During vector search pre-filtering,
PrefilterEvaluator::EvaluateTagsinvokedTag::GetValue()for every candidate document key.Tag::GetValue()dynamically constructed and returned a heap-allocatedabsl::flat_hash_set<absl::string_view>viaParseRecordTags(). For queries evaluating thousands of candidate keys:std::optional<absl::flat_hash_set>by value added copy/move and destructor overhead per candidate check.ParseRecordTags()fully parsed and inserted all tags into the set before predicate matching began, even if the first tag matched.Solution
Tag::GetRawTagString()to return anabsl::string_viewof the key's stored raw tag string without container instantiation or heap allocation.TagPredicate::Evaluate(absl::string_view raw_tag_string, char separator, bool case_sensitive)that lazily splits the raw string and exits early as soon as a tag matches.TagPredicate::MatchesSingleTag()to share implementation between set-based and raw-string-based evaluation overloads.PrefilterEvaluator::EvaluateTags()to leverage the zero-allocation raw tag evaluation path.