[MOD-14956] Add SQ8 quantization support for HNSW index

[dor-forer]

Describe the changes in the pull request

Cherry-pick of ARM-software/VectorSimilarity-for-Arm#4 (head 125ea15d), plus a follow-up review pass. Fourth PR in the SQ8 series, after #999, #1000 and #1002.

Adds 8-bit scalar quantization (SQ8) to the standalone HNSW index:

• VecSimQuantType plus quantType / quantParams on HNSWParams. Both fields are appended at the end of the struct and VecSimQuant_NONE is 0, so existing zero-initialized and designated-initializer construction is unaffected.
• HNSWFactory can build SQ8 indexes for FLOAT32 and FLOAT16 data types with the L2 and IP metrics, wiring QuantPreprocessor and DistanceCalculatorWithNorm, and accounts for SQ8 in EstimateInitialSize and EstimateElementSize.
• For SQ8, quantParams points to a float[dim] mean vector; a null pointer selects quantization without mean normalization.
• New test_hnsw_sq8 unit-test target and suite (44 tests: FP32/FP16 x L2/IP).

SQ8 support for the tiered HNSW index, serialization and benchmarks is deferred to later PRs in the series.

Commit 1: the cherry-pick

ARM's four commits squashed into one, with no functional change to their code. Two Redis-side adjustments:

• Dropped the added SPDX-FileCopyrightText Arm line from the two modified files, matching how [MOD-14953] Add calcDistanceForQuery to IndexCalculatorInterface #999, [MOD-14952] Support normalization in QuantPreprocessor #1000 and [MOD-14955] Add DistanceCalculatorWithNorm #1002 landed. It is kept on the new tests/unit/test_hnsw_sq8.cpp, where the BSD-3-Clause identifier was replaced by this repo's Redis tri-license header.
• Wrapped that header so make check-format passes at the 100-column limit.

Commit 2: review follow-up

No behavioural change intended. Interface, single-source-of-truth and idiom fixes:

• quantParams is now const void *. Every use reads it and two already cast to const float *. Layout-identical, so not an ABI break, and callers passing non-const still compile. Better to fix before the field ships and freezes.
• The VecSimQuant_SQ8 comment claimed "with mean normalization", but mean normalization is optional and selected by quantParams. Reworded.
• GetSQ8StoredDataSize re-derived the stored blob size that QuantPreprocessor's constructors already computed. Two formulas for one layout drift silently, which is the bug class fixed in MOD-15303. The formula now lives once as sq8::storage_bytes_count<Metric, WithNorm>(dim), beside the storage_metadata_count it builds on, and both callers use it. This is why the diff touches types/sq8.h and spaces/computer/preprocessors.h, two files beyond ARM's original four: having one shared definition is the entire point of the fix.
• Restored the return NULL closing the SQ8 branch. Unreachable today, since the type and metric checks leave only FP32/FP16 x L2/IP, but without it adding a type or metric silently falls through and builds an unquantized index.
• assert(ret == 0) on addPreprocessor is now assert(ret != -1). The function returns -1 on failure, 0 when the container is full, and the next free index otherwise, so 0 is merely the only success value at the current container size of one. != -1 is the documented contract and the existing repo idiom.
• Hoisted the tail both branches duplicated (container construction, addPreprocessor, assert, IndexComponents, return); only the preprocessor and calculator differ.
• The mean vector is copied with one assign instead of a zero-filling constructor followed by memcpy, which wrote every element twice.
• Obtaining the query alignment required calling GetDistFunc for a function that is never used, since spaces.h offers no alignment-only query and the asymmetric hint covers the storage operand. That call now lives in a small GetQueryAlignment<DataType> adapter returning the hint, so the call site neither discards a value nor keeps a third distance function in scope beside sym_func and asym_func that must never be called. query_alignment is const.
• GetSQ8StoredDataSize is [[nodiscard]] constexpr and dim / with_norm are const.

Commit 3: reject quantized tiered indexes until MOD-14957

Adding quantType to HNSWParams makes it reachable on the tiered path, where nothing handles it. TieredHNSWFactory::NewIndex forwards primaryIndexParams into HNSWFactory::NewIndex, so the primary index quantizes its storage, while NewBFParams does not copy quantType and the frontend stays unquantized. Reachable from any direct C API caller with algo = VecSimAlgo_TIERED and quantType = VecSimQuant_SQ8:

• FP32/FP16: the getStoredDataSize() assert at tiered_factory.cpp:54 aborts on a debug build; under NDEBUG the index is built with mismatched frontend and backend layouts.
• FP64/BF16/INT8/UINT8: HNSWFactory::NewIndex returns NULL for these types under SQ8, and the result is reinterpret_cast and dereferenced with no null check, so the process segfaults.

The catch (...) in index_factory.cpp does not help, since neither an abort nor a null dereference is an exception. RediSearch cannot set quantType until MOD-14958, so there is no product exposure today; the guard exists so main does not carry the defect between cherry-picks. MOD-14957 should replace this check and its test rather than delete them.

Commit 4: cover SQ8 rejection of unsupported data types

HNSWSQ8ParamsTest.RejectsUnsupportedDataType asserts that FLOAT64, BFLOAT16, INT8 and UINT8 with VecSimQuant_SQ8 all return NULL from index creation. Verified red without the fix: with both the type fence and the fall-through return NULL removed, all four are silently built as unquantized indexes. Also documents at EstimateElementSize why the estimate deliberately does not repeat the check (see the Bugbot thread on this PR).

Commit 5: fix two defects found in review (63271c1)

Both raised by @lerman25, both verified before fixing.

• Out-of-bounds read through the public C API. VecSimIndex_GetDistanceFrom_Unsafe documents blob as a raw dim-by-type vector, but the SQ8 kernels read query metadata appended past that, so honouring the documented contract read past the caller's buffer. Reproduced under ASan: heap-buffer-overflow, READ of size 4 in SQ8_FP32_InnerProduct_Impl via vec_sim.cpp:231. getDistanceFrom_Unsafe now returns INVALID_SCORE for a quantized index, the value getDistanceFromInternal already uses for "no answer". AbstractIndexInitParams gains isQuantized for this, parallel to isDisk. Preprocessing internally was rejected because preprocessQuery also normalizes cosine queries, so it would change behaviour for every existing cosine index; a public prepared-query API is the real answer and belongs with MOD-14958.
• Mean-centred FP16 L2 loses correctness. The query is centred then narrowed back to FP16 while storage keeps FP32 min/delta. Verified numerically: mean 10000 gives a per-component error of 1.0 and a self-distance of 4.0 for an identical vector/query pair, and centring -40000 with mean 40000 gives -inf. Now rejected at construction. FP16 + mean + IP is unaffected and still supported, since that path does not centre the query.

