Repository navigation
Conversation
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. Every node keeps one concrete type and table; polymorphic edges gain StableTypeId endpoint tags added by a staged Lance Operation::Merge.
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.
A prototype on proto/polymorphic-types implemented interface bindings, polymorphic edge endpoints and endpoint generalization through every layer. Its findings change the design: - Tags are filled at generalization through a constant-column staged Merge instead of read as null; `generalized_from` serves only table images that predate the column. The full rewrite stays the proven fallback. - The engine refuses an untyped expansion over a polymorphic edge: such a plan returned wrong rows with no error. - The graph index build excludes polymorphic edges, whose ids collapse in a per-declared-type dense space. - Cycle closing compares (type, id); a named polymorphic edge gets its own budgeted policy and a refusal that names it. - Generalizing a keyed or @unique edge is refused in v1; @card on an interface source is refused until the validator groups by (type, id). - Tag columns sit after every property and are system fields. Adds a Prototype evidence section, tests to extend, two unresolved questions and a decision-log entry.
…ound trips Addresses review on the polymorphic types RFC. - Direction: a source fitting both ends of an edge whose endpoint sets are equal (a same-type edge, or Subject -> Subject) keeps the outgoing reading, as today; only unequal overlapping sets are ambiguous unless the other endpoint decides. The old rule refused the promised recursion. - The multi-hop check compares endpoint sets through one catalog predicate instead of binding names repeated in three places. - Keyed polymorphic edges round-trip through export and load, with a test for typed upsert, export, load and restart. - One catalog resolver maps stored tags to concrete types for every consumer. The prototype implements and tests the direction and multi-hop rules, including a three-hop recursion across three member types with colliding ids.
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.
# Conflicts: # docs/rfcs/README.md
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.
1 participant
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.