fix(cmp, expr): order integer atoms and run the integer arms of binary_range as int64, not double - #528
Merged
Conversation
…y_range as int64, not double Closes #524. A double is exact only to 2^53. Two paths turned an i64 into a double before an integer operation, so above 2^53 distinct integers merged — and every TIMESTAMP (nanoseconds, ~8.4e17, double spacing 128 ns) is such an integer: * ray_gt_fn / ray_lt_fn / ray_gte_fn / ray_lte_fn ended in `as_f64(a) OP as_f64(b)` for every numeric pair, integers included, while ray_eq_fn already compared integers as int64 — so two atoms could differ under == and be neither > nor < each other, and the dyadic min/max built on <=/>= returned the wrong operand. * The integer-output arms of binary_range (I64/TIMESTAMP, I32/DATE/TIME, I16, U8 — ADD SUB MUL DIV MOD MIN2 MAX2) and the integer-family BOOL arm read their operands through LV_READ/RV_READ, which widen to double, and cast back: each operand rounded to the nearest double before an exact integer op. The exact readers (LV_READ_I64/RV_READ_I64) existed and were used only by OP_IDIV. cmp.c: when both atoms are integer-family (I64/I32/I16/U8) the four orderings compare as_i64; float, BOOL and char pairs keep the double compare (their values are exact in a double). expr.c: LV_INT/RV_INT read exactly when both operands are integer-family (src_is_i64_all, loop invariant) and keep the truncating double read for a float operand; every integer-output arm and the integer BOOL arm use them. The IDIV double kernels and AND/OR truthiness (a nonzero test) are unchanged. Vector-with- atom comparisons, reductions and the constant folder were already exact. Measured on 20 M-row I64 vector pairs (release): add 35 -> 30 ms, gt 17 -> 14 ms, sub/eq unchanged — the round trip through double is gone. Test: test/rfl/ops/i64_exact_ordering.rfl — the issue's table (atoms, vector-with-vector arithmetic and comparisons at 2^53 and 2^62, timestamp differences and ordering) plus mixed widths, nulls and floats through the same arms. Claude-Session: https://claude.ai/code/session_017fCcYRuzNagw6nmLqZ9zbU
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #524.
A double is exact only to 2^53. Two paths turned an i64 into a double before an integer operation, so above 2^53 distinct integers merged. Every TIMESTAMP (nanoseconds, about 8.4e17, double spacing 128 ns) is such an integer, so a latency column built from two stamp columns came out 64 ns off and stamps 1 ns apart compared equal.
Root causes
ray_gt_fn,ray_lt_fn,ray_gte_fn,ray_lte_fnended inas_f64(a) OP as_f64(b)for every numeric pair, integers included, whileray_eq_fnalready compared integers as int64. Two atoms could differ under==and be neither>nor<each other, and the dyadicmin/maxbuilt on<=/>=returned the wrong operand.binary_range(I64/TIMESTAMP, I32/DATE/TIME, I16, U8, for ADD SUB MUL DIV MOD MIN2 MAX2) and the integer-family BOOL arm read their operands through the double readers and cast back, so each operand was rounded to the nearest double before an exact integer op. The exact readers existed and were only used byOP_IDIV.Fix
cmp.c: when both atoms are integer-family (I64/I32/I16/U8) the four orderings compare as int64. Float, BOOL and char pairs keep the double compare, whose values are exact in a double.expr.c:LV_INT/RV_INTread exactly when both operands are integer-family (src_is_i64_all, loop-invariant) and keep the truncating double read for a float operand. Every integer-output arm and the integer BOOL arm use them. The IDIV double kernels and AND/OR truthiness (a nonzero test) are unchanged. Vector-with-atom comparisons, reductions and the constant folder were already exact.Results
Every row of the issue's table now matches its expected column, for example
(> Q P53)istrue,(== R (take P62 3))is[false true false],(- D C)is[1000000 1000000 1000000].Timing on 20 M-row I64 vector pairs, release, same box: add 35 → 30 ms, gt 17 → 14 ms, sub and eq unchanged. The round trip through double is gone, so the fix is not slower anywhere.
Tests
test/rfl/ops/i64_exact_ordering.rfl: the issue's cases (atoms, vector-with-vector arithmetic and comparisons at 2^53 and 2^62, timestamp differences and ordering), plus mixed widths (I32 against I64 columns), nulls and floats through the same arms, and the shapes that were already exact as a control. Full suite 3788 of 3788, no UBSan output.Noticed, not changed
Dyadic
min/maxon two 20 M-row vectors takes about 1.7 s on dev and on this branch, far off the ~15 ms of the other comparisons. That shape is not going throughbinary_range; separate issue material.https://claude.ai/code/session_017fCcYRuzNagw6nmLqZ9zbU