@alexey-milovidov @abashkeev @nikitamikhaylov, this PR is for your attention

Thanks for the contribution! This PR logically matches one of the items in our roadmap -

#104122
Apply exact row positioning optimization when rescoring with quantized vector indexes - the vector column will still be read, but rescoring distance calculation will only be executed on specific neighbour rows rather than all rows in the granule. Currently the entire optimization is disabled if rescoring is ON. Tracked here : #103745

and

#103745 (comment)

My idea was to just enable the row-positioning optimization even when rescoring is ON (and drop the _distance column). That would require a small change in MergeTreeRangeReader::fillDistanceColumnAndFilterForVectorSearch() and maybe also in useVectorSearch.cpp. As I wrote, currently we rescore with all vectors in a granule - we really only need to rescore with one (or handful) of vector rows in the granule that was returned by usearch. And this rescoring will happen naturally in the SQL pipeline in the ExpressionStep evaluation. Can you please explain why have you re-implemnented rescoring and added custom distance functions in MergeTreeRangeReader itself?

@shankar-iyer thanks, this makes sense.

I originally extended the existing _distance rewrite because it was the shortest path to proving that exact row positioning helps vector_search_with_rescoring = 1: keep the vector column, compute full-precision distance in the reader, and wire the sort expression to _distance _.

But I agree that the cleaner ClickHouse-native version is to decouple row-positioning from _distance:

• for vector_search_with_rescoring = 0, keep the existing _distance rewrite;
• for vector_search_with_rescoring = 1, propagate vector-search read hints even when _distance is not requested;
• in MergeTreeRangeReader, build/apply the row-offset filter from the hints, but fill _distance only if the read header requests it;
• leave the original L2Distance / cosineDistance / dotProduct expression in the SQL pipeline, so it is evaluated only after row-position filtering.

That should preserve the existing function semantics and avoid custom distance implementations in the reader. I’ll rework the PR in this direction.

@shankar-iyer I reworked the PR in the direction you suggested.

The reader-side fused rescoring prototype is gone now:

• no custom distance kernels in MergeTreeRangeReader;
• no VectorSearchKernel;
• no extra simsimd usage from this PR;
• no _distance rewrite for vector_search_with_rescoring = 1.

Current architecture:

• vector_search_with_rescoring = 0 keeps the existing optimized _distance path.
• vector_search_with_rescoring = 1 keeps the original SQL distance expression (L2Distance, cosineDistance, dotProduct, etc.) and keeps the vector column in the read list.
• The vector index still returns candidate row offsets.
• ReadFromMergeTree propagates vector-search read hints even when _distance is not requested.
• MergeTreeRangeReader::fillDistanceColumnAndFilterForVectorSearch now builds the exact row-position filter from those offsets.
• _distance is filled only if the read header requests it.
• The regular ExpressionStep then computes exact rescoring only for the candidate rows that survived the row-position filter.

So the PR now decouples row-positioning from _distance, while preserving the existing SQL pipeline semantics for exact rescoring.

I also kept explicit _distance readable under rescoring: if _distance is selected explicitly, the vector index is asked to return distances and the virtual column is populated as before.

Validation added/updated:

• 02354_vector_search_rescoring checks that rescoring no longer rewrites to _distance.
• The same test has a throwIf guard proving the row-position filter is applied before the downstream ExpressionStep evaluates the exact distance expression.
• 04109_vector_search_rescoring_distance_select covers:
  • distance expression in the select list under exact rescoring;
  • no _distance rewrite in EXPLAIN;
  • explicit _distance selected under rescoring.
• Existing/focused coverage also includes PREWHERE, non-Float32 element types, Float64 reference precision, cosine zero-vector behavior, and dotProduct fallback.

Local verification:

• ninja -C build_vector_pr programs/clickhouse
• focused stateless run: 8 tests passed, 0 skipped

Local benchmark, synthetic 50k rows with dim = 384, same query shape and local release build:

So for this benchmark the reworked exact-rescoring path is about 3.39x faster than the full exact scan baseline, while still doing exact SQL-pipeline rescoring.

The following is a review note from Claude (posted by @shankar-iyer on its behalf).

