Accept the BI dialect spellings — CHAR_LENGTH, TIMESTAMPADD, TOP n (72/99) (+ a watcher operator-ordering fix) - #360
Merged
Merged
Conversation
…D, TOP n Four spellings a BI tool emits that the grammar rejected, each sourced from a `residual` row of the Epic 19 capture with a bisected cause. Every one is an ALIAS or a re-spelling, never a second mechanism: the render normalises to the canonical form and that render re-parses. - CHAR_LENGTH / CHARACTER_LENGTH are spellings of LENGTH. Our LENGTH already counts CHARACTERS, which is exactly what CHAR_LENGTH means — MySQL's own LENGTH counts BYTES, so CHAR_LENGTH is the spelling that agrees with us. - TIMESTAMPADD is a spelling of DATETIME_ADD and TIMESTAMPDIFF one of DATE_DIFF. The ODBC `(unit, count, base)` argument order they use is the `transactSql` form this parser has always accepted, so the aliases add a name and no semantics. MySQL 8.4 defines TIMESTAMPDIFF(unit, dt1, dt2) as dt2 - dt1 and `date_diff_transact_sql` binds (unit, d1, d2) to between(d1, d2) — same answer, same sign, which is why the MySQL name is safe here. The ODBC interval names (SQL_TSI_DAY, …) become aliases of the units we have. 🔴 TIMESTAMPSUB is deliberately NOT added: it exists in neither ODBC nor MySQL (verified against the 8.4 reference), and the capture itself uses a NEGATIVE count with TIMESTAMPADD, which already worked. - SELECT TOP n (and the parenthesised TOP (n)) is folded into the statement's LIMIT by `Parser.single`, so the AST has exactly ONE owner of the row bound and the render is `LIMIT n`. Combining TOP with LIMIT is refused by name rather than resolved by a precedence rule nobody could guess. Also fixed, found while sizing the `<=` / `<=>` prefix hazard and unrelated to the BI work: `Parser.comparison_operator` ordered `gt` before `ge` and `lt` before `le`. These are string literals, not anchored tokens, so `>` matched the first character of `>=` and left `=` for the value production. `WHEN x >= 0` and `WHEN x <= 0` in CREATE WATCHER were rejected while `>`, `<`, `=` and `<>` parsed. `WhereParser.comparisonOp` always had the right order; this one did not. 🔴 Two defects an independent review caught before this was pushed, both in the TOP production, both now guarded: 1. A named "non-negative row count" refusal used `err`, and `Parsers.|` never tries another alternative after an Error — so `opt` could not backtrack and `SELECT top -1 AS x FROM t`, which parses on main as `top - 1`, became a HARD rejection. It reached every SELECT-bearing production (CTAS, derived tables, IN subqueries). The count guard is now a `failure`, which lets `field` settle the genuine ambiguity, and every case is pinned against the same statement with a different identifier as the control. The spec had PINNED the defect — that test is deleted, not adjusted. 2. `SELECT TOP 5 PERCENT a FROM t` PARSED, as the column `PERCENT` aliased to `a`, and returned rows for a column nobody named: a loud rejection turned into a silent wrong answer (#205/#253 family). `TOP n PERCENT` and `TOP n WITH TIES` are now refused by name. Also from the review: the Int overflow that defeated the count guard one line after it was written, and a TIMESTAMPDIFF assertion satisfied by the sign-INVERTED render — the very binding it existed to pin. Known gap, deliberately not closed: `SELECT DISTINCT TOP n` is rejected while the invalid `SELECT TOP n DISTINCT` is accepted, because `DISTINCT` binds to the IDENTIFIER, not to the SELECT. Closing it means giving `DISTINCT` two owners. Corpus: two rows flip `rejected` -> `parses` (tableau.sql92.wx.010 TOP 1, tableau.sql92.w8.032 CHAR_LENGTH), so PARSE moves 91 -> 93 and the series gains a row. Neither is SCORED, and that is deliberate: `scored = fixed` requires a merged suite asserting the statement's CORRECTNESS, and this PR asserts parse and render fixed-point only. Parse is not an answer (PD-3). The headline stays 70/99. The new series row's commit is `PENDING`, to be stamped at merge. No behaviour change on anything that parsed before: GrammarDiffProbe over 16,438 inputs, branch vs a parser built from origin/main — 11 widened, 0 NARROWED, 0 renders changed on an already-accepted input, 59 rejected both ways with a different message. 🔴 That probe reported 0 narrowings on the DEFECTIVE code too: its corpus never contained `SELECT top -1`. It is necessary and not sufficient. Parse cost, interleaved control/branch, 5 rounds each, first round of each side discarded (medians, µs): bare SELECT control 94.3 [91.8, 113.5] branch 98.2 [96.4, 144.7] SELECT list + LIMIT control 304.5 [289.0, 478.4] branch 300.9 [286.2, 476.4] sum of medians control 4215 [4149, 4331] branch 4205 [4026, 4396] All three ranges OVERLAP => no signal. Reported as no signal, not as a direction, though `top.?` does add one optional regex attempt per SELECT. Every mechanism was mutation-checked — the smallest edit that disables each decision alone, never a whole-branch revert: remove the CHAR_LENGTH aliases 2 red remove TIMESTAMPADD 3 red remove SQL_TSI_DAY only 2 red keep TOP parsing, drop the LIMIT fold 2 red drop the TOP+LIMIT conflict error 1 red drop the count guard 1 red restore the old watcher operator order 1 red make `top` required (kill backtracking) 12 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The parse change landed in the previous commit; this one answers the question
the scoreboard actually publishes. `scored = fixed` requires a merged suite
asserting the statement's CORRECTNESS, so until now the two rows PARSED and
stayed `residual`. Parse is not an answer (PD-3).
`BiDialectExecutionSpec` (testkit + 5 client subclasses) runs against real
Elasticsearch and is built so that accepting the SPELLING is not enough to pass:
- `CHAR_LENGTH` is asserted against a MULTI-BYTE string. `海豚` is 2 characters
and 6 UTF-8 bytes, so a byte-counting implementation — which is exactly what
MySQL's own `LENGTH` does, and the reason `CHAR_LENGTH` is the spelling that
agrees with us — answers 6 and reddens. An ASCII-only assertion would have
been satisfied by either. It is also asserted to agree with `LENGTH` on every
name, and inside the `SUM(...)` the captured statement actually uses.
- `TOP n` is asserted on row COUNT and on the row SET against the `LIMIT n`
spelling of the same statement, over a 3-shard index of 24 documents — above
Elasticsearch's default page of 10, so a dropped bound cannot hide behind the
default. A control asserts the unbounded statement returns all 24.
🔴 The integration mutation was run, not assumed: removing the fold of `TOP`
into the statement's `LIMIT` — while leaving `TOP` parsing — reddens exactly the
three `TOP` execution rows and leaves the `CHAR_LENGTH` rows green.
Scoring: `tableau.sql92.w8.032` and `tableau.sql92.wx.010` move to
`owner = issue:361`, `scored = fixed`, through a SECOND compiled singleton
(`Issue361FixedIds`) beside `Issue328FixedIds` — never a widened
`owner.startsWith("issue:")` predicate, which would hand `fixed` the meaning
"it parsed" for every future issue owner in silence. Checked in both directions,
so an id the code excepts that the table stops scoring is a loud dead exception.
SCORES 70/99 -> 72/99 · 70/75 -> 72/75; `fixed` count 58 -> 60
PARSE stays 93, and the never-counted gap returns to
"21 capability probes + 0 that answer wrongly, incompletely or UNMEASURED"
`tableau.sql92.w4.025` deliberately stays REJECTED and keeps its local owner:
its TIMESTAMPADD blocker is gone, its ODBC `{fn …}` escape blocker is not.
Two defects in the spec's own first draft, both found by running it:
`script_fields` values come back wrapped in Elasticsearch's per-field ARRAY on
the row path while the same expression under an aggregation is a scalar
(pre-existing, documented against #209 — unwrapped here rather than asserted
around); and the 24 bulk documents were seeded with a name that COLLIDED with
one of the named rows, so 25 documents matched and every count oracle was
quietly off by one. The bulk name is now `padding`, which no oracle uses.
Green on real Elasticsearch 6.8 (rest + jest), 7.17, 8.18 and 9.0 — 8/8 on each
of the five clients. `sql/test` 1348, `core/test` 1129, the 2.12 leg and the CI
lint line all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fupelaqu
marked this pull request as ready for review
September 18, 2026 13:00
fupelaqu
added a commit
that referenced
this pull request
Sep 18, 2026
…kwards
Adds the spellings that shipped with SoftClient4ES#360 in the same 0.24.0 train
this PR documents: CHAR_LENGTH / CHARACTER_LENGTH, TIMESTAMPADD, TIMESTAMPDIFF,
the ODBC SQL_TSI_ interval names, and SELECT TOP n.
🔴 And one correction that is not about the new spellings at all.
`functions_date_time.md` said DATEDIFF returns *"(date1 - date2)"* and published
EIGHT examples built on that reading, every one of them claiming a positive
result for arguments in the order later-date-first. The engine returns
`date2 - date1`. Measured on real Elasticsearch 8.18:
DATE_DIFF('2025-01-10', '2025-01-01', DAY) => -9 (the page claimed 9)
DATE_DIFF('2025-01-01', '2025-01-10', DAY) => 9
It was found because TIMESTAMPDIFF had to be added to that same block, and MySQL
defines TIMESTAMPDIFF(unit, dt1, dt2) as dt2 - dt1 — so the page and the alias
could not both be right. The merged pin for
`DATE_DIFF(birthdate, CURRENT_DATE, YEAR)` settles which: it emits
`ChronoUnit.YEARS.between(birthdate, now)`, which is what makes it an AGE rather
than a negative one. Every example in the block has now been EXECUTED and shows
its real result, and a worked example of the reversed sign is published beside
them so the rule is visible rather than implied.
Documented for TOP, in `dql_statements.md`: it is a SPELLING of LIMIT, not a
second bound; `TOP (n)` works; `TOP` is NOT reserved, so `SELECT top FROM t`
still selects a column and `SELECT top - 1 AS x FROM t` still computes one;
TOP+LIMIT is refused by name; TOP carries no OFFSET.
New entries under "Not yet supported": `TOP n PERCENT` and `TOP n WITH TIES`
(refused by name), `SELECT DISTINCT TOP n`, and the ODBC/JDBC escape sequences
`{fn …}` / `{d …}` / `{ts …}` — a tool emitting
`{fn TIMESTAMPADD(SQL_TSI_DAY, -89, CURRENT_DATE)}` is still refused even though
the call inside it now parses. Also recorded: because PERCENT is recognised in
that position, a column of that name cannot be the sole select item straight
after `TOP n` — qualify or quote it.
🔴 There is NO TIMESTAMPSUB, in ODBC or in MySQL, and the docs say so: subtract
with a negative count.
Gate: these docs have no build, so the parse probe IS the gate. All 24 SQL
examples added or changed here were parsed against a parser built from
`origin/main` AFTER #360 merged — 0 rejected, 0 non-round-tripping — and the
date/length results were executed against real Elasticsearch 8.18 rather than
computed by hand. The unit-first `DATE_DIFF(unit, a, b)` spelling is documented
because the probe showed it is what the engine renders BACK.
Version stamped `0.24.0`, checked against `build.sbt` on main rather than
assumed — #360 is an ancestor of main and main is 0.24.0-SNAPSHOT.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fupelaqu
added a commit
that referenced
this pull request
Sep 18, 2026
Independent review of the previous commit. One HIGH, and it is a regression I
introduced rather than a pre-existing gap.
🔴 `DATEDIFF(unit, start, end)` stopped parsing. It is T-SQL's and DuckDB's
spelling — what a BI tool set to a SQL Server or DuckDB dialect emits — and both
define it as `end - start`, which is exactly what this engine already computed.
Moving `DATEDIFF` onto its own token for the MySQL two-argument form took the
unit-first spelling with it, because `date_diff_transact_sql` keyed on
`DateDiff.regex` alone:
SELECT DATEDIFF(DAY, a, b) AS v FROM t
origin/main OK -> SELECT DATE_DIFF(DAY, a, b) AS v FROM t
previous REJECT ParserError(end of input expected)
Fixed by keying that production on BOTH names. The sign is free: unit-first is
`end - start` in T-SQL, in DuckDB and here. Ordering is safe because
`date_diff_transact_sql` is tried before `mysql_date_diff` and `time_unit` fails
on a plain column, so `DATEDIFF(a, b)` still reaches the MySQL form — pinned, so
that ordering cannot silently revert the sign.
🔴 `GrammarDiffProbe` did NOT catch this: its corpus contains no
`DATEDIFF(unit, a, b)` input. That is the SECOND time it has returned a clean
differential over a real narrowing (the first was `SELECT top -1` in #360). It
is necessary and not sufficient, and a spelling removed from a `words` list
needs its own assertion rather than a corpus sweep. Said so at the test.
Also from the review:
- the help document's first example was titled "MySQL's own documented example /
Returns 1" while its SQL used COLUMNS, which cannot return 1. `HelpCorpusSpec`
only probes that an example PARSES, so nothing could catch a false title. It
now uses the literals it claims, and the unit-first spelling is documented.
- `mysql_date_diff` was missing from the `time_function` alternation (present
only in `date_diff_identifier`). No reachable failing input was found — every
operand site tries `identifierWithTransformation` first — so this is symmetry,
not a fix.
Falsification: keying the unit-first production on `DateDiff.regex` alone again
reddens exactly the new assertion. `sql/test` 1357, `core/test` 1129, the 2.12
leg and the CI lint line all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 18, 2026
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 #361
Closes #362
Four dialect spellings a BI tool emits that the grammar rejected, each sourced from a
residualrow of the Epic 19 capture with a bisected cause — plus one unrelated defect found while sizing them.What is accepted now
CHAR_LENGTH/CHARACTER_LENGTHLENGTHtableau.sql92.w8.032TIMESTAMPADD(unit, n, base)DATETIME_ADDtableau.sql92.w4.025(partly — see below)TIMESTAMPDIFF(unit, d1, d2)DATE_DIFFSQL_TSI_DAY,SQL_TSI_MONTH, …SELECT TOP n,SELECT TOP (n)LIMIT ntableau.sql92.wx.010Every one is an alias or a re-spelling, never a second mechanism: the render normalises to the canonical form, and that render re-parses. The ODBC
(unit, count, base)orderTIMESTAMPADDuses is thetransactSqlform this parser has always accepted, so the alias adds a name and no semantics.Two things deliberately not done:
TIMESTAMPSUBis not added. It exists in neither ODBC nor MySQL (verified against the 8.4 reference), and the capture itself uses a negative count withTIMESTAMPADD— which already worked.tableau.sql92.w4.025does not flip. It had two independent blockers; this PR removes the function-spelling one. The other is the ODBC{fn … }escape, which is a different mechanism (a pre-parse normalisation, not a grammar production) and belongs in its own PR.Also fixed:
CREATE WATCHER … WHEN x >= 0Parser.comparison_operatororderedgtbeforegeandltbeforele. These are string literals, not anchored tokens, so>matched the first character of>=and left=for the value production:WhereParser.comparisonOpalways had the right order; this production did not. Unrelated to the BI work — found while sizing the<=/<=>prefix hazard for a future<=>change.🔴 Two defects an independent review caught before this was pushed
SELECT top -1 AS x FROM tregressed from parsing to a hard error. A named "non-negative row count" refusal usederr, andParsers.|never tries another alternative after anError— sooptcould not backtrack to readingtopas a column. It reached every SELECT-bearing production (CTAS, derived tables,INsubqueries). The guard is now afailure, and each case is pinned against the same statement with a different identifier as the control. The spec had pinned the defect; that test is deleted, not adjusted.SELECT TOP 5 PERCENT a FROM tparsed, as the columnPERCENTaliased toa. A loud rejection had become a silent wrong answer (GROUP BY without LIMIT silently returns only the top 10 groups (silent wrong answer) #205/GROUP BY without an aggregate in the SELECT list returns per-document rows instead of one row per group (silent wrong answer) #253 family).TOP n PERCENTandTOP n WITH TIESare now refused by name.Also from the review: an
Intoverflow that defeated the count guard one line after it was written (TOP 2147483648→LIMIT -2147483648), and aTIMESTAMPDIFFassertion satisfied by the sign-inverted render — the very binding it existed to pin.Known gap, deliberately not closed:
SELECT DISTINCT TOP nis rejected while the invalidSELECT TOP n DISTINCTis accepted, becauseDISTINCTbinds to the identifier, not to the SELECT. Closing it means givingDISTINCTtwo owners, which is a restructuring, not a fix.Scoreboard — 70/99 → 72/99 · 72/75
Two rows flip
rejected→parses, so PARSE moves 91 → 93, and both are scored:tableau.sql92.w8.032(SUM(CHAR_LENGTH(name)))rejected·local:char-length-spellingparses·issue:361·fixedtableau.sql92.wx.010(SELECT TOP 1 *)rejected·local:select-top-nparses·issue:361·fixedscored = fixedrequires a merged suite asserting the statement's correctness, not its parse — so the second commit supplies one. The never-counted gap returns to "21 capability probes + 0 that parse but answer wrongly, incompletely or UNMEASURED".The exception is a second compiled singleton (
Issue361FixedIds) besideIssue328FixedIds— never a widenedowner.startsWith("issue:")predicate, which would handfixedthe meaning "it parsed" to every future issue owner in silence. Checked in both directions, so an id the code excepts that the table stops scoring is a loud dead exception.tableau.sql92.w4.025deliberately stays rejected and keeps its local owner: itsTIMESTAMPADDblocker is gone, its{fn …}escape blocker is not.The execution suite, and why it cannot be passed by accepting a spelling
BiDialectExecutionSpec— testkit + 5 client subclasses, against real Elasticsearch:CHAR_LENGTHis asserted against a multi-byte string.海豚is 2 characters and 6 UTF-8 bytes, so a byte-counting implementation — exactly what MySQL's ownLENGTHdoes — answers 6 and reddens. An ASCII-only assertion would have been satisfied by either.TOP nis asserted on row count and row set against theLIMIT nspelling, over a 3-shard index of 24 documents — above Elasticsearch's default page of 10, so a dropped bound cannot hide behind the default. A control asserts the unbounded statement returns all 24.🔴 The integration mutation was run, not assumed: removing the fold of
TOPinto the statement'sLIMIT, while leavingTOPparsing, reddens exactly the threeTOPexecution rows and leaves theCHAR_LENGTHrows green.Two defects in the suite's own first draft, both found by running it:
script_fieldsvalues come back wrapped in Elasticsearch's per-field array on the row path while the same expression under an aggregation is a scalar (pre-existing, documented against #209); and the 24 bulk documents were seeded with a name that collided with one of the named rows, so 25 documents matched and every count oracle was quietly off by one.commitis the literalPENDING, to be stamped at merge — better than a SHA that is wrong the moment the branch is amended. (The existingpost-epic22head row carries its parent commit,7187c7d9; pre-existing, not touched here.)No behaviour change on anything that parsed before
GrammarDiffProbe, 16,438 inputs, this branch vs a parser built fromorigin/main:🔴 That probe reported 0 narrowings on the defective code too — its corpus never contained
SELECT top -1. It is necessary and not sufficient; the review's hand-built input is what found the regression.Parse cost
Interleaved control/branch, 5 rounds each, first round of each side discarded (medians, µs):
SELECTSELECTlist +LIMITAll three ranges overlap ⇒ no signal. Reported as no signal rather than a direction, though
top.?does add one optional regex attempt perSELECT.Mutation matrix
Every mechanism checked by the smallest edit that disables that decision alone — never a whole-branch revert:
CHAR_LENGTHaliasesTIMESTAMPADDSQL_TSI_DAYonlyTOPparsing, drop theLIMITfoldTOP+LIMITconflict errortoprequired (killoptbacktracking)Verification
sql/test1348 passed ·core/test1129 passedBiDialectExecutionSpec8/8 on each of the five clients — real ES 6.8 (rest + jest), 7.17, 8.18, 9.0sql/compile,sql/Test/compile,core/compile,core/Test/compileall cleanheaderCheck scalafmtSbtCheck scalafmtCheck test:scalafmtCheck🤖 Generated with Claude Code