Repository navigation
fix(schema)!: enforce range bounds exactly and preserve stored contracts - #1071
Conversation
The `@range` leaf behind load, mutation and merge converted every value and bound to f64, so above 2^53 distinct integers rounded together: an I64 bounded at 9007199254740992 accepted 9007199254740993, and a U64 above i64::MAX passed an I64 bound at i64::MAX. It also skipped any column type it did not recognise, so `@range` on a numeric list, which the parser accepts, checked nothing. Integers now widen to i128 and compare with integer bounds exactly; an integer and a double compare against the double's exact value, so a decimal bound on an integer property and an integer bound on a float property round neither side. A list property bounds each element, and any other column type is refused rather than skipped. Violation messages print the exact values compared.
|
Approve. The comparison belongs in the one Why
WhatIntegers widen to HowThe change stays in the existing leaf. CompatibilityThe list change is a real break, and the Schema migration v1 refuses to add a non-index constraint, refuses to remove a constraint, and treats a bound change as remove-plus-add because the constraint key includes both bounds ( A later write checks the row image it publishes. An update that keeps an out-of-range element is refused, even when the assignment touches another property. The repair is an update that puts the value inside the range, or a delete. Delete records removed ids and does not run the value check. The same republish rule applies to an integer that the old Compaction and schema apply do not rewrite user values. A branch that only moves pins does not recheck old rows. A merge that adopts rows of a type with Refusal of an unrecognized physical type is the right failure. Declared numeric columns are primitive Arrow types, and a list is Comparison edges
A copied comparator was checked against an IEEE significand oracle on 1,443,050 pairs: Parser defects, do not blockThese are a different cause. This change does not make them worse.
LiabilityThis removes a silent wrong answer on every write path that publishes a node row. The repair is one private compare. There is no new API and no new stored state. The list scan cost follows the number of elements in the rows being written. The next numeric type is one Exactness is the point of the constraint. A single Optional
ValidationReviewed head Before, on the base, with the pull-request case present:
Two temporary probes on the same base, removed afterward and not pushed:
After, at the head,
The case passed in 0.57s. The run report shows all 8 steps. 1 passed, 376 filtered. A temporary parser test on the base, removed afterward, printed:
CI on this SHA is green, including GQT (ordinary) in run Limits of this reviewI did not re-run the full workspace suite, a merge of this case, or a live schema apply. The merge and schema-apply conclusions come from the call graph and from |
…range-correctness
Keep the accepted IR authoritative for historical endpoint and overflowing Float interpretations through a complete source-shape match. New admission preserves endpoint roles and requires integer bounds to fit I64. Extend compiler, GQT, and accepted-contract compatibility coverage and document the migration boundary.
@range values with their bounds exactly|
Approve. There is no blocking defect. Earlier findings
Why
WhatAdmission now names each endpoint. How
CompatibilityThis is a break for new schema text. The A legacy system-column upgrade parses that stored source with the strict parser ( Rows already stored stay readable. Compaction and schema apply do not rewrite user values. A merge that adopts rows of a type with LiabilityThe fix belongs in the parser, which owns admission, and in the one contract check, which owns open. It removes a wrong bound without a new stored format or a second comparator. The compatibility path is one shape transform, used only to accept an already validated IR. Lance 11.0.0 stores the property values as Arrow integers and floats. It has no range constraint. This change does not move a Lance fence. No other open pull request title changes Optional
ValidationReviewed head Before, on
The probe failed. The same parent, with the head
The case failed in 0.61s. Step 8 (mutate) expected the insert of After, at the head, with
4 passed, including upper-only integer and float bounds and the refusal of integers outside
1 passed in 0.02s. The first run with
1 passed in 0.61s. Check runs on this SHA completed success, including GQT (ordinary), Test Workspace, rustfmt, and clippy. One earlier Fix Regression Gate run was cancelled. A later run of that check succeeded. Devin Review reports success and says the full review was skipped. That is not a review of this head. I did not read the CI log line for this case name. The local head run is the proof that the case passes. A LimitsI did not re-run the full workspace suite, a live schema apply, or a system-column upgrade. Those conclusions come from the call graph. I did not measure performance. I did not re-run S3 or Azure locally. The temporary probe and the copied case were removed and were not pushed. Inline comments
|
What & why
Range constraints could accept integers above their maximum after rounding to
f64, and numeric-list constraints were not enforced. The parser also treated..maxas a minimum and silently converted overflowing integer bounds to floating point.This change fixes those admission and enforcement paths without changing the stored bound representation:
I64; decimal bounds remainF64.min..and an overflowing historical floating-point bound with its recorded decimal value. Complete affected outstanding deployments before upgrading.This is breaking for writes that previously bypassed list or exact-integer checks and for newly admitted schemas relying on the parser defects. Existing stored meanings remain unchanged.
Backing issue / RFC
These reproduced defects extend the range-correctness scope already under review in this PR; no separate issue is linked.
Checklist
Local verification
cargo test -p omnigraph-compiler --locked --lib— 490 passed.cargo clippy -p omnigraph-compiler --all-targets --locked -- -D warnings -W clippy::dbg_macro— passed.cargo test -p omnigraph-gqt --locked --test gq_logic_tests range -- --test-threads=1— 4 passed, including exact integer/list enforcement, upper-only scalar/list bounds, and the existing floating-bound reopen regression.cargo test -p omnigraph-engine --locked --lib range_ -- --test-threads=1— 6 passed, including the accepted-contract compatibility matrix, NaN, and exact mixed comparisons.db::upgrade::tests::an_unreadable_root_contract_refuses_as_unsupported_source— passed directly in the freshly built engine test binary.Before correction, the exact-integer GQT case accepted
9007199254740993against maximum9007199254740992; three compiler regressions also failed as predicted. The accepted-contract matrix covers historical and current endpoints, overflowing historical bounds, full-shape mismatches, invalid source, and corrupted hash/domain without changing stored bytes.The three updated branches (#1070, #1071, #1036) also merge cleanly. Their combined code passed five file/scoped-file/S3/Azure collector owners without skips, the real server failure journey, range unit owners, both setup/dead-staging DST pins with replay, and all four range-filter GQT cases. Combined documentation (341 files), formatting, links, required CI names, and merge-group checks passed. Local validation is retained on
codex/release-014-integrated-validation; the final additional merge changes only the clarified testing-guide wording.