The test that should have caught the first one passed a preprocessed blob obtained through a C++-only path no C caller has. It now checks the maths via calcDistanceForQuery and separately asserts the public API reports no answer for a raw vector.

Commit 6: serialization guard and IP graph coverage (9e69259)

• saveIndexIMP refuses a quantized index. The V4 format records neither quantType nor the mean, and the loading path always builds unquantized components, so a saved SQ8 index reloaded with the wrong stride and consumed graph bytes as vector data. Same argument as the tiered guard: fail closed. Caveat: the encoding version is written before this check runs, so a rejected save leaves a stub file, which still fails closed on load.
• GraphConstructionIP. Every other functional test uses L2, so the symmetric SQ8-to-SQ8 IP kernel that graph construction selects was never executed, and this series is the first thing to put it on the insert path. Its vectors vary per component, so it avoids the degenerate min == max branch the other tests take.
• Review nits: assert(false && "...") before the unreachable return NULL (kept alongside it, since assert-only would reopen the silent-fallthrough hole under NDEBUG), and the stray blank line removed.

Commit 7: make the size estimates reject what index creation rejects (d5d93d6)

Closes the three Bugbot findings that were still open, plus one gap none of them covered. No behavioural change on any supported configuration.

The supported-SQ8-combination test now lives in one place instead of being open-coded in NewIndex only: ResolveSQ8Metric applies the is_normalized Cosine-to-IP remap, and SQ8ParamsSupported holds all the fences. Same single-source-of-truth argument as sq8::storage_bytes_count in commit 2.

• NewIndex rejects a quantType that is neither NONE nor SQ8. Previously any other value fell through to the full-precision path and silently built an unquantized index for a caller that asked for a quantized one. Deliberately untested: the enum holds only those two values, so forming a third is undefined behaviour. The guard is what stops adding SQ4 from reopening the hole.
• EstimateInitialSize rejects the same set as NewIndex, replacing a check that validated only the data type. This closes the reported metric gap and one nobody raised: the mean-centred FP16 L2 fence added in 63271c14 was never mirrored into the estimate, so that combination still reported a size.
• TieredHNSWFactory::EstimateInitialSize rejects quantType != NONE, matching its NewIndex guard. It needs its own check rather than inheriting one, since the primary index accepts FP32 with L2 and SQ8 happily and nothing propagates up.

EstimateElementSize is left lax on purpose: size_t return with no sentinel, and VecSimIndex_EstimateElementSize is extern "C", so a throw would carry an exception into the C host. It answers for whatever params it is handed, exactly as VecSimParams_GetStoredDataSize always has on the unquantized path. Both functions now say which one validates and why the other cannot.

Flagged rather than buried: this adds two throw sites reachable through VecSimIndex_EstimateInitialSize, which has no try/catch. That is the existing idiom in both files rather than a new hazard, but the error model for the estimate functions is unsettled and belongs with MOD-14958.

Verified red without the fix: 5 failures, all "throws nothing". RejectsUnsupportedDataType stayed green, since the old code already threw for a bad type.

Commit 8: reject an out-of-range metric, and document the SQ8 distance sentinel (9b1615c)

The two findings from @lerman25's second review round that are inside MOD-14956.

• An out-of-range VecSimMetric aborted the process. FP32 with SQ8 and metric = (VecSimMetric)123 passed the Cosine-only check, matched neither dispatch branch, and hit the assert(false) added in 9e69259c. The unquantized dispatcher throws and VecSimIndex_New catches it, so quantized params were the only way to abort an assertions-enabled host. SQ8ParamsSupported now whitelists L2 and IP. Note commit 7's refactor preserved this hole verbatim rather than introducing it: the check it replaced also tested only for Cosine. RejectsOutOfRangeMetric uses (VecSimMetric)3, outside the valid set but inside the enum's value range, so the test does not itself rely on UB.
• The INVALID_SCORE sentinel is now documented on vec_sim.h. 63271c14 made getDistanceFrom_Unsafe return it for a quantized index, but the declaration still promised a distance, and it is the same NaN used for a missing label, so a caller could treat valid candidates as absent. The declaration now states the restriction, the collision, and that callers must check quantType.

Deferred to new tickets, all under epic MOD-6132

Findings from the second review round that fall outside MOD-14956 ("wire SQ8 into HNSWIndex + factory") and outside the HLD. Each ticket carries the reproduction and the boundary.

Ticket	Finding	Why not here
MOD-17526	Both FP32 SQ8 L2 kernels lose precision through ||x||^2 + ||y||^2 - 2*IP in FP32. Stored [100000, 100008] against query [100000, 100000] returns 0.0 where the truth is 64.0, and quantization is exact for that input. Onset around offset / spread > ~4000.	Pre-existing kernels. The identity is what makes the SQ8-to-SQ8 inner loop a uint8 dot product, so a direct-difference rewrite trades a rare wrong answer for a slowdown nobody measured. Mean normalization removes it entirely, verified to offset 1e6, so the HLD default TRAINING_THRESHOLD is the safe configuration.
MOD-17527	SQ8-to-SQ8 IP SIMD kernels overflow int32 from dim 33,027 (boundary corrected from 33,026: every non-constant quantization has one q = 0).	Pre-existing kernels, and the shared uint8 helper means a fix changes int8/uint8 behaviour too.
MOD-17528	QuantPreprocessor casts NaN to uint8 for finite input when the derived range overflows to infinity. UB on AddVector.	MOD-14952's component. Needs a contract decision, since AddVector cannot report "unquantizable".
MOD-17529	HNSWSerializer::saveIndex truncates the destination before validating, so a rejected save destroys a valid existing snapshot.	Pre-existing ordering that applies to any validation failure, not only the SQ8 guard. Correcting my own earlier claim on that thread that the stub "fails closed".
MOD-17530	BUILD_TESTS getDataByLabel copies dim * sizeof(DataType) from a shorter SQ8 blob.	Test-only, no current caller, so the ASan run here is clean. A landmine for MOD-14957 and MOD-14959.
MOD-17531	GraphConstructionIP executes the symmetric IP kernel but never validates its result (every vector quantizes to the same byte payload); metric and multi axes still uncovered.	Adding the metric axis means reworking hard-coded L2 expectations across several tests, and this is ARM's suite.

