Repository navigation
fix(sql): seven of #373's nine Painless emission defects, plus a semicolon-in-a-literal residual - #375
Merged
Conversation
…373, 1/9) `java.time.LocalTime` is the ONE temporal without `isEqual`: `LocalDate` and `LocalDateTime` declare it, `ZonedDateTime` inherits it from `ChronoZonedDateTime` (checked with `javap`). Both sites that hard-code a temporal equality emitted it for every `SQLTemporal`, so a TIME receiver failed the shard -- MEASURED on ES 8.18, `dynamic method [java.time.LocalTime, isEqual/1] not found`, for `CAST(d AS TIME) = CAST('00:00:00' AS TIME)` in a `script` filter and for `NULLIF(CAST(d AS TIME), CAST('00:00:00' AS TIME))` in a `script_fields`. It is why #370 could pin the TIME identity but not execute it. `=` and `<>` over a TIME receiver now use `compareTo(…) == 0` / `!= 0`. 🔴 `compareTo` and NOT `equals`, and that is the whole point -- found by review of the first version of this commit, which used `equals`. The WHERE dispatch reads `valueType`, the type of the RIGHT operand, while the receiver is the LEFT one, so a MISMATCHED pair reaches the same arm: `WHERE name = CAST('00:00:00' AS TIME)` inside a CASE, a `String` receiver. `equals` takes an `Object` and answers FALSE for that -- a loud shard failure would have become a silent wrong answer, and `<>` would have matched every document. MEASURED on ES 8.18, all three on a `String` receiver: `equals` -> `[false,false,false]`; `compareTo` -> `class_cast_exception`; `isEqual` (main) -> `dynamic method … not found`. For a `LocalTime` the two spellings are identical, nanoseconds included (measured). 🔴 `NULLIF` needed a THIRD key, and two wrong ones were tried before measuring: neither `out` nor `expr1.out` says TIME -- both report TIMESTAMP, the COLUMN's type, even when both arguments are TIME. `Identifier.chainType` (#367) is the derivation that describes the RENDERING, and it answers TIME. `isBefore` / `isAfter` DO exist on `LocalTime`, so the ordering comparisons are untouched, and the other three temporals keep `isEqual` -- widening the arm would change `ZonedDateTime` equality, which compares the INSTANT and is deliberately not `equals`.⚠️ A rendering probe over 1,426 shapes (review) shows 150 renderings moved across 75 statements, every one `isEqual` -> `compareTo` and nothing else. The first version of this commit claimed "exactly the two TIME comparisons moved", measured on a 28-shape probe that was too narrow to surface the mismatch case. #370's `ParameterIdentitySpec` pinned the old spelling and mandated the defect in a comment; both move here. `PredicateFunctionResultSpec` gains the executed rows (`= '00:00:00'` -> every midnight row, `= '00:30:00'` -> none, `<>` -> every row), replacing a comment that said this shape was deliberately absent. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#373, 2/9) `checkCase` emitted `e != null && e.isEqual(c) ? v`: the CASE EXPRESSION was guarded, the CANDIDATE was not. Any document missing the WHEN column called the comparison with `null` -- MEASURED on ES 8.18 over the exact shape the bridge fixture pins, with a document lacking `lastSeen`: before null_pointer_exception: Cannot invoke "ChronoLocalDate.toEpochDay()" because "other" is null after d -> 2025-01-03 (lastSeen + 2) e -> 2025-01-04 (lastUpdated) g -> 2024-10-01 (ELSE) So the published `CASE … WHEN <column> …` failed the WHOLE query on any index where that column is not set on every document. The `Varchar` arm's `compareTo` had the same hole, and so did `NULLIF` one method up in the same file: `NULLIF(CAST(d AS TIME), CAST(ts AS TIME))` answered `null_pointer_exception: Cannot read field "hour" because "other" is null`. Both are fixed here, because shipping one and not the other would make this commit's own claim -- that the sites cannot drift -- false again by the next release. 🔴 `NULLIF`'s guard means NOT-EQUAL, not null. SQL says `NULLIF(a, b)` is NULL when `a = b`; with `b` NULL the comparison is UNKNOWN, so the rows are not equal and the answer is `a`. Short-circuiting `b == null` to the NULL branch would have inverted it -- executed, the document without `ts` correctly yields its own time rather than null. 🔴 The candidate is ALWAYS bound and ALWAYS guarded, and the first version of this commit was wrong to condition that on `cond.nullable` -- found by review. MEASURED: `UPPER(other)` over a keyword column reports `nullable = false` while rendering `(param3 == null) ? null : param3.toUpperCase()`, so `CASE UPPER(name) WHEN UPPER(other) …` was neither bound nor guarded and threw `null_pointer_exception: Cannot read field "value"` -- the identical failure this commit exists to fix, on a reachable statement. None of the three structural predicates I measured can see it: the function wrapper's `name` is EMPTY, its `nullable` is false, and its function is not a `FunctionWithIdentifier`. The unconditional rule needs no predicate and is provably safe -- binding is a no-op when the candidate is already a name, and a redundant `!= null` costs bytes, never an answer. Three literal candidates gain a binding; none changes value. Review checked the one risk I had not raised -- that binding hoists the candidate out of the `e != null &&` short-circuit -- and every hoisted rendering carries its own null guard, so it yields null rather than throwing. 🔴 The TIME spelling follows the RECEIVER, not `out` -- also found by review. An arm keyed on `out` was DEAD: `out` is the CASE's RESULT type and reports TIMESTAMP for `CASE CAST(d AS TIME) WHEN CAST(ts AS TIME) …` whose operands are both `LocalTime`. MEASURED, that shape still answered `dynamic method [java.time.LocalTime, isEqual/1] not found` -- the very failure 1/9 removes -- and now returns values. `Identifier.chainType` (#367) is the key, the same one `NULLIF` needed in 1/9.⚠️ Fixes more than it claims, found by review: `CASE n WHEN ABS(n) THEN 1 ELSE 0 END` emitted `param2 == (param1 == null) ? null : … ? 1 : 0`, where `==` binds tighter than `?:` -- a hard shard failure (`Cannot cast from [java.lang.Double] to [boolean]`) for any non-temporal CASE whose candidate renders a ternary. The unconditional binding parenthesises it away by construction. Both `SQLQuerySpec` copies' `caseWhenExpr` and `NULLIF` pins moved and the new emissions were executed. Every assertion seen red by mutation. `sql` 1415, both bridge copies 208, lint green. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…chain (#373, 3/9) #370 made identity one parameter per (column, folded chain) -- but only in a QUERY context. A temporal chain in an ingest processor registers its PARSED base (`processorTemporal`) as a `LiteralParam`, and that literal is IDENTICAL for every chain over one column, so two chains collapsed onto it and both folds landed on the same parameter: SCRIPT AS (CASE WHEN YEAR(d) > MONTH(d) THEN 1 ELSE 0 END) def param1 = (…ctx.d…).get(ChronoField.YEAR).get(ChronoField.MONTH_OF_YEAR) an `int.get(ChronoField)`. `GREATEST(YEAR(d), MONTH(d))` is the same. The extensions' `FieldAnalyzer` renders through `PainlessContext(Processor)`, so materialized-view enrichment inherits it. `LiteralParam` carries an optional `key` and that registration passes the chain-aware one -- `Identifier.contextKeyOf(base)`, #370's derivation over an arbitrary operand rendering, so the rule reaches this context rather than being restated for it. 🔴 The first version of this commit ALSO keyed the identifier registrations in `addParam`/`get`, for both the processor and the transform contexts. Review found that half untested, and MEASURING it is what settled the design: reverting it changes exactly two shapes, and in BOTH the key only ADDS a duplicate declaration (`def param2 = ctx.d`, byte-identical to `param1`) that nothing uses differently -- because in those shapes the chain is dropped before it reaches the parameter. It fixes nothing and costs a dead `def`, so it is not done.⚠️ That matters most for TRANSFORM, which the first version changed by symmetry and without a test: there the folded functions are dropped ENTIRELY -- `YEAR(d) + MONTH(d)` renders `param1 + param2` with BOTH parameters reading the raw `doc['d'].value` and no `.get(ChronoField…)` anywhere -- so the answer was wrong before and would have stayed wrong, while a transform script is PERSISTED on a materialized view. Pure ALTER churn for no correctness. Recorded as a residual; not touched here.⚠️ This commit fixes the IDENTITY and the ingest script still does not RUN: the extracted `int` is handed back to the processor's temporal parse, which is item 7 and its own commit. MEASURED as an ingest pipeline on ES 8.18 over `{"d":"2025-01-10"}`: the computed column is ABSENT before AND after, because `ignore_failure: true` swallows the throw -- which is why the defect was invisible. No processor shape is made executable by this commit alone (review looked too, and agrees); the executed proof travels with item 7. Both surviving halves seen red by mutation. `sql` 1417, both bridge copies 208. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…373, 4/9) `SQLTypeUtils.coerce` builds its result by interpolating the operand and then wraps it in a null guard that tests the operand AGAIN, so a chained operand was rendered TWICE and every function in it ran twice per document: DATE_ADD(DATE_PARSE(name, 'yyyy-MM-dd'), INTERVAL 1 DAY) > d ((param2 == null) ? null : (def)(param2.plus(1, …)) != null ? (param2 == null) ? null : (def)(param2.plus(1, …)).atStartOfDay(…) : null) 🔴 It is CORRECT: executed on ES 8.18 before and after, both return the same single row. This is COST, not a wrong answer -- the pin is on the count. The operand is bound once through `addParam` and the substitution into `ret` is exact rather than textual luck: `ret` was interpolated from `expr`, so every occurrence of it there IS this operand. Three shapes deduplicate, and the biggest win is a bridge pin where a whole `ZonedDateTime.parse(param1, new DateTimeFormatterBuilder()…toFormatter()…)` was being built twice per document; both `SQLQuerySpec` copies move.⚠️ NOT a refactor of `coerce`: the method has 34 references to its operand and is the one #367 and #370 kept destabilising, so the binding happens at the one site where the duplication is, and no arm is touched. 🔴 Two earlier versions of this commit's third test were VACUOUS -- neither reddened when the binding was made unconditional. The reason is the finding: `addParam` returns the EXISTING name when the literal it is handed IS one, so the "is it already a name?" guard I had written could never matter. It is removed rather than left as unfalsifiable code (measured: no emission moves), and the test now asserts the property that does hold -- no parameter is declared as a bare alias of another.⚠️ The moved `date_diff` bridge pin is dedup-only and is NOT executable on either side: that no-schema fixture reads a keyword doc-value and then parses it as a temporal, so it fails identically before and after (`dynamic method [java.lang.String, toLocalDate/0] not found`). Pre-existing, unrelated, and recorded rather than papered over. `sql` 1418, both bridge copies 208, lint green. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…373, 5/9) `requiresScript` only ever inspected the LEFT identifier, so a comparison of one column with another -- which the Elasticsearch query DSL cannot express at all -- fell through to `rangeQuery` keyed on the left field NAME, with the RIGHT column rendered as DATE MATH: WHERE d > DATE_TRUNC(ts, MONTH) {"range":{"d":{"gt":"ts||/M"}}} WHERE d > ts {"range":{"d":{"gt":"ts||"}}} MEASURED on ES 8.18: `failed to parse date field [ts] with format [strict_date_optional_time||epoch_millis]` -- Elasticsearch read the column NAME as a date literal. A range can only compare a field with a CONSTANT. Routed to the script path, which renders both sides and already emitted a correct comparison; executed over `a` (d = ts = 2025-01-01), `b` (d 2025-01-05, ts 2025-01-04) and `c` (d 2025-01-05, NO ts): `['b']`, which is the truth. Constants are untouched: `range`, `term`, `terms` and `BETWEEN` over literals all keep their bytes, and a function-wrapped LEFT operand still scripts.⚠️ NOT applied to the `BETWEEN` call site: `BetweenExpr.maybeValue` is always a `FromTo`, never an `Identifier`, so passing it there would be dead code. `d BETWEEN ts AND ts` therefore still fails -- LOUDLY, at query-build time (`Unsupported out type for range query: ANY`) -- and is recorded as a residual rather than silently half-fixed. 🔴 The spec attaches a SCHEMA, and that is what lets it see the defect at all. The first version did not: without one every identifier stays `Any`, the date-math branch is never taken, and BOTH the fixed and the unfixed engine emit a script -- it passed with the fix reverted. #306's rule again, and the reason the defect only ever appeared through the live client path. `sql` 1420, bridge 212, es6bridge 208, lint green. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rameter names (#373, 6/9) `ArithmeticExpression.toPainless` read each operand back as a parameter NAME (`ctx.get(left)`) -- the RAW doc-value the identifier registered -- so the function chain never reached the expression and `YEAR(d) + MONTH(d)` computed `d + d`. It is #367's defect one layer up. Three rules, each measured: 1. The parameter shortcut is KEPT where the operand has no chain to drop. A parameter name IS the whole operand exactly then, so the emission is byte-identical for every shape that was already right. 2. A non-bare rendering is declared in the script's PROLOGUE, via the existing `PainlessContext.bindLocal`, and the operand becomes its NAME. A Painless `def x = …;` is a STATEMENT and cannot sit inside a parenthesised operand. 3. A numeric coercion is skipped ONLY over a NULLABLE operand. What Painless refuses is a primitive cast over a null guard, in both directions: `((double) (param1 != null ? … : null))` and `((long) (…))` both answer `Cannot cast null to a primitive type`. And the coercion TARGET is derived from the COLUMN (`out` folds the operands' `out`), so `CAST(n AS DOUBLE) + 1` over an INT column asked DOUBLE -> BIGINT and emitted a cast that both truncates and fails to compile. MEASURED as a real ingest pipeline on ES 8.18 over `{"d":"2025-01-10"}` (truth: 2025 + 1 = 2026): YEAR(d) + MONTH(d) before c = '2025-01-102025-01-10' after c = 2026 YEAR(d) * 100 + MONTH(d) before c = ABSENT after c = 202501 🔴 The "before" of the first row is a CONCATENATION, not a number: with the chain dropped both operands were the raw `ctx.d` string. Against the column's declared `c INTEGER` mapping Elasticsearch REJECTS the whole document (`document_parsing_exception … Preview: '2025-01-102025-01-10'`), so the failure mode is ingest-time DOCUMENT LOSS, not a wrong number in the index. (Corrected after review measured it three times; an earlier version of this message claimed `c = 1`, which no measurement reproduced.) 🔴 The coercion SOURCE is what the operand RENDERS, not its column's type (#367's rule, which this site never applied): `YEAR(d)` renders an `int`, and reading `baseType` asked for a TIMESTAMP -> BIGINT conversion that emitted `.toInstant()` on that `int`. The QUERY half moves too, and is executable. MEASURED on ES 8.18, index `t` (n = 1/5/9, d = 2025-01-01 / 01-05 / 01-09): WHERE YEAR(d) + 1 = 2026 before class_cast (ZonedDateTime + 1) after [a, b, c] Rule 2 also fixes four shapes BROKEN BEFORE this commit, in every venue -- nested arithmetic declared its local inline and so spliced a statement into an expression slot: WHERE n * 2 + 1 > 2 compile error -> [a, b, c] WHERE (n + 1) * (n + 2) > 2 compile error -> [a, b, c] SELECT (n * 2 + 1) * (n * 3 + 1) compile error -> [12, 176, 532] SELECT (CAST(n AS BIGINT) + 1) * (CAST(n AS BIGINT) + 2) worked, and no longer declares `lv1` twice Rule 3 fixes a PRE-EXISTING silent wrong answer wherever a schema IS attached: `CAST(n AS DOUBLE) / 2` divided as integers (`[0, 2, 4]`, and `WHERE … / 2 > 2` returned `[c]` instead of `[b, c]`) because the cast was applied to the wrong side. It is `[0.5, 2.5, 4.5]` / `[b, c]` now. 🔴 Found by review, THREE times, and each finding is what shaped a rule. - v1 dropped the shortcut for EVERY operand. - v2 kept it only for a CHAINLESS one, which still left every CAST operand (a CAST HAS a function) on the render path with an INLINE declaration. Five predicates, one projection and one computed column went from correct results to `compile error` (`invalid sequence of tokens near ['def']`) -- the CAST family above. All seven now return their exact pre-commit values. - v3 skipped the numeric coercion on TYPE alone, which dropped it from the LITERAL too -- and a literal carries no null guard, so its `((double) 2)` was the only thing making the division floating-point. On the schema-LESS rendering path that turned `CAST(n AS DOUBLE) / 2` into INTEGER division, HTTP 200, in script fields, `terms` keys, sort scripts AND predicates (`[b, c]` -> `[c]`: a row silently vanished). #205's family, and production reaches that path whenever `resolveWithSchema` declines -- a wildcard or multi-index FROM, a JOIN, or a mapping that cannot be loaded. NULLABILITY is the discriminator. The guard asserts RULES, not bytes: a `def` may only open a statement, no local may be declared twice, no null-guarded ternary may be cast to a primitive, and the non-nullable operand's cast must survive on BOTH the schema and no-schema paths.⚠️ Residuals, deliberately NOT fixed here and none of them a regression: - `WHERE YEAR(d) + MONTH(d) = 2026` still fails the shard (`illegal_argument`) and `… * 100 + MONTH(d) = 202501` still fails to compile -- the arithmetic node's own type reports its COLUMN's. Correcting that means making `baseType` chain-aware, which #367 MEASURED and recorded as a trap (it sends `ORDER BY DATE_PARSE(name, …)` down the date-math branch). Same wall as item 1. - An operand whose rendering is not a placeable EXPRESSION falls back to its parameter, so its chain is still dropped. Painless has no expression-level `try`, so `TRY_CAST` renders as a STATEMENT (#367's `bindLocalWith` note) and coercing it produced `def lv1 = String.valueOf(try { … } catch …);`, which ES rejects. `DDL TRY_CAST(name AS BIGINT) + 1` is byte-identical to the parent (`c = 1201`, a concatenation rather than 121 -- a PRE-EXISTING `TRY_CAST` residual); its QUERY half now renders and projects `null1` instead of `a1`. - A shape whose chain was dropped and which therefore "worked" by concatenating can now fail LOUDLY: `SCRIPT AS (DATE_FORMAT(d,'yyyy;MM') + 1)` stored `'2025-01-051'` and is now a compile error; `SELECT YEAR(DATE_PARSE(name,…))+1` returned `['a1','b1','c1']` and now fails. Every one of those values was garbage, so loud beats silent -- but under `ignore_failure` the DDL case becomes an ABSENT column. - The operand is now evaluated unconditionally, so `WHERE name = 'a' OR CAST(name AS BIGINT) + 1 > 2` throws `number_format_exception` for every document even when the first disjunct matches. That shape was a compile error on the parent, and it is what `Where.bindLocal` has always done. - `ctx.addParam(left)` still registers the raw column, so a chained operand leaves a `def param1 = ctx.d;` that nothing reads -- new here, and persisted in the customer's pipeline. Recorded as a #373 residual rather than fixed, because removing it conditionally moves parameter numbering everywhere. 🔴 `placeable` is the one place this commit adds a rule of its own, so it does not stay untested. The scan is LIFTED into `PainlessOperandForm` (`private[sql]`, beside `PainlessContext` whose `bindLocal` scaladoc already states the rule) and `PainlessOperandFormSpec` now shares it instead of re-implementing a weaker copy that knew only double quotes and nothing about `try`. Its 32 edge cases are driven directly in `PainlessExpressionFormSpec`, including the load-bearing one: an escaped BACKSLASH closes the literal, so `foo("a\\") ; x` really does contain a separator -- a scan that special-cased only `\"` would swallow the rest of the script and splice a statement. A locating variant keeps the diagnostic that spec had (it names the offending spot rather than counting separators). Review established by mutation that at the EMISSION sites this predicate is inert: no reachable shape renders an operand with a `;` inside a literal, and the `try` arm never decides anything because both Painless `try` emitters always emit a `;` too. That is why the arms are exercised against the helper directly -- three further mutations, each reddening only its own arm: blinding the literal handling reddens 3 cases, blinding the word boundary reddens the identifier case (`entry `/`retry`/`registry` all contain `try`), and a naive escape reddens the escaped-backslash case. Two limitations are pinned as CHARACTERISATION, both on the LOUD side (a compile error, never a wrong value): the `try` arm keys on the spelling `try `, and an unterminated literal swallows a following `;`. One bridge pin moves in both copies (`bridge/` and the hand-maintained `es6/bridge/`): the hoisted local is numbered by the CONTEXT, so `lv0` became `lv1`. Nothing else about that script changed, and BOTH spellings were EXECUTED on ES 8.18 and return the same rows ([-9, 15, 71]) -- a byte pin proves the bytes did not move, not that they run. Every assertion seen red, by four disjoint mutations: splicing the declaration inline reddens the placeability cases; shortcutting a CHAINED operand (the pre-commit behaviour) reddens the chain cases; skipping the numeric coercion on type alone reddens the two non-nullable-operand cases; re-enabling it unconditionally reddens the predicate case. Rendering probe over 59 shapes, with and without a schema, in predicate / projection / processor venues. sql 1429, bridge 212, es6bridge 208 green; `headerCheck scalafmtSbtCheck scalafmtCheck test:scalafmtCheck` clean. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…racted (#373, 7/9) `originalType == SQLTypes.Any` says an identifier NAMES a column. It does NOT say the operand still RENDERS one. Where the function chain has already produced something else, the ingest processor handed that result back to its OWN date parse: `YEAR(d)` renders an `int`, and the emission wrapped it in `(param2 instanceof String ? LocalDate.parse(param2, …) : Instant.ofEpochMilli(param2)…)`. `chainType` (#367) is what says whether the rendering is still a temporal, and it is now the guard at both `processorTemporal` call sites that take an IDENTIFIER (`Where.scala`, `SQLTypeUtils.scala`). There are THREE callers in all: the third (`package.scala`, `processorBase`) is handed `processParamName` -- the raw `ctx.<field>` -- so its operand is the source field by construction and it cannot have this defect. 🔴 The guard admits `chainType == Any`, and that is safe because of a SECOND gate: `processorTemporal` emits a parse only when the column's `declaredType` is a temporal and answers `None` otherwise. MEASURED: `SCRIPT AS (YEAR(unknown))` over an UNDECLARED column is exactly the reachable `Any` case and emits ZERO parses; so does a `VARCHAR` column.⚠️ Anything that widens `processorTemporal` to more declared types removes that safety net silently. MEASURED as real ingest pipelines on ES 8.18 over `{"d":"2025-01-10","n":1}`: CASE WHEN YEAR(d) > MONTH(d) THEN 1 ELSE 0 END before script_exception after c = 1 GREATEST(YEAR(d), MONTH(d)) before script_exception after c = 2025 CASE WHEN n > 0 THEN YEAR(d) ELSE MONTH(d) END before "1970-01-01T00:00:02.025Z" after c = 2025 🔴 The third is the one that matters, and it is why this is not merely a "doesn't run" fix: it did NOT fail. It stored the year 2025 read as epoch MILLIS, so a computed column held a 1970 timestamp and nothing anywhere said so. The other two are loud (`script_exception`), and one of them — `GREATEST(YEAR(d), MONTH(d))` in a processor — is the residual item 6's commit recorded as still broken; it runs now. 🔴 A FOURTH site was implemented and then REMOVED, not shipped, and the reason is a PROOF rather than a measurement. `FunctionN.painless`'s processor arm does not call `processorTemporal` at all -- it calls `SQLTypeUtils.coerce(a, in, context)`, whose `case SQLTypes.Any if ctx.isProcessor` arm IS the site guarded here, so a copy at the call site is provably REDUNDANT. (It is also unobservable: reverting it moves zero of the 59 probe shapes and reddens nothing, which is how it was found -- but redundancy is the reason it stays out.) In a TRANSFORM context `isProcessor` is false and that arm is never taken, so the MV venue is covered by the same argument. #293's lesson is that a live copy beside dead code leaves the dead code; the commit is smaller than the fix I first wrote, as commit 3's was.⚠️ Not fixed here, PRE-EXISTING and unchanged: `GREATEST`/`LEAST` render `Math.max` over `def`, so Painless picks the `double` overload and `_source` carries `2025.0` for an INTEGER column (the INDEXED value is 2025). It reproduces with NO date function — `GREATEST(n, m)` over two INT columns stores `7.0` — so it is a `GREATEST` residual of its own, not this commit's. The guard asserts a RULE: every `instanceof String ? …parse(…)` a processor emits exists to turn the RAW source field into a temporal, so its operand must be that field -- either `ctx.<field>` directly or a PARAMETER that aliases it. 🔴 The first version of the rule demanded a literal `ctx.` prefix, which is false: found by review, `CASE WHEN YEAR(d) > 2000 THEN d ELSE d END` and `YEAR(CASE WHEN 1 = 1 THEN d ELSE d END)` both parse `param1` (= `ctx.d`) CORRECTLY -- the branches return the date column, which must be parsed. Both are now in the table, and executed: the first stores `'2025-01-10T00:00:00.000Z'`. A test that mandates a defect is worse than no test.⚠️ The scan also now admits `?` so it can SEE a nested path (`ctx.meta?.when`); before, the regex matched nothing there and the rule was blind to that venue. That widening is a capability, not a guarded behaviour -- removing it reddens nothing, because no current emission parses a nested path at all. Which is itself the residual below. The complement is pinned too — a bare date column still gets its parse, without which every date function in an ingest script breaks again (21.8 Part C, #315).⚠️ Two shapes go from a VALUE to a LOUD failure, and the cause is NOT the re-parse. Fixing the condition makes a previously-unreachable THEN branch run, and that branch carries item 6's wall -- the branch coercion reads the COLUMN's type instead of the chain's, emitting `(def)(param2.toInstant().toEpochMilli())`, i.e. `.toInstant()` on an extracted `int`: CASE WHEN YEAR(d) = 2025 THEN YEAR(d) ELSE 0 END before c = 0 (WRONG -- the broken condition took the ELSE) after script_exception CASE WHEN d IS NULL THEN 0 ELSE YEAR(d) END before '1970-01-01T00:00:02.025Z' after script_exception Silent-wrong -> loud is this PR's stated preference, and neither is this commit's to fix: the cure is making that coercion's source `chainType`, which is the next item in this family.⚠️ Residual for #373, PRE-EXISTING and identical at the parent: a NESTED date column gets no parse at all (`ctx.meta?.when.get(ChronoField.YEAR)`), which throws -- and under the pipeline's `ignore_failure: true` the computed column is simply ABSENT. That is #368's silent mode for nested date columns, and the new assertion cannot see it because no parse is emitted to inspect. Both assertions seen red by mutation, one per site: dropping the `chainType` guard in `Where.scala` reddens the rule, and so does dropping it in `SQLTypeUtils.scala`. The rule's own source-alias acceptance is load-bearing: dropping it reddens with `List("param1", "param1")`, review's exact false positive. sql 1440, bridge 212, es6bridge 208 green; `headerCheck scalafmtSbtCheck scalafmtCheck test:scalafmtCheck` clean. Refs #373 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dary (#373, 8/9) An ingest script is assembled by splitting the rendered Painless, assigning the LAST statement to the computed column, and read back by recovering that assignment. Both halves treated a `;` inside a STRING LITERAL as code. WRITING -- `ScriptProcessor.fromScript` split on a bare `;`, so the assignment was written INTO the literal. Measured on real Elasticsearch 8.18.3 through `_ingest/pipeline/_simulate`: REPLACE(name, ';', 'x') -> param1.replace("; ctx.c = ", "x") compile error CONCAT(name, 'a;b') -> ... + "a; ctx.c = b" compile error UPPER('a;b') -> "a; ctx.c = b".toUpperCase() 200, column ABSENT The last is valid Painless that computes a value and discards it: nothing throws, `ignore_failure` never comes into it, and the document is stored without the column. A `;` as a REPLACE needle needs no contrived input at all. Only the last statement is affected -- a `;`-bearing literal in a `def paramN` preamble was always emitted correctly, which is what hid this. READING -- `ScriptTarget.of` took the last `ctx.<name> = ` by regex, so a literal CONTAINING that text was read as the assignment: `CONCAT(name, '; ctx.d = 1')` made column `c`'s processor report itself as column `d`'s. `IngestPipeline.diff` keys processors by exactly this and a Map drops the loser of a collision, so an ALTER rewrote the sibling's processor and never added the new column. Found by independent review, and it is the WRITING fix that makes it reachable: before it, the poisoned emission was a compile error so the table could not be created. `PainlessOperandForm` already owned a quote-aware scanner, added earlier in this PR for whether a fragment may sit in an arithmetic operand slot. Its literal handling moves into a shared `scanOutsideLiterals`, and both new users -- `splitStatements` for the write, `withoutLiterals` for the read -- are built on it, so the two halves of the contract cannot drift apart. `firstStatementAt` keeps its early exit. `splitStatements` is byte-identical to `String.split(";")` wherever no literal is involved, so no existing emission moves: proved exhaustively over the literal-free alphabet {a, b, ;, \} up to length 7 (21,845 strings, zero mismatches). It can only ever cut FEWER times, and the scaladoc records why that is safe -- the scanner's escape rule is Painless's own, so a disagreement implies an unbalanced literal, which Elasticsearch rejects either way. Guards: `ScriptProcessorAssemblySpec` asserts the mechanism, not the bytes, against an oracle deliberately independent of the code under test; every new assertion was seen RED by mutation (restoring the bare split reddens the two write assertions; restoring the raw read reddens the two read assertions; blinding the shared scanner reddens nine). Recorded as characterisation, not repaired: `TRY_CAST` in a computed column puts the assignment inside a `catch` block -- the same strategy defeated by a block rather than a literal, loud, and identical before and after this change. sql 1450, softclient4es-sql-bridge 212, es6bridge 208, CI lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fupelaqu
changed the title
fupelaqu
changed the title
This was referenced Sep 20, 2026
Closed
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
1 participant
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.
Part of #373 — seven of its nine items, plus one residual found while triaging the other two.
LocalDate/ZonedDateTimecomparison) and 8 (a second identifier instance's narrowing islost) are not here — see What is not in this PR below.
N/9in each commit subject is a commit ordinal, not an issue item number. The mappingis in the table below; commit
8/9is not issue item 8.What is fixed
3d9f8e6f1/9CAST(d AS TIME) = CAST('00:00:00' AS TIME)usedisEqual, whichLocalTimedoes not have6e2b76a02/9WHENhad no null guard on its right operand908b58943/9YEAR(d)andMONTH(d)collapsed onto one parameterignore_failureswallows the throw and the computed column is simply absentbaa463ac4/99bb0ad7e5/9WHERE d > DATE_TRUNC(ts, MONTH)became `{"range":{"d":{"gt":"ts499f3a266/9YEAR(d) * 100 + MONTH(d)ran over raw doc-values3109c22f7/92df8172f8/9;inside a string literal was taken for a statement boundary, in both the write and the read of an ingest scriptEvery item was executed on real Elasticsearch 8.18.3 before and after, not reasoned about.
The residual is worth reading on its own
ScriptProcessor.fromScriptassembles an ingest script by splitting the rendered Painless andassigning the last statement to the computed column. It split on a bare
;, so a;inside astring literal in the last statement cut the expression in half and the assignment was written
into the literal:
The third is valid Painless that computes a value and discards it — nothing throws, so
ignore_failurenever comes into it and the document is stored without the column. A;as aREPLACEneedle needs no contrived input at all.🔴 Independent review found that fixing only that half would have shipped a new silent defect.
ScriptTarget.ofrecovers which column a script feeds by matching the lastctx.<name> =, and is;-naive in the same way — soCONCAT(name, '; ctx.d = 1')made columnc's processor reportitself as column
d's.IngestPipeline.diffkeys processors by exactly that, and aMapdropsthe loser of a collision: the ALTER rewrote the sibling's processor and never added the new column.
It is reachable only because the write fix lets that table be created at all — before it, the
emission was a compile error. Both halves now share one scanner, so they cannot drift apart.
What is not in this PR
Items 1 and 8 go to their own PR, by the lead's decision. They share a root cause — the
rendering an operand needs is not what its identity records — and both touch parameter identity,
the mechanism #370 has just stabilised with seven rules. Item 8 was confirmed live during this
work, with both a loud and a silent manifestation:
An unrelated
CAST(… AS DATE)comparison truncates the projected value of the same column,because a narrowing can arrive from coercion — which is not part of the chain, and so is invisible
to
contextKey. Two bare instances of one column therefore have identical keys by constructionwhile needing different renderings.
Residuals recorded, not fixed
Found while triaging, all verified pre-existing against a control clone of the merge-base and
deliberately left out of scope. They are tracked outside this PR:
CREATE TABLE … SCRIPT AS (…)never validates that the columns it references exist.validate()returnsRight(())forYEAR(nosuch)andn + nosuch. Two symptoms, one cause:the reference is emitted verbatim, and an undeclared column takes the wrong temporal parse
(
ZonedDateTime.parserefuses the date-only value aDATEcolumn carries, where a declared onecorrectly gets
LocalDate.parse). Nesting is irrelevant — declaredness is the variable.(CASE … END) + 1returns 1 on both branches — the CASE result is not parenthesised and+binds tighter than
?:in Painless. Silent.NULLIF(n, 1) + 1puts the operator outside the null guard — NPE on exactly the inputNULLIFexists to produce.GREATEST/LEASTreturn a Double for an INTEGER column —Math.maxover adefresolvesto the double overload.
BETWEENwith a column bound fails in three different ways (client-side throw with noschema, for every type including numeric; a
ZonedDateTimerelational comparison with one; anda
rangequery carrying a Painless expression as a string bound in the mixed form).TRY_CASTin a computed column has never worked — the assignment lands inside thecatchblock. Loud, identical before and after; pinned as characterisation so it cannot change silently.
Its real remedy is structural: hand
fromScriptthe prologue and expression separately insteadof re-deriving the boundary from their concatenation.
DATE_FORMATpatterns bypassescapePainlessString, so a backslash-terminated pattern emitsan unterminated literal. Loud — and it is the reachable path that makes the new scanner's
under-splitting safety argument necessary; that argument is recorded in the scaladoc because it
is load-bearing.
Verification
sql1450,softclient4es-sql-bridge212,es6bridge208, all green; CI lint lineclean.
residual above is byte-identical on both sides, so none of them is a regression from this branch.
split reddens the two write assertions; restoring the raw read reddens the two read assertions;
blinding the shared scanner reddens nine.
splitStatementsis byte-identical toString.split(";")wherever no literal is involved, so noexisting emission moves — proved exhaustively over the literal-free alphabet
{a, b, ;, \}up tolength 7 (21,845 strings, zero mismatches).
firstStatementAtwas proved unchanged over 411,111inputs and keeps its early exit.
independent of the code under test.
SQL parsing cost
Measured against an interleaved control of the merge-base, 5 rounds each, same session:
DDL CREATE TABLE(median/parse)Both ranges overlap fully, so there is no signal — the medians differ by less than the
round-to-round spread. Measured because
splitStatementsandwithoutLiteralsare per-statementtext scanners on the DDL path, which is exactly the class of change that gets forgotten.
Release notes
need their expectations updated.
Re-run
CREATE TABLE/ALTER … SET SCRIPT ASand reindex for affected tables.🤖 Generated with Claude Code