fix: avoid overflow and underflow in vector functions - #25289
namanjain24-sudo wants to merge 4 commits into
Conversation
array_distance, cosine_distance and array_normalize squared their inputs directly, so finite values whose squares leave the Float64 range returned 0, inf, NaN, a zero vector or NULL instead of the representable result. When a sum of squares may have overflowed or underflowed, recompute it with the values multiplied by a power of two that brings the largest magnitude close to 1. Other rows run the same code as before, and empty, all-zero and non-finite inputs are never rescaled.
2522b64 to
b00beb3
Compare
|
@namanjain24-sudo san |
A fixed 1e-180 threshold rescaled sums that no underflowing square could have changed, such as the squares of values around 1e-100, and the rescaled path is several times slower. An underflowing square is off by at most 2^-1075, so rescale only when the sum is below len * 2^-1012.
|
@aoto-tech thanks, you're right. The threshold only needs to catch sums that an underflowing square could have changed, and I've pushed a length-based threshold, as you suggested. A square that underflows is off by at most half the smallest subnormal value, so I first tried the tightest version, On 20,000 rows of 1536 elements around 1e-100, nothing is rescaled any more. The ratios to Was this close to the fix you had in mind? I'm happy to adjust. |
Nice, the commit looks good to me. Thanks for making the adjustment! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25289 +/- ##
=======================================
Coverage 81.92% 81.92%
=======================================
Files 1134 1134
Lines 426000 426085 +85
Branches 426000 426085 +85
=======================================
+ Hits 349000 349075 +75
- Misses 56304 56308 +4
- Partials 20696 20702 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I'm worried this check hurts performance 🤔 The numbers drop when I run the benchmark locally. I think we need a way to handle the edge cases without adding an expensive check — especially for the cases that don't benefit from it. |
|
The rescue path is only reached by rows whose first-pass sum is exactly zero, The per-element let mut max = 0.0_f64;
for value in values {
max = max.max(value.abs());
}
if max == 0.0 || !max.is_finite() {
return None;
}
This only affects a rare input: one vector all zeros, where the result is For that input the code scans both vectors, finds a scale for the normal let rescale = !dot.is_finite();
let scale1 = if rescale || needs_norm_scale(sq1, len1) {
norm_scale(vals1.iter().copied())
} else {
None
};
let scale2 = if rescale || needs_norm_scale(sq2, len1) {
norm_scale(vals2.iter().copied())
} else {
None
};
if scale1.is_some() || scale2.is_some() {
(dot, sq1, sq2) = dot_and_squares(
vals1, vals2, scale1.unwrap_or(1.0), scale2.unwrap_or(1.0),
);
}Scaling one side only is valid: cosine distance is invariant to scaling each Nit: the assert_eq!(norm_scale([f64::MAX]), Some(f64::from_bits(1 << 51)));
assert_eq!(
norm_scale([f64::MIN_POSITIVE / 4.0]),
Some(f64::from_bits(2045 << 52))
); |
norm_scale no longer returns early on a non-finite value, so its loop vectorizes; it checks the maximum once instead. NaN values are ignored, and the scaled computation still produces NaN. cosine_distance now scans a vector only when its own sum of squares is out of range, and recomputes only when there is something to scale, so a row with one all-zero vector no longer scans both vectors and repeats the dot product.
|
@jayzhan211 thanks, you're right. My earlier timings were on copies of the loops outside DataFusion, so this time I measured inside it: a criterion bench that calls the three UDFs, comparing Rows with values in [-1, 1] never reach the rescue path. They run at 0.92x to 1.03x of
A range means the two runs differed. For I've pushed both suggestions as b3fcbe1:
The commit also includes your doc fix for scaled subnormals and the bit-pattern assertions. To check that no result moved, I ran the previous and new kernels on about 3.7 million vector pairs. They covered lengths from 1 to 1536, magnitudes from subnormal up to 1e300, and zero, NaN, infinite and one-side-only inputs. All three functions are bit-identical to the previous commit, and The length-based threshold from @aoto-tech's review is unchanged, so rows around 1e-100 still never reach the rescue path (0 scans in about 87,000 random rows). On the inputs they flagged in the issue, NaN, infinite, zero and identical vectors give the same bits as Two small additions to your comment:
The bench isn't part of this PR. I can add it as |
Which issue does this PR close?
Rationale for this change
array_distance,cosine_distanceandarray_normalizesquare their inputs directly. When a finite value's square falls outside theFloat64range, they return a wrong answer even though the true result is representable:mainarray_distance([1e-200], [0])0.01e-200array_distance([3e200], [-1e200])inf4e200cosine_distance([3e200, 4e200], [3e200, 4e200])NaN-2.220446049250313e-16cosine_distance([1e-200, 2e-200], [1e-200, 2e-200])NULL0.0array_normalize([3e200, 4e200])[0.0, 0.0][0.6000000000000001, 0.8]array_normalize([3e-200, 4e-200])NULL[0.6, 0.8]The two results that are off in the last bit are ordinary rounding. The existing formula does the same for ordinary inputs: for identical vectors it already returns a small negative
cosine_distanceabout 11% of the time.What changes are included in this PR?
Each function still computes the sum of squares as before. It recomputes only when that sum may be wrong: when it is infinite or NaN, or when it is below
len * 2^-1012, wherelenis the number of elements. The recomputation multiplies every value by a power of two that brings the largest magnitude close to 1 (norm_scaleinutils.rs):array_distancescales the element-wise differences, then divides the result by the same factor.cosine_distancescales each vector on its own. The distance does not change when either vector is multiplied by a positive factor.array_normalizescales the values. Dividing the scaled values by the scaled magnitude gives the same unit vector.Rows whose sums are in range run the same code as before, so their results do not change. The threshold comes from the underflow error: a square that underflows is off by at most half the smallest subnormal value, so
lenof them move the sum by at mostlen * 2^-1075, which is less than2^-63of any sum above the threshold. Because the factor is a power of two, rescaling a value is exact unless the scaled value is subnormal, and such a value is too small to change the result, so a rescaled row gets the same result the unscaled code would have given had it not overflowed or underflowed. Empty, all-zero and infinite inputs are never rescaled, and a NaN input still gives NaN, so their results are also unchanged, includingNULLfor zero vectors.cosine_distancerescales only the vector whose own sum is out of range, which is enough because the distance does not change when either vector is scaled on its own.What is the testing strategy for this PR?
array/array_length.slt,cosine_distance.sltandarray_normalize.slt, plus non-finite inputs to pin the unchanged behaviour..sltrounds floats to 12 decimal places, so the1e-200distance is checked as a ratio.norm_scaleandneeds_norm_scaleinutils.rs.datafusion/functions-nested/srcreverted and the new tests kept, the three new overflow and underflow queries fail (0 Infinityinstead of1 4,NaN NULL NULLinstead of0 0 2, and[0.0, 0.0] NULLinstead of[0.6, 0.8] [0.6, -0.8]). The non-finite queries pass either way, as intended.main. Around the threshold, on 300,000 pairs (97,001 of them rescaled) and on 228,241 pairs whose elements span 1e-320 to 1e-80 (28,330 with subnormal squares), the new code matched a version that always rescales, bit for bit. A tighter threshold oflen * 2^-1021left some rows 1 to 4 ulps away from the rescaled result. Withlen * 2^-1012I found no differences in about 436,000 rows of length 1 to 4096.datafusion-clionmainand on this branch. Only the six rows in the table above changed.There is no existing benchmark for these functions, so I measured with a criterion bench that calls the three UDFs through
invoke_with_args(8,192 rows at dim 4 and 128, 1,024 rows at dim 1536). It is not part of this PR. Ratios are this PR's time overmain(133111f):array_distancecosine_distancearray_normalizearray_distance)Each cell lists dim 4 / 128 / 1536, as the range over two runs. Rows whose values are around 1e-200 are rescaled and cost more, but those are the rows that returned wrong results before. The
array_normalizespeedup comes from writing the output withextend.Are there any user-facing changes?
Only for finite inputs whose squares overflowed or underflowed. Those now return the correct distance or normalized vector instead of
0,inf,NaN, a zero vector orNULL. There are no API changes.