Thanks for iterating on this. A suggestion on framing for the next revision: the smallest change that makes rescoring use the existing row-positioning filter is a much safer starting point than a new code path. The current diff is ~530 lines, but the actual behavior delta ("apply the filter the index already produces, even when _distance is not in the read list") is about 25 lines spread over two files. Starting from the minimal diff and only adding complexity that's clearly justified would be easier to review and lower-risk to land.

Concretely, the smallest sufficient change is:

1. In LoadedMergeTreeDataPartInfoForReader::setReadHints, drop the read_columns.contains("_distance") gate so vector-search hints are stored whenever they're present.
2. In MergeTreeRangeReader::fillDistanceColumnAndFilterForVectorSearch, build the row-offset filter as today, but only allocate / populate the _distance column when read_sample_block.has("_distance"). The existing offset_to_distance map lookup stays.

That's it. No new flag, no optimizer-side plumbing, no changes to filterMarksUsingIndex, no changes to how the index is asked for distances.

A few things in the current diff that are worth dropping for risk reasons:

• The strict std::is_sorted → LOGICAL_ERROR in fillDistanceColumnAndFilterForVectorSearch is a real regression risk. The rows arriving at the range reader aren't always sorted by row offset in practice — binary quantization (vector_similarity('hnsw', …, 'b1', …)) is the case that's been observed, and the issue is upstream of this code in the multi-index-granule loop in filterMarksUsingIndex. The previous local sort + offset→distance map made the consumer immune to this. Replacing it with a hard throw turns a class of currently-working queries into thrown LOGICAL_ERRORs. Keep the defensive local sort; if and when the upstream ordering bug is fixed, the strictness can be added then.

• The switch from "local sort + offset→distance map" to "in-place sort of stored hints + positional lookup" changes the contract of RangesInDataPartReadHints::vector_search_results — consumers may now assume sorted-and-paired, whereas previously they could only assume paired in arbitrary order. That contract change is independent of the rescoring fix; if it's worth making, it deserves its own PR with its own justification. As written, it adds risk to a change that doesn't need it.

• The use_vector_search_result_filter flag in RangesInDataPartReadHints doesn't carry information that isn't already implicit in vector_search_results.has_value() — whenever vector-search hints are present, the consumer wants to apply the row filter. Adding a flag means every producer and consumer now has to be kept consistent on that flag, for no behavioral gain. Removing it would also let the matching optimizer-side helpers (enableVectorSearchRowFilter, the read_distance_column / return_distances recomputation, the extra setVectorSearchParameters call) go away.

• Unrelated changes mixed into the diff — sort_description.size() > 1 → != 1 (looks like a legitimate fix to a potential front() on an empty vector), the createLocalParallelReplicas return std::make_unique<…> → temp-variable refactor, and the return optimization_applied change in optimizeVectorSearchSecondPass. Each is defensible in isolation, but bundling them inflates the review surface and makes bisection harder if anything regresses. Better as standalone PRs.

On the benchmark: the relevant baseline is master with vector_search_with_rescoring = 1 (granule-level rescoring), not a full brute-force scan. Could the table in the PR description be re-run with that row included? The expected uplift for selective queries is in the 20–30% latency range from local measurements with the minimal patch.

And the PR title still says "Fuse vector search rescoring in MergeTree reads", which no longer reflects what the PR does — nothing is being fused. Something like "Apply exact row-positioning for vector search rescoring queries" would match, with a matching changelog entry.

As an aside, if agentic AI / coding tools are part of the workflow here, a tightly-scoped framing like "enable the existing row-positioning optimization even when vector_search_with_rescoring = 1, so the filter the index already produces is applied to rescoring queries too" tends to land close to the two-file change directly. Sharing in case it's useful — broader prompts naturally produce broader diffs.

Happy to share a sketch of the minimal patch as a separate branch if that helps as a reference point. Thanks for working on this.

Please check above comment from Claude, I agree with that.

@shankar-iyer thanks, agreed on keeping the PR smaller and on not using the earlier local benchmark framing as a master-vs-PR claim.

I updated the PR title/description and pushed a scoped revision:

