Repository navigation
fix(sql): TRY_CAST in a computed column, and the type rules the assembly exposed (#382) - #390
Merged
Merged
Conversation
) `TRY_CAST` / `SAFE_CAST` in a computed column had never worked, in any shape. `ScriptProcessor.fromScript` concatenated the rendered prologue and the rendered expression, split the result on `;` and called the last part the expression — so a safe cast, which renders a `try`/`catch` STATEMENT, put the assignment inside the `catch` with no right-hand side: ... catch (Exception e) { return null; ctx.c = } Elasticsearch answers `compile error`, so `CREATE TABLE` itself failed and no document could be indexed. The literal half of the same re-derivation was #373/#375; this is the block half, and the cure for both is to stop throwing the boundary away and then hunting for it. Two edits, and neither alone fixes it: * `function/convert` hoists a safe cast into the prologue in EVERY context that has one, not only in a query. #367 restricted that to QUERY because a processor script is persisted and diffed — an argument that held only while a safe cast in a computed column was broken anyway. No stored processor can churn under the change: such a table could never be created. * `ScriptTarget.assemble(prologue, expression, column)` is HANDED the two halves. Byte-preserving, and that is a contract — `ScriptProcessor.source` lives in `_meta` and `IngestPipeline.diff` compares it. One rule reproduces both of the old branches, including the leading space of a prologue-less script. Found by the new round-trip assertion, and fixed here because writing and reading must stay in step: `ScriptTarget.of` keyed the assignment on `^` or `;` only, so it answered `None` for every hoisted script — a processor with no identity, which `IngestPipeline.diff` turns into ALTER churn. A prologue entry may be a BLOCK, so the separator set is "a statement boundary". `PainlessResidualsSpec`'s "fall back to the parameter when a rendering is a STATEMENT" ASSERTED the defect (the conversion was silently dropped, so `TRY_CAST(name AS DOUBLE) + 1` computed on the raw string). Rewritten to assert the fixed emission; the scanner's true-positive claim it also carried is re-pinned where it is still true, on a context-free rendering. `IngestScript.assemble` publishes the assembly for softclient4es-extensions, which carries a third copy of it. `ScriptTarget` stays `private[schema]`. Guards: 6 new rows in `ScriptProcessorAssemblySpec` (the assignment outside every block, an EXPRESSION never a statement, one readable assignment, the hoist itself, a byte pin, and a 13-shape sweep proving every contexted processor rendering is placeable), the rewritten residual, and cluster-side rows in `GatewayApiIntegrationSpec` — a byte pin proves the bytes did not move, not that Elasticsearch runs them. Mutation-proved, one decision at a time: restoring the Query-only hoist reddens 5 named tests, restoring the splitter reddens 3, and reverting the `ScriptTarget.of` separator reddens 1 — disjoint sets. sql 1520/1520, bridge 212/212, core 1141/1141. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…382) The lead's OQ-2 ruling. `SCRIPT AS (CAST(raw AS BIGINT) + m)` over a KEYWORD column stored 1257 where 132 was asked for: Painless CONCATENATED the two operands and a BIGINT mapping coerced the string happily. HTTP 200 throughout — the #205 silent-wrong-answer family, and reachable today with a plain CAST, so it is neither caused nor fixed by the assembly repair. Mechanism, confirmed by construction rather than from the two samples on the issue (whose anchor named the SOURCE side, which #367 had already fixed): CAST(name AS BIGINT) + m argTypes = List(KEYWORD, BIGINT) out = VARCHAR left out = KEYWORD chainType = BIGINT `FunctionN.argTypes` answers `_.out`, and a schema-resolved `Identifier.out` reports the COLUMN's type, not the chain's — so `leastCommonSuperType` came out VARCHAR and `coerce`'s `(_, _: SQLLiteral)` arm wrapped both operands in `String.valueOf`. The rule was already written one scaladoc below: "Coerced FROM what the operand RENDERS, not from its column's type" (#367). It was applied to the FROM and never to the TO. Repaired at `argTypes`, not at a second local: `argTypes` is the single input to `baseType`, `baseType` feeds `out`, and `out` is the coercion target in BOTH renderings — `toPainless` for the nullable path and `painless` for the other. One derivation fixes both; a local would have left `painless` broken. NOT a DDL defect: the same `out` is the target in a PREDICATE and in a PROJECTION, and both were measured concatenating. All three venues are asserted, and the DDL and query venues are EXECUTED against real Elasticsearch — an emission that merely changed is not evidence, the arithmetic has to produce the number.⚠️ Recorded rather than smuggled in: the SOURCE-side derivation still reads `other.baseType` where the new one reads `other.out`. Unifying them is arguably more accurate, but a mutation restoring `baseType` in `argTypeOf` leaves the ENTIRE estate green — nothing distinguishes the two for a non-`Identifier` operand — so it stays an unguarded change and is not made. Mutation-proved: restoring `args.map(_.out)` reddens exactly the two new assertions. Blast radius measured, not assumed: sql 1524/1524, bridge 212/212, es6 bridge 208/208, core 1141/1141 — no other expectation moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…left behind (#382) The spec's three residual tasks. 1. `fromScript`'s `case Array(single) if single.trim.startsWith("return ")` arm is REMOVED, not kept beside a live one (#293's lesson). It was dead: `sql/src/main` has exactly ONE emitter of the text `return ` — the safe cast's `catch (Exception e) { return null; }` — and that is the CONTEXT-FREE arm, while `fromScript` always supplies a context. The reasoning is not the guard: the 13-shape sweep in `ScriptProcessorAssemblySpec` now asserts the PROPERTY, that no assembled source contains `return ` at all. A `return` in a processor leaves the whole script, so it can never carry a computed column's value. 2. `PainlessOperandForm.splitStatements` loses its only production caller. Grepped first, as the spec asked: the remaining consumers are two specs. It is KEPT and its scaladoc rewritten to say so — it is the executable specification of the literal rule `firstStatementAt` and `withoutLiterals` still enforce (`PainlessExpressionFormSpec` pins its agreement with `String.split(";")` over a literal-free alphabet; `PainlessLiteralEscaping- Spec` uses it as #383's injection differential). Deleting it would take those two proofs with it while the rule they describe stays live. Flagged in the PR body as a lead decision rather than settled here. 3. The post-hoist `placeable` sweep asked for by the spec: MEASURED CLEAN. Thirteen computed-column shapes — safe cast, CASE, arithmetic locals, function chains, literal-bearing scripts — all render an EXPRESSION once the prologue is hoisted, so no LOUD DDL-time rejection is needed. The sweep IS the guard: a future emitter that renders a statement reddens here instead of in a customer's cluster. sql 1524/1524, bridge 212/212, es6 bridge 208/208, core 1141/1141, and the es8 integration leg 87/87 against real Elasticsearch 8.18.3 — including the four #382 rows, which are what say Elasticsearch compiles and RUNS the script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…382) Documents what the fix makes possible, in the two pages that own it, with only what was MEASURED on real Elasticsearch 8.18.3: * a safe cast in a computed column converts what it can and stores NO value for a row it cannot convert — the document is always indexed; * arithmetic over a cast of a string column now COMPUTES. It concatenated before (`'125' + 7` stored 1257, not 132), silently, in a computed column, a WHERE and a projection alike — so tables built from that shape on an earlier version need re-running and reindexing. Both notes say "before 0.24.0" rather than claiming the behaviour was ever documented: `TRY_CAST` in a computed column had never worked in any shape, and the concatenation was never written down. No MDX twin in this repo (there is none); the softclient4es-web page is a separate repo and is recorded as a follow-up rather than edited from here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lliding processor (#382) Two defects found by independent review of this branch, both in the half of #382 that recovers "which column does this ingest script feed". REGRESSION, introduced here. #382 widened `ScriptTarget.of`'s statement-boundary set with `}`, because a hoisted `try { ... } catch (Exception e) {} ` is a statement with no `;`. A `}` inside a Painless COMMENT was then read as code: ctx.c = 1 <block comment containing "} ctx.zzz = 9"> before: Some(c) after: Some(zzz) ctx.c = 1 // } ctx.zzz = 9 before: Some(c) after: Some(zzz) A wrong identity is a COLLISION, and `IngestPipeline.diff` keys processors by it. Fixed at the seam that already answers the same question for string literals -- `PainlessOperandForm.scanOutsideLiterals` now skips line and block comments -- so the two answers cannot drift. Two shapes whose comment PRECEDES the assignment used to answer `None`; they answer correctly now. Comments are reachable from SQL today: `ALTER PIPELINE ... ADD PROCESSOR SCRIPT(source = '...')` and `CREATE PIPELINE ... WITH PROCESSORS (SCRIPT(...))` both take a hand-authored source. THE COLLISION, and it was worse than reported. `IngestPipeline.diff` indexed processors with `Seq.toMap`, which DROPS the loser of a key collision. A generated processor for column `c` beside a hand-authored `... ctx.c = 2` share a recovered identity -- correctly, both assign `c`. Measured on the new guard with `keyed` reverted to `toMap`: declared processor UNCHANGED [ProcessorChanged] <- the ALTER would REWRITE the declared processor with the foreign source declared processor CHANGED [ProcessorChanged] <- 1 diff; the foreign removal LOST Repaired where the drop happens: a colliding member falls back to `p.column` -- the identity it had before the recovery, already content-addressed (`anonymous_<hash>`) for a processor this codebase did not write -- and a residual collision after that gets a deterministic content ordinal, so nothing is ever dropped. Disambiguating the colliding group by CONTENT looked right and is WRONG: only ONE side collides, so the generated processor gained a suffix its unchanged counterpart did not and a change became an add plus two removals (measured). That is why the fallback is `p.column`. Keys that do not collide are untouched, so the 21.8 Part F round trip stays an identity (asserted). RECORDED BOUNDARY, asserted: on ES 6.8 a stored processor has no `description`, so the generated one reads back anonymous and a collision churns (remove + add) instead of reporting a change. Nothing is dropped, which is what this repair is for; telling them apart on 6.8 needs content matching -- a heuristic and a lead call. The collision CLASS pre-existed (a foreign source ending `...; ctx.c = 2` collided before #382); #382 enlarged it. Both halves close here. No test covered a generated-versus-FOREIGN collision -- the existing one uses two generated sources. Mutations seen RED: removing the comment handling reddens the new `ScriptProcessorAssemblySpec` block; reverting `keyed` to `toMap` reddens the two new `PipelineRoundTripIdentitySpec` rows.
…382) The lead's Ruling B. A `TRY_CAST` hoists `def safe1 = null; try { safe1 = ...; } catch (Exception e) {}` into the prologue, so the value the call reads is null exactly when the conversion FAILED -- while the COLUMN it came from is perfectly non-null. `FunctionN.painless` guarded the column's parameter and nothing else, so the null went straight through. MEASURED on real Elasticsearch 8.18.3 over `s = 'abc'`: CONCAT(TRY_CAST(s AS BIGINT), 'x') stored the literal text "nullx" CONCAT('x', TRY_CAST(s AS BIGINT)) stored "xnull" ABS / ROUND / SUBSTRING / TRIM / LOWER / REPLACE / LENGTH over it NPE, swallowed by ignore_failure => column ABSENT The first two are a wrong VALUE that Elasticsearch accepts (Painless `String.valueOf(null)`), not an absent column. ONE guard covers all nine shapes: the null check now also tests the reference the CALL reads, scoped to the new `Function.rendersNullOnFailure` -- overridden only by `convert.Conversion`, as its `safe` flag. The semantics are the engine's OWN, not a third set: `CONCAT(s, 'x')` over a null `s` already emitted `(param1 == null) ? null : ...` -- ANSI, a NULL operand makes the whole call NULL. A failed safe cast IS a null operand, so it answers the same way. The dialects that SKIP a null fragment were considered and rejected: this engine has one rule for a null operand and a second would have to be explained at every venue. The raw parameter is KEPT beside the derived one, and that is load-bearing. A prologue declaration is evaluated UNCONDITIONALLY, so a derived reference cannot stand in for its source where the derivation itself throws on a null -- the ingest temporal base (`param1 instanceof String ? LocalDate.parse(param1, ...) : Instant.ofEpochMilli(param1)`) NPEs before any guard is reached. Replacing rather than adding reddened `IngestTemporalSpec` and `DateDocValueEmissionSpec` (measured), which is why the scope exists. `Function.rendersNullOnFailure` is declared on `Function` rather than pattern-matched on `convert.Conversion` in `FunctionN`: the package object that hosts `FunctionN` is the PARENT of `function.convert`, and a parent reaching into a child for a type test gets copied. Both mutations seen RED: `rendersNullOnFailure = false` reddens 2 of the new `SafeCastNullPropagationSpec` tests; removing the scope (guarding every argument) reddens its byte control PLUS `IngestTemporalSpec` and `DateDocValueEmissionSpec`. Executed on real ES 8.18.3: new `GatewayApiIntegrationSpec` row "make a failed safe cast NULL, not the text \"null\"", which also measures what the documentation promises about a NULL computed column -- a plain SELECT returns the row with an empty value, `IS NOT NULL` skips it. RECORDED, NOT FIXED: `NULLIF` over ANY cast still fails `CREATE TABLE` with a script compile error. Measured as pre-existing -- `NULLIF(CAST(s AS BIGINT), 0)` fails identically while `NULLIF(n, 0)` works -- so it belongs to `NULLIF` over a cast, not to #382. Pinned on its cause.
…, and correct three overstatements (#382) The lead's Ruling A. #382's OQ-2 repair (33fa60b) moved an `Identifier` operand's declared type from the COLUMN's (`out`) to the CHAIN's (`chainType`). That also moves the coercion TARGET for a cast of a NUMERIC column, which the issue never mentioned. MEASURED on real Elasticsearch 8.18.3 with `x DOUBLE = 5.0`: emitted right-hand side SELECT CAST(x AS INTEGER) / 2 975aa87 (lv1 / ((double) 2)) 2.5 this branch (lv1 / 2) 2 and `WHERE CAST(x AS INTEGER)/2 > 2` returns a DIFFERENT SET of rows, HTTP 200 both ways. The computed-column venue moves identically. The lead ruled `= 2` correct -- the cast says what the operand IS, so the arithmetic follows it -- and the two venues must agree. It ships, with the three things it was missing. 1. IT IS GUARDED NOW. Mutating `argTypeOf` back to `args.map(_.out)` used to redden only the 2 OQ-2 assertions; it now reddens 5, including narrowing and widening pins in all three venues (computed column, projection, predicate), and the two zero-movement controls stay GREEN under the mutation, which is what makes them controls. New `GatewayApiIntegrationSpec` row "make a narrowing cast decide the arithmetic, in BOTH venues" executes it on a cluster -- an emission that merely changed is not evidence. 2. THE FALSE TEST IS GONE. "should leave arithmetic over a numeric source byte-identical" pinned two shapes that happen not to move. A differential probe -- this branch against a control whose only difference is `argTypeOf` -- over 1,792 emissions says what is true: no cast at all 128 rows, 0 moved cast to the column's OWN type 104 rows, 0 moved cast to a DIFFERENT type 596 rows moved i.e. a cast whose target DIFFERS from the column's declared type now decides the arithmetic, in both directions. The test states that, and the two zero-movement families are its controls. 3. THE DOCUMENTATION IS CORRECTED. It scoped the change to STRING columns; it now covers the numeric narrowing and widening directions and says a `WHERE` over such an expression matches a different set of rows. RECORDED for the lead, found by the wider probe and named by nobody so far: a numeric column cast to a STRING now CONCATENATES where the string used to be silently parsed BACK to a number -- `CAST(n AS KEYWORD) + 2` computed arithmetic before. It is self-consistent (the arithmetic follows the cast in this direction too) and under `-`, `*`, `/` it becomes a runtime failure on a String receiver instead of a silently un-done cast. Pinned as CHARACTERISATION, not endorsed. Three documentation overstatements, all measured false: - "It works in EVERY shape from 0.24.0 on" -> replaced by the NULLIF-over-a-cast known gap; - "nothing is rejected and nothing is lost" -> true only because the generated processor carries `ignore_failure: true`; the doc now says so, with the consequence of turning it off; - "stored with NO VALUE ... SELECT zip_n returns nothing" -> the `_source` carries `"zip_n": null` and no doc value is written. The doc now states both halves, and the Ruling B integration row measures them on a cluster instead of claiming them.
… safely (#382) Two defects, both PRE-EXISTING on `main`, both in `NullIf.toPainlessCall`, both one question asked of two operands: what does this argument RENDER, and may it be spliced where it is spliced? MEASURED on real Elasticsearch 8.18.3. N1 -- THE TYPE. `NullIf.argTypes` answered `_.out`, and a schema-resolved `Identifier.out` reports the COLUMN's type, not the chain's. So over a KEYWORD column `NULLIF(CAST(s AS BIGINT), 0)` reported `List(KEYWORD, BIGINT)`, `leastCommonSuperType` came out VARCHAR, and the string arm guarded a primitive literal: def param3 = param2 == null || (0 != null && param2.compareTo(0) == 0) ? null : param2; class_cast_exception: Cannot cast from [int] to [java.lang.Object]. `CREATE TABLE` itself failed; `CAST`, `TRY_CAST` and `::` failed identically, and so did the projection and predicate venues. This is OQ-2's root cause in a second function -- and the file already knew the rule three lines above, where `receiverIsTime` deliberately reads `chainType` for exactly this reason (#367). Repaired at `argTypes` rather than at a local, because `argTypes` is the single input to `baseType`, `baseType` feeds `out`, and `out` is what the emission switches on -- so one derivation fixes every venue, and it also stops the expression describing itself as a string to a CTAS target mapping. DECISION, and why NOT the alternative. The addendum offered "emit the `$arg1 != null` guard only when `arg1` is nullable" instead. Measured and rejected: it does not reach `NULLIF(s, 0)`, where the operand really IS a KEYWORD, and there it turns a compile error into a PER-DOCUMENT runtime failure (`wrong_method_type_exception: cannot convert MethodHandle(String,String)int to (Object,int)Object`). That shape is a TYPE MISMATCH; `NullIf.validate()` already computes exactly the right `Left` for it and never fires, because validation runs at PARSE time, before a schema is attached, when `s` has no type at all. RECORDED on #382, not widened into the validator here, and pinned as a characterisation so it reddens when it is fixed. N2 -- THE FORM. `arg0` was spliced RAW three times into `$arg0 == null || ($arg1 != null && $comparison) ? null : $arg0` and `arg1` twice, and `&&`, `||` and `==` all bind tighter than `?:`: NULLIF(UPPER(s), 'X') Cannot cast null to a primitive type [boolean]. LOUD NULLIF(CASE WHEN n > 1 THEN 1 ELSE 2 END, 1) param2 ? 1 : 2 == 1 ? null : param2 ? 1 : 2 reads as `param2 ? 1 : ((2 == 1) ? …)`, so n = 5 stored c = 1 where SQL says NULL -- HTTP 200, the #205 SILENT family. The silent one is why the repair reaches the NUMERIC arm too: that arm is correct only when the compound rendering happens to be a `guard ? null : value`, which is luck, not a rule. Both templates now read their operands from ONE place. A compound rendering is bound to a prologue local via `bindLocal`, which fixes the association AND the triple evaluation (`toUpperCase()` was called three times per document). A NAME or a LITERAL is left exactly as it was, which is what keeps the working population byte-identical. With NO context there is no prologue: an expression gets parentheses, and a rendering that is not an expression at all (a safe cast's try/catch) is left alone. BLAST RADIUS, measured against a `git clone --local` control at this branch's previous HEAD over 618 expressions x 4 venues = 2,460 cells. In the three venues production uses, every ✅ shape the addendum listed is byte-identical except two, and both are stated rather than hidden: - `NULLIF(LENGTH(s), 0)` gains the bound local (it was correct, evaluated twice); - `NULLIF(CAST(s AS BIGINT), n)` moves from the string arm to the numeric one -- the arm its corrected types name, and the one `NULLIF(n, 0)` always used. 23 further expressions move from the numeric arm to the string one; every one of them is a genuine type mismatch of the `NULLIF(s, 0)` family, i.e. the residual recorded above, and none is legitimate SQL. Mutation-proved, each seen RED: restoring `args.map(_.out)` reddens 3 tests; returning the operand unbound reddens 3; binding unconditionally reddens 7 (including both byte-stability pins); binding INLINE instead of into the prologue reddens 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…at settle them (#382) `NullIfOperandSpec` covers both halves of the repair in all three venues (computed column, projection, predicate), and the previous round's characterisation is flipped rather than deleted: it published *"Known gap: NULLIF over any cast -- safe or plain -- is still rejected at CREATE TABLE"*, and a characterisation that survives its own fix is a test MANDATING a defect. It still asserts the same invariant -- that the two cast spellings agree -- which is what makes `TRY_CAST` no worse than `CAST`. What each test is for, and the mutation that reddens it: numeric arm over a cast (6 shapes x 3 venues) argTypes -> args.map(_.out) no `0 != null` guard on a literal argTypes -> args.map(_.out) `NULLIF(s, 0)` still string-armed (RECORDED) binding unconditionally bind a function operand, evaluate it ONCE operand -> unbound bind on the NUMERIC arm too (the SILENT one) operand -> unbound bind a compound SECOND argument operand -> unbound bind neither a name nor a literal binding unconditionally every `def` in the prologue, declared once binding INLINE the working population, byte-exact binding unconditionally the temporal spellings, byte-exact binding unconditionally `countOf(emitted, "toUpperCase()") shouldBe 1` is the "evaluated once" assertion, the shape `PainlessResidualsSpec` already uses for #373 item 9. 🔴 The two `GatewayApiIntegrationSpec` rows are the ones that matter, because a byte pin cannot say a script COMPILES and cannot see a wrong VALUE that Elasticsearch accepts. `nullif_cast` covers N1 (and asserts the `v` column, whose bytes moved while already working); `nullif_fn` covers N2, and its `w` column is the silent shape -- `NULLIF(CASE WHEN n > 1 THEN 1 ELSE 2 END, 1)` over `n = 5` stored `1` where SQL says NULL.⚠️ The testcontainers leg (`es8java/testOnly *JavaClientGatewayApiSpec*`) did NOT run: the Docker daemon on this machine is wedged (an Elasticsearch container stuck in `Created`; `docker ps` answers, `docker inspect` on it times out), which needs a Docker Desktop restart nobody should do silently. The claims are executed nonetheless, on a live Elasticsearch 8.18.3, by taking each computed column's emitted `ScriptProcessor.source` and building the same pipeline, mapping and fixture rows by hand -- against this branch AND against a control clone at the previous HEAD: control every one of the four pipelines rejected, HTTP 400 `compile error` branch all four accepted, and nullif_cast c/t = {1:125, 3:42}, id 2 null; v = {1:125, 2:0}, id 3 null nullif_fn id 1 u=null l=1 w=null; id 2 u='YY' l=null w=2 `EmissionCostProbe` gains the two NULLIF argument forms -- a name and a rendering that has to be hoisted -- because they are now separate shapes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ished gap is gone (#382) Three corrections, each measured by PARSING the published example and EXECUTING its emission on a live Elasticsearch 8.18.3. 1. `functions_conditional.md` example 4 -- `NULLIF(TRIM(description), '')` -- was N2's shape exactly: the engine REJECTED it. This repo has a standing defect class of publishing examples the engine refuses (#302, #261), so it was checked rather than assumed. It now runs: over `' hello '` it stores `hello`, over `' '` it stores NULL. The other three NULLIF examples in that file, the syntax line in `dql_statements.md` and all three examples in `core/src/main/resources/help/functions/conditional/nullif.json` parse and emit scripts Elasticsearch accepts, before and after -- nothing to correct. 2. `functions_type_conversion.md` published *"Known gap in 0.24.0: NULLIF over a cast -- safe or plain -- is still rejected at CREATE TABLE"*. That gap is closed, so the note is replaced by what is now true, with a pointer to the one shape still refused (a value that really is text compared with a number). 3. `functions_conditional.md` and `ddl_statements.md` gain the behaviour a user has to know: the comparison follows what each argument RENDERS, an argument is evaluated once, the two arguments must be comparable, and `expr2` being NULL is not a match. RECORDED, not fixed (out of this round's scope): `operators.md` publishes six NULLIF examples, of which `SELECT total / NULLIF(count, 0) AS avg FROM table` and `SELECT COALESCE(total / NULLIF(cnt, 0), 0) AS avg` are PARSE REJECTIONS (reserved words, and a division inside COALESCE), and `SUM(total) / NULLIF(COUNT(*), 0)` emits a degenerate script from an aggregate in a script venue. All three are pre-existing and unrelated to the emission this PR repairs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
) `simpleOperand` was a `val` on the `NullIf` case class, so the `Regex` was compiled afresh for every NULLIF node the parser builds. Hoisted to the package object beside the other shared derivations; no behaviour change (the same 14 assertions stay green, the emission is byte-identical). Measured before the move and after: the interleaved cost comparison against a control shows no signal either way -- this is hygiene, not a fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LEAD RULING of 2026-09-23, on the regression an independent delta review found
at the edge of this issue's type fix. That fix made `NullIf.argTypes` read what
an operand RENDERS, which is right -- but for a genuine TEXT-against-NUMBER pair
the corrected types send the comparison down the STRING arm, whose `compareTo`
throws per document. `ScriptProcessor.ignoreFailure` defaults to `true`, so in a
computed column the throw is SWALLOWED and the column silently disappears while
`CREATE TABLE` answers 200. Measured on ES 8.18.3:
CREATE TABLE t (s KEYWORD, c BIGINT SCRIPT AS (NULLIF(CAST(s AS BIGINT), s)))
doc {"s":"125"} -> {"s":"125"} -- c GONE, HTTP 200
Do not emit a script that throws per document. A text-vs-number NULLIF is a TYPE
ERROR, so the engine says so, naming the expression, the column and both rendered
types, before anything is deployed or sent.
The seam already existed and is the one issue #384 built: `validateResolved()`
for the query venues (called at `SearchApi.resolveWithSchema`) and
`validateScriptReferences` for DDL, both of which already carry a rule of exactly
this shape and both of which run on a RESOLVED statement -- which is the whole
point, because at parse time every column is `Any` and `NULLIF(s, 0)` is
indistinguishable from `NULLIF(n, 0)`. No generic re-validation was added.
`Case.conditionsOf`'s walk is extracted as `cond.functionsOf` and shared, so a
NULLIF inside ABS(...), a CASE branch, COALESCE or another NULLIF's argument is
the same NULLIF -- two walkers would be two answers to one question.
The rule is deliberately NARROW (lead): exactly TEXT against NUMBER. A
`SQLTypeUtils.matches` version was implemented and MEASURED to refuse 540 further
shapes in the same corpus, temporal-against-number and every boolean pair among
them; those emit today exactly what they emitted before and are recorded as a
boundary. A cast can CREATE the mismatch as well as cure it, so
`NULLIF(CAST(n AS KEYWORD), 0)` -- which worked and stored a value -- is refused.
A NULL operand is never refused, structurally: `SQLTypes.Null` is neither an
`SQLVarchar` nor an `SQLNumeric`, so an explicit short-circuit was dead and was
removed rather than shipped.
One edge no type rule can decide: `NULLIF(d, '2025-01-01')` and `NULLIF(d, 'x')`
are both TIMESTAMP against VARCHAR. The verdict comes from #276's
`TemporalLiterals.normalizeLiteral`, against the column's own mapping `format` --
the same machinery a WHERE clause asks, so the engine has ONE date-literal
grammar. Only the verdict is used; NULLIF does not rewrite its operand.
Second half of the repair, and the validator alone does NOT close the regression:
`String.compareTo` takes a String and nothing else, while `leastCommonSuperType`
answers VARCHAR for a MIXED pair too ([BOOLEAN, KEYWORD], [TIMESTAMP, KEYWORD]).
Those are not text-vs-number, so they are not refused -- and the type fix had
moved them into the `compareTo` arm, where they threw per document: 32 corpus
rows, every one of which answered a VALUE on `origin/main`. The string arm is now
guarded by `argTypes.forall(_.isInstanceOf[SQLVarchar])`; a mixed pair falls to
the `==` arm, which is what it emitted before.
Blast radius, measured over 816 NULLIF expressions x 3 venues = 2,448 cells,
emitted on a control clone at `origin/main` 975aa87 and EXECUTED on a real ES
8.18.3: 376 rows newly rejected (192 expressions), 358 by the type rule and 18 by
the temporal resolver, and NO third rejection reason anywhere in the corpus.
Against main: 483 broken -> working, 109 working -> a LOUD rejection, ZERO
working -> broken, and the only 3 value changes are this issue's own silent wrong
answer being corrected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ruling's two lists, asserted in all three venues, plus the invariants the
delta review asked to be established over a CORPUS rather than over a table --
its MEDIUM-1: the six `0 != null` shapes that survived the previous round were
absent from every table in this file, so every assertion here was green while
the defect shipped.
Corpus: every ordered pair of 28 operands (a bare column of each mapped type,
CAST / TRY_CAST / ::, a function, a literal of each kind, NULL), in all three
venues, and the invariants are:
- never guard a numeric literal against null, over ANY numeric literal;
- emit `compareTo` only when BOTH operands render text -- read off the AST,
not off the emitted string, so it cannot agree with the emission by
construction;
- only ever bind an operand rendering that is a Painless EXPRESSION. This is
the invariant `NullIf.operand`'s scaladoc STATES (review LOW-1): with a
context, a safe cast has already hoisted itself, so what arrives is a
parameter NAME. The scaladoc claimed parity with `ArithmeticExpression`'s
`bind`, which it does not have; it now states what holds and why, and this
asserts it instead of asking the reader to believe it.
A rejection test is unfalsifiable while any `Left` passes it (story 21.4), so
every rejection row asserts that the message names BOTH rendered types, is not
an internal-error label, and -- in DDL -- names the expression and the column.
The executed row asserts the ERROR, not a stored value: the DDL is refused, the
index is NOT created, the two query venues are refused the same way, and the
temporal edge runs against a real `date` mapping (`NULLIF(created,
'2024-01-15')` accepted, `NULLIF(created, 'yesterday')` refused naming the
literal and the field). Green on real ES 8.18.3 (92/92 for the whole suite) and
on ES 6.8.23.
Two mutations came back GREEN first and both were coverage holes, not harness
faults. Restoring `argTypes = args.map(_.out)` left every other assertion green
while moving 57 corpus rows -- a numeric column cast to TEXT must take the
STRING arm, and the new `compareTo` guard happens to give the same answer for
the headline shape. Relaxing `mappedColumn` to accept a function-wrapped operand
was green until the CAST-beside-a-literal row existed. Both rows were added;
all eight mutations are now RED and each names the assertion that guards it.
Also recorded here, byte-identical to `origin/main` and NOT fixed:
`NULLIF(0, NULL)` renders `0 == null`, the same `class_cast_exception` the guard
this issue removed produced. Degenerate SQL -- a constant NULLIF a constant --
found by widening the numeric-literal guard to the corpus, and the ruling never
rejects on a NULL operand. The pin reddens when it is fixed.
And the stale cross-reference the review found (LOW-2): the integration row is
called "compile and run NULLIF over a cast, and compute (#382 N1)".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note the previous round added generalised "the two arguments must be comparable" to "Elasticsearch rejects the script it generates" (delta review MEDIUM-2). That is no longer what happens: the ENGINE rejects it, earlier and with a message the reader can act on, and the note now says so for both venues -- and says what the old behaviour was, because in a computed column it was not a rejection at all but a column silently missing from every document while `CREATE TABLE` answered 200. Adds the two rules the ruling introduces that a user cannot guess: a cast can CREATE the mismatch as well as cure it (`NULLIF(CAST(qty AS KEYWORD), 0)` is refused even though `qty` is numeric), and a string literal compared with a `date` column is checked against that column's mapping `format`, exactly as in a WHERE clause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion (#382) An operand's type is what it RENDERS, not what its column is declared as. That rule fixes the silent concatenation (`CAST(amount_str AS BIGINT) + fee` stored 1257 instead of 132) and, by the same stroke, makes a narrowing cast decide the division: `CAST(price AS INTEGER) / 2` over 5.0 was 2.5 and is now 2. Records it where a reader meets it — the `/` operator's own section, beside the existing integer-vs-float examples — as a deliberate engine decision rather than a side effect, with where it puts us against other engines: PostgreSQL answers 2, MySQL and DuckDB answer 2.5 and spell truncation DIV and //. All three measured, not quoted. Also states the consequence nobody would guess: a query containing a JOIN evaluates its SELECT list in DuckDB (`JoinPlanner` hands it `identifier.sql`), so the same expression answers 2.5 there and 2 without a JOIN. Cast explicitly when a query mixes both paths. Refs #382 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lead's ruling of 2026-09-23. A division of numbers is FLOATING-POINT
whatever its operands are; the result type of a division is a property of the
OPERATOR, not of its arguments. `+`, `-`, `*` and `%` still derive theirs from
what the operands RENDER — the OQ-2 repair (`33fa60bc`) is untouched, and only
`/` is overridden.
Three defects close with it, all MEASURED on real Elasticsearch 8.18.3:
- `SELECT n / m` over two INTEGER columns answered 3 for 7/2, while the SAME
statement inside a JOIN answered 3.5 — `JoinPlanner` hands the SELECT list
to DuckDB, which like MySQL treats `/` as always-decimal. The divergence
therefore already existed on main for a plain `n / m`; aligning `/` removes
the class rather than one instance of it.
- `c DOUBLE SCRIPT AS (n / m)` emitted `param1 / param2` with NO coercion, so
a column the user DECLARED DOUBLE stored 3.0. The declared type was ignored.
- A zero divisor was never the NULL the documentation promises. Integer
division THREW (HTTP 400 in a search; the column silently ABSENT in an
ingest pipeline under `ignore_failure`), and FLOATING division produced
Infinity — which Elasticsearch refuses to index, so the WHOLE DOCUMENT was
rejected. That last one is a data-loss bug that predates this ruling for any
`double` column, and making integer division floating would have extended it
to integer columns. So the guard ships WITH the ruling, not after it.
Four derivations, each placed where it has to be rather than where it reads
best:
- `baseType` answers DOUBLE for `/` over a NUMERIC fold. A non-numeric fold
(`s / 2` over a KEYWORD column) is left exactly as it was, so the ruling
widens nothing outside arithmetic. The escape hatch is `out`, not a special
case: an explicit cast sets `out` and `floatingDivision` reads it.
- `needsDoubleCast` emits `((double) <left>) / <right>` INSIDE the null guard,
where the value is known non-null — a primitive cast over the guarded
expression is what Painless refuses. It fires ONLY when neither operand
already emits floating, because `ScriptProcessor.source` is persisted and
`IngestPipeline.diff` compares it: an unconditional cast would churn every
stored computed column that divides, including the ones that always
answered correctly.
- `divisorMayBeZero` proves a non-zero numeric LITERAL divisor safe, so `/ 2`
and `/ 100` emit no guard at all. 🔴 A literal operand is NOT a
`NumericValue` — `identifierWithValue` is `(value ^^ functionAsIdentifier)
>> cast`, so `/ 2` arrives as a `GenericIdentifier` with an EMPTY name
carrying `LongValue(2)`. The obvious spelling, written first, proved every
divisor un-provable and emitted `((double) 2) == 0` on every `/ 2`.
- the three hand-written guard branches collapse into ONE condition list,
which is what makes room for a third condition; the output is byte-identical
for every other operator by construction.
🔴 The non-nullable rendering needs a `(def)` on the live branch: Painless types
`cond ? null : <primitive>` as `Object`, so without it `SELECT 10 / 0` answered
`class_cast_exception: Cannot cast from [double] to [java.lang.Object]` instead
of NULL. Measured — the same trap story 21.8 hit with a primitive method outside
a null guard.
Blast radius measured, not assumed. A differential corpus of 5,070 emissions
(five operators x thirteen operand shapes x thirteen, in four venues plus the
expression's reported type) against a control clone at the previous HEAD: 726
cells move, EVERY one of them `/`; 0 of the 4,056 `+`/`-`/`*`/`%` cells move.
Executed on a real index, 47 of 81 value-cells move and every move is a value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#382) New `DivisionResultTypeSpec` — ten assertions over the mechanism (the type the expression REPORTS), the three production venues byte for byte, the zero guard, the shapes that must NOT move, and the shapes the grammar still refuses. Every one was SEEN RED by the mutation it guards, over eight mutations: M1 baseType drops the DIVIDE arm 7 assertions M2 divisorMayBeZero := false 4 M3 divisorMayBeZero := true 6 M4 needsDoubleCast := false 2 M5 drop the `(def)` on the live branch 1 M6 the DIVIDE arm also takes MODULO 1 M7 drop the numeric-fold guard 2 M8 MODULO joins both arms 2, incl. "leave every other operator byte-identical" 🔴 The `#267` characterisation row carries POSITIVE controls — `CAST(n AS INTEGER) / 2`, `FLOOR(x) / 2` and `ABS(n)` all parse — because "everything is rejected" would otherwise satisfy an assertion that everything is rejected. `ArithmeticCoercionTargetSpec`: the narrowing-cast row moves to `+`, and the division it used to own is pinned in its own row as FLOATING again. 🔴 It also records what the move revealed: for a NUMERIC column, `/` was the ONLY operator whose narrowing was visible in the ANSWER (`CAST(x AS INTEGER) + 2` over 5.7 is 6 under either target), so the `+` rows are emission pins by necessity and Ruling A's value-level evidence lives in the STRING-source rows. The blast-radius scaladoc is AMENDED rather than restated: its numbers stay as they were measured for the OQ-2 commit, with the division half marked superseded. Both `SQLQuerySpec` copies — the template and the hand-maintained es6 one — move one `div` script field to `(param1 / ((double) 2))`, using the `(double)(\d)` re-spacing rule the mathematic-function expectation in the same file already had. `GatewayApiIntegrationSpec` (inherited by all five clients), executed on real Elasticsearch 8.18.3: - "divide as SQL divides, not as Java does" — three rows, three inexact divisors, the computed column and the query and a predicate whose row set the truncating reading could not produce; `%` beside them as the control, and `CAST(c AS INTEGER)` as the documented truncation idiom. - "index the document a zero divisor used to destroy" — the INSERT itself is the assertion: on main the `x / 0` row is REJECTED by Elasticsearch. - the amended narrowing row, whose fixture (7.5) keeps it non-vacuous: the cast still narrows the OPERAND, so 3.5 against the uncast 3.75. 🔴 One existing pin moved, and it is the ruling itself: "fail loudly at execution on a broken constant script" used `SELECT 1/0`, which is no longer broken. It now uses `SELECT 1 % 0` — the operator the ruling deliberately left alone — and pins `SELECT 1/0 AS c` as NULL beside it, so the asymmetry is stated rather than lost. `EmissionCostProbe` gains the two division shapes, since a change to `/` is invisible to a corpus that never divides. sql 1562/1562, core 1141/1141, bridge 212/212, es6 bridge 208/208, JavaClientGatewayApiSpec 94/94. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewrites the `/` operator's section around the shipped rule: division is floating-point whatever its operands are, we match MySQL and DuckDB (PostgreSQL is the outlier that truncates), and the JOIN path — which evaluates the SELECT list in DuckDB and therefore already disagreed with us on `main` — now agrees. The "Integer vs Float Division" examples taught `10 / 3 -> 3`; they now teach what the engine answers.⚠️ There is no truncating-division operator and the page says so instead of implying one. `CAST(a / b AS INTEGER)`, `FLOOR(a / b)` and `ABS(a / b)` are PARSE ERRORS — an arithmetic expression is not yet accepted as the operand of a cast or of a function (#267) — so the documented way to truncate is to compute the quotient into a column and cast THAT column, which is executed in `GatewayApiIntegrationSpec` rather than asserted here. 🔴 The "Division by Zero Protection" section was worse than stale: EVERY guard it recommended is measured not to work. - `COALESCE(a / b, 0)` and `CASE WHEN b != 0 THEN a / b ELSE 0 END` are PARSE ERRORS — the same #267 family, which the issue does not say reaches `CASE`. - `a / NULLIF(b, 0)` parses and then throws `null_pointer_exception` in a search on exactly the rows where `b = 0`, and silently drops the computed column in an ingest pipeline. Identical on `main`; the `NULLIF` chain reports `nullable = false`, so the division emits no null guard for it. - `SELECT total / count …`, published on three pages, is a parse error: `count` is a reserved word. Since `0.24.0` none of them is needed for division: the engine answers NULL. That is documented with what it replaced — integer division threw (HTTP 400 in a search, a silently absent column in an ingest pipeline) and floating division produced `Infinity`, which Elasticsearch refuses to index, so the whole document was rejected. Two older limitations are written down rather than left to be rediscovered, both measured identically before and after: `ORDER BY <arithmetic over a nullable column>` fails, because the bridge emits a `number`-typed script sort and Elasticsearch rejects a sort script that can return null; and the `NULLIF` division idiom above. `%` gains its own note: the ruling covers `/` only, so `a % 0` still throws, and the page says where that surfaces in each venue. Also corrected: `operator_precedence.md` (the `10 / 3 -> 3` example and its `CASE` guard), `functions_type_conversion.md` (the cast is no longer what avoids integer division — it is what reads a number out of a text column), and `functions_conditional.md` (the `NULLIF` division idiom).⚠️ The MDX twins in `softclient4es-web` are NOT touched here and need the same changes (per `feedback_dual_docs_sync`). Refs #382 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Corrects an attribution in the three commits above, in the engine scaladoc, in `DivisionResultTypeSpec` and in `operators.md` at both sites. 🔴 #267 is a DIFFERENT defect and it is FIXED. Its subject is a bare LITERAL operand — *"CAST accepts no bare literal operand"* — it was CLOSED 2026-09-04, and `CAST('125' AS BIGINT)`, `TRY_CAST('125' AS BIGINT)` and `CONVERT('125', BIGINT)` all parse today. Citing a closed issue for a defect it never covered sends the next reader to the wrong place. The observation itself stands and is re-measured on a clean clone at `origin/main`; it is simply UNFILED, and it is wider than "division inside a cast": CAST(n / m AS INTEGER) REJECT '(binary|varbinary)' expected but '/' found CAST(n + m AS INTEGER) REJECT <- ANY arithmetic, not just division CAST((n / m) AS INTEGER) REJECT <- parenthesising does NOT help FLOOR(n / m) ABS(n / m) REJECT ')' expected but '/' found COALESCE(n / m, 0) REJECT CASE WHEN n > 1 THEN n / m ... END REJECT '(END)\b' expected but '/' found FLOOR(x) PARSES <- control total / NULLIF(cnt, 0) PARSES <- arithmetic OVER a function is fine So the rule is DIRECTIONAL — `f(<arithmetic>)` is rejected, `<arithmetic> f(…)` is accepted — and that is what the documentation now says, because it is what a reader needs in order to predict which rewrite works. The characterisation row grows from three shapes to seven, keeps its positive controls and gains the directional one (`n / NULLIF(m, 0)`), so "everything is rejected" still cannot satisfy it.⚠️ Stated plainly for the lead rather than buried in a doc note: with `/` now yielding DOUBLE and `f(<arithmetic>)` unparseable, there is **no single-expression way to obtain a truncated quotient**. The workaround — compute the quotient into a column and cast THAT column — is documented and executed, but it requires a DDL. Refs #382 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…382) Asked for by the lead while reviewing #382: the same four questions were spelled out as `isInstanceOf[SQL...]` at 22 sites across six files (`function/cond`, `query/Where`, `SQLTypeUtils`, `operator/math/ArithmeticExpression`, `sql/package`), and the ones that disagreed are exactly where the defects were. `leastCommonSuperType` had already learnt that `contains(Varchar)` misses `Text` and `Keyword` while `isInstanceOf[SQLVarchar]` does not, and that lesson had to be relearnt a hundred lines below in `coerce`. `isText`, `isNumber`, `isTemporal`, `isBoolean` and `isUnknown` are concrete methods on the SEALED trait, because the families are what that file DECLARES: a predicate written beside the hierarchy cannot fall out of step with it, and a new member of a family is answered for by every caller at once.⚠️ `isText` is `SQLVarchar` and NOT `SQLChar`, which is a sibling under `SQLLiteral` rather than a subtype. That is what every one of the 22 sites meant, so the extraction preserves it exactly; whether CHAR should be text is a separate decision with its own blast radius, and making it here would have changed behaviour under cover of a refactor. Additive and source-compatible: `SQLType` is sealed, so no downstream subclass exists to break. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by the #382 review, confirmed on real Elasticsearch 6.8.23 and 8.18.3. `toPainlessCall` has three arms — both text (`compareTo`), both temporal (`isEqual`/`compareTo`), and a final `==`. The refusal beside them listed the ONE pair it had measured, TEXT against NUMBER, so everything else fell through to `==` — which Painless applies to any two references without complaining, answering `false`. A date against a number, a boolean against anything, and a date against a string literal were therefore ACCEPTED and returned the first argument for EVERY document, HTTP 200, in every venue. The same silent always-false this rule already refused TEXT-against-NUMBER to avoid. The refusal is now DERIVED FROM THE ARMS (`comparable`): both text, both temporal, both numeric, both boolean, or one side UNKNOWN. A predicate written beside the arms says nothing when a new arm appears; one derived from them cannot drift. UNKNOWN accepts on purpose — an unresolved column reports `Any`, and refusing there would reject legitimate SQL on every schema-less path while the resolved spelling of the same statement is fine. 🔴 A temporal operand against a STRING is refused too, including a WELL-FORMED date literal, and `temporalLiteralError` stays reachable and goes FIRST so it can say something better: a malformed literal is still named as such against the column's mapping format, a well-formed one gets a message saying the engine cannot do it YET. Making it work needs a `String -> temporal` coercion `SQLTypeUtils.coerce` does not have (its temporal arms are all temporal-to-temporal), one per subtype, in three venues, on four majors — and Elasticsearch date math (`now-1d`) has no Painless equivalent at all. Scoped as its own story rather than half-built here.⚠️ The check runs at the ONE schema-resolution seam, so it reaches schema-resolved SELECTs; a wildcard or multi-index FROM, and DML, do not reach it. Stated in the docs rather than implied. Guards: the corpus invariants gain a DERIVED population canary (every operand compared with itself must still emit, or `offenders shouldBe empty` would be satisfied by refusing everything); the reject list reads the two type names from the message's own `compares X with Y` clause rather than with `include`, which the echoed expression and #262's 200 chars of caller SQL made free; and `mixesTextWithNonText` now excludes BOTH homogeneous families and UNKNOWN, after the widened predicate flagged the temporal arm's legitimate `compareTo`. Mutation-proved: reverting the rule to the hand-written pair reddens 4 assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four defects in the ruling as it was written, all found by the review. 🔴 The rule switched OFF whenever the schema does not attach. `SQLTypes.Any` is not an `SQLNumeric`, and `leastCommonSuperType(List(Any, Any))` short-circuits to `Any`, so the numeric-fold gate was false for a statement whose columns carry no type — which `SearchApi.resolveWithSchema` declines to attach for a wildcard FROM, a comma-separated FROM, a client that is not an `IndicesApi`, and any mapping whose load failed or is negatively cached. `SELECT n / m FROM logs-*` answered 3 while the same statement over `logs_2025` answered 3.5, and — worse — the zero guard was not emitted either, so a zero divisor stayed an `arithmetic_exception` in a search and a REJECTED DOCUMENT on ingest. That is the data-loss bug the guard exists to close, and `operators.md` promises both unconditionally. 🔴 A literal-ZERO divisor is now FOLDED to `null` instead of guarded, and that is what makes the ruling work on Elasticsearch 6.8. The guard renders as a parenthesised conditional, and where the division is the WHOLE script — a constant like `SELECT 1/0`, with no `def param…` in front of it — 6.8's Painless refuses a body that is nothing but a conditional (`Extraneous conditional statement`). Over COLUMNS it is fine on every version, because the declarations precede the conditional. Found by the es6 integration leg; a unit suite could not have seen it, because the emission is well-formed on 8.18 and the byte pin said so. 🔴 `emittedType` claimed a coercion `SQLTypeUtils.coerce` does not perform: it has no `(Any, Double)` arm, so an operand whose rendering is UNKNOWN is emitted unchanged while `needsDoubleCast` believed it carried the float. An `unsigned_long` field (no `SQLTypes` arm ⇒ `Any`, doc value a `long`) divided by an `integer` column divided as INTEGERS inside an expression the engine reports as DOUBLE — with the guard present, so it looked right. 🔴 The divisor was read TWICE — once in the guard, once in the division — and was bound only when nullable, so `x / RANDOM()` guarded a different draw than it divided by. It is now bound whenever it is not a bare name. Documentation: the "verified in an aggregation script" claim is removed (no aggregation assertion exists anywhere, and `SUM(a) / SUM(b)` renders as a `bucket_script`, a venue this branch never tested); the `%` note no longer recommends `CASE WHEN b != 0 THEN a % b END`, which the same page proves is a parse error; and the `%` note gains the FLOATING half of the data loss it was warning about — `NaN`, which Elasticsearch refuses to index, rejecting the whole document. Executed on real Elasticsearch 6.8.23 (rest + jest) and 8.18.3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…comment (#382) Three more found by the review. 🔴 Ruling B's null guard missed two whole populations. `checkIfNullable` GATES the block and asked only about `nullable`, so the guard was never attempted when NO argument was nullable — and a failing safe cast over a LITERAL or a NOT NULL column is exactly that case: `CONCAT(TRY_CAST('abc' AS BIGINT), 'x')` still stored the literal text "nullx", and `ABS(TRY_CAST('abc' AS DOUBLE))` still threw an NPE that `ignore_failure` swallowed into an absent column, AFTER the ruling meant to close them. And `FunctionChain.rendersNullOnFailure` never descended into a `FunctionN`'s args, so a safe cast one function deeper — `CONCAT(NULLIF(TRY_CAST(s AS BIGINT), 0), 'x')` — was invisible. Both halves are needed; either alone leaves a population.⚠️ `argRendersNullOnFailure` is a package-level function, NOT a member of `FunctionN`: as a trait member it is mixed into every implementor and scalac 2.13.16 fails during `mixin` with `AssertionError: List(object package$FunctionN, object package$FunctionN)`. It needs no `this`, so nothing is lost. 🔴 A Painless REGEX literal containing `/*` was read as a block comment. `WHERE path RLIKE 'a/*b'` emits `($p ==~ /a\/*b/)`, so those bytes appear OUTSIDE every quoted literal: with no `*/` to close it the scanner blanked the rest of the script — losing the trailing `ctx.<column> = ` from `ScriptTarget.of` (the ALTER churn the scanner exists to stop) and able to hide a real `;` from `firstStatementAt`. A pattern merely STARTING with `*` does it with no escape at all, so the arm precedes both comment arms. `~` is the whole discriminator and it is sufficient: `==~` and `=~` are the only operators Painless spells with it. 🔴 `IngestPipeline.diff`'s collision fallback could displace a processor that did NOT collide, so an untouched pipeline member was reported Removed + Added. Keys nobody had to derive are now reserved, which makes the scaladoc's promise true. And a comment corrected rather than code: `nullIfErrors`' `filterNot(_.materialized)` is not the `isRegular` carve-out and does not contradict it — a STORED script EMITS NOTHING, so there is no Painless for the rule to be wrong about. STORED and "materialized view" are different things, and a reviewer read them as one. Test-integrity repairs in the same files: the statement-start scanner gets its own test (it was widened at the same time as the test carrying its true positive was rewritten, leaving nothing able to redden it); `ScriptTarget.of(...).take(1)` becomes the whole identity, in a test whose subject is a processor claiming ANOTHER column's name; and the safe-cast guard assertion follows the derivation chain from `safe1` instead of using `params.last` as a positional proxy for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Run on real Elasticsearch 6.8.23 (rest + jest) and 8.18.3; inherited by all five
clients.
🔴 The temporal-literal rows caught a contradiction the unit suite could not.
`NULLIF(created, '2024-01-15')` was asserted here to SUCCEED, while the unit spec
had already been amended to expect a refusal — and `sql/test` plus `core/test`
were both green with that disagreement sitting in the tree. The cluster is what
has an opinion. Both rows now assert refusal AND tell the two judges apart: the
engine's message (`not supported yet`, naming the two types) versus the
resolver's (naming the literal and the field), each asserting the ABSENCE of the
other's wording, so "everything is refused" cannot satisfy them.
🔴 The `%` control was VACUOUS, and worse than the review first reported. Both
`intsById` (`.toDouble.toLong`) and `doublesById` (`.toDouble`) erase the
int/double distinction the control exists to detect, and the computed column
cannot show it either — `r` is mapped INTEGER, so Elasticsearch coerces whatever
the script produced on index. Widening the ruling to `%` left every assertion
green. A QUERY projection is where it IS observable: `script_fields` returns what
Painless produced with no mapping to coerce it, so an integer `%` surfaces as `1`
and a promoted one as `1.0`. New `rawById` reads it unparsed, and the `/` row
beside it carries the decimal point, so the `%` row is shown to discriminate
rather than merely to pass.
🔴 The row stating the `"nullx"` defect compared the WRAPPED value, so
`List("nullx")` would have passed it — on a row whose whole purpose is to fail on
the pre-fix emission. It now reads through `scalarOf`, which unwraps
Elasticsearch's per-field array, and the null assertions use the same
`isNullColumn` predicate as the rest of the file rather than a weaker local one
that does not recognise `List(null)`.
And an attribution corrected: the arithmetic-operand gap is UNFILED, not #267 —
a different defect (a bare LITERAL operand) closed 2026-09-04. This is the site
the correction commit missed, and it is the most read of the three, being in the
shared testkit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Asked for by the lead while reviewing #382: "We added since v0.24.0 a lot of treatments within SearchApi, how does it affect the performance? The SQL parsing and the rendering have been checked regularly, I wonder if the impacts of effective search have been ever measured properly." They had not. `ParseCostProbe` measures parsing, `EmissionCostProbe` measures rendering, and core's `ParseCostProbeSpec` measures RESPONSE parsing — nothing measured the work between "the statement is parsed" and "Elasticsearch is called", which is where 0.24.0 put most of its new per-request work: the relational-closure guard (22.1), uncorrelated WHERE-subquery phase one (22.2), temporal-literal normalisation (#276), the schema ATTACH (#306), and the post-resolution rules `Case.conditionsOf` (#384) and `NullIf.mismatchesOf` (#382). `ResolveCostProbe` reports, per statement: `parse` (the yardstick), the whole `resolve`, `attach`, `temporal`, `validate`, and `resolve x2`. 🔴 `resolve x2` is the number an un-LIMITed row query really pays: `search` resolves, then routes through `scrollRows` -> `ScrollApi.scroll`, which resolves AGAIN — stated in `resolveWithSchema`'s own comment and pinned by `SchemaAttachSpec`'s idempotence test. Quoting `resolve` alone understates that shape by 2x. First baseline (Apple silicon, 800 runs after 150 discarded): resolution is 0.4-5 % of the parse it follows — 0.6-8.4 us against a 69-356 us parse. Two things that looked expensive are not: `validateResolved` costs 0.1-0.9 us, so the second AST walk is a clarity finding and not a cost one, and the whole `update(Some(schema))` rebuild is 0.5-1.5 us even for a window or GROUP BY. The only stage that registers is `TemporalLiterals` at ~2.7 us, and only for a statement that carries a temporal WHERE literal.⚠️ It excludes `loadSchema` (cached — an IO miss is a different order) and the Elasticsearch round-trip, and it is an ABSOLUTE measurement, not an attribution against a 0.23.x control. No assertion, a `main` only — #269/#270 spent a round on a timing assertion that flunked unrelated PRs on loaded CI runners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#382) `known_limitations.md` is the page with a published twin, and none of these were on it — they were only in `operators.md`, or nowhere. - **Arithmetic is not accepted inside a function, a `CAST` or a `CASE` branch.** The rule is DIRECTIONAL — arithmetic over a function call is fine, a function call over arithmetic is not — and parenthesising does not help. Its practical consequences are that there is no single-expression truncated quotient and no in-expression guard for `%`. Recorded as UNFILED, which it is: #267 is a different, closed defect about a bare literal operand. - **`%` by zero is not guarded**, in both its shapes: integer operands throw, floating operands produce `NaN` and Elasticsearch rejects the whole document. - **`ORDER BY` over arithmetic on a nullable column** fails, because the bridge emits a `number`-typed script sort and Elasticsearch refuses a sort script that can return null.⚠️ `softclient4es-web` publishes NO operators, precedence or conditional-functions page, so the `/` ruling reaches no published page at all. The empty twin-edit list the `/` docs commit recorded is correct; the real gap is the inverse, and it belongs to that repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the arithmetic-operand limitation added earlier in this branch, which described only the shapes that are REJECTED. `(a / b)::INTEGER` is accepted and emits byte-identically to `a / b` — the conversion is discarded while the expression reports INTEGER. Measured on a clean tree at `origin/main` `975aa87b`, so pre-existing, and it applies to any operator (`(a + b)::DOUBLE`, `(d + 1)::INTEGER`). It matters here because it is the workaround a reader reaches for after hitting the parse errors above, and it is the only member of the family that fails SILENTLY. Recorded rather than filed: the class has been filed and fixed twice already (#304, #306) and the shape is hand-written SQL no BI tool generates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fupelaqu
marked this pull request as ready for review
September 23, 2026 14:02
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 #382
What was broken
TRY_CAST/SAFE_CASTin a computed column had never worked, in any shape. The generated ingest script put the assignment inside acatch, with no right-hand side:fromScriptre-derived the prologue/expression boundary by splitting the rendered Painless on;. A;inside a literal was #373/#375; a;inside a block is this one. The cure is not a smarter scanner — it is to stop throwing the boundary away and then hunting for it.Measuring it found the family is wider, and one member was silent:
TRY_CAST(name AS DOUBLE) + 1dropped the conversion and computed on the raw string, HTTP 200.What changed
IngestScript.assemble)c BIGINT SCRIPT AS (TRY_CAST(name AS BIGINT))→'125'→125,'abc'→nulltry/catchinto the prologue in every context with oneCONCAT/COALESCE/CASE/UPPERover aTRY_CASTweredef param2 = try { … };compile errorsCAST(raw AS BIGINT) + m,'125'+7stored 1257, now 132/itself always yields DOUBLECAST(x AS INTEGER) / 2over7.5→3.5(the cast narrows7.5to7), against3.75uncast"null"CONCAT(TRY_CAST(raw AS BIGINT),'x')over'abc'stored"nullx", now NULLNULLIFover a cast or a nullable function worksNULLIF(TRIM(description), '')— an example our own docs publish — now gives' hello '→'hello',' '→NULLNULLIFover a compound argument was a silent wrong answerNULLIF(CASE WHEN n>1 THEN 1 ELSE 2 END, 1)overn=5stored 1 where SQL says NULLNULLIFargument is evaluated once, not three timesNULLIF(UPPER(s),'X')renderedUPPER(s)3×NULLIFrefuses every pair it cannot compare, derived from the arms/always yields DOUBLE, and a zero divisor is NULL, on every pathSELECT n / m→3.5;n / 0→ NULL instead of a rejected documentScriptTarget.ofno longer reads a Painless comment — or a regex literal — as codectx.c = 1 /* } ctx.zzz = 9 */reported columnzzz;RLIKE 'a/*b'blanked the script#11, in full — an
ALTERthat silently did nothingA
scriptprocessor has nofield, so Elasticsearch hands a hand-authored one back anonymous and its identity is recovered from the lastctx.<name> =in the source. Two processors claiming the same column made the diff indexed bySeq.toMapdrop the loser — theALTERdid nothing and the stored script kept computing the old expression. Content-based disambiguation was tried and is wrong (only one side collides, so a change becomes an add plus two removals). A colliding member falls back to its pre-recovery identity, and — found by review — that fallback now reserves the keys nobody had to derive, so it cannot displace a processor that did not collide.The engine decision behind #3 and #4
They are one method,
argTypeOf: an operand's type is what it renders, not what its column is declared as. Reverting one reverts the other (measured: the1257concatenation returns, 6 tests red).The
/ruling (lead, 2026-09-23) is separate and stronger: the result type of a division is a property of the operator, not of its arguments.CAST(5.0 AS INTEGER) / 2— measured, not quoted: PostgreSQL 16 → 2, MySQL 8 → 2.5, DuckDB 1.5.5 → 2.5. We follow MySQL and DuckDB. PostgreSQL is the outlier that truncates, and it spells truncation separately (DIV,//) — we have no such operator, so the documented way to truncate is to compute the quotient into a column and cast THAT column.JoinPlannerhands itfield.identifier.sql), and DuckDB's/is always decimal — so onmainSELECT n / manswered3outside a JOIN and3.5inside one, for the same two INTEGER columns. Aligning/removes the class, not one instance of it.Release notes (0.24.0)
TRY_CASTin a computed column works. An unconvertible row is indexed with the column NULL —_sourcecarries"c": null, no doc value — so a plainSELECTreturns the row with an empty value whileIS NOT NULLand aggregations skip it.WHEREand a projection alike. AWHEREover such an expression matches a different row set. Arithmetic with no cast and a cast to the column's own type are unchanged (0 of 232 corpus rows move for the cast rule; the/rule below moves more)./always yields DOUBLE.SELECT n / mover two INTEGER columns answers3.5, not3. This changes projections, computed columns andWHERErow sets for every statement that divides.+,-,*and%are unchanged. There is no truncating-division operator: compute the quotient into a column and cast that column.Infinity, which Elasticsearch refuses to index — rejecting the whole document. That was data loss on anydoublecolumn.SELECT 1/0now returns NULL.%is NOT covered and still throws (integer) or yieldsNaNand loses the document (floating) — see known limitations.NULLIFrefuses every pair it cannot compare. Comparable means both text, both temporal, both numeric, both boolean, or one side NULL / an unresolved column.NULLIF(<date>, <number>),NULLIF(<boolean>, <anything else>)andNULLIF(<date>, '<string literal>')were ACCEPTED before and silently returned the first argument for every row — Painless==answersfalsefor those pairs without failing. A well-formed date literal is refused too, saying so explicitly; cast it (NULLIF(created_at, CAST('2025-01-01' AS DATE))). A follow-up story is scoped for the coercion.NULLIF(CAST(qty AS KEYWORD), 0)stored a value before. Remedy: cast the other side, or drop the cast.ScriptProcessor.sourceis persisted in_metaandIngestPipeline.diffcompares it. (a) A stored computed column of a moved shape (cast-arithmetic,NULLIF(<nullable function>, …)) is reportedProcessorChanged. (b) Every stored column that divides moved too:n / 2→(param1 / ((double) 2)), andn / mgained a(double)cast and a zero guard. Re-runCREATE TABLE/ALTER … SET SCRIPT ASand reindex for affected tables; downstream repositories pinning generated JSON need updating.IngestPipeline.diffno longer drops a processor on a recovered-identity collision, and no longer re-keys one that did not collide. On ES 6.8 (no processordescription) such a collision churns rather than reporting a change.app.softnetwork.elastic.sql.schema.IngestScript.assemble,Function.rendersNullOnFailure, and the type-family predicates onSQLType(isText,isNumber,isTemporal,isBoolean,isUnknown). NewResolveCostProbeincore/src/test.Two decisions the spec required be stated here
PainlessOperandForm.splitStatementshas no production caller since the assembly is handed its two halves. It is kept for two differential specs and its scaladoc says so. Lead decision requested: keep or remove.case Array(single) if single.trim.startsWith("return ")arm was REMOVED, proved unreachable as a property rather than argued (\breturn\babsent across the 13-shape sweep). Thesoftclient4es-extensionscopy has the same arm and is deleted with it on that branch.Evidence
sql/test1567/1567 ·core/test1141/1141es8java94/94 (8.18.3),es6rest92/92 andes6jest92/92 (6.8.23, 2assume-skipped each)++ 2.12.20compilingsql/main,sql/testandcore/test· the exact CI lint line (headerCheck scalafmtSbtCheck scalafmtCheck test:scalafmtCheck), files staged firstsbt compile testat the root🔴 What the integration legs found that the unit suites could not
Both are recorded because they are the argument for running them at all.
((((double) 0) == 0) ? null : …)→illegal_argument_exception: Extraneous conditional statement. Well-formed on 8.18, and the byte pin said so. Fixed by folding a literal-zero divisor tonull— same answer everywhere, one less runtime guard. Divisions over columns were always fine, because the parameter declarations precede the conditional.NULLIF(created, '2024-01-15')was asserted to succeed inGatewayApiIntegrationSpecwhile the unit spec had already been amended to expect a refusal — andsql/testpluscore/testwere both green with that disagreement in the tree.Parse, render and resolve cost
origin/main. Parsing: exactly one change (NULLIF(CAST(n AS KEYWORD), 0), the shape release note 6 names). Rendering: zero changes. Cost: no signal — interleaved medians 523 ms vs 522 ms, ranges 493–654 vs 493–682, overlapping.ResolveCostProbe, asked for by the lead): resolution is 0.4–5 % of the parse it follows — 0.6–8.4 µs against a 69–356 µs parse.validateResolvedis 0.1–0.9 µs, so the second AST walk this branch adds is not a cost concern; the wholeupdate(Some(schema))rebuild is 0.5–1.5 µs. The only stage that registers isTemporalLiteralsat ~2.7 µs. 🔴 An un-LIMITed row query pays the chain twice (search→scrollRows→scroll).loadSchemaand the round-trip, and is not an attribution against a 0.23.x control.Recorded, not fixed
HAVING <function>(<aggregate>)silently drops the whole clause. Filed, documented, identical before and after this branch.CAST(a / b AS INTEGER),FLOOR(a / b),COALESCE(a / b, 0)andCASE WHEN … THEN a / b ENDare all parse errors, directionally (arithmetic over a function is fine). It is not CAST/TRY_CAST/CONVERT reject bare literal operands — CAST('125' AS BIGINT) does not parse #267, which is a different defect (a bare literal operand), closed 2026-09-04. Documented inknown_limitations.md. Needs a lead decision: file it, or accept an explicitly-unfiled dependency, since a shipped design argument leans on it.%by zero is not guarded — integer operands throw; floating operands yieldNaNand Elasticsearch rejects the whole document, the same data loss the/guard fixes. Documented.softclient4es-webpublishes no operators, precedence or conditional-functions page, so the/rule reaches no published page. Belongs to that repo.NULLIF(<numeric literal>, NULL)emits0 == null— byte-identical onmain, degenerate SQL, pinned as a characterisation.copyBridge(build.sbt:340) races sbt's own source glob: when it loses, a bridge compiles from zero sources,publishLocalreports success, and a 318-byte empty jar lands in~/.ivy2/local. Reproduced three times.Sequencing
softclient4es-extensionshas a companion branch that deletes its private copy of this assembly and callsIngestScript.assemble. Its PR is deliberately not open: it cannot compile until this merges and0.24.0-SNAPSHOTis published (the snapshot on JFrog carries noIngestScript, andrelease.ymlfires only onworkflow_dispatchor av*tag — merging alone publishes nothing). Verify jar contents, not the exit code, when publishing.🤖 Generated with Claude Code