Skip to content

rfc: amend the shared expression model with the operator vocabulary - #805

Draft
ragnorc wants to merge 3 commits into
ModernRelay:mainfrom
ragnorc:rfc/gq-operator-vocabulary
Draft

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

Conversation

@ragnorc

@ragnorc ragnorc commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

What this is

An amendment to the shared expression model RFC (docs/rfcs/2026-09-24-shared-expression-model.md). It states one rule for GQ's operators, so the language stays consistent as it grows, and lists the changes the rule implies. It changes no code.

Why

Issue #801 asked for set membership, and #803 added in. Looking at the operators as a whole turned up two problems next to it:

  • contains and starts_with are the only infix words that are not reserved. So $ms contains $m.number parses as a traversal over an edge named contains and fails with expected clause.
  • contains does two different things depending on its left operand's type: membership for a list, substring for a String. A parameter declared String instead of [String] silently gets substring matching. With $ms: String bound to "M12", ($ms contains $m.number) returns the matter M1. in does not have this problem, because it refuses a String on the right.

The rule

  • Symbols compare two values: = != < <= > >=.
  • Reserved words are the logic of the language: and, or, not, is null, in, and the pattern blocks. This set is closed.
  • Everything else is a function call, with the value being tested first and options as name: value.
  • One spelling means one operation: a type decides whether a query is valid, never which operation runs.

What changes

Phase 4 is #803 (in), recorded as shipped. The amendment adds:

  • Phase 5: starts_with(s, p) and contains(s, part) become calls. The infix forms keep working for one release, with a warning in omnigraph lint, queries validate and cluster plan. L contains x on a list rewrites to x in L. List literals accept parameters (x in [$a, $b]). Call options are name: value, which RFC 0048 (rrf(…, k: 60)) and the analyzed lexical search RFC (terms(…)) use.
  • Phase 6: the release after, the infix forms are removed.
  • T38 (search calls only as top-level conditions) is recorded as the one place a true/false expression can't go, ending once the lexical RFC's exact scan exists.
  • Language version: feat(gq): add list membership with scan pushdown #803 reserved in without a bump, which is a major under the compatibility rule. Phase 5 moves GQ_LANGUAGE_VERSION to (3, 0) and phase 6 to (4, 0).

Review notes

The edit follows the shape of the #795 amendment: a new section, rollout phases 4 to 6, four rows under Alternatives, and a dated decision-log entry that quotes every superseded sentence. The main alternative, keeping contains and starts_with infix and reserving them, is recorded under Alternatives with why it loses. About 57 lines in 19 .gqt case files and about ten lines of docs use the infix forms today.

Related: #801, #803, #792, #793.

Checks: python3 scripts/check-docs.py, bash scripts/check-agents-md.sh, typos.

@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.

Recommendation: request changes for the membership contract error in the inline comment. This is a documentation defect, not a new runtime defect. Reviewed head: 49f436ce618be5024940a49130f532f0e56eddb9. I use COMMENT because this account is also the PR author.

This PR proposes one spelling for each kind of query operation. Today, contains can mean list membership or substring search. A wrong parameter type can therefore produce a valid query with the wrong meaning. The proposal uses x in list for membership and contains(text, part) for substrings. It also moves starts_with to a call, permits parameters inside list literals, and defines named options. Old infix spellings would warn for one release, then fail to parse. This PR changes only the RFC. Those compiler and migration changes remain future work.

Correctness and workload

The relevant contract is the rows a read selects and the rows a mutation changes. People and agents use both ad hoc queries and stored queries. Empty parameter lists and nullable fields are normal inputs. The RFC must describe them exactly. not (x in []) selects every non-null x, contrary to the new sentence. The existing case also updates three rows through this predicate. Keep that behavior and correct the text.

The basic design addresses causes. It removes the type-dependent choice between two meanings, and call syntax avoids the traversal ambiguity. The existing compiler already lowers in to the same membership operation as list contains. Both proposed string calls can reuse existing execution operations. See compiler lowering and scan lowering.

Tradeoffs and liability

  • One explicit membership spelling reduces ambiguity. The cost is rewriting queries, documentation, and client prompts. The warning window adds temporary syntax and diagnostic obligations.
  • Reusing typed operations preserves the current scan and index paths. I checked Lance 11.0.0's expression entry points and scalar-index parsing. This proposal needs no new storage state or cloud-specific path. I measured no performance gain.
  • Stored source makes removal operationally significant. At this head, a stored-query parse failure quarantines its graph during startup. The promised warnings in lint, queries validate, and cluster plan must precede removal. See startup handling.

Long-term liability should fall if the implementation normalizes spellings into the existing operations and removes the compatibility rule on schedule. Five similar additions should share call parsing and type checks, without five separate compatibility mechanisms. The optional inline comment qualifies the claim that new calls need no grammar. Also account for the existing fuzzy(field, query, max_edits) option when defining the named-option migration. Its parser already accepts a third argument.

