Conversation
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/edge/python/src/query.rs (1)
304-360:⚠️ Potential issue | 🟡 MinorUpdate Python stub to match the
Fusion.Rrf(k, weights)API.The stub in
qdrant_edge.pyi(line 1425) still documentsRrfk(rrfk: int), but the implementation now exposesRrf { k, weights: Option<Vec<f32>> }. Update the stub to reflect the new signature with both parameters.
🤖 Fix all issues with AI agents
In `@lib/api/src/rest/schema.rs`:
- Around line 541-545: The weights field (Option<Vec<f32>>) must be validated
against the API contract: add request-level validation that when
weights.is_some() it has the same length as the corresponding prefetches list
and every weight is finite, non-NaN, and non-negative (or within any documented
min/max); if validation fails, return a 400 error with a clear message.
Implement this check in the request validation path for the struct that contains
weights (e.g., add a validate() method or add checks in the handler that
constructs scoring input) so you explicitly reject mismatched lengths and
invalid f32 values before scoring is invoked.
In `@lib/segment/src/common/reciprocal_rank_fusion.rs`:
- Around line 38-58: The docstring for rrf_scoring is inaccurate: it claims
weights "must match the number of sources" but the implementation (in
rrf_scoring where weight is computed via weights.and_then(|w|
w.get(source_idx).copied()).unwrap_or(1.0)) falls back to 1.0 for missing
entries; update the documentation for the weights parameter to state that
weights are optional, that if provided they should preferably match the number
of sources but any missing entries will default to 1.0 (or alternatively enforce
validation by checking weights.len() == number_of_sources and returning an
error/panic); reference rrf_scoring and the weights handling logic when making
the change.
In `@tests/openapi/helpers/helpers.py`:
- Around line 141-143: The helper iterates responses using source_idx and
directly indexes weights, which raises IndexError when weights is shorter;
change how weight is derived in the loop (the weight variable near "for
source_idx, response in enumerate(responses):") to use a safe fallback of 1.0
when weights is None or source_idx >= len(weights) (e.g., check weights and
bounds before indexing) so missing weights mirror the Rust scorer's default.
In `@tests/openapi/test_query.py`:
- Around line 489-493: The test uses zip(sorted(rrf_expected, key=get_id),
sorted(rrf_result, key=get_id)) which will silently ignore length mismatches;
change the call to use zip(..., strict=True) so the comparison fails if
rrf_expected and rrf_result differ in length, referencing the variables
rrf_expected, rrf_result and the key function get_id in the test loop.
🧹 Nitpick comments (4)
lib/collection/src/operations/universal_query/shard_query.rs (1)
26-26: Prefer explicit field ignores instead of..inRrfmatch.This keeps the match future-proof when new fields are added.
♻️ Suggested change
- FusionInternal::Rrf { .. } | FusionInternal::Dbsf => Some(Order::LargeBetter), + FusionInternal::Rrf { k: _, weights: _ } | FusionInternal::Dbsf => { + Some(Order::LargeBetter) + }lib/api/src/grpc/proto/points.proto (1)
957-961: Clarify missing/zero‑weight behavior in the proto docs.Current docs only say weights “should match” prefetches, but the scoring logic treats missing weights as 1.0 and non‑positive weights as 0.0. Documenting this avoids client confusion.
💡 Suggested doc tweak
- // The number of weights should match the number of prefetches. + // The number of weights should match the number of prefetches; missing weights + // default to 1.0 and non‑positive weights contribute 0 to scoring.lib/segment/src/common/reciprocal_rank_fusion.rs (1)
14-31: Document non‑positive weight behavior inposition_score.The function returns 0.0 for
weight <= 0.0, but the doc doesn’t mention this. Adding a short note will align expectations.✍️ Suggested doc tweak
-/// Higher weights give more influence to a source. For example, with weight=3.0: +/// Higher weights give more influence to a source. Non‑positive weights contribute 0.0. +/// For example, with weight=3.0:lib/shard/src/query/mod.rs (1)
150-150: Use explicit field ignoring instead of..Per coding guidelines, prefer explicit field ignoring with
: _to ensure compilation fails when new fields are added, forcing explicit consideration of how they should be handled.Suggested fix
- FusionInternal::Rrf { .. } => true, + FusionInternal::Rrf { k: _, weights: _ } => true,As per coding guidelines: "Prefer explicit field ignoring using
: _over using..in Rust struct destructuring".
| for source_idx, response in enumerate(responses): | ||
| weight = weights[source_idx] if weights else 1.0 | ||
| for i, scored_point in enumerate(response): |
There was a problem hiding this comment.
Guard against short weights lists to match server fallback.
Direct indexing raises IndexError when fewer weights than sources are passed. The Rust scorer defaults missing weights to 1.0, so mirroring that avoids test helper crashes.
💡 Suggested fix
- for source_idx, response in enumerate(responses):
- weight = weights[source_idx] if weights else 1.0
+ for source_idx, response in enumerate(responses):
+ weight = weights[source_idx] if (weights and source_idx < len(weights)) else 1.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for source_idx, response in enumerate(responses): | |
| weight = weights[source_idx] if weights else 1.0 | |
| for i, scored_point in enumerate(response): | |
| for source_idx, response in enumerate(responses): | |
| weight = weights[source_idx] if (weights and source_idx < len(weights)) else 1.0 | |
| for i, scored_point in enumerate(response): |
🤖 Prompt for AI Agents
In `@tests/openapi/helpers/helpers.py` around lines 141 - 143, The helper iterates
responses using source_idx and directly indexes weights, which raises IndexError
when weights is shorter; change how weight is derived in the loop (the weight
variable near "for source_idx, response in enumerate(responses):") to use a safe
fallback of 1.0 when weights is None or source_idx >= len(weights) (e.g., check
weights and bounds before indexing) so missing weights mirror the Rust scorer's
default.
| # rrf order is not deterministic with same scores, so we need to sort by id | ||
| for expected, result in zip(sorted(rrf_expected, key=get_id), sorted(rrf_result, key=get_id)): | ||
| assert expected["id"] == result["id"] | ||
| assert expected.get("payload") == result.get("payload") | ||
| assert isclose(expected["score"], result["score"], rel_tol=1e-5) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the specific lines
head -n 495 tests/openapi/test_query.py | tail -n 10Repository: qdrant/qdrant
Length of output: 505
🏁 Script executed:
# Also check the overall context around these lines
head -n 500 tests/openapi/test_query.py | tail -n 20Repository: qdrant/qdrant
Length of output: 811
🏁 Script executed:
# Check Python version requirements in the project
find . -maxdepth 2 -type f \( -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" -o -name ".python-version" \) | head -20Repository: qdrant/qdrant
Length of output: 79
🏁 Script executed:
# Check Python version requirements
cat ./tests/pyproject.toml | head -50Repository: qdrant/qdrant
Length of output: 1094
🏁 Script executed:
# Also check the root pyproject.toml
cat pyproject.toml | head -50Repository: qdrant/qdrant
Length of output: 103
🏁 Script executed:
# Look for python version in setup.py or other config files at root
find . -maxdepth 1 -type f \( -name "setup.py" -o -name "setup.cfg" \) -exec cat {} \;Repository: qdrant/qdrant
Length of output: 39
Add strict zip to catch length mismatches in test assertions.
zip() stops at the shorter sequence, so if rrf_expected and rrf_result have different lengths, the test would silently pass by only comparing the overlapping elements. Since the project requires Python ≥3.10, use strict=True to ensure both lists have the same length:
Proposed fix
- for expected, result in zip(sorted(rrf_expected, key=get_id), sorted(rrf_result, key=get_id)):
+ expected_sorted = sorted(rrf_expected, key=get_id)
+ result_sorted = sorted(rrf_result, key=get_id)
+ for expected, result in zip(expected_sorted, result_sorted, strict=True):🧰 Tools
🪛 Ruff (0.14.14)
[warning] 490-490: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In `@tests/openapi/test_query.py` around lines 489 - 493, The test uses
zip(sorted(rrf_expected, key=get_id), sorted(rrf_result, key=get_id)) which will
silently ignore length mismatches; change the call to use zip(..., strict=True)
so the comparison fails if rrf_expected and rrf_result differ in length,
referencing the variables rrf_expected, rrf_result and the key function get_id
in the test loop.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/shard/src/query/planned_query.rs`:
- Around line 209-210: The comment above the MergePlan::new(sources,
rescore_stages)? call is misleading—update it to state that a root-level query
without prefetches will not perform additional rescoring passes beyond the
merge, but may still have rescore_stages set (e.g., for MMR) and those stages
will be honored; change the comment to explicitly mention rescore_stages/MMR so
future readers understand that rescore_stages can be present even when no extra
rescoring is performed.
🧹 Nitpick comments (1)
docs/redoc/master/openapi.json (1)
15715-15735: Consider constrainingweightsvalues (and non-empty array) if negatives/empty are invalid.If Weighted RRF expects non-negative weights and a non-empty list when provided, reflect that in the schema to prevent invalid client payloads.
♻️ Suggested schema tightening
"weights": { "description": "Weights for each prefetch source. Higher weight gives more influence on the final ranking. If not specified, all prefetches are weighted equally. The number of weights should match the number of prefetches.", "type": "array", + "minItems": 1, "items": { "type": "number", - "format": "float" + "format": "float", + "minimum": 0 }, "nullable": true }
| // Root-level query without prefetches means we won't do any extra rescoring | ||
| let merge_plan = MergePlan::new(sources, rescore_stages)?; |
There was a problem hiding this comment.
Clarify the “no extra rescoring” comment.
Root-level queries without prefetches still set rescore_stages for MMR, so this comment is misleading.
✏️ Suggested wording
- // Root-level query without prefetches means we won't do any extra rescoring
+ // Root-level query without prefetches only uses rescore_stages for MMR📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Root-level query without prefetches means we won't do any extra rescoring | |
| let merge_plan = MergePlan::new(sources, rescore_stages)?; | |
| // Root-level query without prefetches only uses rescore_stages for MMR | |
| let merge_plan = MergePlan::new(sources, rescore_stages)?; |
🤖 Prompt for AI Agents
In `@lib/shard/src/query/planned_query.rs` around lines 209 - 210, The comment
above the MergePlan::new(sources, rescore_stages)? call is misleading—update it
to state that a root-level query without prefetches will not perform additional
rescoring passes beyond the merge, but may still have rescore_stages set (e.g.,
for MMR) and those stages will be honored; change the comment to explicitly
mention rescore_stages/MMR so future readers understand that rescore_stages can
be present even when no extra rescoring is performed.
coszio
left a comment
There was a problem hiding this comment.
Only some minor nits, but looks good to me
| if weight <= 0.0 { | ||
| return 0.0; | ||
| } | ||
| 1.0 / (position as f32 * (1.0 / weight) + k as f32) |
There was a problem hiding this comment.
Initially I was going to suggest to put the weight in the numerator of the original function. Like
In
I would only suggest to do the division directly:
| 1.0 / (position as f32 * (1.0 / weight) + k as f32) | |
| 1.0 / (position as f32 / weight) + k as f32) |
There was a problem hiding this comment.
I believe you ment
1.0 / (position as f32 / weight + k as f32)
| } | ||
| ScoringQuery::Fusion(fusion) => match fusion { | ||
| FusionInternal::RrfK(_) | FusionInternal::Dbsf => Some(Order::LargeBetter), | ||
| FusionInternal::Rrf { .. } | FusionInternal::Dbsf => Some(Order::LargeBetter), |
There was a problem hiding this comment.
We should destructure for letting the compiler complain in the future
There was a problem hiding this comment.
not sure about this specific case. Usually the problem happens when we use one parameter, but not others. In this case we don't care about any parameters what so ever.
But I guess I have to follow my own requirements.
| /// Compute the RRF score for a given position with optional weight. | ||
| /// | ||
| /// The formula is: `1.0 / ((position + 1) as f32 / weight + k as f32 - 1.0)` | ||
| /// | ||
| /// With weight=1.0 (default), this becomes the standard RRF formula: `1.0 / (position + k)` | ||
| /// | ||
| /// Higher or lower weight means the positions are "compressed" or "stretched": | ||
| /// | ||
| /// weight=3.0 is equivalent to dividing the position by 3, | ||
| /// so element at pos 3 contributes like pos 1 would with weight=1.0. | ||
| /// | ||
| /// `(position + 1)` accounts for 0-based indexing, | ||
| /// so weight affects the score of the top-ranked item (pos=0) as well. | ||
| /// | ||
| /// This means a 3:1 weight ratio is equivalent to "for each 3 results of first prefetch, | ||
| /// have one result of second". | ||
| fn position_score(position: usize, k: usize, weight: f32) -> f32 { | ||
| // Avoid division by zero - if weight is 0, treat as negligible contribution | ||
| if weight <= 0.0 { | ||
| return 0.0; | ||
| } | ||
|
|
||
| 1.0 / ((position + 1) as f32 / weight + k as f32 - 1.0) |
There was a problem hiding this comment.
this formula doesn't exactly match document, but have 2 nice properties:
- if weight = 1.0, reduces to regular rrf as we had before
- if weight != 1.0, score for position=0 is affected by weight
There was a problem hiding this comment.
I wish we initially started ranks at 1 so that we don't need to do this for backward compatibility. Original function works nicely if ranks start at 1. The only inconvenience is that k gets slightly affected, compared to the original function.
Maybe docs can use the simpler formula, and stating that ranks start at 1.
The alternative would be to have the 2 different formula implementations
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/segment/src/common/reciprocal_rank_fusion.rs`:
- Around line 186-202: Update the inaccurate inline math in the
test_rrf_scoring_weighted comments to match the actual implementation used by
rrf_scoring (the formula 1/((pos+1)/weight + k - 1)), not 1/(pos*(1/weight) +
k); adjust the worked examples for Point 1 and Point 2 using DEFAULT_RRF_K and
the weights array [3.0, 1.0] so the numeric values reflect 1/((pos+1)/w + k - 1)
and clearly show why scored_points[0].score > scored_points[1].score, keeping
the existing assertions unchanged.
- Around line 16-38: The RRF formula in position_score currently uses (position
+ 1)/weight + k - 1 which breaks the documented equivalence and the test; change
the implementation in position_score to use position as f32 / weight + k as f32
for the denominator (i.e., denom = position as f32 / weight + k as f32; return
1.0 / denom), keep the existing weight<=0.0 guard, and update any comments if
needed so the code matches the docstring/tests (this touches the position_score
function).
🧹 Nitpick comments (1)
tests/openapi/helpers/helpers.py (1)
121-121: Use explicitOptionaltype annotation.The static analysis tool flags
weights: List[float] = Noneas an implicitOptional. PEP 484 requires the type to explicitly includeNone.Suggested fix
+from typing import Any, Dict, List, Optional ... def reciprocal_rank_fusion( - responses: List[List[Any]], limit: int = 10, weights: List[float] = None + responses: List[List[Any]], limit: int = 10, weights: Optional[List[float]] = None ) -> List[Any]:
| /// Compute the RRF score for a given position with optional weight. | ||
| /// | ||
| /// The formula is: `1.0 / ((position + 1) as f32 / weight + k as f32 - 1.0)` | ||
| /// | ||
| /// With weight=1.0 (default), this becomes the standard RRF formula: `1.0 / (position + k)` | ||
| /// | ||
| /// Higher or lower weight means the positions are "compressed" or "stretched": | ||
| /// | ||
| /// weight=3.0 is equivalent to dividing the position by 3, | ||
| /// so element at pos 3 contributes like pos 1 would with weight=1.0. | ||
| /// | ||
| /// `(position + 1)` accounts for 0-based indexing, | ||
| /// so weight affects the score of the top-ranked item (pos=0) as well. | ||
| /// | ||
| /// This means a 3:1 weight ratio is equivalent to "for each 3 results of first prefetch, | ||
| /// have one result of second". | ||
| fn position_score(position: usize, k: usize, weight: f32) -> f32 { | ||
| // Avoid division by zero - if weight is 0, treat as negligible contribution | ||
| if weight <= 0.0 { | ||
| return 0.0; | ||
| } | ||
|
|
||
| 1.0 / ((position + 1) as f32 / weight + k as f32 - 1.0) |
There was a problem hiding this comment.
I wish we initially started ranks at 1 so that we don't need to do this for backward compatibility. Original function works nicely if ranks start at 1. The only inconvenience is that k gets slightly affected, compared to the original function.
Maybe docs can use the simpler formula, and stating that ranks start at 1.
The alternative would be to have the 2 different formula implementations
* weighted rrf implementation * test * fmt * fix edge * validate number of sources and number of weights * do not partial match * upd schema * review fixes * update formula * remove calcualtions from tests * update comment, because AI have OCD * fmt
Based on https://www.notion.so/qdrant/Weighted-RRF-255674779d3380b19f29da1de6ad81d6
response:
Details