Verification

Run on the final tree, after both commits:

Check	Result
./check-format.sh	clean
g++ -Wall -Werror -fsyntax-only, with and without -DNDEBUG	clean
make build DEBUG=1	clean, no warnings
test_hnsw_sq8	43/43 passed
make unit_test DEBUG=1	2650/2650 passed
make asan	2649/2649 passed, 0 sanitizer reports

Not run: FP_64=1 variants, since this change is FP32/FP16 only.
Both new guards were confirmed red without their fix, and the ASan repro above is clean after it.

Test count moved 2653 -> 2650. Dropping FLOAT16-with-mean from the functional type set removed 11 typed tests for a combination that is now rejected, and the two new tests added 6 back. FP16 + mean + IP is left with construction coverage only, and the multi-label path and full metric parameterization remain uncovered; both are tracked in MOD-17531 rather than silently dropped.

Reviewed and deliberately left alone

• The query_alignment hint comes from the symmetric DataType dispatcher while the asymmetric kernel that consumes the query uses unaligned loads (_mm512_loadu_ps). This costs nothing: QuantPreprocessor::preprocessQuery always allocates a fresh blob via allocate_aligned, so the hint only selects that allocation's alignment. It matches the asymmetric-types contract in spaces.h.
• The SQ8 branch of EstimateInitialSize uses <float> for the index class even on the FP16 path. Verified correct with a static_assert on sizeof for both the single and multi index classes.
• mean_sum_squares accumulates in float. It is a constant additive term on the IP path only, identical for every candidate, so it cannot affect ranking, only the absolute reported distance. Left as is.
• Three raw new (allocator) calls with no RAII between them leak if a later constructor throws. preprocessors_factory.h does the same, so this is repo-wide debt rather than something this PR introduced.

Which issues this PR fixes

1. MOD-14956

Main objects this PR modified

1. HNSWFactory index creation and memory estimation
2. HNSWParams and the new VecSimQuantType public API
3. sq8::storage_bytes_count, now the single definition of the SQ8 storage layout size
4. SQ8 HNSW unit-test target and test suite

Mark if applicable

• This PR introduces API changes
• This PR introduces serialization changes

🤖 Generated with Claude Code

---

Note

Medium Risk
New public API and index construction path affect memory layout and distance semantics; guards limit tiered/serialization/unsafe-distance footguns, but SQ8 correctness and estimate/throw behavior through the C API remain areas to watch in review.

Overview
Adds SQ8 (8-bit scalar quantization) to standalone HNSW, exposed through VecSimQuantType, HNSWParams::quantType, and optional quantParams (FP32 mean vector or NULL). HNSWFactory builds quantized indexes for FLOAT32/FLOAT16 with L2/IP, wiring QuantPreprocessor and SQ8 distance calculators; unsupported combinations fail closed (NULL from NewIndex, matching EstimateInitialSize throws).

Storage layout is centralized in sq8::storage_bytes_count(); QuantPreprocessor and element-size estimation use it. Indexes set isQuantized on AbstractIndexInitParams / VecSimIndexAbstract.

Safety / scope limits: VecSimIndex_GetDistanceFrom_Unsafe and HNSW getDistanceFrom_Unsafe return INVALID_SCORE on quantized indexes (raw blobs are not valid query blobs). Save/load refuses quantized indexes until serialization supports them. Tiered HNSW rejects quantType != NONE so primary/frontend blob layouts cannot diverge. Mean-centered FP16 + L2 is rejected at construction.

New test_hnsw_sq8 target covers creation, search, sizing, parameter rejection, serialization refusal, and tiered rejection.

^{Reviewed by Cursor Bugbot for commit ac05e54. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

❌ Patch coverage is 96.29630% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.14%. Comparing base (efd63da) to head (ac05e54).

Files with missing lines	Patch %	Lines
src/VecSim/index_factories/hnsw_factory.cpp	96.73%	3 Missing ⚠️
src/VecSim/algorithms/hnsw/hnsw_multi.h	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
- Coverage   97.17%   97.14%   -0.04%     
==========================================
  Files         141      141              
  Lines        8328     8428     +100     
==========================================
+ Hits         8093     8187      +94     
- Misses        235      241       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lerman25]

I lack context for this,
Left some comments, some are AI that seem reasonable
Also there are other AI comments if you can address them

[lerman25]

Suggested change

Nit

[dor-forer]

Will take it, thanks. Cosmetic only, so I have grouped it with the assert suggestion below rather than pushing a commit for a blank line on its own.

[dor-forer]

Done in 9e69259.

[lerman25]

Maybe assert false here ?

[dor-forer]

Good call, and it matches repo precedent (svs_factory.cpp:89 and friends use assert(false && "...") for unreachable type/metric combinations).

I would like to do both rather than swap one for the other:

assert(false && "unhandled SQ8 type/metric combination");
return NULL;

The assert makes a debug build shout if a future type or metric reaches here, which is what you are after. Keeping the return NULL means a release build still fails closed instead of falling through and silently building an unquantized index, which is the regression that line exists to prevent (it was missing until 4d09236). Assert-only would restore exactly that hole under NDEBUG.

Shout if you would rather have the assert alone and I will drop the return.

[dor-forer]

Done in 9e69259, kept alongside the return NULL as described above.

[lerman25]

I rechecked this after the assertion was added, and this location is reachable today: the SQ8 branch rejects Cosine but does not reject other out-of-range VecSimMetric values. A public C caller using FP32/SQ8 with metric = (VecSimMetric)123 reaches this assertion and aborts an assertions-enabled process; I reproduced it with a death test on the current head. Before this change the trailing return NULL handled that path, while the unquantized dispatcher throws and is caught by VecSimIndex_New. Please explicitly reject anything other than L2/IP before dispatch, or remove the assertion, and add an invalid-enum regression.

[dor-forer]

Confirmed and fixed in 9b1615c. You are right that the assert made this reachable, and right about the asymmetry: the unquantized dispatcher throws and VecSimIndex_New catches it, so quantized params were the only way to abort the host.

Worth being precise about what happened, since it was my change either way. The refactor in d5d93d6 pulled the fences into a single SQ8ParamsSupported predicate, and that predicate tested resolved_metric == VecSimMetric_Cosine, exactly like the open-coded check it replaced. So the hole was preserved verbatim rather than introduced, and my "unreachable today" comment on the assert was wrong for any metric outside the enum.

