Repository navigation
Conversation
ragnorc
left a comment
There was a problem hiding this comment.
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, andcluster planmust 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 |
There was a problem hiding this comment.
[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. |
There was a problem hiding this comment.
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.
# Conflicts: # docs/rfcs/2026-09-24-shared-expression-model.md
|
Brought up to date with Review of
The decision log quotes each superseded sentence. |
ragnorc
left a comment
There was a problem hiding this comment.
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
nottoo". That was false.not (x in [])selects every row whosexis not null, in a read and in a mutationwhere. 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 nullx, 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-nullxand null for a nullx. 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
- The owning GQT case still states the old sentence. The header notes of
issue_801_in_predicate_over_a_list.gqtsay "an empty set matches nothing, undernottoo". The steps in that same file assert the opposite (empty_set_under_notreturnsm1andm2;recode_outside_the_empty_setchanges 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.gqtfile makes CI run the GQT jobs, which costs little here. - The phase 5 version step starts from an old value. The table gives phase 5 as
(2, 0)to(3, 0).mainis 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 current2.x" or(2, 3), so the table does not go out of date again. See the inline comment. - 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 filtersissue_801_in_predicate_over_a_listandlist_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 tonull:$m.code in $csreturned 0 rows,not ($m.code in $cs)returned 0 rows, andupdate 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 | |
There was a problem hiding this comment.
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.
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:containsandstarts_withare the only infix words that are not reserved. So$ms contains $m.numberparses as a traversal over an edge namedcontainsand fails withexpected clause.containsdoes two different things depending on its left operand's type: membership for a list, substring for a String. A parameter declaredStringinstead of[String]silently gets substring matching. With$ms: Stringbound to"M12",($ms contains $m.number)returns the matterM1.indoes not have this problem, because it refuses a String on the right.The rule
= != < <= > >=.and,or,not,is null,in, and the pattern blocks. This set is closed.name: value.What changes
Phase 4 is #803 (
in), recorded as shipped. The amendment adds:starts_with(s, p)andcontains(s, part)become calls. The infix forms keep working for one release, with a warning inomnigraph lint,queries validateandcluster plan.L contains xon a list rewrites tox in L. List literals accept parameters (x in [$a, $b]). Call options arename: value, which RFC 0048 (rrf(…, k: 60)) and the analyzed lexical search RFC (terms(…)) use.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.inwithout a bump, which is a major under the compatibility rule. Phase 5 movesGQ_LANGUAGE_VERSIONto(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
containsandstarts_withinfix and reserving them, is recorded under Alternatives with why it loses. About 57 lines in 19.gqtcase 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.