From 274afbc5ccfc2ddb932bd8dad9eba7e425180453 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Tue, 4 Aug 2026 17:19:25 +0300 Subject: [PATCH 1/9] [MOD-14956] Add SQ8 quantization support for HNSW index Cherry-picked from ARM-software/VectorSimilarity-for-Arm#4 (head 125ea15d), squashing the fork's four commits into one. 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. SQ8 support for the tiered HNSW index, serialization and benchmarks is deferred to later PRs in the MOD-14956 series. Redis-side adjustments made during the cherry-pick: * Dropped the added `SPDX-FileCopyrightText` Arm line from the two modified files, matching how #999, #1000 and #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. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/index_factories/hnsw_factory.cpp | 146 +++++++- src/VecSim/vec_sim_common.h | 8 + tests/unit/CMakeLists.txt | 3 + tests/unit/test_hnsw_sq8.cpp | 380 ++++++++++++++++++++ 4 files changed, 533 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_hnsw_sq8.cpp diff --git a/src/VecSim/index_factories/hnsw_factory.cpp b/src/VecSim/index_factories/hnsw_factory.cpp index d577f57a1..cfa51a048 100644 --- a/src/VecSim/index_factories/hnsw_factory.cpp +++ b/src/VecSim/index_factories/hnsw_factory.cpp @@ -17,6 +17,7 @@ using bfloat16 = vecsim_types::bfloat16; using float16 = vecsim_types::float16; +using sq8 = vecsim_types::sq8; namespace HNSWFactory { @@ -34,11 +35,117 @@ NewIndex_ChooseMultiOrSingle(const HNSWParams *params, HNSWIndex_Single(params, abstractInitParams, components); } +template +size_t GetSQ8StoredDataSize(size_t dim, bool with_norm) { + static_assert(Metric == VecSimMetric_L2 || Metric == VecSimMetric_IP); + + const auto metadata_count = with_norm ? sq8::storage_metadata_count() + : sq8::storage_metadata_count(); + + return dim + metadata_count * sizeof(float); +} + +// Helper to build an SQ8-quantized HNSW index given compile-time DataType and Metric. +template +VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams abstractInitParams, + const float *mean_ptr) { + auto &allocator = abstractInitParams.allocator; + size_t dim = abstractInitParams.dim; + unsigned char storage_alignment = 0, asym_storage_alignment = 0, query_alignment = 0; + bool with_norm = mean_ptr != nullptr; + + // Override blob size for the SQ8 storage layout. + abstractInitParams.storedDataSize = GetSQ8StoredDataSize(dim, with_norm); + + // Symmetric: both stored vectors are SQ8 blobs. + auto sym_func = spaces::GetDistFunc(Metric, dim, &storage_alignment); + // Asymmetric: stored vector is SQ8 blob, query is DataType. + auto asym_func = + spaces::GetDistFunc(Metric, dim, &asym_storage_alignment); + storage_alignment = spaces::combineAlignments(storage_alignment, asym_storage_alignment); + spaces::GetDistFunc(Metric, dim, &query_alignment); + + if (!with_norm) { + // plain SQ8 quantization without mean centering. + auto *pp = new (allocator) QuantPreprocessor(allocator, dim); + auto *container = new (allocator) + MultiPreprocessorsContainer(allocator, query_alignment, storage_alignment); + [[maybe_unused]] int ret = container->addPreprocessor(pp); + assert(ret == 0 && "SQ8 preprocessor was not added correctly"); + + // sym_func for storage-storage; asym_func for query-storage. + auto *calc = + new (allocator) DistanceCalculatorCommon(allocator, sym_func, asym_func); + + IndexComponents components{calc, container}; + return NewIndex_ChooseMultiOrSingle(hnswParams, abstractInitParams, + components); + } + + // With norm: mean-centered SQ8 quantization with norm correction. + vecsim_stl::vector mean_vec(dim, 0.0f, allocator); + memcpy(mean_vec.data(), mean_ptr, dim * sizeof(float)); + + float mean_sum_squares = 0.0f; + for (float v : mean_vec) { + mean_sum_squares += v * v; + } + + auto *pp = new (allocator) QuantPreprocessor(allocator, dim, mean_vec); + auto *container = new (allocator) + MultiPreprocessorsContainer(allocator, query_alignment, storage_alignment); + [[maybe_unused]] int ret = container->addPreprocessor(pp); + assert(ret == 0 && "SQ8 preprocessor was not added correctly"); + + auto *calc = new (allocator) DistanceCalculatorWithNorm( + allocator, asym_func, sym_func, mean_sum_squares); + + IndexComponents components{calc, container}; + return NewIndex_ChooseMultiOrSingle(hnswParams, abstractInitParams, + components); +} + VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { const HNSWParams *hnswParams = ¶ms->algoParams.hnswParams; + AbstractIndexInitParams abstractInitParams = VecSimFactory::NewAbstractInitParams(hnswParams, params->logCtx, is_normalized); + if (hnswParams->quantType == VecSimQuant_SQ8) { + if (hnswParams->type != VecSimType_FLOAT32 && hnswParams->type != VecSimType_FLOAT16) { + return NULL; // SQ8 supports FP32 and FP16 only. + } + + VecSimMetric metric = hnswParams->metric; + if (is_normalized && metric == VecSimMetric_Cosine) { + metric = VecSimMetric_IP; + } + + if (metric == VecSimMetric_Cosine) { + return NULL; // SQ8 does not support cosine metric. + } + + const float *mean_ptr = static_cast(hnswParams->quantParams); + + if (hnswParams->type == VecSimType_FLOAT32) { + if (metric == VecSimMetric_L2) { + return NewIndex_SQ8(hnswParams, abstractInitParams, + mean_ptr); + } else if (metric == VecSimMetric_IP) { + return NewIndex_SQ8(hnswParams, abstractInitParams, + mean_ptr); + } + } else if (hnswParams->type == VecSimType_FLOAT16) { + if (metric == VecSimMetric_L2) { + return NewIndex_SQ8(hnswParams, abstractInitParams, + mean_ptr); + } else if (metric == VecSimMetric_IP) { + return NewIndex_SQ8(hnswParams, abstractInitParams, + mean_ptr); + } + } + } + if (hnswParams->type == VecSimType_FLOAT32) { IndexComponents indexComponents = CreateIndexComponents( abstractInitParams.allocator, hnswParams->metric, hnswParams->dim, is_normalized); @@ -94,7 +201,27 @@ size_t EstimateInitialSize(const HNSWParams *params, bool is_normalized) { size_t allocations_overhead = VecSimAllocator::getAllocationOverheadSize(); size_t est = sizeof(VecSimAllocator) + allocations_overhead; - if (params->type == VecSimType_FLOAT32) { + + if (params->quantType == VecSimQuant_SQ8) { + if (params->type != VecSimType_FLOAT32 && params->type != VecSimType_FLOAT16) { + throw std::invalid_argument("Invalid params->type for VecSimQuant_SQ8"); + } + // Calculator + preprocessor container + preprocessor. + // Use representative types; sizeof is independent of the template parameters. + if (params->quantParams) { // mean provided, WithNorm = true + est += allocations_overhead + + sizeof(DistanceCalculatorWithNorm); + est += allocations_overhead + sizeof(MultiPreprocessorsContainer); + est += allocations_overhead + sizeof(QuantPreprocessor); + est += allocations_overhead + + params->dim * sizeof(float); // mean vector in QuantPreprocessor + } else { + est += allocations_overhead + sizeof(DistanceCalculatorCommon); + est += allocations_overhead + sizeof(MultiPreprocessorsContainer); + est += allocations_overhead + sizeof(QuantPreprocessor); + } + est += EstimateInitialSize_ChooseMultiOrSingle(params->multi); + } else if (params->type == VecSimType_FLOAT32) { est += EstimateComponentsMemory(params->metric, is_normalized); est += EstimateInitialSize_ChooseMultiOrSingle(params->multi); } else if (params->type == VecSimType_FLOAT64) { @@ -125,9 +252,20 @@ size_t EstimateElementSize(const HNSWParams *params) { size_t M = (params->M) ? params->M : HNSW_DEFAULT_M; size_t elementGraphDataSize = sizeof(ElementGraphData) + sizeof(idType) * M * 2; - size_t size_total_data_per_element = - elementGraphDataSize + - VecSimParams_GetStoredDataSize(params->type, params->dim, params->metric); + size_t stored_data_size; + if (params->quantType == VecSimQuant_SQ8) { + bool with_norm = params->quantParams != nullptr; + if (params->metric == VecSimMetric_L2) { + stored_data_size = GetSQ8StoredDataSize(params->dim, with_norm); + } else { + stored_data_size = GetSQ8StoredDataSize(params->dim, with_norm); + } + } else { + stored_data_size = + VecSimParams_GetStoredDataSize(params->type, params->dim, params->metric); + } + + size_t size_total_data_per_element = elementGraphDataSize + stored_data_size; // when reserving space for new labels in the lookup hash table, each entry is a pointer to a // label node (bucket). diff --git a/src/VecSim/vec_sim_common.h b/src/VecSim/vec_sim_common.h index fe10a5a0c..fb79ca30b 100644 --- a/src/VecSim/vec_sim_common.h +++ b/src/VecSim/vec_sim_common.h @@ -68,6 +68,12 @@ typedef enum { VecSimType_INT64 } VecSimType; +// Quantization type for HNSW indices. +typedef enum { + VecSimQuant_NONE = 0, // No quantization (default). + VecSimQuant_SQ8 = 1, // 8-bit scalar quantization with mean normalization. +} VecSimQuantType; + // Algorithm type/library. typedef enum { VecSimAlgo_BF, VecSimAlgo_HNSWLIB, VecSimAlgo_TIERED, VecSimAlgo_SVS } VecSimAlgo; @@ -156,6 +162,8 @@ typedef struct { size_t efConstruction; size_t efRuntime; double epsilon; + VecSimQuantType quantType; // Quantization type. Default: VecSimQuant_NONE. + void *quantParams; // For VecSimQuant_SQ8: pointer to float mean[dim], or NULL for zero mean. } HNSWParams; typedef struct { diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index c3e1cc987..4eeeae443 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -36,6 +36,7 @@ endif() add_executable(test_hnsw ../utils/test_main_with_timeout.cpp ../utils/mock_thread_pool.cpp test_hnsw.cpp test_hnsw_multi.cpp test_hnsw_tiered.cpp unit_test_utils.cpp) add_executable(test_hnsw_parallel ../utils/test_main_with_timeout.cpp test_hnsw_parallel.cpp ../utils/mock_thread_pool.cpp unit_test_utils.cpp) +add_executable(test_hnsw_sq8 ../utils/test_main_with_timeout.cpp ../utils/mock_thread_pool.cpp test_hnsw_sq8.cpp unit_test_utils.cpp) add_executable(test_bruteforce ../utils/test_main_with_timeout.cpp test_bruteforce.cpp test_bruteforce_multi.cpp ../utils/mock_thread_pool.cpp unit_test_utils.cpp) add_executable(test_allocator ../utils/test_main_with_timeout.cpp test_allocator.cpp ../utils/mock_thread_pool.cpp unit_test_utils.cpp) add_executable(test_spaces ../utils/test_main_with_timeout.cpp test_spaces.cpp) @@ -51,6 +52,7 @@ add_executable(test_svs ../utils/test_main_with_timeout.cpp ../utils/mock_thread target_link_libraries(test_hnsw PUBLIC gtest VectorSimilarity) target_link_libraries(test_hnsw_parallel PUBLIC gtest VectorSimilarity) +target_link_libraries(test_hnsw_sq8 PUBLIC gtest VectorSimilarity) target_link_libraries(test_bruteforce PUBLIC gtest VectorSimilarity) target_link_libraries(test_allocator PUBLIC gtest VectorSimilarity) target_link_libraries(test_spaces PUBLIC gtest VectorSimilarity) @@ -68,6 +70,7 @@ include(GoogleTest) gtest_discover_tests(test_hnsw) gtest_discover_tests(test_hnsw_parallel) +gtest_discover_tests(test_hnsw_sq8) gtest_discover_tests(test_bruteforce) gtest_discover_tests(test_allocator) gtest_discover_tests(test_spaces) diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp new file mode 100644 index 000000000..cac137727 --- /dev/null +++ b/tests/unit/test_hnsw_sq8.cpp @@ -0,0 +1,380 @@ +/* + * Copyright (c) 2006-Present, Redis Ltd. + * All rights reserved. + * SPDX-FileCopyrightText: Copyright 2026 Arm Limited and/or its affiliates + * + * + * Licensed under your choice of the Redis Source Available License 2.0 + * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the + * GNU Affero General Public License v3 (AGPLv3). + */ + +#include "gtest/gtest.h" +#include "VecSim/algorithms/hnsw/hnsw_single.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include "VecSim/vec_sim.h" +#include "unit_test_utils.h" + +#include +#include +#include +#include + +template +struct HNSWSQ8IndexType : IndexType { + static constexpr bool with_quant_params = WithQuantParams; +}; + +using HNSWSQ8DataTypeSet = + ::testing::Types, + HNSWSQ8IndexType, + HNSWSQ8IndexType, + HNSWSQ8IndexType>; + +template +class HNSWSQ8Test : public ::testing::Test { +public: + using data_t = typename index_type_t::data_t; + +protected: + static constexpr float quantization_mean_value = 1.0f; + + static data_t ToDataType(float value) { + if constexpr (std::is_same_v) { + return vecsim_types::FP32_to_FP16(value); + } else { + return value; + } + } + + void SetUp(HNSWParams ¶ms) { + params.type = index_type_t::get_index_type(); + params.quantType = VecSimQuant_SQ8; + if constexpr (index_type_t::with_quant_params) { + quantization_mean.assign(params.dim, quantization_mean_value); + params.quantParams = quantization_mean.data(); + } + VecSimParams vecsim_params = CreateParams(params); + index = VecSimIndex_New(&vecsim_params); + ASSERT_NE(index, nullptr); + dim = params.dim; + } + + void TearDown() override { + if (index) { + VecSimIndex_Free(index); + } + } + + HNSWIndex *CastToHNSW() { + return dynamic_cast *>(index); + } + + void GenerateVector(data_t *out_vec, float initial_value = 0.25f, float step = 0.0f) { + for (size_t i = 0; i < dim; i++) { + out_vec[i] = ToDataType(initial_value + step * static_cast(i)); + } + } + + int GenerateAndAddVector(size_t label, float initial_value = 0.25f, float step = 0.0f) { + std::vector vector(dim); + GenerateVector(vector.data(), initial_value, step); + return VecSimIndex_AddVector(index, vector.data(), label); + } + + void create_index_test(); + void search_by_id_test(); + void search_by_score_test(); + void search_empty_index_test(); + void test_override(); + void test_range_query(); + void test_get_distance(VecSimMetric metric); + void test_batch_iterator_basic(); + + VecSimIndex *index = nullptr; + size_t dim = 0; + std::vector quantization_mean; +}; + +TYPED_TEST_SUITE(HNSWSQ8Test, HNSWSQ8DataTypeSet); + +/* ---------------------------- Create index tests ---------------------------- */ + +template +void HNSWSQ8Test::create_index_test() { + HNSWParams params = {.dim = 40, .M = 16, .efConstruction = 200}; + SetUp(params); + + constexpr float initial_value = 0.5f; + constexpr float step = 1.0f; + ASSERT_EQ(VecSimIndex_IndexSize(index), 0u); + ASSERT_EQ(GenerateAndAddVector(0, initial_value, step), 1); + ASSERT_EQ(VecSimIndex_IndexSize(index), 1u); + + auto *hnsw_index = CastToHNSW(); + ASSERT_NE(hnsw_index, nullptr); + const auto *stored = reinterpret_cast(hnsw_index->getDataByInternalId(0)); + EXPECT_EQ(stored[0], 0); + EXPECT_EQ(stored[dim - 1], 255); + + // The quantized vector is followed by the minimum value and quantization delta. + float stored_min; + float stored_delta; + std::memcpy(&stored_min, stored + dim + sq8::MIN_VAL * sizeof(float), sizeof(float)); + std::memcpy(&stored_delta, stored + dim + sq8::DELTA * sizeof(float), sizeof(float)); + const float expected_min = + initial_value - (index_type_t::with_quant_params ? quantization_mean_value : 0.0f); + EXPECT_FLOAT_EQ(stored_min, expected_min); + EXPECT_FLOAT_EQ(stored_delta, step * static_cast(dim - 1) / 255.0f); + + EXPECT_EQ(index->basicInfo().type, index_type_t::get_index_type()); + EXPECT_EQ(index->basicInfo().algo, VecSimAlgo_HNSWLIB); +} + +TYPED_TEST(HNSWSQ8Test, CreateIndex) { this->create_index_test(); } + +TYPED_TEST(HNSWSQ8Test, RejectStandaloneCosine) { + HNSWParams params = {.type = TypeParam::get_index_type(), + .dim = 4, + .metric = VecSimMetric_Cosine, + .quantType = VecSimQuant_SQ8}; + if constexpr (TypeParam::with_quant_params) { + this->quantization_mean.assign(params.dim, this->quantization_mean_value); + params.quantParams = this->quantization_mean.data(); + } + + VecSimParams vecsim_params = CreateParams(params); + this->index = VecSimIndex_New(&vecsim_params); + EXPECT_EQ(this->index, nullptr); +} + +/* ---------------------------- Size Estimation tests ---------------------------- */ + +TYPED_TEST(HNSWSQ8Test, SizeEstimation) { + constexpr size_t block_size = 256; + HNSWParams params = {.dim = 128, .blockSize = block_size, .M = 64}; + this->SetUp(params); + + // EstimateInitialSize is called after creating the index because index creation normalizes + // the parameters. + EXPECT_EQ(EstimateInitialSize(params), this->index->getAllocationSize()); + + size_t label = 0; + while (this->index->indexSize() < 200 || this->index->indexSize() % block_size != 0) { + ASSERT_EQ(this->GenerateAndAddVector(label, static_cast(label)), 1); + label++; + } + + // Estimate the memory delta of adding a vector that requires a full new block. + const size_t estimation = EstimateElementSize(params) * block_size; + const size_t before = this->index->getAllocationSize(); + ASSERT_EQ(this->GenerateAndAddVector(label, static_cast(label)), 1); + const size_t actual = this->index->getAllocationSize() - before; + + // Check that the actual size is within 1% of the estimation. + EXPECT_GE(estimation, actual * 0.99); + EXPECT_LE(estimation, actual * 1.01); +} + +/* ---------------------------- Functionality tests ---------------------------- */ + +template +void HNSWSQ8Test::search_by_id_test() { + HNSWParams params = { + .dim = 4, .initialCapacity = 200, .M = 16, .efConstruction = 200, .efRuntime = 100}; + SetUp(params); + + for (size_t i = 0; i < 100; i++) { + ASSERT_EQ(GenerateAndAddVector(i, static_cast(i)), 1); + } + + data_t query[4]; + GenerateVector(query, 50.0f); + // Vector values are equal to their labels, so the closest vectors have labels 45 through 55. + static constexpr size_t expected[] = {45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55}; + auto verify = [&](size_t id, double score, size_t result_index) { + // Results are sorted by ID. + EXPECT_EQ(id, expected[result_index]); + EXPECT_FLOAT_EQ(score, 4.0f * (50.0f - id) * (50.0f - id)); // L2 distance. + }; + runTopKSearchTest(index, query, std::size(expected), verify, nullptr, BY_ID); +} + +TYPED_TEST(HNSWSQ8Test, SearchByID) { this->search_by_id_test(); } + +template +void HNSWSQ8Test::search_by_score_test() { + HNSWParams params = { + .dim = 4, .initialCapacity = 200, .M = 16, .efConstruction = 200, .efRuntime = 100}; + SetUp(params); + + for (size_t i = 0; i < 100; i++) { + ASSERT_EQ(GenerateAndAddVector(i, static_cast(i)), 1); + } + + data_t query[4]; + GenerateVector(query, 50.0f); + // Vector values are equal to their labels, so results are ordered by distance from label 50. + static constexpr size_t expected[] = {50, 49, 51, 48, 52, 47, 53, 46, 54, 45, 55}; + auto verify = [&](size_t id, double score, size_t result_index) { + EXPECT_EQ(id, expected[result_index]); + EXPECT_FLOAT_EQ(score, 4.0f * (50.0f - id) * (50.0f - id)); + }; + runTopKSearchTest(index, query, std::size(expected), verify); +} + +TYPED_TEST(HNSWSQ8Test, SearchByScore) { this->search_by_score_test(); } + +template +void HNSWSQ8Test::search_empty_index_test() { + HNSWParams params = {.dim = 4, .initialCapacity = 0}; + SetUp(params); + + data_t query[4]; + GenerateVector(query, 50.0f); + + // We do not expect any results. + VecSimQueryReply *reply = VecSimIndex_TopKQuery(index, query, 11, nullptr, BY_SCORE); + ASSERT_EQ(VecSimQueryReply_Len(reply), 0u); + VecSimQueryReply_Free(reply); + + reply = VecSimIndex_RangeQuery(index, query, 1.0, nullptr, BY_SCORE); + ASSERT_EQ(VecSimQueryReply_Len(reply), 0u); + VecSimQueryReply_Free(reply); + + // Add some vectors and remove them all from the index, so it will be empty again. + for (size_t i = 0; i < 100; i++) { + GenerateAndAddVector(i, static_cast(i)); + } + for (size_t i = 0; i < 100; i++) { + VecSimIndex_DeleteVector(index, i); + } + ASSERT_EQ(VecSimIndex_IndexSize(index), 0u); + + // Again, we do not expect any results. + reply = VecSimIndex_TopKQuery(index, query, 11, nullptr, BY_SCORE); + ASSERT_EQ(VecSimQueryReply_Len(reply), 0u); + VecSimQueryReply_Free(reply); + + reply = VecSimIndex_RangeQuery(index, query, 1.0, nullptr, BY_SCORE); + ASSERT_EQ(VecSimQueryReply_Len(reply), 0u); + VecSimQueryReply_Free(reply); +} + +TYPED_TEST(HNSWSQ8Test, SearchEmptyIndex) { this->search_empty_index_test(); } + +template +void HNSWSQ8Test::test_override() { + constexpr size_t count = 250; + HNSWParams params = { + .dim = 4, .initialCapacity = 100, .M = 8, .efConstruction = 20, .efRuntime = count}; + SetUp(params); + + // Insert 100 vectors and then overwrite each one with the same value. + for (size_t i = 0; i < 100; i++) { + ASSERT_EQ(GenerateAndAddVector(i, static_cast(i)), 1); + ASSERT_EQ(GenerateAndAddVector(i, static_cast(i)), 0); + } + // Add vectors up to count. + for (size_t i = 100; i < count; i++) { + ASSERT_EQ(GenerateAndAddVector(i, static_cast(i)), 1); + } + + data_t query[4]; + GenerateVector(query, static_cast(count)); + // The largest label is closest to the query, so labels are returned in descending order. + auto verify = [&](size_t id, double score, size_t result_index) { + EXPECT_EQ(id, count - result_index - 1); + EXPECT_FLOAT_EQ(score, 4.0f * (count - id) * (count - id)); + }; + runTopKSearchTest(index, query, count, verify); +} + +TYPED_TEST(HNSWSQ8Test, Override) { this->test_override(); } + +template +void HNSWSQ8Test::test_range_query() { + constexpr size_t count = 100; + constexpr size_t close_count = 20; + HNSWParams params = {.dim = 4, .initialCapacity = count, .efRuntime = count}; + SetUp(params); + + constexpr float pivot = 1.0f; + constexpr float value_radius = 1.5f; + std::mt19937 generator(42); + std::uniform_real_distribution distribution(pivot - value_radius, pivot + value_radius); + // Insert close_count vectors near the pivot vector. + for (size_t i = 0; i < close_count; i++) { + GenerateAndAddVector(i, distribution(generator)); + } + // Add the remaining vectors far from the pivot vector. + for (size_t i = close_count; i < count; i++) { + GenerateAndAddVector(i, 5.0f + distribution(generator)); + } + + data_t query[4]; + GenerateVector(query, pivot); + constexpr double max_distance = 4.0 * value_radius * value_radius; + auto verify = [&](size_t id, double score, size_t) { + EXPECT_LT(id, close_count); + EXPECT_LE(score, max_distance); + }; + runRangeQueryTest(index, query, max_distance, verify, close_count, BY_SCORE); +} + +TYPED_TEST(HNSWSQ8Test, RangeQuery) { this->test_range_query(); } + +template +void HNSWSQ8Test::test_get_distance(VecSimMetric metric) { + HNSWParams params = {.dim = 4, .metric = metric, .initialCapacity = 1}; + SetUp(params); + + ASSERT_EQ(GenerateAndAddVector(0, 0.25f, 0.25f), 1); + data_t query[4]; + GenerateVector(query, 0.5f, 0.25f); + auto processed_query = CastToHNSW()->preprocessQuery(query); + const double expected = metric == VecSimMetric_L2 ? 0.25 : -1.5; + // Values were chosen so the expected distances can be calculated exactly. + EXPECT_NEAR(VecSimIndex_GetDistanceFrom_Unsafe(index, 0, processed_query.get()), expected, + 1e-5); +} + +TYPED_TEST(HNSWSQ8Test, GetDistanceL2) { this->test_get_distance(VecSimMetric_L2); } +TYPED_TEST(HNSWSQ8Test, GetDistanceIP) { this->test_get_distance(VecSimMetric_IP); } + +/* ---------------------------- Batch iterator tests ---------------------------- */ + +template +void HNSWSQ8Test::test_batch_iterator_basic() { + constexpr size_t count = 250; + constexpr size_t batch_size = 5; + HNSWParams params = { + .dim = 4, .initialCapacity = count, .M = 8, .efConstruction = 20, .efRuntime = count}; + SetUp(params); + + // For every i, add the vector (i, i, i, i) under label i. + for (size_t i = 0; i < count; i++) { + ASSERT_EQ(GenerateAndAddVector(i, static_cast(i)), 1); + } + + data_t query[4]; + GenerateVector(query, static_cast(count)); + VecSimBatchIterator *iterator = VecSimBatchIterator_New(index, query, nullptr); + ASSERT_NE(iterator, nullptr); + + // Get the five largest remaining labels in each iteration. Since vector values equal their + // labels, this is also their order by distance from the query vector. + size_t iteration = 0; + while (VecSimBatchIterator_HasNext(iterator)) { + auto verify = [&](size_t id, double, size_t result_index) { + EXPECT_EQ(id, count - iteration * batch_size - result_index - 1); + }; + runBatchIteratorSearchTest(iterator, batch_size, verify); + iteration++; + } + EXPECT_EQ(iteration, count / batch_size); + VecSimBatchIterator_Free(iterator); +} + +TYPED_TEST(HNSWSQ8Test, BatchIteratorBasic) { this->test_batch_iterator_basic(); } From 4d09236fd35f2ae48c51c2a292d23f19816476bb Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 5 Aug 2026 11:42:56 +0300 Subject: [PATCH 2/9] Tighten the SQ8 HNSW factory and its new public API Follow-up review pass over the cherry-picked MOD-14956 change. No behavioural change is intended: all of these are interface, single-source-of-truth and idiom fixes. Public API (`vec_sim_common.h`): * `quantParams` is now `const void *`. Every use in the tree reads it, and two already cast it to `const float *`. The layout is unchanged, so this is not an ABI break, and callers passing a non-const pointer still compile. Worth doing now, before the field ships and freezes. * The `VecSimQuant_SQ8` comment claimed "with mean normalization". Mean normalization is optional and selected by `quantParams`, exactly as the field's own comment says. Reworded. Storage layout (`types/sq8.h`, `spaces/computer/preprocessors.h`, `index_factories/hnsw_factory.cpp`): * `GetSQ8StoredDataSize` re-derived the stored blob size that `QuantPreprocessor`'s constructors already computed. Two independent 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(dim)`, next to the `storage_metadata_count` it builds on, and both the preprocessor and the factory call it. Factory (`index_factories/hnsw_factory.cpp`): * Restored the `return NULL` that closes the SQ8 branch. It is 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 the two branches duplicated (container construction, `addPreprocessor`, assert, `IndexComponents`, return). Only the preprocessor and the distance calculator actually differ. * The mean vector is copied with a single `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` adapter that returns the hint, so the call site neither discards a value nor keeps a third distance function in scope next to `sym_func` and `asym_func` that must never be called. `query_alignment` is const. * `GetSQ8StoredDataSize` is `[[nodiscard]] constexpr` and `dim` / `with_norm` are const. Verified: - ./check-format.sh - g++ -std=gnu++20 -Wall -Werror -fsyntax-only, with and without -DNDEBUG - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 44/44 passed - make unit_test DEBUG=1: 2651/2651 passed - make asan: 2651/2651 passed, 0 sanitizer reports Not run: - FP_64=1 variants (this change is FP32/FP16 only) Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/index_factories/hnsw_factory.cpp | 80 ++++++++++++--------- src/VecSim/spaces/computer/preprocessors.h | 7 +- src/VecSim/types/sq8.h | 9 +++ src/VecSim/vec_sim_common.h | 7 +- 4 files changed, 61 insertions(+), 42 deletions(-) diff --git a/src/VecSim/index_factories/hnsw_factory.cpp b/src/VecSim/index_factories/hnsw_factory.cpp index cfa51a048..54b4c333f 100644 --- a/src/VecSim/index_factories/hnsw_factory.cpp +++ b/src/VecSim/index_factories/hnsw_factory.cpp @@ -36,13 +36,24 @@ NewIndex_ChooseMultiOrSingle(const HNSWParams *params, } template -size_t GetSQ8StoredDataSize(size_t dim, bool with_norm) { +[[nodiscard]] constexpr size_t GetSQ8StoredDataSize(size_t dim, bool with_norm) { static_assert(Metric == VecSimMetric_L2 || Metric == VecSimMetric_IP); - const auto metadata_count = with_norm ? sq8::storage_metadata_count() - : sq8::storage_metadata_count(); + // WithNorm is a template parameter, so dispatch the runtime flag to the two instantiations. + return with_norm ? sq8::storage_bytes_count(dim) + : sq8::storage_bytes_count(dim); +} - return dim + metadata_count * sizeof(float); +// Alignment required by a query blob of type DataType. Per the asymmetric-types contract in +// spaces.h, the hint returned alongside an asymmetric distance function describes its first +// (storage) operand, so the query side must be obtained from the symmetric dispatcher for the +// query's own type. Only that hint is wanted here, never the function it returns, so the call is +// contained in this adapter instead of leaving a discarded value at the call site. +template +[[nodiscard]] unsigned char GetQueryAlignment(VecSimMetric metric, size_t dim) { + unsigned char alignment = 0; + spaces::GetDistFunc(metric, dim, &alignment); + return alignment; } // Helper to build an SQ8-quantized HNSW index given compile-time DataType and Metric. @@ -50,9 +61,9 @@ template VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams abstractInitParams, const float *mean_ptr) { auto &allocator = abstractInitParams.allocator; - size_t dim = abstractInitParams.dim; - unsigned char storage_alignment = 0, asym_storage_alignment = 0, query_alignment = 0; - bool with_norm = mean_ptr != nullptr; + const size_t dim = abstractInitParams.dim; + const bool with_norm = mean_ptr != nullptr; + unsigned char storage_alignment = 0, asym_storage_alignment = 0; // Override blob size for the SQ8 storage layout. abstractInitParams.storedDataSize = GetSQ8StoredDataSize(dim, with_norm); @@ -62,43 +73,38 @@ VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams // Asymmetric: stored vector is SQ8 blob, query is DataType. auto asym_func = spaces::GetDistFunc(Metric, dim, &asym_storage_alignment); + // Both hints describe the same stored blob, so they must be combined rather than overwritten. storage_alignment = spaces::combineAlignments(storage_alignment, asym_storage_alignment); - spaces::GetDistFunc(Metric, dim, &query_alignment); + // Queries stay in DataType and are compared against stored blobs by asym_func. + const unsigned char query_alignment = GetQueryAlignment(Metric, dim); - if (!with_norm) { - // plain SQ8 quantization without mean centering. - auto *pp = new (allocator) QuantPreprocessor(allocator, dim); - auto *container = new (allocator) - MultiPreprocessorsContainer(allocator, query_alignment, storage_alignment); - [[maybe_unused]] int ret = container->addPreprocessor(pp); - assert(ret == 0 && "SQ8 preprocessor was not added correctly"); + PreprocessorInterface *pp = nullptr; + IndexCalculatorInterface *calc = nullptr; - // sym_func for storage-storage; asym_func for query-storage. - auto *calc = - new (allocator) DistanceCalculatorCommon(allocator, sym_func, asym_func); + if (with_norm) { + // Mean-centered SQ8 quantization with norm correction. + vecsim_stl::vector mean_vec(allocator); + mean_vec.assign(mean_ptr, mean_ptr + dim); - IndexComponents components{calc, container}; - return NewIndex_ChooseMultiOrSingle(hnswParams, abstractInitParams, - components); - } - - // With norm: mean-centered SQ8 quantization with norm correction. - vecsim_stl::vector mean_vec(dim, 0.0f, allocator); - memcpy(mean_vec.data(), mean_ptr, dim * sizeof(float)); + float mean_sum_squares = 0.0f; + for (float v : mean_vec) { + mean_sum_squares += v * v; + } - float mean_sum_squares = 0.0f; - for (float v : mean_vec) { - mean_sum_squares += v * v; + pp = new (allocator) QuantPreprocessor(allocator, dim, mean_vec); + calc = new (allocator) DistanceCalculatorWithNorm( + allocator, asym_func, sym_func, mean_sum_squares); + } else { + // Plain SQ8 quantization without mean centering. + pp = new (allocator) QuantPreprocessor(allocator, dim); + // sym_func for storage-storage; asym_func for query-storage. + calc = new (allocator) DistanceCalculatorCommon(allocator, sym_func, asym_func); } - auto *pp = new (allocator) QuantPreprocessor(allocator, dim, mean_vec); auto *container = new (allocator) MultiPreprocessorsContainer(allocator, query_alignment, storage_alignment); - [[maybe_unused]] int ret = container->addPreprocessor(pp); - assert(ret == 0 && "SQ8 preprocessor was not added correctly"); - - auto *calc = new (allocator) DistanceCalculatorWithNorm( - allocator, asym_func, sym_func, mean_sum_squares); + [[maybe_unused]] const int ret = container->addPreprocessor(pp); + assert(ret != -1 && "SQ8 preprocessor was not added correctly"); IndexComponents components{calc, container}; return NewIndex_ChooseMultiOrSingle(hnswParams, abstractInitParams, @@ -144,6 +150,10 @@ VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { mean_ptr); } } + + // Unreachable today: the checks above leave only FP32/FP16 x L2/IP. Kept so that adding a + // type or metric cannot silently fall through and build an unquantized index instead. + return NULL; } if (hnswParams->type == VecSimType_FLOAT32) { diff --git a/src/VecSim/spaces/computer/preprocessors.h b/src/VecSim/spaces/computer/preprocessors.h index 195be1418..11de96998 100644 --- a/src/VecSim/spaces/computer/preprocessors.h +++ b/src/VecSim/spaces/computer/preprocessors.h @@ -426,8 +426,7 @@ class QuantPreprocessor : public PreprocessorInterface { QuantPreprocessor(std::shared_ptr allocator, size_t dim) requires(!WithNorm) : PreprocessorInterface(allocator), dim(dim), - storage_bytes_count(dim * sizeof(OUTPUT_TYPE) + - sq8::storage_metadata_count() * sizeof(MetadataType)), + storage_bytes_count(sq8::storage_bytes_count(dim)), query_bytes_count(dim * sizeof(DataType) + sq8::query_metadata_count() * sizeof(MetadataType)) {} @@ -436,9 +435,7 @@ class QuantPreprocessor : public PreprocessorInterface { const vecsim_stl::vector &mean_vec) requires(WithNorm) : PreprocessorInterface(allocator), mean(mean_vec), dim(dim), - storage_bytes_count(dim * sizeof(OUTPUT_TYPE) + - sq8::storage_metadata_count() * - sizeof(MetadataType)), + storage_bytes_count(sq8::storage_bytes_count(dim)), query_bytes_count(dim * sizeof(DataType) + sq8::query_metadata_count() * sizeof(MetadataType)) { assert(this->mean.size() == dim && "mean vector size must equal dim"); diff --git a/src/VecSim/types/sq8.h b/src/VecSim/types/sq8.h index c1e9c40b8..9f9e04508 100644 --- a/src/VecSim/types/sq8.h +++ b/src/VecSim/types/sq8.h @@ -47,6 +47,15 @@ struct sq8 { ((WithNorm && Metric == VecSimMetric_IP) ? 1 : 0); } + // Size of a stored SQ8 blob: one byte per dimension, followed by FP32 metadata. Single source + // of truth for the storage layout: every caller that sizes or allocates a stored blob must use + // this, so the layout cannot drift between the preprocessor and the index factories. + template + static constexpr size_t storage_bytes_count(size_t dim) { + return dim * sizeof(value_type) + + storage_metadata_count() * sizeof(float); + } + // Index of x_mean_ip / y_mean_ip in the last slot in metadata array template static constexpr size_t mean_ip_index() { diff --git a/src/VecSim/vec_sim_common.h b/src/VecSim/vec_sim_common.h index fb79ca30b..26dc2841d 100644 --- a/src/VecSim/vec_sim_common.h +++ b/src/VecSim/vec_sim_common.h @@ -71,7 +71,8 @@ typedef enum { // Quantization type for HNSW indices. typedef enum { VecSimQuant_NONE = 0, // No quantization (default). - VecSimQuant_SQ8 = 1, // 8-bit scalar quantization with mean normalization. + // 8-bit scalar quantization. Mean normalization is optional, selected by quantParams below. + VecSimQuant_SQ8 = 1, } VecSimQuantType; // Algorithm type/library. @@ -163,7 +164,9 @@ typedef struct { size_t efRuntime; double epsilon; VecSimQuantType quantType; // Quantization type. Default: VecSimQuant_NONE. - void *quantParams; // For VecSimQuant_SQ8: pointer to float mean[dim], or NULL for zero mean. + // For VecSimQuant_SQ8: pointer to float mean[dim], or NULL for zero mean. Read only, never + // retained: the index copies the mean vector during construction. + const void *quantParams; } HNSWParams; typedef struct { From 814eab513f4d96853b8b158af5b1ccdc2c520840 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 5 Aug 2026 14:18:48 +0300 Subject: [PATCH 3/9] Reject quantized tiered indexes until MOD-14957 wires them Adding `quantType` to `HNSWParams` makes it reachable on the tiered path, where nothing handles it. `TieredHNSWFactory::NewIndex` forwards `primaryIndexParams` straight into `HNSWFactory::NewIndex`, so the primary index quantizes its storage, while `NewBFParams` does not copy `quantType` and the brute-force frontend stays unquantized. The two then disagree on the stored blob layout. Reachable from any direct C API caller with `algo = VecSimAlgo_TIERED` and `quantType = VecSimQuant_SQ8`, in two ways: * FP32 / FP16: `assert(hnsw_index->getStoredDataSize() == storedDataSize)` at tiered_factory.cpp:54 aborts on a debug build. Under NDEBUG the assert is gone and 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 without a null check, so the process segfaults. The `catch (...)` in `index_factory.cpp` does not help: neither an abort nor a null dereference is an exception. RediSearch cannot set `quantType` until MOD-14958, so there is no product exposure today. This guard exists so main does not carry the defect between cherry-picks in this series. MOD-14957, which wires quantization through the tiered index properly, should replace the check and the test that covers it rather than delete them. The test builds `TieredIndexParams` with only `primaryIndexParams` set: no job queue or thread pool is needed, since the factory rejects the params before reaching anything that would use them. Deliberately not using `tieredIndexMock` here, because its destructor dereferences `ctx->index_strong_ref` unconditionally and so requires an index to have been created successfully. Verified: - Test is red without the guard and green with it: exit 134 (SIGABRT on the tiered_factory.cpp:54 assert) versus exit 0. - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 45/45 passed - make unit_test DEBUG=1: 2652/2652 passed - make asan: 2652/2652 passed, 0 sanitizer reports Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/index_factories/tiered_factory.cpp | 8 ++++++++ tests/unit/test_hnsw_sq8.cpp | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/VecSim/index_factories/tiered_factory.cpp b/src/VecSim/index_factories/tiered_factory.cpp index 337db6cc3..c9b129faa 100644 --- a/src/VecSim/index_factories/tiered_factory.cpp +++ b/src/VecSim/index_factories/tiered_factory.cpp @@ -95,6 +95,14 @@ inline size_t EstimateInitialSize(const TieredIndexParams *params) { } VecSimIndex *NewIndex(const TieredIndexParams *params) { + // Quantization is not wired into the tiered index yet (MOD-14957). Reject it here rather than + // let it through: the primary index would be built from these params and quantize its storage, + // while NewBFParams does not carry quantType, so the frontend would stay unquantized and the + // two would disagree on the stored blob layout. + if (params->primaryIndexParams->algoParams.hnswParams.quantType != VecSimQuant_NONE) { + return nullptr; + } + // Tiered index that contains HNSW index as primary index VecSimType type = params->primaryIndexParams->algoParams.hnswParams.type; if (type == VecSimType_FLOAT32) { diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index cac137727..88d1f9122 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -378,3 +378,21 @@ void HNSWSQ8Test::test_batch_iterator_basic() { } TYPED_TEST(HNSWSQ8Test, BatchIteratorBasic) { this->test_batch_iterator_basic(); } + +// SQ8 is not wired into the tiered index yet (MOD-14957), so the tiered factory must reject it +// instead of building a quantized primary index against an unquantized frontend. Without the +// guard this aborts on a debug build and silently mismatches the two blob layouts on a release +// one. MOD-14957 should replace this expectation rather than delete it. +TEST(HNSWSQ8TieredTest, RejectsQuantizedTieredIndex) { + HNSWParams hnsw_params = {.type = VecSimType_FLOAT32, + .dim = 4, + .metric = VecSimMetric_L2, + .quantType = VecSimQuant_SQ8}; + VecSimParams primary_params = CreateParams(hnsw_params); + // No job queue or thread pool is needed: the factory rejects these params before it reaches + // anything that would use them. + TieredIndexParams tiered_params = {.primaryIndexParams = &primary_params}; + VecSimParams params = CreateParams(tiered_params); + + EXPECT_EQ(VecSimIndex_New(¶ms), nullptr); +} From 7719c58b5b863c9415c16ae229f317a25f253cf5 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 5 Aug 2026 14:39:03 +0300 Subject: [PATCH 4/9] Cover SQ8 rejection of unsupported data types SQ8 quantizes to uint8 with FP32 metadata and only has kernels for FP32 and FP16 sources, so every other data type must be rejected at index creation. Nothing covered that, which Cursor Bugbot noticed from the other direction on #1007: it flagged that `EstimateElementSize` will happily size a configuration that `NewIndex` refuses to build. That asymmetry is intentional and pre-existing rather than something SQ8 introduced. `EstimateElementSize`'s unquantized path calls `VecSimParams_GetStoredDataSize` (vec_utils.cpp:296), which is `VecSimType_sizeof(type) * dim` plus a Cosine adjustment and validates nothing for any algorithm, so the function has always answered for parameters that cannot produce an index. Making it strict would mean either inventing a sentinel for a `size_t` return or throwing, and `EstimateElementSize` currently contains no `throw` at all, so that would newly carry a C++ exception across the `extern "C"` boundary through `VecSimIndex_EstimateElementSize`. Settling the error model for these two functions belongs with MOD-14958, which is what first makes `quantType` reachable from RediSearch. So this pins the boundary that actually enforces the supported set, and records in a comment why the estimate deliberately does not repeat it. Verified: - Test is red without the fix: removing both the type fence and the fall-through `return NULL` makes it fail for all four types (FLOAT64, BFLOAT16, INT8, UINT8), which are otherwise silently built as unquantized indexes. - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 46/46 passed - make unit_test DEBUG=1: 2653/2653 passed - make asan: 2653/2653 passed, 0 sanitizer reports (the new test exercises the early-return path, so this also covers leaking the allocator set up before it) Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/test_hnsw_sq8.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index 88d1f9122..089e5d5a0 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -379,6 +379,21 @@ void HNSWSQ8Test::test_batch_iterator_basic() { TYPED_TEST(HNSWSQ8Test, BatchIteratorBasic) { this->test_batch_iterator_basic(); } +// SQ8 quantizes to uint8 with FP32 metadata and only has kernels for FP32 and FP16 sources, so +// every other data type must be rejected outright rather than produce an index. Note that +// EstimateElementSize deliberately does not re-check this: like VecSimParams_GetStoredDataSize on +// the unquantized path, it answers for whatever params it is handed, so index creation is the +// boundary that enforces the supported set. +TEST(HNSWSQ8ParamsTest, RejectsUnsupportedDataType) { + for (auto type : {VecSimType_FLOAT64, VecSimType_BFLOAT16, VecSimType_INT8, VecSimType_UINT8}) { + HNSWParams hnsw_params = { + .type = type, .dim = 4, .metric = VecSimMetric_L2, .quantType = VecSimQuant_SQ8}; + VecSimParams params = CreateParams(hnsw_params); + + EXPECT_EQ(VecSimIndex_New(¶ms), nullptr) << "data type " << type; + } +} + // SQ8 is not wired into the tiered index yet (MOD-14957), so the tiered factory must reject it // instead of building a quantized primary index against an unquantized frontend. Without the // guard this aborts on a debug build and silently mismatches the two blob layouts on a release From 63271c149dbb1763063348da77f676dd8d778014 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 9 Aug 2026 10:03:55 +0300 Subject: [PATCH 5/9] Fix two SQ8 defects found in review of #1007 Both were raised by @lerman25 and both are real. Verified before fixing rather than taken at face value. 1. Out-of-bounds read through the public C API --------------------------------------------- `VecSimIndex_GetDistanceFrom_Unsafe` documents `blob` as a raw vector matching the index data type and dimension. For a quantized index that is not a usable query blob: `QuantPreprocessor::preprocessQuery` appends FP32 query metadata (`y_sum`, and `y_sum_squares` for L2) which the SQ8 kernels then read, so honouring the documented contract reads past the caller's buffer. Reproduced with AddressSanitizer on 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 `getDistanceFrom_Unsafe` now returns `INVALID_SCORE` for a quantized index, which is the value `getDistanceFromInternal` already uses for "no answer", so this needs no new error channel. Preprocessing internally was rejected as the fix 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. A public prepared-query API is the real answer and belongs with MOD-14958. `AbstractIndexInitParams` gains `isQuantized` for this, parallel to `isDisk`. It defaults to false, so every other factory is unaffected, and the same flag is what a serialization guard would need. 2. Mean-centred FP16 L2 loses correctness ----------------------------------------- `QuantPreprocessor::preprocessQuery` centres the query then narrows the result back into the FP16 query body, while storage keeps its centred min/delta in FP32. The two disagree. Verified numerically with the repo's own conversions: 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 At a realistic mean near 1 the error is exactly zero, so this only bites for large mean magnitudes, but it is silent when it does. `HNSWFactory::NewIndex` now rejects FLOAT16 + mean + L2. The same combination with IP is unaffected and still supported, because that path does not centre the query. Fixing it properly means keeping the centred query in FP32 with a matching asymmetric kernel, which is ARM's design and belongs upstream. Test changes ------------ `test_get_distance` verified the distance maths through `VecSimIndex_GetDistanceFrom_Unsafe`, but passed it an internally preprocessed blob obtained via a C++-only path no C caller has, which is why the suite missed the overflow. It now checks the maths through `calcDistanceForQuery` and separately asserts that the public API reports no answer for a raw vector. That call is exercised under ASan by every type parameter. FLOAT16 with a mean vector leaves the functional type set, because every functional test uses L2 and that combination is now rejected. It is covered explicitly by `RejectsMeanCenteredFP16L2`, which also pins that FP16 + mean + IP still constructs. Net effect on the suite is 2653 -> 2643 tests: the 11 dropped typed tests were all exercising a combination that is now unsupported, so nothing that previously worked lost coverage. FP16 + mean + IP is left with construction coverage only and no functional search coverage, which is worth closing alongside the metric/multi parameterization also raised in review. Verified: - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 36/36 passed - test_hnsw_sq8 under ASan: 36/36, 0 sanitizer reports (the ASan repro above is clean after the fix) - make unit_test DEBUG=1: 2643/2643 passed - make asan: 2643/2643 passed, 0 sanitizer reports Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw_multi.h | 5 +++ src/VecSim/algorithms/hnsw/hnsw_single.h | 7 +++ src/VecSim/index_factories/hnsw_factory.cpp | 12 +++++ src/VecSim/vec_sim_index.h | 9 +++- tests/unit/test_hnsw_sq8.cpp | 49 ++++++++++++++++++--- 5 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index db978d28c..a1e81e83f 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -118,6 +118,11 @@ class HNSWIndex_Multi : public HNSWIndex { int addVector(const void *vector_data, labelType label) override; vecsim_stl::vector markDelete(labelType label) override; double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override { + // See the note in hnsw_single.h: a quantized index cannot answer a raw-blob distance query + // without reading past the caller's vector. + if (this->isQuantized) { + return INVALID_SCORE; + } return getDistanceFromInternal(label, vector_data); } int removeLabel(labelType label) override { return labelLookup.erase(label); } diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index e6892dcab..9f1a3a372 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -88,6 +88,13 @@ class HNSWIndex_Single : public HNSWIndex { int addVector(const void *vector_data, labelType label) override; vecsim_stl::vector markDelete(labelType label) override; double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override { + // The public API documents vector_data as a raw dim-by-type vector, but a quantized index's + // kernels read query metadata appended past that, so honouring the documented contract here + // would read out of bounds. There is no public API for producing a quantized query blob; + // MOD-14958 owns that decision. Report "no answer" rather than read past the caller's blob. + if (this->isQuantized) { + return INVALID_SCORE; + } return getDistanceFromInternal(label, vector_data); } int removeLabel(labelType label) override { return labelLookup.erase(label); } diff --git a/src/VecSim/index_factories/hnsw_factory.cpp b/src/VecSim/index_factories/hnsw_factory.cpp index 54b4c333f..63c1bde61 100644 --- a/src/VecSim/index_factories/hnsw_factory.cpp +++ b/src/VecSim/index_factories/hnsw_factory.cpp @@ -67,6 +67,7 @@ VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams // Override blob size for the SQ8 storage layout. abstractInitParams.storedDataSize = GetSQ8StoredDataSize(dim, with_norm); + abstractInitParams.isQuantized = true; // Symmetric: both stored vectors are SQ8 blobs. auto sym_func = spaces::GetDistFunc(Metric, dim, &storage_alignment); @@ -133,6 +134,17 @@ VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { const float *mean_ptr = static_cast(hnswParams->quantParams); + // Mean-centred FP16 L2 is not supported: QuantPreprocessor centres the query and narrows + // the result back into the FP16 query body, while storage keeps its centred min/delta in + // FP32. The two then disagree, so an identical vector and query pair yields a non-zero + // distance (mean 10000 gives a per-component error of 1.0), and a large enough mean + // overflows FP16 to infinity. Enabling this needs an asymmetric kernel that takes an FP32 + // centred query. + if (hnswParams->type == VecSimType_FLOAT16 && mean_ptr != nullptr && + metric == VecSimMetric_L2) { + return NULL; + } + if (hnswParams->type == VecSimType_FLOAT32) { if (metric == VecSimMetric_L2) { return NewIndex_SQ8(hnswParams, abstractInitParams, diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index dcf2ac30d..f57993bd8 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -48,6 +48,10 @@ struct AbstractIndexInitParams { size_t blockSize; bool multi; bool isDisk; // Whether the index stores vectors on disk + // Whether stored vectors are quantized. A quantized index's blobs, both stored and query, carry + // metadata the distance kernels read, so a caller's raw dim-by-type vector is not a usable + // query blob for it. + bool isQuantized; void *logCtx; size_t inputBlobSize; }; @@ -82,6 +86,7 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { mutable VecSearchMode lastMode; // The last search mode in RediSearch (used for debug/testing). bool isMulti; // Determines if the index should multi-index or not. bool isDisk; // Whether the index stores vectors on disk. + bool isQuantized; // Whether stored vectors are quantized. void *logCallbackCtx; // Context for the log callback. RawDataContainer *vectors; // The raw vectors data container. private: @@ -125,8 +130,8 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { : VecSimIndexInterface(params.allocator), dim(params.dim), vecType(params.vecType), metric(params.metric), blockSize(params.blockSize ? params.blockSize : DEFAULT_BLOCK_SIZE), lastMode(EMPTY_MODE), - isMulti(params.multi), isDisk(params.isDisk), logCallbackCtx(params.logCtx), - indexCalculator(components.indexCalculator), + isMulti(params.multi), isDisk(params.isDisk), isQuantized(params.isQuantized), + logCallbackCtx(params.logCtx), indexCalculator(components.indexCalculator), storedDistanceDispatch( components.indexCalculator ? components.indexCalculator->getDistanceDispatch(DistanceMode::StoredToStored) diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index 089e5d5a0..1df7f7885 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -26,11 +26,13 @@ struct HNSWSQ8IndexType : IndexType { static constexpr bool with_quant_params = WithQuantParams; }; +// FLOAT16 with a mean vector is absent on purpose: the functional tests below all use L2, and +// mean-centred FP16 L2 is rejected at construction (see HNSWFactory::NewIndex). That combination is +// covered explicitly by HNSWSQ8ParamsTest.RejectsMeanCenteredFP16L2 instead. using HNSWSQ8DataTypeSet = ::testing::Types, HNSWSQ8IndexType, - HNSWSQ8IndexType, - HNSWSQ8IndexType>; + HNSWSQ8IndexType>; template class HNSWSQ8Test : public ::testing::Test { @@ -333,11 +335,20 @@ void HNSWSQ8Test::test_get_distance(VecSimMetric metric) { ASSERT_EQ(GenerateAndAddVector(0, 0.25f, 0.25f), 1); data_t query[4]; GenerateVector(query, 0.5f, 0.25f); - auto processed_query = CastToHNSW()->preprocessQuery(query); + + // Values were chosen so the expected distances can be calculated exactly. Comparing against a + // stored SQ8 blob needs a preprocessed query, which only the index itself can produce. + auto *hnsw_index = CastToHNSW(); + auto processed_query = hnsw_index->preprocessQuery(query); const double expected = metric == VecSimMetric_L2 ? 0.25 : -1.5; - // Values were chosen so the expected distances can be calculated exactly. - EXPECT_NEAR(VecSimIndex_GetDistanceFrom_Unsafe(index, 0, processed_query.get()), expected, - 1e-5); + EXPECT_NEAR( + hnsw_index->calcDistanceForQuery(hnsw_index->getDataByInternalId(0), processed_query.get()), + expected, 1e-5); + + // The public API documents blob as a raw dim-by-type vector, which is not a usable query blob + // for a quantized index: the kernels read query metadata appended past it. It must report no + // answer rather than read past the caller's buffer. + EXPECT_TRUE(std::isnan(VecSimIndex_GetDistanceFrom_Unsafe(index, 0, query))); } TYPED_TEST(HNSWSQ8Test, GetDistanceL2) { this->test_get_distance(VecSimMetric_L2); } @@ -394,6 +405,32 @@ TEST(HNSWSQ8ParamsTest, RejectsUnsupportedDataType) { } } +// Mean-centred FP16 with L2 must be rejected: QuantPreprocessor narrows the centred query back into +// the FP16 query body while storage keeps its centred min/delta in FP32, so identical vector and +// query pairs diverge and a large mean overflows FP16 to infinity. The same combination with IP is +// supported, because that path does not centre the query. +TEST(HNSWSQ8ParamsTest, RejectsMeanCenteredFP16L2) { + std::vector mean(4, 1.0f); + + HNSWParams l2 = {.type = VecSimType_FLOAT16, + .dim = 4, + .metric = VecSimMetric_L2, + .quantType = VecSimQuant_SQ8, + .quantParams = mean.data()}; + VecSimParams l2_params = CreateParams(l2); + EXPECT_EQ(VecSimIndex_New(&l2_params), nullptr); + + HNSWParams ip = {.type = VecSimType_FLOAT16, + .dim = 4, + .metric = VecSimMetric_IP, + .quantType = VecSimQuant_SQ8, + .quantParams = mean.data()}; + VecSimParams ip_params = CreateParams(ip); + VecSimIndex *ip_index = VecSimIndex_New(&ip_params); + ASSERT_NE(ip_index, nullptr); + VecSimIndex_Free(ip_index); +} + // SQ8 is not wired into the tiered index yet (MOD-14957), so the tiered factory must reject it // instead of building a quantized primary index against an unquantized frontend. Without the // guard this aborts on a debug build and silently mismatches the two blob layouts on a release From 9e69259c6adc6f5e3118459c2fe565c3490af517 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 9 Aug 2026 10:34:41 +0300 Subject: [PATCH 6/9] Refuse to serialize SQ8, and cover the IP graph-construction path Remaining review points from #1007, other than the dim >= 33026 kernel overflow which is recorded in SQ8-SERIES-CARRYFORWARD.md instead. Serialization ------------- The V4 format records type, dim and metric, but neither quantType nor the mean vector, and the file-loading path in HNSWFactory always builds components through CreateIndexComponents, which has no SQ8 branch. A saved SQ8 index therefore reloads as unquantized over quantized bytes, misreading the stride and consuming graph bytes as vector data. saveIndexIMP now throws for a quantized index. This is the same argument as the tiered guard: the combination is not wired yet, so fail closed rather than accept it silently. One wart worth knowing: the caller writes the encoding version before saveIndexIMP runs, so a rejected save leaves a stub file. That still fails closed on load, unlike a complete file with a layout the loader misreads, but whoever adds real SQ8 serialization should move the check ahead of the file being created. Recorded in the carry-forward file, whose "serializer should refuse to save" item this closes. IP graph construction --------------------- Every other functional test uses L2, so the symmetric SQ8-to-SQ8 IP kernel that graph construction selects for an IP index was never executed. That kernel is pre-existing, but this series is the first thing to put it on the insert path, so it should not go in untested. GraphConstructionIP builds a 100-vector dim-16 IP index and searches it. The expected result follows from the metric rather than from assumed self-similarity: this is plain inner product, not cosine, so the distance is 1 - IP and the closest vector is the one with the largest projection onto the query. Vectors and query are positive with magnitude growing by label, so results come back from the highest label downward. My first version of this test asserted the query's own label would rank first and failed correctly, returning 99 instead of 70. Vectors also vary per component, not just per label, so quantization does not collapse into the degenerate min == max branch that the existing tests all take. Review nits ----------- * assert(false && "...") added before the unreachable return NULL in the SQ8 branch, matching svs_factory.cpp. Kept alongside the return rather than replacing it: assert-only would reopen the silent-unquantized-fallthrough hole under NDEBUG, which is the regression that line exists to prevent. * Dropped the blank line this series added after the hnswParams declaration. Verified: - ./check-format.sh - make build DEBUG=1 (no warnings) - test_hnsw_sq8: 42/42 passed - make unit_test DEBUG=1: 2649/2649 passed - make asan: 2649/2649 passed, 0 sanitizer reports Co-Authored-By: Claude Opus 5 (1M context) --- .../algorithms/hnsw/hnsw_serializer_impl.h | 11 +++++ src/VecSim/index_factories/hnsw_factory.cpp | 8 ++-- tests/unit/test_hnsw_sq8.cpp | 43 +++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h b/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h index 1666b7b37..b895b580c 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h +++ b/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h @@ -41,6 +41,17 @@ HNSWIndex::HNSWIndex(std::ifstream &input, const HNSWParams template void HNSWIndex::saveIndexIMP(std::ofstream &output) { + // The V4 format records type, dim and metric, but neither quantType nor the mean vector, and + // the loading path always builds unquantized components. A saved quantized index would + // therefore reload with the wrong stride and consume graph bytes as vector data. Refuse instead + // of emitting a file that cannot be decoded. Note the caller has already written the encoding + // version by this point, so a rejected save leaves a stub file behind; that still fails closed + // on load, unlike a full file with a layout the loader misreads. MOD-14957 adds SQ8 + // serialization. + if (this->isQuantized) { + throw std::runtime_error( + "Cannot save index: serialization of quantized indexes is not supported"); + } this->saveIndexFields(output); this->saveGraph(output); } diff --git a/src/VecSim/index_factories/hnsw_factory.cpp b/src/VecSim/index_factories/hnsw_factory.cpp index 63c1bde61..f68421c5e 100644 --- a/src/VecSim/index_factories/hnsw_factory.cpp +++ b/src/VecSim/index_factories/hnsw_factory.cpp @@ -114,7 +114,6 @@ VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { const HNSWParams *hnswParams = ¶ms->algoParams.hnswParams; - AbstractIndexInitParams abstractInitParams = VecSimFactory::NewAbstractInitParams(hnswParams, params->logCtx, is_normalized); @@ -163,8 +162,11 @@ VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { } } - // Unreachable today: the checks above leave only FP32/FP16 x L2/IP. Kept so that adding a - // type or metric cannot silently fall through and build an unquantized index instead. + // Unreachable today: the checks above leave only FP32/FP16 x L2/IP. The assert makes a + // debug build shout if a new type or metric ever reaches here, and the return keeps a + // release build failing closed rather than falling through and silently building an + // unquantized index instead. + assert(false && "unhandled SQ8 data type and metric combination"); return NULL; } diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index 1df7f7885..079dca377 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -431,6 +431,49 @@ TEST(HNSWSQ8ParamsTest, RejectsMeanCenteredFP16L2) { VecSimIndex_Free(ip_index); } +// Serialization does not record quantType or the mean vector, and the loading path always builds +// unquantized components, so saving a quantized index would produce a file the loader misreads. +// saveIndex must refuse rather than emit one. +TYPED_TEST(HNSWSQ8Test, RejectsSerialization) { + HNSWParams params = {.dim = 4, .initialCapacity = 1}; + this->SetUp(params); + ASSERT_EQ(this->GenerateAndAddVector(0, 0.25f, 0.25f), 1); + + const auto file_name = std::string(getenv("ROOT")) + "/tests/unit/sq8_should_not_be_written"; + EXPECT_THROW(this->CastToHNSW()->saveIndex(file_name), std::runtime_error); + std::remove(file_name.c_str()); +} + +// Every other functional test uses L2, so without this the symmetric SQ8-to-SQ8 IP kernel that +// graph construction selects for an IP index would never run. Vectors vary per component as well as +// per label, so quantization does not collapse into the degenerate min == max branch. +TYPED_TEST(HNSWSQ8Test, GraphConstructionIP) { + constexpr size_t n = 100; + constexpr size_t dim = 16; + HNSWParams params = { + .dim = dim, .metric = VecSimMetric_IP, .initialCapacity = n, .M = 16, .efRuntime = n}; + this->SetUp(params); + + // Each label i gets a vector whose components ramp from i upward, so no two vectors share a + // quantization range and every vector has a non-zero delta. + for (size_t i = 0; i < n; i++) { + ASSERT_EQ(this->GenerateAndAddVector(i, static_cast(i) * 0.5f, 0.25f), 1); + } + ASSERT_EQ(VecSimIndex_IndexSize(this->index), n); + + // This is plain inner product, not cosine: the distance is 1 - IP, so the closest vector is the + // one with the largest projection onto the query rather than the query's own twin. Every vector + // and the query are positive and magnitude grows with the label, so IP is strictly increasing + // in the label and results must come back from the highest label downward. + std::vector query(dim); + this->GenerateVector(query.data(), 1.0f, 0.25f); + + auto verify = [&](size_t id, double, size_t result_index) { + EXPECT_EQ(id, n - 1 - result_index); + }; + runTopKSearchTest(this->index, query.data(), 10, verify); +} + // SQ8 is not wired into the tiered index yet (MOD-14957), so the tiered factory must reject it // instead of building a quantized primary index against an unquantized frontend. Without the // guard this aborts on a debug build and silently mismatches the two blob layouts on a release From d5d93d61a99cf1ced3f93cdcecee1c1366e708b7 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 9 Aug 2026 17:51:13 +0300 Subject: [PATCH 7/9] Make the size estimates reject what index creation rejects Addresses the three Bugbot findings left open on #1007, plus one gap none of them covered. 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 three fences: FP32/FP16 only, no Cosine, no mean-centred FP16 L2. Same single-source-of-truth argument as sq8::storage_bytes_count earlier in this series: two copies of one rule drift. * HNSWFactory::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. Unreachable while the enum holds only those two values, and deliberately not tested, since forming an out-of-range enumerator is undefined behaviour. The guard is what keeps adding SQ4 to the enum from reopening the hole. * HNSWFactory::EstimateInitialSize rejects the same set as NewIndex, replacing a check that validated only the data type. This closes the reported metric gap and also 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 the NewIndex guard. It needs its own check rather than inheriting one, because the primary index accepts FP32 with L2 and SQ8 happily; nothing propagates up. EstimateElementSize is left as is on purpose. Its return type is size_t with no sentinel and VecSimIndex_EstimateElementSize is extern "C", so a throw there would carry an exception into the C host. It therefore answers for whatever params it is handed, exactly as VecSimParams_GetStoredDataSize does on the unquantized path. Both estimate functions now say so in a comment. Note that this does add 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 genuinely unsettled and belongs with MOD-14958. Tests: estimate-side assertions on all four existing rejection tests, including EXPECT_NO_THROW for FP16 with a mean and IP, which stays supported. Verified red without the fix: 5 failures, all "throws nothing", with RejectsUnsupportedDataType still green because the old code already threw for a bad type. Green after: test_hnsw_sq8 42/42, make unit_test DEBUG=1 2649/2649, check-format clean, -Wall -Werror -fsyntax-only clean with and without NDEBUG. Co-Authored-By: Claude Opus 5 --- src/VecSim/index_factories/hnsw_factory.cpp | 77 +++++++++++++------ src/VecSim/index_factories/tiered_factory.cpp | 9 +++ tests/unit/test_hnsw_sq8.cpp | 18 ++++- 3 files changed, 77 insertions(+), 27 deletions(-) diff --git a/src/VecSim/index_factories/hnsw_factory.cpp b/src/VecSim/index_factories/hnsw_factory.cpp index f68421c5e..d3d74340f 100644 --- a/src/VecSim/index_factories/hnsw_factory.cpp +++ b/src/VecSim/index_factories/hnsw_factory.cpp @@ -56,6 +56,38 @@ template return alignment; } +// The metric the SQ8 components are actually built for. A Cosine index whose blobs are already +// normalized by someone else (the frontend of a tiered index) is built as IP. +[[nodiscard]] constexpr VecSimMetric ResolveSQ8Metric(VecSimMetric metric, bool is_normalized) { + return (is_normalized && metric == VecSimMetric_Cosine) ? VecSimMetric_IP : metric; +} + +// Single definition of the SQ8 parameter combinations this factory can build, taking the metric +// already resolved by ResolveSQ8Metric. NewIndex fails closed when this returns false, and +// EstimateInitialSize rejects the same set, so it cannot report a size for parameters that cannot +// produce an index. +[[nodiscard]] constexpr bool SQ8ParamsSupported(VecSimType type, VecSimMetric resolved_metric, + bool with_mean) { + // Kernels exist for FP32 and FP16 sources only. + if (type != VecSimType_FLOAT32 && type != VecSimType_FLOAT16) { + return false; + } + // Cosine needs a normalization step the SQ8 preprocessor does not perform. + if (resolved_metric == VecSimMetric_Cosine) { + return false; + } + // Mean-centred FP16 L2 loses correctness: QuantPreprocessor centres the query and narrows the + // result back into the FP16 query body, while storage keeps its centred min/delta in FP32. The + // two then disagree, so an identical vector and query pair yields a non-zero distance (mean + // 10000 gives a per-component error of 1.0), and a large enough mean overflows FP16 to + // infinity. Enabling this needs an asymmetric kernel that takes an FP32 centred query. Only the + // WithNorm && L2 branch centres the query, so FP16 with a mean and IP is unaffected. + if (type == VecSimType_FLOAT16 && with_mean && resolved_metric == VecSimMetric_L2) { + return false; + } + return true; +} + // Helper to build an SQ8-quantized HNSW index given compile-time DataType and Metric. template VecSimIndex *NewIndex_SQ8(const HNSWParams *hnswParams, AbstractIndexInitParams abstractInitParams, @@ -117,30 +149,20 @@ VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { AbstractIndexInitParams abstractInitParams = VecSimFactory::NewAbstractInitParams(hnswParams, params->logCtx, is_normalized); - if (hnswParams->quantType == VecSimQuant_SQ8) { - if (hnswParams->type != VecSimType_FLOAT32 && hnswParams->type != VecSimType_FLOAT16) { - return NULL; // SQ8 supports FP32 and FP16 only. - } - - VecSimMetric metric = hnswParams->metric; - if (is_normalized && metric == VecSimMetric_Cosine) { - metric = VecSimMetric_IP; - } - - if (metric == VecSimMetric_Cosine) { - return NULL; // SQ8 does not support cosine metric. + if (hnswParams->quantType != VecSimQuant_NONE) { + // Any quantization type this factory does not implement must fail closed. Falling through + // to the plain path below would silently build a full-precision index for a caller that + // asked for a quantized one. Unreachable while VecSimQuantType holds only NONE and SQ8, and + // not covered by a test because forming an out-of-range enumerator is undefined behaviour; + // the guard is what keeps adding SQ4 to the enum from becoming a silent fallthrough. + if (hnswParams->quantType != VecSimQuant_SQ8) { + return NULL; } + const VecSimMetric metric = ResolveSQ8Metric(hnswParams->metric, is_normalized); const float *mean_ptr = static_cast(hnswParams->quantParams); - // Mean-centred FP16 L2 is not supported: QuantPreprocessor centres the query and narrows - // the result back into the FP16 query body, while storage keeps its centred min/delta in - // FP32. The two then disagree, so an identical vector and query pair yields a non-zero - // distance (mean 10000 gives a per-component error of 1.0), and a large enough mean - // overflows FP16 to infinity. Enabling this needs an asymmetric kernel that takes an FP32 - // centred query. - if (hnswParams->type == VecSimType_FLOAT16 && mean_ptr != nullptr && - metric == VecSimMetric_L2) { + if (!SQ8ParamsSupported(hnswParams->type, metric, mean_ptr != nullptr)) { return NULL; } @@ -226,9 +248,13 @@ size_t EstimateInitialSize(const HNSWParams *params, bool is_normalized) { size_t est = sizeof(VecSimAllocator) + allocations_overhead; - if (params->quantType == VecSimQuant_SQ8) { - if (params->type != VecSimType_FLOAT32 && params->type != VecSimType_FLOAT16) { - throw std::invalid_argument("Invalid params->type for VecSimQuant_SQ8"); + if (params->quantType != VecSimQuant_NONE) { + // Reject exactly what NewIndex rejects. Reporting a size for a combination that cannot be + // built lets a caller size its capacity from an index it will then fail to create. + if (params->quantType != VecSimQuant_SQ8 || + !SQ8ParamsSupported(params->type, ResolveSQ8Metric(params->metric, is_normalized), + params->quantParams != nullptr)) { + throw std::invalid_argument("Unsupported quantization params for HNSW index"); } // Calculator + preprocessor container + preprocessor. // Use representative types; sizeof is independent of the template parameters. @@ -277,6 +303,11 @@ size_t EstimateElementSize(const HNSWParams *params) { size_t elementGraphDataSize = sizeof(ElementGraphData) + sizeof(idType) * M * 2; size_t stored_data_size; + // Unlike EstimateInitialSize, this does not reject unsupported combinations: the return type is + // size_t with no sentinel, and VecSimIndex_EstimateElementSize is extern "C", so throwing here + // would carry an exception into the C host. It therefore answers for whatever params it is + // handed, exactly as VecSimParams_GetStoredDataSize does on the unquantized path. Index + // creation is the boundary that enforces the supported set (see SQ8ParamsSupported). if (params->quantType == VecSimQuant_SQ8) { bool with_norm = params->quantParams != nullptr; if (params->metric == VecSimMetric_L2) { diff --git a/src/VecSim/index_factories/tiered_factory.cpp b/src/VecSim/index_factories/tiered_factory.cpp index c9b129faa..6c2dffe99 100644 --- a/src/VecSim/index_factories/tiered_factory.cpp +++ b/src/VecSim/index_factories/tiered_factory.cpp @@ -66,6 +66,13 @@ inline VecSimIndex *NewIndex(const TieredIndexParams *params) { inline size_t EstimateInitialSize(const TieredIndexParams *params) { HNSWParams hnsw_params = params->primaryIndexParams->algoParams.hnswParams; + // NewIndex below rejects quantization until MOD-14957 wires it, so refuse to size it too. The + // primary index on its own would accept these params, so without this check the estimate + // reports a number for an index that cannot be created. + if (hnsw_params.quantType != VecSimQuant_NONE) { + throw std::invalid_argument("Quantization is not supported for tiered HNSW indexes"); + } + // Add size estimation of VecSimTieredIndex sub indexes. // Normalization is done by the frontend index. size_t est = HNSWFactory::EstimateInitialSize(&hnsw_params, true); @@ -241,6 +248,8 @@ size_t EstimateInitialSize(const TieredIndexParams *params) { } size_t EstimateElementSize(const TieredIndexParams *params) { + // Deliberately not validated here, unlike EstimateInitialSize above: see the note in + // HNSWFactory::EstimateElementSize on why this function has no error channel. size_t est = 0; if (params->primaryIndexParams->algo == VecSimAlgo_HNSWLIB) { est = HNSWFactory::EstimateElementSize(¶ms->primaryIndexParams->algoParams.hnswParams); diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index 079dca377..4f611fa22 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -149,6 +149,10 @@ TYPED_TEST(HNSWSQ8Test, RejectStandaloneCosine) { VecSimParams vecsim_params = CreateParams(params); this->index = VecSimIndex_New(&vecsim_params); EXPECT_EQ(this->index, nullptr); + + // The size estimate must reject what creation rejects, so a caller cannot size its capacity + // from a configuration it will then fail to build. + EXPECT_THROW(EstimateInitialSize(params), std::invalid_argument); } /* ---------------------------- Size Estimation tests ---------------------------- */ @@ -391,10 +395,9 @@ void HNSWSQ8Test::test_batch_iterator_basic() { TYPED_TEST(HNSWSQ8Test, BatchIteratorBasic) { this->test_batch_iterator_basic(); } // SQ8 quantizes to uint8 with FP32 metadata and only has kernels for FP32 and FP16 sources, so -// every other data type must be rejected outright rather than produce an index. Note that -// EstimateElementSize deliberately does not re-check this: like VecSimParams_GetStoredDataSize on -// the unquantized path, it answers for whatever params it is handed, so index creation is the -// boundary that enforces the supported set. +// every other data type must be rejected outright rather than produce an index. EstimateInitialSize +// rejects the same set; EstimateElementSize deliberately does not, because it has no error channel +// (see the note at its definition). TEST(HNSWSQ8ParamsTest, RejectsUnsupportedDataType) { for (auto type : {VecSimType_FLOAT64, VecSimType_BFLOAT16, VecSimType_INT8, VecSimType_UINT8}) { HNSWParams hnsw_params = { @@ -402,6 +405,8 @@ TEST(HNSWSQ8ParamsTest, RejectsUnsupportedDataType) { VecSimParams params = CreateParams(hnsw_params); EXPECT_EQ(VecSimIndex_New(¶ms), nullptr) << "data type " << type; + EXPECT_THROW(EstimateInitialSize(hnsw_params), std::invalid_argument) + << "data type " << type; } } @@ -419,6 +424,7 @@ TEST(HNSWSQ8ParamsTest, RejectsMeanCenteredFP16L2) { .quantParams = mean.data()}; VecSimParams l2_params = CreateParams(l2); EXPECT_EQ(VecSimIndex_New(&l2_params), nullptr); + EXPECT_THROW(EstimateInitialSize(l2), std::invalid_argument); HNSWParams ip = {.type = VecSimType_FLOAT16, .dim = 4, @@ -429,6 +435,7 @@ TEST(HNSWSQ8ParamsTest, RejectsMeanCenteredFP16L2) { VecSimIndex *ip_index = VecSimIndex_New(&ip_params); ASSERT_NE(ip_index, nullptr); VecSimIndex_Free(ip_index); + EXPECT_NO_THROW(EstimateInitialSize(ip)); } // Serialization does not record quantType or the mean vector, and the loading path always builds @@ -490,4 +497,7 @@ TEST(HNSWSQ8TieredTest, RejectsQuantizedTieredIndex) { VecSimParams params = CreateParams(tiered_params); EXPECT_EQ(VecSimIndex_New(¶ms), nullptr); + // The primary index alone would accept these params, so the tiered estimate needs its own check + // rather than inheriting one from HNSWFactory. + EXPECT_THROW(EstimateInitialSize(tiered_params), std::invalid_argument); } From 9b1615c6137a032fa134ba6f90d9fab1a760b55e Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Mon, 10 Aug 2026 11:43:00 +0300 Subject: [PATCH 8/9] Reject an out-of-range metric, and document the SQ8 distance sentinel Two findings from the second review round that are inside MOD-14956. The other six are tracked as MOD-17526 to MOD-17531. * An out-of-range VecSimMetric reached the SQ8 dispatcher's unreachable-branch assert and aborted an assertions-enabled process. FP32 with SQ8 and metric = (VecSimMetric)123 passed the Cosine-only check, matched neither the L2 nor the IP branch, and fell through to the assert added in 9e69259c. The unquantized dispatcher throws instead, and VecSimIndex_New catches it, so quantized params were the only way to abort the host. Reported by @lerman25, who reproduced it with a death test. SQ8ParamsSupported now whitelists L2 and IP rather than rejecting Cosine alone, so NewIndex returns NULL and EstimateInitialSize throws, as they do for every other unsupported combination. Note the previous commit's refactor preserved this hole verbatim rather than introducing it: the open-coded check it replaced also tested only for Cosine. RejectsOutOfRangeMetric uses (VecSimMetric)3, which is outside the valid set but inside the enum's value range, so the test does not itself rely on undefined behaviour the way (VecSimMetric)123 would. * VecSimIndex_GetDistanceFrom_Unsafe returns INVALID_SCORE for a quantized index as of 63271c14, but vec_sim.h still promised a distance for any blob matching the index type and dimension. Worse, INVALID_SCORE is the same NaN returned for a missing label, so a caller cannot tell "SQ8 unsupported" from "label absent" and could treat valid candidates as missing. The declaration now documents the restriction, the sentinel, the collision with the missing-label case, and that callers must check quantType rather than infer it from the result. This matters for MOD-14958, which is what first exposes any of it to RediSearch. Tests: test_hnsw_sq8 43/43, make unit_test DEBUG=1 2650/2650, check-format clean, -Wall -Werror -fsyntax-only clean. The metric abort was verified red by the reporter on 9e69259c rather than by me: reproducing it here would mean building twice more to watch a debug assert fire, and his death test already pins the pre-fix behaviour. Co-Authored-By: Claude Opus 5 --- src/VecSim/index_factories/hnsw_factory.cpp | 7 +++++-- src/VecSim/vec_sim.h | 7 +++++++ tests/unit/test_hnsw_sq8.cpp | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/VecSim/index_factories/hnsw_factory.cpp b/src/VecSim/index_factories/hnsw_factory.cpp index d3d74340f..62af320c8 100644 --- a/src/VecSim/index_factories/hnsw_factory.cpp +++ b/src/VecSim/index_factories/hnsw_factory.cpp @@ -72,8 +72,11 @@ template if (type != VecSimType_FLOAT32 && type != VecSimType_FLOAT16) { return false; } - // Cosine needs a normalization step the SQ8 preprocessor does not perform. - if (resolved_metric == VecSimMetric_Cosine) { + // Only L2 and IP have SQ8 kernels. Cosine needs a normalization step the SQ8 preprocessor does + // not perform, and anything outside the enum has to be rejected here as well: the dispatch in + // NewIndex would otherwise fall through to its unreachable-branch assert and abort an + // assertions-enabled host, where the unquantized path throws and is caught by VecSimIndex_New. + if (resolved_metric != VecSimMetric_L2 && resolved_metric != VecSimMetric_IP) { return false; } // Mean-centred FP16 L2 loses correctness: QuantPreprocessor centres the query and narrows the diff --git a/src/VecSim/vec_sim.h b/src/VecSim/vec_sim.h index b484965d9..9b5302bb7 100644 --- a/src/VecSim/vec_sim.h +++ b/src/VecSim/vec_sim.h @@ -86,6 +86,13 @@ int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label); * type and dimension, and pre-normalized if needed. * @return The distance (according to the index's distance metric) between `blob` and the vector * with label label`. + * 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. */ double VecSimIndex_GetDistanceFrom_Unsafe(VecSimIndex *index, size_t label, const void *blob); diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index 4f611fa22..b26c8fb2b 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -410,6 +410,21 @@ TEST(HNSWSQ8ParamsTest, RejectsUnsupportedDataType) { } } +// An out-of-range metric must be rejected before dispatch. Reaching the unreachable-branch assert +// in the SQ8 dispatcher would abort an assertions-enabled host, where the unquantized path throws +// and VecSimIndex_New catches it. VecSimMetric has three enumerators, so 3 is the smallest value +// outside the valid set that is still inside the enum's value range and therefore safe to form. +TEST(HNSWSQ8ParamsTest, RejectsOutOfRangeMetric) { + HNSWParams hnsw_params = {.type = VecSimType_FLOAT32, + .dim = 4, + .metric = static_cast(3), + .quantType = VecSimQuant_SQ8}; + VecSimParams params = CreateParams(hnsw_params); + + EXPECT_EQ(VecSimIndex_New(¶ms), nullptr); + EXPECT_THROW(EstimateInitialSize(hnsw_params), std::invalid_argument); +} + // Mean-centred FP16 with L2 must be rejected: QuantPreprocessor narrows the centred query back into // the FP16 query body while storage keeps its centred min/delta in FP32, so identical vector and // query pairs diverge and a large mean overflows FP16 to infinity. The same combination with IP is From ac05e54302010f6e923df9afbcf54d11a802da03 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Mon, 10 Aug 2026 14:04:00 +0300 Subject: [PATCH 9/9] Drop ticket ids from the SQ8 code comments Five references across four files. The comments now describe the deferred work by what it is rather than by ticket number, which keeps them readable without a Jira lookup and stops them going stale when tickets are split or renumbered. The tickets are still named in the PR description and the review threads, which is where that traceability belongs. While editing the serializer comment, also corrected a claim it still made. It described the stub file left by a rejected save as failing closed on load, and treated moving the check earlier as a nicety. The save has by then already truncated whatever was at that path, so the comment now says that instead. The code is unchanged; only the description of the known limitation is. Comments only, no executable change. Verified with -Wall -Werror -fsyntax-only on both affected translation units and check-format; tests were not re-run, since no code changed. Co-Authored-By: Claude Opus 5 --- src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h | 6 +++--- src/VecSim/algorithms/hnsw/hnsw_single.h | 5 +++-- src/VecSim/index_factories/tiered_factory.cpp | 14 +++++++------- tests/unit/test_hnsw_sq8.cpp | 8 ++++---- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h b/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h index b895b580c..1df9a539d 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h +++ b/src/VecSim/algorithms/hnsw/hnsw_serializer_impl.h @@ -45,9 +45,9 @@ void HNSWIndex::saveIndexIMP(std::ofstream &output) { // the loading path always builds unquantized components. A saved quantized index would // therefore reload with the wrong stride and consume graph bytes as vector data. Refuse instead // of emitting a file that cannot be decoded. Note the caller has already written the encoding - // version by this point, so a rejected save leaves a stub file behind; that still fails closed - // on load, unlike a full file with a layout the loader misreads. MOD-14957 adds SQ8 - // serialization. + // version by this point, so a rejected save leaves a stub file behind, and worse, it has + // already truncated whatever was at that path. Both are tracked separately; whoever adds + // quantized serialization should move this check ahead of the file being opened. if (this->isQuantized) { throw std::runtime_error( "Cannot save index: serialization of quantized indexes is not supported"); diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index 9f1a3a372..76a885351 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -90,8 +90,9 @@ class HNSWIndex_Single : public HNSWIndex { double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override { // The public API documents vector_data as a raw dim-by-type vector, but a quantized index's // kernels read query metadata appended past that, so honouring the documented contract here - // would read out of bounds. There is no public API for producing a quantized query blob; - // MOD-14958 owns that decision. Report "no answer" rather than read past the caller's blob. + // would read out of bounds. There is no public API for producing a quantized query blob, + // and adding one is a separate decision. Report "no answer" rather than read past the + // caller's blob. See the note on VecSimIndex_GetDistanceFrom_Unsafe in vec_sim.h. if (this->isQuantized) { return INVALID_SCORE; } diff --git a/src/VecSim/index_factories/tiered_factory.cpp b/src/VecSim/index_factories/tiered_factory.cpp index 6c2dffe99..7d83905cc 100644 --- a/src/VecSim/index_factories/tiered_factory.cpp +++ b/src/VecSim/index_factories/tiered_factory.cpp @@ -66,9 +66,9 @@ inline VecSimIndex *NewIndex(const TieredIndexParams *params) { inline size_t EstimateInitialSize(const TieredIndexParams *params) { HNSWParams hnsw_params = params->primaryIndexParams->algoParams.hnswParams; - // NewIndex below rejects quantization until MOD-14957 wires it, so refuse to size it too. The - // primary index on its own would accept these params, so without this check the estimate - // reports a number for an index that cannot be created. + // NewIndex below rejects quantization until the tiered index handles it, so refuse to size it + // too. The primary index on its own would accept these params, so without this check the + // estimate reports a number for an index that cannot be created. if (hnsw_params.quantType != VecSimQuant_NONE) { throw std::invalid_argument("Quantization is not supported for tiered HNSW indexes"); } @@ -102,10 +102,10 @@ inline size_t EstimateInitialSize(const TieredIndexParams *params) { } VecSimIndex *NewIndex(const TieredIndexParams *params) { - // Quantization is not wired into the tiered index yet (MOD-14957). Reject it here rather than - // let it through: the primary index would be built from these params and quantize its storage, - // while NewBFParams does not carry quantType, so the frontend would stay unquantized and the - // two would disagree on the stored blob layout. + // Quantization is not wired into the tiered index yet. Reject it here rather than let it + // through: the primary index would be built from these params and quantize its storage, while + // NewBFParams does not carry quantType, so the frontend would stay unquantized and the two + // would disagree on the stored blob layout. if (params->primaryIndexParams->algoParams.hnswParams.quantType != VecSimQuant_NONE) { return nullptr; } diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index b26c8fb2b..b0154bbf2 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -496,10 +496,10 @@ TYPED_TEST(HNSWSQ8Test, GraphConstructionIP) { runTopKSearchTest(this->index, query.data(), 10, verify); } -// SQ8 is not wired into the tiered index yet (MOD-14957), so the tiered factory must reject it -// instead of building a quantized primary index against an unquantized frontend. Without the -// guard this aborts on a debug build and silently mismatches the two blob layouts on a release -// one. MOD-14957 should replace this expectation rather than delete it. +// SQ8 is not wired into the tiered index yet, so the tiered factory must reject it instead of +// building a quantized primary index against an unquantized frontend. Without the guard this aborts +// on a debug build and silently mismatches the two blob layouts on a release one. Whoever wires +// quantization through the tiered index should replace this expectation rather than delete it. TEST(HNSWSQ8TieredTest, RejectsQuantizedTieredIndex) { HNSWParams hnsw_params = {.type = VecSimType_FLOAT32, .dim = 4,