SQ8ParamsSupported now whitelists L2 and IP instead:

if (resolved_metric != VecSimMetric_L2 && resolved_metric != VecSimMetric_IP) {
    return false;
}

So NewIndex returns NULL and EstimateInitialSize throws, which is what every other unsupported combination does, and the assert keeps its job of catching a new type or metric that the dispatch forgot rather than caller garbage.

HNSWSQ8ParamsTest.RejectsOutOfRangeMetric pins it. One deliberate difference from your repro: it uses (VecSimMetric)3 rather than 123. VecSimMetric has three enumerators, so its value range is 0 to 3, and 3 is the smallest value outside the valid set that is still inside that range. Forming 123 is itself undefined behaviour, and I would rather the regression not depend on UB to demonstrate a UB fix. Same code path, same result.

[lerman25]

This test bypasses the public VecSimIndex_GetDistanceFrom_Unsafe contract by manually constructing an internal processed query. The API documents blob as a raw type×dimension vector, and C callers have no preprocessing API. For FP32 dim=4 L2, a valid raw query is 16 bytes, but the SQ8 kernel reads the appended y_sum and y_sum_squares at bytes 16–23, causing out-of-bounds reads. Please test this API with query directly and either preprocess internally, expose a public reusable prepared-query context, or reject direct-distance lookups for SQ8.

[dor-forer]

Confirmed and fixed in 63271c1. You were right, and it is worse than a contract mismatch: it is an out-of-bounds read reachable from the public C API. I reproduced it under AddressSanitizer with a dim=4 FP32 L2 SQ8 index and a correctly sized 16-byte heap query:

ERROR: AddressSanitizer: heap-buffer-overflow, READ of size 4
  #0 SQ8_FP32_InnerProduct_Impl            IP.cpp:65
  #6 VecSimIndex_GetDistanceFrom_Unsafe    vec_sim.cpp:231

Your diagnosis of why the suite missed it is also exactly right: the test obtained a preprocessed blob through CastToHNSW()->preprocessQuery(...), a C++-only path no C caller has.

getDistanceFrom_Unsafe now returns INVALID_SCORE for a quantized index, which is already the value getDistanceFromInternal uses for "no answer", so it needs no new error channel. I considered your first suggestion, preprocessing internally, and did not take it here: preprocessQuery also normalizes cosine queries, so applying it would change behaviour for every existing cosine index, and it would add a per-call allocation on RediSearch's scoring path. Your third suggestion, a public reusable prepared-query context, is the right long-term answer and belongs with MOD-14958, which is what first exposes any of this to the host.

The test now checks the distance maths through calcDistanceForQuery and separately asserts the public API reports no answer for a raw vector, so the raw-blob call is exercised under ASan by every type parameter.

[lerman25]

The out-of-bounds read is fixed by the rejection path. One API-contract detail remains: vec_sim.h still promises that this function returns the distance for a matching type×dimension blob, while an SQ8 index now returns INVALID_SCORE for every label—the same NaN used for a missing label. A standalone caller cannot distinguish “SQ8 operation unsupported” from “label absent.” If the prepared-query API is intentionally deferred, please document this SQ8 restriction and sentinel behavior on the public declaration so callers and MOD-14958 do not treat valid candidates as missing.

[dor-forer]

Fair, and fixed in 9b1615c. The sentinel collision is a real trap: INVALID_SCORE is the same NaN getDistanceFromInternal returns for a missing label, so a standalone caller cannot tell "SQ8 unsupported" from "label absent" and could drop valid candidates as missing. That is exactly the failure mode MOD-14958 would hit.

vec_sim.h now documents it on the declaration rather than leaving the old promise standing:

 * NOT SUPPORTED for a quantized index (HNSWParams::quantType != VecSimQuant_NONE), which always
 * returns INVALID_SCORE here, for every label. The quantized kernels read query metadata appended
 * past the raw vector, which a blob matching the documented type and dimension does not carry, so
 * honouring the contract above would read past the caller's buffer. Note that INVALID_SCORE is the
 * same NaN returned when the label is absent, so a caller cannot distinguish the two: check
 * quantType rather than inferring it from the result. Obtaining a real distance needs a prepared
 * query, which has no public API yet.

The prepared-query API is still the real answer and still belongs with MOD-14958, but a caller reading the header now finds out before writing the bug rather than after.

[lerman25]

Blocking: FP16 + mean + L2 loses correctness through this instantiation. QuantPreprocessor<float16, L2, true>::preprocessQuery computes input[i] - mean[i] in FP32, then narrows it back into the FP16 query body, while storage quantization keeps its centered min/delta in FP32. Identical vector/query pairs can therefore diverge: for x = y = [1,1,1,1] and mean [10000,...], storage represents -9999 but the query rounds to -10000, yielding self-distance 4. A valid FP16 query -40000 with mean 40000 also overflows after centering. Please keep mean-centered FP16 L2 queries in FP32 with a matching asymmetric kernel, or reject/validate this combination, and add a regression.

[dor-forer]

Confirmed and fixed in 63271c1. I reproduced your numbers exactly using the repo's own conversions before changing anything:

x = 1, mean = 10000
centred storage (fp32) = -9999.0
centred query   (fp16) = -10000.0    -> per-component error 1.0
L2^2 for an identical vector/query pair at dim=4 = 4.0

centring -40000 with mean 40000 = -80000 -> fp16 -inf

One qualifier worth recording: at a realistic mean near 1 the error is exactly zero, so this only bites for large mean magnitudes. It is silent when it does, though, so it still needs handling.

HNSWFactory::NewIndex now rejects FLOAT16 + mean + L2. Note the scope is narrower than the comment implies: only the WithNorm && L2 branch centres the query, so FP16 + mean + IP is unaffected and stays supported. RejectsMeanCenteredFP16L2 pins both halves of that.

Your preferred fix, keeping the centred query in FP32 with a matching asymmetric kernel, is the correct one but it is an ARM design change plus new kernel work, so I have left it for their series rather than doing it in a cherry-pick. FLOAT16-with-mean also leaves the functional type set, since every functional test uses L2; that trades 11 typed tests for correctness, and none of them were exercising a combination that still works.

[lerman25]

