Skip to content

fix(schema)!: enforce range bounds exactly and preserve stored contracts - #1071

Merged
aaltshuler merged 3 commits into
ModernRelay:mainfrom
ragnorc:claude/modest-bhabha-ec17be
Oct 18, 2026
Merged

aaltshuler merged 3 commits into
ModernRelay:mainfrom
ragnorc:claude/modest-bhabha-ec17be

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

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 ..max as 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:

  • Compare integer values and bounds exactly, including mixed integer/float comparisons; enforce bounds on every non-null list element and reject unsupported physical types.
  • Parse upper-only and lower-only bounds separately. New integer bounds must fit I64; decimal bounds remain F64.
  • Preserve already accepted schemas on open and root upgrade. The source must match the complete current or complete historical shape of the hash-validated accepted IR. No mixed interpretation, identity bypass, or rewrite is allowed.
  • Document migration behavior: restating old source uses corrected admission rules. Spell an accepted historical minimum as 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

  • Focused range-admission, enforcement, and persisted-contract compatibility change
  • Existing test owners extended; original and newly found failures reproduced before correction
  • User guide, skill reference, and permanent release notes updated
  • Architectural invariants reviewed; no persisted format change or weakened contract validation

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.
  • Existing db::upgrade::tests::an_unreadable_root_contract_refuses_as_unsupported_source — passed directly in the freshly built engine test binary.
  • Formatting, documentation checker (340 files), targeted spelling, release-note preview, and whitespace checks — passed.

Before correction, the exact-integer GQT case accepted 9007199254740993 against maximum 9007199254740992; 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.

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.

ragnorc commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Approve. The comparison belongs in the one @range checker. It is exact on the edges that were checked. There is no blocking defect. Track the two parser defects. Do not hold this change for them.

Why

@range is a write-time integrity rule. Load, mutation, and merge all call loader::validate_value_constraints. That function turned every value and every bound into f64. Above 2^53, distinct integers become one double. An I64 maximum of 9007199254740992 accepted 9007199254740993. A U64 above i64::MAX matched an I64 maximum at i64::MAX, because both sides became 2^63. The function also returned success for a column type it did not recognize. The parser accepts @range on a numeric list, so that declaration checked nothing. A declared constraint then failed in silence. That breaks the rule that an integrity failure must be loud.

What

Integers widen to i128 and compare with integer bounds in that domain. F32 widens to F64 with no rounding. An integer and a double compare against the double’s exact value. A list property bounds each non-null element. A null list and a null element stay exempt. Any other column that holds a non-null value is refused. An all-null column has nothing to check. Violation text prints the exact values. An integral double prints as that integer. The default double text for 2^63 is 9223372036854776000.

How

The change stays in the existing leaf. validate_changeset is the door for load (loader/mod.rs) and mutation (exec/mutation.rs through staging::to_changeset). Merge calls evaluate_value_constraints_with_sink, and that function calls the same leaf. A proven fast-forward skips validation only when the node type has no @range, no @check, no unique constraint, and no enum. The merge scan projects every column that is not a vector or a blob, so a numeric list is in the batch. Edges have no @range. Bounds stay ConstraintBound::Integer(i64) and Float(f64). The stored schema format does not change.

Compatibility

The list change is a real break, and the ! in the title is warranted.

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 (schema_plan.rs, constraint_ir_key). A new node type may still be created with @range. Applying the same schema text does not rewrite rows and does not scan them. Rows already stored stay readable. That includes list elements and integers that the old checker accepted.

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 f64 compare let through.

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 @range does recheck them.

Refusal of an unrecognized physical type is the right failure. Declared numeric columns are primitive Arrow types, and a list is List of one of those (PropType::to_arrow). In Lance 11.0.0, dictionary examples in the pinned crate are string dictionaries. A later read type that this checker does not know fails the write. LargeList is outside the declared layout and fails closed the same way. Arrow 58.3.0 keeps list offsets as indexes into values(), including after a slice, so the offset walk matches ListArray::value.

