MDEV-39858: Reloading COSINE metric index from disk degrades search r… - #5184
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates FVectorNode::load_from_record in sql/vector_mhnsw.cc to set vec_ptr->abs2 to 0.5f when the metric is COSINE. The reviewer pointed out that calling postprocess right before this assignment is redundant and wasteful because postprocess already computes abs2. They suggested skipping the postprocess call entirely when the metric is COSINE to avoid unnecessary computation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (ctx->metric == COSINE) | ||
| vec_ptr->abs2= 0.5f; |
There was a problem hiding this comment.
Calling vec_ptr->postprocess(ctx->vec_len) on line 885 dynamically recomputes abs2 using floating-point math (including an expensive dot product over the vector dimensions). Since this calculated value is immediately overwritten with 0.5f for the COSINE metric, the entire computation in postprocess is redundant and wasted.\n\nThis can cause a significant performance bottleneck when reloading large, high-dimensional indexes from disk.\n\nIf postprocess only computes abs2, consider skipping it entirely for the COSINE metric:\n\ncpp\n if (ctx->metric == COSINE)\n vec_ptr->abs2= 0.5f;\n else\n vec_ptr->postprocess(ctx->vec_len);\n
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Can you please think of a regression test for this?
Maybe craft a vector, then restart the server and compare pre- and after?
gkodinov
left a comment
There was a problem hiding this comment.
Can you please look at why the vector test is failing in buildbot?
There was a problem hiding this comment.
Pull request overview
This PR addresses a consistency issue in the MHNSW vector index for the COSINE metric by ensuring vectors reloaded from disk use the same fixed abs2 value as vectors created in-memory, avoiding rounding-noise drift from recomputation on quantized coordinates.
Changes:
- After loading a vector from
FIELD_VECand runningpostprocess(), forceabs2 = 0.5fwhen the index metric isCOSINE. - Aligns reload behavior (
FVectorNode::load_from_record) with in-memory vector creation behavior (FVector::create) for COSINE normalization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@gkodinov sorry for the delay. the test |
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please keep working with the final reviewer assigned.
There was a problem hiding this comment.
I kind of agree with Gemini, let's not recalculate abs2 if you don't need it. basically it means, let's remove postprocess completely there isn't much code reuse if we don't calculate abs2 unconditionally.
but it's really a trivial matter and you're doing much more important GSoC task. So feel free to ignore this review, if you'd like, and I'll later do the change and push
…ecall due to abs2 quantization noise When a vector is created in-memory using FVector::create() during normal inserts, its squared magnitude (abs2) under the COSINE metric is hardcoded to 0.5f. However, when the index is reloaded from disk (after a server restart, FLUSH TABLES, or ALTER TABLE), the index uses FVectorNode::load_from_record(). This method reads the stored scale and quantized int16 coordinates from the database record, and runs postprocess(). Inside postprocess(), abs2 is dynamically recomputed using floating-point math: abs2 = subabs2 + scale * scale * dot_product(d, d, vec_len) / 2; Because the coordinates stored on disk are quantized int16 values, this recalculation introduces rounding noise. This affects high dimensions datasets, and it is increasing as M increases. Added hardcoded abs2=0.5 to FVectorNode::load_from_record and removed postprocess()
7933350 to
2aa80e5
Compare
|
It is okay I did it quickly. If you want to change anything else feel free to do it if it's going to save time. |
When a vector is created in-memory using FVector::create() during normal inserts, its squared magnitude (abs2) under the COSINE metric is hardcoded to 0.5f.
However, when the index is reloaded from disk (after a server restart, FLUSH TABLES, or ALTER TABLE), the index uses FVectorNode::load_from_record(). This method reads the stored scale and quantized int16 coordinates from the database record, and runs postprocess(). Inside postprocess(), abs2 is dynamically recomputed using floating-point math: abs2 = subabs2 + scale * scale * dot_product(d, d, vec_len) / 2;
Because the coordinates stored on disk are quantized int16 values, this recalculation introduces rounding noise.
This affects high dimensions datasets, and it is increasing as M increases.
Added hardcoded abs2=0.5 to FVectorNode::load_from_record