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