Repository navigation
A function on the left of IN / NOT IN was rendered twice - #366
Merged
Merged
Conversation
Closes #365 `Identifier.sql` already renders the function chain — `Identifier` is sealed with a single implementation that never overrides `sql`, and `Expression.functions` IS `identifier.functions` — and `InExpr.sql` re-applied the head function on top of it. How that looked depended on the function's own `toString`: WHERE ABS(a) IN (1) -> WHERE ABS(a)(ABS(a)) IN (1) re-parse FAILS WHERE YEAR(a) IN (2024) -> WHERE YEAR(YEAR(a)) IN (2024) re-parse SUCCEEDS⚠️ The severity is NOT what I first filed. I called the second case a silent wrong answer, because `YEAR(YEAR(a))` is legal SQL asking the year OF THE YEAR. An independent review MEASURED it and that is false: the temporal emission collapses a repeated extractor, so the Painless is BYTE-IDENTICAL to `YEAR(a)` across every statement whose render moved — 5,934 old renders rejected outright, 3,387 re-parsing to identical Painless, zero genuinely silent wrong answers. 🔴 The lesson, since I got this backwards: *"it re-parses to something legal" is a claim about the GRAMMAR; "it answers differently" is a claim about the EMISSION.* I made the second on the evidence of the first. So the extractors rendered wrongly and still ANSWERED correctly. The LOUD half — ABS / LOWER / LENGTH / UPPER / COUNT / MAX — is the one that costs something, and it does not self-heal (see the release note below). The fix is a return to the house form, not a new convention: every sibling render in this file already interpolates `$identifier` — the comparison arm (`:508`) and `BETWEEN` (`:1689`) — and the `id` wrapper existed at exactly one site. ## RELEASE NOTE A materialized view or computed column authored BEFORE this fix has the broken render PERSISTED in its metadata: `MaterializedViewExtension` stores it and re-runs `client.run(alter.sql)`, and `SHOW CREATE MATERIALIZED VIEW` echoes it. This fix does not rewrite what is already stored, so any view whose WHERE/HAVING uses `f(x) IN (…)` with a non-extractor function must be RE-CREATED. ## The guard is a PROPERTY OVER THE CATALOGUE, for the `f(a)` shape `InPredicateRenderSpec` sweeps every accepted spelling from `SQLKeywords.functionTokens` (not a list kept in the test, which would go stale the moment a function is added) across BOTH venues an `IN` can appear in, and asserts the render is a fixed point. 🔴 Hand-picked pins would have been written against `ABS`-shaped functions and would have MISSED the extractors — they are the ones whose broken render looks fine. Non-vacuity is asserted over the material it guards: >= 80 spellings and >= 250 statements (88 / 292 today), plus both failure shapes named. 🔴 And do NOT "strengthen" this to AST equality. `Identifier` inherits `PainlessParam.equals`, which compares only `param`, so `DAY(a)` and `DAY(DAY(a))` are EQUAL as ASTs. `Parser(stmt.sql) == Right(stmt)` — the repo's standard round-trip assertion — is blind to this whole defect class; the render fixed point is the strong form.⚠️ `HAVING` is not a variation on `WHERE` here — it is where the AGGREGATES live, and `HAVING COUNT(x) IN (1, 2)` rendered `COUNT(x)(COUNT(x))`. A WHERE-only sweep would have left that half unguarded; the differential probe surfaced it.⚠️ Scope: a property over `f(a)`, not every writable call. The zero-argument `AVG() IN (1,2)` still renders unparseably, as it does on the untouched arms (`AVG() > 1`, `SELECT AVG()`) — a parser-accepts-`f()` problem, not this one. Three of my own assertions were wrong before they were right, all caught by running them: equality with the input (the render legitimately normalises `IN (1, 2)` to `IN (1,2)`); counting the operand spelling over the whole statement (`COUNT(x)` appears twice legitimately when it is also in the SELECT list, and `YEAR(YEAR(a))` still contains `YEAR(a)` exactly once — it is the function NAME inside the PREDICATE segment that goes 1 -> 2 under both shapes); and a `split(" WHERE | HAVING ")` that a value list containing " WHERE " would defeat. ## Verification - Mutation, restoring the `id` wrapper: **292 of 292** swept statements redden, plus the named rows. The controls stay green. - `GrammarDiffProbe`, 16,536 inputs, branch vs `origin/main`: **0 narrowed, 0 widened, 14 renders changed — every one a fix**, including four HAVING / aggregate shapes. - Independent review built a real `origin/main` control and ran a 10,354-statement differential (153 spellings x 15 operand shapes x 14 venues): **OLD-RIGHT-NEW-WRONG = 0**; old re-parse failures 5,934 -> 22 (the pre-existing `f()` family). Painless / ES-query emission for 398 `InExpr` nodes: identical SHA on both builds. Bridge JSON pins 208/208. - `sql/test` 1352, `core/test` 1129, the 2.12 leg, the CI lint line. - No parse-cost measurement: the diff touches a render method only, not the parse path, and the differential confirms no verdict moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fupelaqu
force-pushed
the
fix/365-in-render-duplication
branch
from
September 18, 2026 16:41
01a480b to
e0fca06
Compare
fupelaqu
marked this pull request as ready for review
September 18, 2026 17:49
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.
fix(sql): a function on the left of IN was rendered TWICE
Closes #365
Identifier.sqlalready renders the function chain —Identifieris sealed witha single implementation that never overrides
sql, andExpression.functionsISidentifier.functions— andInExpr.sqlre-applied the head function on top ofit. How that looked depended on the function's own
toString:WHERE ABS(a) IN (1) -> WHERE ABS(a)(ABS(a)) IN (1) re-parse FAILS
WHERE YEAR(a) IN (2024) -> WHERE YEAR(YEAR(a)) IN (2024) re-parse SUCCEEDS
wrong answer, because
YEAR(YEAR(a))is legal SQL asking the year OF THE YEAR.An independent review MEASURED it and that is false: the temporal emission
collapses a repeated extractor, so the Painless is BYTE-IDENTICAL to
YEAR(a)across every statement whose render moved — 5,934 old renders rejected outright,
3,387 re-parsing to identical Painless, zero genuinely silent wrong answers.
🔴 The lesson, since I got this backwards: "it re-parses to something legal" is
a claim about the GRAMMAR; "it answers differently" is a claim about the
EMISSION. I made the second on the evidence of the first.
So the extractors rendered wrongly and still ANSWERED correctly. The LOUD half —
ABS / LOWER / LENGTH / UPPER / COUNT / MAX — is the one that costs something, and
it does not self-heal (see the release note below).
The fix is a return to the house form, not a new convention: every sibling render
in this file already interpolates
$identifier— the comparison arm (:508) andBETWEEN(:1689) — and theidwrapper existed at exactly one site.RELEASE NOTE
A materialized view or computed column authored BEFORE this fix has the broken
render PERSISTED in its metadata:
MaterializedViewExtensionstores it andre-runs
client.run(alter.sql), andSHOW CREATE MATERIALIZED VIEWechoes it.This fix does not rewrite what is already stored, so any view whose WHERE/HAVING
uses
f(x) IN (…)with a non-extractor function must be RE-CREATED.The guard is a PROPERTY OVER THE CATALOGUE, for the
f(a)shapeInPredicateRenderSpecsweeps every accepted spelling fromSQLKeywords.functionTokens(not a list kept in the test, which would go stalethe moment a function is added) across BOTH venues an
INcan appear in, andasserts the render is a fixed point.
🔴 Hand-picked pins would have been written against
ABS-shaped functions andwould have MISSED the extractors — they are the ones whose broken render looks
fine. Non-vacuity is asserted over the material it guards: >= 80 spellings and
🔴 And do NOT "strengthen" this to AST equality.
IdentifierinheritsPainlessParam.equals, which compares onlyparam, soDAY(a)andDAY(DAY(a))are EQUAL as ASTs.Parser(stmt.sql) == Right(stmt)— the repo'sstandard round-trip assertion — is blind to this whole defect class; the render
fixed point is the strong form.
HAVINGis not a variation onWHEREhere — it is where the AGGREGATES live,and
HAVING COUNT(x) IN (1, 2)renderedCOUNT(x)(COUNT(x)). A WHERE-only sweepwould have left that half unguarded; the differential probe surfaced it.
f(a), not every writable call. The zero-argumentAVG() IN (1,2)still renders unparseably, as it does on the untouched arms(
AVG() > 1,SELECT AVG()) — a parser-accepts-f()problem, not this one.Three of my own assertions were wrong before they were right, all caught by
running them: equality with the input (the render legitimately normalises
IN (1, 2)toIN (1,2)); counting the operand spelling over the wholestatement (
COUNT(x)appears twice legitimately when it is also in the SELECTlist, and
YEAR(YEAR(a))still containsYEAR(a)exactly once — it is thefunction NAME inside the PREDICATE segment that goes 1 -> 2 under both shapes);
and a
split(" WHERE | HAVING ")that a value list containing " WHERE " woulddefeat.
Verification
idwrapper: 292 of 292 swept statements redden,plus the named rows. The controls stay green.
GrammarDiffProbe, 16,536 inputs, branch vsorigin/main: 0 narrowed,0 widened, 14 renders changed — every one a fix, including four HAVING /
aggregate shapes.
origin/maincontrol and ran a 10,354-statementdifferential (153 spellings x 15 operand shapes x 14 venues):
OLD-RIGHT-NEW-WRONG = 0; old re-parse failures 5,934 -> 22 (the pre-existing
f()family). Painless / ES-query emission for 398InExprnodes: identicalSHA on both builds. Bridge JSON pins 208/208.
sql/test1352,core/test1129, the 2.12 leg, the CI lint line.path, and the differential confirms no verdict moved.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
🤖 Generated with Claude Code
I attempted to fold it into this PR and reverted the attempt, because the fix I wrote did not work and shipping it would have been an unverified change.
What is established, by execution on real Elasticsearch 8.18 over a week of dates:
A sweep of every accepted spelling in
SQLKeywords.functionTokensshowsWEEKDAY/DAYOFWEEKis the only function whose Painless is a statement sequence (def left = def arg0 = …;) — 2 broken, 94 clean — because it is the onlyExtractputting a nullable value inargs.🔴 But making that emission a single expression changes none of the measured behaviour. So the malformed Painless is real and is not the (whole) cause. Something upstream already produces a predicate that cannot match — consistent with a
term/termspushdown on the raw date field, except thatYEAR(d) = 2025is pushed down correctly, so it is not a general defect either.Rather than guess further inside a PR about something else, it is #367 with the measurements and the ruled-out hypothesis recorded.
This PR is unchanged: the reviewed
INdouble-render fix, rebased ontomainafter #364.