[MOD-17528] Make the SQ8 quantized byte conversion defined for all finite input

[dor-forer]

Describe the changes in the pull request

QuantPreprocessor::quantize could execute undefined behaviour for input the API accepts. Three paths, all ending at the same conversion to a byte, and all reachable with entirely finite components.

The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range overflowed: max - min became inf, delta inf, inv_delta 0, and the per-element product inf * 0 = NaN, whose conversion to an integer is undefined. UBSan: -nan is outside the range of representable values of type 'unsigned char'. The existing diff == 0 guard covers equal values, not overflow of the subtraction.

With WithNorm, centring is an FP32 subtraction, so finite input against a finite mean reaches inf before any range is computed: FLT_MAX against mean -FLT_MAX centres to 6.8e38. Widening the range does not help, because the value is already lost upstream.

And delta is stored as FP32, so (float)(diff / 255) underflows to 0 while diff itself is nonzero, leaving 1/delta inf and scaling the minimum element, numerator exactly zero, to 0 * inf.

A fourth path, raised by Cursor on this PR: transformed_value clamps a centered storage component, but preprocessQuery centered by hand with no clamp, so storage held FLT_MAX where the query held inf. The query metadata derives from those components, so one overflowing component poisons every distance computed against that query and asymmetric L2 could return NaN from 0 * inf. The query loop now calls transformed_value, so both paths share one transform rather than two copies that have to agree.

