From 3d9f8e6f4752674b280b912245ce5749fa54ad53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 19 Sep 2026 18:48:39 +0200 Subject: [PATCH 1/8] fix(sql): a TIME comparison used a spelling LocalTime does not have (#373, 1/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../elastic/sql/function/cond/package.scala | 16 +++ .../softnetwork/elastic/sql/query/Where.scala | 19 ++- .../sql/query/PainlessResidualsSpec.scala | 135 ++++++++++++++++++ .../sql/query/ParameterIdentitySpec.scala | 8 +- .../client/PredicateFunctionResultSpec.scala | 9 +- 5 files changed, 178 insertions(+), 9 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala index 11fd9fc9..f81f0cc3 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala @@ -186,6 +186,14 @@ package object cond { override def baseType: SQLType = SQLTypeUtils.leastCommonSuperType(argTypes) + /** Whether the value `expr1` RENDERS is a `java.time.LocalTime`. `out` and `expr1.out` both + * report the column's type; `Identifier.chainType` reports the chain's (#367). + */ + private[this] def receiverIsTime: Boolean = expr1 match { + case i: Identifier => i.chainType == SQLTypes.Time + case other => other.out == SQLTypes.Time + } + private[this] def checkIfExpressionNullable(expr: PainlessScript): Boolean = expr match { case f: FunctionChain if f.functions.nonEmpty => true case _ => false @@ -204,6 +212,14 @@ package object cond { out match { case SQLTypes.Varchar => s"$arg0 == null || $arg0.compareTo($arg1) == 0 ? null : $arg0" + // πŸ”΄ Keyed on what the RECEIVER's chain RENDERS, not on `out` (issue #373). + // `LocalTime` has no `isEqual`, and MEASURED on ES 8.18 before this, + // `NULLIF(CAST(d AS TIME), CAST('00:00:00' AS TIME))` failed the shard in a real + // `script_fields`. Neither `out` nor `expr1.out` says so: both are TIMESTAMP, the + // COLUMN's type -- `chainType` is the derivation that describes the rendering + // (#367), and it answers TIME. Two wrong keys were tried before measuring it. + case _: SQLTemporal if receiverIsTime => + s"$arg0 == null || $arg0.compareTo($arg1) == 0 ? null : $arg0" case _: SQLTemporal => s"$arg0 == null || $arg0.isEqual($arg1) ? null : $arg0" case _ => s"$arg0 == $arg1 ? null : $arg0" } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala index 051c419e..5432f218 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala @@ -959,7 +959,22 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } case EQ => valueType match { - case SQLTypes.Varchar => + // πŸ”΄ `java.time.LocalTime` is the ONE temporal with no `isEqual` (issue #373): the + // other three have it -- `LocalDate` and `LocalDateTime` declare it, `ZonedDateTime` + // inherits it from `ChronoZonedDateTime`. MEASURED on ES 8.18: `CAST(d AS TIME) = + // CAST('00:00:00' AS TIME)` answered `dynamic method [java.time.LocalTime, + // isEqual/1] not found`, a shard failure. + // + // πŸ”΄ `compareTo(…) == 0` and NOT `equals`, and the difference is the whole point. + // This dispatch reads `valueType`, the type of the RIGHT operand, while the receiver + // is the LEFT one -- so a mismatched pair reaches here too (`WHERE name = + // CAST('00:00:00' AS TIME)` inside a CASE, a `String` receiver). `equals` takes an + // `Object` and answers FALSE for a mismatch: a loud shard failure would have become + // a silent wrong answer, and `<>` would have matched every document. MEASURED on ES + // 8.18, all three: `equals` -> `[false,false,false]`; `compareTo` -> + // `class_cast_exception`; `isEqual` (main) -> `dynamic method … not found`. The two + // spellings are otherwise identical for a `LocalTime`, nanoseconds included. + case SQLTypes.Varchar | SQLTypes.Time => return s"$param.compareTo($value) == 0" case _: SQLTemporal if !isAggregation && !hasBucket => return s"$param.isEqual($value)" @@ -967,7 +982,7 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } case NE | DIFF => valueType match { - case SQLTypes.Varchar => + case SQLTypes.Varchar | SQLTypes.Time => return s"$param.compareTo($value) != 0" case _: SQLTemporal if !isAggregation && !hasBucket => return s"$param.isEqual($value) == false" diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala new file mode 100644 index 00000000..2c6a4ba3 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala @@ -0,0 +1,135 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.schema.{Column, Table => SchemaTable} +import app.softnetwork.elastic.sql.`type`.SQLTypes +import app.softnetwork.elastic.sql.{PainlessContext, PainlessContextType} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Issue #373 β€” the Painless-emission defects surfaced by #370, fixed in one PR, one commit each. + * + * All nine are PRE-EXISTING: each was found by EXECUTING the emission of a #370 shape on real + * Elasticsearch and comparing it with a control at `main`. They share one theme β€” the emitted + * script must match the RECEIVER the pipeline really hands it, and the chain that produced it β€” + * but each has its own decision point, so each gets its own commit and its own test here. + * + * πŸ”΄ What a byte pin in this file can and cannot say: it fixes the emission, not its validity. + * Every expectation below names the Elasticsearch behaviour it was measured against, and the + * executed row lives in `PredicateFunctionResultSpec`. + */ +class PainlessResidualsSpec extends AnyFlatSpec with Matchers { + + private val schema: SchemaTable = SchemaTable( + "t", + columns = List( + Column("d", SQLTypes.Date), + Column("ts", SQLTypes.Timestamp), + Column("name", SQLTypes.Keyword), + Column("n", SQLTypes.Int) + ) + ) + + private def single(sql: String, withSchema: Boolean = true): SingleSearch = + Parser(sql) match { + case Right(ss: SingleSearch) => if (withSchema) ss.update(Some(schema)) else ss.update() + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + /** The WHERE criteria rendered as ONE script, prologue included. */ + protected def predicateOf(sql: String, withSchema: Boolean = true): String = { + val ctx = PainlessContext(PainlessContextType.Query) + val body = single(sql, withSchema).where + .flatMap(_.criteria) + .map(_.painless(Some(ctx))) + .getOrElse(fail(s"[$sql] has no WHERE")) + s"$ctx$body" + } + + /** The first SELECT field rendered as one script. */ + protected def fieldOf(sql: String, withSchema: Boolean = true): String = { + val ctx = PainlessContext(PainlessContextType.Query) + // πŸ”΄ The body is rendered FIRST and bound: it is what REGISTERS the parameters, and a string + // interpolation evaluates `ctx.toString` before the call beside it, so an inline + // `s"$ctx${…painless(ctx)}"` prints an EMPTY prologue. Caught by this spec's own NULLIF pin. + val body = single(sql, withSchema).select.fields.head.painless(Some(ctx)) + s"$ctx$body" + } + + // -- item 2: the comparison spelling must exist on the receiver -------------------------------- + + /** `java.time.LocalTime` is the only temporal without `isEqual`: `LocalDate` and `LocalDateTime` + * declare it and `ZonedDateTime` inherits it from `ChronoZonedDateTime` (checked with `javap`). + * MEASURED on ES 8.18 before this commit: `dynamic method [java.time.LocalTime, isEqual/1] not + * found` β€” a shard failure on `main`, and the reason #370 could pin the TIME identity but not + * execute it. + */ + "a TIME comparison" should "use a spelling LocalTime has" in { + predicateOf("SELECT name FROM t WHERE CAST(d AS TIME) = CAST('00:00:00' AS TIME)") shouldBe + "def param1 = (doc['d'].size() == 0 ? null : doc['d'].value.toLocalTime()); " + + "def param2 = LocalTime.parse(\"00:00:00\", DateTimeFormatter.ISO_LOCAL_TIME); " + + "param1 == null ? false : (param1.compareTo(param2) == 0)" + predicateOf("SELECT name FROM t WHERE CAST(d AS TIME) <> CAST('00:00:00' AS TIME)") should + endWith("param1 == null ? false : (param1.compareTo(param2) != 0)") + } + + it should "leave every other temporal on isEqual" in { + // `isEqual` is instant equality for a `ZonedDateTime` and is NOT `equals` (which compares the + // zone too), so this arm must not be widened: DATE and TIMESTAMP keep their spelling. + predicateOf("SELECT name FROM t WHERE CAST(d AS DATE) = CAST('2025-01-05' AS DATE)") should + endWith("param1 == null ? false : (param1.isEqual(param2))") + predicateOf("SELECT name FROM t WHERE ts = CAST('2025-01-05T00:00:00' AS TIMESTAMP)") should + endWith("param1 == null ? false : (param1.isEqual(param2))") + } + + /** πŸ”΄ The dispatch reads `valueType` -- the type of the RIGHT operand -- while the receiver is + * the LEFT one, so a MISMATCHED pair reaches this arm too (`name` is a keyword column; the + * statement is accepted because a bare column is `Any` at parse time). `equals` takes an + * `Object` and would answer FALSE for that, turning a loud shard failure into a silent wrong + * answer -- and `<>` into "every document". `compareTo` keeps it loud. 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`. + * + * Found by review of this commit; the shape is reachable today through a CASE, and the item-4 + * commit routes the bare `WHERE name = CAST(… AS TIME)` here too. + */ + it should "stay LOUD when the receiver is not a LocalTime" in { + predicateOf( + "SELECT name FROM t WHERE CASE WHEN name = CAST('00:00:00' AS TIME) THEN 1 ELSE 0 END = 1" + ) should include("param1.compareTo(param2) == 0") + } + + /** `NULLIF` had the same hole at its own site. πŸ”΄ Neither `out` nor `expr1.out` could key it: + * both report TIMESTAMP, the COLUMN's type, even though both arguments are TIME -- + * `Identifier.chainType` (#367) is the derivation that describes the RENDERING, and two wrong + * keys were tried before that was measured. Executed on ES 8.18 over midnight dates: before, + * `dynamic method [java.time.LocalTime, isEqual/1] not found`; after, `null` on every row (they + * all equal 00:00:00, which is what NULLIF returns null for). + */ + "NULLIF over a TIME receiver" should "use the same spelling" in { + fieldOf("SELECT NULLIF(CAST(d AS TIME), CAST('00:00:00' AS TIME)) AS c FROM t") should + include("param2 == null || param2.compareTo(param3) == 0 ? null : param2") + } + + it should "leave the ordering comparisons alone" in { + // `isBefore` / `isAfter` DO exist on `LocalTime`, so only `=` and `<>` moved. + predicateOf("SELECT name FROM t WHERE CAST(d AS TIME) > CAST('00:30:00' AS TIME)") should + endWith("(param1.isAfter(param2))") + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala index 219b5848..856bfd5c 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala @@ -340,9 +340,8 @@ class ParameterIdentitySpec extends AnyFlatSpec with Matchers { // The TIME twin (review): `CAST(ts AS TIME) + INTERVAL 1 HOUR > …` was right on `main` for the // same accidental reason, and re-narrowed a `LocalTime` once the interval fold kept the // coercion. Executed on ES 8.18 over midnight dates: the first selects every document. The - // second is pinned for its IDENTITY only -- `=` is spelled `isEqual`, which `LocalTime` does - // not have (`dynamic method [java.time.LocalTime, isEqual/1] not found`), on `main` too: - // #367's comparison spelling, not owned here. + // second now EXECUTES: `=` over a TIME receiver is spelled `equals`, which `LocalTime` has + // (issue #373) -- it was pinned for its identity only while it still emitted `isEqual`. predicateOf( "SELECT name FROM t WHERE CAST(d AS TIME) + INTERVAL 1 HOUR > CAST('00:30:00' AS TIME)" ) shouldBe @@ -352,7 +351,8 @@ class ParameterIdentitySpec extends AnyFlatSpec with Matchers { predicateOf("SELECT name FROM t WHERE CAST(d AS TIME) = CAST('00:00:00' AS TIME)") shouldBe s"def param1 = $guardedDoc.toLocalTime()); " + "def param2 = LocalTime.parse(\"00:00:00\", DateTimeFormatter.ISO_LOCAL_TIME); " + - "param1 == null ? false : (param1.isEqual(param2))" + // `compareTo`, not `isEqual`: `LocalTime` is the one temporal without it (issue #373). + "param1 == null ? false : (param1.compareTo(param2) == 0)" } "the identity key" should "not read the narrowing off the object" in { diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala index ea5da278..386b57c0 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala @@ -330,9 +330,12 @@ trait PredicateFunctionResultSpec extends AnyFlatSpecLike with ElasticDockerTest // (`.toLocalTime()`) does not exist on ES 6.8's Joda-compatible doc-value -- pre-existing. assume(esMajor >= 7, "a TIME narrowing of a raw date column is unsupported on ES 6.8") selected("CAST(d AS TIME) + INTERVAL 1 HOUR > CAST('00:30:00' AS TIME)") shouldBe allIds - // `CAST(d AS TIME) = ` is deliberately NOT here: the identity is right, but `=` - // is spelled `isEqual`, which `java.time.LocalTime` does not have (#367's comparison - // spelling, loud on `main` too). + // `=` over a TIME receiver EXECUTES since issue #373: it was spelled `isEqual`, which + // `java.time.LocalTime` does not have, and it is spelled `compareTo(…) == 0` now. Every + // fixture date is midnight, so all of them match and none matches 00:30. + selected("CAST(d AS TIME) = CAST('00:00:00' AS TIME)") shouldBe allIds + selected("CAST(d AS TIME) = CAST('00:30:00' AS TIME)") shouldBe Set.empty[String] + selected("CAST(d AS TIME) <> CAST('00:30:00' AS TIME)") shouldBe allIds } /** πŸ”΄ Issue #370's LOUD face: two different extractions of one column in ONE expression. From 6e2b76a0ef8053e9f2b3e885636974d74b3b6147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 19 Sep 2026 18:55:02 +0200 Subject: [PATCH 2/8] fix(sql): a CASE candidate is bound and guarded before the comparison (#373, 2/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 …` 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 --- .../elastic/sql/SQLQuerySpec.scala | 6 +- .../elastic/sql/SQLQuerySpec.scala | 6 +- .../elastic/sql/function/cond/package.scala | 100 ++++++++++++++---- .../sql/query/PainlessResidualsSpec.scala | 82 +++++++++++++- .../sql/query/ParameterIdentitySpec.scala | 6 +- 5 files changed, 172 insertions(+), 28 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 913539cb..b961061c 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -2178,7 +2178,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "c": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")).minus(2, ChronoUnit.DAYS); def param3 = param1 == null || param1.isEqual(param2) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate(); (param3 != null ? param3 : param4)", + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")).minus(2, ChronoUnit.DAYS); def param3 = param1 == null || (param2 != null && param1.isEqual(param2)) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate(); (param3 != null ? param3 : param4)", | "params": { | "__now__": 1767139200000 | } @@ -2339,7 +2339,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "c": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")); def param3 = param1 == null || param1.isEqual(param2) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(2, ChronoUnit.HOURS); def safe1 = null; try { safe1 = (param3 != null ? param3 : param4); } catch (Exception e) {} safe1", + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")); def param3 = param1 == null || (param2 != null && param1.isEqual(param2)) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(2, ChronoUnit.HOURS); def safe1 = null; try { safe1 = (param3 != null ? param3 : param4); } catch (Exception e) {} safe1", | "params": { | "__now__": 1767139200000 | } @@ -2505,7 +2505,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "c": { | "script": { | "lang": "painless", - | "source": "def param1 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(7, ChronoUnit.DAYS); def param2 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate().minus(3, ChronoUnit.DAYS)); def param3 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate()); def param4 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toLocalDate()); def param5 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate().plus(2, ChronoUnit.DAYS)); def param6 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); param1 != null && param1.isEqual(param2) ? param3 : param1 != null && param1.isEqual(param4) ? param5 : param6", + | "source": "def param1 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(7, ChronoUnit.DAYS); def param2 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate().minus(3, ChronoUnit.DAYS)); def param3 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate()); def param4 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toLocalDate()); def param5 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate().plus(2, ChronoUnit.DAYS)); def param6 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); param1 != null && param2 != null && param1.isEqual(param2) ? param3 : param1 != null && param4 != null && param1.isEqual(param4) ? param5 : param6", | "params": { | "__now__": 1767139200000 | } diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 38ea9e97..99ba13a0 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -2178,7 +2178,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "c": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")).minus(2, ChronoUnit.DAYS); def param3 = param1 == null || param1.isEqual(param2) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate(); (param3 != null ? param3 : param4)", + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")).minus(2, ChronoUnit.DAYS); def param3 = param1 == null || (param2 != null && param1.isEqual(param2)) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate(); (param3 != null ? param3 : param4)", | "params": { | "__now__": 1767139200000 | } @@ -2323,7 +2323,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "c": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")); def param3 = param1 == null || param1.isEqual(param2) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(2, ChronoUnit.HOURS); def safe1 = null; try { safe1 = (param3 != null ? param3 : param4); } catch (Exception e) {} safe1", + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = LocalDate.parse(\"2025-09-11\", DateTimeFormatter.ofPattern(\"yyyy-MM-dd\")); def param3 = param1 == null || (param2 != null && param1.isEqual(param2)) ? null : param1; def param4 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(2, ChronoUnit.HOURS); def safe1 = null; try { safe1 = (param3 != null ? param3 : param4); } catch (Exception e) {} safe1", | "params": { | "__now__": 1767139200000 | } @@ -2489,7 +2489,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "c": { | "script": { | "lang": "painless", - | "source": "def param1 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(7, ChronoUnit.DAYS); def param2 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate().minus(3, ChronoUnit.DAYS)); def param3 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate()); def param4 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toLocalDate()); def param5 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate().plus(2, ChronoUnit.DAYS)); def param6 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); param1 != null && param1.isEqual(param2) ? param3 : param1 != null && param1.isEqual(param4) ? param5 : param6", + | "source": "def param1 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().minus(7, ChronoUnit.DAYS); def param2 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate().minus(3, ChronoUnit.DAYS)); def param3 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate()); def param4 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toLocalDate()); def param5 = (doc['lastSeen'].size() == 0 ? null : doc['lastSeen'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate().plus(2, ChronoUnit.DAYS)); def param6 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); param1 != null && param2 != null && param1.isEqual(param2) ? param3 : param1 != null && param4 != null && param1.isEqual(param4) ? param5 : param6", | "params": { | "__now__": 1767139200000 | } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala index f81f0cc3..8ee1c715 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala @@ -208,10 +208,21 @@ package object cond { ): String = { callArgs match { case List(arg0, arg1) => + // πŸ”΄ The SECOND argument is guarded too (issue #373, found by review) -- the same hole + // `checkCase` had, one method up in this file. `NULLIF(CAST(d AS TIME), CAST(ts AS + // TIME))` called the comparison with a null `arg1` for any document missing `ts`: + // MEASURED on ES 8.18, `null_pointer_exception: Cannot read field "hour" because + // "other" is null`. + // + // πŸ”΄ And the 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 `arg1 == null` to the NULL branch would invert that. + def nullIf(comparison: String): String = + s"$arg0 == null || ($arg1 != null && $comparison) ? null : $arg0" val expr = out match { case SQLTypes.Varchar => - s"$arg0 == null || $arg0.compareTo($arg1) == 0 ? null : $arg0" + nullIf(s"$arg0.compareTo($arg1) == 0") // πŸ”΄ Keyed on what the RECEIVER's chain RENDERS, not on `out` (issue #373). // `LocalTime` has no `isEqual`, and MEASURED on ES 8.18 before this, // `NULLIF(CAST(d AS TIME), CAST('00:00:00' AS TIME))` failed the shard in a real @@ -219,9 +230,10 @@ package object cond { // COLUMN's type -- `chainType` is the derivation that describes the rendering // (#367), and it answers TIME. Two wrong keys were tried before measuring it. case _: SQLTemporal if receiverIsTime => - s"$arg0 == null || $arg0.compareTo($arg1) == 0 ? null : $arg0" - case _: SQLTemporal => s"$arg0 == null || $arg0.isEqual($arg1) ? null : $arg0" - case _ => s"$arg0 == $arg1 ? null : $arg0" + nullIf(s"$arg0.compareTo($arg1) == 0") + case _: SQLTemporal => nullIf(s"$arg0.isEqual($arg1)") + // `==` is null-safe in Painless, so a numeric / boolean pair needs no guard. + case _ => s"$arg0 == $arg1 ? null : $arg0" } context match { case Some(ctx) => @@ -312,14 +324,52 @@ package object cond { else Right(()) } - private[this] def checkCase(e: String, c: String, v: String): String = { - out match { - case SQLTypes.Varchar => - s"$e != null && $e.compareTo($c) == 0 ? $v" - case _: SQLTemporal => - s"$e != null && $e.isEqual($c) ? $v" - case _ => s"$e == $c ? $v" - } + /** Whether the CASE EXPRESSION -- the receiver of every `WHEN` comparison -- renders a + * `java.time.LocalTime`. `out` reports the CASE's result type and cannot say (issue #373). + */ + private[this] def receiverIsTime: Boolean = expression.exists { + case i: Identifier => i.chainType == SQLTypes.Time + case other => other.out == SQLTypes.Time + } + + /** @param candidateNullable + * whether `c` is a BOUND parameter that can hold `null`. + * + * πŸ”΄ The CANDIDATE needs a guard too (issue #373). `CASE WHEN THEN …` emitted + * `e != null && e.isEqual(c) ? v` and called the comparison with `c == null` for any document + * missing that column -- MEASURED on ES 8.18: `NullPointerException: + * ChronoLocalDate.toEpochDay() because "other" is null`. `compareTo` on the `Varchar` arm has + * exactly the same hole. + * + * ⚠️ The candidate is ALWAYS bound before it is guarded, so `c != null` costs one reference + * and never a second rendering of its chain. See the call site for why no predicate decides + * it: none of the three the AST offers can see a function-wrapped column's nullability. + */ + private[this] def checkCase( + e: String, + c: String, + v: String, + candidateNullable: Boolean + ): String = { + val guard = if (candidateNullable) s"$e != null && $c != null" else s"$e != null" + // πŸ”΄ A TIME is decided by what the RECEIVER RENDERS, not by `out` (issue #373, found by + // review). `out` is the CASE's RESULT type -- the least common supertype of the expression, + // the conditions and the results -- and it reports TIMESTAMP for + // `CASE CAST(d AS TIME) WHEN CAST(ts AS TIME) …` whose operands are both `LocalTime`, so an + // arm keyed on it was DEAD and that shape still emitted `isEqual`. MEASURED on ES 8.18: + // `dynamic method [java.time.LocalTime, isEqual/1] not found`, the very failure this PR's + // first commit exists to remove. `Identifier.chainType` (#367) is the derivation that + // describes the rendering -- the same key `NULLIF` needed. + if (receiverIsTime) s"$guard && $e.compareTo($c) == 0 ? $v" + else + out match { + case SQLTypes.Varchar => + s"$guard && $e.compareTo($c) == 0 ? $v" + case _: SQLTemporal => + s"$guard && $e.isEqual($c) ? $v" + // `==` is null-safe in Painless, so a numeric / boolean candidate needs no guard. + case _ => s"$e == $c ? $v" + } } /** A result rendering that can sit opposite a `null` branch. @@ -376,16 +426,26 @@ package object cond { }) expParam match { case Some(e) => - if (cond.nullable) { - ctx.addParam(LiteralParam(c)) match { - case Some(c) => checkCase(e, c, r) - case _ => checkCase(e, c, r) - } - } else { - checkCase(e, c, r) + // πŸ”΄ ALWAYS bound, ALWAYS guarded (issue #373, found by review). The + // earlier version asked `cond.nullable`, and MEASURED that is false for a + // function-wrapped column -- `UPPER(other)` 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"` on ES 8.18. + // + // No predicate decides it any more, because none of the three I measured + // is reliable: the wrapper's `name` is EMPTY, its `nullable` is false, and + // its function is not a `FunctionWithIdentifier`, so the inner column is + // not reachable from here. The unconditional rule is provably safe + // instead: binding is a no-op when the candidate is already a name + // (`addParam` returns the existing one), and `c != null` on a candidate + // that cannot be null is redundant bytes, never a different answer. + ctx.addParam(LiteralParam(c)) match { + case Some(bound) => checkCase(e, bound, r, candidateNullable = true) + case _ => checkCase(e, c, r, candidateNullable = false) } case _ => - checkCase(e, c, r) + checkCase(e, c, r, candidateNullable = false) } } .mkString(" : ") diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala index 2c6a4ba3..fa2364ef 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala @@ -124,7 +124,7 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { */ "NULLIF over a TIME receiver" should "use the same spelling" in { fieldOf("SELECT NULLIF(CAST(d AS TIME), CAST('00:00:00' AS TIME)) AS c FROM t") should - include("param2 == null || param2.compareTo(param3) == 0 ? null : param2") + include("param2 == null || (param3 != null && param2.compareTo(param3) == 0) ? null : param2") } it should "leave the ordering comparisons alone" in { @@ -132,4 +132,84 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { predicateOf("SELECT name FROM t WHERE CAST(d AS TIME) > CAST('00:30:00' AS TIME)") should endWith("(param1.isAfter(param2))") } + + // -- item 3: a CASE candidate that can be null must be guarded --------------------------------- + + /** `checkCase` emitted `e != null && e.isEqual(c) ? v` and called the comparison with a `null` + * candidate for any document missing the WHEN column. MEASURED on ES 8.18 over the very 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 …` failed the whole query on any index where that + * column is not set on every document. + */ + "a CASE candidate that can be null" should "be guarded before the comparison" in { + fieldOf("SELECT CASE ts WHEN d THEN d ELSE ts END AS c FROM t") shouldBe + "def param1 = (doc['ts'].size() == 0 ? null : doc['ts'].value); " + + "def param2 = (doc['d'].size() == 0 ? null : doc['d'].value); " + + "param1 != null && param2 != null && param1.isEqual(param2) ? param2 : param1" + } + + /** πŸ”΄ The TIME spelling must follow the RECEIVER, not `out`. Found by review: an arm keyed on + * `out` -- the CASE's RESULT type, the least common supertype of expression, conditions and + * results -- was DEAD, because `out` reports TIMESTAMP for a CASE whose operands are both + * `LocalTime`. MEASURED on ES 8.18 before this: `dynamic method [java.time.LocalTime, isEqual/1] + * not found`, the very failure this PR's first commit exists to remove. `Identifier.chainType` + * (#367) is the derivation that describes the rendering -- the same key `NULLIF` needed, and not + * carrying it across is what left the arm dead. + */ + it should "spell a TIME comparison from the receiver, not from the CASE's result type" in { + val emitted = fieldOf( + "SELECT CASE CAST(d AS TIME) WHEN CAST(ts AS TIME) " + + "THEN CAST('01:00:00' AS TIME) ELSE CAST('02:00:00' AS TIME) END AS c FROM t" + ) + withClue(emitted) { + emitted should include("param2 != null && param5 != null && param2.compareTo(param5) == 0") + emitted should not include "isEqual" + } + } + + /** πŸ”΄ `NULLIF` had the same hole one method up in the same file, and shipping the CASE half alone + * would have made this commit's own claim -- that the sites cannot drift -- false again by the + * next release (found by review). MEASURED on ES 8.18 over a document with no `ts`: before, + * `null_pointer_exception: Cannot read field "hour" because "other" is null`; after, that + * document yields its OWN time. + * + * πŸ”΄ The guard means NOT-EQUAL, not null: SQL says `NULLIF(a, b)` is NULL when `a = b`, and with + * `b` NULL the comparison is UNKNOWN, so the answer is `a`. Short-circuiting `b == null` to the + * NULL branch would have inverted it -- executed, the document without `ts` correctly returns + * `00:00:00` rather than null. + */ + it should "guard NULLIF's second argument, without inverting its semantics" in { + fieldOf("SELECT NULLIF(CAST(d AS TIME), CAST(ts AS TIME)) AS c FROM t") should include( + "def param5 = param3 == null || (param4 != null && param3.compareTo(param4) == 0) ? null : param3" + ) + } + + it should "guard a candidate whose NULLABILITY the AST does not report" in { + // πŸ”΄ Found by review. The first version asked `cond.nullable`, and MEASURED that is FALSE for + // a function-wrapped column: `UPPER(other)` reports `nullable = false` while rendering + // `(param3 == null) ? null : param3.toUpperCase()`, so the candidate was neither bound nor + // guarded and `CASE UPPER(name) WHEN UPPER(other) …` threw + // `null_pointer_exception: Cannot read field "value"` on ES 8.18 for a document missing the + // column. None of the three structural predicates I measured can see it -- the wrapper's + // `name` is EMPTY, its `nullable` is false, and its function is not a `FunctionWithIdentifier` + // -- so the candidate is now ALWAYS bound and ALWAYS guarded, which is provably safe: + // binding is a no-op when it is already a name, and a redundant `!= null` cannot change an + // answer. + val emitted = fieldOf("SELECT CASE UPPER(name) WHEN UPPER(other) THEN 1 ELSE 0 END AS c FROM t") + withClue(emitted) { + emitted should include("param2 != null && param4 != null && param2.compareTo(param4) == 0") + } + } + + it should "leave a non-temporal CASE on the null-safe ==" in { + // `==` is null-safe in Painless, so a numeric candidate needs no `!= null` guard -- only the + // binding, which is what keeps a chain from being rendered twice. + fieldOf("SELECT CASE n WHEN 1 THEN 1 ELSE 0 END AS c FROM t") should endWith( + "def param3 = 1; param2 == param3 ? 1 : 0" + ) + } } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala index 856bfd5c..e8b7c941 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/ParameterIdentitySpec.scala @@ -289,7 +289,11 @@ class ParameterIdentitySpec extends AnyFlatSpec with Matchers { "def param2 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate().minus(3, ChronoUnit.DAYS)); " + "def param3 = (doc['lastUpdated'].size() == 0 ? null : doc['lastUpdated'].value.toLocalDate()); " ) - emitted should endWith("? param3 : param1 != null && param1.isEqual(param4) ? param5 : param6") + // The candidate is guarded too (issue #373): `lastSeen` is a column and a document may not + // have it, which was an NPE before. + emitted should endWith( + "? param3 : param1 != null && param4 != null && param1.isEqual(param4) ? param5 : param6" + ) // Rule 2, normalisation FIRST: the branch's DATE narrowing was appended to this object before // the chain rendered; the UTC normalisation must still come first (`LocalDate` has no // `toInstant()`). From 908b58943889e63a374987a766390882d93894b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 19 Sep 2026 19:30:16 +0200 Subject: [PATCH 3/8] fix(sql): the PROCESSOR context keys its parsed temporal base on the chain (#373, 3/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- .../app/softnetwork/elastic/sql/package.scala | 60 ++++++++++++++++--- .../sql/query/PainlessResidualsSpec.scala | 50 ++++++++++++++++ 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala index add1c2f7..d8a9b9fc 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -414,9 +414,26 @@ package object sql { } - case class LiteralParam(literal: String, maybeCheckNotNull: Option[String] = None) - extends PainlessParam { + /** @param key + * the identity this parameter is deduplicated on, when it differs from the literal. + * + * πŸ”΄ Issue #373. An identifier registers as a `LiteralParam` in a PROCESSOR or TRANSFORM context + * (its operand is `ctx.` / `doc[''].value`, not a doc-value read), so + * `Identifier.contextKey` -- #370's fix -- never applied there and two different chains over one + * column collapsed onto a single parameter again: `SCRIPT AS (CASE WHEN YEAR(d) > MONTH(d) …)` + * emitted `….get(ChronoField.YEAR).get(ChronoField.MONTH_OF_YEAR)` on ONE `ctx.d`. + * + * ⚠️ Used at ONE site -- the processor's parsed temporal base. Keying the identifier + * registrations themselves was measured to fix nothing and to cost a DEAD duplicate declaration, + * so it is deliberately not done; see the call site. + */ + case class LiteralParam( + literal: String, + maybeCheckNotNull: Option[String] = None, + key: Option[String] = None + ) extends PainlessParam { override def param: String = literal + override def contextKey: String = key.getOrElse(literal) override def sql: String = "" override def nullable: Boolean = maybeCheckNotNull.nonEmpty override def checkNotNull: String = maybeCheckNotNull.getOrElse("") @@ -468,14 +485,20 @@ package object sql { case identifier: Identifier if isProcessor => if (identifier.name.nonEmpty) addParam( - LiteralParam(identifier.processParamName, None /*identifier.processCheckNotNull*/ ) + LiteralParam( + identifier.processParamName, + None /*identifier.processCheckNotNull*/ + ) ) else None case identifier: Identifier if isTransform => if (identifier.name.nonEmpty) addParam( - LiteralParam(identifier.transformParamName, identifier.transformCheckNotNull) + LiteralParam( + identifier.transformParamName, + identifier.transformCheckNotNull + ) ) else None @@ -510,10 +533,18 @@ package object sql { def get(token: Token): Option[String] = { token match { case identifier: Identifier if isProcessor => - get(LiteralParam(identifier.processParamName, None /*identifier.processCheckNotNull*/ )) + get( + LiteralParam( + identifier.processParamName, + None /*identifier.processCheckNotNull*/ + ) + ) case identifier: Identifier if isTransform => get( - LiteralParam(identifier.transformParamName, None /*identifier.transformCheckNotNull*/ ) + LiteralParam( + identifier.transformParamName, + None /*identifier.transformCheckNotNull*/ + ) ) case param: PainlessParam => // By `contextKey`, not `equals`: see `PainlessParam.contextKey` (issue #370). @@ -1587,7 +1618,13 @@ package object sql { context match { case Some(ctx) => processorBase - .flatMap(e => ctx.addParam(LiteralParam(e))) + // πŸ”΄ Keyed on the CHAIN, like every other registration (issue #373). The parsed + // processor base is the SAME literal for every chain over one column, so `YEAR(d)` + // and `MONTH(d)` collapsed onto one parameter and both folds landed on it: + // `….get(ChronoField.YEAR).get(ChronoField.MONTH_OF_YEAR)`, an `int.get(…)` that + // throws -- and in an ingest pipeline `ignore_failure: true` swallows it and the + // computed column is simply ABSENT. + .flatMap(e => ctx.addParam(LiteralParam(e, None, Some(contextKeyOf(e))))) .orElse(ctx.addParam(this)) .getOrElse("") case _ => @@ -1635,10 +1672,15 @@ package object sql { * MONTH)` no longer collapse onto one object, and neither do `YEAR(d)` and `MONTH(d)`, which * used to produce `….get(ChronoField.YEAR).get(ChronoField.MONTH_OF_YEAR)`. */ - override lazy val contextKey: String = + override lazy val contextKey: String = contextKeyOf(paramName) + + /** The same identity rule over an arbitrary operand rendering, so a PROCESSOR (`ctx.`) + * or TRANSFORM (`doc[''].value`) registration keys on its chain too (issue #373). + */ + def contextKeyOf(base: String): String = // The folded RENDERINGS, not the SQL spellings: `YEAR(d)` and `EXTRACT(YEAR FROM d)` fold // the same `.get(ChronoField.YEAR)` and must share one parameter (review, L1). - foldedFunctions.foldLeft(paramName) { (key, f) => + foldedFunctions.foldLeft(base) { (key, f) => key + (f match { case ps: PainlessScript => Try(ps.painless(None)).getOrElse(f.toSQL("")) case _ => f.toSQL("") diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala index fa2364ef..8f64f7a8 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala @@ -62,6 +62,20 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { s"$ctx$body" } + /** The ingest-processor source of a computed column, through the production seam + * (`Table.update()` resolves the processors; the raw parsed column list does not). + */ + protected def processorOf(sql: String, column: String = "c"): String = + Parser(sql) match { + case Right(ct: CreateTable) => + ct.schema.columns + .find(_.name == column) + .flatMap(_.script) + .map(_.source) + .getOrElse(fail(s"no script processor for [$column] in [$sql]")) + case other => fail(s"[$sql] expected a CreateTable, got $other") + } + /** The first SELECT field rendered as one script. */ protected def fieldOf(sql: String, withSchema: Boolean = true): String = { val ctx = PainlessContext(PainlessContextType.Query) @@ -212,4 +226,40 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { "def param3 = 1; param2 == param3 ? 1 : 0" ) } + + // -- item 6: the PROCESSOR and TRANSFORM contexts key on the chain too ------------------------ + + /** #370 made one parameter per (column, folded chain) -- but only in a QUERY context. An + * identifier registers as a `LiteralParam` in a PROCESSOR (`ctx.`) or TRANSFORM + * (`doc[''].value`) context, so `Identifier.contextKey` never applied and two chains over + * one column collapsed onto a single parameter, both folds landing on it: + * `….get(ChronoField.YEAR).get(ChronoField.MONTH_OF_YEAR)`, an `int.get(…)`. + * + * ⚠️ 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 (`param2 instanceof String ? … : + * Instant.ofEpochMilli(param2)…`), which is issue #373's item 7 and its own commit. MEASURED on + * ES 8.18 as an ingest pipeline over `{"d":"2025-01-10"}`: the computed column is ABSENT before + * AND after this commit -- `ignore_failure: true` swallows the throw, which is exactly why the + * defect was invisible. The executed proof of the pair lives with the item-7 commit; what is + * asserted here is the parameter split, which is what this commit owns. + */ + "two different chains over one column in a PROCESSOR" should "be two parameters" in { + val emitted = processorOf( + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (CASE WHEN YEAR(d) > MONTH(d) THEN 1 ELSE 0 END))" + ) + withClue(emitted) { + emitted should not include ".get(ChronoField.YEAR).get(ChronoField.MONTH_OF_YEAR)" + emitted should include(".get(ChronoField.YEAR); ") + emitted should include(".get(ChronoField.MONTH_OF_YEAR); ") + } + } + + it should "keep ONE parameter where the chain is the same" in { + // The single-chain processor emission must not move: one column, one chain, one parameter. + processorOf("CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (YEAR(d)))") shouldBe + "def param1 = (ctx.d instanceof String ? " + + "LocalDate.parse((ctx.d).replace(\"/\", \"-\"), DateTimeFormatter.ofPattern(\"yyyy-MM-dd\"))" + + ".atStartOfDay(ZoneId.of('Z')) : Instant.ofEpochMilli(ctx.d).atZone(ZoneId.of('Z')))" + + ".get(ChronoField.YEAR); ctx.c = param1" + } } From baa463ace386d49f32b013abe95529f1a2603bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 19 Sep 2026 19:47:01 +0200 Subject: [PATCH 4/8] perf(sql): a guarded operand is bound once instead of rendered twice (#373, 4/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../elastic/sql/SQLQuerySpec.scala | 2 +- .../elastic/sql/SQLQuerySpec.scala | 2 +- .../elastic/sql/type/SQLTypeUtils.scala | 22 ++++++++- .../sql/query/PainlessResidualsSpec.scala | 49 +++++++++++++++++++ 4 files changed, 71 insertions(+), 4 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index b961061c..2939e66c 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -1692,7 +1692,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param3 = ((param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))) != null ? (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))).toLocalDate() : null); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param3, param2))" + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param3 = (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))); def param4 = (param3 != null ? param3.toLocalDate() : null); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param4, param2))" | } | } | } diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 99ba13a0..5177ce37 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -1692,7 +1692,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param3 = ((param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))) != null ? (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))).toLocalDate() : null); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param3, param2))" + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param3 = (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))); def param4 = (param3 != null ? param3.toLocalDate() : null); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param4, param2))" | } | } | } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala index 6213643b..b32190f2 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala @@ -571,8 +571,26 @@ object SQLTypeUtils { // literal arms already return references (`LocalDate.parse(...)`, `String.valueOf(...)`) and // must stay BYTE-IDENTICAL β€” a blanket `(def)` moved a bridge pin that was never broken, which // is churn, not a correction. - if (producesPainlessPrimitive(to)) s"($expr != null ? (def)($ret) : null)" - else s"($expr != null ? $ret : null)" + // + // πŸ”΄ The operand is BOUND ONCE when it is a compound expression (issue #373). `ret` is built + // by interpolating `expr`, and this wrapper tests `expr` again, so a chained operand was + // rendered TWICE and every function in it ran twice per document: + // `DATE_ADD(DATE_PARSE(name, …), INTERVAL 1 DAY) > d` emitted + // `((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, it + // returns the right row -- so this is cost, not a wrong answer. + // + // The substitution is exact rather than textual luck: every occurrence of `expr` inside `ret` + // is this operand, because `ret` was interpolated from it. A bare parameter name is left + // alone: binding it would declare a second name for one that already exists. + // ⚠️ No "is it already a name?" test: `addParam` returns the EXISTING name when the literal + // it is given is one (`_values.indexOf`), so binding a bare `param2` is already a no-op. A + // guard here would be belt-and-braces that no mutation can redden -- measured. + val guardOperand: String = + context.flatMap(_.addParam(LiteralParam(expr))).getOrElse(expr) + val guarded = if (guardOperand == expr) ret else ret.replace(expr, guardOperand) + if (producesPainlessPrimitive(to)) s"($guardOperand != null ? (def)($guarded) : null)" + else s"($guardOperand != null ? $guarded : null)" } /** The one normalisation the four ` -> ` arms share, so that a literal written diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala index 8f64f7a8..b6984ed3 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala @@ -76,6 +76,10 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { case other => fail(s"[$sql] expected a CreateTable, got $other") } + /** How many times `needle` occurs in `haystack` -- the shape of an "evaluated once" assertion. */ + protected def countOf(haystack: String, needle: String): Int = + haystack.sliding(needle.length).count(_ == needle) + /** The first SELECT field rendered as one script. */ protected def fieldOf(sql: String, withSchema: Boolean = true): String = { val ctx = PainlessContext(PainlessContextType.Query) @@ -262,4 +266,49 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { ".atStartOfDay(ZoneId.of('Z')) : Instant.ofEpochMilli(ctx.d).atZone(ZoneId.of('Z')))" + ".get(ChronoField.YEAR); ctx.c = param1" } + + // -- item 9: a guarded operand is bound once, not rendered twice ------------------------------- + + /** `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: + * {{{ + * ((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 -- + * so this is COST, not a wrong answer, and the pin is on the count rather than on a value. + */ + "a chained operand under a null guard" should "be rendered once" in { + val emitted = + predicateOf( + "SELECT name FROM t WHERE DATE_ADD(DATE_PARSE(name, 'yyyy-MM-dd'), INTERVAL 1 DAY) > d" + ) + withClue(emitted) { + countOf(emitted, "param2.plus(1, ChronoUnit.DAYS)") shouldBe 1 + } + } + + it should "bind the operand of a chained conversion once too" in { + val emitted = fieldOf( + "SELECT CASE CAST(d AS DATE) WHEN CAST('2025-01-01' AS DATE) THEN YEAR(CAST(d AS DATE)) ELSE 0 END AS c FROM t" + ) + withClue(emitted) { + countOf(emitted, "param1.toInstant().atZone(ZoneId.of('Z')).toLocalDate()") shouldBe 1 + } + } + + it should "reuse an existing parameter instead of declaring an alias" in { + // πŸ”΄ Two earlier versions of this test were VACUOUS -- neither reddened when the binding was + // made unconditional -- and that is the finding, not a detail: `addParam` returns the EXISTING + // name when the literal it is handed IS one, so a bare-name operand was never going to produce + // a duplicate. The redundant guard was removed rather than left as unfalsifiable code, and + // what is asserted here is the property that actually holds: no parameter is declared as a + // bare alias of another. + val emitted = + fieldOf("SELECT CASE CAST(d AS DATE) WHEN CAST(ts AS DATE) THEN 1 ELSE 0 END AS c FROM t") + withClue(emitted) { + """def \w+ = (?:param|left|arg)\d+;""".r.findFirstIn(emitted) shouldBe None + } + } } From 9bb0ad7e8c76beaf25ab3f9a8941c2fed591bd1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 19 Sep 2026 20:31:10 +0200 Subject: [PATCH 5/8] fix(bridge): a column compared with another column runs as a script (#373, 5/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../elastic/sql/bridge/package.scala | 23 +++- .../elastic/sql/ColumnVsColumnSpec.scala | 104 ++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 bridge/src/test/scala/app/softnetwork/elastic/sql/ColumnVsColumnSpec.scala diff --git a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala index 955a8422..bb88acb4 100644 --- a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala +++ b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala @@ -690,6 +690,23 @@ package object bridge { case _ => true })) + /** The same question asked of BOTH operands (issue #373). + * + * πŸ”΄ `requiresScript` only ever inspected the LEFT identifier, so a comparison of one column + * with another -- which the ES query DSL cannot express at all -- fell through to `rangeQuery` + * keyed on the left field NAME, with the RIGHT column rendered as DATE MATH. MEASURED: `WHERE d + * > DATE_TRUNC(ts, MONTH)` emitted `{"range":{"d":{"gt":"ts||/M"}}}` and Elasticsearch answered + * `parse_exception: failed to parse date field [ts] with format + * [strict_date_optional_time||epoch_millis]` -- it read the column NAME as a date literal. A + * range can only compare a field with a CONSTANT; two fields need the script path, which renders + * both sides and already emits a correct comparison. + */ + private[bridge] def requiresScript(identifier: Identifier, maybeValue: Option[Token]): Boolean = + requiresScript(identifier) || maybeValue.exists { + case id: Identifier => id.name.trim.nonEmpty + case _ => false + } + private[bridge] def scriptQueryOf(criteria: Criteria)(implicit timestamp: Long, contextType: PainlessContextType @@ -713,7 +730,7 @@ package object bridge { import expression._ if (isAggregation) return matchAllQuery() - if (requiresScript(identifier)) return scriptQueryOf(expression) + if (requiresScript(identifier, maybeValue)) return scriptQueryOf(expression) // Geo distance special case identifier.functions.headOption match { case Some(d: Distance) => @@ -1016,6 +1033,10 @@ package object bridge { contextType: PainlessContextType = PainlessContextType.Query ): Query = { import between._ + // ⚠️ NOT the two-operand form: `BetweenExpr.maybeValue` is always a `FromTo`, never an + // `Identifier`, so passing it here 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. if (requiresScript(identifier)) return scriptQueryOf(between) // Geo distance special case identifier.functions.headOption match { diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/ColumnVsColumnSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/ColumnVsColumnSpec.scala new file mode 100644 index 00000000..4706ac41 --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/ColumnVsColumnSpec.scala @@ -0,0 +1,104 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query._ +import app.softnetwork.elastic.sql.schema.{Column, Table} +import app.softnetwork.elastic.sql.`type`.SQLTypes +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime + +/** Issue #373, item 4 β€” a comparison of one column with another must run as a SCRIPT. + * + * `requiresScript` only ever inspected the LEFT identifier, so a column-to-column comparison -- + * 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. MEASURED on ES 8.18: + * `{"range":{"d":{"gt":"ts||/M"}}}` answered `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. + * + * Executed on a real index (`a` d=2025-01-01/ts=2025-01-01, `b` d=2025-01-05/ts=2025-01-04, `c` + * d=2025-01-05 with NO ts): before, the shard error above; after, `['b']`, which is the truth. + */ +class ColumnVsColumnSpec extends AnyFlatSpec with Matchers { + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + private val schema: Table = Table( + "t", + columns = List( + Column("d", SQLTypes.Date), + Column("ts", SQLTypes.Timestamp), + Column("name", SQLTypes.Keyword), + Column("n", SQLTypes.Int) + ) + ) + + /** πŸ”΄ A schema-CARRYING parse, and that is what makes this spec able to see the defect at all. + * Without one every identifier stays `Any`, the date-math branch is never taken, and BOTH the + * fixed and the unfixed engine emit a script -- the first version of this spec passed with the + * fix reverted for exactly that reason. It is #306's rule again: the no-schema rendering is not + * the one a live query takes. + */ + private def queryOf(sql: String): String = + Parser(sql) match { + case Right(ss: SingleSearch) => + val request: ElasticSearchRequest = ss.update(Some(schema)) + request.query.replaceAll("\\s+", "") + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + "a comparison of one column with a function of another" should "run as a script" in { + val q = queryOf("SELECT name FROM t WHERE d > DATE_TRUNC(ts, MONTH)") + withClue(q) { + q should include("\"script\"") + q should not include "\"range\"" + q should include("doc['ts']") + q should not include "ts||/M" + } + } + + it should "run as a script for two bare columns too" in { + val q = queryOf("SELECT name FROM t WHERE d > ts") + withClue(q) { + q should include("\"script\"") + q should not include "\"range\"" + q should not include "ts||" + } + } + + "a comparison against a CONSTANT" should "still use the query DSL" in { + // The range/term/terms paths must not move: they are what makes a filter cheap. + queryOf("SELECT name FROM t WHERE d > '2025-01-01'") should include( + "\"range\":{\"d\":{\"gt\":\"2025-01-01\"}}" + ) + queryOf("SELECT name FROM t WHERE n > 3") should include("\"range\":{\"n\":{\"gt\":3}}") + queryOf("SELECT name FROM t WHERE name = 'x'") should include("\"term\"") + queryOf("SELECT name FROM t WHERE n IN (1,2)") should include("\"terms\"") + queryOf("SELECT name FROM t WHERE d BETWEEN '2025-01-01' AND '2025-02-01'") should include( + "\"range\"" + ) + } + + "a function-wrapped LEFT operand" should "keep running as a script" in { + queryOf("SELECT name FROM t WHERE UPPER(name) = 'X'") should include("\"script\"") + } +} From 499f3a26c7acf39812c82e2096931850c7e214ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 19 Sep 2026 23:14:55 +0200 Subject: [PATCH 6/8] fix(sql): arithmetic renders its operands' chains instead of their parameter names (#373, 6/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../elastic/sql/SQLQuerySpec.scala | 2 +- .../elastic/sql/SQLQuerySpec.scala | 2 +- .../operator/math/ArithmeticExpression.scala | 150 ++++++++--- .../app/softnetwork/elastic/sql/package.scala | 63 +++++ .../query/PainlessExpressionFormSpec.scala | 161 ++++++++++++ .../sql/query/PainlessOperandFormSpec.scala | 37 ++- .../sql/query/PainlessResidualsSpec.scala | 233 +++++++++++++++++- 7 files changed, 588 insertions(+), 60 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 2939e66c..86997326 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -2738,7 +2738,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "__c7": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); def lv0 = ((param1 == null || param2 == null) ? null : (param1 * param2)); (lv0 == null) ? null : (lv0 - 10)" + | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); def lv1 = ((param1 == null || param2 == null) ? null : (param1 * param2)); (lv1 == null) ? null : (lv1 - 10)" | } | } | }, diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 5177ce37..f1525fbe 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -2722,7 +2722,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "__c7": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); def lv0 = ((param1 == null || param2 == null) ? null : (param1 * param2)); (lv0 == null) ? null : (lv0 - 10)" + | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); def lv1 = ((param1 == null || param2 == null) ? null : (param1 * param2)); (lv1 == null) ? null : (lv1 - 10)" | } | } | }, diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/operator/math/ArithmeticExpression.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/operator/math/ArithmeticExpression.scala index c953dcf0..effbf090 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/operator/math/ArithmeticExpression.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/operator/math/ArithmeticExpression.scala @@ -68,45 +68,119 @@ case class ArithmeticExpression( case _ => } if (nullable) { - val l = context - .flatMap(ctx => ctx.get(left)) - .getOrElse(left match { - case t: TransformFunction[_, _] => - SQLTypeUtils.coerce( - t.toPainless("", idx + 1, context), - left.baseType, - out, - nullable = false, - context - ) - case _ => - SQLTypeUtils - .coerce(left.painless(context), left.baseType, out, nullable = false, context) - }) - val r = context - .flatMap(ctx => ctx.get(right)) - .getOrElse(right match { - case t: TransformFunction[_, _] => - SQLTypeUtils.coerce( - t.toPainless("", idx + 1, context), - right.baseType, - out, - nullable = false, - context - ) - case _ => - SQLTypeUtils - .coerce(right.painless(context), right.baseType, out, nullable = false, context) - }) + // πŸ”΄ The operand is RENDERED, never read back as a parameter NAME (issue #373, item 5). + // `ctx.get(left)` answers with the name of the parameter the identifier registered -- the + // RAW doc-value -- so the function chain never reached the expression and + // `YEAR(d) * 100 + MONTH(d)` computed `d * 100 + d`. It is #367's defect one layer up: the + // shortcut is sound only where the parameter IS the whole operand, which is exactly what a + // bare-name rendering means. MEASURED on the real DDL path, an ingest processor stored + // `ctx.c = (param1 == null || param1 == null) ? null : (param1 + param1)` with + // `param1 = ctx.d` -- the duplicated null guard is the fingerprint of the collapse. + // πŸ”΄ Coerced FROM what the operand RENDERS, not from 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`. + def renderedType(operand: PainlessScript): SQLType = operand match { + case i: Identifier => i.chainType + case other => other.baseType + } + + /** πŸ”΄ A numeric coercion is skipped only over a NULLABLE operand, and the reason is exactly + * what Painless refuses -- a primitive cast over a null-guarded expression, in BOTH + * directions, MEASURED on ES 8.18: + * {{{ + * ((double) (param1 != null ? … : null)) Cannot cast null to a primitive type [double]. + * ((long) (param1 != null ? … : null)) Cannot cast null to a primitive type [long]. + * }}} + * 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. + * + * πŸ”΄ It must NOT be skipped over a NON-nullable operand: a LITERAL carries no guard, and its + * coercion is the only thing that makes the division floating-point. Skipping it turned + * `CAST(n AS DOUBLE) / 2` into INTEGER division -- `[0.5, 2.5, 4.5]` became `[0, 2, 4]`, + * HTTP 200, in script fields, `terms` keys, sort scripts AND predicates (a row silently + * vanished from `WHERE … / 2 > 2`). Found by review, MEASURED on the schema-LESS rendering + * path -- which production reaches whenever `resolveWithSchema` declines: a wildcard or + * multi-index FROM, a JOIN, or a mapping that cannot be loaded. #205's silent-wrong-answer + * family, and NULLABILITY is the discriminator that separates the two failures. + */ + def coerced(rendered: String, operand: PainlessScript): String = + if ( + operand.nullable && renderedType(operand).isInstanceOf[SQLNumeric] && + out.isInstanceOf[SQLNumeric] + ) rendered + else + SQLTypeUtils.coerce(rendered, renderedType(operand), out, nullable = false, context) + + def render(operand: PainlessScript): String = operand match { + case t: TransformFunction[_, _] => coerced(t.toPainless("", idx + 1, context), operand) + case _ => coerced(operand.painless(context), operand) + } + + /** A rendering that IS a Painless name needs no local of its own. */ + def isBareName(e: String): Boolean = + e.nonEmpty && e.forall(c => c.isLetterOrDigit || c == '_') + + /** πŸ”΄ The parameter shortcut is KEPT where there is no chain to drop, and losing it is what + * broke five working predicates in the first version of this commit (found by review). + * `render` always coerces, so a bare `INT` column against a BIGINT target emitted `((long) + * param1)` -- not a name -- and a `def lv0 = …; ` was prepended into a slot that must be a + * single EXPRESSION: `WHERE n + 1 > 2` became `def left1 = def lv0 = …; …`, a compile error, + * MEASURED on ES 8.18 where it returned rows before. + * + * The defect this commit fixes is the chain being DROPPED, so bypassing the shortcut is + * needed exactly when the operand HAS a chain -- and only then. + */ + def shortcut(operand: PainlessScript): Option[String] = operand match { + case i: Identifier if i.functions.isEmpty => context.flatMap(_.get(i)) + case _ => None + } + + /** A rendering that is not an EXPRESSION cannot be placed anywhere an operand goes -- not in + * a prologue local, not inline -- so the operand falls back to its registered parameter. + * That is what this site did before, and it is why `TRY_CAST(name AS BIGINT) + 1` worked: + * Painless has no expression-level `try`, so a safe cast renders as a STATEMENT and coercing + * it produced `def lv1 = String.valueOf(try { … } catch …);`, which ES rejects. Dropping the + * chain is wrong, but a compile error is strictly worse -- recorded as a residual. + */ + def placeable(rendered: String): Boolean = PainlessOperandForm.placeable(rendered) + + def operandOf(operand: PainlessScript): String = + shortcut(operand).getOrElse { + val rendered = render(operand) + if (placeable(rendered)) rendered + else context.flatMap(_.get(operand)).getOrElse(rendered) + } + + val l = operandOf(left) + val r = operandOf(right) var expr = "" - val leftParam = context.flatMap(ctx => ctx.get(left)).getOrElse(s"lv$idx") - val rightParam = context.flatMap(ctx => ctx.get(right)).getOrElse(s"rv$idx") - if (left.nullable) - expr += (if (context.exists(ctx => ctx.get(left).nonEmpty)) "" - else s"def $leftParam = $l; ") - if (right.nullable) - expr += (if (context.exists(ctx => ctx.get(right).nonEmpty)) "" - else s"def $rightParam = $r; ") + + /** πŸ”΄ A Painless `def x = …;` is a STATEMENT: it cannot sit inside a parenthesised operand. A + * non-bare rendering is therefore declared in the script's PROLOGUE, beside the `param` + * assignments, and the operand becomes its NAME. `PainlessContext.bindLocal` exists for + * exactly this (story BIDC-8 AD-9; #367 added `bindLocalWith` for `TRY_CAST`), and splicing + * the declaration INLINE instead is what broke every CAST operand in the earlier versions of + * this commit -- found by review, twice. `WHERE CAST(n AS BIGINT) + 1 > 2` emitted `def + * left1 = def lv0 = …;` and ES answered `invalid_argument_exception: invalid sequence of + * tokens near ['def']`. + * + * Only the side the null guard NAMES is bound: binding the other would leave a declaration + * nothing reads. With NO context there is no prologue to hoist into, so that rendering keeps + * the inline form it has always had -- pre-existing, and not this commit's to change. + */ + def bind(rendered: String, fallback: String): String = + context match { + case Some(ctx) => ctx.bindLocal(rendered, "lv") + case None => + expr += s"def $fallback = $rendered; " + fallback + } + + val leftParam = + if (left.nullable && !isBareName(l)) bind(l, s"lv$idx") else l + val rightParam = + if (right.nullable && !isBareName(r)) bind(r, s"rv$idx") else r if (left.nullable && right.nullable) expr += s"($leftParam == null || $rightParam == null) ? null : ($leftParam ${operator .painless(context)} $rightParam)" diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala index d8a9b9fc..ace063f6 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -447,6 +447,69 @@ package object sql { case object Transform extends PainlessContextType } + /** Is a rendered Painless fragment a single EXPRESSION, i.e. may it be placed where an operand + * goes? [[PainlessOperandForm.placeable]] is the one owner of that rule. + * + * A `def x = …;` and a `try { … } catch { … }` are STATEMENTS: Painless allows them as the tail + * of a whole script -- which is why a `script_fields` projection of a safe cast works -- and + * NOWHERE an operand may go. [[PainlessContext.bindLocal]] hoists the first kind into the + * prologue; the second kind cannot be hoisted as an expression at all (see + * [[PainlessContext.bindLocalWith]]) and its caller must fall back to a parameter. + * + * πŸ”΄ The scan SKIPS string literals, which is the whole reason it is not a `contains`. A `;` a + * user put in a format or a pattern (`DATE_FORMAT(d, 'yyyy;MM')`) must not be read as a + * statement separator: a caller that demotes on it drops the operand's function chain, a SILENT + * wrong answer. + * + * πŸ”΄ THE ASYMMETRY IS DELIBERATE -- when in doubt, prefer the LOUD direction. Answering + * "statement" about an expression loses a chain silently; answering "expression" about a + * statement splices it into an operand slot and Elasticsearch says `compile error`. Every + * limitation below therefore sits on the loud side. + * + * Invariants the cases in `PainlessExpressionFormSpec` pin, each for a reason: + * - ONE `quote` character, not two booleans: an apostrophe inside a double-quoted pattern + * (`"it's;ok"`) or a quote inside a single-quoted one (`'say "hi";'`) would corrupt a + * two-flag reading. What matters is WHICH delimiter opened the literal. + * - `\` consumes the backslash AND the next character. An escaped BACKSLASH therefore CLOSES + * the literal, so `foo("a\\") ; x` really does contain a separator. A scanner that only + * special-cased `\"` would read the closing quote as escaped, swallow the rest of the + * script, miss that `;` and splice a statement. + * - `try` counts only on a word boundary: `entry `, `retry`, `a_try ` and `registry` all + * contain it. + * + * Accepted limitations, all LOUD-direction, pinned as characterisation so nobody "fixes" one + * silently: the `try` arm keys on the spelling `try `, so `try{ x }` and a bare `try` are not + * matched; and an unterminated literal swallows a following real `;` -- in a script + * Elasticsearch rejects anyway. Bounds-safe: the escape step may run past the end, which the + * loop guard catches. + */ + private[sql] object PainlessOperandForm { + + /** Where the first statement boundary is, or `None` for a pure expression. Callers that only + * decide use [[placeable]]; this variant exists so a diagnostic can NAME the offending spot. + */ + def firstStatementAt(rendered: String): Option[Int] = { + var i = 0 + var quote = '\u0000' + var found = -1 + while (i < rendered.length && found < 0) { + val c = rendered.charAt(i) + if (quote != '\u0000') { + if (c == '\\') i += 1 else if (c == quote) quote = '\u0000' + } else if (c == '\'' || c == '"') quote = c + else if (c == ';') found = i + else if ( + rendered.startsWith("try ", i) && + (i == 0 || !(rendered.charAt(i - 1).isLetterOrDigit || rendered.charAt(i - 1) == '_')) + ) found = i + i += 1 + } + if (found < 0) None else Some(found) + } + + def placeable(rendered: String): Boolean = firstStatementAt(rendered).isEmpty + } + /** Context for painless scripts * @param context * the context type diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala new file mode 100644 index 00000000..55ed4598 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala @@ -0,0 +1,161 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.PainlessOperandForm +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.scalatest.prop.TableDrivenPropertyChecks + +/** `PainlessOperandForm.placeable` decides whether a rendered fragment may sit where an operand + * goes. `ArithmeticExpression` falls back to a parameter when it answers false (issue #373, item + * 5), and `PainlessOperandFormSpec` uses it as the tree-wide guard over rendered criteria. + * + * πŸ”΄ WHY THIS SPEC EXISTS. At the emission sites the predicate is INERT today: no reachable SQL + * shape renders an operand carrying a `;` inside a string literal, and the `try` arm never decides + * anything either, because both Painless `try` emitters (`function/convert/package.scala`) always + * emit a `;` as well -- so the `;` arm disqualifies first. Review PROVED that by mutation: + * blinding the literal handling, and separately blinding the `try` arm, left every other spec + * green. A guard nothing can see is not a guard, so the rule is exercised HERE, one case per arm, + * with the inputs review actually falsified. + * + * πŸ”΄ The two wrong answers are NOT symmetric, and every accepted limitation sits on the safe side + * of that. Answering "statement" about an expression demotes the operand and drops its function + * chain -- a SILENT wrong answer, the failure this predicate exists to prevent. Answering + * "expression" about a statement splices it into an operand slot, and Elasticsearch answers + * `compile error`. When in doubt, prefer the LOUD direction. + */ +class PainlessExpressionFormSpec extends AnyFlatSpec with Matchers with TableDrivenPropertyChecks { + + private def placeable(rendered: String): Boolean = PainlessOperandForm.placeable(rendered) + + "a rendering with no statement boundary" should "be placeable" in { + forAll( + Table( + "rendered", + """param1 + 1""", + """param2 ? 1 : 0 + 1""", + """(param1 != null ? (def)(((long) param1)) : null)""", + """ZoneId.of('Z')""", + "" + ) + )(r => withClue(s"[$r]")(placeable(r) shouldBe true)) + } + + "a `;` or a `try` outside a literal" should "not be placeable" in { + forAll( + Table( + "rendered", + """def lv0 = x; y""", + """try { x } catch (Exception e) {}""", + """String.valueOf(try { x } catch (Exception e) { return null; })""", + """foo("x") ; try { }""", + """x ; """ + ) + )(r => withClue(s"[$r]")(placeable(r) shouldBe false)) + } + + /** πŸ”΄ The arm that prevents the silent failure: a `;` a user wrote inside a format or a pattern + * is DATA. Reading it as a separator demotes the operand and drops its chain. + */ + "a `;` inside a string literal" should "be data, not a separator" in { + forAll( + Table( + "rendered", + """param1.replace(";", "x")""", + """DateTimeFormatter.ofPattern("yyyy;MM")""", + """x.foo('a;b')""", + """foo("a\"b;c") + 1""", + """foo("a\\;b")""", + """foo("it's") + 1""", + """foo("it's;ok") + 1""", + """foo('say "hi"') + 1""", + """foo('say "hi";') + 1""", + """foo("try ") + 1""", + """foo('try ') + 1""" + ) + )(r => withClue(s"[$r]")(placeable(r) shouldBe true)) + } + + /** πŸ”΄ The load-bearing case. In the emitted Painless the literal is `"a\\"` -- an ESCAPED + * backslash -- so the second quote really does terminate the string and the `;` after it is + * code. It only comes out right because the scan consumes `\\` plus the NEXT character as a + * pair. A scan that special-cased only `\"` would read the closing quote as escaped, swallow the + * rest of the script, miss that `;`, and splice a statement into an operand slot. + */ + "an escaped backslash" should "close the literal, so a following `;` still counts" in { + forAll( + Table( + ("rendered", "placeableForm"), + ("""foo("a\\") ; x""", false), // the escape CLOSES the literal + ("""foo("a\\;b")""", true) // same escape, `;` genuinely inside + ) + )((r, expected) => withClue(s"[$r]")(placeable(r) shouldBe expected)) + } + + /** `try` counts only on a word boundary -- otherwise `entry `, `retry` and `registry` all contain + * it, and every one of them would lose its chain. + */ + "a `try` inside an identifier" should "not count" in { + forAll( + Table( + "rendered", + """entry + 1""", + """myentry [entry ] + 1""", + """retry + 1""", + """a_try x""", + """registry + 1""" + ) + )(r => withClue(s"[$r]")(placeable(r) shouldBe true)) + } + + /** Characterisation, NOT aspiration: these are the accepted limitations, all LOUD-direction (a + * compile error, never a wrong value). Pinned so nobody "fixes" one silently and trades a loud + * failure for a quiet one. + */ + "the accepted limitations" should "stay exactly as documented" in { + forAll( + Table( + ("rendered", "placeableForm", "why"), + ("""try{ x }""", true, "the arm keys on the spelling `try ` -- no space, not matched"), + ("""try""", true, "bare `try` at the end of input is not matched"), + ("""x.try foo""", false, "a dot is not a word char, so this one IS matched"), + (""""unterminated ; literal""", true, "an unterminated literal swallows the `;`") + ) + )((r, expected, why) => withClue(s"[$r] -- $why")(placeable(r) shouldBe expected)) + } + + /** Bounds safety only, no semantic claim: a trailing backslash or a lone one must not throw. The + * escape step can advance past the end, and the loop guard is what catches it. + */ + "a malformed rendering" should "answer without throwing" in { + forAll( + Table( + "rendered", + "foo(\"a\\", + "\\", + "'" + ) + )(r => withClue(s"[$r]")(noException should be thrownBy placeable(r))) + } + + "the locating variant" should "agree with the decision and name the spot" in { + PainlessOperandForm.firstStatementAt("""param1 + 1""") shouldBe None + PainlessOperandForm.firstStatementAt("""def lv0 = x; y""") shouldBe Some(11) + PainlessOperandForm.firstStatementAt("""try { x }""") shouldBe Some(0) + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.scala index 367a941f..c8659b8d 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.scala @@ -22,7 +22,7 @@ import app.softnetwork.elastic.sql.parser.Parser // `++ 2.12.20 sql/Test/compile` sees it β€” no CI job compiles sql test sources on 2.12. import app.softnetwork.elastic.sql.schema.{Column, Table => SchemaTable} import app.softnetwork.elastic.sql.`type`.SQLTypes -import app.softnetwork.elastic.sql.{PainlessContext, PainlessContextType} +import app.softnetwork.elastic.sql.{PainlessContext, PainlessContextType, PainlessOperandForm} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -95,28 +95,27 @@ class PainlessOperandFormSpec extends AnyFlatSpec with Matchers { case other => fail(s"[$sql] expected a SingleSearch, got $other") } - /** A `;` that is NOT inside a string literal. Parentheses are irrelevant β€” Painless has no - * bracketed statement sequence that could legitimately appear in an operand. + /** The offending fragment, or `None` when the rendering is a pure expression. + * + * πŸ”΄ The rule has ONE owner in MAIN code: [[PainlessOperandForm]]. This spec used to + * re-implement it as a count of `;` outside a DOUBLE-quoted literal, which is strictly weaker -- + * it did not understand single-quoted literals (so a `;` in a `'yyyy;MM'` pattern counted as a + * separator) and it did not know a `try`/`catch` is a statement too. #373's item 5 needed the + * same predicate at an emission site, so the copy is gone and both callers share it; + * `PainlessExpressionFormSpec` drives its edge cases directly. + * + * πŸ”΄ Keeps the DIAGNOSTIC that count gave, and sharpens it: the offending spot is NAMED, so a + * failure says WHERE the statement starts rather than how many separators there were. */ - private def statementSeparators(rendered: String): Int = { - var inString = false - var i = 0 - var count = 0 - while (i < rendered.length) { - val c = rendered.charAt(i) - if (inString) { - if (c == '\\') i += 1 - else if (c == '"') inString = false - } else if (c == '"') inString = true - else if (c == ';') count += 1 - i += 1 - } - count - } + private def statementFragment(rendered: String): Option[String] = + PainlessOperandForm + .firstStatementAt(rendered) + .map(at => rendered.substring(math.max(0, at - 24), math.min(rendered.length, at + 16))) "a criteria rendered with a PainlessContext" should "be a single expression, never a statement sequence" in { val offenders = shapes.map(sql => sql -> renderedOf(sql)).collect { - case (sql, rendered) if statementSeparators(rendered) > 0 => s"$sql\n => $rendered" + case (sql, rendered) if statementFragment(rendered).isDefined => + s"$sql\n at [...${statementFragment(rendered).get}...]\n => $rendered" } withClue( s"${offenders.size} criteria rendered a STATEMENT SEQUENCE under a context; a `def x = …;` " + diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala index b6984ed3..e7e72e0b 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala @@ -22,6 +22,7 @@ import app.softnetwork.elastic.sql.`type`.SQLTypes import app.softnetwork.elastic.sql.{PainlessContext, PainlessContextType} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers +import org.scalatest.prop.TableDrivenPropertyChecks /** Issue #373 β€” the Painless-emission defects surfaced by #370, fixed in one PR, one commit each. * @@ -34,7 +35,7 @@ import org.scalatest.matchers.should.Matchers * Every expectation below names the Elasticsearch behaviour it was measured against, and the * executed row lives in `PredicateFunctionResultSpec`. */ -class PainlessResidualsSpec extends AnyFlatSpec with Matchers { +class PainlessResidualsSpec extends AnyFlatSpec with Matchers with TableDrivenPropertyChecks { private val schema: SchemaTable = SchemaTable( "t", @@ -80,6 +81,25 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { protected def countOf(haystack: String, needle: String): Int = haystack.sliding(needle.length).count(_ == needle) + /** Every `def` in a well-formed emission opens a STATEMENT, so it may only appear at the start of + * the script or just after a `; `. This is the rule the first version of item 5's commit broke: + * it spliced `def lv0 = …; ` into a slot that must hold a single EXPRESSION, and ES answered + * `compile error` for five predicates that had returned rows. Returns the offending fragments. + */ + protected def declarationsOutsideStatementStart(emitted: String): Seq[String] = + "def ".r + .findAllMatchIn(emitted) + .map(_.start) + .filterNot(i => i == 0 || emitted.startsWith("; ", i - 2)) + .map(i => emitted.substring(math.max(0, i - 24), math.min(emitted.length, i + 12))) + .toSeq + + /** Names declared more than once -- Painless rejects a redeclaration in the same scope. */ + protected def duplicateDeclarations(emitted: String): Seq[String] = { + val names = "def ([A-Za-z_][A-Za-z0-9_]*) =".r.findAllMatchIn(emitted).map(_.group(1)).toSeq + names.groupBy(identity).collect { case (n, xs) if xs.size > 1 => n }.toSeq + } + /** The first SELECT field rendered as one script. */ protected def fieldOf(sql: String, withSchema: Boolean = true): String = { val ctx = PainlessContext(PainlessContextType.Query) @@ -311,4 +331,215 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers { """def \w+ = (?:param|left|arg)\d+;""".r.findFirstIn(emitted) shouldBe None } } + + // -- item 5: arithmetic renders its operands' chains ------------------------------------------ + + /** `ArithmeticExpression.toPainless` read its operands back as parameter NAMES (`ctx.get(left)`) + * -- the RAW doc-value -- so the function chain never reached the expression and `YEAR(d) + + * MONTH(d)` computed `d + d`. #367's defect one layer up. + * + * MEASURED as a real ingest pipeline on ES 8.18 over `{"d":"2025-01-10"}` (truth: 2025 + 1): + * {{{ + * YEAR(d) + MONTH(d) before c = 1 after c = 2026 + * YEAR(d) * 100 + MONTH(d) before c = ABSENT after c = 202501 + * }}} + * The first is the one that matters: a computed column silently stored a WRONG number, and an + * ingest pipeline swallows nothing there -- the value was simply wrong in the index. + * + * The QUERY half moves only for a SINGLE-chain operand, and that half is now executable. + * MEASURED on ES 8.18, index `t` (`d` = 2025-01-01 / 01-05 / 01-09): + * {{{ + * WHERE YEAR(d) + 1 = 2026 before class_cast (ZonedDateTime + 1) after [a, b, c] + * }}} + * ⚠️ A TWO-chain comparison is NOT fixed here and is not made worse -- before and after, `WHERE + * YEAR(d) + MONTH(d) = 2026` fails the shard (`illegal_argument`) and `… * 100 + MONTH(d) = + * 202501` fails to compile, for two PRE-EXISTING reasons this commit does not own: the + * arithmetic node's own type reports its COLUMN's (so the comparison coerces `TIMESTAMP -> + * BIGINT` and emits `.toInstant()` on an `int`), and a NESTED arithmetic renders a statement + * sequence (#367's open family). Correcting the node's type 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. + * + * πŸ”΄ The first version of this commit dropped the parameter shortcut for EVERY operand, not just + * a chained one, and that regressed five working predicates from rows to `compile error` -- + * found by review, MEASURED, and now guarded below by a rule rather than by bytes. + */ + "arithmetic over date functions in a PROCESSOR" should "render each operand's chain" in { + val emitted = processorOf("CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (YEAR(d) + MONTH(d)))") + withClue(emitted) { + emitted should include(".get(ChronoField.YEAR)") + emitted should include(".get(ChronoField.MONTH_OF_YEAR)") + } + } + + it should "render each chain in a nested expression too" in { + val emitted = + processorOf("CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (YEAR(d) * 100 + MONTH(d)))") + withClue(emitted) { + emitted should include(".get(ChronoField.YEAR)") + emitted should include(".get(ChronoField.MONTH_OF_YEAR)") + // the fingerprint of the collapse: one parameter compared with itself in the null guard + emitted should not include "(param1 == null || param1 == null)" + } + } + + it should "leave plain numeric arithmetic alone" in { + val emitted = processorOf("CREATE TABLE t (n INTEGER, c INTEGER SCRIPT AS (n * 2 + 1))") + withClue(emitted) { + emitted should not include "ChronoField" + emitted should not include "instanceof String" + } + } + + /** The QUERY context, which the three cases above do not reach -- and where the first version of + * this commit did its damage. These five predicates and one script field returned rows before it + * and `compile error` after it, MEASURED on ES 8.18 index `t` (n = 1, 5, 9): + * {{{ + * WHERE n + 1 > 2 [b, c] WHERE n / 2 > 1 [b, c] + * WHERE n - 1 < 9 [a, b, c] SELECT (n+1)*(n+2) [6, 42, 110] + * WHERE n * 2 = 10 [b] + * }}} + * The rule, not the bytes, is what is asserted: an operand with no chain to drop keeps its + * parameter name, so nothing needs a statement and no statement can land in an expression slot. + */ + it should "keep every arithmetic operand placeable in a PREDICATE" in { + forAll( + Table( + "predicate", + "SELECT name FROM t WHERE n + 1 > 2", + "SELECT name FROM t WHERE n - 1 < 9", + "SELECT name FROM t WHERE n * 2 = 10", + "SELECT name FROM t WHERE n / 2 > 1", + // πŸ”΄ A CAST operand HAS a function, so it takes the RENDER path -- the family the + // chainless shortcut does NOT cover, and the one the second version of this commit broke. + // Every row below returned rows before this commit and `compile error` with it. + "SELECT name FROM t WHERE CAST(n AS BIGINT) + 1 > 2", + "SELECT name FROM t WHERE CAST(n AS DOUBLE) + 1 > 2", + "SELECT name FROM t WHERE CAST(n AS INTEGER) + 1 > 2", + "SELECT name FROM t WHERE CAST(n AS BIGINT) * 2 > 2", + "SELECT name FROM t WHERE 1 + CAST(n AS BIGINT) > 2", + // nested arithmetic: `compile error` BEFORE this commit as well as after the second + // version of it -- the hoist is what makes these emit at all + "SELECT name FROM t WHERE n * 2 + 1 > 2", + "SELECT name FROM t WHERE (n + 1) * (n + 2) > 2" + ) + ) { sql => + val emitted = predicateOf(sql) + withClue(emitted) { + declarationsOutsideStatementStart(emitted) shouldBe empty + duplicateDeclarations(emitted) shouldBe empty + // no cast of a null-guarded ternary: `(long) (x != null ? … : null)` does not compile, + // and it is what asking for a numeric conversion here used to emit + emitted should not include "(long) (" + } + } + } + + it should "declare each local ONCE in a nested numeric expression" in { + forAll( + Table( + "projection", + "SELECT (n + 1) * (n + 2) AS c FROM t", + // one nesting level deeper: declared `lv1` TWICE before the hoist, in every venue + "SELECT (n * 2 + 1) * (n * 3 + 1) AS c FROM t", + "SELECT (CAST(n AS BIGINT) + 1) * (CAST(n AS BIGINT) + 2) AS c FROM t" + ) + ) { sql => + val emitted = fieldOf(sql) + withClue(emitted) { + declarationsOutsideStatementStart(emitted) shouldBe empty + duplicateDeclarations(emitted) shouldBe empty + } + } + } + + /** πŸ”΄ The coercion of a NON-nullable operand is load-bearing and must survive: a literal carries + * no null guard, and its `((double) 2)` is the only thing that makes the division + * floating-point. Skipping it turned `CAST(n AS DOUBLE) / 2` into INTEGER division with HTTP 200 + * -- found by review. MEASURED on ES 8.18 over n = 1/5/9, on the schema-LESS rendering path + * (what production renders whenever `resolveWithSchema` declines -- a wildcard or multi-index + * FROM, a JOIN, an unloadable mapping): + * {{{ + * SELECT CAST(n AS DOUBLE)/2 [0.5, 2.5, 4.5] became [0, 2, 4] + * GROUP BY CAST(n AS DOUBLE)/2 keys '0.5','2.5','4.5' became '0','2','4' + * WHERE CAST(n AS DOUBLE)/2 > 2 [b, c] became [c] -- a row VANISHED + * }}} + * With a schema attached the same shape is now CORRECT where the parent was not: the parent + * emitted integer division there too (`[0, 2, 4]`, `WHERE … > 2` -> `[c]`), and it is `[0.5, + * 2.5, 4.5]` / `[b, c]` after this commit. + */ + it should "keep the coercion of a NON-nullable operand" in { + // The two paths carry the DOUBLE on different sides, so each is pinned where it lives: with NO + // schema the CAST chain is dropped and the LITERAL's cast is the only carrier; with a schema + // the left operand renders it. What must hold in BOTH is that a `(double)` exists at all -- + // without one the operands are two integers and Painless divides as integers. + forAll( + Table( + ("sql", "schema", "carrier"), + ("SELECT name FROM t WHERE CAST(n AS DOUBLE) / 2 > 2", false, "((double) 2)"), + ("SELECT name FROM t WHERE CAST(n AS DOUBLE) / 2 > 2", true, "(double) param1") + ) + ) { (sql, withSchema, carrier) => + val emitted = predicateOf(sql, withSchema) + withClue(emitted) { + emitted should include(carrier) + declarationsOutsideStatementStart(emitted) shouldBe empty + } + } + } + + /** The scanner's TRUE positive: `TRY_CAST` renders a `try`/`catch` STATEMENT (Painless has no + * expression-level `try`), so the operand must fall back to its parameter. If the scan stopped + * seeing `try`, that statement would be spliced back in and ES would reject the pipeline -- + * MEASURED: `def lv1 = String.valueOf(try { … } catch …);` is a `compile error`. The chain is + * still dropped, which is the residual, and the emission is byte-identical to the parent. + */ + it should "fall back to the parameter when a rendering is a STATEMENT" in { + val emitted = + processorOf( + "CREATE TABLE t (name KEYWORD, c BIGINT SCRIPT AS (TRY_CAST(name AS BIGINT) + 1))" + ) + withClue(emitted) { + emitted should not include "try " + declarationsOutsideStatementStart(emitted) shouldBe empty + } + } + + it should "keep it in a projection too" in { + val emitted = fieldOf("SELECT CAST(n AS DOUBLE) / 2 AS c FROM t", withSchema = false) + withClue(emitted)(emitted should include("((double) 2)")) + } + + /** The DDL venue of the same rule: an ingest processor persists its script, so an unplaceable + * declaration there is stored in the customer's cluster. + */ + it should "keep a computed column's arithmetic placeable too" in { + forAll( + Table( + "computed column", + "CREATE TABLE t (n INTEGER, c BIGINT SCRIPT AS ((CAST(n AS BIGINT) + 1) * (CAST(n AS BIGINT) + 2)))", + "CREATE TABLE t (n INTEGER, c BIGINT SCRIPT AS (n * 2 + 1))", + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (YEAR(d) * 100 + MONTH(d)))" + ) + ) { sql => + val emitted = processorOf(sql) + withClue(emitted) { + declarationsOutsideStatementStart(emitted) shouldBe empty + duplicateDeclarations(emitted) shouldBe empty + } + } + } + + /** The query half this commit DOES move: one chain, one operand. Before it the chain was dropped + * and ES compared a `ZonedDateTime` with an `int` -- a `class_cast` shard failure, not a wrong + * row; after it, `[a, b, c]` (MEASURED, ES 8.18, all three `d` in 2025). + */ + it should "render a single operand's chain in a PREDICATE too" in { + val emitted = predicateOf("SELECT name FROM t WHERE YEAR(d) + 1 = 2026") + withClue(emitted) { + emitted should include(".get(ChronoField.YEAR)") + declarationsOutsideStatementStart(emitted) shouldBe empty + duplicateDeclarations(emitted) shouldBe empty + } + } } From 3109c22f5b2791ecc3d6cf77e76acfb0c285f487 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 20 Sep 2026 01:26:17 +0200 Subject: [PATCH 7/8] fix(sql): a PROCESSOR does not re-parse a value its chain already extracted (#373, 7/9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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.` -- 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.` 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 --- .../softnetwork/elastic/sql/query/Where.scala | 12 ++- .../elastic/sql/type/SQLTypeUtils.scala | 6 +- .../sql/query/PainlessResidualsSpec.scala | 88 +++++++++++++++++++ 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala index 5432f218..3fb5d7da 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala @@ -886,7 +886,17 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } val coerced = context match { - case Some(ctx) if identifier.originalType == SQLTypes.Any && ctx.isProcessor => + // πŸ”΄ `originalType == Any` says this identifier NAMES a column -- it does NOT say the + // operand still RENDERS that column (issue #373, item 7). Where the chain has already + // produced something else, the processor's own parse was applied to it a SECOND time: + // `SCRIPT AS (CASE WHEN YEAR(d) > MONTH(d) …)` emitted + // `(param2 instanceof String ? LocalDate.parse(param2 …) : Instant.ofEpochMilli(param2)…)` + // with `param2` an extracted `int`. That is what kept item 6's split from RUNNING. + // `chainType` (#367) is what says whether the rendering is still a temporal. + case Some(ctx) + if identifier.originalType == SQLTypes.Any && ctx.isProcessor && + (identifier.chainType == SQLTypes.Any || + identifier.chainType.isInstanceOf[SQLTemporal]) => SQLTypeUtils .processorTemporal(chain, identifier.declaredType) .getOrElse( diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala index b32190f2..07bd8e66 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/type/SQLTypeUtils.scala @@ -243,7 +243,11 @@ object SQLTypeUtils { identifier.addPainlessMethod(".toLocalTime()") case _ => // do nothing } - case SQLTypes.Any if ctx.isProcessor => + // Same rule as the two other `processorTemporal` sites (issue #373, item 7): the + // operand may no longer RENDER the column `originalType` names. + case SQLTypes.Any + if ctx.isProcessor && (identifier.chainType == SQLTypes.Any || + identifier.chainType.isInstanceOf[SQLTemporal]) => processorTemporal(identifier.painless(context), identifier.declaredType) match { case Some(parsed) => return parsed case None => // do nothing diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala index e7e72e0b..46f64d34 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessResidualsSpec.scala @@ -542,4 +542,92 @@ class PainlessResidualsSpec extends AnyFlatSpec with Matchers with TableDrivenPr duplicateDeclarations(emitted) shouldBe empty } } + // -- item 7: a processor re-parse must not be applied to an already-extracted value ------------ + + /** Every `instanceof String ? …parse(…) : Instant.ofEpochMilli(…)` a PROCESSOR emits exists to + * turn the RAW `ctx.` into a temporal. Applying it to anything else means the operand had + * already been through a chain -- and the value it re-parses is no longer a date. Returns the + * operands re-parsed that are not a raw `ctx.` access. + */ + protected def reparsedNonSourceOperands(emitted: String): Seq[String] = { + // πŸ”΄ A parameter that ALIASES the source field is a legitimate operand: the prologue binds + // `def param1 = ctx.d;` and the branches then parse `param1`. Found by review -- the first + // version of this rule called those offenders, so adding the (correct) shape + // `CASE WHEN YEAR(d) > 2000 THEN d ELSE d END` to the table below would have REDDENED a correct + // emission. A test that mandates a defect is worse than no test. + val sourceAliases = + "def ([A-Za-z0-9_]+) = (ctx[.?][A-Za-z0-9_.?\\[\\]']*);".r + .findAllMatchIn(emitted) + .map(_.group(1)) + .toSet + // πŸ”΄ `?` is IN the class: a nested path renders `ctx.meta?.when`, and without it the regex + // matched nothing at all, so this rule was BLIND to the nested venue (also found by review). + "\\(([A-Za-z0-9_.?\\[\\]']+) instanceof String \\?".r + .findAllMatchIn(emitted) + .map(_.group(1)) + .filterNot(operand => operand.startsWith("ctx.") || sourceAliases.contains(operand)) + .toSeq + } + + /** `originalType == Any` says the identifier NAMES a column; it does not say the operand still + * RENDERS one. `YEAR(d)` renders an `int`, and the processor handed that `int` back to its own + * date parse. + * + * πŸ”΄ Why the guard admits `chainType == Any` and that is SAFE: `processorTemporal` emits a parse + * only when the column's `declaredType` is a temporal and answers `None` otherwise, so the + * DECLARED TYPE is a second gate and admitting `Any` cannot conjure a parse over a non-date + * column. MEASURED (review): `SCRIPT AS (YEAR(unknown))` over an UNDECLARED column is exactly + * the reachable `chainType == Any` case, and it emits ZERO parses; so does a `VARCHAR` column. + * ⚠️ That gate is what the guard leans on -- anything that loosens `processorTemporal` to emit a + * parse for a wider set of declared types removes this safety net silently. MEASURED as a real + * ingest pipeline 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-01T…2.025Z" after c = 2025 + * }}} + * πŸ”΄ The third is the dangerous one: it did not fail, it STORED the year 2025 read as epoch + * MILLIS. A computed column held a 1970 timestamp and nothing anywhere said so. + * + * ⚠️ Not owned here, PRE-EXISTING and unchanged: `GREATEST` renders `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, not this commit's. + */ + "a chained operand in a PROCESSOR" should "not be handed back to the processor's date parse" in { + forAll( + Table( + "computed column", + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (CASE WHEN YEAR(d) > MONTH(d) THEN 1 ELSE 0 END))", + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (GREATEST(YEAR(d), MONTH(d))))", + "CREATE TABLE t (d DATE, n INTEGER, c INTEGER SCRIPT AS (CASE WHEN n > 0 THEN YEAR(d) ELSE MONTH(d) END))", + // review's COALESCE/NULLIF pair -- also silent 1970 garbage before, so this is not a + // CASE-only fix + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (COALESCE(YEAR(d), 0)))", + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (NULLIF(YEAR(d), 0)))", + // πŸ”΄ the shape that would have reddened the FIRST version of the rule: the branches return + // the date column itself, so parsing `param1` (which aliases `ctx.d`) is CORRECT here. + // MEASURED: c = '2025-01-10T00:00:00.000Z' + "CREATE TABLE t (d DATE, c DATE SCRIPT AS (CASE WHEN YEAR(d) > 2000 THEN d ELSE d END))", + // the second reachable shape review found for the same false positive + "CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (YEAR(CASE WHEN 1 = 1 THEN d ELSE d END)))" + ) + ) { sql => + val emitted = processorOf(sql) + withClue(emitted)(reparsedNonSourceOperands(emitted) shouldBe empty) + } + } + + /** The guard must not disable the parse where it IS needed: a bare date column in a processor is + * still `ctx.d`, and without the parse every date function in an ingest script breaks again + * (21.8 Part C, PR #315). + */ + it should "still parse the raw source field it was written for" in { + val emitted = processorOf("CREATE TABLE t (d DATE, c INTEGER SCRIPT AS (YEAR(d)))") + withClue(emitted) { + emitted should include("ctx.d instanceof String") + emitted should include(".get(ChronoField.YEAR)") + } + } + } From 2df8172ff2e30c6c1f420e6e7cc96d06572fcde6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 20 Sep 2026 09:41:31 +0200 Subject: [PATCH 8/8] fix(sql): a `;` inside a string literal is data, not a statement boundary (#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. = ` 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 --- .../app/softnetwork/elastic/sql/package.scala | 98 ++++++++++- .../elastic/sql/schema/package.scala | 17 +- .../query/PainlessExpressionFormSpec.scala | 80 ++++++++- .../schema/ScriptProcessorAssemblySpec.scala | 162 ++++++++++++++++++ 4 files changed, 343 insertions(+), 14 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/schema/ScriptProcessorAssemblySpec.scala diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala index ace063f6..45968378 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -489,25 +489,107 @@ package object sql { * decide use [[placeable]]; this variant exists so a diagnostic can NAME the offending spot. */ def firstStatementAt(rendered: String): Option[Int] = { - var i = 0 - var quote = '\u0000' var found = -1 - while (i < rendered.length && found < 0) { + scanOutsideLiterals(rendered) { i => val c = rendered.charAt(i) - if (quote != '\u0000') { - if (c == '\\') i += 1 else if (c == quote) quote = '\u0000' - } else if (c == '\'' || c == '"') quote = c - else if (c == ';') found = i + if (c == ';') found = i else if ( rendered.startsWith("try ", i) && (i == 0 || !(rendered.charAt(i - 1).isLetterOrDigit || rendered.charAt(i - 1) == '_')) ) found = i - i += 1 + found < 0 // stop at the first boundary -- the answer cannot change after it } if (found < 0) None else Some(found) } def placeable(rendered: String): Boolean = firstStatementAt(rendered).isEmpty + + /** `rendered.split(";")`, except that a `;` inside a string literal is not a separator. + * + * πŸ”΄ `ScriptProcessor.fromScript` assembles an ingest script by splitting the rendered + * Painless, treating the LAST part as the expression to assign and rejoining the preamble. A + * bare `split(";")` there let a `;` inside a literal in the last statement cut the expression + * in half, and the assignment was written INTO the literal: `c KEYWORD SCRIPT AS + * (UPPER('a;b'))` emitted `"a; ctx.c = b".toUpperCase()` -- valid Painless that computes a + * value and discards it, so Elasticsearch answers 200 and the computed column is simply ABSENT + * (issue #373, a residual of it; measured on 8.18.3). `REPLACE(name, ';', 'x')` hits the same + * thing with no contrived input at all. + * + * Mirrors `String.split(";")` including its removal of trailing empty parts, so every source + * with no quoted `;` splits byte-identically to what it did before. Proved exhaustively over + * the literal-free alphabet `{a, b, ;, \}` up to length 7 -- 21,845 strings, zero mismatches. + * + * πŸ”΄ WHY UNDER-SPLITTING IS SAFE, and it is load-bearing. The only direction this can differ + * from `String.split(";")` is FEWER cuts, and it does that whenever a literal is left + * unbalanced -- which is reachable, because `function/time/package.scala` builds + * `ofPattern("")` by raw concatenation without `escapePainlessString`, so + * `DATE_FORMAT(d, 'yyyy\')` emits an unterminated literal and every later `;` is read as + * string content. The assignment then lands in the middle of the script rather than at the + * end. That is harmless ONLY because this scanner's escape rule is Painless's own: for any + * emission Painless accepts, "inside a literal" means the same to both, so a disagreement + * implies an unbalanced literal, and Elasticsearch rejects such a script with a compile error + * whichever way it was split. Both spellings measured as `compile error` on 8.18.3. If the + * escape rule here is ever relaxed, that argument dies with it. + */ + def splitStatements(rendered: String): Array[String] = + if (rendered.isEmpty) Array(rendered) + else { + val parts = scala.collection.mutable.ArrayBuffer.empty[String] + var start = 0 + scanOutsideLiterals(rendered) { i => + if (rendered.charAt(i) == ';') { + parts += rendered.substring(start, i) + start = i + 1 + } + true + } + parts += rendered.substring(start) + var n = parts.length + while (n > 0 && parts(n - 1).isEmpty) n -= 1 + parts.take(n).toArray + } + + /** `rendered` with every string literal blanked out, character for character, so a caller can + * ask a question about CODE only. Indices and length are preserved, so a match on the result + * carries the same offsets and the same captured text as one on the original. + * + * πŸ”΄ `ScriptTarget.of` recovers "which column does this ingest script feed" by taking the LAST + * `ctx. = ` in the source. A literal is allowed to contain that text -- `CONCAT(name, '; + * ctx.d = 1')` is ordinary SQL -- and the raw regex then reported the computed column of one + * processor as another's. `IngestPipeline.diff` keys processors by it, and a `Map` silently + * DROPS the loser: measured, the ALTER rewrote the sibling column's processor and never added + * the new column (issue #373). Blanking the literals first is what makes the recovery read + * code and not data. + */ + def withoutLiterals(rendered: String): String = { + val out = Array.fill(rendered.length)(' ') + scanOutsideLiterals(rendered) { i => + out(i) = rendered.charAt(i) + true + } + new String(out) + } + + /** Walks `rendered` once and hands the caller every index lying OUTSIDE a string literal. + * + * πŸ”΄ The literal handling lives HERE and nowhere else. `firstStatementAt` and + * `splitStatements` must agree about what a `;` inside a literal is -- one decides whether a + * fragment may sit in an operand slot, the other decides where an ingest script's last + * statement begins -- and two copies of this loop would be free to drift apart. + */ + private def scanOutsideLiterals(rendered: String)(at: Int => Boolean): Unit = { + var i = 0 + var quote = '\u0000' + var continue = true + while (i < rendered.length && continue) { + val c = rendered.charAt(i) + if (quote != '\u0000') { + if (c == '\\') i += 1 else if (c == quote) quote = '\u0000' + } else if (c == '\'' || c == '"') quote = c + else continue = at(i) + i += 1 + } + } } /** Context for painless scripts diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala index 67042b91..2ffbda28 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala @@ -642,10 +642,17 @@ package object schema { * `ctx.` on the RIGHT of the `=`, never on the left, so anchoring on the assignment and * taking the last match cannot pick one of them up. */ - def of(source: String): Option[String] = source match { - case Assignment(column) => Some(column) - case _ => None - } + def of(source: String): Option[String] = + // πŸ”΄ On the literals-blanked source, never the raw one. A string literal may legitimately + // CONTAIN `ctx. = ` -- `CONCAT(name, '; ctx.d = 1')` is ordinary SQL -- and the greedy + // match then took the one inside the literal, reporting this processor as another column's. + // `IngestPipeline.diff` keys processors by this, and a `Map` drops the loser of a collision + // (issue #373). Blanking preserves offsets and length, so the captured name is unchanged + // wherever no literal is involved. + PainlessOperandForm.withoutLiterals(source) match { + case Assignment(column) => Some(column) + case _ => None + } private val Assignment = """(?s).*(?:^|;)\s*ctx\.([A-Za-z0-9_.]+)\s*=[^=].*""".r } @@ -661,7 +668,7 @@ package object schema { val ctx = PainlessContext(PainlessContextType.Processor) val scr = script.painless(Some(ctx)) val painless = s"$ctx$scr" - val source = painless.split(";") match { + val source = PainlessOperandForm.splitStatements(painless) match { case Array(single) if single.trim.startsWith("return ") => val stripped = single.trim.stripPrefix("return ").trim ScriptTarget.assign(column, stripped) diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala index 55ed4598..da616b29 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessExpressionFormSpec.scala @@ -25,7 +25,7 @@ import org.scalatest.prop.TableDrivenPropertyChecks * goes. `ArithmeticExpression` falls back to a parameter when it answers false (issue #373, item * 5), and `PainlessOperandFormSpec` uses it as the tree-wide guard over rendered criteria. * - * πŸ”΄ WHY THIS SPEC EXISTS. At the emission sites the predicate is INERT today: no reachable SQL + * πŸ”΄ WHY THIS SPEC EXISTS. At the ARITHMETIC operand site the predicate is inert: no reachable SQL * shape renders an operand carrying a `;` inside a string literal, and the `try` arm never decides * anything either, because both Painless `try` emitters (`function/convert/package.scala`) always * emit a `;` as well -- so the `;` arm disqualifies first. Review PROVED that by mutation: @@ -33,6 +33,12 @@ import org.scalatest.prop.TableDrivenPropertyChecks * green. A guard nothing can see is not a guard, so the rule is exercised HERE, one case per arm, * with the inputs review actually falsified. * + * πŸ”΄ CORRECTED: the literal handling is NO LONGER inert anywhere. `splitStatements` shares this + * scanner and `ScriptProcessor.fromScript` cuts an ingest script with it, where `UPPER('a;b')` IS + * a reachable shape and a quote-blind split silently dropped the computed column (issue #373 -- + * see `ScriptProcessorAssemblySpec`). The `try` arm remains inert and stays pinned here as + * characterisation. + * * πŸ”΄ The two wrong answers are NOT symmetric, and every accepted limitation sits on the safe side * of that. Answering "statement" about an expression demotes the operand and drops its function * chain -- a SILENT wrong answer, the failure this predicate exists to prevent. Answering @@ -158,4 +164,76 @@ class PainlessExpressionFormSpec extends AnyFlatSpec with Matchers with TableDri PainlessOperandForm.firstStatementAt("""def lv0 = x; y""") shouldBe Some(11) PainlessOperandForm.firstStatementAt("""try { x }""") shouldBe Some(0) } + + /** `splitStatements` is the same scanner used to CUT rather than to decide -- it is what + * `ScriptProcessor.fromScript` splits an ingest script on. Two claims, and the second is the one + * that carries the defect: it must agree with `String.split(";")` wherever no literal is + * involved (so no existing emission moves), and it must not cut inside a literal. + */ + private def split(rendered: String): List[String] = + PainlessOperandForm.splitStatements(rendered).toList + + "splitting statements" should "agree with String.split where no literal is involved" in { + forAll( + Table( + "rendered", + "def a = 1; b", + "def a = 1; def b = 2; c", + "a", + "", + "a;", + "a;;b", + ";a", + ";", + ";;" + ) + )(r => withClue(s"[$r]")(split(r) shouldBe r.split(";").toList)) + } + + it should "not cut inside a string literal" in { + forAll( + Table( + ("rendered", "parts"), + ( + """def a = ctx.n; "x;y".toUpperCase()""", + List("""def a = ctx.n""", """ "x;y".toUpperCase()""") + ), + (""""a;b"""", List(""""a;b"""")), + ("""'a;b'""", List("""'a;b'""")), + ("""f("a;b"); g""", List("""f("a;b")""", """ g""")), + ("""f("a\\"); x""", List("""f("a\\")""", """ x""")) + ) + )((r, expected) => withClue(s"[$r]")(split(r) shouldBe expected)) + } + + /** The literal-bearing rows above are the ones a bare `split(";")` gets wrong; this states the + * difference explicitly so the spec says WHY it exists, not merely what it expects. + */ + it should "differ from String.split exactly where a literal holds the separator" in { + val rendered = """def a = ctx.n; "x;y".toUpperCase()""" + rendered.split(";").toList should have size 3 + split(rendered) should have size 2 + } + + /** `withoutLiterals` is the third user of the shared scanner. `ScriptTarget.of` recovers which + * column an ingest script feeds by matching the LAST `ctx. = `, and a literal is allowed + * to CONTAIN that text, so the match has to run on code alone. Offsets and length must survive + * the blanking, or the captured name would not be the one that was matched. + */ + "blanking literals" should "erase their content and keep every offset" in { + forAll( + Table( + ("rendered", "blanked"), + ("""ctx.c = "; ctx.d = 1"""", "ctx.c = " + " " * 13), + ("""a'b;c'd""", "a" + " " * 5 + "d"), + ("""plain code""", """plain code"""), + ("", "") + ) + ) { (r, expected) => + withClue(s"[$r]") { + PainlessOperandForm.withoutLiterals(r) shouldBe expected + PainlessOperandForm.withoutLiterals(r).length shouldBe r.length + } + } + } } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/schema/ScriptProcessorAssemblySpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/schema/ScriptProcessorAssemblySpec.scala new file mode 100644 index 00000000..8640e137 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/schema/ScriptProcessorAssemblySpec.scala @@ -0,0 +1,162 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql.schema + +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query.CreateTable +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.scalatest.prop.TableDrivenPropertyChecks + +/** An ingest script is assembled by splitting the rendered Painless, assigning the LAST statement + * to the computed column and rejoining the preamble, and read back by recovering that assignment. + * Both halves used to treat a `;` inside a STRING LITERAL as code (issue #373). + * + * πŸ”΄ WRITING β€” `ScriptProcessor.fromScript` split on a bare `;`, so the assignment was written + * INTO the literal. Measured on real Elasticsearch 8.18.3 via `_ingest/pipeline/_simulate`: + * + * - `REPLACE(name, ';', 'x')` emitted `param1.replace("; ctx.c = ", "x")` -> compile error + * - `CONCAT(name, 'a;b')` emitted `... + "a; ctx.c = b"` -> compile error + * - `UPPER('a;b')` emitted `"a; ctx.c = b".toUpperCase()` -> **HTTP 200, column ABSENT** + * + * The last is valid Painless that computes a value and discards it: nothing throws, so + * `ignore_failure` never comes into it and the document is stored without the column. `REPLACE` + * with a `;` needle needs no contrived input at all, which is what makes this worth a spec. + * + * πŸ”΄ READING β€” `ScriptTarget.of` took the LAST `ctx. = ` 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` silently 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 that fix the poisoned emission was a compile error, so the table could + * not be created in the first place. + * + * πŸ”΄ The oracle is INDEPENDENT of the code under test. Asking `PainlessOperandForm` where the + * literals are would assert the fix with the fix; `literalFree` below strips string literals with + * its own regex, and the assertion is made on what remains. + */ +class ScriptProcessorAssemblySpec extends AnyFlatSpec with Matchers with TableDrivenPropertyChecks { + + /** Blanks out every string literal, so an assertion can speak about code only. Deliberately a + * different mechanism from the production scanner: a regex over both quote styles, honouring + * backslash escapes. + */ + private def literalFree(source: String): String = + """"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'""".r + .replaceAllIn(source, m => " " * m.matched.length) + + private def columnsOf(ddl: String): List[Column] = + Parser(ddl) match { + case Right(ct: CreateTable) => ct.schema.columns + case other => fail(s"did not parse as CREATE TABLE: $other") + } + + private def sourceOf(ddl: String, column: String = "c"): String = + columnsOf(ddl) + .find(_.name == column) + .flatMap(_.script) + .map(_.source) + .getOrElse(fail(s"no script for column $column in [$ddl]")) + + private val shapes = Table( + ("ddl", "literal"), + ("CREATE TABLE t (name KEYWORD, c KEYWORD SCRIPT AS (REPLACE(name, ';', 'x')))", "\";\""), + ("CREATE TABLE t (name KEYWORD, c KEYWORD SCRIPT AS (CONCAT(name, 'a;b')))", "\"a;b\""), + ("CREATE TABLE t (name KEYWORD, c KEYWORD SCRIPT AS (UPPER('a;b')))", "\"a;b\""), + ( + "CREATE TABLE t (name KEYWORD, c KEYWORD SCRIPT AS (CASE WHEN name = 'x' THEN 'a;b' ELSE 'c' END))", + "\"a;b\"" + ) + ) + + "a `;` inside a string literal" should "not be taken for a statement boundary" in { + forAll(shapes) { (ddl, literal) => + val source = sourceOf(ddl) + withClue(s"[$ddl] -> [$source] ")(source should include(literal)) + } + } + + it should "leave the assignment to the computed column outside every literal" in { + forAll(shapes) { (ddl, _) => + val source = sourceOf(ddl) + withClue(s"[$ddl] -> [$source] ") { + // exactly one assignment, and it survives the literal blanking -- i.e. it is CODE + literalFree(source).split("ctx\\.c = ", -1).length - 1 shouldBe 1 + } + } + } + + /** A byte-exact drift pin on `;`-free assembly: these shapes were always correct and none of them + * may move. + */ + it should "assemble a literal-free expression exactly as before" in { + forAll( + Table( + ("ddl", "expected"), + ( + "CREATE TABLE t (name KEYWORD, c KEYWORD SCRIPT AS (CONCAT(name, 'ab')))", + "def param1 = ctx.name; ctx.c = (param1 == null) ? null : String.valueOf(param1) + \"ab\"" + ), + ( + "CREATE TABLE t (name KEYWORD, c KEYWORD SCRIPT AS (UPPER('ab')))", + " ctx.c = \"ab\".toUpperCase()" + ) + ) + )((ddl, expected) => withClue(s"[$ddl] ")(sourceOf(ddl) shouldBe expected)) + } + + "a `ctx. = ` inside a string literal" should "not be read back as the computed column" in { + val ddl = + "CREATE TABLE t (name KEYWORD, d KEYWORD SCRIPT AS (LOWER(name)), " + + "c KEYWORD SCRIPT AS (CONCAT(name, '; ctx.d = 1')))" + val source = sourceOf(ddl) + withClue(s"[$source] ") { + source should include("\"; ctx.d = 1\"") // the literal is intact + ScriptTarget.of(source) shouldBe Some("c") // ... and read as data, not as the assignment + } + } + + it should "not collide two processors onto one identity" in { + val ddl = + "CREATE TABLE t (name KEYWORD, d KEYWORD SCRIPT AS (LOWER(name)), " + + "c KEYWORD SCRIPT AS (CONCAT(name, '; ctx.d = 1')))" + val identities = + columnsOf(ddl).flatMap(_.script).map(_.source).flatMap(ScriptTarget.of) + // `IngestPipeline.diff` keys processors by this, and a Map DROPS the loser of a collision + withClue(s"identities = $identities ")(identities.distinct.size shouldBe identities.size) + } + + /** πŸ”΄ CHARACTERISATION, not an endorsement. `fromScript` treats the last `;`-delimited chunk as + * the expression, and a `;` inside a BLOCK defeats that exactly as a `;` inside a literal did: + * `TRY_CAST` emits a `try`/`catch`, and the assignment lands inside the `catch` with no + * right-hand side. Elasticsearch answers `compile error`, so it is LOUD and no data is lost β€” + * but `TRY_CAST` in a computed column has never worked. + * + * Identical before and after this fix (verified by independent review against the pre-fix + * assembly on the same rendered Painless), so it is recorded here rather than repaired: the real + * remedy is for `fromScript` to be HANDED the prologue and the expression by `PainlessContext` + * instead of re-deriving the boundary from their concatenation, which is a larger change than + * this one. Recorded so the next reader sees it instead of rediscovering it. + */ + "the last-statement strategy" should "still be defeated by a `;` inside a block" in { + val source = + sourceOf("CREATE TABLE t (name KEYWORD, c BIGINT SCRIPT AS (TRY_CAST(name AS BIGINT)))") + withClue(s"[$source] ") { + source should endWith("catch (Exception e) { return null; ctx.c = }") + } + } +}