The net addition is 167 RFC lines. It adds a specification to maintain, but no runtime abstraction. Line count alone does not establish reduced liability.

Validation

  • Local: all 409 compiler tests passed. Documentation links, AGENTS links, and the changed RFC's spelling check passed.
  • Local: both cited membership GQT cases passed, including indexed columns, writes, and restart. The issue 801 case confirms the negated empty-list read and the three-row mutation. No source or test edits were needed.
  • Exact-head CI: GQ Logic Tests passed. The workspace test job was skipped. I did not rerun the full workspace or cloud suites.
  • I inspected callers, tests, the migration surfaces, relevant upstream documentation, and the pinned Lance implementation. The future string-call syntax, warnings, and removal cannot receive implementation validation in this documentation-only PR.

list on the right, a literal or a parameter list is pushed into the scan as
DataFusion's structured `InList` (`engine/scan.rs:942`), a key or unique
property gets a bounded row estimate, a null `x` reads null, and an empty
`S` matches nothing, under `not` too. The user guide states it

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.

[P2] State the result of negating empty membership correctly

An empty list makes x in [] false for a non-null x. Applying not makes it true. The existing empty_set_under_not step returns m1 and m2. The later recode_outside_the_empty_set mutation changes three rows. Both are in the case cited here. Scan lowering also preserves null for a null needle and false for any other needle before negation. Please state these cases separately. As written, the contract says a negated empty-list mutation selects nothing, when it can select every non-null row. Correct the sentence without changing the existing behavior.

1. **The reserved set is closed.** It holds what applies to values of every
type: truth values, null, membership and quantification. An operation
joins it only by amending this RFC; every other operation is a call, so
a new capability adds a function to the type checker and no grammar.

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.

Optional: qualify the claim that a call needs no grammar

Calls avoid reserving another word, but the current parser has a separate grammar rule for each call. The phase 5 table also adds separate starts_with and contains call rules and changes operand. Thus, new calls still require grammar changes under the stated rollout. Either specify a shared call production, or narrow this claim to avoiding new reserved words and traversal ambiguity. This distinction matters to the liability argument: five more calls should not require five more special parsing paths by accident.

@ragnorc

ragnorc commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Brought up to date with main (merge cdc1dff6). The conflict was in this RFC. Its frontmatter updated and the decision log now keep main's 2026-10-02 Blob-clear amendment after this PR's 2026-09-29 entry.

Review of 49f436ce:

  • [P2] Negating empty membership (inline, Operators and calls). Fixed in b1f35289. The sentence now states each case: an empty S makes x in S false for a non-null x and null for a null x, because the scan lowers it to x IS NULL AND NULL (engine/scan.rs:1372 at ea4a16d5). So not (x in []) selects every row whose x is not null, in a read and in a mutation where. A null list parameter reads null. This matches empty_set_under_not (m1, m2) and recode_outside_the_empty_set (three rows), and changes no behavior.
  • Optional: "a call needs no grammar" (inline, rule 1). Narrowed in b1f35289. A call reserves no word and is never ambiguous, but it still needs a parse rule. Today there is one rule per call, and phase 5 adds two. I declined the shared call production: it would change how every existing call parses (now_call, search_call, the ranking calls), which is outside this amendment. The text says so, so a later RFC can take it. The Alternatives row is narrowed the same way.

The decision log quotes each superseded sentence.

@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.

Recommendation: ready. I found no blocking defect. Reviewed head: b1f3528982f11d8e6c674265bc280046addc9a87. I use COMMENT because this account is also the PR author.

This review covers the change since the review of 49f436ce: the merge of main (cdc1dff6) and the fix commit b1f35289. The earlier review still applies to the rest of the amendment. I do not repeat it here.

Why, what, how, impact

  • Why. The earlier text said that an empty list "matches nothing, under not too". That was false. not (x in []) selects every row whose x is not null, in a read and in a mutation where. Someone who implements phase 5 from the RFC could build the wrong behavior.
  • What. The commit states each case separately: a non-null x, a null x, a negated empty list, and a null list parameter. It also narrows the claim that "a call needs no grammar". A call reserves no word, but it still needs a parse rule.
  • How. It changes only RFC text: 28 lines added and 7 removed. The decision log quotes each sentence it replaces.
  • Impact. No runtime change. The text now matches the behavior that the code and the cited GQT cases have.

Correctness

The new text agrees with both evaluation paths at this head:

  • Scan pushdown: an empty constant list becomes needle IS NULL AND NULL, which is false for a non-null x and null for a null x. A null list becomes a null literal (scan.rs:1356-1373).
  • Fallback evaluation, used when the list does not push down: a null list or a null needle gives null, and an empty list gives false (expr.rs:329-361).

The two paths agree, so the RFC can state one rule. The section anchors (query.pest:187, engine/scan.rs:942, docs/user/queries/index.md:166) still resolve correctly at the pinned baf10c94. The one new anchor names its own commit (ea4a16d5), which is correct.