Blocking: the newly selected symmetric SQ8 kernel can overflow for valid large dimensions. On AVX512 VNNI, SQ8_SQ8_InnerProductImp receives an int from UINT8_InnerProductImp, whose horizontal reduction is signed 32-bit. A dimension-33027 vector that quantizes one component to 0 and 33026 components to 255 has self-dot 65025 * 33026 = 2147515650, exceeding INT_MAX. The wrapped value feeds both IP and L2 graph construction/pruning, so HNSW can be built with incorrect distances. Please use a wide/chunked accumulation or select a safe fallback above the overflow boundary, and add a boundary regression.

[dor-forer]

The mechanism is real and I verified it, but I would like to take it as a separate ticket rather than in this cherry-pick. Two corrections to the scope first, both of which make it worth its own change:

It is not AVX512-only. All four SIMD SQ8-to-SQ8 kernels route through UINT8_InnerProductImp and inherit the int accumulator: IP_AVX512F_BW_VL_VNNI_SQ8_SQ8.h, IP_NEON_SQ8_SQ8.h, IP_NEON_DOTPROD_SQ8_SQ8.h and IP_SVE_SQ8_SQ8.h. The scalar fallback in IP.cpp:150 accumulates into a float and does not wrap, so the exposure is exactly the SIMD paths, on x86 and ARM alike.

The threshold is confirmed: the per-element product caps at 255*255 = 65025, and _mm512_reduce_add_epi32 returns int, so the sum exceeds INT_MAX from dim = 33026 (your 33026 case overshoots by 32,003). I found no dimension cap anywhere in VecSim, so it is reachable in principle from a direct C API caller.

My reasoning for separating it: the kernels are pre-existing (they landed with the SQ8 distance-function PRs and are already exercised by test_spaces and the benchmarks), this PR is only the first thing to select them for storage-to-storage comparisons, and the trigger needs a dimension over 33025 plus data that quantizes almost every component to 255. Fixing it properly means widening or chunking the accumulation in the shared UINT8 helper, which changes int8/uint8 index behaviour too and deserves its own boundary regression rather than riding along here.

I will open a ticket against the kernels with the above. Say the word if you would rather it block this PR and I will pull it in.

[lerman25]

I rechecked the exact boundary while re-reviewing the latest head. The original comment's first realizable self-dot overflow at dimension 33,027 is correct. Every non-constant per-vector quantization has at least one q=0, so at dimension 33,026 the maximum self-dot is 33,025 * 65,025 = 2,147,450,625, which still fits INT_MAX; dimension 33,027 permits 33,026 * 65,025 = 2,147,515,650, exceeding it by 32,003. The broader cross-platform overflow conclusion remains unchanged.

[dor-forer]

Accepted, your boundary is right and mine was off by one dimension. The reasoning is the part I had missed: the minimum of every non-constant vector maps to q = 0, so at least one component contributes nothing to the self-dot. At dim 33,026 the largest achievable value is 33,025 * 65,025 = 2,147,450,625, which still fits INT_MAX; 33,027 permits 33,026 * 65,025 = 2,147,515,650, over by 32,003.

Corrected in the carry-forward doc, which had propagated 33,026 from my earlier reply, and MOD-17527 carries the corrected boundary along with the four affected SIMD kernels, the float-accumulating scalar fallback, and the note that the shared uint8 helper means a fix changes int8 and uint8 index behaviour too and needs its own boundary regression.

[lerman25]

Still blocking. The implementation may predate this PR, but this PR is what selects the symmetric SQ8 kernel for HNSW graph construction. Dimension 33,027 is accepted by the public API and deterministically produces wrapped distances, so MOD-17527 is useful tracking but does not make the new path safe to merge. Please widen or chunk the accumulator, select a safe fallback, or reject SQ8 above a proven safe dimension before enabling it here.

[lerman25]

The existing test serializer now silently accepts a layout its loader cannot decode. V4 records type, dim, and metric, but not quantType or the mean, and loading always constructs unquantized components. For example, FP32/L2 at dim 128 writes a 144-byte SQ8 blob per vector, while the loader expects 512 bytes of FP32 data and consumes following graph bytes as vector data. If SQ8 serialization is intentionally deferred, please make saveIndex() reject SQ8 and test that failure so it cannot emit a corrupt/unloadable file.

[dor-forer]

Confirmed structurally, and I agree with your proposed fix. quantType appears nowhere in hnsw_serializer.h, and the file-loading path in HNSWFactory::NewIndex always builds components through CreateIndexComponents, which has no SQ8 branch at all, so a saved SQ8 index reloads as an unquantized one with the wrong stride.

This is the same shape as the tiered hazard: a combination that is not wired yet but is silently accepted. The guard belongs in this PR by the same argument, and the isQuantized flag added in 63271c1 for the distance-API fix is what saveIndex would test.

I have not done it in this round because the requested scope was the two blocking findings. It is a small follow-up: reject SQ8 in saveIndex and test that it fails rather than emitting a file the loader cannot decode. Happy to add it here if you want it before merge.

[dor-forer]

Done in 9e69259. saveIndexIMP now throws for a quantized index, covered by HNSWSQ8Test.RejectsSerialization across all three type parameters.

One wart to flag rather than hide: HNSWSerializer::saveIndex writes the encoding version before calling saveIndexIMP, so a rejected save leaves a stub file behind. That still fails closed on load, unlike a complete file whose layout the loader misreads, but validating before the file is created would need a new virtual hook on the serializer base across four files, which felt disproportionate for a path that cannot be reached from RediSearch yet. Noted in the carry-forward doc so whoever adds real SQ8 serialization moves the check earlier.

[lerman25]

Re-verified on 9e69259: the consequence is stronger than merely leaving a new stub. HNSWSerializer::saveIndex opens location with truncation and writes V4 before this guard runs. I pre-created the destination with sentinel contents, invoked the rejected SQ8 save, and after the exception the sentinel had been replaced by exactly the four-byte version field. Thus an unsupported save can destroy a valid existing snapshot. Please validate before opening/truncating (or save atomically through a temporary file), and make the regression assert that an existing destination remains byte-for-byte unchanged.

[dor-forer]

You are right and my previous reply on this thread was wrong. I called the leftover stub "fails closed on load" and treated moving the check earlier as a nicety. It truncates a valid snapshot, which is data loss, not a cosmetic wart. Corrected in the carry-forward doc as well, since that is where I recorded the mistaken version.

The ordering is as you describe: saveIndex opens location with truncation and writes V4 before saveIndexIMP runs any validation, so the guard added in 9e69259 fires only after the previous file is already gone.

