-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow thresholding on vector and fulltext indexes for Hybrid retrievers #239
Conversation
95dd2d9
to
4c1976c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few minor points
I'd be interested in seeing an example of this. Here are the points bothering me:
|
3e4d313
to
ef33344
Compare
f2447f6
to
be3d3cb
Compare
@CodiumAI-Agent /update_changelog |
Changelog updates: 🔄 2025-01-15Added
|
…thresholds are provided
3ac51fd
to
72a6e88
Compare
f"RETURN n.node AS node, (n.score / ft_index_max_score) AS score }} " | ||
f"WITH node, max(score) AS score ORDER BY score DESC LIMIT $top_k" | ||
) | ||
return f"""CALL () {{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this to a multi-line string here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helps me modify the unit tests more easily without worrying about correct indentation and spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With multi-line strings, you do have to worry about correct indentation though. With the
(
"Hello "
"world"
)
approach you only have to worry about there being a space at the end of every line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I still generally prefer multi-line strings as I find them more readable and tend to avoid mistakes after changes. Do you think we should revert this back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m inclined to prefer the older approach, but I’m also open to this option. @stellasia what are your thoughts?
I apologise for chiming in a bit late, but here's my opinion. |
Changelog updates: 🔄 2025-01-22Added
|
Changelog updates: 🔄 2025-02-24Added
|
Description
Allow thresholding on vector and fulltext indexes for Hybrid retrievers. Two thresholds can be provided by the user during search to determine the importance of the search results from either vector or fulltext index.
Type of Change
Complexity
Complexity: Low
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):