Comparison edges

integer_cmp_float truncates toward zero. The gap to that integer is less than 1, so the fraction decides only when the integral parts are equal. A negative non-integer truncates toward zero, and total_cmp then places the integer above the more-negative double. -0.0 truncates to -0.0, casts to i128 0, and an inclusive range treats -0.0 and 0.0 as equal. Above 2^53 every finite f64 is an integer, so the fraction arm is idle. An integer-valued f64 in [-2^127, 2^127) casts to i128 exactly. i128::MAX as f64 is 2^127, so i128::MAX is less than that double. i128::MIN equals -2^127. A NaN value is refused before compare. A NaN bound, which schema text cannot spell, is an error.

A copied comparator was checked against an IEEE significand oracle on 1,443,050 pairs: i128 edges, u64::MAX, 2^53, both infinities, both NaNs, -0.0, subnormals, and an f32 stride. There was no mismatch. An f32 value widened through Into<f64> stays exact. The crate unit test pins the corners the grammar cannot spell.

Parser defects, do not block

These are a different cause. This change does not make them worse.

  1. @range(level, ..10) is stored as minimum 10 and no maximum. 10.. stores the same pair. extract_range_bounds assigns the first literal to the minimum (parser.rs around lines 517–531). The grammar already has a separate .. form. The fix belongs there.
  2. An integer literal above i64::MAX falls through to f64. 9223372036854775809 becomes 2^63. Exact support needs a wider persisted ConstraintBound. That is a format decision. Until that decision, refuse the literal in parse_constraint_bound (around lines 540–546). U64 values above i64::MAX reach the checker through a load. Mutation parameters already stop at i64::MAX.

Liability

This 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 RangeColumn arm, not a second comparator. Five similar fixes should stay inside this leaf. The remaining contract gap is the parser, and those two fixes do not need a new abstraction. No other open pull request changes @range or the bound type.

Exactness is the point of the constraint. A single f64 compare is cheaper and gives the wrong answer past 2^53. That cost is the right one to drop.

Optional

  1. crates/omnigraph-compiler/src/schema/parser.rs, extract_range_bounds and parse_constraint_bound. File the two parser defects. Make ..max store a maximum. Until a wider bound is an accepted schema change, refuse an integer literal that does not fit in i64 instead of rounding it to f64.
  2. changelog.d/range-constraint-exact-comparison.breaking.md. State that an update which keeps an out-of-range element is refused, including an update of another property. State that delete is still allowed and that stored rows stay readable. State that the same republish rule applies to an integer the old compare accepted. The fixed note can point at that sentence.
  3. crates/omnigraph/src/loader/mod.rs, beside test_range_constraint_rejects_nan. The unrecognized-type refusal has no test. A .gqt case cannot declare a physical type the schema rejects. Add a unit test that a non-null Utf8 column with @range is refused and that an all-null column of that type is not.

Validation

Reviewed head ea334fb2d5e186b16f9e9451e45f07e52d68504f. Base be031fea766fc8b280137f9ad9f34d4d2b3087bc.

Before, on the base, with the pull-request case present:

cargo test -p omnigraph-gqt --locked --test gq_logic_tests range_constraint -- --nocapture --test-threads=1

case::range_constraint_compares_integers_exactly.gqt failed. Step 1 (mutate) expected an error containing @range violation on Counter.count: value 9007199254740993 > max 9007199254740992, and the mutation succeeded. That is the predicted f64 failure. Runtime about 0.59s.

Two temporary probes on the same base, removed afterward and not pushed:

  • A list insert of [0, 11] under @range(counts, 0..10) succeeded. The list constraint was not enforced.
  • A generated load of U64 9223372036854775808 under a maximum of 9223372036854775807 succeeded.

After, at the head, CARGO_TARGET_DIR shared with the base build:

cargo test -p omnigraph-engine --locked --lib compare_exactly -- --nocapture

loader::tests::range_operands_compare_exactly passed. 1 passed, 486 filtered.

