[MOD-12647] fix: handle the case in Coordinator when SCORE is sent alone without extra fields.#7492
Merged
[MOD-12647] fix: handle the case in Coordinator when SCORE is sent alone without extra fields.#7492
Conversation
JoanFM
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
JoanFM
force-pushed
the
joan-mod-12647
branch
from
583e942 to
ee99514
Compare
JoanFM
force-pushed
the
joan-mod-12647
branch
from
5a1a31a to
556a4ce
Compare
kei-nan
reviewed
Nov 24, 2025
kei-nan
reviewed
Nov 24, 2025
kei-nan
reviewed
Nov 24, 2025
kei-nan
reviewed
Nov 24, 2025
JoanFM
force-pushed
the
joan-mod-12647
branch
from
beb37ef to
0bb5d75
Compare
kei-nan
reviewed
Nov 24, 2025
JoanFM
force-pushed
the
joan-mod-12647
branch
from
0bb5d75 to
f164ece
Compare
…butes in response from shard
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7492 +/- ##
==========================================
- Coverage 85.05% 84.98% -0.07%
==========================================
Files 348 349 +1
Lines 53773 53879 +106
Branches 14353 14384 +31
==========================================
+ Hits 45734 45787 +53
- Misses 7843 7896 +53
Partials 196 196
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
oshadmi
reviewed
Nov 26, 2025
tests/pytests/test_expire.py
Outdated
| env.assertEqual(res, [1, ['t', 'arr'], ['t', 'bar']]) | ||
|
|
||
|
|
||
| def test_expire_ft_hybrid(env): |
Collaborator
There was a problem hiding this comment.
If the fix is relevant for RESP3, shouldn't the test also run using RESP3?
Collaborator
Author
There was a problem hiding this comment.
isnt it the default here? how should I enforce this?
Collaborator
Author
There was a problem hiding this comment.
Added tests for both
| results[key] = result | ||
| total_results = response.get('total_results', 0) | ||
| return results, total_results | ||
|
|
There was a problem hiding this comment.
Bug: Breaking API change in helper function
The get_results_from_hybrid_response function signature changed from returning a single dict to returning a tuple (results, total_results). This breaks all existing callers in other test files like test_default_scorer.py that aren't updated in this commit, causing them to fail when they try to use the return value as a dict instead of unpacking the tuple.
Collaborator
Author
There was a problem hiding this comment.
the signature did not change, it returns results, total_results in both cases.
oshadmi
reviewed
Nov 26, 2025
JoanFM
force-pushed
the
joan-mod-12647
branch
from
421b932 to
451305b
Compare
oshadmi
reviewed
Nov 26, 2025
oshadmi
approved these changes
Nov 26, 2025
kei-nan
approved these changes
Nov 27, 2025
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-7492-to-2.10 origin/2.10
cd .worktree/backport-7492-to-2.10
git switch --create backport-7492-to-2.10
git cherry-pick -x b916f078190664ddf8d51d521f79caa13e3fc1bf |
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 8.2
git worktree add -d .worktree/backport-7492-to-8.2 origin/8.2
cd .worktree/backport-7492-to-8.2
git switch --create backport-7492-to-8.2
git cherry-pick -x b916f078190664ddf8d51d521f79caa13e3fc1bf |
redisearch-backport-pull-request bot
pushed a commit
that referenced
this pull request
Nov 27, 2025
…one without extra fields. (#7492) * fix: Do not send score alone if Expired flag is set * test: add test_expire_ft_hybrid * improve test * do not send NULL, simply avoid serialize result if expired * improving test * handle more complete hybrid expire test * small change in test * revert changes in hybrid exec * remove assertion that cannot be guaranteed, and handle no extra_attributes in response from shard * handle both resp protocols (cherry picked from commit b916f07)
Contributor
|
Successfully created backport PR for |
JoanFM
added a commit
that referenced
this pull request
Nov 27, 2025
…one without extra fields. (#7492) * fix: Do not send score alone if Expired flag is set * test: add test_expire_ft_hybrid * improve test * do not send NULL, simply avoid serialize result if expired * improving test * handle more complete hybrid expire test * small change in test * revert changes in hybrid exec * remove assertion that cannot be guaranteed, and handle no extra_attributes in response from shard * handle both resp protocols (cherry picked from commit b916f07)
JoanFM
added a commit
that referenced
this pull request
Nov 27, 2025
…one without extra fields. (#7492) * fix: Do not send score alone if Expired flag is set * test: add test_expire_ft_hybrid * improve test * do not send NULL, simply avoid serialize result if expired * improving test * handle more complete hybrid expire test * small change in test * revert changes in hybrid exec * remove assertion that cannot be guaranteed, and handle no extra_attributes in response from shard * handle both resp protocols (cherry picked from commit b916f07)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 27, 2025
…ent alone without extra fields. (#7553) [MOD-12647] fix: handle the case in Coordinator when SCORE is sent alone without extra fields. (#7492) * fix: Do not send score alone if Expired flag is set * test: add test_expire_ft_hybrid * improve test * do not send NULL, simply avoid serialize result if expired * improving test * handle more complete hybrid expire test * small change in test * revert changes in hybrid exec * remove assertion that cannot be guaranteed, and handle no extra_attributes in response from shard * handle both resp protocols (cherry picked from commit b916f07) Co-authored-by: Joan Fontanals <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 2, 2025
…ent alone without extra fields. (#7554) * [MOD-12647] fix: handle the case in Coordinator when SCORE is sent alone without extra fields. (#7492) * fix: Do not send score alone if Expired flag is set * test: add test_expire_ft_hybrid * improve test * do not send NULL, simply avoid serialize result if expired * improving test * handle more complete hybrid expire test * small change in test * revert changes in hybrid exec * remove assertion that cannot be guaranteed, and handle no extra_attributes in response from shard * handle both resp protocols (cherry picked from commit b916f07) * small refactor
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
3 participants
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.
Describe the changes in the pull request
In case of Loading Document fails because key is expired or trimmed, Hybrid Result serializer is sending scores even when documents are not loaded.
This breaks the invairants in the Coordinator leading to a potential crash.
Note
Gracefully handle RESP3 rows with scores but no fields in the coordinator, update hybrid response parsing for RESP3, and add FT.HYBRID expiration tests for RESP2/RESP3.
src/coord/rpnet.c):scorewithoutextra_attributes(e.g., expired docs): addhas_fieldsguard and skip field writes when absent.get_results_from_hybrid_responseto parse RESP3 dict replies and return results withtotal_results.test_expire_ft_hybrid_resp2/resp3) verifying only non-expired docs are returned with expected attributes.Written by Cursor Bugbot for commit 451305b. This will update automatically on new commits. Configure here.