One thing worth adding, because it changes who owns the fix: this is not specific to the SQ8 guard, and not caused by it. Truncate-before-validate applies to any validation failure inside saveIndexIMP. Without the guard an SQ8 save writes a complete file whose layout the loader misreads, which also overwrites the previous snapshot. So SQ8 made a pre-existing hazard observable rather than creating it.

Tracked as MOD-17529, which had no owning ticket before: HLD section 3.3.6 puts feature persistence in RediSearch RDB (MOD-14958) and says nothing about VecSim's own serializer. The ticket takes your requirement verbatim, that the regression assert the destination is byte-for-byte unchanged rather than merely that the save threw, and prefers your atomic-temp-file option over validate-before-open, since it fixes the whole class instead of one check.

[lerman25]

Still blocking. The SQ8 serialization guard added here deterministically reaches this pre-existing truncate-before-validate ordering, so an unsupported operation introduced by this PR can destroy an existing snapshot. Tracking the general serializer problem in MOD-17529 is useful, but this PR still needs to validate before opening the destination or save atomically, with a preservation regression.

[lerman25]

This type set varies source type and mean presence, but not metric or multi. As a result, the graph/search/range/batch/override tests all exercise the default L2 single-index path; IP only appears in the one-vector direct-distance test. That leaves the new symmetric IP kernel used during HNSW graph construction and the multi-label path untested. Please parameterize the functional suite over metric and multi, and use non-constant vectors so those paths are meaningfully exercised.

[dor-forer]

Accurate, and sharper than the note I had written for myself. Confirming the specifics: the type set varies only source type and mean presence, every functional test takes the default L2 path, and IP appears solely in the one-vector distance test, so the symmetric SQ8-to-SQ8 IP kernel used during graph construction is genuinely unexercised. Your point about constant vectors is right too: GenerateVector defaults to step = 0, so most tests build all-equal components and take the degenerate min == max quantization branch.

Two notes on the current state. 63271c1 removed FLOAT16-with-mean from that type set, because every functional test uses L2 and mean-centred FP16 L2 is now rejected, so FP16 + mean + IP is currently left with construction coverage only. That makes the metric axis you are asking for more valuable, not less.

I have not parameterized over metric and multi in this round, since the requested scope was the two blocking findings and this is ARM's suite. It is the right next step and I would rather do it deliberately than bolt it on: the L2 expectations in several tests are hard-coded, so adding the metric axis means reworking the expected values, not just widening the type list. Tell me if you want that in this PR or tracked for the series.

[dor-forer]

Partly addressed in 9e69259, and you were right that it mattered.

GraphConstructionIP builds a 100-vector dim-16 IP index and searches it, so the symmetric SQ8-to-SQ8 IP kernel that graph construction selects now actually runs. Its vectors also vary per component rather than only per label, so it does not take the degenerate min == max branch that the existing tests all hit.

Worth reporting how that went, since it supports your point: my first version asserted that querying with a copy of an inserted vector would return that label first. It failed, returning 99 instead of 70, because this is plain inner product rather than cosine: the distance is 1 - IP, so the winner is the vector with the largest projection, not the query's own twin. The expectation now derives from the metric instead of from assumed self-similarity. An untested kernel plus an untested assumption is exactly the gap you were pointing at.

Not done: full parameterization over metric and multi. Several existing tests hard-code L2 expectations, so that axis means reworking their expected values rather than widening the type list, and it is ARM's suite. I would rather track it for the series than half-do it here. The multi-label path is still uncovered.

[lerman25]

I re-verified the follow-up test. It is useful execution coverage, but it still does not validate the symmetric IP kernel's result. Here every vector is x[i,j] = 0.5*i + 0.25*j; per-vector SQ8 therefore gives the same byte payload for every label (q[j] = 17*j), with only metadata changing. The callback discards the score, and efRuntime = n lets the final asymmetric query path effectively rank the whole connected graph, so wrong symmetric scores/neighbor choices can still pass. Please add a direct stored↔stored reference comparison across differing quantized patterns (and ideally a non-exhaustive graph-routing assertion); multi also remains uncovered.

[dor-forer]

Accepted, and this is a sharper reading of my own test than I had. All three points hold:

• Every vector is x[i][j] = 0.5*i + 0.25*j, so per-vector SQ8 gives the same byte payload for every label, q[j] = 17*j, with only metadata differing. I fixed the degenerate min == max branch the other tests hit and replaced it with a different degeneracy.
• The callback discards the score and checks label order only.
• efRuntime = n lets the asymmetric path effectively rank the whole connected graph, so wrong symmetric scores or neighbour choices still produce the expected order.

So the test proves the symmetric kernel executes, which nothing did before, and proves nothing about its result. That is worth less than I claimed.

Tracked as MOD-17531 with all three gaps, plus the metric and multi axes, plus the FP16-with-mean coverage that left the functional type set when mean-centred FP16 L2 was rejected. It records what you asked for: a direct stored-to-stored reference comparison across differing quantized patterns, and a non-exhaustive graph-routing assertion.

Not pulling it into this PR for the reason I gave last round, which you have not disputed: the metric axis means reworking hard-coded L2 expected values across several tests rather than widening a type list, and this is ARM's suite. Half-doing it here would produce exactly the kind of test that passes without checking anything, which is the thing you are objecting to.

[lerman25]

Split verdict: the broad metric x multi parameterization and FP16-with-mean IP coverage are reasonable to defer to MOD-17531. Directly validating the symmetric stored-to-stored kernel is blocking here, though: MOD-14956 promises insert correctness, and GraphConstructionIP currently proves only that the kernel executes. Please add a reference comparison using differing quantized byte patterns; broader suite expansion can remain in the ticket.

[lerman25]

Re-verified against 9e69259c6adc6f5e3118459c2fe565c3490af517 with focused runtime reproducers.

[lerman25]

Blocking: selecting these FP32 SQ8 distance functions exposes catastrophic cancellation in both L2 kernels. On this exact head I ran the actual QuantPreprocessor with stored x = [100000, 100008] and y = [100000, 100000]. Quantization represents x exactly (q = [0, 255], delta = 8/255), so reconstructed L2² is 64; SQ8_FP32_L2Sqr returns 0 and SQ8_SQ8_L2Sqr returns 4096. Both use ||x||² + ||y||² - 2*IP in FP32, so nearby high-offset vectors can be mis-ranked during both query evaluation and graph construction. Please use a numerically stable direct-difference/centred formulation (or sufficient precision) and add this regression.