cargo test -p omnigraph-gqt --locked --test gq_logic_tests range_constraint_compares_integers_exactly -- --nocapture --test-threads=1

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:

  • @range(level, ..10) and @range(level, 10..) both store minimum Integer(10) and no maximum.
  • @range(level, 0..9223372036854775809) stores maximum Float equal to 2^63.

CI on this SHA is green, including GQT (ordinary) in run 39108425850 and Test Workspace, rustfmt, and clippy in run 39108425943. Devin Review reports pass with the note that 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.

Limits of this review

I 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 schema_plan. I did not measure performance. The significand oracle compared a copied function, and the crate unit test passed separately. There was no S3 or Azure run for this review. The temporary probes and the parser test were removed and were not pushed.

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.
@aaltshuler aaltshuler changed the title fix(loader)!: compare @range values with their bounds exactly fix(schema)!: enforce range bounds exactly and preserve stored contracts Oct 18, 2026
@aaltshuler
aaltshuler added this pull request to the merge queue Oct 18, 2026

ragnorc commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Approve. There is no blocking defect.

Earlier findings

  1. Fixed. New admission stores ..max as a maximum. An integer bound outside I64 is refused. A stored contract still opens when the accepted IR matches the current shape, or the whole historical shape.
  2. Still open. changelog.d/range-constraint-exact-comparison.breaking.md does not state the republish rule. A later write refuses a row that still holds an out-of-range element. That includes a write that changes another property. Delete stays allowed. An integer that the old f64 compare accepted follows the same rule.
  3. Still open. validate_value_constraints still refuses a non-null column type that @range cannot check, and it still skips an all-null column. Neither case has a test.

Why

@range is a write-time integrity rule. The previous head compared a value with its bound exactly. The parser still stored ..max as a minimum, and it stored an integer outside I64 as a rounded F64. A declared maximum then enforced the wrong end, or a different number. That is a silent wrong constraint. An integrity failure must be loud. The accepted schema IR must stay the source of truth.

What

Admission now names each endpoint. ..10 is an inclusive maximum. 10.. is an inclusive minimum. An integer bound must fit I64. A decimal bound stays F64. Open and root upgrade do not rewrite a stored contract. They accept the stored source only when the current shape, or one complete historical shape, matches the accepted IR. A mix of the two readings is refused. Init, schema apply, and deployment validation use the strict parser.

How

range_min and range_max are separate grammar rules. parse_constraint_bound refuses an integer outside I64 in admission mode. Persisted-contract mode still reads that integer as the F64 the old parser stored. validate_current_source_matches hashes the compiled shape against the IR. On a mismatch it moves every upper-only node range to the minimum and accepts the source only when that whole shape matches. Load, mutation, and merge still enforce the catalog built from the IR. The comparison leaf is unchanged. The merge d1b225bb brings this branch onto current main. It does not change the range checker or the parser.

Compatibility

This is a break for new schema text. The ! in the title is warranted. An existing graph keeps the bounds in its IR. schema show still prints the stored source. Under the new reading, an old ..max looks like a maximum, while the engine enforces the stored minimum. Before a schema apply, an import, or a new graph, write that historical minimum as min... Write a stored rounded integer bound as a decimal of the recorded value. A schema apply of the old spelling plans as a constraint change, which migration v1 refuses, so the graph stays as it is. A schema apply of the restated spelling keeps the same constraint. The source hash changes, so the apply publishes the new source.

A legacy system-column upgrade parses that stored source with the strict parser (render_system_column_upgrade_target). Historical ..max, or an integer outside I64, makes the upgrade refuse before it writes. The error does not name this migration. Current-vintage graphs return before that check. The repair on a legacy graph is the schema-apply restatement above, then the column upgrade.

Rows already stored stay readable. Compaction and schema apply do not rewrite user values. A merge that adopts rows of a type with @range checks those rows against the target IR.

Liability

