fix(arith): keep integer div exact and UB-free - #403
Conversation
div routed integer operands through doubles, silently corrupting every result above 2^53 (div 9007199254740993 1 -> 9007199254740992) and tripping UBSan at q == 2^63 (div -9223372036854775807 -1) because the q > (double)INT64_MAX guard can never fire. Integer operands now divide in int64 space with a floor correction, matching the temporal mod path; the double path is kept only for float operands with a tightened guard.
singaraiona
left a comment
There was a problem hiding this comment.
The scalar ray_idiv_fn change is correct, and the new integration/math cases pass, but this does not fix compiled query execution. select/update compile div to OP_IDIV; the I64 kernel in src/ops/expr.c still reads both operands as double before floor/cast. On this PR build:
(set T (table [v] (list [9007199254740993 123456789012345678])))
(at (select {q: (div v 1) from: T}) (quote q))
returns [9007199254740992 123456789012345680], and update has the same result. The top-level vector tests pass because that call path maps through ray_idiv_fn per element; they do not cover the query/DAG path. Please make integer OP_IDIV stay in integer space in the compiled/fallback kernels too, and add select/update regressions above 2^53.
singaraiona
left a comment
There was a problem hiding this comment.
The correctness half of this is good work and the bugs are provably real — I reproduced all of them on current dev: silent precision loss above 2^53 ((div 9007199254740993 3) off by one), wrong 0Nl at the 2^63 boundary, a broken (+ (* (div a b) b) (mod a b)) == a identity for large ints, and genuine UB (confirmed with -fsanitize=float-cast-overflow: base's q > (double)INT64_MAX guard rounds to exactly 2^63 and lets q == 2^63 through the cast; your >= 9223372036854775808.0 bound is correct). Floor semantics are preserved exactly — I diffed ~60 cases across all sign combos, widths, and paths; every in-range result is byte-identical. Merges cleanly onto current dev, suite green.
One blocker: the hot columnar div path gets ~2x slower. Hardware 64-bit idiv is scalar-only, replacing the vectorizable divsd+floor. Measured on a 20M-row table (release, controls flat in the same run): select (div a b) 449 → 816 ms (+79%), select (div a 7) 407 → 742 ms (+90%). div backs temporal idioms ((div ts 60), (div ts 86400)), so this lands on real query paths.
Suggested fix that keeps your exactness: pay for int64 only when it matters. Per morsel, check whether both operand magnitudes fit in ±2^53 (cheap scan, or column min/max stats where available) and dispatch the common case to the existing vectorized double loop, the big-value case to your exact int64 loop. Your src_is_i64_all branch is already hoisted outside the loop, so this slots in naturally. For loop-invariant scalar divisors, a libdivide-style reciprocal is the alternative.
Also, for symmetry/completeness (non-blocking but cheap while you're in there): the INT64_MIN / -1 guard exists in floor_idiv_i64_checked() but not in ray_idiv_fn's new integer path (unreachable today because INT64_MIN is the null sentinel, but the asymmetry will bite someone); the new narrow I32/I16/U8 arms truncate with plain casts where the float arms used the _null() checked casts; expr.c:3191's I64 OP_DIV arm still uses the old r*ri!=li form instead of your helper; and tests for div-by-zero through the new integer path and the narrow-width arms would round out the coverage. 0x1p63 would document the magic bound better than the decimal literal.
One repo follow-up on our side regardless of this PR: adding -fsanitize=float-cast-overflow to DEBUG_CFLAGS — it's not in either compiler's UBSan default set, which is why CI never saw this.
…rnel Keeping every integer OP_IDIV on the exact scalar int64 kernel (PR RayforceDB#403) made the hot columnar div path ~2x slower: hardware 64-bit idiv is scalar and replaced the vectorizable divsd+floor. Restore the fast path without losing exactness — per morsel, when both operands are within ±2^53 (a cheap scan, over the in-cache morsel in the fused kernels) delegate to the vectorizable double loop, which is bit-exact there (round(a/b) cannot cross an integer boundary in that range); fall back to the exact int64 kernel only for large magnitudes or nulls. Applied to the fused null-aware and non-null I64 kernels and all four binary_range output arms. Measured (cache-proof, distinct constant divisors, 20M rows ×10 iters, release): exact-only 0.41s -> dual-path 0.24s. Also folds in the review's smaller points: - ray_idiv_fn guards INT64_MIN/-1 in the integer path (symmetry with floor_idiv_i64_checked; unreachable today but removes latent UB); - the exact narrow I32/I16/U8 arms saturate to match their ray_cast_f64_to_iN_null double counterparts instead of wrapping; - binary_range's I64 OP_DIV arm uses floor_idiv_i64_checked, not the open-coded form; - 0x1p63 documents the scalar float-cast bound. Extends test/rfl/integration/math.rfl: fast/exact boundary agreement at 2^53, mixed small+big morsels, div-by-zero through the integer path, and narrow-output floor div.
|
Thanks — the perf blocker and the smaller points are addressed in Blocker — the ~2x columnar slowdown. Restored the fast path without giving up exactness, along the lines you suggested: per morsel, when both operands are within ±2^53 the floor-div is delegated to the vectorizable double loop; only genuinely large magnitudes (or nulls) take the exact scalar int64 kernel. Within ±2^53 the double result is bit-exact — a non-integer A/B on this machine, cache-proof (distinct constant divisors so identical-query reuse can't skew it), 20M rows × 10 iters, release:
(My first attempt at an A/B was noise — repeating one identical Smaller points, all folded in:
Agreed on adding |
singaraiona
left a comment
There was a problem hiding this comment.
Re-reviewed at c6a9cfd — this is now in good shape and I'm taking it. The ±2^53 magnitude fast path is exactly right, and it held up under hostile testing: the gate proof is sound (including the half-ULP equality edge, which forces a power-of-two divisor and hence an exact integer quotient), 16,640 exhaustive boundary cases both signs plus 10M+ randomized fast-vs-exact comparisons found zero mismatches, in-gate results are byte-identical to base, sanitizers (including float-cast-overflow) are clean, and every item from the first review is addressed — the narrow-arm saturating clamps even match ray_cast_f64_to_*_null's real behavior, which my first review described wrongly (those helpers saturate, not null).
One residual we'll fix in a follow-up commit on our side rather than another round-trip: select (div a 7) (scalar divisor) is still +47% vs base — entirely gate-scan overhead, since the scan re-reads the broadcast scalar n times through the RV_READ_I64 ternary chain in two separate passes. The fix is to hoist scalar-operand range checks out of the loop, scan only genuine vector operands through their typed pointers, and fuse the two passes. select (div a b) is already down to +7%.
Also noting for a separate issue, not this PR: +, -, *, % on int64 vectors still round through double above 2^53 (identical in base — this PR fixes division only), so the exactness story has a sibling chapter left.
Thanks for the careful iteration — the exhaustive tests you added at the gate boundary are exactly the right kind.
…ollow-up) PR #403 added a ±2^53 magnitude gate so integer `div` keeps the vectorizable double kernel for small values. The gate itself was cheap in principle but paid for twice over in binary_range: it walked BOTH operands in a full extra pass, reading every element through the 6-way LV_READ_I64/RV_READ_I64 ternary chain — including a broadcast SCALAR divisor, whose single value was re-read n times. `select (div a 7)` over 20M rows went 107 -> 164 ms (min-of-10, -c 1). Scan plumbing only — the predicate, the exact int64 kernel, the gate constant and every result are untouched: - a broadcast scalar operand is range-checked once, outside any loop; - a narrow vector (I32/U32/I16/BOOL/U8) cannot leave ±2^53, so it is not scanned at all; - an I64 vector is scanned through its typed data pointer, and two I64 vectors share a single pass (i64_span2_dbl_exact); - for the hot `I64 column ÷ (I64 column | int scalar)` shapes with I64 output there is no pre-pass: the range check rides inside the divide loop (values already in registers) and the range is recomputed by the exact kernel only if the flag fires — the same all-or-nothing decision, minus a DRAM-bound second read of the column. - the fused kernel's two span scans are likewise fused into one. Span scans are now branchless accumulations so the passing (common) case stays auto-vectorized. Measured (release, 20M rows, min-of-10, -c 1 | -c 8): base 2ec5284 merged 66a80fb this div a 7 107 | 31 164 | 45 65 | 24 div a b 91 | 42 114 | 43 94 | 42 * a b (ctl) 40 | 30 40 | 30 40 | 30 Verified byte-identical to 66a80fb on a div corpus (all sign combos, nulls, 2^53±1 both signs, INT64_MIN-adjacent, scalar and vector divisors, narrow widths, timestamp column) at -c 1/2/4/8. math.rfl pins the fast lane, the redo lane and the scalar-divisor lane of the inline gate.
Summary: keep integer-only div in int64 space to avoid double precision loss above 2^53 and avoid UB at the 2^63 float boundary; add math regressions for exact large integers, vector paths, float out-of-range nulls, and floor semantics. Tests: make test TEST_CORES=2.