Repository navigation
fix(sql): make Parser.apply total, and fix N-ary relation predicates (#250) - #282
Merged
Merged
Conversation
0.22.0 is released (tag v0.22.0). Epic 21 rides 0.23.0-SNAPSHOT; this bump is the epic's first dev commit and, story 21.4 landing first, it opens this branch. Story 21.4: Parser.apply is total - no exception escapes the Either Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Story 21.4: `Parser.apply` is total — no exception escapes the `Either` Refs #250 `Parser.apply` is typed `Either[ParserError, Statement]` and did not honour it. Five production call sites match on that Either with no `try` of their own (SQLImplicits.queryToStatement, IndicesApi x3, the searchAs macro, where a throw is a raw scalac crash), and one sibling-repo consumer — arrow's Flight JOIN planner — matches it too. The measured repro is jOOQ's rendering of RLIKE, `(name like_regex 'Jo.*')`, the only crash row of the Epic 19 census. Two halves, covering disjoint surfaces: * `err` owns the grammar. All five `throw` sites under `parser/` are gone — the issue named three; the audit found five. The three `WhereParser` helpers return `Either[String, …]` (both `@tailrec` preserved) and their four combinator callers — `where`, `having`, `on`, `case_condition` — emit `err(reason)` from a `>>` continuation; `FromParser.on` and `identifierWithArithmeticExpression` convert in place. `err`, never `failure`: a `failure` falls through `opt(where)` and silently drops the clause, which is the #213 data-loss mode. `case_condition` also gains the default arm it never had (a MatchError escapes exactly like a throw and no source scan can see it). * A `NonFatal` boundary catch owns what `err` structurally cannot reach. `single` calls `.update()` inside its own action, so the whole AST-update pass runs during the parse, and two sites there crash — `bucketNames` indexes `select.fields(n - 1)` unchecked (a bare index, no `throw` token), `Bucket.update` throws on a non-positive ordinal. The catch wraps the WHOLE method body: `normalize` dereferences `query.length`, so `Parser(null)` throws before `parse` ever runs. Its message is labelled `Internal parser error: <Class>[: <detail>]` so an internal fault stays distinguishable from a syntax error, and both accessors it reads are themselves guarded — `getSimpleName` can raise on JDK 8 and a custom `getMessage` can throw, either of which would escape the very method whose point is totality. Three shapes that used to be ACCEPTED are now rejected, each of which previously parsed into a statement that was not the one written: * a dangling `AND`/`OR` left `Where(None)`, which renders as no clause at all — `DELETE FROM orders WHERE id = 1 AND` parsed as `DELETE FROM orders` and emptied the index; * a stray unmatched `)` was silently discarded; * a `NESTED`/`CHILD`/`PARENT` predicate with three or more criteria had its relation scope silently collapsed to the first criterion, the rest escaping onto the parent document. Found by review, verified in both directions. Two silent-truncation arms of `processTokensHelper` are converted for the same reason, after instrumenting both and confirming no input in any suite reaches them with data to lose. `ParserError` gains `cause: Option[Throwable] = None`, set only by the boundary catch, threaded through all five `core` parse sites — it is the only stack trace an internal parser fault ever produces. The two `ElasticResult.attempt(Parser(...))` wrappers and their now-unreachable `ElasticFailure` arms are removed (`attempt` catches exactly `NonFatal`, and so does `apply`), together with `GatewayApi.parseFailureReason`; inputs that used to throw now get an honest `400` instead of no status at all. `PipelineApi`'s reason is bounded by the same `excerpt` the SQL route uses. `Parser.apply` no longer prints the combinator message to stderr. Guards: a new `ParserTotalitySpec` asserts not-throwing as its own assertion first, then that the rejection came from the grammar and NOT from the boundary catch — without that third assertion every case is unfalsifiable, since restoring a throw still yields a Left containing the same reason. Verified by restoring one throw and observing seven tests go red. Plus a `parser/**` source scan with no allow-list, self-guarded against scanning nothing, and a token-prefix sweep over ten seeds. sql 598, core 857, bridge 120, macros-tests 19 — green on 2.13.16 and 2.12.20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e relation Story 21.4: `Parser.apply` is total — no exception escapes the `Either` Refs #250 Follow-up to 7491e9e, on the lead's ruling: fix the grammar properly rather than accept the break that commit introduced. Root cause. `nestedPredicate` / `childPredicate` / `parentPredicate` took a strictly BINARY `predicate` (`criteria ~ (and|or) ~ not.? ~ criteria`), so a relation predicate with three or more criteria could not match and fell through to `X.regex ~ start.? ~ criteria ~ end.?` — whose `start.?` swallowed the `(` while its `end.?` never fired. That was not a syntax error: it PARSED, with the relation scope collapsed to the FIRST criterion and the rest escaping onto the PARENT document. Measured: WHERE id = 1 AND child(a = 2 AND b = 3 AND c = 4) parsed as WHERE id = 1 AND CHILD(a = 2) AND b = 3 AND c = 4 and the emitted query put `b` and `c` in the OUTER bool, matched against the wrong documents entirely. With `OR` the operator itself spanned the relation boundary, so a parent matched if either it had a matching child or its own field matched. This is the #205/#253 silent-wrong-answer family. Fix at the arity's source. The parenthesised form now takes N criteria: `relationTokens` collects the same alternatives `whereCriteria` offers, minus a bare `end` (the closing paren belongs to the relation predicate — `rep1` does not backtrack, so leaving it in would swallow it and everything after it), with `relationGroup` matching a `(` together with its OWN `)` and re-emitting both so sub-groups keep working. The tokens are reduced by `processTokens` — the same function `where`, `having`, `on` and `case_condition` use — so `NESTED(X)` now produces exactly the criteria tree `WHERE X` produces and associativity, grouping and NOT handling cannot drift. The `start.?` / `end.?` on the criteria forms are GONE, which closes the twin hole the previous commit could only pin as a KNOWN GAP: an opening paren no longer commits to a form that never requires the closing one, so `WHERE child(a = 1 AND b = 2` is rejected instead of parsing as `CHILD(a = 1) AND b = 2`. Those forms now serve only the paren-less spelling (`NESTED a = 1`). Proof is the GENERATED ES QUERY, not the parse — a statement that parses and still scopes wrongly is worse than the loud failure it replaced. Four new assertions pin the emitted JSON: all N terms inside the single `has_child` / `has_parent` query with nothing leaking to the outer bool, for AND, for OR, and for the mixed case where a genuine parent-level criterion must STAY outside. They are added to BOTH `bridge/src/test` (the template copied into es7/8/9) and the hand-maintained `es6/bridge/src/test`. Plus parser-level round-trip tests: every N-ary render re-parses to an equal AST. The OQ-5 and OQ-6 rejections are unchanged, and the one- and two-criteria forms — the regression boundary — still parse and still scope correctly. sql 599, core 857, macros-tests 19, bridge template 124, es6 124, es7 124, es8 124, es9 124 (that one under sbt17: elastic4s for es9 is Java-17 bytecode). sql and core green on 2.13.16 and 2.12.20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… parse Story 21.4: `Parser.apply` is total — no exception escapes the `Either` Refs #250 Delta-review round on 5c64276. Three legs; the two with project access both found the same HIGH independently. The HIGH: `NESTED(...)` WITHOUT a `JOIN UNNEST(<path>) AS <alias>` emits `{"match_all":{}}` — the criteria are discarded and every document matches. That is pre-existing at arity 1, 2 and 3 alike, but arity >= 3 used to be REJECTED, so the N-ary work made it newly reachable — and the acceptance row I had written asserted only `isRight`, which CERTIFIED it. Fixed three ways: the query-level NESTED assertion uses the declared-path spelling (the one that can be wrong in an interesting way, and which is correct — all three terms inside `nested.query` with the right `path` and `inner_hits`); the parse rows are re-pointed to that spelling; and the undeclared one is pinned under a test whose name says it is a known pre-existing emission defect, to be deleted when fixed rather than retargeted. Second certification removed: the acceptance rows used unqualified names, but `has_child.type` / `has_parent.parent_type` are derived from the FIRST criterion's leading name segment, so `child(a = 1 AND b = 2)` emits `"type": "a"` — a join type that does not exist, which ES answers with zero hits rather than an error. Every relation fixture and row is now dotted. The central claim is now stated executably. Two single-operator JSON pins could not prove "CHILD(X) contains exactly the tree WHERE X produces" — all-AND and all-OR are association-insensitive, so they show that nesting exists, not that it is the right nesting. A tree-equivalence property asserts structural equality of the two criteria trees across ten shapes, including `a AND b OR c`, `a OR b AND c`, `(a OR b) AND c` and both NOT positions. It also guards the hand-copied alternation: if `whereCriteria` gains a shape `relationTokens` cannot reach, the trees stop matching and it fails. Also added: an OR-plus-parent-criterion pin (the mirror-image bug — every other assertion proves nothing leaks OUT of the relation, that one proves nothing is wrongly pulled IN); degenerate relation bodies (`child(AND)`, `child(a = 1 AND)`, `child(OR OR)`, `child(a = 1 b = 2)`) with the full no-throw / isLeft / not-internal contract, which also exercise the `Right(None)` arm and prove its `relation` parameter is load-bearing; an empty-relation case of its own; and a control proving a column literally named `child` still parses, since the `err` is non-backtracking. The comment that claimed "ONE deliberate omission" from `whereCriteria` now enumerates all three (bare `end`, bare `start`, `then_case`) — a correctness argument that rests on a difference has to state the difference exactly. Measured and recorded, not fixed here (each with a local issue record): NESTED without a declared path; the relation type guessed from a name prefix; and `NOT c = 3` rendering as `c NOT = 3`, which does not re-parse — pre-existing and identical at top level on main, verified eight ways across both trees, newly reachable at arity >= 3. Parse depth: the relation path is an order of magnitude CHEAPER than the equivalent top-level nesting (N=100 in 2.4 s vs an OutOfMemoryError after 408 s), so it is not a new amplifier, but nothing bounds depth anywhere; no catch added, since AD-1 deliberately lets VirtualMachineError escape. sql 604, core 857, macros-tests 19; all five bridge suites 126 (template, es6 hand-maintained, es7, es8, es9 — the last under sbt17). sql and core green on 2.13.16 and 2.12.20. Every bridge suite needed a SECOND run: the first after a template edit reports the stale count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fupelaqu
marked this pull request as ready for review
September 5, 2026 08:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #250
Closes #280
Closes #281
Epic 21 story 21.4. This branch also carries the epic's opening commit —
build.sbt0.22.0→0.23.0-SNAPSHOT(9dca152b, nothing else in it).1.
Parser.applyis total (#250)It is typed
Either[ParserError, Statement]and did not honour it. Five production call sitesmatch on that
Eitherwith notryof their own —SQLImplicits.queryToStatement(an implicitconversion),
IndicesApi×3 (update/delete/insert by query) and thesearchAsmacro (where a throwis a raw scalac crash) — plus one sibling-repo consumer, arrow's Flight JOIN planner. The measured
repro is jOOQ's rendering of
RLIKEas(name like_regex 'Jo.*'): the onlycrashrow of the259-row Epic 19 census probe.
Two halves, covering disjoint surfaces — not two nets over one hole.
errowns the grammar. All fivethrowsites underparser/are gone (#250 named three;the audit found five). The three
WhereParserhelpers returnEither[String, …]with both@tailrecpreserved, and their four combinator callers —where,having,FromParser.on,case_condition— emiterr(reason).err, neverfailure: afailurethere falls throughopt(where)and silently drops the clause, the #213 data-loss mode.case_conditionalso gains thedefault arm it never had (a
MatchErrorescapes exactly like a throw, and no source scan can seeit).
processTokensis narrowed toprivate[parser].A
NonFatalboundary catch owns whaterrstructurally cannot reach.singlecalls.update()inside its own action, so the AST-update pass runs during the parse, and two sites there crash —
SingleSearch.bucketNamesindexesselect.fields(n - 1)unchecked (a bare index, nothrowtoken, so no grep-based guard can ever see it) and
Bucket.updatethrows on a non-positiveordinal. The
trywraps the whole method body:normalizedereferencesquery.length, soParser(null)throws beforeparseruns. The message is labelledInternal parser error: <Class>[: <detail>], and both accessors it reads are themselves guarded —getSimpleNamecan raiseInternalErroron JDK 8 and a customgetMessagecan throw, either ofwhich would escape the very method whose point is totality.
Nothing that parses today is lost to the
errconversions: every input that reaches one used tothrow, which aborted the whole parse.
ParserErrorgainscause: Option[Throwable] = None, set only by the boundary catch and threadedthrough all five
coreparse sites — it is the only stack trace an internal parser fault everproduces. Both
ElasticResult.attempt(Parser(...))wrappers and their now-unreachableElasticFailurearms are removed (attemptcatches exactlyNonFatal, and so doesapply), alongwith
GatewayApi.parseFailureReasonand its four tests. Inputs that used to throw now get an honest400instead of no status at all.PipelineApi's reason is bounded by the sameexcerptthe SQLroute uses.
Parser.applyno longer prints the combinator message to stderr.2.⚠️ Two shapes that used to be accepted are now rejected (#280)
Each previously parsed into a statement that was not the one written:
DELETE FROM orders WHERE id = 1 ANDDELETE FROM orders— empties the indexWHERE clause requires criteriaSELECT a FROM t WHERE a = 1)… WHERE a = 1,)discardedUnbalanced parenthesesTwo silent-truncation arms of
processTokensHelperare converted for the same reason, afterinstrumenting both and confirming across sql/core/bridge/macros that nothing reaches them with
data to lose (the catch-all: never reached; the
Nilfallback: 26 times, always a danglingoperator).
3. A silent wrong answer is FIXED: N-ary relation predicates (#281)
NESTED/CHILD/PARENTaccepted exactly two criteria. With three or more the statementstill parsed, but the relation scope collapsed to the first criterion and the rest escaped onto
the parent document; with
ORthe operator spanned the relation boundary. Fixed at the arity'ssource and reduced through
processTokens, so a relation body is exactly the tree the sameexpression produces at top level. An unmatched opening parenthesis is now rejected too.
This was found by the code review, not by the implementation, and it falsified an invariant written
into a source comment —
extractSubTokensis not the only route to an unmatched).Verification
sql604 ·core857 ·macros-tests19 · all five bridge suites 126 (thebridge/template,the hand-maintained
es6/bridge/, es7, es8, and es9 undersbt17).sqlandcoregreen on2.13.16 and 2.12.20 (
+ sql/test,+ core/test—+ compiledoes not reach test sources).scalafmtCheck+test:scalafmtCheckclean. No Docker.and the real one only on the second, logging
Copying bridge template sources. Never accept a firstrun after touching
bridge/src/test.Guards that can actually fail.
ParserTotalitySpecasserts not-throwing as its own assertionfirst, then that the rejection came from the grammar and not from the boundary catch. That third
assertion is the point: without it every case is unfalsifiable, because restoring a
throwstillyields a
Leftwhose message contains the same reason. Verified by restoring one throw andobserving 7 tests go red. Plus a
parser/**source scan with no allow-list, self-guardedagainst scanning nothing (asserted, never
assumed); a tree-equivalence property over ten shapes(single, all-
AND, all-OR, mixedAND/ORboth ways, explicit grouping,NOTforms) provingCHILD(X)≡WHERE X; and generated-ES-query assertions in both bridge copies — all-ANDandall-
ORpins alone are association-insensitive and prove only that nesting exists, not that it isright.
Parse-cost check (AD-11), measured and honestly inconclusive:
ParserSpec(304 tests), twosessions of 10 runs per tree, warm-up discarded — baseline median 3.50 s (2.90–4.40), patched
3.75 s (2.89–4.61). The +7 % sits inside the noise band and a shorter round showed the opposite
ordering. No measurable regression at this resolution — deliberately not rounded up to a "zero
cost" claim.
Known, measured, tracked separately
NESTED(…)without aJOIN UNNEST(<path>) AS <alias>emitsmatch_all— pre-existing, at everyarity; this PR only makes the 3-or-more spelling reach it.
NOT c = 3renders asc NOT = 3, which does not re-parse — pre-existing, identical at top level.GROUP BYon a digit-bearing column (city2,q4) is reported as400 Internal parser errorinstead of crashing. Not newly broken; semantics owned by story 21.3,where three pinned sites carry the instruction retarget, never delete.
Release notes (0.23.0)
See the story's release-note block. Headline items: the two newly-rejected shapes; the N-ary
relation fix (if you have such a query, its results were wrong and will now change);
statusCodeNone → Some(400)on the previously-throwing route;Internal parser error: …as adistinguishable label carrying the cause; the parser no longer writing to stderr; and two
binary-incompatible
sqlAPI changes —ParserErrorarity (soequals/hashCode/toStringchange) and
processTokensnarrowed toprivate[parser]with a new return type.🤖 Generated with Claude Code