• removed the reader-side fused rescoring/custom distance path from the effective diff;
• kept vector_search_with_rescoring = 1 in the normal SQL pipeline;
• use the vector-search second pass only to mark analyzed parts where exact row offsets are available;
• MergeTreeRangeReader applies the existing part-offset filter even when _distance is not requested;
• _distance is still populated only when the read header requests it.

I kept one explicit use_vector_search_result_filter signal instead of storing/applying hints unconditionally. I tested the unconditional version and it changed the explicit PREWHERE case: vector_search_results can be present for mark pruning even when row-level filtering must remain disabled. The focused stateless test now covers that distinction.

Validation:

• ninja -C build_vector_pr programs/clickhouse
• tests/clickhouse-test -b build_vector_pr/programs/clickhouse --queries tests/queries --no-stateful --no-random-settings --no-random-merge-tree-settings --no-zookeeper --no-shard --timeout 180 02354_vector_search_rescoring: 3 tests passed, 0 skipped.

I removed the old local perf claim from the PR description. The relevant performance comparison should be against current master with vector_search_with_rescoring = 1, not against a full exact-scan fallback shape.

Workflow [PR], commit [b4574a1]

Summary: ❌

• Performance Comparison (amd_release, master_head, 4/6): Performance dashboard

AI Review

Summary

This PR narrows vector_search_with_rescoring = 1 reads to vector-index candidate rows before the exact SQL distance expression runs, updates the row-hint plumbing across ReadFromMergeTree and MergeTreeRangeReader, covers multi-sub-index candidate aggregation, and updates the setting/docs text. The current diff addresses the prior multi-granule and documentation concerns, and I did not find a remaining correctness blocker.

Missing context / blind spots

• ⚠️ I did not run the focused vector-search stateless tests locally in this review; I relied on the current code/diff, prior discussion, author-provided focused runs, and the PR CI report fetched through the repository helper.

Final Verdict

Status: ✅ Approve

Stress test run with binary quantization and rescoring_multiplier = 10

[2026-05-27 01:30:57] > Start execution for [Extract truthset hackernews_openai_10m_1k.tar]
[2026-05-27 01:30:57] Run command: [tar xvf hackernews_openai_10m_1k.tar]
[2026-05-27 01:30:57] hackernews_openai_10m_1k_vectors.npy
[2026-05-27 01:30:57] hackernews_openai_10m_1k_neighbours.npy
[2026-05-27 01:30:57] hackernews_openai_10m_1k_distances.npy
[2026-05-27 01:30:57] Wed May 27 03:30:57 2026  :  Loaded truth set containing 1000 vectors
[2026-05-27 01:31:01] Wed May 27 03:31:01 2026  :  Vector indexes have loaded!
[2026-05-27 01:33:37] Wed May 27 03:33:37 2026  :  Runtime for ANN : 155.615 seconds
[2026-05-27 01:33:37] Wed May 27 03:33:37 2026  :  Calculating recall...
[2026-05-27 01:33:37] Wed May 27 03:33:37 2026  :  Recall is 0.9919400000000037
[2026-05-27 01:33:37] Wed May 27 03:33:37 2026  :  Dropping table hackernews_openai ...

Earlier run without PR -

Wed Apr 29 14:35:32 2026  :  Loaded truth set containing 1000 vectors
Wed Apr 29 14:35:36 2026  :  Vector indexes have loaded!
Wed Apr 29 14:38:52 2026  :  Runtime for ANN : 195.913 seconds
Wed Apr 29 14:38:52 2026  :  Calculating recall...
Wed Apr 29 14:38:52 2026  :  Recall is 0.9959900000000024

Optimization is cool - 195 secs -> 155 secs for 1000 queries for k=100

@shankar-iyer Thanks for running the stress test!

This is useful because it validates the intended production-like path: binary quantization with exact rescoring. The measured runtime improvement is about 20.6% wall-clock reduction, or about 25.9% higher query throughput for the 1000-query run.

The small recall difference also makes sense to me. This PR removes the previous granule spillover behavior: instead of rescoring all rows from the granules returned by the vector index, it applies the exact row-position filter first and then lets the regular SQL expression compute the full-precision distance. That should reduce unnecessary reads and distance evaluations, but it can also remove incidental extra candidates that happened to live in the same granules. For workloads that need the previous recall level, increasing the fetch/rescoring multiplier should be the right knob.