Stack position 2 of 3, base dor-forer-MOD-17527-uint8-accumulators (#1014). Review the top two commits; GitHub retargets this to main when #1014 merges.

Which issues this PR fixes

1. MOD-17528
2. Two further paths to the same undefined behaviour, raised in review of [MOD-17526][MOD-17527] Make SQ8 metadata exact and fix the symmetric L2 formulation #1011 by @lerman25 (FP32 centring overflow) and Cursor Bugbot (FP32 delta underflow). preprocessors.h moves here, so the fixes and their tests live in this PR.

Main objects this PR modified

1. QuantPreprocessor::find_min_max — now carries the postcondition that both endpoints are finite and ordered.
2. QuantPreprocessor::transformed_value — applies the same clamp, so the postcondition holds for every element and not only the endpoints.
3. QuantPreprocessor::quantize — double range, one delta guard, and a to_byte that saturates without std::round.
4. QuantPreprocessor::preprocessQuery: centers through transformed_value, so the query carries the same clamp as storage.
5. tests/unit/test_components.cpp.

Mark if applicable

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

---

The fix

All three paths close by giving find_min_max a postcondition — both endpoints finite, min <= max — established where it can be broken rather than cleaned up at the call site. The WithNorm branch creates the inf itself from two valid operands; the plain branch passes through whatever the input holds.

if (!(min_val <= max_val)) {            // false when either endpoint is NaN
    return {0.0f, 0.0f};
}
return {std::clamp(min_val, -representable_float, representable_float),
        std::clamp(max_val, -representable_float, representable_float)};

Two-sided on both endpoints, deliberately: for an all-+inf vector inf <= inf passes the order check, so a one-sided std::max would leave min at +inf and store it. min is an FP32 field, so a non-finite endpoint could not be represented under any arithmetic — this is the storage limit as much as a guard.

transformed_value applies the same clamp (Cursor Bugbot, this PR). Clamping only the endpoints left elements outside them: when every centred value overflows the same way, both endpoints collapse to one clamped value — a constant range — but the element was still ±inf, scaled to ±inf, and saturated:

                      endpoints only        both clamped
all centre to +inf    {255, 255}      ->     {0, 0}
all centre to -inf    {0,   0}        ->     {0, 0}

A non-constant code for a constant range, and asymmetric between the two signs, reachable from finite input against a finite mean. The bound is a single shared representable_float member so the two clamps cannot drift apart, which was the underlying fragility.

Why inv_delta stays double

Not for precision — the byte only needs ±0.5 in 255, where FP32 gives 6e-8. It is because an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta = 2.7e-39, whose FP64 reciprocal 3.6e38 is finite and correctly maps the top element to exactly 255.000. Pinned by the subnormal_delta_survives case.

The per-element bound

const double bounded = scaled > 0.0 ? scaled : 0.0;
return static_cast<OUTPUT_TYPE>((bounded > 255.0 ? 255.0 : bounded) + 0.5);

form	instructions (-O3)	libm calls
std::clamp + std::round	9	1
fmin(fmax(...)) + std::round	10	3
this	8	0

Bounding first makes +0.5 and truncation equivalent to std::round for non-negative values, which these are. std::round is an out-of-line libm call at this translation unit's baseline and ran once per element.

Scope, stated precisely

• The byte conversion is defined for all finite components. That is the goal and it is met.
• The stored min and delta are finite with delta > 0. The sums are not covered — accumulated in FP32 over the input values, so [-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this function's own arithmetic is now well defined. Separate problem, separate change. The parameterized test is named ScaleMetadataAndBytesAreAsExpected for that reason.
• Non-finite components are unsupported, and nothing here is tested against them. std::minmax_element requires its comparison to induce a strict weak ordering, and floating-point < is not one once a NaN is present: incomparability must be transitive, yet 1.0 is incomparable with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating that precondition is undefined behaviour inside the algorithm, before any range exists, so no assertion about the outcome would be portable — including a weak one about metadata finiteness, and UBSan reporting nothing does not establish otherwise. An earlier revision had such a test; it was removed.
• Metadata meaning is unchanged. The sums are still FP32 over the input. Making them exact integer sums over the quantized bytes is a storage-contract change that must move with every kernel reading them, and stays in [MOD-17526][MOD-17527] Make SQ8 metadata exact and fix the symmetric L2 formulation #1011.

The order check on the endpoints remains as an invariant guard, not a NaN policy: everything downstream assumes min <= max, and asserting it once per vector costs less than proving it cannot be violated. It also means a NaN endpoint cannot be stored, so a caller error degrades to meaningless-but-finite metadata rather than poisoning every distance against that vector.

Rejecting non-finite components at the public ingestion boundary is the actual fix and is filed separately. Nothing in VecSim does it today — grep -niE "isfinite|isnan" over vec_sim.cpp and vec_sim_index.h is empty.

How comparable systems handle it

	non-finite policy	zero range
Lucene	validates components, throws on NaN/inf	explicit equal-range handling
Elasticsearch	rejects NaN, inf, and overflowing magnitudes	delegates to Lucene
Faiss	assumed finite	explicit vdiff != 0
Qdrant	no validation; defined bytes only via Rust's saturating cast, which C++ lacks	none, can store an unusable scale

None silently quantizes non-finite input, which is why this PR does not either.

Tests

The MOD-17528 reproduction, the WithNorm centring case, QuantizationCollapsedCenteredRangeIsConstantAndSymmetric (both signs, so the asymmetry above cannot return), and a table-driven matrix over finite input only: constant vectors positive/negative/zero, a single element, a subnormal but representable delta, a range that underflows and collapses, and the full FP32 range.

Expected bytes and scale metadata are asserted rather than a range check, which is vacuous for uint8_t. Every expectation was derived by simulating the pipeline, not predicted — which corrected three of them, including that the full-range midpoint is 128, not 127, because 127.5 + 0.5 truncates up.

Known coverage gap

One uncovered line: the NaN fallback return {0.0f, 0.0f};. It is unreachable for supported input — both branches produce min <= max by construction for finite values — so reaching it requires the NaN input that makes the std::minmax_element call undefined in the first place. Deleting the guard is worse: std::clamp propagates NaN, so min would be stored as NaN. Both codecov statuses pass (patch 95.83% against a 5% threshold). Noted in the code so it is not re-filed or "fixed" by removing the guard.

Verification

-fsyntax-only -Wall -Wextra clean on QuantPreprocessor<float, L2, false>, <float, IP, true> and <float16, L2, false>; check-format.sh clean. Moving the normalization into find_min_max also reduced codegen: <float, IP, true> 231 → 222 instructions, <float16, L2, false> 327 → 304, <float, L2, false> unchanged.

Not built or executed locally. CI is the first real check, and the UBSan job is the one that matters here.

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches the hot preprocessForStorage / preprocessQuery path for SQ8 indices on every add and search, but changes are narrowly scoped to numerical edge cases and align query with storage rather than altering the public API.

Overview
Fixes undefined behavior and NaN-prone SQ8 preprocessing on vector add/query paths for inputs the API already accepts (including ±FLT_MAX and normalized L2 centering).

QuantPreprocessor::quantize now computes range and scale in double, forces a positive delta when FP32 would underflow, uses a to_byte saturating round (no std::round / inf * 0), and documents that only finite components are supported while min/delta must stay finite (sum metadata overflow is explicitly out of scope).

find_min_max and transformed_value share an FLT_MAX clamp so every centered element stays within the stored range; preprocessQuery for normalized L2 reuses transformed_value so query bodies match storage instead of leaving ±inf that broke asymmetric L2.

Unit tests add UBSan-focused regressions (full FP32 range, subnormal delta, collapsed centered ranges, query/storage clamp parity) plus a parameterized finite-input matrix for expected bytes and scale metadata.

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

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.18%. Comparing base (e2a83f9) to head (40659e8).

⚠️ Current head 40659e8 differs from pull request most recent head f959a60

Please upload reports for the commit f959a60 to get more accurate results.

Files with missing lines	Patch %	Lines
src/VecSim/spaces/computer/preprocessors.h	95.83%	1 Missing ⚠️

Additional details and impacted files

@@                            Coverage Diff                             @@
##           dor-forer-MOD-17527-uint8-accumulators    #1015      +/-   ##
==========================================================================
- Coverage                                   97.19%   97.18%   -0.01%     
==========================================================================
  Files                                         142      142              
  Lines                                        8381     8392      +11     
==========================================================================
+ Hits                                         8146     8156      +10     
- Misses                                        235      236       +1     

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

🚀 New features to boost your workflow:

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

[dor-forer]

On the codecov annotation: the one uncovered line is the NaN fallback in find_min_max, and it is uncovered on purpose.

if (!(min_val <= max_val)) {
    return {0.0f, 0.0f};
}

It is unreachable for supported input. Both branches above produce min <= max by construction for finite values: the plain branch takes them from std::minmax_element, which returns elements of the range, and the WithNorm branch builds them with std::min/std::max in a loop. So the condition is false only when an endpoint is NaN.

Reaching it therefore requires NaN input, which is exactly what makes the std::minmax_element call above undefined: its precondition is that the comparison induce a strict weak ordering, and floating-point < is not one once a NaN is present, because incomparability stops being transitive. The earlier revision of this PR did have a test that passed NaN and asserted metadata finiteness; it was removed for that reason, and this uncovered line is the direct consequence.

The guard stays because deleting it is worse: std::clamp propagates NaN, so min would be stored as NaN and every distance computed against that vector would be poisoned.

Both codecov statuses pass (patch 95.83% against a 5% threshold, project -0.01% against 1%), so nothing is blocked. Noted in the code so it does not get re-filed or "fixed" by removing the guard.

The real fix is rejecting non-finite components at the public ingestion boundary, which nothing in VecSim does today and which is filed separately.

[cursor]

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

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

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