Repository navigation
fix(engine): historical queries bind properties by stable identity - #959
Conversation
A historical query plans against the current accepted schema, and its tables were rebound by table identity, but its properties were matched to stored columns by name only. After a property was dropped and re-added with the same name, a query at an earlier commit read the dropped property's values under the new one, in projections and in pushed filters, against invariant 6. Every stored property column already records its stable property id (omnigraph.stable_property_id). The stored-field checks behind every scan, hydration and edge read now resolve a property by that id. bind_catalog_aliases records which tables a historical view pins at a version other than the live one; only there can the column of a property's name belong to another lifetime. On such a table a property is refused with a request error when the column of its name carries another id (a same-name re-add), when the id sits under an earlier name (a rename since) or nowhere (an addition since), and, as Blob reads do, when the column carries no id because it was written before 0.10. Renamed and later-added properties used to fail there with an internal error; crossing a rename stays unsupported.
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve at d142e17eddb1cdd8667767c93ea0cd280560b223. No blocking defects or optional changes to request. This is a COMMENT review because the authenticated account is the PR author.
This PR prevents an old value from appearing as a new property. If Person.note is dropped and then added again, the two properties have different identities. A historical query previously matched their column names and could return the dropped value. The fix checks the persisted property ID before it reads a column. It returns a typed error when the selected version does not contain the current property under its current name.
The underlying rule is invariant 6: names do not establish a property's lifetime. The fix belongs at the boundary between the accepted schema and the stored field. The shared stored_field check covers projections and pushed predicates. Its callers also cover search fields, deferred return columns, traversal destinations, and edge properties. I traced key narrowing: it keeps the original property predicate beside the derived ID predicate. I also checked the metadata-count shortcut: it accepts entity counts, not property counts. Neither route removes a required property check.
The accepted catalog and its live snapshot are captured together. bind_catalog_aliases derives coverage from stable table identity, path, published version, native branch, and the persisted read witness. It does not infer coverage from a name or version number alone. The extra snapshot set is derived, local to the read, and rebuilt on binding. It adds no durable authority and no extra storage read to catalog capture.
The tradeoff is deliberate. An unchanged property still reads at an older commit. A renamed property, a later addition, or a same-name replacement refuses. Whole-node results therefore refuse if any current property is absent. Older unmarked columns refuse unless the selected table is still the exact current physical version. This limits historical convenience, but it avoids both a value leak and invented nulls. The user guide and release note state this limit. It follows the existing Blob rule.
This reduces long-term liability: one field check replaces name-only trust across the scalar read paths. It reuses accepted property IDs and existing version witnesses; it adds no format, writer, migration, or alternate query language. The new obligation is to carry coverage through each stored-field reader. Five more operators should call the same check, rather than add five policies. The remaining Blob-specific validation still owns Blob encoding and integrity checks. Current reads incur schema/metadata comparisons; historical reads also derive table coverage. I did not measure latency or throughput.
I checked the relevant Lance schema, evolution, and versioning documentation against the locked, unmodified Lance 11.0.0, source commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. The inspected source files match their Cargo.lock-verified crate archives. Arrow/Lance field conversion preserves metadata, and renaming preserves the field's identity metadata. OmniGraph's schema evolution separately stamps and checks its graph property IDs. Lance field IDs remain a physical mechanism, not graph identity. I also inspected these surfaces in Lance 13.0.0: metadata preservation and rename behavior remain compatible with this approach. This is source inspection, not qualification of an OmniGraph dependency upgrade.
I surveyed the other open PRs. This complements #839/#815: plan acceptance and diagnostics do not replace validation against the selected stored version. It also fits the immutable-version caches in #818/#825. The per-concrete-table boundary composes with the direction in #884; actor views in #951 will need to preserve it across restricted reads. Search work in #792/#793/#952 must keep the same field check. #677 may later change historical schema semantics, but it is a draft and does not change today's contract. I found no reason to defer this fix for those changes.
The regression has a real before/after proof. GQT cannot perform schema apply at this head: its step/control types cannot express the required drop/re-add sequence. The existing Rust point_in_time owner is appropriate.
- Before:
680fdfef96a61db898c75468e6bb42858bc4d3af, with only the PR's regression-test diff applied. One test ran and failed because the historical query returnedp.note = "secret"instead of refusing. This was a behavioral failure, not a build or setup failure. - After: exact head
d142e17eddb1cdd8667767c93ea0cd280560b223. The same test passed. It covers both historical entry points, projection, a pushed filter, rename, addition, whole-node projection, edge properties, and a valid unchanged property. - Additional local checks at head: all 18
point_in_time::tests passed; the property-ID/legacy decision test passed; the catalog alias/coverage test passed. Temporary test edits were restored. The isolated checkout is clean at the reviewed head.
The exact regression command was run at both commits:
RUSTUP_TOOLCHAIN=1.97.1 \
CARGO_TARGET_DIR=/tmp/review-928/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
RUST_MIN_STACK=16777216 RUSTC_WRAPPER= \
cargo test -p omnigraph-engine --locked --test integration \
point_in_time::historical_query_binds_properties_by_identity_not_name \
-- --exact --nocaptureThe additional commands used the same environment:
cargo test -p omnigraph-engine --locked --test integration point_in_time:: -- --nocapture
cargo test -p omnigraph-engine --locked --lib historical_schema_binds_properties_by_stable_property_id -- --nocapture
cargo test -p omnigraph-catalog --locked --lib tests::historical_alias_binding_keeps_same_name_node_and_edge_identities_distinct -- --exact --nocaptureSeparately, workspace/storage CI and GQT/served/DST-mode CI completed successfully; both workflow runs name this exact head SHA. I did not rerun those full matrices locally. The legacy unmarked-column branch has a local unit proof, not a new end-to-end old-store fixture. Search-specific schema-evolution paths were traced in code, not each reproduced separately.
1 participant
What & why
A historical query (
run_query_at, or a query atReadTarget::Snapshot) could read a dropped property's values through a property later re-added with the same name. It plans against the current schema, and its tables were already rebound by table identity, but its properties were matched to stored columns by name only. Invariant 6 says identity survives renames, not drop and re-add, and is never inferred from a name. This PR makes historical reads resolve each property by the stable property id its column persists (omnigraph.stable_property_id), the mechanism Blob reads already use.Reproduction, before this change: schema
Person { name @key, note: String? }, write Alice withnote = "secret"(commit A), dropnote, re-addnote: String?. Then at A,return { $p.note }returned"secret", and the filter$p.note = "secret"matched Alice.Backing issue / RFC
Changes
stored_fieldincrates/omnigraph/src/engine/typed_value.rsresolves a declared property by its stable id.check_stored_fieldsandcheck_scan_leavesuse it, so every scan, pushed filter, hydration and edge read is covered. System columns carry no id and still resolve by name.Snapshot::bind_catalog_aliases(catalog, live)(catalog crate) now takes the live view the catalog was accepted against. It records each table the historical view pins at a version other than the live one (contract_covers). Only there can the column of a property's name belong to another lifetime. Coverage is decided byDatasetEntry::same_physical_version, the same comparison Blob uses.property 'note' of node type 'Person' does not exist at this snapshotproperty 'remark' of node type 'Person' is named 'memo' at this snapshot; a historical query cannot read a property across a rename… has no persisted property-lifetime witness at this snapshot(the RFC 0033 Blob rule)docs/user/branching/index.md, a re-add note indocs/user/schema/index.md. Developer guide:docs/dev/execution.md. Release note:changelog.d/historical-query-property-identity.fixed.md.Checklist
Local verification
All runs on macOS, debug build, with
RUST_MIN_STACK=16777216as CI sets. Without itcomposite_flowoverflows the 2 MiB default stack.cargo test -p omnigraph-engine --test integration point_in_time::historical_query_binds_properties_by_identity_not_name: fails with name-only matching (returns"secret"), passes with the fixcargo test -p omnigraph-engine --locked: lib 469, integration 608 (8 ignored), schema_apply 32, search 28, writes 57, lance_surface_guards 65, policy_engine_chassis 24, forbidden_apis 35; all passcargo test -p omnigraph-catalog --locked: 198 passedcargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-gqt-served --exclude omnigraph-dst --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints: 4316 passed, 0 failed, 32 ignoredcargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro: cleancargo fmt --all --check,typos,bash scripts/check-agents-md.sh,python3 scripts/check-docs.py,python3 scripts/release_notes.py preview --working-tree: passcargo test -p omnigraph-gqt --locked --lib --test runner_dispatch: 47 + 26 passed with--test-threads=1. In parallel,concurrent_block_refusals(starved_wall, 4 s budget) timed out at load average 15–45 from other builds on the machine.cargo test -p omnigraph-gqt --locked --test gq_logic_tests: 329–331 of 332 passed under the same load. The few failures were cases hitting the 10 s wall budget at 10.01 s, and each passed alone in about 8.5 s. None reads an old snapshot.Notes for reviewers
point_in_time.rs, beside the type rename and re-add test. GQT has no schema-apply step, so a.gqtcase cannot express it. It covers a node projection, a pushed filter, rename, a later addition, a whole node and an edge property, through both historical entry points. The decision logic, including the column-without-id path, is unit-tested intyped_value.rs. Coverage is unit-tested in the catalog. No current writer produces a column without an id, so that path has no integration test.return { $p }is refused at commits made before a property was added (it was an Internal error before). Binding by identity (reading the old column under the current name, nulling absent nullable properties) would touch roughly ten Lance read sites and is left as a possible follow-up.