Skip to content

rfc(gq3): the language, its search contracts, one break and the programme - #1302

Open
ragnorc wants to merge 16 commits into
ModernRelay:mainfrom
ragnorc:rfc/gq-3
Open

ragnorc wants to merge 16 commits into
ModernRelay:mainfrom
ragnorc:rfc/gq-3

Conversation

@ragnorc

@ragnorc ragnorc commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

What this is

One RFC for GQ 3, replacing four open drafts that specified one surface from four documents: the GQ 3 grammar (#1202), the GQ 3 programme (#1213), analyzed lexical search (#792) and RFC 0048 search contracts (#793). Those four close with this PR; #1244 is answered in the Alternatives table and on its thread.

docs/rfcs/2026-11-07-gq-3.md states, in layers that do not move:

  • Axioms (eight): totality with a declared cost, one snapshot, the plan as the truth, closure, typed semantics, one spelling per term, reads free of effects, quality declared not chosen.
  • Core: twelve operators over typed relations, three terminal write sinks, Call as compile-time inlining; every surface form desugars to exactly one core term, with the closure table and the envelope (first-order with counting and ordering plus two declared-quality operators; below Datalog, above relational algebra).
  • Surface: let and from inside match; arithmetic; retrieval as relation functions l2, nearest(…, k:, recall:), bm25, with rrf(…, k:, weights:) and union; one declaration keyword query, the body's shape deciding read or write; a write body that takes a read's clauses before insert, update $x, delete $x.
  • Search contracts: @index(p, fulltext, analyzer = …, scorer = …) with three immutable profiles and bm25_v1 over the field corpus; terms(…) consumed by match_terms and bm25; Vector(n, distance = …); embedding recipes; the exact scan as oracle under every index state; the eleven laws and four populations of RFC 0048, with its reviewed default-window inconsistency resolved by construction (k is required).
  • The break: edition 3 under gq 3;, seven edition-2 spellings rewritten by queries migrate with a typed-IR proof (six) or rows-equality (rrf); a one-release edition-2 window rewritten at boot; queries fmt.
  • The programme: stage edges, slot owners, verdicts on every open language, search and access RFC, and the rules every later change answers.

Assets

  • assets/2026-10-28-gq-3-grammar-evaluation.md: the whole-language evaluation (outcome paragraph amended for closure).
  • assets/2026-11-07-gq-3-grammar-comparison.md: every GQ 3 production against ISO GQL, Cypher, SQL and Datalog.
  • assets/2026-11-07-gq-3-before-and-after.md: forty-two kinds of query in edition 2 and edition 3, "Not supported" where edition 2 cannot express one.

Checks

typos, python3 scripts/check-docs.py (473 files OK), git diff --check.

…ramme

One RFC replaces four open drafts: the GQ 3 grammar (PR 1202), the GQ 3
programme (PR 1213), analyzed lexical search (PR 792) and RFC 0048 (PR 793).
It states eight axioms, a twelve-operator core with three write sinks, the
surface (let, from, arithmetic, relation functions, one query keyword),
the search contracts restated on that surface, the seven-row break with its
rewriter, and the stage programme with verdicts on every open language,
search and access RFC. Three evidence assets accompany it: the whole-language
evaluation, a construct-by-construct grammar comparison against GQL, Cypher,
SQL and Datalog, and an edition 2 against edition 3 corpus by kind of query.
@ragnorc ragnorc changed the title rfc: GQ 3, the language, its search contracts, one break and the programme rfc(gq3): the language, its search contracts, one break and the programme Nov 8, 2026

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of d626e288 (RFC: GQ 3)

Why. GQ grew one feature at a time. The four drafts that tried to fix this (#1202, #1213, #792, #793) drifted from each other. This RFC puts them in one document with four layers: axioms, a core algebra, the surface syntax, and the search contracts. Then it plans one breaking release (edition 3) and a sequence of additive stages.

What I checked. I read the whole RFC and spot-checked the two assets. I compared the claims about today's behaviour with the code at the PR base ea4a16d5 (the RFC cites 03e0b623, which is on main) and with crates.io Lance 13.0.0.

Outcome: revise before acceptance. Consolidating the drafts removes a real liability: there are fewer documents to drift and one decision log. The search section is accurate about today's behaviour. But the write half of the design changes two engine rules that the RFC says do not change. Also, three acceptance gates in blocked_on cannot pass as the text is written now. The four blocking items below are about the text. None of them needs a new mechanism.

Blocking

B1. The write examples break the D₂ rule, which the RFC says is "unchanged". close_company (line 515) and retire (line 1189) each delete $k and update $p in one body. Today the engine refuses this before any I/O (enforce_no_mixed_destructive_constructive, crates/omnigraph/src/exec/mutation.rs:757-795). This is not only a parser rule. Other code depends on it: the delete-count deduplication (mutation.rs:726-731, "If D₂ is ever relaxed, this dedup must be revisited") and the commit-path change discovery (changes/candidate_scan.rs:23-26, "one engine-authored transaction is either a row-set-preserving insert/update or a delete, never both"). Invariant 4 also names the split. Choose one of two options:

  • Keep D₂. Make the examples legal by splitting them, and state that a mixed body is a typed refusal.
  • Lift D₂. Then say so in Invariants and Compatibility, name the invariant-4 change, and add the deduplication and change-discovery proofs to Evidence.

The examples are the RFC's main argument for writes fed by reads, so this choice changes the design.

B2. One shared clause relation per write body changes the meaning of multi-statement mutations, so the rewriter's IR-equality gate cannot pass. The grammar allows clauses only before all statements (mutation_body = (!stmt_keyword ~ clause)* ~ mutation_stmt+). Each statement then "runs once per row of the clauses' relation". Edition 2 bodies often have several statements, and each one has its own where. Here are three ways the rewrite goes wrong:

  • update Person … where name = "alice" and update Marker … where name = "initial" (cases/detached_multitable_update_failure_after_table_2.gqt, w(): four updates on four types). The rewrite gives one product of four bindings. If any one predicate matches nothing, the product is empty and no update runs.
  • A clause-less insert beside a rewritten update … where. The insert now runs once per clause row, not once.
  • Edition 2 has read-your-writes inside a mutation: an update … where scans the earlier statements' pending rows (scan_with_pending, mutation.rs:1809-1825). The RFC says "a later statement cannot read an earlier statement's effect". The two IRs can be equal and still run differently.

A simple scan found 17 declarations in crates/omnigraph-gqt/cases/ with two or more statements, at least one of them update/delete … where. These declarations cannot meet "zero hand edits … typed-IR equality". To fix this, give each statement its own scope (for example, a statement-local clause block, or one clause relation per statement). Or make the rewriter refuse these bodies and remove them from the gate. In both cases, state the read-your-writes decision explicitly.

B3. The rules for ranking populations contradict each other, and the rewriter writes text that the type checker refuses. §5 says a ranking's population "must carry exactly one node column". But:

  • The §6 example ranks { … return { $r, $p } }, which has two node columns.
  • The rewriter's nearest, bm25 and rrf rows write a population that "returns every variable the pattern binds". Every ranked edition-2 query with a traversal therefore gets two or more node columns. The user guide documents this shape as supported ("Ranking a traversal-reached binding").
  • Fuse returns R[$x, s: F64] (§2), so the other bindings are gone after from rrf(…). Law 4 and §4 say "fan-out rows of one target carry its score", and today's fused order keeps every fan-out row (docs/user/search/index.md, "an entity that fans out to several rows contributes each of them").
  • ApproxTopK says "at most k rows", but law 4 implies at most k targets, plus their fan-out rows.

One rule fixes all four: a population may carry any columns. The ranked column is named by the field argument (for example bm25(rel, $p.text, q) instead of a bare text). The output is every population row of the selected targets, so k bounds targets and not rows. Fuse keeps the arms' columns, or the rewriter joins them back.

B4. Axiom A6 is stated as absolute, but the RFC keeps several double spellings, so the "desugaring-uniqueness test" in blocked_on fails by construction. A6 says that two surface forms with one core term "are a violation". The RFC keeps all of these pairs:

  • exists { p } and count { p } > 0
  • not { p } and count { p } = 0
  • the property braces and the equivalent filter (§6 says they "are one Filter")
  • both triple orders
  • { tags: "rust" } and "rust" in $d.tags (both lower to CompOp::Contains: ir/lower.rs:898-905, ir/coerce.rs:371-387)
  • search, match_text, fuzzy and match_terms(…, terms(…))

fmt and lint handle all of these. That is a good policy, but it is not the policy that A6 states. Restate A6 as "one canonical spelling per core term; every other accepted spelling is listed in an alias table that fmt rewrites and lint names". Then make the test check that table. After that change, the Alternatives rows that refuse a form only because of A6 (a scalar l2, @approximate, where on statements) need a second reason, or a note that they could become alias rows.

Should fix

  • The rrf rewrite has no proof for stored user text. The rewriter "refuses to write when the edited text does not compile to a typed IR equal to the original's", but the rrf row is not IR-equal by design. Its rows also differ when a traversal removes some of an arm's window. Today a nearest arm in rrf is a top-k window taken before a postfilter that the rrf_plan gate chooses (docs/user/search/index.md:40-42, 86-88). The rewrite ranks inside the population. That change is a fix, but "proved rows-equal on the corpus" proves nothing about a user's data. Classify the row as a change in meaning: migrate reports it and does not apply it silently, and the boot window treats it the same way.
  • The edition window covers only sources held in the registry. A file without a gq line is read as edition 3 (line 927), so -e, --query files, lint and a cluster apply of an unmarked source directory refuse edition-2 constructs at once. This contradicts "edition 2 compiles for one release". Define how the window classifies unmarked text. Also name the format owner of the edition that the registry newly records for each source.
  • Unsigned integers and integer-to-float widening. The value table (line 197) has no U32 or U64, but both are in the grammars (query.pest:144, schema.pest:47). The arithmetic rules do not define U64 ± I64. "An integer with a float is F64" silently loses precision above 2^53. Invariant 9 asks for an explicit Cast in that case. Add the unsigned types and make the widening a visible Cast, or refuse it.
  • Fan-out in writes. An Update whose relation has two rows for one entity, with different assigned values (update $p set { city: $c.city } with two $c), is not defined. An unkeyed edge insert creates one edge per fan-out row. Make conflicting assignments a typed error (invariant 8) and state the cardinality of an insert.
  • Diagnostic codes. T17 currently means "nearest ordering requires a limit", and the RFC reuses it for "nearest without k". codes.rs says a published meaning is frozen, so allocate a new code. §5 lets update take "a bound node or edge variable", but T16 refuses an update of an edge today. Say that T16 stays.
  • Grammar consistency.
    • The weighted example in the before-and-after asset writes { from l2(…) return { … } }, with no match. The §7 grammar refuses that (inline_read = "{" ~ read_body ~ "}", and read_body starts with match_clause).
    • §3 says a write body has "zero or more match clauses", but the grammar and every example use bare clauses. Choose one form and pin it in a parser test.
  • Line 760. order { $distance, $x.id }: $x.id is a user property, and the identity is $x.@id. Order already breaks ties by identity, so the second key can be removed.

Optional

  • k: means the window size on nearest and the RRF constant on rrf, often in one expression (rrf(nearest(…, k: n), …, k: 60)). A different option name for the constant would remove a likely agent error.
  • english_v1 copies Lance's filter order: stemming comes before stop-word removal (lance-index 13.0.0 tokenizer.rs:1082-1095). A profile is immutable, so state that this order is a deliberate choice.
  • The registry row for 2026-11-07 is above the 2026-10-29 row. docs/rfcs/README.md says rows are "by date".

Verified as correct

  • GQ_LANGUAGE_VERSION = (2, 3) (omnigraph-compiler/src/lib.rs:48) and EXPLAIN_VERSION = 8 (omnigraph-planner/src/explain.rs:34).
  • The Lance 13 default analysis is simple, a 40-byte token filter, lowercase, stem, stop words and ASCII folding (lance-index 13.0.0 tokenizer.rs:634-657), so english_v1 differs from it only by the token filter and NFC, as the RFC says.
  • search and match_text build FullTextSearchQuery::new, which is Operator::Or (query.rs:391-398), and fuzzy with no budget is 2 edits (engine/scan.rs:961-965).
  • The list-contains to in rewrite is IR-equal.
  • #1202, #1213, #792 and #793 are closed and not merged.

Liability

The core adds Correlate with a choice for the planner, a plan budget, and cardinality estimates across a correlation. These are real long-term costs, and the RFC names them. The grammar-pin rule ("no new rule without an amendment") is a good control for what the language looks like after five more changes. The large unnamed liability is in B1 and B2: two rules of write execution change, but the RFC calls them unchanged. The engine comments say that this is exactly the complexity D₂ prevents.

Validation

  • Local runs in a detached worktree at d626e288:
    • python3 scripts/check-docs.py with the pinned docs requirements: "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK.
    • git diff --check origin/main...HEAD: clean.
    • typos was not run, because the binary is not on this host.
  • CI at d626e288 (04:31Z): 25 pass, including PR Title, GQT (dst), the DST pinned suite, RustFS S3 and Azurite; 4 skipping; 2 pending (Test Workspace, Storage Upgrade Compatibility).
  • Regression proof does not apply: the PR changes only RFC text and contains no code or query change.
  • The corpus count in B2 comes from a source scan of .gqt files, not from running a rewriter.
  • I did not build Lance and did not run any search instrument.

Comment thread docs/rfcs/2026-11-07-gq-3.md Outdated
plan. No semantics pass through strings, settings, transport flags or side
tables. Every refusal is a typed code with the fix that makes the text legal.

**A6. One spelling per term.** Two surface forms that desugar to the same

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

B4: as stated, A6 is violated by forms this RFC keeps (exists/count > 0, not/count = 0, braces and the filter, both triple orders, list braces and in, the three lexical rows and match_terms). Restate it as one canonical spelling plus an alias table that fmt rewrites, and make the uniqueness test check that table.

Comment thread docs/rfcs/2026-11-07-gq-3.md Outdated
endpoint assignment being a key literal or a bound reference. There is no
`where` on a statement: the clauses are the selection, and a second
predicate on the sink would be a second spelling of a filter (A6). Each
statement runs once per row of the clauses' relation; its assignment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

B2: one clause relation shared by all statements makes an edition-2 multi-statement body a product. If one statement's where matches nothing, the other statements do not run, and a clause-less insert runs once per row. It also removes today's in-mutation read-your-writes (scan_with_pending). There are 17 such declarations in cases/, for example detached_multitable_update_failure_after_table_2.gqt w().

query close_company($slug: String) {
$c: Company { slug: $slug }
$p $k:worksAt $c
delete $k

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

B1: delete $k with update $p in one body is refused today by D₂ (exec/mutation.rs:757-795), which the RFC says is unchanged. The delete deduplication (mutation.rs:726-731) and commit-path change discovery (changes/candidate_scan.rs:23-26) depend on D₂. Keep D₂ and split the example, or lift D₂ explicitly with those proofs.

match {
from bm25(
{ match { $r: SourceRevision { matter_number: $matter } $r hasPassage $p contains($p.text, "tariff") }
return { $r, $p } },

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

B3: this population has two node columns ($r, $p), and §5 refuses that ("exactly one node column"). The rewriter rows write every bound variable, so all traversal-shaped ranked reads hit this. Also Fuse returns only [$x, s], which drops $r after rrf. Suggest: any columns, the target named by the field ($p.text), and k bounding targets, not rows.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of 73f6ee9c (delta from d626e288)

This review covers only the new commit. The review of d626e288 still applies. This commit does not change B1 to B4 or the should-fix items, so they stay open.

Why. The previous revision had a batch declaration: several reads under one snapshot, with @brief and @phase. This commit removes it. The argument is that one snapshot and one round trip are properties of a request, not of the language.

What. batch_decl leaves the grammar and the T6x list. @brief and @phase leave the annotation slots and become request fields. @budget stays a per-declaration annotation. Stage N now describes the feature as a request shape. The asset example becomes query bounded() @budget(...), and a new Alternatives row and a decision-log entry give the reason.

Outcome. The direction is correct. The new text is accurate, except for one claim about what exists today. The overall recommendation is unchanged: revise before acceptance, because of B1 to B4.

Assessment

The change removes a primitive. It removes a declaration kind, two reserved annotations, one freeze-list slot and one grammar rule. batch { q1 q2 } and a file with two declarations would have compiled to the same thing, so the RFC's own axiom A6 argues against keeping both. @brief and @phase change no plan, so they belong to the request. Five more changes of the same kind would add request fields, not grammar, which costs less to keep stable.

The new text has no stale batch, @brief or @phase references. No other open RFC (#1244, #1197, #1149) depends on batch.

Should fix

S1. "A request shape the inline door already has" is too strong (line 1236 and the decision log at line 1593). Today the inline door compiles a file of several declarations, but it runs one of them:

  • QueryRequest.name selects one declaration "when query declares multiple" (crates/omnigraph-api-types/src/lib.rs:1435-1458). There is one params object, and the struct is deny_unknown_fields.
  • ReadOutput is one result with one query_name (lib.rs:651). The Arrow IPC response carries that name in a single Omnigraph-Query-Name header (lib.rs:120), so one stream holds one result.
  • A stored query is invoked by one name in the path, /graphs/{graph_id}/queries/{query_name}. "A request listing names" has no route today.

So "several declarations in one request, one result each" is new HTTP contract work. It is a smaller cost than a language construct, but stage N must decide these points:

  • how params map to each declaration (one shared namespace, or one object per declaration);
  • the response envelope, and what happens to the Arrow IPC media type;
  • whether one failing declaration fails the whole request or gives a per-result error;
  • the operation for a list of stored names, and its invoke_query gate for each name;
  • the inventory, OpenAPI and convention-test rows (docs/dev/http-api.md).

Suggested text: the request already pins one snapshot and already compiles a file of declarations, and stage N adds running all of them with one result each, additively under the HTTP conventions. W1 already uses this wording for its transport work.

S2. The freeze list does not say that the request shape is not available yet (line 975). The "Declaration kinds" row puts "a request may carry several declarations … one result each" in the Edition 3 column. The "In this release" cell does not mention it, and the Owner cell names only the one-file loader. A reader cannot tell that this is stage N work. Add "running several declarations in one request is stage N" to the release cell, and add N to the owners.

Optional

  • W2 still depends on "W1, N" (line 1365). The N dependency came from "the batch's write annotations", which this commit removes. Remove N, or say what W2 still needs from N.
  • The evidence asset 2026-10-28-gq-3-grammar-evaluation.md still recommends batch (lines 2845, 2939-2967, 3033, 3646, 3654). Its outcome paragraph already says the RFC wins where they differ. But that paragraph lists the positions brought into line, and batch is not in the list. Add it there, so a reader of line 3033 ("the three closure forms plus batch cover the measured demand") knows the demand is now met by a request shape.

Validation

  • Local runs in a clean detached worktree at 73f6ee9c:
    • python3 scripts/check-docs.py with the pinned docs requirements (Python 3.12): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK.
    • git diff --check origin/main...HEAD: clean.
    • typos was not run, because the binary is not on this host.
  • CI at 73f6ee9c: 28 pass, 4 skipping, none failing or pending.
  • The S1 statements come from reading the code at this head. I did not send requests to a server.
  • A regression test does not apply: the commit changes only RFC text.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of 05d913e4 (delta from 73f6ee9c)

This review covers only the new commit. The reviews of d626e288 and 73f6ee9c still apply. This commit does not touch B1 to B4, and it does not address S1 or S2 of the last review, so they stay open.

Why. The previous revision reserved every future form as a "slot": it parsed, it was refused with a T6x code naming its owner, and a doc check kept a slot-owner table in sync. This commit removes that machinery. Its argument: a form that a later stage adds starts at a token that edition 3 refuses, so adding it later is additive and needs no reservation.

What. The T6x list, the slot-owner table, the slot-owner check, the graph_commits reservation, the federation and TC stage rows, the sequence picture, the rollout restatement, the coverage request setting and several verdict rows go. The freeze list gets a "Later" column. Unknown annotations become metadata, as in .pg. Only @if_commit and @valid are refused. The Lance BM25 evidence moves from section 4 to section 10, and the orthogonality and precedent tables move to the assets. Net: −74 lines.

Outcome. The direction is correct, with one exception. Removing the slot machinery removes real liability: a refusal path per future form, an owner table that can drift, and a doc-check rule. The argument "a refused token is free" holds for grammar productions. It does not hold for annotations, because this same commit makes every unknown annotation accepted. The overall recommendation is unchanged: revise before acceptance.

Blocking

B5. "Every later form is additive" is false for annotations, and the asset's own @budget example shows the break (line 882, line 943, and asset line 637).

  • Today .gq refuses an unknown query annotation. query_annotation admits only @description and @instruction (crates/omnigraph-compiler/src/query/query.pest:118). I checked this: query q() @foo("x") { … } fails omnigraph lint, and the same @foo("x") on a .pg node passes. So the line 882 rule ("retained as metadata, as .pg does today") is a change for queries, not a carry-over.
  • With that rule, query bounded() @budget(rows = 1000, on_exceed = refuse) (asset line 654) compiles at B and runs without a bound. When stage N types @budget, the same stored text starts to refuse, or with on_exceed = partial to return fewer rows. That is a change in meaning for accepted text, not an additive change. Rule 5, "Cases first", does not catch it: no refusal exists at B, so N has no expect error case to flip.
  • The reason the RFC gives for refusing @if_commit and @valid ("retaining them would mislead a writer") applies the same way to @budget and @example. It also applies to a typo such as @descripton. Today that typo is an error. Under line 882 it is silently kept.

Choose one of two options:

  • Keep today's .gq rule. An unknown query annotation is a type error. Then every annotation a later stage types starts from a refusal, and the additivity argument holds without exceptions. Metadata for tools can use a single typed annotation (for example @meta(key = …)), which A6 would accept.
  • Keep open annotations. Then refuse by name every annotation the RFC already plans (@budget, @example, @if_commit, @valid), give each one an expect error case at B, and state that a stage can type only a name that B refuses.

Should fix

  • S3. coverage = exact stands (line 1360) points to a setting this commit removes. The RFC 0048 verdict row still says it stands, but the coverage request setting paragraph is gone, and the decision log lists it as removed. Remove it from the verdict row, or say that it moves to a later stage.
  • S4. The asset now disagrees with the RFC (asset lines 637-639).
    • "Each is a parse error today": @budget(…) is accepted as metadata under line 882 (see B5). $t: @Type is a type error per the freeze-list row "an unknown @ name is a type error", not a parse error. Only crm::Person is a parse error.
    • "@if_commit alone is refused by name": line 882 and the Annotations row refuse both @if_commit and @valid.
  • S1 and S2 from the last review are still open. The decision log (line 1515) still says the multi-declaration request is a shape "the inline door already has". The Declaration kinds row (line 949) now has a "Later" column, but it names only the one-file loader. Running several declarations in one request is stage N work, and the row should say so.

Optional

  • The BM25 guards are not in the repository (line 1310). native_fuzzy_bm25_rewards_a_rare_expansion and native_bm25_idf_can_round_a_common_term_to_zero are not on main or at this head (git grep finds only this RFC). They are at 1e0bed40, the head of #751, which was closed so that each guard "returns in the implementation PR that first depends on it". Section 10 is headed "Existing owners", so the bullet reads as present evidence. Say that the guards are at 1e0bed40, and name the stage that restores them (B's exact lexical scan, or L).
  • The refusal of @if_commit and @valid has no code. The removed T6x bullet gave reserved constructs a code. The new bullet (line 882) gives none. codes.rs freezes a published meaning, so name the code in the error list.
  • @valid has two descriptions. The Schema roles row (line 965) says @valid is "accepted and refused at use" in .pg. The Annotations row (line 961) says it is "refused". State that the refusal at line 882 is for query declarations, and that the schema role behaves as row 965 says.
  • W2 still depends on "W1, N" (line 1343). The last review raised this.

Liability

The commit removes more than it adds. The slot table and its doc check were a second source of truth beside the freeze list, and that kind of state drifts. The replacement rule ("a refused token is free") needs no maintenance, but only if edition 3 refuses everything that a later stage will define. B5 is the one place where the commit adds liability: every unknown annotation becomes accepted surface. That surface either constrains each later stage or changes meaning when the stage lands. Five more annotation-typed stages would each meet this problem.

Validation

  • Local runs in a clean detached worktree at 05d913e4:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check origin/main...HEAD: clean.
    • typos was not run, because the binary is not on this host.
  • Annotation behaviour: omnigraph lint (CLI 0.14.1-edge+269299b) on a one-node schema with @foo("x") and @budget(5) passes. A query with @foo("x") fails. The grammar at this head (query.pest:118) matches this result.
  • git grep for the two guard names at this head and on main finds only the RFC. gh pr view 751 shows the PR closed and not merged.
  • CI at 05d913e4: 28 pass, 4 skipping, none failing or pending.
  • A regression test does not apply: the commit changes only RFC text.

Comment thread docs/rfcs/2026-11-07-gq-3.md Outdated
order to schema order, an equality between a bound variable's property
and a value written as a filter into that binding's braces, a `search` or
`match_text` or `fuzzy` call to `match_terms` once that lands).
- An unknown annotation is retained as metadata, as `.pg` does today; a

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

B5: today .gq refuses an unknown query annotation (query.pest:118 admits only @description and @instruction). With this rule, @budget(rows = 1000, on_exceed = refuse) compiles at B and runs without a bound, and then starts to refuse when stage N types it. That is a change in meaning, not an additive change. Keep unknown query annotations a type error, or refuse every planned name (@budget, @example, @if_commit, @valid) at B.

```

**Added by later stages.** Edition 2: **Not supported** and not parseable.
Each is a parse error today and additive when its stage lands; `@if_commit`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S4: under RFC line 882, @budget(…) below is kept as metadata, not a parse error. $t: @Type is a type error per the freeze list. Also, the RFC refuses both @if_commit and @valid, not @if_commit alone.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of ab74f629 (delta from 05d913e4)

This review covers only the new commit. The reviews of d626e288, 73f6ee9c and 05d913e4 still apply. This commit does not touch B1 to B5 or S1 to S4, so they stay open.

Why. The previous text refused recursion (A1) and named an open upper bound on Expand as the one later change for unbounded reachability. After a comparison with CozoDB, this commit admits recursion.

What. Fix becomes the thirteenth core operator: the least fixpoint of a declared read that calls itself. The read may not negate or count its own rows. It may aggregate them only with min or max. A computed column needs min or max and @budget(rounds = n). A1 is restated. A call cycle is no longer always T70: a monotone cycle compiles to Fix. A new stage, Rec, ships the evaluator. Until then a self-call is T70. Graph algorithms join stage R as relation functions. Two examples (reachable, distance), test rows, three Alternatives rows and open question 11 are added. Net: +95 lines (136 added, 41 removed).

Outcome. The direction is sound. One operator replaces a special case, and the finite active domain bounds it. But the admission rule is not correct yet. It is a list of refused spellings (not, count, other aggregates), not a property of the operators. So it accepts some bodies whose fixpoint is not defined, and it refuses one monotone form. The overall recommendation stays the same: revise before acceptance.

Blocking

B6. The monotonicity rule accepts non-monotone bodies and refuses a monotone one (lines 231, 496-500, 583, 884).

  • Cut (order … limit), ApproxTopK (nearest(…, k:)) and Fuse (rrf) are not not, count or an aggregate, so the rule accepts them over the recursive relation. None of them is monotone: more input rows can remove an output row.
  • Example. Take reachable (line 505) and add order { $r.handle } limit 1 to the step arm. Use the graph a→b, a→c, b→g, c→e, e→f.
    • Naive evaluation (the step reads the whole relation each round) gives {b, c, e}.
    • Semi-naive evaluation (the cost shape at line 231: the step reads only the rows the last round added) gives {b, c, e, f}.
    • Line 231 describes both, but they give different answers. So "the least fixpoint" has no single meaning for this body.
  • In the other direction, exists { p } desugars to count { p } > 0 (line 298). If the rule applies to the core term, "may not count its own rows" refuses exists { from reachable($from) … }. That form is monotone.
  • With min or max, the rule does not say what the body may do with the aggregated column. Cozo's aggregation chapter says that recursive semi-lattice aggregation has "soundness constraints on what can be done on the bindings coming from the auto-recursive parts", and that Cozo cannot check them. The RFC needs to state these constraints or refuse the uses it does not check.

Fix: define monotonicity once, on the core operators. Add one column to the operator table (line 217) that says which inputs each operator is monotone in:

  • Monotone: Scan, Filter over reference or stored columns, Project, Union, Expand, Correlate with a monotone inner term, and Group with no aggregates.
  • Not monotone: Cut, ApproxTopK, Fuse, Filter(¬ …), and Group with count, sum or avg.

A cycle compiles to Fix only when every operator between the recursive reference and the head is monotone in that input. Otherwise T70 names the operator. The type checker then owns one rule. Each later operator (rerank, norm and the stage R algorithms) fills in one cell and does not need its own exception. Add T70 cases for limit, nearest and rrf in a step arm, and an accepted exists case, to the list at line 1293.

Should fix

  • S5. Fix is not specified as a set operator, but Project and Union keep duplicates (lines 222, 225, 231). "Until none is new" has a meaning only for sets. If reachable returns { $q } and not distinct { $q }, two readings are possible. In the first, each round adds the same rows again, and the read does not end on a cycle. In the second, the head is silently deduplicated, which contradicts the Project row. State that the recursive relation is a set: either Fix removes duplicates, or the head must be distinct or a keyed min/max group, and T70 otherwise. The pinned DataFusion 54.0.0 RecursiveQueryExec has exactly this switch (is_distinct). Its doc comment says that it applies no check for infinite recursion and leaves that to the planner (recursive_query.rs lines 48-62).

  • S6. The RFC does not say which arguments a recursive call may pass (lines 256-262, 482, 496-502). Call is inlined at compile time with its scalar arguments substituted. Both examples pass $from unchanged. But a relation expression may read any variable in scope (line 482), so a step can write from reachable($r.handle). That is one relation for each argument value, not one f: R → R, and compile-time inlining does not end. Do one of the two:

    • Require each call on a cycle to pass the cycle's parameters unchanged, and refuse other calls with T70.
    • Define the parameterized fixpoint: the arguments become columns, which are finite because they come from stored values. Then give its bound.
  • S7. reachable is not the unbounded case of Expand (lines 525, 1276, 1323). Expand uses the shortest distance from the start node, so the start node comes back only through its own self-loop, never through a cycle (docs/user/queries/traversal.md lines 39-42). The Fix step from reachable($from) $q knows $r is a plain triple. I tested this with the installed CLI on a→b, b→a, b→c:

    • $p knows{1,5} $f from a returns {b, c}.
    • The two-triple chain $p knows $x $x knows $f returns {a, c}.

    So reachable("a") returns {a, b, c}, and knows{1,n} returns {b, c}. The test at line 1323 compares the two on a cyclic graph and will fail. Line 1276 calls one a special case of the other. Say where they differ (the start node), and make the test compare them with that difference. Another option is to exclude the start node in the example.

  • S8. Two statements are now stale or not correct.

    • The Expand row (line 219) still says "an open upper bound is the one later change this operator admits". Line 1276 and the decision log say that Fix replaces it.
    • The envelope (lines 369-374) says that the degree of the polynomial is "the nesting depth of Correlate, Expand and Fix". A Fix bound depends on the width of the recursive relation, not on nesting. A two-column transitive closure (all pairs) at nesting depth 1 has up to |Person|² rows. In the Fix row (line 231), "≤ |domain| of its reference columns" must be the product of the column domains (with min or max, the number of keys). "Or the budget" bounds rounds, not rows. Suggested text: rows ≤ the product of the key-column domains; rounds ≤ rows, or the budget.
  • S9. The freeze list has no row for @budget(rounds = n), and two places decide its stage (lines 1011, 1411, 1546). The Annotations row (line 1011) lists only @budget(rows, ms, on_exceed). The Rec row says "N for @budget". Open question 11 asks whether Rec or N types it. Let one place decide. This also adds to B5: at B, line 932 keeps @budget(rounds = 50) as metadata on any read. When a later stage types rounds, the same accepted text changes meaning.

Optional

  • "The rules are Datalog's" (line 496). Stratified Datalog does not allow aggregation through recursion. min and max in recursion is the semi-lattice extension (Cozo's term). Say so, so that a reader does not look for it in Datalog.
  • Line 356 says that Cozo "warns that it can be expensive". Cozo's queries chapter says that such queries "may produce infinite relations" and that it cannot reject all of them. Line 1275 already says this correctly. Use the same words.
  • Cost shape at line 231. With min or max, the delta must hold the rows whose aggregate improved, not only the rows that were added.
  • The #1244 verdict (lines 1240 and 1435) says that #1244 converges with the core. #1244 says "Direct and indirect recursion refuse during compilation". After this commit, recursion is a new difference. Add it to the Alternatives row.
  • The PR description still says "twelve operators".

Liability

The commit adds one operator, one evaluator stage, a monotonicity check and a round budget. It removes the planned open-bound Expand, so shortest distance and reachability get one owner. That is fewer primitives, and the choice is correct. The liability is in the admission rule. With a list of refused spellings, every new operator brings a new question: is it safe inside a recursion? Stage R alone adds rerank, norm and three graph algorithms, so five more operators give five more exceptions to remember. A monotonicity column on the operator table makes each operator answer that question once, in the one table that already owns signatures and bounds. If stage Rec builds on DataFusion, RecursiveQueryExec with is_distinct already gives set semi-naive evaluation for recursion over references. It has no lattice aggregation, so distance needs its own owner. Name that in the Rec row, so that the design keeps one evaluator and does not get two.

Validation

  • Local runs in a clean detached worktree at ab74f629:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check 05d913e4 ab74f629: clean.
    • typos was not run, because the binary is not on this host.
  • Expand start-node behaviour: omnigraph CLI 0.14.1-edge+269299b, local graph node Person { handle: String @key }, edge Knows: Person -> Person, edges a→b, b→a, b→c. knows{1,5} from a: {b, c}. knows{2,2}: {c}. Two triples: {a, c}.
  • B6 example: no Fix evaluator exists yet (stage Rec). I computed the trace with a short script that runs the step arm both ways (whole relation each round, and last-round rows only) over the graph above. This is a hand model of line 231, not engine output.
  • Read: Cozo's queries chapter (Recursion, Aggregation) and aggregations chapter (semi-lattice soundness); DataFusion datafusion-physical-plan 54.0.0 recursive_query.rs (the version in Cargo.lock); the recursion section of #1244.
  • CI at ab74f629: 28 pass, 4 skipping, none failing or pending.
  • A regression test does not apply: the commit changes only RFC text.

Comment thread docs/rfcs/2026-11-07-gq-3.md Outdated
| **Score**(R, $x, kind, field, q) | `R` → `R + [s: F32]` | an exact score per row: `l2` is the squared L2 distance of `field` to `q`; `bm25` is the BM25 of `field` against the indexed table's statistics, the population only filters; a null field value drops the row *[as today]*; a non-finite input is a typed error | `≤ \|R\|` | reads every row's field |
| **ApproxTopK**(R, $x, kind, field, q, k, recall) | `R` → `R + [s: F32]`, at most `k` rows | the best `k` of the population under `kind`, approximate with declared `recall`, reported exhaustive when the engine searched everything (A8); the overfetch ladder and probe policy are its plan policy *[as today]* | `≤ k` | index probes, widened by the ladder |
| **Fuse**(R1…Rn, method, params) | ordered scored relations sharing one node column → `R[$x, s: F64]` | `rrf`: `Σ 1 / (k + rank_i)` over the arms in which `$x` appears; a missing arm contributes nothing, not zero; joined by node identity | `≤ Σ \|Ri\|` | linear in the arms |
| **Fix**(f) | `f: R → R` monotone → `R` | the least fixpoint of a declared read over itself: rows added round by round until none is new; `f` may not negate or count its own rows, and may aggregate them only with `min` and `max`; a column it computes rather than reads is admitted only under one of those and a declared round budget | `≤ \|domain\|` of its reference columns, or the budget | semi-naive: each round joins only the rows the last round added |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

B6. This rule names refused spellings (not, count, other aggregates), not monotone operators. It accepts Cut, ApproxTopK and Fuse over the recursive relation. None of them is monotone. With order { $r.handle } limit 1 on reachable's step arm over a→b, a→c, b→g, c→e, e→f, naive evaluation gives {b, c, e} and the semi-naive cost shape in this row gives {b, c, e, f}. It also refuses exists, which desugars to count { p } > 0 (line 298) and is monotone. Suggest a per-operator monotonicity column in this table, so that T70 names the first non-monotone operator on the cycle.

Comment thread docs/rfcs/2026-11-07-gq-3.md Outdated

| Operator | Signature | Semantics | Cardinality | Cost shape |
|---|---|---|---|---|
| **Scan**(T) | → `R[$x: N<T>]` | every node of type T in the snapshot | `\|T\|` | linear in `\|T\|`, or index-bounded by a pushed Filter |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S8. This row still says "an open upper bound is the one later change this operator admits". Line 1276 and the decision log now replace the open bound with Fix. Also, reachable and knows{1,n} differ at the start node on a cycle (see S7), so the replacement is not exact.

…ix over sets, bound Call, named ranked columns, write groups, the certified-margin fetch, the parser and red-team decisions

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of d79b6ea7 (delta from ab74f629)

This review covers only the two new commits (f7a5ca07, d79b6ea7). The earlier reviews of d626e288, 73f6ee9c, 05d913e4 and ab74f629 still apply where the status list below says "open".

Why. Five spikes (parser, rewriter, a reference interpreter of the core, Lance lexical guards, a red-team corpus) tested the RFC. This commit folds their findings back into the text.

What. The core gets a total tie rule for Order, an edge selector on Expand, an edge Scan ($k: E), the not-distinct equality, Fix over sets with a fuller monotonicity rule, and Call as binding with pass-through recursion. l2 becomes distance, which reads the field's declared distance. A ranking names its column ($d.embedding). rrf returns only the target and score. A write becomes a sequence of groups, and a later group sees the earlier groups' effects. Unknown annotations are refused except four metadata names. The WAND fast path becomes a certified candidate generator with an F64 rescore and a margin ε. The grammar gets the full edition-3 rule set and the prototype's decisions. Net: +302 lines in the RFC (628 added, 326 removed).

Outcome. Most of the earlier blocking findings are now fixed, and the spike evidence makes the text much more concrete. Two new statements are not correct, and the engine shows it: the write-conflict rule contradicts the group rule, and limit with no order is marked [as today] but is not what the engine does today. The recommendation stays: revise before acceptance.

Blocking

B7. The new conflict rule refuses the sequential writes that the group rule keeps (lines 267-270, 610-612, 1177).

  • Line 267: "Two different values for one entity and property … or two different upserts of one key within one write are a typed refusal."
  • Line 610: "a later group sees the effects of the groups before it [as today], so a body that inserts a document and then updates it by key still works". That body writes two different values for one entity and property in one write, so line 267 refuses it.
  • The rewriter writes "one group per statement, in order" (line 1177). So every edition-2 body that writes one property twice becomes a refusal after the rewrite. The Compatibility section (line 1749) does not list this break, and the "rewritten corpus green" gate in blocked_on would fail on any such case.
  • Today all three of these bodies succeed with one commit, and the last write wins (installed CLI, local graph):
    • insert Person { handle: "d", name: "first" } update Person set { name: "second" } where handle = "d" gives second.
    • insert Person { handle: "e", name: "x" } insert Person { handle: "e", name: "y" } gives y.
    • two update Person set { name: … } where handle = "a" statements give the second value.

Fix: scope the conflict rule to one group, which is the unit that shares a snapshot. Inside a group, two different effects on one entity, property or key have no order, so a refusal is correct. Between groups, a later group overwrites as today. Also remove "an update and a delete of one entity" from the list. The per-write D₂ split (line 271) already makes that case impossible, so the clause is dead text.

B8. limit with no order is not deterministic today, and the new rule changes its rows and its cost (lines 236, 242).

  • The Cut row says that over an unordered input it "orders by the tie rule alone first [as today]". Line 242 says that limit without order "therefore stays legal and deterministic".
  • Today the engine does not order there. On a local graph with Persons a to e, match { $p: Person } return { $p.handle } limit 2 returns {b, c}. explain shows LimitExec over ScanExec with ordering: null. The shipped skill says the same (skills/omnigraph/SKILL.md line 95): "Without order, limit n may return any n valid rows (the subset can change between releases)".
  • Under the new rule the same read returns {a, b}. That is a legal answer under today's contract, so it is not a break for users. But the [as today] tag is false, the rewriter's typed-IR equality cannot hold for this shape (edition 2 lowers to a bare limit, edition 3 to an order and a cut), and the cost changes.
  • The cost column of the Cut row says k. With an order by identity first, the cut must read the whole input: |R| log k, not k. For a paging-style limit 50 over a large type, that is a full scan in place of an early stop. A1 makes the cost column part of the contract, so it must be correct.

Fix: mark the rule [choice] and give the reason (the interpreter's 138 of 12,799 result). State the cost of a Cut over an unordered input as |R| log k. Name it in the rewriter section as a shape whose rows can change, with the rows-equality exception that the rrf row already has. If an early stop is still wanted for an unordered limit, that is a different operator with a declared nondeterminism, and A6 and A8 would need to say so. Do not keep both meanings under one Cut.

Should fix

  • S10. The new second bound on Expand is false when $k binds (line 229). The row adds "≤ |R| · |T′|". With $k bound, one row comes out for each edge, and parallel edges stay distinct. Test: five unkeyed Knows edges a→b, with three Person nodes. $p $k:knows $f from a returns 5 rows, more than |R| · |T′| = 3. $p knows $f returns 1 row. Apply the |T′| bound only when $k is not bound. With $k, the bound is ≤ |R| · |E<e>|, or the fan-out bound alone.
  • S11. The fuzzy budget claim does not match Lance 13 (line 834). The text says the expansion "keeps the fifty lexicographically smallest expansions per term". In lance-index 13.0.0 the budget is one budget for the whole query, used in query-position order (src/scalar/inverted.rs lines 127-166; the default of 50 is at src/scalar/inverted/query.rs line 83). Lance's own test says "max_expansions should cap the whole fuzzy query, not each token" (src/scalar/inverted/index/tests/query.rs line 475). So a multi-term fuzzy query can give a later term no expansion at all, not even its exact form. That is a wider break than the text says. Correct the sentence. Add a multi-term case to the gq3_native_fuzzy_expansion_truncates_membership_silently guard. Also check that stage L's "complete fuzzy expansion through with_max_expansions within the automaton's limit" sizes the budget for all terms together.
  • S12. Two new type and name changes are not in the rewriter's fixed map (lines 237, 1179, 1187).
    • bm25 becomes F64. Today every ranking score except rrf is F32 (crates/omnigraph-compiler/src/query/typecheck.rs lines 416-423). Line 1187 says that a result column "keeps its name and type", so the rewrite of bm25(…) as s changes a column type that the text says it keeps.
    • The fixed map has one rename, d._distance to _distance. Today an unaliased bm25(…) is d._score and an unaliased rrf(…) is d._rrf (docs/user/search/index.md lines 131-135). Edition 3 names them score.
    • List the F32 to F64 change and both renames in the fixed map and in Compatibility. Or keep bm25 at F32 in edition 3 and move the F64 score to stage L, where the rescore needs it.
  • S13. The spike evidence is not pinned, and two new rules depend on code that is not in this repository (lines 52, 1342, 1528, 1859-1861). The RFC names branches (spike/gq3-core and the others "on the fork"). The reports under docs/spikes/ are on those branches, not in this PR. Branch names move, so the counts (2,801 recursions, 3,231 declarations, 395 red-team cases) cannot be checked against the exact code that produced them. Two statements give that code a lasting role: the interpreter is "kept beside the RFC … and a change to the core changes it first" (line 1528), and "the parser spike's corpus walk is the regression owner" (line 1342). Nothing in docs/dev/testing.md owns either, and CI does not run them. Link each spike at its commit (ragnorc/omnigraph at c5ba069a1b core, 0162a33005 parser, f7b4d934a5 rewriter, 12e64ab51b lexical, f882e85bf3 red team). Then either move the reports into assets/ and give the interpreter an owner when stage B lands, or call them evidence only. Otherwise the RFC has a second executable copy of section 2 that can drift (invariant 12).
  • S14. Stale or incomplete text after the fold.
    • Line 1681, the RFC 0048 verdict row: "exact: true is l2" must be distance. This is the last stale use of the old name. The same row still says "coverage = exact stands" (S3, open).
    • Line 405, the envelope: "degree is the nesting depth of Correlate, Expand and Fix". The Fix row (line 240) now bounds rows by the key-column domains, so the degree of a Fix bound is the number of key columns, not the nesting depth (second half of S8, open). Also write "the product of the key-column domains" in the Fix row: "|domain| of its key columns" can be read as one domain.
    • Line 1660, the Rec row still says "B; N for @budget". Line 1150 now types @budget(plan = n, rounds = n) at B, so Rec does not wait for N. Also say what B does with @budget(rows = …), which the asset uses at line 677. If B ignores an argument that it does not know, B5 comes back at the argument level. If B refuses it with stage N named, say so in the Annotations row.
    • The Unresolved list is numbered 1, 11, 2, 4, 3, 4, 5, …. Markdown renumbers it, but the source is hard to cite. Item 11 is not an open question of this RFC. It is a defect in the shipped engine: fuzzy($d.title, $q, 1) can miss the document that holds the exact term. No issue exists for it (#748 is about the analyzer, not the budget). File it now, with S11's multi-term case, and link it from the RFC.

Optional

  • The monotonicity rule in the Fix row (line 240) is now complete for the thirteen operators. It is still a list of surface forms. Stage R adds rerank(model, text, k), a top-k, and the list does not name it, so as written a rerank over the recursive rows would pass. Add one sentence to the stage R row: each new relation function declares whether it is monotone in its input, and T70 is the default.
  • "no not over its own rows" (line 240): say "no not whose pattern reads the recursive relation". A not { $r blocked $_ } that is correlated on a recursive row but reads only stored edges is stratified and monotone. A reader can take the current words to refuse it.
  • The #1244 row (line 1438) still does not list recursion as a difference. #1244 refuses recursion at compile time, and this RFC now admits it.
  • S1 and S2 are still open (the decision log at line 1845, "inline door already has", and the Declaration kinds row at line 1138, which does not name stage N).

Status of earlier findings

  • Fixed: B1 (the examples are split and D₂ stays per write), B2 (groups with sequential visibility), B3 (a ranking names its column, and the rest rides as payload), B4 (A6 now has the formatter's complete list), B5 (unknown annotations are refused; this also matches today's .gq grammar, which admits only @description and @instruction, query.pest lines 118-120), B6 (sets, the full rule, exists admitted, the 74-of-817 evidence), S5, S6, S7, and the Fix row of S8.
  • Partly fixed: S8 (the envelope) and S9 (the Rec row). See S14.
  • Open: S1, S2, S3 (see S14), and S4: asset lines 661-662 still say "Each is a parse error today" and "@if_commit alone is refused by name". At this head @budget is typed at B, $t: @Type is a type error, and @valid is refused too.

Liability

The fold removes more than it adds. One Expand with a selector replaces two Union desugarings. distance gives one exact spelling on every vector field. The group rule keeps edition 2's sequential writes, so the break does not get an eighth spelling. The red-team rule that every fix is a byte-range suggestion gives the refusal texts one owner and a test. The new mechanism with lasting cost is the certified-margin fetch. It needs an ε table per term count, an F64 rescorer outside Lance and five plan-eligibility conditions (no deletion files, no row policy, one table, prefilters only, full coverage). All five are conditions on choosing a fast path, with the exact scan as the fallback, so invariant 7 holds. Write them as one eligibility predicate in RFC 0047's validator, not as checks spread across the scan code. The two risks for later changes are B7 and S13. If the conflict rule and the group rule stay two separate statements, each later write feature (returning, assert) must decide which one applies. If the interpreter stays outside the repository, each core change must update a second copy that CI does not check.

Validation

  • Clean detached worktree at d79b6ea7:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12.16): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check ab74f629 d79b6ea7: clean.
    • typos was not run, because the binary is not on this host.
  • Engine behaviour, omnigraph CLI 0.14.1-edge+269299b, local graph node Person { handle: String @key name: String age: I32? }, edge Knows: Person -> Person:
    • S10: five unkeyed a→b edges, three Persons. $p $k:knows $f from a: 5 rows. $p knows $f: 1 row.
    • B7: the three bodies above each published one commit. Final values second, y, u2.
    • B8: limit 2 with no order returned {b, c}. explain: Page / LimitExec over ScanExec, ordering: null.
    • Null order: asc puts nulls first and desc puts them last, which agrees with the RFC's fixed value order and docs/user/queries/index.md line 233.
    • .gq annotations: @foo and @owner(…) are refused by lint (Q000).
  • Code read: lance-index 13.0.0 src/scalar/inverted.rs (expansion budget), src/scalar/inverted/query.rs (default 50), src/scalar/inverted/index/tests/query.rs (whole-query cap); omnigraph-compiler typecheck.rs score_type; query.pest annotation rules; docs/user/search/index.md; skills/omnigraph/SKILL.md.
  • Not checked: the spike counts, which I did not rerun (I only confirmed that the five branches and their heads exist); the ε value; the WAND and NFC claims of section 4.
  • CI at d79b6ea7: 28 pass, 4 skipping, none failing or pending.
  • A regression test does not apply: the commits change only RFC text.

…atements, recursive types, the rrf population rewrite, the grammar decisions and the restated gate

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of d0155e50 (delta from d79b6ea7)

This review covers only the new commit d0155e50. The earlier reviews of d626e288, 73f6ee9c, 05d913e4, ab74f629 and d79b6ea7 still apply where the status list below says "open".

Why. Four more spikes (type checker, executor, second grammar round, competence instrument) tested the RFC. This commit folds their results back into the text.

What.

  • Writes: the statements of a group now run in order, and each keyed effect sees the statements before it. The conflict refusal now covers only the rows of one statement (a run-time E error). Across statements, the later effect wins. Whole-write validation runs once, at publication.
  • Core: three leaves (Unit, Env, RecRef). A block correlates when it names an outer variable again, and the inline-read T68 no longer applies to blocks. A recursive read types to Fix in release B, and the planner refuses it with P006. The monotonicity rule is stated on the term.
  • Surface: a triple with no bound end is now legal. It starts from a Scan of the edge's source type. A filter moves to just after the binder of its last variable. Arithmetic gets rules for literal types, mixed signedness, % and Date.
  • Diagnostics: codes T71 to T80, P006 and an E namespace.
  • Rewriter: the normalisation N1 to N7. The rrf row now emits a "type-population" form, because the population form changes rows on 3 of 21 cases.
  • Gate: the competence gate now uses the full front end and compares correctness with edition 2.
  • Asset: six examples corrected.
  • Net: +321 and −153 lines in three files.

Outcome. B7 is fixed, and the write rule now matches what the engine does. Most of the new text is careful, and I confirmed six of its [as today] claims on the engine. Two new problems need a fix before acceptance. (1) The RFC no longer states today's rule that a block must read an outer variable. With the new unanchored triple, a text that is refused today gets a new meaning with no warning. (2) The acceptance gate and the rewriter tally do not count the three corpus cases that the new grouped-order rule refuses. B8 is still open. The recommendation stays: revise before acceptance.

Blocking

B9. A block that reads no outer variable is no longer refused, and the new triple rule makes this a silent change (lines 333, 337-339, 562-567, 1859-1862).

  • Today a block must read an outer-bound variable. not { $q: Person { name: "zed" } } is T9 ("a negation block is uncorrelated"), and an exists block is T39 (crates/omnigraph-compiler/src/query/codes.rs lines 35 and 57). The RFC does not state this rule. The derived-forms table (lines 337-339) lowers every block to Correlate, which accepts an inner term that does not read Env.
  • This commit also makes two other rules explicit: a block sees only the text before it (lines 562-566), and a triple with no bound end is legal (line 333).
  • Together they give a new meaning to a text that is refused today. match { not { $p knows $_ } $p: Person } return { $p.handle } is T5 today. Under the new rules, $p in the block is a fresh name, the triple starts from a Scan of Person, and the block becomes a constant test: it removes every row if any person knows anyone. The writer wanted "persons who know nobody". The formatter keeps the order of blocks (lines 566-567), so it does not repair this.
  • Compatibility (lines 1859-1862) lists the texts that edition 3 now accepts. This one is not in the list.

Fix: keep today's rule as a typing rule: a block must read a variable bound before it. Keep T9 and T39, and print the fix "move the block after the binder of $p". State it once, in the Correlate record or in the scope section. If dropping the rule is a decision, list it in Compatibility and add a lint warning for a block that names a variable bound after it.

B10. The gate and the rewriter tally do not count the three cases that the new grouped-order rule refuses (lines 15, 269, 1254-1258, 1856-1859).

  • Compatibility now says that three texts edition 2 accepts are refused: "an order key over grouped rows that is a meta-field neither returned nor a key (3 stored cases)". In the corpus I find these three: issue_955_aggregate_comparison.gqt (aggregate_metadata_name_still_orders, constant_edge_type_still_orders_groups) and meta_fields_resolve_by_role.gqt (aggregate_identity_order). All three expect unordered rows.
  • The first blocked_on item requires "no hand edit to query text beyond the seventeen fusion cases" and "the rewritten corpus green". The rewriter table has no row for this shape, so the gate cannot pass as written.
  • The tally at lines 1254-1258 is not updated for this fold. It counts "28 multi-statement bodies that the group rule rewrites" among the 50 texts that "carry a refusal", but the new group rule keeps those bodies (line 269: 52 reproduce). It does not count the three grouped-order cases.

Fix: add a rewriter row. All three cases expect unordered rows, so the rewriter can drop an order key over groups that reads neither a key nor an aggregate; the rows stay equal as a bag. Where a limit follows, refuse with the fix printed, because the rows can change. Or count the three as hand edits in blocked_on and in Compatibility. Then redo the tally under the group rule.

B8 (open, not changed). The Cut row (line 241) still says that an unordered input is ordered by the tie rule [as today], with cost k. Line 247 still says that limit without order "stays legal and deterministic". The engine does not do this today (see the previous review for the evidence).

Should fix

S15. The scope of a write group is not stated (lines 266-272, 636-641, 1236).

  • "A later group sees the groups before it" can mean their effects or their variables. The text does not say whether a second group can read $p from the first group, or bind $p again. The rewriter turns two edition-2 statements into two groups, each with "a name the declaration does not use" (line 1236), so it needs this rule too.
  • An assignment reads the group's rows, which are evaluated once before the statements run. So once arithmetic exists, the group boundary changes the result. $p: Person { handle: "a" } update $p set { age: $p.age + 1 } update $p set { age: $p.age + 1 } is one group and adds 1. The same two statements, each after its own binding, are two groups and add 2.

Fix: state that a group's variables are local to the group, that a later group binds again (under any name), and that $x.prop in an assignment reads the group's rows, not the effects of earlier statements in the same group. Add the example above to the Writes tests.

S16. A7 disagrees with the new write rule and with the wrapper refusal (lines 159, 167, 186-190, 270, 641, 1068, 1204, 1407-1409).

  • A7 says that a write "evaluates them and its effects on one snapshot (A2)". With several groups, and with statements that see earlier keyed effects, a write reads several states. A2 (line 159) is about reads.
  • A7 says that a body opening with match { } "is a read when return follows and a write when a statement does". Lines 1068 and 1204 make that body a parse error whose fix removes the wrapper, and lines 1407-1409 say that the kind is decided there only to print the fix.
  • "publishes once (A4)" appears three times (lines 188, 270, 641). A4 is Closure (line 167). No axiom says that a write publishes once.

Fix: in A7, say that each group's clauses read the state that the earlier groups left, and that the write publishes once. Say that a statement after match { } is edition 2's wrapper, refused with the fix. Cite the correct axiom for publish-once, or put that rule into A7.

S17. The IR-equality claim for rrf does not fit the form the rewriter emits (lines 15, 1243, 1265-1281).

  • The rewriter now emits the type-population form for rrf: let $all = { match { $d: T } return { $d } }, then the fused rows, then "the rest of P, correlated on $d". N1 ("the read's only match clause is the from of a ranking") and N2 ("the population is the whole match") do not hold for that form.
  • N2 maps a population to the ranked scan with the pattern's filters as a prefilter. For rrf, line 1243 says that edition 2 windows the whole type and filters afterwards. So N2 can make two terms with different rows IR-equal. The numbers show it: the population form is rows-equal on only 18 of 21 cases.
  • So "27 of 28 rrf rewrites are IR-equal" (line 1275) is either about the form that is not emitted, or it is not evidence that the rows are equal.

Fix: limit N2 to nearest and bm25. In blocked_on, write "typed-IR equality on every declaration except the rrf row", as the previous version did, and keep the rows-equal check for rrf. Remove the 27-of-28 count, or say which form it measured.

S18. The "in full" grammar uses rules that it does not define (lines 1325-1375).

  • The block uses 17 rule names that it does not define and that query.pest and schema.pest on main do not define either: agg_name, annotation_value, call_name, edge_sel3, kw_exists, kw_false, kw_let, kw_match, kw_optional, kw_order, kw_query, kw_return, kw_true, or_expr3, type_ref_or_kind, undirected_edge3 and where_expr.
  • where_expr is new in this commit. It carries one of this fold's decisions (it "admits bare names", lines 1099-1102 and 1373), so its definition is part of what the rule-set pin must pin.

Fix: define where_expr and the missing keywords, or say which rules come unchanged from a named grammar file. Otherwise "In full" is not true, and the rule-set pin of section 11 has no exact set to pin.

S13 (open, now larger). This commit adds four more spike branches and gives them lasting roles: "the type checker spike (spike/gq3-typeck, 556 tests) owns these today" (lines 1597-1598); "(they are, on spike/gq3-typeck)" and "6 remain on spike/gq3-grammar2" (line 17); "the type checker produces it today and the planner answers P006" (line 18), which is true of the spike, not of main. Pin each one at its commit, ragnorc/omnigraph 2f393bb0ca (gq3-typeck), 0032c2b808 (gq3-exec), 9fef4cf3a9 (gq3-grammar2) and 45d11951ce (gq3-instrument), together with the five from the previous review. Write "on the spike" where the text says "today".

Optional

  • L codes exist already: L201 warns on a nullable property (crates/omnigraph-compiler/src/query/lint.rs line 12, docs/user/queries/index.md line 326). Line 1120 says "The lint prefix L is added". Write "new L codes", numbered after L201.
  • T76 is a good change. Today a body that mixes delete with insert passes lint and fails at run time with an uncoded bad_request. Name this move in "What a reader sees changed". The GQT corpus has no case for the D₂ refusal today, so add one.
  • Unresolved 12 is answered by the RFC's own rule: an order over groups reads keys and aggregates only (lines 1858-1859), and return distinct is a Group with no aggregates (line 336). Only a rank key is left, and the rewriter row at line 1241 already drops it. Unresolved 13 repeats item 11 for fuzzy. Both edition-2 defects are real and still have no issue. I confirmed the second one: bm25($d.title, $s) > 0.0 in a filter passes lint and fails at run time with "unsupported expression in filter" (bad_request, no code). File both and link them.
  • The unanchored triple (line 333): say whether "the smaller-named end for <e>" means the variable name. Say what happens for an edge selection whose members have different source types, for example $a (knows | likes) $b with neither end bound. Keeping T5 there is the smallest rule.
  • The rrf rewrite keeps edition 2's "window the type, then filter" in every migrated text, and line 1243 says that this is against law 1. List those texts in the rewriter's report, so that their owners can move each one to the population form on purpose.
  • The gate compares "semantic correctness on the shared tasks" with edition 2, but the text reports edition 3 over all 45 tasks (38 to 39). Report edition 3 on the 26 shared tasks, and say which number decides when runs differ (edition 2 gave 24 and 25).

Status of earlier findings

  • Fixed: B7. The conflict rule now covers the rows of one statement, across statements the later effect wins, and the dead "an update and a delete of one entity" clause is gone.
  • Open:
    • B8 (see above).
    • S10: the Expand row (line 234) still has ≤ |R| · |T′| with no condition on $k.
    • S11: lines 866-867 still say "the fifty lexicographically smallest expansions per term".
    • S12: the fixed map still has no d._score or d._rrf rename and no F32 to F64 change for bm25.
    • S13 (see above).
    • S14: line 1783 still says "exact: true is l2"; the envelope (line 420) and the Rec row (line 1762) did not change; the Unresolved list is now numbered 1, 11, 2, 4, 3, 4, …, 10, 12, 13.
    • S1, S2, S3, and S4 (asset lines 685-686 still say "Each is a parse error today" and "@if_commit alone is refused by name").

Liability

The fold removes liability in the write path. One rule (statements run in order, the later effect wins, the conflicts of one statement's rows are refused) replaces two rules that disagreed, and it matches the engine, so the 52 stored multi-statement bodies need no new spelling. Moving the D₂ refusal to a type code (T76) removes a refusal that only run time finds. The new liability is in three places. (1) Rules that are true only on spike branches: the type checker that owns tests, the planner that answers P006, the red-team counts. Until they are pinned and moved into this repository, each core change must also update a second copy that CI does not run. (2) The rrf migration keeps, in every migrated text and with no flag, a behaviour that the RFC calls wrong. (3) The unanchored triple interacts with the scope rules (B9). Each later feature that adds a binder (a polymorphic Scan, optional in writes) must check that interaction again, unless the block rule is stated once and owned by the type checker.

Validation

  • Clean detached worktree at d0155e50:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12.16): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check d79b6ea7 d0155e50: clean.
    • typos was not run, because the binary is not on this host.
  • Engine behaviour, omnigraph CLI 0.14.1-edge+269299b, local graphs (Person, Company, Knows, WorksAt; Item with an I64 key):
    • Block re-mention: $p: Person not { $p: Person { name: "bob" } } returns every person except bob. The [as today] claim holds.
    • Uncorrelated blocks (B9): not { $q: Person { name: "zed" } } is T9; exists { $q: Person { name: "bob" } } is T39; a not block before the binder of its variable is T9; not { $p knows $_ } before $p: Person is T5.
    • A triple with no bound end, $a knows $b alone, is T5. The claim holds.
    • @index(title, fulltext) and title: String @index(fulltext) are refused: "@index on Doc references unknown property 'fulltext'". The claim holds.
    • An edge insert with from: "5" into an I64 @key endpoint succeeds. The claim holds.
    • insert Tag { … } delete Tag where …: lint OK; at run time bad_request with no code.
    • bm25($d.title, $s) > 0.0 as a filter: lint OK; at run time "unsupported expression in filter" (bad_request, no code).
  • Code read: codes.rs (T5, T9, T39, T42, T58; P001 to P005; no T60 or higher on main), lint.rs (L201), the rule names in query.pest and schema.pest on main, and the three GQT cases in B10. Other open PRs: none uses T71 to T80, P006 or an E namespace (#1101 adds T60, which this RFC does not use).
  • Not checked: the spike counts (I did not rerun them; I confirmed that the four new branches exist on the fork, at the commits above), the instrument results, and ε.
  • CI at d0155e50: 28 pass, 4 skipping, none failing or pending.
  • A regression test does not apply: the commit changes only RFC text and its assets.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of a8c332ff (delta from d0155e50)

This review covers only the two new commits, 1f43be0f and a8c332ff. The review of d0155e50 and the earlier ones still apply where the status list below says "open".

Why. The type checker now exists on the front-end branch, and the competence instrument has run through it. The RFC must say how the compiler holds two editions without two pipelines, and it must record what the instrument found.

What.

  • New section "One IR" (lines 1778-1797). Both editions lower into the typed core IR, and the planner reads only the core. Edition 2 goes through the same surface tree and type checker as edition 3. Today's pipeline IR and the frozen v1 reference engine are deleted. No module name carries an edition number.
  • New blocker (line 18): one IR, with every edition-2 GQT case keeping its rows, shapes, plans and codes.
  • M0 (line 1757) and the red-team blocker (line 17): the instrument, the 30-task corpus and the 395-case red-team corpus stay outside the repository.
  • Line 16 records the full-front-end run of 2026-11-09.
  • Net: +29 and −5 lines in one file.

Outcome. The direction is good. One IR, one checker and no parallel edition modules remove a lot of liability, and deleting v1 together with the IR it depends on is a coherent choice. But there are two new blocking problems. (1) The set of places where edition 2 and edition 3 type differently is named wrong and is too small. As written, the new "keeps its codes" blocker cannot pass. (2) The run recorded on line 16 fails the gate on its own numbers, and the measurement that could pass the gate needs stage B, which the gate controls. B8, B9 and B10 are still open. My recommendation stays the same: revise before acceptance.

Blocking

B11. The edition differences in the shared checker are not the "seven freeze-list rows", and the "keeps its codes" blocker conflicts with Compatibility (lines 18, 1781-1789, 1191-1215, 1870-1882).

  • Line 1782 says that "the seven freeze-list rows" are "the only places the edition changes what the checker accepts". The freeze list (lines 1191-1215) has 17 rows, and it lists edition-3 surfaces, not differences. The seven are the rewriter's spellings (lines 68 and 1870).
  • Compatibility (lines 1870-1882) lists more differences than seven. Edition 2 accepts, and edition 3 refuses, 17 rrf shapes (identical arms, arms over two bindings) and 3 grouped-order texts. Edition 2 refuses, and edition 3 accepts, nine texts (T40, T5, T42, T9, T58).
  • The GQT corpus pins some of those edition-2 refusals today. Two examples: issue_566_order_key_binds_to_return_item.gqt boolean_key_unbound (order { $p.age > 30 }, T42, line 143), and issue_764_relationship_count_filter.gqt outer_property_as_the_bound_is_refused (count { … } >= $n.followers, T40, line 187).
  • Suppose edition 2 goes through the edition-3 checker and only seven rules are gated by edition. Then those cases are accepted under gq 2;, and the 20 edition-2 texts above are refused. Line 18 says that every edition-2 case keeps its codes. Lines 1788-1789 say that "a code that moves is one the fixed map of section 6 names". But the fixed map is the rewriter's map for edition-3 output. The two sentences do not agree, and neither one matches Compatibility.

Fix: make the edition-gated rules one table, owned by the type checker, and make that table equal to Compatibility: the seven spellings, the two rrf shapes with no rewrite, the grouped-order rule, and the five lifted refusals. Add one test per row under gq 2; and under gq 3;. Or decide that edition 2 takes edition 3's typing on some of those rows. In that case, list each GQT case and code that moves in the line-18 blocker. Do not call the table the freeze list. Each row is a branch in the checker that someone must maintain until the edition window closes. The exact count is the liability this section is meant to keep small.

B12. The 2026-11-09 run fails the gate, and the measure that would credit edition 3 needs the stage the gate controls (lines 16, 18, 1757-1758).

  • The gate: "first-try acceptance and semantic correctness on the shared tasks at or above edition 2's". The new run gives acceptance of 44 to 45 of 45, against 43 for edition 2, so acceptance passes. It gives correctness of 23 to 25 of 45 by typed-IR equality, against 28 of 45 by the same measure for edition 2. On the only correctness measure taken for both editions, edition 3 is lower.
  • Line 16 explains the gap ("a hand reading of the misses found most of them the right answer in another spelling"). But it does not say that the gate failed, and the item stays a blocker with no verdict. The hand reading is now the deciding evidence, and it is not recorded: which tasks, which verdict per task, and who read them.
  • Line 16 says that typed-IR equality "cannot credit [those spellings] until edition 3 executes". Edition 3 executes when the planner lowers the core (line 18), which is stage B work. Line 1758 says that B "ships after M0's instrument". So as written, M0 cannot reach the measure that would let it pass until B ships.

Fix: choose a measure that M0 can take before B and that credits equal meanings. Two options use what the RFC already has. (a) Put "One IR" before the M0 correctness run. When the planner reads the core, the edition-3 forms that lower into today's operators execute, so you can compare rows on the tasks that use only those forms. (b) Normalise both IRs with N1 to N7, plus a rule for each equivalent spelling the hand reading found, and report both editions on that one measure. In both cases, state this run's result (correctness by typed-IR: failed) and what changes before the next run. Publish the per-task hand verdicts beside the numbers. The seeded-repair rate is only "reported". It fell from 89-94% (16 to 17 of 18) to 79-83% (23 to 24 of 29). Say whether that drop is acceptable.

Should fix

S19. The gate evidence is outside the repository with no pinned commit, and two sections still name it as an in-tree owner (lines 16-17, 1757, 1585-1589, 1860-1861).

  • M0 says that the instrument and the 30-task corpus stay "outside the repository for now (branch gq3/phase-0-instrument on the fork)". That branch head is ragnorc/omnigraph cb0f4634 (committed 2026-11-09T09:04Z). It is the head of #1317, which was closed at 23:44Z, just before this push. A branch can move. Gate evidence needs a commit.
  • Section 10 (lines 1585-1589) still lists the 395-case red-team corpus under "Existing owners" as a compiler parser test whose "mismatch set is pinned and must shrink to zero". Invariant 13 (lines 1860-1861) says that "the instrument owns agent competence". Both now name owners that are not in the repository and that CI does not run. This is S13 again, and larger.

Fix: pin cb0f4634 (or the exact commit the 2026-11-09 run used), and pin the model and card hashes beside the numbers on line 16. In section 10, make the in-tree owner what the changelog line says: one parser or checker test for each red-team case that pins a distinct behaviour. Make the 395-case run spike evidence at a pinned commit. Change invariant 13 to match.

S20. The reason given for deleting v1 is not exact (lines 1791-1795).

  • v1 does not read today's pipeline IR. It reads a separate, frozen omnigraph_compiler::ir::untyped::QueryIR, which QueryIR::erase makes only at the test reference door (crates/omnigraph-compiler/src/ir/erase.rs, 825 lines; ir/untyped.rs; docs/dev/architecture.md line 105). So deleting the pipeline IR does not force deleting v1. The other option is a new erasure from the core IR to untyped. That is the real cost of keeping v1, and the RFC should name it.
  • The corpus has 34 --- expect same as v1 steps in 16 cases (15 under cases/, 1 under cases_slow/), not fifteen. Every one of those steps also pins its rows (4 of them pin zero rows), so "sits beside pinned rows" is true.
  • Some differential signal goes with v1. cases_slow/v2/wide_column_scan_limit_answers.gqt (line 2) records a red run where v2 hit the memory limit and v1 returned the rows.
  • "Today's typed expression nodes", with Cast (IRExpr::Cast, BlockAggregateExpr::Cast, mirrored in omnigraph-planner/src/mirror.rs), live in ir/mod.rs beside the IROp pipeline that is deleted. I confirmed the claim. But the text does not say where the boundary between "kept" and "deleted" runs.

Fix: say that v1 reads ir::untyped through erase.rs, and that the choice is between deleting v1 and writing a core-to-untyped erasure. Give the cost of each choice. Write "34 comparisons in 16 cases". List what goes with v1: the omnigraph-reference-engine crate, ir/untyped.rs, ir/erase.rs, tests/frozen.rs, the forbidden_apis.rs guard, the GQT section "Reference comparison", and the v1 text in docs/dev/testing.md, execution.md, architecture.md and .github/CODEOWNERS. Name the types that stay (IRExpr, BlockAggregateExpr, IRProjection?) and the types that go (IROp, QueryIR).

S21. The open implementation PR builds the parallel structure that this section removes.

  • #1316 (draft, head 75cb05fc, not updated since this commit) adds ir/core3.rs, query/ast3.rs, parser3.rs, typeck3.rs beside typecheck.rs, habits3.rs, print3.rs and planner edition3.rs. It also adds 397 red-team corpus files under query/testdata/gq3-redteam/.
  • The new text says that no module carries an edition number, that one checker serves both editions, and that the red-team corpus stays out of tree. #1316 does the opposite in each case.

Fix: in the stage table or in "One IR", say that #1316 converges before it merges: rename the modules, merge the two checkers, and move the corpus out of the tree, keeping one test per distinct behaviour. Otherwise the first implementation PR ships the two-pipeline structure that this RFC rules out, and a later PR must remove it.

Optional

  • Line 16 puts edition 3's typed-IR count (23 to 25) and edition 2's rows count (39 to 40) in one sentence. A small table would be clearer: one row per metric, one column per edition, with the denominator and the three runs. Edition 2's acceptance moved from 44 of 45 (2026-11-08) to 43 (2026-11-09). Say whether the cause is the front end, the card or the run-to-run spread.
  • The earlier optional point about the shared tasks is now handled: both editions are reported over the same 45 tasks.

Status of earlier findings

These commits do not touch the earlier findings. Still open: B8, B9, B10; S10, S11, S12, S14, S15, S16, S17, S18; S13 (now part of S19); and S1 to S4.

Liability

The One IR decision removes liability by design. One IR, one checker and one planner input replace two pipelines. Deleting v1 together with the IR it reads removes the frozen 6,584-line engine crate, untyped.rs (315 lines), erase.rs (825 lines) and their guards, with no second erasure left behind. Five more editions under this rule add rows to one gated table, not five module trees. The new liability is in three places. (1) Every edition-gated rule is a branch in the shared checker until the window closes, so the table must be exact (B11). (2) Without v1, the GQT corpus loses its only differential check. Pinned rows remain, but a case like the memory-limit one would no longer get a second answer. (3) The gate evidence (the instrument, the agent corpus, the red-team corpus) is outside the repository, so CI does not run it and nothing stops it from drifting from the type checker (S19).

Validation

  • Clean detached worktree at a8c332ff:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12.16): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check d0155e50 a8c332ff: clean.
    • typos was not run, because the binary is not on this host.
  • Code read at a8c332ff: ir/erase.rs (QueryIR::erase → untyped::QueryIR), ir/untyped.rs, ir/mod.rs (IRExpr::Cast, BlockAggregateExpr::Cast), omnigraph-planner/src/mirror.rs (the Cast mirror), docs/dev/architecture.md line 105, and the GQT README section "Reference comparison".
  • Corpus counts, from a script over crates/omnigraph-gqt/cases*: 34 exact --- expect same as v1 lines in 16 files. Every one of those steps has a rows expectation (4 are empty). The edition-2 error expectations include T5 ×4, T40 ×2, T42 ×5 and T9 ×1. I read the T42 and T40 cases cited in B11.
  • Other PRs: #1316's file list through the GitHub API (head 75cb05fc). #1317 is closed (2026-11-09T23:44:17Z), and its head cb0f4634 is the head of ragnorc/omnigraph gq3/phase-0-instrument.
  • Not checked: the instrument numbers on line 16 (I did not rerun the instrument), and the hand reading.
  • CI at a8c332ff when I wrote this: 19 pass, 4 skipping, 7 pending, none failing.
  • A regression test does not apply: the commits change only RFC text.

…; the recursive example no longer shadows the distance row

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of 88cb905d (delta from a8c332ff)

This review covers only the new commit 88cb905d (+7/−5 lines, docs/rfcs/2026-11-07-gq-3.md). The review of a8c332ff and the earlier ones still apply where the status list below says "open".

Why. The previous review found two problems in the text this commit changes. The recursive example used a read name that the RFC's own rules refuse. "One IR" also named too few edition differences in the shared checker (B11).

What.

  • Lines 599-610: the recursive example distance($from) is now hops($from).
  • Lines 1782-1785: the edition now changes what the checker accepts at "the seven freeze-list rows and at the acceptance differences the Compatibility section lists, all in one place in the checker". Before, it said the seven rows were the only places.

Outcome. Both changes go in the right direction. The rename is correct and complete. The "One IR" change adopts the main part of the B11 fix: one table in the checker, equal to the Compatibility list. But B11 is not closed. The new sentence still calls the seven rows "freeze-list rows". One sentence about codes now conflicts with the new rule. No test owner exists for the table yet. And one acceptance difference that Compatibility describes is not counted, while the freeze list says the opposite. My recommendation stays the same: revise before acceptance.

Closed by this commit

  • Read name distance shadowed a registry row. distance is a registry row of kind relation (line 1040). A declared read may not shadow a registry name (lines 1035 and 1049). So the old example broke the RFC's own Call rule. hops is not a registry row, a clause keyword or a reserved word, and no other distance( in the file names the example. Every remaining distance( is the ranking row (lines 349, 525, 765, 915, 1525).

Blocking

B11 (narrowed). The edition-gated table is now the right idea, but its rows are still not stated exactly (lines 18, 1782-1789, 1082, 1205, 1577, 1870-1888).

  1. Name. The freeze list (lines 1191-1215) has 17 rows, and each row is an edition-3 surface, not a difference. The seven are the rewriter table's spellings (lines 68 and 1872). Also, Compatibility already lists those seven, so "the seven … rows and the acceptance differences the Compatibility section lists" counts them twice. Write "at the differences the Compatibility section lists". Then Compatibility is the one source for the table.
  2. Codes. Lines 1787-1789 still say "a code that moves is one the fixed map of section 6 names". With every difference gated, no edition-2 code moves under gq 2;. The fixed map changes codes in the rewritten edition-3 copy, not in edition-2 cases. Line 18 says that every edition-2 case "keeps its … codes" with no exception. Remove the clause, or say that it applies to the rewritten copy.
  3. A difference that is not counted. Compatibility (lines 1886-1888) says that widening <e> over an asymmetric alternation "turns a refusal into a meaning". That is an acceptance difference: today's checker refuses it with T22 (crates/omnigraph-compiler/src/query/typecheck.rs lines 2307-2312 for the wildcard and lines 2407-2414 for each member). But it is not one of the "nine texts edition 2 refuses". The freeze list traversal row (line 1205) says edition 3 works "with today's T5 and T22", and line 1082 says that T22 keeps its text. These statements cannot all be true. Decide which one holds. If edition 3 widens, add the row to the table and to the count, and keep T22 under gq 2;. If the widening also covers the wildcard, the GQT case issue_659_compiler_scopes.gqt undirected_cross_type_wildcard (lines 210-214, $c <*> $p between Company and Person, T22) is an edition-2 refusal that must stay. If edition 3 keeps T22, remove the widening sentence.
  4. Test owner. Section 10 has "one test per freeze-list row" for the parser (line 1577), but no test for each edition-gated row. Add one type-check test per row, under gq 2; and under gq 3;. Each row is a branch in the shared checker until the edition window closes, so each one needs an owner that fails when the row drifts.

Status of earlier findings

This commit closes the shadowing problem above and part of B11. Still open: B8, B9, B10, B11 (items 1-4 above), B12; S1 to S4, S10 to S12, S14 to S21 (S13 is part of S19). Line 1795 still says "fifteen" expect same as v1 comparisons (S20).

Liability

The change lowers liability. One gated table that equals Compatibility is the smallest structure that keeps edition 2 stable while one checker serves both editions. Each further edition adds rows to that table, not a second checker. The cost is that each row is a live branch in the checker until the window closes. So the table must have an exact row count, one source (Compatibility) and one test per row under each edition. Items 1-4 are the gaps between the text and that structure. The rename adds no liability and removes an internal conflict.

Validation

  • Clean detached worktree at 88cb905d:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12.16): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check a8c332ff 88cb905d: clean.
    • typos was not run, because the binary is not on this host.
  • Text checks: every distance( and hops in the RFC; the Call rule and the registry rows (lines 1033-1049); the freeze list row count (17); Compatibility (lines 1870-1888).
  • Code read at 88cb905d: the T22 checks in crates/omnigraph-compiler/src/query/typecheck.rs and the T22 case in crates/omnigraph-gqt/cases/issue_659_compiler_scopes.gqt.
  • CI at 88cb905d: 26 pass, 4 skipped, 2 fail. The failures are RustFS S3 Integration (default) and Azurite Azure Integration. The RustFS failure is an assertion at crates/omnigraph-server/tests/integration/s3.rs:97. The same two jobs fail on main at c61f242e (run 42727252027), and #1332 changes that test. This PR changes only RFC text, so it does not cause them.
  • A regression test does not apply: the commit changes only RFC text.

… one change, one grammar, checker and IR, a one-shot rewriter, a core reference interpreter

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of 0bb5f69c (delta from 88cb905d)

This review covers only the two new commits, both in docs/rfcs/2026-11-07-gq-3.md:

  • cf0594d8: the grammar rules take plain names (no 3 suffix).
  • 0bb5f69c: no backwards compatibility. Edition 3 replaces edition 2 in one change. There is no edition line, no boot rewrite and no queries migrate. A one-shot rewriter converts text. A core reference interpreter replaces engine v1.

The review of 88cb905d and the earlier reviews still apply where "Status" below says "open".

Why. The edition window was the main source of B11. It needed one gated rule table in the checker, an edition recorded beside each stored source, a boot rewrite and a second grammar. This commit removes the window, so all of that goes.

What. Sections 6 ("One language", "The rewriter", "Stored sources at upgrade"), 7 (grammar names), 10 (test owners), 11 ("One IR", stage B), Compatibility, the line-18 blocker and the decision log.

Outcome. The decision lowers code liability, and the text is mostly consistent with it. But the text does not yet say how the break reaches a running deployment. The rewriter needs the edition-2 front end that this change deletes (B13). The server refuses each stored-query bundle on the other side of the upgrade, so no upgrade order avoids an outage, and rollback needs the old bundle (B14). One B11 item is still open. My recommendation stays: revise before acceptance.

Closed or changed by this delta

  • B11 items 1, 2 and 4: closed. No edition-gated table remains, so its name, its codes under gq 2; and its per-row tests are no longer needed. Item 3 is still open (below).
  • cf0594d8: correct. Edition 2's grammar no longer shares the file, so plain names do not collide. I found no leftover …3 rule name in section 7.
  • The edition line is gone throughout. No gq 3;, gq 2;, "edition line", "edition window" or queries migrate remains in the RFC text, except the decision log entry that records the removal. There is one stale line (S22).
  • S20: partly closed. The "fifteen comparisons" count is gone. The new interpreter answers the earlier liability point that removing v1 would leave GQT with no differential check. But the reason given is still not exact (S23).
  • S21: changed. #1316 at a14fe7ba no longer has modules with an edition number in the name. But it now builds what this commit removes: query/syntax/edition.rs, query/syntax/edition_window/, and the Q022 case crates/omnigraph-gqt/cases/diagnostics/q022_malformed_edition_line.gqt ("gq 3; as the fix"). Before #1316 merges, it must remove the edition line and Q022, and move the edition-2 path out of the product (B13 decides where it goes).

Blocking

B13. The rewriter needs edition 2's front end, and this change deletes it from the product (lines 18, 1149-1150, 1221-1229, 1286-1288, 1876-1877).

  • The rewriter "parses each source under edition 2" and "refuses to write when the edited text does not compile to a typed IR equal to the original's" (lines 1226-1228). For that proof, the edition-2 parser and checker must lower into the same core IR as edition 3, in one binary. For the rrf row, the proof is rows-equality, so that binary must also run both spellings on the engine.
  • Line 1149 says that the release has "no edition-2 parser in the product". Line 1776 says that edition 2's grammar and checker "are deleted in the same change". So nothing built from the released tree can run the rewriter.
  • But operators must run it: "an operator's bundle once before upgrading" (lines 1225-1226) and "the one-shot rewriter converts stored text with proof before an upgrade" (lines 1876-1877). The RFC does not say which binary they run, where its source is, or how they get it.

Fix: name the tool's home and how it is distributed. One option: build it from the last commit in which both front ends lower into the core (this is what #1316's edition_window/ builds), record that commit, and attach the binary to the break release. Keep it buildable while known deployments have not upgraded. The other option is to ship it in the product for one release, but that is the edition window this commit removes. Put the chosen option in stage B and in the line-18 blocker.

B14. The upgrade has an outage in every order, and rollback needs the old bundle (lines 1284-1288, 1894-1897).

  • Today the server type-checks every stored query with its own compiler when it opens a graph (validate_and_attach, crates/omnigraph-server/src/lib.rs lines 3752-3786). It does the same again when a served cluster apply activates (validate_deployment_activation, crates/omnigraph-server/src/registry.rs lines 648-668, InvalidQueries). A breakage quarantines that graph. Startup fails when no graph is healthy, or when --require-all-graphs is set (lib.rs lines 232-234 and 4161-4164).
  • So an old server refuses the rewritten bundle at activation, and a new server quarantines a graph whose bundle is not rewritten. No order keeps a graph with stored queries serving across the binary swap. "An operator rewrites the bundle before upgrading" can only mean rewriting the files. It cannot mean applying them.
  • "Refuses at boot" (line 1286) is not exact. The server quarantines the graph. A deployment with one graph does not start.
  • Rollback: after an operator applies the rewritten bundle, a reverted binary (edition 2) quarantines it. No reverse rewriter exists. "Reverting is reverting the change" (line 1895) is true for the repository, but not for a deployment.

Fix: write the operator procedure in "Stored sources at upgrade". (1) Rewrite the files. (2) Stop serving. (3) Change the binary. (4) Apply the bundle. Say whether this is a direct root apply with writers stopped, or a served apply to a server that started with the graph quarantined, and confirm that the server supports that path. (5) Start the server and verify. State the expected outage for each deployment. Say that the pre-rewrite bundle revision is the rollback artifact, and that the binary and the bundle roll back together. Say who runs the procedure for managed clusters. If an outage for each deployment is acceptable, say so in Compatibility. That is the cost that this decision accepts in exchange for one grammar.

B11 (item 3, still open). <e> over an asymmetric alternation: widen it or keep T22 (lines 1083, 1204, 1877-1879, 18).

Without editions this is no longer an edition question, but the text still contradicts itself. Line 1877 says the widening "turns a refusal into a meaning". Line 1083 says T22 keeps its text, and the freeze-list traversal row (line 1204) says traversal works "with today's T5 and T22". GQT pins the refusal: issue_659_compiler_scopes.gqt undirected_cross_type_wildcard (lines 210-214, $c <*> $p, T22). Line 18 says that every rewritten case keeps its rows "except where the fixed map names a move", and a refusal that becomes rows is not in the fixed map. Choose one. If edition 3 widens, add the change to Compatibility's list of lifted refusals, name every GQT case whose T22 changes, and correct lines 1083 and 1204. If it does not widen, remove the sentence.

Should fix

S22. Stale test owner (lines 1663-1664). The Server owner still lists "the boot rewrite and its warning" and "the release-after refusal". Neither exists now. Replace them with: an edition-2 stored source quarantines its graph, and the diagnostic carries the fix; queries validate and cluster plan report it.

S23. The reference interpreter has no in-tree owner or scope (lines 18, 1784-1788, 1602-1610).

  • Section 10 names only the Python spike (spike/gq3-core, 142 tests). But the line-18 blocker needs "a core reference interpreter" in the product's place of v1. Name the crate and the language. Name the GQT directive that replaces --- expect same as v1 (34 steps in 14 files under cases/ at this head, plus the generated cases in crates/omnigraph-gqt/src/tests.rs lines 10 and 57). Say whether it runs on every case or only on marked cases.
  • Scope: v1 is frozen, so keeping it costs almost nothing. An interpreter that serves as the oracle must grow with each operator and registry row of stages S, L and R. Say which rows it evaluates (for example exact distance and bm25_v1). Also say whether it shares one exact scorer with the engine's exact scan of section 4. A shared scorer cannot drift but checks less. An independent scorer checks more, but two scorers must stay equal. Choose one and state it.
  • S20, item 1: v1 does not read the pipeline IR. It reads ir::untyped, made by ir/erase.rs (both are present at this head). "Goes with the pipeline IR it reads" (line 1784) is still not exact. Write that v1, untyped.rs and erase.rs go because the interpreter replaces them.

Optional

  • Wire (lines 1891-1893). The HTTP contract stays at 0.14, but POST /query, /mutate and MCP stop accepting edition-2 text. docs/dev/versioning.md already requires coordinated client and server upgrades, and each refused form prints its fix, so this can be acceptable. But a client cannot find the server's language from the contract header. Either say that GQ source text is not part of the wire contract version, or advance the version.
  • Line 135 still calls the break "the one release that moves stored text". The move now happens outside the release, with a tool. Consider rewording.

Status of earlier findings

This delta closes B11 items 1, 2 and 4, and part of S20. It changes S21 (above). It does not touch B8, B9, B10, B12, S1 to S4, S10 to S12, S14 to S19, so those stay open.

Liability

The decision removes liability by design. It removes the edition-line parser, the edition field in the registry, the boot rewrite, a queries migrate command, the gated rule table in the checker, a second grammar with suffixed names, and the clean-up release after the window. Five more breaks like this one each cost one change and one tool. They do not add branches that live in the checker.

It adds three costs. (1) An outage for each deployment at each break, and a rollback in which the binary and the bundle must go back together (B14). (2) A tool that must stay buildable from a recorded commit until known operators have upgraded (B13). (3) A reference interpreter that is a second implementation of every core operator, and that must grow with the programme (S23). The interpreter is a better oracle than v1, because it checks the IR that the planner reads. Overall, code liability goes down and operational cost per break goes up. That is a good trade if the number of deployments is small and the procedure is written down.

Validation

  • Clean detached worktree at 0bb5f69c:
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt (Python 3.12.16): "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check 88cb905d 0bb5f69c: clean.
    • typos was not run, because the binary is not on this host.
  • Text checks over the whole RFC: gq 3, gq 2;, "edition line", queries migrate, "boot", "window", v1, T22, "interpreter", "fixed map".
  • Code read at 0bb5f69c: crates/omnigraph-server/src/lib.rs (validate_and_attach, the quarantine policy), crates/omnigraph-server/src/registry.rs (validate_deployment_activation), crates/omnigraph-compiler/src/ir/ (erase.rs, untyped.rs), the GQT expect same as v1 steps and issue_659_compiler_scopes.gqt. I did not run a server upgrade. The outage in B14 comes from reading the code, not from a test.
  • #1316 at a14fe7ba: file list and description (edition line, edition_window/, Q022).
  • CI at 0bb5f69c: 28 pass, 4 skipped, 0 fail (including Storage Upgrade Compatibility).
  • A regression test does not apply: the commits change only RFC text.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of 24991046 (delta from 0bb5f69c)

This review covers one new commit, 24991046 ("no upgrade command; a stored source in the old syntax fails with a fix an agent applies"). It changes only docs/rfcs/2026-11-07-gq-3.md (+12/-7): "The rewriter", "Stored sources at upgrade", Compatibility and the decision log. The review of 0bb5f69c and the earlier reviews still apply where "Status" below says "open".

Why. B13 found that operators had to run a rewriter that needs the edition-2 front end, which the break deletes from the product.

What. Operators no longer run the rewriter. It runs once over the repository, in the break's change, and then it is discarded. No upgrade or migrate command ships. A stored source in edition 2 fails with its diagnostic, and an agent or a person applies the printed fix.

Impact. B13 is closed by design. But the printed fix is now the only way that operator stored text moves to edition 3. The RFC makes the fixes "part of the break's contract" without the proof that the rewriter has. It also says that every refused edition-2 form has a fix, which other sections contradict (B15). B14 (the outage and rollback during the upgrade) is still open. My recommendation stays: revise before acceptance.

Closed or changed by this delta

  • B13: closed. The release's own CLI prints the fixes, so operators need no other binary. lint, cluster validate and cluster plan already carry the compiler's diagnostic, with its suggestion, for parse and type errors in stored queries (validate_query_source, crates/omnigraph-cluster/src/config.rs lines 1378-1425; Diagnostic.detail, crates/omnigraph-cluster/src/types.rs lines 20-23). The rewriter needs both front ends only inside the break's change. Optional: record the commit in which the rewriter ran, so that its report (600 rewritten, 50 refused, 17 hand edits; lines 1256-1260) can be reproduced after the tool is discarded.
  • B14: changed, still open. Step 1 of the operator procedure is now "apply the printed fixes with the new CLI, before the binary swap". The rest of B14 does not change: the old server refuses the edition-3 bundle at activation, the new server quarantines a graph whose bundle is still edition 2, rollback needs the old bundle together with the old binary, and line 1287 still says "refuses at boot" (the server quarantines the graph). The RFC still does not say who applies the fixes for a managed cluster.
  • The decision log, line 1151 and Compatibility agree with the decision. I found no remaining text that says an operator runs the rewriter.

Blocking

B15. The printed fix replaces a proven conversion with an unproven one, and the RFC says more fixes exist than it defines (lines 135-136, 1151-1152, 1287-1292, 1880-1881).

(a) No proof of meaning.

  • The rewriter "refuses to write when the edited text does not compile to a typed IR equal to the original's" (lines 1228-1229), after normalisation N1 to N7. For rrf it writes the type-population form, because the population form "changes rows on 3 of 21 stored cases" (line 1245).
  • A printed fix comes from a release that has no edition-2 checker, so it cannot do this check. The per-code test (lines 1088-1091, and the third blocked_on entry) applies each suggestion and compares the reparsed tree with a pinned one. That proves the syntax. It does not prove that the meaning of the edition-2 text is kept.
  • The T65 rows in section 3 (lines 678-679) give the fix for nearest and bm25. No text says what T65 prints for an edition-2 order { rrf(…) }, for a rank key under an aggregate (k takes the value of limit), or for the unaliased nearest column that becomes _distance. If the rrf fix prints the canonical population form, an operator's fusion query changes rows without an error. The rewriter refuses to do exactly that.
  • Line 135 says that the break moves stored text "mechanically and with proof". This is now true for the repository only.

Fix. This uses only existing parts and adds no compatibility path to the product:

  1. In the break's change, while both front ends exist, apply the printed fix to each edition-2 declaration that the rewriter rewrites (the 600). Require the result to be tree-equal to the rewriter's proven output.
  2. Keep those pairs (edition-2 text and proven edition-3 text) as the per-code fixtures. After edition 2 is deleted, the fixtures still compare each fix with a proven result, and they need only edition 3 to run.
  3. Give applicability: machine_applicable only to fixes that pass. Give needs_review to the others. docs/user/queries/diagnostics.md (lines 62-63) already defines both values.
  4. State that the rrf fix prints the type-population form of line 1245, or list the changed fusion rows in Compatibility's "Results".

(b) Not every refused edition-2 form has a fix. Lines 1151-1152 and 1289-1290 say that every refused edition-2 form prints its fix. But Compatibility (lines 1868-1874) lists edition-2 text that is refused with no rewrite: an rrf with identical arms (15 cases), an rrf whose arms rank two bindings (2 cases), and an order key over grouped rows that is a meta-field (3 cases). Section 4's errors (lines 1006-1009) give no fix for these. Fix: change the claim to "every form in the rewriter table". Add these three shapes to the section 3 list of refusals that name the construct but have no mechanical fix, and say what the author must decide for each one.

(c) The list of codes is wrong (line 1291). It names T7, T65, T71 and Q008.

  • Q008 is not allocated. At this head, docs/user/queries/diagnostics.md lists Q001 to Q005. The RFC allocates T71 to T80, P006 and the E namespace (lines 1108-1118), but no Q code. Q008 appears only on line 1291.
  • The list does not include the refusal for infix contains and starts_with, which covers the first three rewriter rows. Line 697 gives that refusal no code.
  • The list does not include the parse error for delete T where and update T set … where (where_stmt, line 1372), unless that is what Q008 means.

Fix: add a "Code" column to the rewriter table, and say that the fix for each row's code is part of the break's contract. Then the contract has one source, and no second list can drift from the table.

Should fix

  • S22 (still open, now lines 1667-1668). The Server test owner still lists "the boot rewrite and its warning" and "the release-after refusal". Replace them with: an edition-2 stored source quarantines its graph, and its diagnostic carries the fix; queries validate, cluster validate and cluster plan report that fix as a suggestion.

Optional

  • A source can have several edition-2 forms. Say whether one diagnostic carries the suggestions for all of them, or whether the author fixes them one at a time and compiles again. An agent loop works either way. But for an operator with many stored queries, the number of rounds is part of the upgrade cost in B14.
  • Say how long the edition-2 recognisers stay in the grammar (where_stmt is "edition 2's form, recognised first to print the fix", line 1372). They also catch habits that agents have, so "for all time" can be the right answer. Write it down, so that the next break does not have to decide it again.

Status of earlier findings

This delta closes B13 and changes B14 (above). It does not touch B8, B9, B10, B11 item 3, B12, S1 to S4, S10 to S12, S14 to S19, S22 or S23, so those stay open. S21 still applies: #1316 at a14fe7ba builds the edition line, edition_window/ and Q022, and the head of #1316 has not changed.

Liability

The commit removes liability. No migrate command ships, and no operator tool has to stay buildable from an old commit. That removes the cost (2) in the liability section of the previous review.

The knowledge of how to convert does not go away, though. It moves into the fix printers, which are in the product: the where_stmt rule, the infix-predicate recogniser and the T65 fixes. They become the operator's only conversion path, and nothing checks them against meaning. With the fixtures from B15(a), each fix printer is held to a proven result for the cost of a fixture file per code. Five more breaks like this one each add some recognisers and their fixtures. That stays linear, but only if the fixtures come from a proof and are not written by hand.

Validation

  • Clean detached worktree at 24991046 (removed afterwards; no source edits):
    • python3 scripts/check-docs.py with the pinned scripts/requirements-docs.txt: "Documentation OK (473 Markdown files checked)".
    • bash scripts/check-agents-md.sh: OK (67 links, 63 docs).
    • git diff --check 0bb5f69c 24991046: clean.
    • typos was not run, because the binary is not on this host.
  • Text checks over the whole RFC: Q008 and the other Q codes, T7, T65, T71, "operator", "bundle", "at boot", "by hand", the rewriter table and the Compatibility list.
  • Code and docs read at 24991046: crates/omnigraph-cluster/src/config.rs (validate_query_source, stored_declarations), crates/omnigraph-cluster/src/types.rs (Diagnostic), docs/user/queries/diagnostics.md (parse codes, suggestion, applicability).
  • I did not run an upgrade or a fix printer; no fix printer exists yet. B15(a) comes from reading the text.
  • CI at 24991046: 28 pass, 4 skipped, 0 fail.
  • A regression test does not apply, because the commit changes only RFC text.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the delta 2499104..1f5aef7 ("designs and names for relation parameters and paths, stages RP and Path"). The earlier commits are covered by the review on 2499104.

Why, what, how, impact. The commit moves two items out of "Inexpressible by decision": relation-valued parameters (stage RP) and paths as values (stage Path). RP inlines a relation argument under the existing Call rule, so the planner never sees it, and adds one leaf, Values, for rows that a client binds. Path adds a Path value kind, three mode words, shortest derived over Fix, and min_by/max_by. The change is design text only. It adds no code and has no runtime effect. Its impact is on the grammar and the checker rules that #1316 and the later stages will implement, so its spelling and its rules must agree with the rest of the RFC.

Outcome: revise. The design direction is sound. Reusing Call inlining and Fix is better than adding core operators, and the polynomial-only path table is correct (counting simple paths is #P-hard, and a grid has exponentially many shortest paths). Three statements disagree with the RFC's own rules or with the grammar.

Blocking

B1. knows*{1,3} and knows* are not GQ syntax, and * already means the wildcard selector.

  • The RFC keeps edition 2's triples "<e>, (a | b), *{m,n}, $k:e" (gq-3.md:447). In edition 2, * is the wildcard edge selector and the bounds follow the selector: query.pest:169-175 (edge_wildcard = { "*" }, then traversal_bounds?). The edition-3 grammar in #1316 has the same shape: grammar.pest:254-257 parses one edge_sel, which is an alternation, the wildcard, or an edge name, and then optional bounds.
  • The RFC's own asset writes $p knows{1,2} $q (gq-3-before-and-after.md:232) and $p *{1,3} $near (:164).
  • The new text writes $a $p:knows*{1,3} $b, shortest $a $p:knows* $b and the Names row $p:knows*{1,3}. It also says that $a knows*{1,3} $b "stays … as today" (gq-3.md:1864), but today's form is knows{1,3}. Both parsers refuse knows*{1,3}. In $p:*, the reader cannot tell "any edge type" from "any length".
  • Suggestion: keep the existing spelling, $a $p:knows{1,3} $b, and write the one unbounded form as an open upper bound, shortest $a $p:knows{1,} $b. traversal_bounds already parses {1,} and refuses it today (T15). shortest would then be the one context that admits it. This adds no token and keeps one spelling for a hop count. Update the table at :1844, the examples at :1850-:1860, the bullets at :1864-:1886 and the Names row at :1912.

B2. shortest computes a distance under min, and the Fix rule requires a declared round budget for that, but the design declares none.

  • The core table says: "a column it computes rather than reads is admitted only under min or max with a declared round budget" (gq-3.md:246). The Recursion section says the same, with @budget(rounds = n) (:587).
  • shortest "keeps each target's least distance with min". That distance is computed (d + 1), so it is the hops case. But the design says shortest "admits an unbounded *" and gives no budget.
  • Either rule can hold, but the RFC must choose one. (a) The derivation supplies a budget: with a unit step, the least distances settle within |T′| rounds, so the derived Fix carries rounds = |T′|. State this as the derivation's budget, not as an exception to the rule. (b) The query must declare @budget(rounds = n). If you choose (b), stage Path depends on N as well as Rec, because Rec's @budget needs N (programme table, Rec row). The Path row now says "B; Rec for shortest".

B3. Binding $p: changes which endpoints match, not only how many rows match. The text says it follows the one-hop $k: rule.

  • Expand is "set semantics over endpoint pairs, shortest distance" (gq-3.md:236), and it never returns the start node (:616). A path binder gives "one row per path" of length m..n.
  • Example with edges a→b, a→c and c→b of type knows: $a knows{2,2} $b from a gives no rows, because b and c are both at distance 1. $a $p:knows{2,2} $b gives b through a→c→b. With edges a→c and c→a, the trail a→c→a returns a itself, which Expand never does.
  • So return distinct { $b } gives different sets with and without the binder. A one-hop $k: changes only multiplicity (parallel edges). An agent that adds $p: only to read length($p) gets a different set of $b without any message.
  • Suggestion: remove the analogy. State the rule: "a path binder matches paths by their own length; without one, the triple matches endpoints by shortest distance". Include the example above. Then have the reference interpreter check the stated relation: for m = 1, the distinct endpoints of the path form are the unbound form's endpoints plus the start node when a cycle returns to it. If a corpus shows agents trip on this, an N-stage notification can name it.

Optional

  • O1. shortest *{m,n} needs a definition. The Fix derivation computes geodesics, so the bound can only filter the geodesic length. GQL's ANY SHORTEST with a quantifier {m,n} instead returns the shortest path among paths whose length is in m..n. Example: with a→b direct and a→c→b, {2,3} gives no row under the derivation, but gives a→c→b under GQL. State the filter meaning and the difference from GQL.
  • O2. The bounded-path row says its bound is "Expand's own". Only the first term of Expand's bound applies. Paths have no ≤ |R| · |T′| bound, so write the fanout sum alone.
  • O3. Forward pointers. The core still says "over three leaves: Unit, Env, RecRef" (:224). The type table still says a relation column is "scalar or reference" (:393). The Call paragraph still says "Scalar arguments only [choice]; a relation-valued parameter is a named closure exception" (around :300). The Values leaf, the Path kind and List of references change all three when their stages land. One clause in each RP/Path stage row that names these amendments keeps the core text in one place.
  • O4. RP row and open question 4. The programme row commits to client-bound relation parameters. Open question 4 asks whether they ship with the stage. Mark that half of the row as conditional on the question.
  • O5. Cost of a correlated relation argument. A relation argument with free variables, passed into a recursive read, makes one Fix per outer row (Correlate over Fix). Say so beside "A closure", so the cost shape stays explicit.
  • O6. Open-question numbering. The open-question list reads 1, 11, 2, 4, 3, 4, 5, … 14, and this commit adds a second item 4 (stage RP). GitHub renumbers ordered lists in sequence, so a reference to "open question 4" can mean either item. The problem is older than this commit, but this commit touches the list.

Liability

The delta adds no code. As design, it adds one core leaf (Values), one value kind (Path), List of references, three mode words, the shortest derived form and two lattice aggregates. It keeps the planner first-order, so A1's proof does not change. That is a smaller liability than a shortest_paths relation function beside Fix would be, and the commit removes that function from stage R. The new value kind has equality and order rules, so distinct and Order need no new case. After five more value kinds of this sort, the main cost is the per-kind rules for equality, order and cell typing. The design states them. B1 would add a second spelling for a hop count, and B3 a silent change of meaning, which is why both are blocking.

Validation

  • Exact head 1f5aef7, base 2499104, merge base ea4a16d with main. Reviewed in an isolated worktree.
  • python3 scripts/check-docs.py at 1f5aef7, with the pinned doc packages: "Documentation OK (473 Markdown files checked)". typos was not run because it is not installed on this host.
  • CI on the exact head: 26 check runs succeeded and 4 were skipped (tag and release jobs).
  • Grammar claims were checked against crates/omnigraph-compiler/src/query/query.pest on this head and crates/omnigraph-compiler/src/query/syntax/grammar.pest on #1316 head ae9c2a4.
  • GQT regression: not applicable, because the change is RFC text with no behavior. Lance: not applicable, because the change touches no storage path.
  • Not done: no parser or interpreter was run on the new examples. The B1 claim comes from reading the grammar files, not from running them.

…bsolute BM25 margin, composed degrees, and the undefined edge cases

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the delta 1f5aef7..9853010 ("fix the whole-document review: monotone lattice steps, an absolute BM25 margin, composed degrees, and the undefined edge cases"). The earlier commits are covered by the review on 1f5aef7.

Why, what, how, impact. The commit answers a review of the whole RFC. It narrows the Fix rule to steps that keep the order of a min or max column. It adds an absolute term δ to the certified BM25 fetch, because Lance can round a common term's IDF to zero. It derives the polynomial degree from the operator bounds, not from nesting depth. It defines several edge cases: the Expand start node and self-loops, a name shared by a node type and an edge type, what recall: promises, lazy if and coalesce, cosine at a zero vector, a BM25 term with no stored match, the exact target window, and the ranks of an ordered rrf arm. It removes stage W2. It corrects the two assets. The change is design text only. Its impact is on the rules that #1316 and the later stages implement.

Outcome: revise. Most of the delta is correct and makes the RFC more exact. The δ term is necessary: no relative bound covers an error from zero, and 2.2 is the correct upper bound of a BM25 term weight with k1 = 1.2. The composed degree fixes a real error (Correlate(Correlate(A, B), Correlate(C, D)) has degree four, not two). Lazy if and coalesce make if($a = 0, 0, 1 / $a) safe and keep the A3 guard rule in one place. Three statements are wrong or incomplete. Also, B1 to B3 of the previous review are still open at this head.

Blocking

B1. The new self-loop rule changes rows that today's engine returns and that a test fixes, and the RFC does not list the change.

  • The Expand row now says that "a range other than the single hop never returns [the start node], through a cycle [as today] or a self-loop [choice]", and that the single hop returns it through a self-loop (gq-3.md:235, :637).
  • Today the engine returns the start node through its own self-loop in a range. variable_hops_handle_self_loop (crates/omnigraph/tests/integration/traversal_indexed.rs:365-383) loads a->a, a->b and expects $p knows{1,5} $f from a to return ["a", "b"]. The comment there states the rule: "the start node's own self-edge is a hop-1 destination".
  • So the [choice] is a change in results for stored queries. The Compatibility section says "Rows change only where edition 2's answer depended on index state or letter case, …" (gq-3.md:2088). This change is not in that list, and the one-shot rewriter cannot detect it because the text does not change.
  • The rule also makes the result shrink when the range grows. From a with a self-loop, knows{1,1} returns a, but knows{1,2} does not. Today {1,1} is a subset of {1,n}. An agent that widens a bound expects to get more rows, not fewer.
  • Suggestion: keep today's rule. A self-loop is a hop-1 edge, so the start node is at distance 1 through it in every range with m = 1, and a cycle of length 2 or more never returns it. This is what the engine and its tests do, it keeps {1,1} ⊆ {1,n}, and no stored query changes. If you keep the new choice, add it to the Results bullet, state the {1,1} ⊄ {1,n} consequence, and change the test in the same stage.

B2. The monotone rule checks the order of the column that an expression reads, but not the direction of the column that it writes. A min column can feed a max column, and the result then depends on evaluation order.

  • The new rule admits $d + t, $d * c and similar expressions "that read" a min or max column (gq-3.md:246, :607). It does not say that the result must go to a column with the same direction.
  • Example: return { $q, min($d) as d, max($e) as e }, where the step computes $r as q, $d + $w as d, $d + $w as e and $w is an edge property. Every expression reads $d with $d + t, so the new rule admits it. With edges s→a (10), s→b (1), b→a (1), a→c (1):
    • Naive and semi-naive rounds both give c: d = 3, e = 11. The stale a.d = 10 from round 1 reaches c.e before round 2 improves a.d to 2.
    • A priority (Dijkstra) order gives c: d = 3, e = 3, because it never expands the stale a.d = 10.
  • When d improves, it gets smaller, so e gets a smaller candidate. In e's lattice that is a worse value. The step is antitone from d to e, the operator is not monotone in the product lattice, and the least fixpoint is not defined. The reference-interpreter check (naive against semi-naive) cannot detect this, because both strategies use the same rounds. The answer changes only under a different schedule, and the planner is free to choose a schedule.
  • Suggestion: add one clause to the core row and to the Recursion section: an expression that reads a min column feeds only a min column, and an expression that reads a max column feeds only a max column (for min_by and max_by, the same applies to the ordering argument). Everything else is T70. Add the example above to the T70 cases.

B3. The empty line that replaces the W2 row splits the programme table, so the last three rows are not a table.

  • At gq-3.md:1824 the W2 row is now an empty line. In Markdown an empty line ends a table. The rows **I**, **Poly**, **A** access and the schema-namespaces row (gq-3.md:1825-1827) then render as one paragraph of text with pipe characters. I checked this with markdown-it, which renders the **I**, **Poly** row inside a paragraph. check-docs.py passes, so the docs check does not find this.
  • Suggestion: delete the line instead of emptying it.

Still open from the review on 1f5aef7

The delta does not touch the Path design, so B1 to B3 of that review still apply as written: the spelling knows*{1,3} and knows* (gq-3.md:1899-1935, :1968), the round budget of shortest, and the change in endpoint semantics when a path binder is added.

Optional

  • O1. Multiplication is listed in one operand order only. The core admits $d + t and t + $d, but only $d * c. With the rule as written, 2 * $d is T70 and $d * 2 is admitted. Either admit c * $d too, or say that the formatter writes the constant on the right.
  • O2. The interpreter evidence is older than the new rule. gq-3.md:643 still says that the interpreter agrees on "every one of 2,801 random recursions these rules admit". The rules changed in this commit. If the generator made order-reversing steps such as -$d, some of the 2,801 are now refused and the number is wrong. If it did not make them, the new restriction has no interpreter evidence. Rerun the generator under the new rule and state the expression space it covers. Include steps that read one lattice column and write another (see B2).
  • O3. Cosine at a zero vector is a behaviour change, and the index path needs an explicit filter. The paragraph at gq-3.md:802 refuses zero vectors on write and drops stored ones from distance and nearest. Both are changes for existing data: today a load can store a zero vector in a cosine field. Only the embedding pipeline refuses one (validate_and_normalize_embedding, crates/omnigraph/src/embedding.rs:589-606). Neither change is in the Compatibility bullets. Pinned Lance 13.0.0 computes 1 - xy / (x_norm * y_norm) (lance-linalg cosine.rs:1316-1321), so a zero vector gives NaN in the index path. The rule "drops its row … so the scan and the index agree" therefore needs an engine filter on both paths, and nearest must not let NaN rows take window slots. Name the owner of this filter in the stage B row.
  • O4. gq-3.md:807-808 has two empty lines after the cosine paragraph. This has no effect on the output.

Liability

The delta adds no code. It removes liability in three places: one stated degree rule instead of an informal one, one lazy-evaluation rule for the two conditional rows, and one precise acceptance test for the certified BM25 fetch. It adds two obligations: a new validation on cosine writes, and a filter for stored zero vectors on two read paths. If the B1 choice stays, it adds a third: a semantic break that no rewriter or diagnostic can show to an agent. The B2 gap is a hole in the one rule that makes Fix well defined. After five more lattice features of this sort (for example min_by, max_by, or new lattice aggregates), each one needs the same direction clause, so one rule in the core is cheaper than a rule per aggregate.

Validation

  • Exact head 9853010, previous reviewed head 1f5aef7, merge base ea4a16d with main. Reviewed in an isolated worktree.
  • python3 scripts/check-docs.py at 9853010, with the pinned doc packages: "Documentation OK (473 Markdown files checked)". typos was not run because it is not installed on this host.
  • B3: the markdown-it parser from the pinned doc packages puts the **I**, **Poly** row in a paragraph, not in a table.
  • B1: checked against crates/omnigraph/tests/integration/traversal_indexed.rs:331-383 on this head. I did not run that test.
  • B2: a short script ran the example above under naive rounds, semi-naive rounds and a priority order. The results were c.e = 11, 11 and 3. The script models the stated Fix rule. It is not the RFC's reference interpreter.
  • O3: checked against lance-linalg 13.0.0 (the workspace pins lance = "=13.0.0") and crates/omnigraph/src/embedding.rs on this head.
  • CI on the exact head when I posted this: 25 check runs succeeded, 4 were skipped, and 2 were still running.
  • GQT regression: not applicable, because the change is RFC text with no behaviour. If B1 keeps the new choice, the stage that implements it must change variable_hops_handle_self_loop and add a GQT case for {1,1} against {1,2} with a start-node self-loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant