Repository navigation
Conversation
disjunct() and optional() asked the parent node for its string without
parens, so the prefix landed on the first child only: disjunct('foo', 'bar')
rendered '(-foo bar)' and RediSearch read that as 'require bar, exclude
foo' instead of 'exclude both'. The parent already adds parens for a node
with more than one child, so let it.
Signed-off-by: simpleqt <2015900818@qq.com>
simpleqt
requested review from
ahmedsobeh,
bogdanp05,
mkmkme and
smeso
as code owners
October 21, 2026 16:14
Contributor
Author
|
Closing this to get my open pull requests in this repository down to a sane number: I had 8 open here at once, which is more than anyone should be asked to review. The branch stays in my fork if it is ever useful - happy to reopen on request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix the
-and~search prefixes to cover every childdisjunct()andoptional()build a prefix node, but asked the parent for itsstring without parens:
so the prefix landed on the first child only:
RediSearch binds the
-of(-foo bar)tofooalone, so that query reads as"require bar, exclude foo" — not the "all of its children must evaluate to
false" this node documents. The same applies to
~, where it boosts only thefirst child instead of the group. The result is a silently different query with
no error.
The parent already owns this decision —
Node._should_use_parenreturnslen(self.params) > 1when the caller has no preference — so the fix is to stopoverriding it and let the parent add the parens, passing the caller's choice
through:
Testing
tests/test_search_querystring_nodes.py(new, serverless — the search testmodules carry a module-level skip because they need a running service):
assert '(-foo bar)' == '-(foo bar)'for both prefixes, plus the nested-union,
disjunct_unionand composition cases(
tests/test_helpers.py,tests/test_parsers) → 19 passedThe tests also pin the explicit cases (
to_string(True)/to_string(False)),which is what stops this from regressing into the opposite mistake of never
grouping.
ruffat the version the repo pins (0.17.0:checkandformat --check) isclean on both files.
No existing test asserted the old strings (checked with
git grep), and the onlyin-repo consumer of these nodes is the query builder, so nothing else changes.
Note: my open PR #370 also edits
valkey/commands/search/querystring.py, atthe unrelated
geo()helper. The two changes touch different functions, sowhichever lands second should merge cleanly, but say the word if you would rather
have them as one change and I will combine them.
AI assistance: this change was produced with AI assistance (an autonomous coding
agent); the analysis, the reproduction, the before/after test runs and the lint
runs were executed locally, and a human reviewed the result before submission.