Adding support for HYBRID search. #3813
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for hybrid search functionality to the Redis search client, enabling combined text and vector similarity searches. The implementation introduces new query types, result parsers, and comprehensive test coverage.
Key changes:
- New hybrid query classes for combining text search and vector similarity operations
- Hybrid search command execution and result parsing
- Post-processing configuration for filtering, sorting, and aggregating results
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_search.py | Comprehensive test suite covering hybrid search functionality including various query types, filters, and post-processing options |
| redis/connection.py | Minor whitespace cleanup (removed blank line) |
| redis/commands/search/hybrid_query.py | New file implementing hybrid query classes and post-processing configuration |
| redis/commands/search/commands.py | Added hybrid_search method and result parsing logic |
| redis/commands/search/init.py | Registered hybrid search command parser |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
3 times, most recently
from
1e8345a to
3b3b4f5
Compare
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
from
3b3b4f5 to
551315b
Compare
petyaslavova
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
2 times, most recently
from
0ea7eec to
fc7c324
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
from
ce28c55 to
ae531b0
Compare
| def scorer(self, scorer: str) -> "HybridSearchQuery": | ||
| """ | ||
| Scoring algorithm for text search query. | ||
| Allowed values are "TFIDF" or "BM25" |
There was a problem hiding this comment.
Allowed values are type?: 'BM25' | 'TFIDF' | 'DISMAX' | 'DOCSCORE' according to the design. Also maybe it makes sense to keep value as Enum so we don't need to add validation around it?
There was a problem hiding this comment.
Tested the other options - they also work + found some other that are not listed in the spec and are supported. I would prefer to leave it as string and even not be too detailed in the docstring - I am not sure what are all of the actually supported values...
There was a problem hiding this comment.
@petyaslavova Well even if there's only few that you're sure about, I would make it Enum. This way user knows which values could be used, if even we dunno what work and what's not, for user it would be even harder to find what is the actual values
| Args: | ||
| method: The combine method to use - RRF or LINEAR. | ||
| kwargs: Additional combine parameters. |
There was a problem hiding this comment.
Same problem here, isn't really clear which parameters could be used with which method
| the alias for the projection, and the value is the projection | ||
| expression itself, for example `apply(square_root="sqrt(@foo)")`. | ||
| """ | ||
| for alias, expr in kwexpr.items(): |
There was a problem hiding this comment.
PRD also doesn't specifies an option to have multiple APPLY keywords
There was a problem hiding this comment.
But it works... And there is no other way to apply more than one field transformation except to have the apply twice or more. There is a test covering that case.
|
|
||
| return self | ||
|
|
||
| def sort_by(self, *sortby: "SortbyField") -> Self: |
There was a problem hiding this comment.
Looks like SortByField is missing WITHCOUNT option
There was a problem hiding this comment.
I can't find such an option in the spec.
There was a problem hiding this comment.
This is in the design doc. I think it based on FT.SEARCH spec
There was a problem hiding this comment.
This one is for the ft.search command. For ft.hybrid we still have only the confluence page. It doesn't support everything that is supported for ft.search.
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
from
ae531b0 to
a0985b4
Compare
…he problematic tests to use less data(sync and async tests)
…Extend a test to use two reducers.
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
from
f6a468a to
aa60a40
Compare
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
4 times, most recently
from
e79e5fb to
1bfffef
Compare
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Jit has detected 1 important finding in this PR that you should review.
Jit encountered an internal error and cannot comment on each finding.
You can ask a Jit admin to comment #jit_ignore_all on this PR to ignore the findings.
Here are the findings in this PR:
- Security Control: Static Code Analysis Semgrep Pro
- Type: Yaml.Github-Actions.Security.Run-Shell-Injection.Run-Shell-Injection
- Description: Using variable interpolation
${{...}}withgithubcontext data in arun:step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code.githubcontext data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable withenv:to store the data and use the environment variable in therun:script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR". - Severity: HIGH
- Learn More: Link
- Filename: .github/actions/run-tests/action.yml
- Lines: 107-136
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
from
1bfffef to
61f01fa
Compare
petyaslavova
force-pushed
the
ps_add_hybrid_search
branch
from
61f01fa to
6925030
Compare
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Please provide a description of the change here.