Skip to content

Commit 6d567ba

Browse files
committed
Added comments according to code review requests.
1 parent 1eb1cbf commit 6d567ba

File tree

3 files changed

+17
-2
lines changed

3 files changed

+17
-2
lines changed

src/VecSim/algorithms/svs/svs_utils.h

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,13 @@ inline bool check_cpuid() {
150150
}
151151
// clang-format on
152152

153-
// Check if the SVS implementation supports Qquantization mode
153+
// Check if the SVS implementation supports Quantization mode
154154
// @param quant_bits requested SVS quantization mode
155155
// @return pair<fallbackMode, bool>
156+
// @note even if VecSimSvsQuantBits is a simple enum value,
157+
// in theory, it can be a complex type with a combination of modes:
158+
// - primary bits, secondary/residual bits, dimesionality reduction, etc.
159+
// which can be incompatible to each-other.
156160
inline std::pair<VecSimSvsQuantBits, bool> isSVSQuantBitsSupported(VecSimSvsQuantBits quant_bits) {
157161
// If HAVE_SVS_LVQ is not defined, we don't support any quantization mode
158162
// else we check if the CPU supports SVS LVQ
@@ -163,9 +167,13 @@ inline std::pair<VecSimSvsQuantBits, bool> isSVSQuantBitsSupported(VecSimSvsQuan
163167
;
164168

165169
// If the quantization mode is not supported, we fallback to non-quantized mode
170+
// - this is temporary solution until we have a basic quantization mode in SVS
171+
// TODO: use basic SVS quantization as a fallback for unsupported modes
166172
auto fallBack = supported ? quant_bits : VecSimSvsQuant_NONE;
167173

168-
// And always return true, as far as non-quantized mode is always supported
174+
// And always return true, as far as fallback mode is always supported
175+
// Upon further decision changes, some cases should treated as not-supported
176+
// So we will need return false.
169177
return std::make_pair(fallBack, true);
170178
}
171179
} // namespace svs_details

src/VecSim/index_factories/svs_factory.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,9 @@ size_t EstimateInitialSize(const SVSParams *params, bool is_normalized) {
181181

182182
} // namespace SVSFactory
183183

184+
// This is a temporary solution to avoid breaking the build when SVS is not available
185+
// and to allow the code to compile without SVS support.
186+
// TODO: remove HAVE_SVS when SVS will support all Redis platfoms and compilers
184187
#else // HAVE_SVS
185188
namespace SVSFactory {
186189
VecSimIndex *NewIndex(const VecSimParams *params, bool is_normalized) { return NULL; }

tests/unit/test_svs.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66
#if HAVE_SVS
77
#include "VecSim/algorithms/svs/svs.h"
88

9+
// There are possible cases when SVS Index cannot be created with the requested quantization mode
10+
// due to platform and/or hardware limitations or combination of requested 'compression' modes.
11+
// This assert handle those cases and skip a test if the mode is not supported.
12+
// Elsewhere, test will fail if the index creation failed with no reason explained above.
913
#define ASSERT_INDEX(index) \
1014
if (index == nullptr) { \
1115
if (std::get<1>(svs_details::isSVSQuantBitsSupported(TypeParam::get_quant_bits()))) { \

0 commit comments

Comments
 (0)