Narrowing the call claim makes the liability argument honest. Each new call costs one parse rule today. The text says this, and it leaves a shared call production to a later RFC instead of adding one here.

Optional findings

  1. The owning GQT case still states the old sentence. The header notes of issue_801_in_predicate_over_a_list.gqt say "an empty set matches nothing, under not too". The steps in that same file assert the opposite (empty_set_under_not returns m1 and m2; recode_outside_the_empty_set changes three rows). The RFC names this case as the owner of the behavior, so its notes now disagree with both the RFC and its own steps. I suggest a one-line correction to the note in this PR, so that no copy of the old claim remains. A change to a .gqt file makes CI run the GQT jobs, which costs little here.
  2. The phase 5 version step starts from an old value. The table gives phase 5 as (2, 0) to (3, 0). main is now at (2, 3) (lib.rs:48), because #827, #942 and #1016 each added a minor after this amendment was written. The target (3, 0) does not change. Write "from the current 2.x" or (2, 3), so the table does not go out of date again. See the inline comment.
  3. No GQT step owns the null list parameter. The sentence "A null list parameter reads null for every x" is correct, but neither cited case checks it. I added a temporary step (below), and it passed. If you want the RFC's claim "these cases own it" to cover every sentence, add that step to the issue 801 case. If not, say that the code owns this one case.

Context from other open PRs

#1302 (the GQ 3 RFC) builds on this amendment. Its function registry, with one generic name(args, opt: v) rule, is the shared call production that this text now leaves to a later RFC. Its editions proposal would replace the (3, 0) then (4, 0) steps over two releases. #1302 also quotes the sentence that this commit replaces ("a new capability adds a function to the type checker and no grammar"). After this PR merges, #1302 should quote the new text. Nothing here conflicts with #1302. This is context only, not a new requirement for this PR. #1244 does not touch these operators.

Validation

All local results are at b1f35289, in a clean worktree:

  • cargo test -p omnigraph-gqt -p omnigraph-gqt-core --locked --test gq_logic_tests, with filters issue_801_in_predicate_over_a_list and list_membership_over_indexed_columns_across_writes_and_restart: both passed.
  • Temporary case issue_801_zz_tmp_review_null_list.gqt (same schema and seed as the issue 801 case), with $cs: [I64]? bound to null: $m.code in $cs returned 0 rows, not ($m.code in $cs) returned 0 rows, and update Matter set { code: 7 } where not (code in $cs) changed 0 nodes. Control: when I expected one row in the first step, the case failed with "expected 1 rows, got 0", so an empty expectation is a real assertion. I deleted the temporary case. I made no other edits, and I pushed nothing.
  • python3 scripts/check-docs.py: "Documentation OK (469 Markdown files checked)". bash scripts/check-agents-md.sh: OK.
  • Exact-head CI: all checks pass. The GQT jobs classified this PR as docs-only and ran no cases ("No GQT input changed"). So CI does not show the cited cases at this head. The local runs above do.

Limitations: typos is not installed here, so I did not run it. Phase 5 and phase 6 are future work, so I could not test them in this PR. I made no performance claims.

| 2 | rewritten: `expr`, `clause`, `filter`, `mutation_predicate`, `mutation_assignment`, `prop_match`, `projection`; added: `or_expr`, `and_expr`, `not_expr`, `comparison`, `null_test`, `operand`, `kw_and`, `kw_or`, `kw_not`, `kw_is`, `kw_null`, `reserved`, `reserved_property`, `reserved_alias`, `expr_ident`; kept unreferenced and deprecated, text unchanged: `match_value`, `text_search_clause` (deleted in the release after this one); `ident`, `comp_op`, `filter_op`, `ordering` untouched | `(1, 0)` to `(2, 0)` | reserving five words narrows what an operand and a return alias accept |
| 3 | none; `query.pest` is untouched | none | the refusal moves from the engine to the type checker; no rule hash moves |
| 4 (PR [803](https://github.com/ModernRelay/omnigraph/pull/803)) | added: `kw_in`; rewritten: `comparison`, `reserved`, `traversal`, `undirected_edge` | none taken; `(2, 0)` stayed | reserving `in` narrows what an operand and a return alias accept, a major under the rule above; phase 5 records it |
| 5 | added: `legacy_text_op`, the `starts_with` and `contains` call rules; rewritten: `filter_op`, `operand`, `list_lit`; deprecated: `legacy_text_op` | `(2, 0)` to `(3, 0)` | phase 4's reservation; phase 5 itself only widens |

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.

Optional: this row starts phase 5 from (2, 0), but main is now at (2, 3) (crates/omnigraph-compiler/src/lib.rs:48). #827 took (2, 1), #942 took (2, 2) and #1016 took (2, 3) after this amendment was written. The target (3, 0) is still correct. Write "current 2.x to (3, 0)" or (2, 3) to (3, 0), so that the table does not go out of date again when another minor lands before phase 5.

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