[dor-forer]

Confirmed, and I reproduced it independently before deciding anything. Pure FP32 arithmetic, your exact input:

x = [100000, 100008] (stored, SQ8)   y = [100000, 100000] (query, FP32)
q = [0, 255], delta = 8/255 -> reconstructs to exactly [100000, 100008]
x_sum_sq = 20001599488   (true 20001600064, off by 576)
ip       = 20000800768   (true 20000800000, off by 768)
kernel L2^2 = 0.0        true L2^2 = 64.0

Your framing is right and worth restating because it is the part that makes this real: quantization is exact here, so this is the summation form and not the 8 bits. Drop the quantizer entirely and ||x||^2 + ||y||^2 - 2*IP in FP32 still returns 0. One FP32 step at 4e10 is about 4768, roughly 75x the true answer.

I also measured the onset, which sharpens the exposure:

common offset (spread 8)	no mean vector	with mean vector
0 to 1e4	correct (64)	correct
1e5, 1e6	0 instead of 64	correct (64)

Onset is around offset / spread > ~4000, where the ratio exceeds the FP32 mantissa. Mean normalization removes it entirely, verified to offset 1e6, because centring shrinks exactly the magnitudes the identity is unstable on. So the exposure is SQ8 with TRAINING_THRESHOLD = 0 on data with a large DC offset, and the HLD default of 10 * DEFAULT_BLOCK_SIZE is the safe configuration.

Tracked as MOD-17526 rather than fixed here, with the repro, the boundary table and both fix options in the ticket. My reasoning: the kernels are pre-existing, MOD-14956 is scoped to wiring them into the factory, and the HLD does not specify their numerics. More to the point, the identity is what reduces the SQ8-to-SQ8 inner loop to a uint8 dot product, which is the entire reason VNNI and NEON DOTPROD help. Your preferred fix, a direct-difference formulation, gives that up, and I am not willing to trade a rare wrong answer for a permanent slowdown nobody has measured inside a cherry-pick. The ticket flags a cheaper candidate to measure first: widen the L2 metadata and the final subtraction to FP64, which fixes this case because the cancellation is in that subtraction.

Where I think you have the stronger argument, and I want it on the record rather than buried: unlike the int32 overflow, this needs no absurd dimension, just a plausible data distribution. So I am not deferring it on "unreachable", only on "the fix is a kernel design change that needs ARM's benchmark numbers". Say the word and I will pull it in.

[lerman25]

Still blocking. Null quantParams is an explicitly supported configuration in this PR, and this PR is what wires the inaccurate kernels into both query evaluation and graph construction. The need to benchmark alternative kernel formulations affects which fix to choose, not whether known-wrong distances can ship. Please fix the calculation or reject the affected no-mean SQ8 L2 configuration; MOD-17526 can retain broader optimization work.

[lerman25]

Blocking: the compact storage enabled here also reaches the BUILD_TESTS getDataByLabel helpers, which still copy dim * sizeof(DataType) from the stored pointer. I reproduced this on the current head with FP32/L2, dim=128, blockSize=1: the SQ8 data is 144 bytes, and ASan reports a 512-byte read past the resulting 160-byte element allocation at hnsw_single.h:55. hnsw_multi.h:80 has the same copy. Since this flag is now available, please either reject that helper for quantized indexes or safely dequantize into DataType, with single and multi regressions.

[dor-forer]

Confirmed. Both call sites do exactly what you describe:

memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType));

hnsw_single.h:55 and hnsw_multi.h:80, and for a quantized index the stored blob is one byte per component plus 16 to 20 bytes of metadata, so dim * sizeof(DataType) overshoots by design. Your dim=128 FP32 numbers line up: 144 bytes stored, 512 read.

Tracked as MOD-17530 rather than fixed here. Two reasons, and one caveat against myself.

MOD-14956 is scoped to the factory path, the calcDistanceForQuery call sites and the new HNSWParams fields; these are BUILD_TESTS helpers that no product path reaches, and no current test calls them on a quantized index, which is why the ASan run on this PR is clean. So there is no exposure to close today.

The caveat: this is a landmine for the next tickets in the epic. MOD-14957 and MOD-14959 will naturally call getDataByLabel on a quantized index and get a heap overread instead of a clear failure. The ticket says so, and prefers dequantizing into DataType over rejecting, since recovering vectors for comparison is exactly what these helpers exist for. If you would rather have the guard in this PR so main never carries it, that is a four-line change plus the two regressions you asked for and I will add it.

[lerman25]

Okay to defer to MOD-17530. I rechecked the scope: both affected helpers are compiled only under BUILD_TESTS, no product path reaches them, and this PR does not call them for a quantized index. The ticket should remain a prerequisite for tests in MOD-14957 or MOD-14959, but I do not consider it merge-blocking for this PR.

[lerman25]

Blocking: this preprocessor accepts finite FP32 vectors whose scale arithmetic becomes non-finite. On this head, processing [-FLT_MAX, +FLT_MAX] makes max-min and delta infinite, inv_delta zero, and then Inf * 0 NaN; UBSan reports preprocessors.h:326:63: -nan is outside the range of representable values of type 'unsigned char'. Thus AddVector executes C++ undefined behavior for finite input. Please reject non-finite derived ranges/metadata or compute the range in wider precision and explicitly validate/clamp before conversion, with a sanitizer regression.

[dor-forer]

Confirmed from the code. preprocessors.h:273-275:

const MetadataType diff = (max_val - min_val);
const MetadataType delta = (diff == 0.0f) ? MetadataType{1} : diff / MetadataType{255};
const MetadataType inv_delta = MetadataType{1} / delta;

For [-FLT_MAX, +FLT_MAX] the subtraction itself overflows: diff = inf, delta = inf, inv_delta = 0, then (x - min_val) * inv_delta is inf * 0 = NaN, and the static_cast<uint8_t> at line 326 is undefined behaviour. The diff == 0 guard covers the degenerate equal-values case but not overflow of the range, which is the gap you found.

Worth stating explicitly since it is what makes this more than a hardening nit: every input value is finite and within the type. Only the derived range is not. So AddVector executes UB for input the API accepts as valid.