The 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. with_historical_range_endpoints must stay on that match path. Five similar historical readings would add more branches to the same check. That is the cost of keeping stored contracts unchanged. Exact admission is the right cost. One f64 parse is shorter and stores the wrong bound.

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 @range or the bound type.

Optional

  1. changelog.d/range-constraint-exact-comparison.breaking.md. Add the republish and delete sentences from finding 2. The same sentences belong on the integer case, not only the list case.
  2. crates/omnigraph/src/loader/mod.rs, beside test_range_constraint_rejects_nan. A non-null Utf8 column with @range must be refused. An all-null column of that type must pass. A .gqt case cannot declare a physical type the schema rejects.
  3. docs/user/schema/index.md and the system-column upgrade error. Name the legacy column-upgrade refusal, and name the schema-apply restatement that clears it.

Validation

Reviewed head c382a85f716282862a11207638743dab446add47. Base be031fea766fc8b280137f9ad9f34d4d2b3087bc. The parent of the reviewed commit is d1b225bb46386194a7593977210693f2dd6ff0e0. Parser files at that parent match ea334fb2.

Before, on d1b225bb, with a temporary probe that was removed and not pushed:

CARGO_TARGET_DIR=/tmp/target-parent cargo test -p omnigraph-compiler --locked --lib -- probe_upper_only_is_max_and_overflow_integer_is_refused -- --nocapture --test-threads=1

The probe failed. @range(v, ..10) stored minimum Integer(10) and no maximum. @range(v, ..9223372036854775808) was accepted as minimum Float 2^63. overflow_refused=false.

The same parent, with the head .gqt case copied in temporarily and the engine rebuilt from that commit:

CARGO_TARGET_DIR=/tmp/target-parent cargo test -p omnigraph-gqt --locked --test gq_logic_tests range_constraint_compares_integers_exactly -- --nocapture --test-threads=1

The case failed in 0.61s. Step 8 (mutate) expected the insert of ceiling: -1 to succeed. The mutation returned @range violation on Counter.ceiling: value -1 < min 10. That is the predicted old reading of ..10.

After, at the head, with CARGO_TARGET_DIR=/workspace/target:

cargo test -p omnigraph-compiler --locked --lib -- parse_range -- --test-threads=1

4 passed, including upper-only integer and float bounds and the refusal of integers outside I64.

cargo test -p omnigraph-engine --locked --lib accepted_range_contracts_keep_their_complete_historical_shape -- --nocapture --test-threads=1

1 passed in 0.02s. The first run with --exact and the short name selected 0 tests. The substring run above is the proof.

cargo test -p omnigraph-gqt --locked --test gq_logic_tests range_constraint_compares_integers_exactly -- --nocapture --test-threads=1

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 .gqt case cannot build an accepted IR that disagrees with the current parser, so the historical contract test is the owner for open and root upgrade. The case in this pull request is the owner for the new reading of ..max. The compiler test is the owner for the integer refusal, because an invalid schema cannot be a passing .gqt case.

Limits

I 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

changelog.d/range-constraint-exact-comparison.breaking.md, line 4: Still open from the review of ea334fb2. Add three facts. A later write refuses a row that still holds an out-of-range element, including a write that changes another property. Delete stays allowed. An integer that the old f64 compare accepted follows the same rule.

crates/omnigraph/src/loader/mod.rs, line 5401: Still open from the review of ea334fb2. Add a unit test beside test_range_constraint_rejects_nan. A non-null Utf8 column with @range must be refused. An all-null column of that type must pass. A .gqt case cannot declare a physical type the schema rejects.

docs/user/schema/index.md, line 105: Optional. A legacy system-column upgrade parses the stored source with the strict parser in render_system_column_upgrade_target. Historical ..max, or an integer outside I64, makes that upgrade refuse before it writes. The error does not name this migration. A schema apply of the restated source publishes it, because the source hash changes. The column upgrade can then proceed. Please name that step here.

Merged via the queue into ModernRelay:main with commit 26caff8 Oct 18, 2026
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants