feat(schema): resolve system column spellings per graph - #548
Draft
azimafroozeh wants to merge 2 commits into
Draft
feat(schema): resolve system column spellings per graph#548azimafroozeh wants to merge 2 commits into
azimafroozeh wants to merge 2 commits into
Conversation
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.
What & why
This PR implements the coexistence layer of RFC 0040 (#546): the engine stops assuming the spellings of its implicit stored columns and resolves them per graph, keyed on the graph's schema IR version. Motivated by #529, whose companion reservation patch this design supersedes.
__id,__src,__dst; every existing graph keeps version 2 and its current spellings, for every table it has and every table it later gains, with no migration anywhere.Catalog::system_columns); roughly one hundred hardcoded"id"/"src"/"dst"occurrences across the engine, compiler lowering, CLI, and server now route through it._are rejected at new schema admission, subsuming the per-name Lance reservation list; on new graphsid,src, anddstbecome ordinary property names._row_idcan still be restated), still rejects the exact Lance virtual names and the legacy spellings, and additionally reserves the__spellings the future upgrade must claim.GET /schemagains an optionalsystem_columnsfield (also printed byomnigraph schema show) so multi-graph clients discover each graph's spellings instead of guessing; payloads carry each graph's own column names.Backing issue / RFC
docs/rfcs/0040-system-column-namespace.md, rfc: add RFC 0040, the system column namespace #546); motivated by bug: a node property namedidsilently collides with the implicit physical id column, and @key then fails claiming the property is undeclared #529. The upgrade operation, the@-meta-field query syntax, and the deprecation lint are the RFC's remaining layers and land separately.Checklist
docs/user/schema/index.mddocuments the reserved namespace, per-graph spellings, and the discovery field)Local verification
cargo test --workspace --no-fail-fast— [landing; on-track: only the known sandbox blob failure]cargo test --workspace --no-fail-fast --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints— [landing; this graph exists locally because review caught a failpoints-only break invisible to default features]cargo test -p omnigraph-engine --test legacy_columns --features failpoints— greencargo clippy --workspace --all-targets— zero diagnosticscargo fmt --all --check— cleanNotes for reviewers
id/src/dston a legacy graph fails with an error naming the upgrade, and the__spellings stay reserved on legacy graphs so the upgrade's rename targets remain free.__id_idxon new graphs).SchemaOutput.system_columnsis additive and optional; older clients ignore it, newer clients tolerate its absence.from/toenvelopes and the JSONL edge envelope keep their fixed keys; export JSONL id keys follow the graph's spelling so export/import round-trips are internally consistent.#[cfg(feature = "failpoints")]only and re-validates the flipped IR before any write.