Skip to content

Remove dead duplicate Verify() from ft_search_parser - #1367

Open
allenss-amazon wants to merge 1 commit into
valkey-io:mainfrom
allenss-amazon:timeoutlimit
Open

allenss-amazon wants to merge 1 commit into
valkey-io:mainfrom
allenss-amazon:timeoutlimit

Conversation

@allenss-amazon

Copy link
Copy Markdown
Member

Deletes the unreachable Verify() in the anonymous namespace of src/commands/ft_search_parser.cc.

History:

No commit since has touched either validation body, so the dead copy is still byte-identical to VerifyQueryString(). It performs no check that VerifyQueryString() does not already perform.

No behavior change. Release build and all 23 unit test suites pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CmmwNpR7mMTD7CcTF9Xbd5

valkey-io#527 (ed06484) renamed the anonymous-namespace Verify() to VerifyQueryString(),
gave it external linkage so FT.AGGREGATE could call it, and updated its only
call site. valkey-io#618 (9b29188) then re-added the original Verify() body verbatim,
with no caller -- a copy/paste slip.

The resurrected copy has been unreachable ever since, and no commit has touched
either validation body since, so it is still byte-identical to
VerifyQueryString(). Deleting it removes no check that VerifyQueryString() does
not already perform.

No behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CmmwNpR7mMTD7CcTF9Xbd5
Signed-off-by: Allen Samuels <allenss@amazon.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Reviewers for this PR

  • First Pass Reviewer: @Aksha1812 — Please do your best to do a detailed review on the PR and get a response on your feedback. Once the first pass is done, notify the maintainer assigned to this PR to follow up on the final review and getting the PR merged. You can reach out to the people owning the relevant code paths for more help on the review.
  • Maintainer Reviewer: @yairgott — Once the first review is done, please follow up with a final review and help to merge the change in.

Assigned automatically to the least-assigned members of the reviewer pools in .github/reviewer-pools.json. Use /reviewer or /remove-reviewer to adjust.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 8d04a3e7-731f-4a9e-80e2-26bb38e44104

📥 Commits

Reviewing files that changed from the base of the PR and between e4880d4 and 025a0ac.

📒 Files selected for processing (1)
  • src/commands/ft_search_parser.cc
💤 Files with no reviewable changes (1)
  • src/commands/ft_search_parser.cc

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The patch relocates query validation from the anonymous namespace to VerifyQueryString. SearchCommand::ParseCommand now calls the relocated function. Validation behavior remains unchanged.

Changes

Query verification

Layer / File(s) Summary
Relocate query verification
src/commands/ft_search_parser.cc
The Verify helper was replaced by VerifyQueryString. ParseCommand now calls VerifyQueryString(*this).

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 025a0

This change removes an unused duplicate query-validation function without altering the active validation path. No merge-readiness risk is identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: removal of the unused duplicate Verify() function from ft_search_parser.
Description check ✅ Passed The description directly explains the duplicate function, its history, the absence of behavior change, and validation results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant