[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).

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	46/46 passed
make unit_test DEBUG=1	2653/2653 passed
make asan	2653/2653 passed, 0 sanitizer reports

Not run: FP_64=1 variants, since this change is FP32/FP16 only.

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

[cursor]

SQ8 element estimate skips validation

Low Severity

The new SQ8 branch in EstimateElementSize always applies GetSQ8StoredDataSize whenever quantType is VecSimQuant_SQ8, without checking that type is FLOAT32 or FLOAT16 or that the metric is supported. In the same file, NewIndex returns NULL for unsupported types and standalone Cosine, and EstimateInitialSize throws on invalid SQ8 types. Callers that size capacity from VecSimIndex_EstimateElementSize alone can get per-element byte counts for parameter sets that cannot produce an index.

Additional Locations (2)

• src/VecSim/index_factories/hnsw_factory.cpp#L119-L123
• src/VecSim/index_factories/hnsw_factory.cpp#L214-L218

 

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

[dor-forer]

Accurate, but I do not think it should change here, and the reasoning is worth recording.

The asymmetry is pre-existing behaviour of EstimateElementSize rather than something SQ8 introduced. Its unquantized path calls VecSimParams_GetStoredDataSize (vec_utils.cpp:296-302), which is VecSimType_sizeof(type) * dim plus a Cosine/int8 adjustment and validates nothing, for any algorithm. So the function has always returned a per-element size for parameters that cannot produce an index; the SQ8 branch matches that existing contract.

Making it strict needs an error channel it does not have. The return type is size_t, so the options are a sentinel or a throw, and EstimateElementSize currently contains no throw at all: the only ones in the file are in EstimateInitialSize and the file-loading paths. Adding one would newly carry a C++ exception across the extern "C" boundary via VecSimIndex_EstimateElementSize, which this library specifically avoids, since an exception reaching the C host aborts the host process.

The genuinely inconsistent one is arguably EstimateInitialSize being strict enough to throw, not EstimateElementSize being lax. Deciding the error model for both belongs with MOD-14958, which is what first makes quantType reachable from RediSearch; there is no product exposure before then.

What I did add is coverage of the boundary that does enforce the supported set: HNSWSQ8ParamsTest.RejectsUnsupportedDataType (7719c58) asserts that FLOAT64, BFLOAT16, INT8 and UINT8 with VecSimQuant_SQ8 all return NULL from index creation, plus a comment at the estimate explaining why it deliberately does not repeat the check. Verified red without the fix: with both the type fence and the fall-through return NULL removed, all four types are silently built as unquantized indexes.

[codecov]

Codecov Report

❌ Patch coverage is 95.40230% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.13%. Comparing base (efd63da) to head (7719c58).

Files with missing lines	Patch %	Lines
src/VecSim/index_factories/hnsw_factory.cpp	95.06%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
- Coverage   97.15%   97.13%   -0.03%     
==========================================
  Files         141      141              
  Lines        8328     8409      +81     
==========================================
+ Hits         8091     8168      +77     
- Misses        237      241       +4     

☔ 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.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.

There are 4 total unresolved issues (including 1 from previous review).

^{❌ 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 814eab5. Configure here.}

[cursor]

Tiered estimates ignore SQ8 rejection

Medium Severity

Tiered index NewIndex functions reject invalid configurations, such as non-NONE quantization, but the associated EstimateInitialSize and EstimateElementSize functions don't perform these same validation checks. This can lead to positive memory estimates for configurations that cannot actually be instantiated.

Additional Locations (1)

• src/VecSim/index_factories/tiered_factory.cpp#L224-L251

 

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

[cursor]

Unknown quantType builds unquantized index

Medium Severity

HNSWFactory::NewIndex only handles quantType == VecSimQuant_SQ8 explicitly. Any other non-VecSimQuant_NONE value falls through to the ordinary unquantized CreateIndexComponents path and builds a full-precision HNSW index without error. Callers that set an unsupported or forward-looking quantization code get silent misconfiguration instead of NULL, unlike the tiered factory which rejects any non-NONE quantType.

 

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

[cursor]

SQ8 initial size skips metric

Low Severity

The VecSimQuant_SQ8 branch in EstimateInitialSize validates params->type but not params->metric. Configurations such as SQ8 with VecSimMetric_Cosine receive a full SQ8 initial-size estimate even though NewIndex returns NULL for the same params (cosine is rejected unless remapped via is_normalized, which the public C API does not use).

 

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

[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

[lerman25]

Maybe assert false here ?

[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.

[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.

[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.

[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.

[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.