Tracked as MOD-17528 rather than fixed here. QuantPreprocessor is MOD-14952's component, already merged in #1000, and MOD-14956 only wires it into the factory. The HLD covers two quantization edge cases in section 5.3, zero variance and zero-magnitude cosine vectors, but not a non-finite derived range, so this needs a decision rather than a patch: there is no way for AddVector to report "unquantizable", so the fix is probably compute-in-wider-precision or clamp, not reject, and that choice belongs with the preprocessor owner. The ticket carries your UBSan output and asks for both regressions, including one where the range overflows without either endpoint being extreme.

[lerman25]

Still blocking. This PR is the first change that makes QuantPreprocessor reachable through public AddVector, so finite input now reaches C++ undefined behavior in a supported SQ8 index. The design and performance tradeoff may require more work, but that does not make the public UB safe to defer. Please reformulate or widen the arithmetic, or otherwise prevent the unsafe configuration from being exposed here, with the sanitizer regression in MOD-17528.

[cursor]

L2 SQ8 catastrophic cancellation

High Severity

Wiring FP32 SQ8 L2 into HNSW selects kernels that compute L2² as ||x||² + ||y||² - 2*IP in FP32. For nearby high-offset vectors that cancel, SQ8_FP32_L2Sqr and SQ8_SQ8_L2Sqr return badly wrong distances, so graph construction and search can mis-rank candidates.

 

^{Reviewed by Cursor Bugbot for commit d5d93d6. Configure here.}

[dor-forer]

Confirmed, and independently reproduced: stored [100000, 100008] against query [100000, 100000] returns 0.0 where the truth is 64.0, with quantization exact for that input, so this is the summation form and not the 8 bits. SQ8_SQ8_L2Sqr returns 4096 for the same pair.

Same finding @lerman25 raised on this PR; the full analysis, including the measured onset around offset / spread > ~4000 and the fact that mean normalization removes it entirely, is in that thread and in MOD-17526.

Deferred rather than fixed here: the kernels are pre-existing, MOD-14956 is scoped to wiring them into the factory, and the identity is what makes the SQ8-to-SQ8 inner loop a uint8 dot product that VNNI and NEON DOTPROD accelerate, so replacing it needs benchmark numbers this cherry-pick cannot produce.

[cursor]

getDataByLabel SQ8 over-read

High Severity

getDataByLabel still copies dim * sizeof(DataType) from the stored pointer. SQ8 blobs are much smaller (bytes plus metadata), so under BUILD_TESTS this reads past the element allocation. The public distance path was guarded for quantization, but these helpers were not.

Additional Locations (1)

• src/VecSim/algorithms/hnsw/hnsw_multi.h#L70-L82

 

^{Reviewed by Cursor Bugbot for commit d5d93d6. Configure here.}

[dor-forer]

Confirmed. Both getDataByLabel overloads copy dim * sizeof(DataType) from stored data, which overshoots an SQ8 blob of one byte per component plus 16 to 20 bytes of metadata. Same finding @lerman25 raised, with an ASan repro at FP32 dim=128: 144 bytes stored, 512 read.

Tracked as MOD-17530. Both helpers are behind BUILD_TESTS and no current test calls them on a quantized index, which is why the ASan run on this PR is clean, so there is nothing to close today; the ticket exists because MOD-14957 and MOD-14959 will call them and deserve a clear failure rather than a heap overread.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 4 total unresolved issues (including 3 from previous reviews).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9b1615c. Configure here.}

[cursor]

Quantization hits UB on finite input

High Severity

quantize derives delta and inv_delta from max - min in FP32 with no finiteness check. Finite inputs such as [-FLT_MAX, +FLT_MAX] make the range and delta infinite, inv_delta zero, then Inf * 0 yields NaN; casting that NaN to uint8_t is undefined behavior. AddVector on an SQ8 index therefore executes C++ UB for otherwise valid finite vectors.

 

^{Reviewed by Cursor Bugbot for commit 9b1615c. Configure here.}

[dor-forer]

Confirmed, same defect @lerman25 raised on this PR, tracked as MOD-17528. Mechanism verified from preprocessors.h:273-275: diff = inf, delta = inf, inv_delta = 0, then inf * 0 is NaN and the static_cast<uint8_t> is UB.

Two things worth adding to the record, because they cut in opposite directions.

Against deferring, and I will not lean on "pre-existing" here: QuantPreprocessor merged with MOD-14952 in #1000, but before this PR nothing constructed an index that used it, so it was reachable only from test_components. This PR is what first puts it on AddVector. So while the buggy code is not new, its reachability from the public API is.

For deferring: the fix is not a finiteness check away, which is why it needs the preprocessor owner rather than a cherry-pick. I measured the obvious repair:

current, all FP32:            diff=inf  delta=inf  inv_delta=0   -> value = nan
widen diff to double only:    diff=6.8e38  delta=2.67e36 (finite, fits FP32)
                              but per-element (x - min_val) in FP32 is still inf
                              -> value = inf, and casting inf to uint8_t is equally UB
widen the per-element
subtraction to double too:     -> value = 255.0, correct

So the range is not the only thing that overflows: the per-element (x - min_val) does too. Getting this right means either double arithmetic per element on the insert path, which is a throughput decision on the hot path, or a reformulation such as x * inv_delta - min_val * inv_delta. Either way it is a design call with a benchmark attached, and AddVector has no way to report "unquantizable" if the answer turns out to be rejection.

The ticket carries both the UBSan output and this table, and asks for a regression at [-FLT_MAX, +FLT_MAX] plus one where the range overflows without either endpoint being extreme.

[lerman25]

Blocking: SQ8 L2 mixes metadata from the original stored vector with a cross term computed from its quantized reconstruction, so it can return a negative distance even at small magnitudes. On ac05e54 through the public C API, storing [0, 0.25, 1] and querying [0, 0.2501, 1] makes a radius-0 query return the label with score -0.000490427. This is distinct from MOD-17526: there is no large offset, and the failure comes from the norm and cross term describing different vectors. Please make all terms use the same representation, or use a stable direct-difference formulation, and add this non-grid radius-zero regression.

[lerman25]

Blocking: this new throw escapes through the public extern C VecSimIndex_EstimateInitialSize wrapper, which has no catch or status channel. I compiled an actual C caller against this head; FP32 plus SQ8 plus Cosine terminates with exit 134 after an uncaught std::invalid_argument. The new EXPECT_THROW tests call the C wrapper from C++ and therefore encode behavior that a C host cannot handle. Please keep the C API non-throwing by catching at the boundary and returning a documented failure value, or introduce a status plus out-parameter API, with a C-facing regression. Deferring the general estimator contract to MOD-14958 does not make the new reachable throw sites safe here.