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

Stack position 2 of 3, base dor-forer-MOD-17527-uint8-accumulators (#1014). Review only the top commit; 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. 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 hot-path vector ingestion quantization used by distance kernels; behavior changes for extreme finite ranges but removes UB and keeps metadata contract (finite min/delta) for supported input.

Overview
Fixes undefined behavior in SQ8 storage quantization for finite vectors the API already accepts—previously reachable on AddVector via UBSan (e.g. NaN → unsigned char).

QuantPreprocessor::quantize now computes range in double (max - min no longer overflows for ±FLT_MAX), guards delta when FP32 underflow would make it zero, uses a double inv_delta for subnormal scales, and maps values with a to_byte saturating path instead of std::round on possibly-NaN products.

find_min_max documents and enforces finite ordered endpoints (clamp to FLT_MAX, invariant check); transformed_value applies the same clamp under WithNorm so centered ±inf cannot yield asymmetric non-constant byte patterns when the range collapses.

Tests add UBSan-focused cases (full float range, centered extremes, collapsed centered range symmetry) plus a parameterized finite-input matrix for scale metadata and bytes. Sum metadata overflow is explicitly out of scope.

^{Reviewed by Cursor Bugbot for commit 61c4e90. 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.
⚠️ Please upload report for BASE (dor-forer-MOD-17527-uint8-accumulators@a6db25f). Learn more about missing BASE report.

⚠️ Current head 6d9e1ba differs from pull request most recent head 61c4e90

Please upload reports for the commit 61c4e90 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.16%           
=========================================================================
  Files                                             ?      141           
  Lines                                             ?     8385           
  Branches                                          ?        0           
=========================================================================
  Hits                                              ?     8147           
  Misses                                            ?      238           
  Partials                                          ?        0           

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

[cursor]

Query path skips centering clamp

Medium Severity

transformed_value now clamps centered storage values to ±representable_float, but preprocessQuery for WithNorm L2 still centers without that clamp. For the same finite overflow cases this PR treats as supported, storage becomes clamped while the query body and its metadata can stay ±inf, so asymmetric L2 can yield NaN distances (for example via 0 * inf in the quantized dot product).

Additional Locations (1)

• src/VecSim/spaces/computer/preprocessors.h#L590-L597

 

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