So I would frame this as a latency/read-amplification improvement, not as a recall improvement.

Thanks, this was the right simplification. I updated the PR so optimizePrewhere now skips implicit PREWHERE movement for any vector search lookup, not only when vector_search_with_rescoring is disabled. That also lets the rescoring/prewhere regression test use the default optimize_move_to_prewhere / query_plan_optimize_prewhere settings instead of per-query opt-outs.

Validation I re-ran locally:

• ninja -C build_vector_pr programs/clickhouse
• focused 02354_vector_search_rescoring_and_prewhere
• vector-search stateless sweep: 02354_vector_search_rescoring, 02354_vector_search_rescoring_distance_in_select_list, 02354_vector_search_pre_and_post_filtering, 02354_vector_search_binary_quantization, 02354_vector_search_rescoring_and_prewhere
• git diff --check

I also did a focused local perf sanity check for this guard change only. Setup: synthetic 50k rows, dim=384, vector_similarity('hnsw', 'L2Distance', ...), 100 query iterations, max_threads = 1, expression/aggregate/sort compilation disabled, and vector_search_index_fetch_multiplier = 20 so result row counts match before/after. Both plans still materialize explicit L2Distance and use the vector_similarity skip index; _distance does not appear in the valid EXPLAIN output.

I discarded a lower-multiplier selective run from perf claims because row counts diverged (20 before vs 15 after), so the table above is the apples-to-apples run. This is not meant as a production benchmark, only a regression check for the requested PREWHERE guard change.

Addressed. I removed the explicit helper setting overrides from the two vector rescoring stateless tests:

• query_plan_optimize_lazy_materialization
• query_plan_execute_functions_after_sorting
• compile_expressions
• compile_sort_description

I kept only the explicit PREWHERE on/off settings in the block that intentionally tests PREWHERE behavior.

Re-checked locally after the change:

• 02354_vector_search_rescoring
• 02354_vector_search_rescoring_distance_in_select_list
• 02354_vector_search_rescoring_and_prewhere

Result: 3 tests passed, 0 skipped. Also ran git diff --check.

Pushed follow-up fixes on 7e227e5f4d54cc982fdcc912f5413500cea5adba.

Addressed the latest review feedback:

• aggregated vector-search candidate rows across multiple vector index granules instead of keeping only the last sub-index result;
• added a small-GRANULARITY 2 stateless regression so rescoring preserves candidates from multiple vector index granules;
• added coverage for explicit _distance with and without vector_search_with_rescoring;
• updated vector_search_with_rescoring setting text and ANN docs to describe filtering to candidate rows before final rescoring when the plan allows it;
• adjusted test wording/comments per review: no exact row-positioning term in public/test output, no ExpressionStep mention in test text, and no old-state wording;
• clarified the use_vector_search_result_filter comment and the vector-search/PREWHERE optimizer comment.

Local verification on the pushed branch:

• ninja -C build_verify programs/clickhouse
• git diff --check
• targeted stateless run:
  • 02354_vector_search_rescoring
  • 02354_vector_search_rescoring_distance_in_select_list
  • 02354_vector_search_rescoring_and_prewhere

Result: 3 tests passed, 0 skipped.

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 38 queries analysed

This PR changes only the vector-similarity-search rescoring path: the query-plan optimizations that handle vector indexes (PREWHERE conflict resolution, second-pass candidate-row filtering) and the MergeTree read-hint plumbing that feeds vector distances back into the reader. ClickBench queries do not use vector-similarity indexes, so none of the 10 flagged deltas can plausibly be caused by this change. The fact that many unrelated queries all shift in the same direction by ~6-18%, with only 6 source runs against 30 base runs, points to instance/build-level variance rather than a real per-query effect. All flagged queries are downgraded to not-sure.

clickbench

⚠️ 10 inconclusive

tpch_adapted_1_official

🟢 No significant changes

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 107/116 (92.24%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

Only failure right now is in the "Performance Comparision", unrelated and is noise.

This caused conflicts with #108423 and had to be reverted in #108812. Please re-land it