Repository navigation
Conversation
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: keep this RFC in draft and resolve the two contract conflicts below before acceptance. This review covers ee55aa75948c7d911f765685ad2182f9a4205898. The PR changes documentation only. These findings concern the proposed contract, not new runtime failures in this PR.
What this PR means
Today, each graph node belongs to one concrete type. Applications need separate edge families or query variants when the same relationship can involve several types. This RFC lets an interface or union name that group. A query could then traverse one Subject -> Subject relationship across people and organizations.
Each node still lives in its concrete table. The compiler resolves an abstract type into a set of concrete types for one accepted snapshot. Queries read those tables and combine their results. Polymorphic edges store a stable type ID beside each endpoint ID. One graph publication still makes the complete change visible.
This could simplify application schemas and queries. It also makes some formerly single-table work depend on the number of concrete types. The RFC does not implement that behavior yet.
Changes needed before acceptance
- Define how endpoint generalization preserves keyed-edge identity. Adding a tag changes the canonical key tuple. The proposed metadata-only migration leaves the old edge ID unchanged. The unresolved choice of tag spelling does not resolve this migration conflict.
- Define direction for equal endpoint sets. The rule that rejects a source which fits both ends also rejects the promised
Subject -> Subjectrecursion. The current compiler chooses outgoing traversal for equal concrete endpoint types.
The inline comments give the examples, source references, and required checks.
Tradeoffs and long-term liability
- Keeping concrete tables and one graph publication limits liability. It reuses the existing storage and visibility model. It avoids a second registry that maps each node to its type.
- Type sets can remove repeated application edge definitions and query branches. The engine must then support type sets across scans, traversal, mutation, constraints, serialization, and historical reads. That is a substantial maintenance cost, even though this PR only adds prose.
- A stored type tag makes endpoint identity explicit and avoids searching every possible node table. Nullable tags avoid rewriting existing edge data. In exchange, every consumer must interpret an absent or null tag through the same implicit-type rule. Five similar features should reuse one logical endpoint identity and one type-set resolver. Separate rules for cascade, keys, export, and traversal would multiply liability.
- For graph workloads with many small concrete tables, opening N tables can dominate an abstract query. Larger scans also need bounded memory and I/O across those tables. The proposed budgets and one edge scan per window fit this workload. They do not establish a latency improvement. Interface-wide uniqueness adds reads across member tables and graph-level conflict validation.
- Refusing cross-table full-text scores is a sound initial boundary. Lance computes BM25 statistics within each dataset. A later shared search projection would add another derived artifact and its maintenance cost.
The design can reduce application liability, but it increases engine liability. Acceptance should depend on closing the identity and direction rules and keeping the added mechanisms shared. The net addition of 705 documentation lines does not measure the eventual software cost.
Evidence and limits
I checked the repository invariants, testing guide, first-principles guide, and Lance alignment material. I traced the current compiler, keyed mutation and load paths, keyed write matching, test owners, and relevant storage code.
The pinned substrate is Lance 11.0.0, source commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its AllNulls path retains existing fragments. Its Merge validation preserves existing field bindings and requires new field IDs above the prior maximum. This supports the tag-column mechanism. It does not change existing logical edge IDs.
I also checked DataFusion 54.0.0 UnionExec and OmniGraph's partition-zero execution. The RFC's schema normalization and requirement to stream all arms have a concrete basis.
Local checks at the reviewed head:
- All 434 compiler library tests passed. These include the existing outgoing-direction test for a same-type edge.
- Documentation validation passed for 204 Markdown files. Agent-guide links and diff whitespace checks passed.
- An additional key-codec unit run did not reach test execution during its dependency rebuild. I stopped that review-owned build. The identity finding rests on code inspection, not a reproduced polymorphic migration. No temporary source edits remain.
GitHub's GQT run passed and records this exact head SHA. These checks validate existing behavior and documentation. They do not validate the proposed polymorphic implementation. I did not reproduce the RFC author's compression, throughput, or detached-merge measurements. No cross-type performance claim is independently established by this review.
| | Rename interface (`@rename_from`) | supported | none | | ||
| | Add `implements` when the node already declares every inherited property compatibly | supported | none; satisfaction links change | | ||
| | Add `implements` with missing nullable properties | supported | today's add-property path for those properties | | ||
| | Generalize an endpoint from `C` to an interface or union containing `C` | supported | staged `Operation::Merge` adds the tag column; `implicit_type = C` | |
There was a problem hiding this comment.
[P2] Define keyed-edge identity before promising metadata-only generalization
This row marks generalization as supported with only a new nullable tag column. But the keyed-edge rule above also adds that tag to the canonical ID tuple. Consider an existing @key(@src, @dst) edge whose ID is ["alice","acme"]. Adding a type component produces a different ID, regardless of whether the component uses a name or a stable type ID.
The current mutation path derives this ID with canonical_key_id. Keyed writes match on that ID. A metadata-only Merge retains the old ID. A later typed upsert therefore cannot match the old row by its new canonical ID. The loader also rejects an explicit ID that differs from its canonical key, so exported old rows need a defined round-trip rule too.
Choose an identity-preserving encoding, define an explicit identity migration, or exclude keyed edges from this supported migration. The unresolved spelling question does not cover existing IDs. Require an existing keyed edge to survive generalization, typed upsert, export/load, and restart with one logical row and the intended stable identity.
There was a problem hiding this comment.
Addressed in 5ca8cd0 and b132a84. v1 refuses generalizing a keyed or @unique edge; rewriting ids versus keeping legacy ids is now an unresolved question. The prototype reproduced the conflict: after generalization the old row keeps [src, dst] while a later keyed write derives the id with the type, so a Merge load would insert a duplicate. For edges created polymorphic, the RFC now states the export/load round trip and requires the typed upsert, export, load and restart test you listed.
| alternation. Direction is still inferred. When a source's set fits both ends of | ||
| an edge (for example `Mentions: Named -> Note` with `Note implements Named`), | ||
| the traversal is refused unless the other endpoint's declared type decides it. |
There was a problem hiding this comment.
[P2] Exempt equal endpoint sets from the ambiguous-direction refusal
For the next paragraph's RelatedTo: Subject -> Subject, every valid source fits both ends. Any valid destination also fits both ends, so its declared type cannot decide the direction. The stated rule therefore rejects the very recursive traversal that the RFC promises. It would also change ordinary same-type traversal if applied to singleton type sets.
The current resolve_member chooses Direction::Out when the source matches the edge's source type, including equal endpoint types. The existing test_traversal_direction_out asserts that behavior and passed locally.
Define a direction rule for equal endpoint sets, such as retaining the outgoing default, while rejecting genuinely ambiguous overlaps between unequal sets. Include one-hop and recursive tests for equal sets, plus a refusal test for the unequal overlapping case.
There was a problem hiding this comment.
Agreed, fixed in b132a84. Equal endpoint sets keep the outgoing reading, today's rule for same-type edges (test_traversal_direction_out unchanged and passing); only unequal overlapping sets are refused unless the other endpoint decides. The recursion was also blocked by #885's multi-hop check, which compares binding names in typecheck, ExpandStep validation and plan admission; all three now use one catalog predicate. Prototype tests cover one-hop and {1,3} over RelatedTo: Named -> Named across Person, Organization and Note with colliding ids, and refusal over Mentions: Named -> Note unless a Person destination decides.
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve this follow-up as a draft RFC. Both earlier P2 contract findings are resolved. I found no new blocking design defect. One optional clarification is inline. Before merge, correct the existing PR-title failure: use rfc: instead of docs(rfcs):.
Reviewed b132a84f993fc018d8312699c7395a83dafd90c3, including the changes since ee55aa75948c7d911f765685ad2182f9a4205898. This is a COMMENT review because the authenticated account is the author. Approval of this documentation update does not establish that the proposed runtime behavior works.
The RFC would let one relationship connect several concrete node types. Applications could replace repeated edge families and query variants with interfaces or unions. Nodes keep their concrete tables. Abstract queries combine those tables, and polymorphic edges store each endpoint's stable type ID beside its node ID. The graph still publishes a complete change once.
The two corrections address their causes:
- Equal endpoint sets now default to outgoing traversal. This preserves today's same-type rule. Unequal overlapping sets need a declared endpoint that decides the reading. A shared catalog predicate would check whether each destination type can start another hop. That covers recursion from a concrete subtype without special-casing
RelatedTo. - Generalizing keyed or unique edges is refused in v1. This avoids keeping old IDs while new writes derive different IDs. The current key encoder and loader's explicit-ID check support that concern. The refusal removes the unsafe migration from the supported contract. It adds no legacy-ID branch. The spelling for newly polymorphic keys remains an explicit decision before implementation, including rename and export/load behavior.
The revised catalog resolver also reduces liability. Validation, cascade, keys, export, change feeds, and traversal would share one endpoint interpretation. Filling live tags removes repeated null-tag rules. Historical images still need the single generalized_from rule. Excluding polymorphic edges from the existing graph index addresses the broader identity problem: its builder interns bare IDs under declared endpoint names.
The tradeoffs remain substantial. Type sets, tags, grammar, wire fields, cursor membership, and historical interpretation add permanent obligations. Many small member tables mean more opens and probes. Constant tags reduce encoded data, but adding a column still creates files per fragment. Five related features should reuse the resolver and type-set operations. Separate mappings in each consumer would multiply maintenance work. The facet clarification below helps keep that choice explicit.
I checked the revised storage proposal against the complete relevant upstream guidance and pinned Lance 11.0.0, source ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its fragment-level add-columns API returns fragment metadata and schema without committing. The dataset-level API commits inline. Its Merge validator checks preserved fragments, row counts, field bindings, and fresh field IDs. These source checks support the proposed composition. They do not prove the complete constant-column detached transaction. The RFC correctly leaves that probe open.
Local validation on the exact head:
RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
cargo test -p omnigraph-compiler --locked --lib -j 3All 434 compiler tests passed, with none ignored or filtered out. This includes test_traversal_direction_out. Documentation validation passed for 204 Markdown files. AGENTS/doc indexes, formatting, the RFC spelling check, and diff whitespace also passed. The isolated checkout is clean. No source or test changes were retained.
This PR changes documentation only. There is no runtime fix here for which GQT can demonstrate a behavioral failure before and success after. The prototype's reported tests are author evidence, not locally reproduced results. Its implementation should include minimal GQT cases for equal-set recursion, unequal-set ambiguity, and colliding IDs in cycle closing and negation. Each bug regression must fail for its predicted behavior before the fix and pass afterward. Detached publication, file preservation, and restart mechanics belong in the existing Rust owners. A parser refusal before a new feature exists would not prove those runtime fixes.
CI is separate evidence. The GQT job is green but explicitly skipped its tests because no GQT input changed. It checked out merge 369ca082db9231289ecc305c78f7f8386dab74cc, whose tree differs from this head. Workspace and the pinned DST suite were skipped too. The title check failed with the prefix error above. I did not reproduce the prototype, constant-column probe, cloud behavior, or compression and throughput measurements. Those remain limits on design acceptance and implementation evidence.
| `EdgeMember` gains concrete `src_type` and `dst_type`, so a polymorphic edge | ||
| expands into one member per concrete endpoint pair (a facet). |
There was a problem hiding this comment.
[P3] Align the IR description with the tag-based prototype
This paragraph still requires one EdgeMember per concrete endpoint pair. The revised expansion and prototype sections say that one tagged scan works without per-pair facets. Please state whether the final plan keeps those facets, and what they own if it does. With N source types and M destination types, this choice adds N × M members before any rows are read. A shared endpoint declaration and tag resolver could avoid that extra representation. This is an optional clarification for the draft, not an observed runtime defect.
ragnorc
changed the title
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve this documentation update while the RFC remains draft and implementation remains not-started. I found no new blocking contract defect. One new optional correction is inline. The earlier optional question about per-pair facets still applies; I have not posted it again. This is a COMMENT review because the authenticated account is the author.
Reviewed f6ffa61d7588fa299376e3cf5352d0c73eb7dda5, including changes since b132a84f993fc018d8312699c7395a83dafd90c3. The direct RFC edit adds the prototype link and revision. The other commit merges main. The PR still changes only the RFC and its index relative to its current base.
The proposal lets one relationship connect several concrete node types. Interfaces or unions would replace repeated edge families and separate query variants. Nodes keep their concrete tables. Each operation resolves abstract types against one accepted catalog. Polymorphic edges store the endpoint's stable type ID beside its node ID. A shared resolver makes traversal, validation, cascade, keys and export interpret that pair consistently. Multi-table writes still publish once.
The earlier identity and direction corrections remain sound. Refusing generalization of keyed or unique edges avoids mixing old IDs with a new canonical key format. Equal endpoint sets retain outgoing traversal. Unequal overlapping sets require enough type information to choose a direction. These rules address the whole class of affected endpoints. They avoid a legacy-ID exception and an edge-name-specific traversal patch. The current compiler's same-type direction and cross-type hop-bound tests passed locally.
I resolved the new link to prototype commit 74cb7a8f172b59e6861850080335de5e095e0859. I read its findings and GQT case. The case uses colliding Person and Organization IDs and checks typed traversal, cycle closing, negation and cascade. The findings explicitly exclude constant-column staging, unions, abstract search, whole-node projection and interface-wide mutations from the prototype. Its reported passing suite counts remain author evidence. I did not execute the prototype.
The main merge changes the migration baseline. Current schema evolution already stages metadata-only Project and Merge operations. Schema apply commits that chain detached and publishes its final pin. The RFC should build on this owner and separate existing all-null additions from the proposed constant-filled tag addition. The latter still needs the stated storage probe. The inline correction concerns this changed baseline, not a new runtime failure.
The liability tradeoff remains explicit. Concrete tables and one endpoint resolver can reduce repeated application schemas and avoid a second identity registry. Type sets, stored tags, historical tag synthesis, wire fields and cursor membership still add permanent engine obligations. Many small member tables increase opens and integrity probes. Constant encoding may reduce bytes, but one added file per fragment still has storage and collection costs. Five similar features should reuse the same resolver, typed identity and schema-evolution path. Separate facet mappings or migration owners would add avoidable liability. No compression, latency or throughput claim was measured in this review.
I checked the repository guidance and relevant upstream material, then rechecked the storage assumptions in pinned Lance 11.0.0 (ab6b5bbe46009ed78746b444df8db59a8bc5d842). Fragment-level add_columns returns fragment metadata and schema without committing. AllNulls retains fragments and requires nullable added columns. Merge validation checks original fragments, row counts and field bindings. These facts support the proposed composition. They do not establish the full constant-column detached transaction or its recovery behavior.
Validation and limits:
- Local, exact head: all 466 compiler library tests passed, with none ignored or filtered out. Documentation checks passed for 242 Markdown files. AGENTS/doc indexes, formatting, RFC spelling and diff checks passed. The isolated checkout is clean. No source or test edits remain.
- GQT: this PR ships no runtime fix, so no behavioral before/after regression proof applies to this documentation change. The eventual implementation needs minimal GQT regressions for observable bugs such as colliding-ID cycle closing and negation. Each must fail for the predicted behavioral reason before its fix and pass after. Missing feature support or a parser failure does not establish that proof. Storage file preservation and crash behavior belong in the existing Rust owners.
- CI: the latest title check accepts the corrected
rfc:title. The earlier title failure is superseded. The ordinary GQT job explicitly skipped tests because no GQT input changed. Workspace and pinned DST were also skipped. Its checkout,f028fce17a0048e12a9f85c93d05a46fc9a189ad, has the same tree as this head. That does not turn skipped suites into runtime evidence.
Local compiler command:
RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' cargo test -p omnigraph-compiler --locked --lib -j 3This review does not qualify a live cloud deployment, the prototype's performance, or the unimplemented durable format. Existing main-only schema apply and single-writer-process control boundaries remain. Immediately before posting, I checked reviews and inline comments again and confirmed the open PR's exact head.
| row ids and indexes on existing columns are kept. Today's add-property path | ||
| replaces the table with an Overwrite instead, which drops index coverage and | ||
| assigns fresh row ids. The forbidden-API registry pins the new primitive's |
There was a problem hiding this comment.
[P3] Update the migration baseline after merging main
The merged code now uses TableStore::plan_schema_evolution and stage_schema_evolution (table_store.rs:4225,4386) for metadata-only Project/Merge changes. schema_apply.rs:791 stages and commits that chain without rewriting rows. This paragraph and the fallback description at lines 353–356 still describe the removed add-property rewrite path. Please update the baseline and identify how the constant-column addition extends the existing schema-evolution owner. Keep that unproven backfill distinct from the all-null Merge that already exists. This will avoid planning a parallel migration path from stale assumptions. This is an optional correction for the draft, not a runtime defect in this PR.
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve this update while the RFC remains draft and implementation remains not-started. I found no new blocking design defect. One optional clarification is inline. This is a COMMENT review because the authenticated account is the author.
Reviewed 724af7a0acede28ef6298f04ab01e9c1b78f9601, including the changes since the last reviewed commit, f6ffa61d7588fa299376e3cf5352d0c73eb7dda5. The PR changes documentation only.
The proposal lets applications bind a query or UI view to an interface, and use one relationship across several concrete node types. Each node keeps its concrete table. A query resolves the member set against one accepted catalog, reads the relevant tables, and combines their results. Polymorphic edges identify an endpoint by its stable type ID and node ID. The new text makes the declared result shape independent of the current members. Adding an implementor can add rows without changing a view's columns.
Correctness and previous comments
Both previous optional comments are resolved. The IR section now specifies one EdgeMember, with row routing by tag. The migration section now starts from the existing metadata-only schema evolution path. Neither needs another inline comment.
The revised design addresses causes at their owning layers:
- A declared interface determines the output schema. The current member closure determines rows. This covers empty, singleton and larger sets without changing the public shape.
- Abstract bindings reach a union plan with per-member table pins, filters, access paths and late materialization. This addresses the class of single-table assumptions. The current HydrateColumns constructor confirms why intercepting one scan function is insufficient.
- One endpoint resolver interprets explicit, null and absent tags. Cascade includes null tags only for the implicit concrete type. Thus deleting Person “alice” must preserve edges to Organization “alice”. Keyed or unique endpoint generalization remains refused in v1; equal endpoint sets retain the outgoing rule. The earlier identity and direction fixes remain intact.
- Change-feed membership changes require a typed refusal and rebaseline. Stored-query binding identity and result descriptors prevent silent changes after names are reused. These are explicit new contracts, not behavior already supplied by today's concrete-name feed filter or query typecheck.
The optional clarification concerns the two public type values: @type changes on rename, while @type_id must remain stable. The lowering description currently maps both to the same column without defining their separate expressions.
Tradeoffs and liability
The update removes a proposed constant-column backfill and a per-pair facet model. It reuses concrete tables, schema staging, and one graph publication. This lowers implementation liability. The null-tag choice avoids writing files during generalization, but makes correct catalog-based interpretation a permanent obligation, including historical rows. A shared resolver is essential.
The feature still adds type-set operations, typed identity through joins and traversal, catalog and wire fields, registry descriptors, and feed membership rules. These costs are justified by stable interface-bound queries and mixed-type relationships. They should remain derived from the accepted catalog. Five similar features should reuse those owners rather than add separate membership or identity maps.
For many small member tables, opens and index probes can dominate latency. For larger scans, concurrency and memory bounds matter. Per-arm pruning and access selection address both shapes. Holding more handles trades retained metadata for fewer opens; the present cache caps handles at 64. The 75-member S3 latency target is an acceptance target, not a measured result of this review.
Validation and limits
I read the current repository instructions, invariants, testing, systems and Lance alignment guides. I reused the earlier complete upstream review and checked the relevant data-evolution, table-schema, DataFusion and performance guidance against the pinned source.
Lance is 11.0.0, source ab6b5bbe46009ed78746b444df8db59a8bc5d842. OmniGraph's schema evolution planner and staging compose with Lance's Merge validation and missing-field null reader. This supports unchanged fragments with a new nullable tag. I also checked DataFusion 54.0.0 UnionExec: its partitions span the inputs, whereas OmniGraph executes root partition zero. The RFC correctly requires an operator that streams every arm.
Local command on the exact reviewed head:
RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
cargo test -p omnigraph-compiler --locked --lib -j3466 passed; none failed, ignored or filtered out. python3 scripts/check-docs.py passed for 242 Markdown files. bash scripts/check-agents-md.sh, typos docs/rfcs/2026-10-07-polymorphic-types.md, cargo fmt --all --check, and git diff --check b63a930debdd5c737edf0def1aa52fe786326435...HEAD also passed. The isolated checkout is clean; no temporary source or test edits remain.
There is no runtime bug fix in this documentation diff, so a GQT failure-before/pass-after proof is not applicable. I inspected the tests and findings at prototype commit 8f9f231d, including null-tag migration and colliding-ID cascade. I did not run that prototype. Its suite counts and compression, throughput and cloud measurements remain author evidence. The implementation must include minimal GQT cases for observable contracts and behavioral before/after proof for its bug fixes; file preservation and detached publication belong in the existing Rust owners.
CI is separate evidence. The ordinary GQT job is green but explicitly skipped the tests. Workspace tests and pinned DST were skipped too. Storage Upgrade Compatibility ran and passed. These jobs used merge ad78c24b3eacde74f80ed8a91ca4cad0b45e9103, whose tree differs from the reviewed head. They do not prove the proposed polymorphic behavior. The title check now passes.
| A polymorphic edge stays one `EdgeMember`; its rows are routed by tag at | ||
| execution. The prototype needed no per-pair facets. | ||
| - `$x.@type` and `$x.@type_id` lower to literals for a singleton set and to the | ||
| query-only `~node_type` column otherwise, as `$e.@type` does with |
There was a problem hiding this comment.
[P3] Define separate name and stable-ID expressions
Optional clarification for the draft: the public contract gives @type the canonical name and @type_id a decimal stable ID, but this rule lowers both to the same ~node_type column. For a multi-member binding, projecting both or filtering by one requires distinct expressions; otherwise one value is lost, and a rename can change the identity used by clients or tie-breaks. Please specify the column's value and how the other public value is derived from the same accepted catalog, including per-arm constant folding and expansion output. This can keep one identity source without a second registry. The implementation should include a small GQT case that projects and filters both values, then renames a member: the name changes, the stable ID does not, and the behavior also holds when the interface grows from one member to two.
724af7a to
12f70b0
Compare
Contracts from a review against UI components bound to interfaces, which packed references do not cover: - Result schemas come from declared types and never depend on membership; a redeclaration may not drop an inherited @key or @unique. - `@type_id` is an expression separate from `@type`: the identity source is `~node_type_id`, the arm's StableTypeId or the packed destination's type half; `~node_type` is derived from it through the captured catalog. - The union scan resolves filter, index access, statistics and late materialization per arm; on main every single-table shortcut broke. - Pruning runs after parameter binding, for reads and interface-wide mutations; a non-member type is refused. - The catalog route's field list and schema hash, per-commit feed expansion with a membership refusal, refusal of policy-denied members, interface-property migration rows, one namespace with stored-query pinning, and concurrent arms with a handle cache sized to the catalog.
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve this documentation update while all three RFCs remain draft / not-started. I found no blocker to retaining them as proposals. Two nonblocking draft follow-ups are inline: the paging claim and the interaction with group commit. These need resolution before accepting the combined runtime contracts. This is a COMMENT review because the authenticated account is the author.
Reviewed 498ac7ce212678793c880274912a45784a446618, including the changes since 724af7a0acede28ef6298f04ab01e9c1b78f9601. The diff from base 47dfe7b4a00c1d6405bc076dc6f4f549ae6fc06c contains three RFCs and their index; it changes no runtime code.
The PR would let a query or UI view target an interface across several concrete node tables. One relationship could replace a family of nearly identical edge types. This revision adds a much larger prerequisite: every row receives an engine-issued integer ID, and every endpoint packs its type and row ID into one value. A separate per-table sequence reserves blocks of IDs. Applications keep natural-key addressing for keyed nodes, but unkeyed node handles and all edge handles change. Operators must convert existing graphs before ordinary writes resume.
The earlier name-versus-ID comment is resolved. The new lowering rule gives @type_id one stable identity source and derives @type through the captured catalog. The result-shape rules also cover empty, singleton and larger member sets without changing an interface's declared columns.
The important correctness choices are coherent at the proposal level:
- One checked NodeRef owns endpoint identity across traversal, joins, dedup, cascade and export. This addresses collisions between equal keys in different types. Packed endpoints also remove the old keyed-edge generalization exception, after the integer upgrade.
- Sequence reservations grant numbers only on confirmed conditional-write success. Lost acknowledgements grant nothing, missing state is not reset, and local callers share a root/table guard. These rules match the actual storage adapter: remote updates use backend ETags; local compare-and-replace requires caller exclusion. They do not create local cross-process fencing. Block caching has established precedent in PostgreSQL and Vitess, including gaps and allocation order that differs from commit order.
- Merge correspondence is needed once independent inserts can have equal natural keys but different IDs, and keys can later change. Current three-way classification aligns string IDs directly. The proposed relation preserves that logical alignment across repeated merges. Publishing it with the row tables, carrying deleted-row equivalences, and normalizing endpoint images address the general problem; a one-time endpoint rewrite would not.
- The upgrade extends the existing UpgradeIntent owner with a frozen conversion plan and confirmed grants. Its fence, resumable staging and single activation are necessary obligations. A successful string-to-integer conversion alone would not prove crash recovery or retained-history safety.
Tradeoffs and liability. This removes repeated endpoint interpretation and keeps one concrete table per type. It adds permanent sequence state, a correspondence dataset, old-format readers, conversion recovery, new public-handle rules, and finite 16/48-bit capacities. The shared schema allocator also spends IDs on properties and incarnations. This is a larger lifetime commitment than the previous tag-column proposal; net simplification and speed are not yet proved. Five more operators should reuse the same NodeRef and member resolver. Five more features must not add their own counters, identity maps or migration protocols. The sequence and correspondence state have distinct jobs: allocation grants versus accepted merge equivalence.
Bulk ingestion can amortize reservations. Short-lived writers consume unused grants, and interface reads still open several datasets. The proposed million-allocation bound is arithmetic, not a benchmark. The local prototype used the earlier identity representation. It does not establish integer conversion, remote allocation throughput, bounded merge-closure cost, or the 75-member S3 latency target.
Alignment. I checked the open-PR inventory. #785's group-commit admission currently relies on disjoint inserted IDs; natural-key conflicts must survive the new surrogate-ID model (inline). #951 explicitly proposes replacing this RFC's policy-denied-member behavior with actor-view semantics, so that remains a joint design decision. #963's live-branch schema work must not silently relax this upgrade's main-only fence. #964 and #677 need the correspondence pin and format boundary preserved in history, adoption and restore. #839's plan acceptance must cover union arms and their per-table predicates; #792/#793 remain the owners of lexical and ranking semantics.
I checked the pinned, unmodified Lance 11.0.0 source (ab6b5bbe46009ed78746b444df8db59a8bc5d842), including scalar-index support and transaction boundaries, against the upstream storage, index and versioning documentation. The Lance 13 release includes manifest-discovery, scalar-index and restore-conflict changes. Those are future requalification work, not evidence that this graph-wide identity protocol is supplied by Lance.
Validation and limits. On the exact head, in a clean isolated checkout:
/tmp/review-916-docs/bin/python scripts/check-docs.py: passed, 258 Markdown files. The initial attempts found missing Python dependencies and then a missing localv0.13.0tag; using the docs environment and fetching the required tag resolved both setup failures.bash scripts/check-agents-md.sh,typoson all three RFCs, andgit diff --check 47dfe7b4a00c1d6405bc076dc6f4f549ae6fc06c HEAD: passed.- With
CARGO_TARGET_DIR=/tmp/review-928/target,RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable',RUST_MIN_STACK=16777216, andRUSTC_WRAPPER=:cargo test --locked -p omnigraph-storage --lib remote_conditional_updates_require_backend_etags -- --exact tests::remote_conditional_updates_require_backend_etags --nocapture: 1 passed.cargo test --locked -p omnigraph-storage --lib contract_suite -- --nocapture: local and in-memory contracts passed. The Azure wrapper reported success but explicitly skipped its body because no test container was configured; it is not Azure evidence. - The CI run associated with this head is green. Workspace tests were skipped. The GQT run says no GQT input changed and skipped the actual suites; green wrappers are not regression proof.
No runtime bug fix exists between the stated base and head, so a behavioral before/after GQT proof is not applicable to this PR. The new features still need the RFCs' minimal GQT cases and Rust storage/merge/failpoint owners when implemented. I added no temporary source or test edits. No new identity, sequence, migration or performance behavior was executed in this review.
| Ordering ties break on the packed reference, so an abstract binding's total | ||
| order is its `order` keys, then `@type_id`, then the row id. That makes keyset | ||
| paging by filters exact across members, because each arm evaluates the filter |
There was a problem hiding this comment.
[P3] Make the resume predicate match an observable ordering tuple
Optional correction for the draft: a total order on the packed reference does not by itself make paging by public filters exact. The identity RFC keeps a keyed node's @id equal to its natural key and hides the numeric row ID. For two rows tied on the explicit order keys, IDs 1 and 2 can have public keys zeta and alpha. After returning zeta, a lexical @id > "zeta" filter misses alpha. Please state how the client expresses the hidden tie-break, or narrow this claim to queries with an explicit unique public ordering tuple whose comparisons the resume filter repeats. The deferred opaque cursor can carry the internal tuple. This avoids adding another public identity API just to support paging. Once paging ships, a minimal two-row GQT should use key order opposite to allocation order and verify both pages without loss or duplication. There is no new paging implementation to test in this PR.
There was a problem hiding this comment.
Narrowed. Paging by filters is exact only with a unique public set of ordering fields that the resume filter repeats. For keyed members that is the order keys, then @type_id, then the interface key. The deferred paging cursor carries the internal tuple. The two-row GQT, with key order opposite to allocation order, is recorded for when paging ships.
| New rows take their `__id` from the sequence cache. A moved graph head returns | ||
| `ReadSetChanged` under `ExactGraphHead`; retries capture a fresh snapshot, | ||
| resolve keys and validate again, and restage where required. A retained | ||
| confirmed allocation can supply the same ids, but cannot validate stale reads. |
There was a problem hiding this comment.
[P3] Record the changed admission premise for group commit
Optional draft alignment, but required before combining this design with #785: the group-commit RFC admits append-only writes when their inserted ID sets are disjoint. Its proof equates ID absence with natural-key absence. Here two writers can both find Person.slug = "acme" absent and receive different sequence IDs. Their ID sets are disjoint, but their natural keys conflict. The current ExactGraphHead retry rule remains safe; the pending batching relaxation would no longer be justified. Please add an explicit dependency/compatibility rule: group-commit admission must compare the natural-key tuples that staging tested, including normalized endpoint/key tuples for keyed edges, or classify those writes as exclusive until that proof exists. Keep this in the existing admission owner. Require a focused concurrent-write test with one duplicate key and distinct grants, plus a distinct-key control. A sequential GQT cannot establish this interleaving.
There was a problem hiding this comment.
Added to the identity RFC's Writes section. Group commit must compare the natural keys that staging tested; for keyed edges that is the packed endpoints plus the key properties. Otherwise it treats keyed inserts and upserts as exclusive. Unkeyed inserts are unaffected, and a concurrent key change is still refused, because endpoint tables already count as validation reads. Test: two entries inserting one duplicate key under different id grants, plus a distinct-key control.
|
@azimafroozeh I read your rework ( What
Happy to adjust any of it to fit your direction. Assessment Packed references are the right root-cause fix:
Three points I'd push on:
Edited: removed a wire-migration note and an optional type-id counter ask; measured headroom and further findings are in the follow-up below. Evidence: the probes are in |
|
@azimafroozeh Follow-up on the points above, investigated against 1. Phase 1 before the storage upgrade (
2. Merge correspondence is correct, but heavier than the common case needs. Today, merge already behaves as a natural-key join, because the cursors join on
From analysis, not yet tested:
Alternatives considered:
A simpler first step: align rows by surrogate first, then by (type, key) in all three images, with no stored state. With immutable keys, this reproduces today's semantics exactly. With mutable keys, it is still correct for every row that was never coalesced. The one degraded case is a coalesced row whose key later changes; I'd make that a typed conflict at first. If cross-branch key changes matter, carry correspondence as an alias list on the surviving row ( 3. Write cost should stay flat if key resolution reuses the existing probes. Measured today: an append of 1,000 edges between existing keyed nodes makes 4 data reads across 3 tables. Endpoints are already resolved in one batched RI probe per node table, and the same per-type key probe can also serve the 4. Type-id headroom is fine; no separate counter is needed. The personal graph's dataset paths (from |
Abstract type sets resolve against the actor's schema view, and view.restricted replaces the policy_denied_member refusal and the policy_filtered marker: the refusal disclosed that a hidden member exists, and its only escape required naming a type the caller may not know. An inherited placement cannot be dropped or added per member, and the catalog route, schema hash, descriptors and feed refusals describe the schema view. Resolves the interface-named policy rules question.
…d scorer Rechecked against the pinned Lance 11.0.0 source and a probe on the prototype branch: - Packed UInt64 endpoint filters (type range, =, IN, values above 2^63) are served by scalar indexes. Endpoint columns use BTREE, since a bitmap loads every distinct key, and packed filters are built from u64 literals. - Row addresses and stable row ids are per dataset, so neither keys rows across member tables. - A handle reopen costs a HEAD because the manifest size is not passed. - bm25 across members becomes a phase 3 feature: one corpus scorer from all members' inverted indexes (build_global_bm25_scorer) injected into each arm (with_base_scorer), replacing the derived projection for cross-type full-text ranking. Verified in source, not yet run.
|
@azimafroozeh I rechecked the Lance-dependent assumptions in both RFCs against the pinned Lance 11.0.0 source, and with a direct probe on the prototype branch ( Holds (probe plus source):
For the identity RFC:
A design opportunity, now in the RFC (
This is verified in source only; I haven't run it. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| - **Search across members (phase 3).** `bm25` over an abstract binding builds | ||
| one corpus scorer from every member's inverted index with Lance's | ||
| `build_global_bm25_scorer`, which sums document counts, token totals and |
There was a problem hiding this comment.
[P1] Build the shared scorer from the actor's statistics source
Blocking. This paragraph sums index-wide statistics "from every member's inverted index". The same revision adopts #951 for visibility. #951's non-interference contract covers search ranks. Its "Full-text statistics" section requires view-independent statistics (public or uniform) for restricted actors on placed types. Bypass actors keep index-wide statistics. Summed index-wide statistics carry the term frequencies of rows the actor cannot see. If "every member" means the declared closure, they also carry hidden member types. The Policy section says those types are absent from the set. So the two contracts conflict in this document.
The affected class is every score computed across tables: bm25 across members now, and later rrf or compound full-text queries over N arms.
Please state the rule here, once:
- The scorer covers the type set after resolution against the actor's schema view. It never covers the declared closure.
- Each member's statistics come from the source rfc: granular permissioning #951 selects for that actor.
- When the source differs between arms (one arm restricted, one not), all arms use one source, so the scores stay comparable. View-independent statistics for all arms is the safe choice.
- Bypass actors keep the summed index-wide scorer.
Test, with the other actor-view server tests: a restricted actor's bm25 scores over an interface do not change when a hidden row, or a hidden member type with matching rows, is added.
There was a problem hiding this comment.
Fixed in 014f921. The scorer now covers only the member types the actor can see, never the full declared set. Statistics come from the source #951 picks for that actor. One query uses one source for every type: if any type needs view-independent statistics, all of them use it. Bypass actors keep the summed index-wide statistics. The hidden-row and hidden-type test is under Tests to extend → Server.
| per-term document frequencies over any list of index handles. Each arm's | ||
| full-text plan takes that scorer through `with_base_scorer`, so every arm | ||
| scores against the same statistics and scores merge like distances. The |
There was a problem hiding this comment.
[P1] On Lance 11 the arms do not score against the same statistics
Blocking, because merging the arms' scores depends on this claim. Two cases break it.
Unindexed rows. In Lance 11.0.0 the scanner adds a FlatMatchQueryExec for rows that no segment covers. That branch takes the injected scorer and adds its own dataset's unindexed rows to it (lance-index scalar/inverted/index/flat_search.rs: initialize_scorer, called from flat_bm25_search_stream_with_options_and_scorer). So each arm scores with "shared statistics + its own unindexed rows". In OmniGraph this is the normal state after a write: commit_staged_detached commits an untrained segment, and "postings still come from explicit reconciliation" (crates/omnigraph/src/table_store.rs, doc comment near line 3888).
I ran a temporary probe of that public function, with one shared scorer (100 docs, document frequency 5) for two arms. The same text, graph database, got these scores:
- 2.91 through the shared scorer (the indexed path);
- 2.75 in an arm with 1 unindexed row;
- 1.00 in an arm with 50 unindexed rows.
So a member with a write backlog ranks its own matches lower, and the merged order depends on index coverage. Invariant 6 says coverage can change cost, not results.
Pruning. Line 492 says pruned members are never opened. If pruned arms leave the scorer, an optimizer decision changes the scores: @type_id = $t, and an equivalent filter that does not prove pruning, give different scores for the same row. If pruned arms stay in the scorer, they must be opened.
Please define the corpus as a contract, not as a mechanism:
- The corpus is the resolved type set after rebinding. Filters, including type tests, never change it. This matches one table today, where a filter does not change the statistics.
- Unindexed rows are in the corpus for every arm, or the text says that scores are approximate while coverage is partial and gives the bound. For the first option: one pass collects each arm's flat statistics, then every arm scores with the total. Lance's
SharedFtsScorerdoes this inside one dataset. - A pruned member contributes its index statistics without a data scan.
When this is implemented, extend bm25_distinct_scores_rank_descending.gqt to an interface fixture. Check that the scores do not change when a type-test filter is added. If GQT cannot keep one member's postings unreconciled, use the search.rs owner for that case.
There was a problem hiding this comment.
Agreed, and confirmed in the Lance source. One addition: for row-level documents, Lance 11 shares no scorer between one table's indexed and flat branches either. It does so only for list elements, and that is unchanged in 13.0.0. So single-table bm25 already depends on index coverage. The corpus is now a contract: the resolved type set after rebinding. Filters and type tests never change it, and skipping a type removes its rows but not its statistics. Unindexed rows count for every type: one pass collects statistics, then everything scores against one total. Tests: the GQT on an interface with a type-test filter, and the indexing-backlog case in tests/search.rs.
| fuzzy expansions. Lance's scanner does not expose the hook, so the engine | ||
| builds those plans itself. This replaces a derived projection for | ||
| cross-type full-text ranking; the API is verified in source, not yet run. |
There was a problem hiding this comment.
[P3] Give scorer injection one owner, preferably in Lance
Optional, but it changes the long-term liability. Scanner::full_text_search does more than score. In Lance 11.0.0 it also:
- selects the segments;
- adds the flat branch for unindexed fragments;
- blocks overlay-stale rows (
fts_overlay_plan, thenMatchQueryExec::with_overlay_block); - falls back to a full scan for legacy segments.
All of these are in lance/src/dataset/scanner.rs. MatchQueryExec::with_overlay_block is pub(crate) in 11.0.0, 13.0.0 and 14.0.0-beta.19. So a plan that the engine builds cannot do that step through the public API. A hand-built plan copies these rules, and each Lance upgrade must recheck them. That conflicts with this RFC's invariant text, "Nothing duplicates a Lance primitive".
#951 plans the same engine-built full-text plan for restricted actors. Two hand-built planners will drift. Please name one owner:
- Best: an upstream scanner option that accepts a base scorer. The engine then supplies the statistics and Lance keeps the planning. Record it as a phase-3 dependency or in
blocked_on. - Otherwise: one engine module, shared with rfc: granular permissioning #951. Its input is the resolved type set and the actor's statistics source. Its output is one plan per arm. A differential test compares its rows with
Scanner::full_text_searchon a dataset that has unindexed fragments.
There was a problem hiding this comment.
Took the upstream route, and plans the engine builds itself are now a rejected alternative. Phase 3 depends on a scanner option that takes a corpus scorer and uses it unchanged in both branches. It also needs a call that returns one dataset's statistics for the query's tokens. This is recorded under Rollout phase 3, not blocked_on, because phases 1 and 2 don't need it. One engine module, shared with #951, is the only code that chooses or sums statistics. with_overlay_block is also pub(crate) in 13.0.0. 14.0.0-beta.19 isn't on crates.io, so I cited 11 and 13.
| of opening every member. A bound type that is not a member of `I` is a typed | ||
| refusal, not zero rows. A member whose predicate matches nothing stages |
There was a problem hiding this comment.
[P3] Give a hidden member's @type_id the same refusal as an unknown ID
Optional and small. The Policy section makes a hidden type name fail like a misspelling. That covers literals. Here $t is a value, and it is checked after parameter binding. Please state that a $t that names a hidden member gets the same refusal, byte for byte, as an ID that is not in the actor's catalog. #951 requires this for hidden names. Without it, a component can probe type IDs and learn which hidden members exist.
A server test with the other actor-view cases can pin it: bind $t to a hidden member's ID and to an unused ID, then compare the two error bodies.
There was a problem hiding this comment.
Added. A bound $t or @type value that names a hidden type gets the refusal of a value missing from the actor's catalog, byte for byte. A server test compares the two error bodies.
… cost as a change Bypass actors, and reads no view restricts, sum index statistics and unindexed rows over the resolved type set. Restricted actors use Actor views' view statistics for every arm, which replaced public and uniform statistics there. The warm-reopen paragraph now states today's HEAD and GET for a detached pin and proposes the commit-handler change. The public paging tuple names every key property and the unkeyed Type:number sort. A hidden type id's refusal matches an unknown id apart from the echoed value.
A non-negative i64 still coerces to UInt64; a literal Lance cannot coerce, such as a wrapped negative i64 or a Decimal128 above i64::MAX, loses the index. Packed values above i64::MAX come from every type id from 32768 up.
|
Optional 3: phase 3 names both Lance additions, and |
| of its rows. It sums the members' counts, and every arm scores against | ||
| that total. The flat operator adds its own input to a supplied base | ||
| (`lance-index` `flat_search.rs` `initialize_scorer`), so each arm's base | ||
| is the other arms' sum. Hidden rows, hidden member types and index |
There was a problem hiding this comment.
[P1] The flat base and the scanner corpus are different values
Blocking. This sentence gives each arm the other arms' sum, because initialize_scorer adds its own input. The module sentence below says the output is one scorer. The scanner option uses that corpus unchanged.
initialize_scorer (lance-index flat_search.rs) copies the base's document count, token total and document frequency, then adds every input row. A flat call whose base is already the full population counts this arm twice. Arms of different sizes then disagree. That is the split the earlier probe measured (2.91, 2.75, 1.00).
The unchanged scanner must receive the full population, including this arm. The flat call must receive the other arms only. Record both in the module: the per-arm counts, and their sum. Pass the sum of the other arms to the flat call. Pass the sum of every arm to the scanner.
The bypass bullet is short one field. It adds "document and token counts" for unindexed rows. initialize_scorer also adds document frequency: one per input row whose count for that query token is greater than zero. Index statistics already carry document frequency for indexed rows. The unindexed addition has to carry it too. Without it, an unindexed match is scored as if the term were rarer than it is, and two members with different backlogs disagree. Name document frequency in that bullet, as the restricted bullet already does.
| `e_tag`, on an ordinary open of a detached pin. `get_manifest` then uses | ||
| the session cache, and a later open of that pin sends no request. A pin's | ||
| `manifest_size` and `e_tag` are optional (`TableVersionMetadata`), so a pin | ||
| without them keeps the HEAD and the GET. |
There was a problem hiding this comment.
[P3] A missing e_tag does not keep the HEAD
Optional. Dataset::get_manifest skips the session cache only when size is absent (lance dataset.rs). The cache key stores e_tag, and a null e_tag still caches. open_with_size skips the HEAD when the size is known (lance-io object_store.rs). A pin with a size and no e_tag does not send the HEAD. Say that only a missing size keeps the HEAD and the GET.
The handler cannot return the pin yet. resolve_version_location receives the version (lance_clone.rs). open_pinned_dataset receives the version and not TableVersionMetadata (instrumentation.rs). Phase 1 needs a carrier from the caller that holds the pin into the handler, as CLONE_CONTEXT carries a clone location. The handler has no pin to return until that carrier exists.
| with no query filter, and sums them. Every arm scores against that | ||
| total. | ||
| - **A restricted actor, when the view restricts any member the query | ||
| reads:** view statistics for every arm, as Actor views defines them for |
There was a problem hiding this comment.
[P3] Choose the procedure from the resolved type set
Optional. "Any member the query reads" can mean the arms that return rows. Take an interface {Public, Secret} where the view restricts Secret, and a query that prunes Secret with @type = Public. If the procedure then sees no restricted arm, it takes index statistics. The next sentence adds the pruned member's population. Secret's index statistics include hidden rows.
The corpus above is the resolved type set, and pruning removes rows, not statistics. Write the condition as "any member of the resolved type set, including a member pruning removed." The same view-statistics procedure then covers every arm.
| traversal probes) is built from `u64` literals. Lance coerces a literal to | ||
| the column type only when the value fits (`lance-datafusion` | ||
| `safe_coerce_scalar`): a non-negative `i64` still coerces, but a literal it | ||
| cannot coerce, such as a wrapped negative `i64` or a `Decimal128` above |
There was a problem hiding this comment.
[P3] Every Decimal128 loses the packed index
Optional. safe_coerce_scalar returns None for every Decimal128 when the target is UInt64, including a value that fits in i64 (lance-datafusion expr.rs). maybe_scalar then builds no indexed expression (lance-index scalar/expression.rs), and the filter runs as a scan. The rows stay correct. The index is what is lost.
A SQL integer above i64::MAX is one way to get a Decimal128. A smaller Decimal128 takes the same path. Say that the type fails coercion, and that SQL uses that type for an integer above i64::MAX. The non-negative i64 rule and the type-id 32768 bound are right: 32768 << 48 is 2^63.
|
Recommendation: request changes before this revision is accepted. The three RFCs can stay drafts. This note is a comment. The tool that posts it cannot set a review state. Reviewed commit What changed The revision answers the 2026-10-18 review. Each actor class has one corpus procedure. Bypass actors, and reads that no view restricts, sum index statistics and unindexed rows. Restricted actors use view statistics for every arm, which is the population #951 now specifies. The warm-reopen paragraph states today's HEAD and GET, and moves the commit-handler change to phase 1. The public paging tuple names every key property, and an unkeyed Earlier findings
Blocking
Optional
Tradeoffs and liability The revision removes the false claim that a warm reopen is already free. It removes the single collection step that put hidden term frequencies into a restricted score. It adds one procedure per actor class, aligned with #951's withdrawal of public and uniform statistics. The new liability is the scorer value. The flat operator adds its input. The future scanner uses a corpus unchanged. Those callers need different bases. "One scorer" names one of them. An implementer who passes that scorer to This RFC withholds multi-member No performance number is claimed for the pruned-member pass. The text says that pass is not measured. A selective query can still read every member's visible text. I make no cost claim. Lance alignment Pinned Lance is 11.0.0 (
OmniGraph at this head: #951 at Validation Local, in a detached worktree at
CI at this head is green: CI, GQT, PR Title. Regression proof This delta fixes no runtime behavior. A GQT failure-before and pass-after does not apply. The concrete claims about Lance 11, Lance 13's scorer visibility, and current OmniGraph were checked by reading the pinned sources and the code at this head. I did not add a temporary test, and I did not push one. Limits I did not re-run the numeric probe (scores 2.91, 2.75 and 1.00). I did not open a dataset on S3. I did not run the prototype. I did not execute #951. Lance 13 was read from the |
…spaces' type references Stored-query pinning records the kind and id of every type and edge name a query resolves, not only bindings. The migration table refuses only cross-kind reuse in one apply, so a same-kind rename and re-add, including a move under Schema namespaces, is caught here. Type references share Schema namespaces' qualified spelling, and item 14 no longer says namespace.
Qualified names from Schema namespaces fit Type:number handles when the number follows the last colon. A namespace move is a rename, so it respells every unkeyed node and edge handle of the type.
| `ownedBy`: the migration table refuses only cross-kind reuse in one apply, so | ||
| pinning is what catches same-kind reuse. A move under | ||
| [Schema namespaces](https://github.com/ModernRelay/omnigraph/pull/1015) is a | ||
| rename and relies on the same guard. |
There was a problem hiding this comment.
Posting test for review delivery. This comment will be replaced by the full review if the API accepts it.
There was a problem hiding this comment.
[P1] Pinning does not see a same-kind re-add in one apply
Blocking. This paragraph says one apply can rename edge OwnedBy away, add a new edge OwnedBy, and give the stored name a different id. Pinning then refuses the stored query. The migration table refuses only a cross-kind reuse, so pinning is the guard. The decision log puts a namespace move in that class and says pinning catches it.
resolve in crates/omnigraph-compiler/src/catalog/schema_ir.rs matches a desired declaration to an accepted type by kind and name first. A desired edge OwnedBy with no hint takes the accepted OwnedBy id. edge HeldBy @rename_from("OwnedBy") takes that same id. The function then returns accepted edge 'OwnedBy' is consumed by more than one desired declaration. The apply stops in the resolver. Pinning does not run.
Temporary tests in catalog::schema_ir::tests, run at this head and then removed:
same_kind_rename_and_readd_in_one_apply_consumes_the_id_twicereturns that error.same_kind_readd_across_two_applies_mints_a_new_id: the rename apply keeps the id. A later apply that addsOwnedBymints a new id. Pinning is the guard for that second apply.cross_kind_name_reuse_in_one_apply_is_admitted:node Eventrenamed toGathering, plus a newinterface Event, resolves. The interface receives a new id. The table's cross-kind refusal is a new rule. The resolver admits this case today.
#1015 at 2e672c0c says a move keeps StableTypeId. A stored query that still resolves the name, for example through use crm;, keeps that id. Pinning accepts it. A query that still says the old unqualified name fails typecheck. Pinning refuses the case where the old name resolves to a different id. #1015 also says ad-hoc query text is resolved again and can change meaning. This paragraph omits that path.
Write one rule:
- A pure rename, including a namespace move, keeps the id. A stored query that still resolves to that id is accepted. A stored query whose name no longer resolves fails typecheck.
- A later apply that adds a declaration with the released name gives that declaration a new id. Pinning refuses a stored query whose recorded name now has a different kind or id.
- State what one apply does when the desired schema contains both the rename and a new declaration with the old name. Today the resolver rejects it. If the design wants a new id on the new declaration, say that
@rename_fromwins over the exact name match, and nameresolveas the function that changes. - Item 14 says "same-apply reuse refused" with no kind. The table and phase 0 refuse cross-kind reuse. Use those words in item 14.
- The cluster test should accept a pure rename and refuse the second-apply re-add. It should follow the one-apply rule you choose. Cover bindings, rebindings, alternation members and load targets. The paragraph records those names. The test lists a subset.
- The decision log sentence that a move is caught by pinning should match this rule.
#1015's pinning list omits rebindings and alternation members. Keep one list in both drafts.
This is invariant 6. Identity is the stable id. The resolver owns the one-apply match. The registry owns the later check. A sentence that gives the later check the resolver's job can refuse a legal move, or miss the two-apply retarget.
| | 11 | Live views | Expanding at cursor creation freezes membership. | Per-commit expansion and membership refusal. | | ||
| | 12 | Remove-`implements` dependency | "Depends" was undefined. | Typecheck-based rule and registry pinning. | | ||
| | 13 | Per-type policy | A denied member was silently filtered. | Member sets from the actor's schema view, with `view.restricted` as the completeness signal. | | ||
| | 14 | Shared core name collision | `interface Event` and `node Event` coexist on main; reusing a renamed name changes stored queries silently. | No interface shares a name with a node or edge type; same-apply reuse refused; registry pinning. | |
There was a problem hiding this comment.
[P3] Compare canonical names
Optional. This cell says an interface shares no name with a node or edge type. #1015 applies that rule to the canonical name, so crm::Event as an interface and core::Event as a node can coexist (2e672c0c, "Kinds"). Write "canonical name" in this cell and in the schema bullet ("A new schema may not declare an interface and a node or edge type with the same name"). The final segment can repeat. "same-apply reuse refused" also needs the kind limit from the migration table: cross-kind reuse in one apply.
|
Recommendation: request changes before this revision is accepted. The three RFCs can stay drafts. This note is a comment. The reviews API returned 403, so this account cannot set a review state. Reviewed commit What this change does A stored query names types and edges. A later schema change can make a name point at a different declaration. This revision records the kind and the stable id of every name the query resolves, and it refuses the apply when a name resolves to a different kind or id. It also splits a public handle at the last colon, so Earlier findings
Blocking
Optional
What holds in this delta
Tradeoffs and liability The revision aims at one guard for a renamed name: record the stable id, and refuse when the name later resolves to a different id. That is the right layer for a stored query. Invariant 6 says identity is the stable id. A name is not a second identity. The new text gives that guard a case the resolver already rejects, and it puts a namespace move in the same class. #1015 says a pure move keeps the id and keeps a stored query that still resolves. An implementer who follows this decision log refuses that move. An implementer who follows the resolver never reaches pinning for a one-apply re-add, and can miss the two-apply re-add, which is the case pinning exists for. Five later name features (namespaces, interfaces, unions, alternations, actor views) then each grow a private guard. One change to The handle rule is one parser. It removes a second split that would cut The open scorer finding is the larger engine liability, and this delta leaves it in place. One population record (per-arm counts and their sum) serves the flat call and the scanner. "One scorer" names one of the two values. No performance number is claimed in this delta. I make no cost claim. Validation Local, detached at
Temporary compiler tests, added in
Pinned Lance is 11.0.0 ( CI at this head is green: CI, GQT, PR Title. The GQT ordinary job took "Skip when no GQT input changed" and skipped the test steps. Green CI is not evidence for this design. Regression proof This delta fixes no runtime behavior. A GQT failure-before and pass-after does not apply. GQT cannot express schema-identity assignment. That contract is the compiler resolver, and the owner is the Limits I did not re-run the numeric probe (scores 2.91, 2.75 and 1.00). I did not open a dataset on S3. I did not run the prototype. I did not execute #951, #963, #996 or #1015. I read those drafts at |
| It ships with or directly after the identity RFC's `NodeRef`, so no phase | ||
| runs with type names in plans and type ids in rows. Phases 1 and 2 build | ||
| on it. Its serialized-plan and IR contract changes are decided in that | ||
| issue's RFC, not here. |
There was a problem hiding this comment.
[P1] Add the id on the typed IR and drop it in erase
Blocking. This sentence says the IR contract is decided in issue #1079. That issue says adding type_id beside type_name keeps the frozen engine compiling, because the engine matches IR nodes with ...
The frozen engine reads ir/untyped.rs. QueryIR::erase builds that IR from the typed IR in ir/mod.rs. tests/frozen.rs pins ir/untyped.rs and every file under crates/omnigraph-reference-engine/src/ by hash.
These matches list every field and omit ..:
collect_pipeline_columnsinquery.rs. Its comment says a new field must be decided there.- the execute match in
query.rs collect_node_bindingsinprojection.rs
Add the id on the typed IR only. erase drops it, as it already drops distinct. Leave ir/untyped.rs and the reference-engine sources unchanged. Correct the same sentence in #1079. #1079 is an issue, and it says an RFC is still likely needed.
| - the planner and engine key table lookups, plan pins and caches by id, | ||
| keeping the name for explain output and errors. | ||
|
|
||
| It ships with or directly after the identity RFC's `NodeRef`, so no phase |
There was a problem hiding this comment.
[P3] Put this item in the same release as NodeRef
Optional. "Directly after" allows a release that has NodeRef in rows and type names in plans. The next sentence forbids that release. Put this item in the same release as NodeRef, before phases 1 and 2. The other phase-0 bullets can still ship earlier. #1015 at 9c360b5a says namespaces do not depend on #1079. That part matches.
| 2026-10-18: phase 0 also resolves type references to `StableTypeId` below | ||
| the compiler (issue #1079), with or directly after the identity RFC's | ||
| `NodeRef`. The prototype's first draft chose between two identity rules per | ||
| site and got four of them wrong (P1, P2, P4, P6), so plans and rows should not |
There was a problem hiding this comment.
[P3] P1, P2, P4 and P6 belong to the tag-column draft
Optional. The finding table in this RFC says those four belong to the tag column, and the identity RFC removes them. They do not show that a plan name and a row id are the same defect. The reasons for one vocabulary are the name-to-id translation at each engine boundary, and derived state that stamps type names (graph_index/persist.rs TableStamp, the change-feed filter_digest). Cite those.
|
Recommendation: request changes before this revision is accepted. The three RFCs can stay drafts. This note is a comment. This account posts a comment and does not set a review state. Reviewed commit What this change does Phase 0 gains one item. Type references below the compiler become Findings from the review of
The earlier items that the Blocking
Optional
What holds in this delta These claims in #1079 match the code at this head. The same symbols on
Tradeoffs and liability One id below the compiler is the right source of truth once The codebase already has the compatibility door. The new text records a false compile rule. An implementer who follows it edits pinned files, or expects a compile that fails. That adds liability. Stating the This accepts a larger phase 0 so phases 1 and 2 do not run with two vocabularies. Name-keyed plans are correct inside one snapshot. They become a second vocabulary after No performance number is claimed in this delta. I make no cost claim. Validation Local, detached at
Pinned Lance is 11.0.0 ( CI at this head: CI Gate passed. Classify skipped the workspace tests, Clippy and rustfmt. GQT ordinary printed "No GQT input changed; skipping the GQ logic tests." PR Title passed. Green CI is not evidence for this design. Regression proof This delta fixes no runtime behavior. A GQT failure-before and pass-after does not apply. GQT cannot express a frozen-source hash or an IR field addition. That contract belongs to Limits I did not re-run the numeric probe. I did not open a dataset on S3. I did not run the prototype. I did not execute #951, #963, #996 or #1015. I read those drafts at the commits named above. Lance 13 was not executed in this pass. |
…very write is checked Name attempt validation and the commit revalidation as the point where an interface-wide mutation is checked against the actor's view, as Actor views checks every write. A check before staging could not see staged placement values.
|
Recommendation: request changes before this revision is accepted. The RFC stays a draft. Reviewed commit What this change does The Policy bullet moves the actor-view check for an interface-wide mutation. The check is now in attempt validation, over the staged rows, and again in the commit revalidation. The stated reason is that a check before staging cannot see staged placement values. A refusal leaves every staged arm detached. The 2026-10-20 decision-log entry records the same rule. Earlier findings The review of Blocking, still open:
Optional, still open:
Blocking
A view refusal inside
Actor views' Writes section names those validation functions, then says Name one row check: Optional The six items above stay open. This commit adds none. What holds The view check does not belong at typecheck time. Inherited Tradeoffs and liability One integrity evaluation, before any staged file, is the contract a mutation already has. Invariant 4 publishes once. Invariant 5 keeps a pre-publication effect unreachable, and a view refusal has no effect to reach. The view rule joins The new sentence puts the row check on the authority door and says a refusal leaves detached arms. An implementer who follows it writes files before the view answer, or threads the change set into This commit removes no liability until the sentence names No performance number is claimed. I make no cost claim. Validation Local, detached at
Code inspection on CI at this head: CI Gate passed. Classify skipped the workspace tests, Clippy and rustfmt. GQT ordinary printed "No GQT input changed; skipping the GQ logic tests." PR Title passed. Devin Review skipped this head. Green CI is not evidence for this design. Regression proof This commit fixes no runtime behavior. A GQT failure-before and pass-after does not apply. GQT cannot see the order of Limits I did not run a mutation. I did not execute the actor-views, schema-apply, or namespace drafts. I did not re-run the Lance probes from earlier reviews. Pinned Lance on this main is 13.0.0. This commit makes no new Lance claim. Inline finding on the Policy bullet ( [P1] Put the view check in Blocking. This sentence checks every arm again in the commit revalidation, and says a refusal leaves every staged arm detached. On
A view refusal in Actor views' Writes section names the validation functions, then says Name one row check: |
What & why
Adds a draft RFC for first-class polymorphism: interfaces and inline unions become abstract node types that queries can bind, traverse to and mutate through, and edge endpoints may be interfaces or unions. Today
interface/implementsparse and persist but nothing after the schema compiler reads them; typed edge alternation left heterogeneous endpoint unions out of scope.The proposal keeps one concrete type and one table per node. Abstract types are type sets resolved once per invocation and lowered to per-type pieces the engine already runs. Polymorphic edges gain
__src_type/__dst_typecolumns holding the endpoint'sStableTypeId, added by a staged LanceOperation::Merge(no data rewrite;add_columnsstays forbidden).Backing issue / RFC
Checklist
Local verification
python3 scripts/check-docs.py— Documentation OK (204 Markdown files checked)bash scripts/check-agents-md.sh— passedtypos docs/rfcs/2026-10-07-polymorphic-types.md— cleanNotes for reviewers
Operation::Mergeadds a nullable column without touching data files or indexes, that BTREE and Bitmap indexes serve aUInt64tag, and that BM25 scores differ across datasets for the same document.worksAt{1,2}) is accepted and silently runs one hop.