From 518da2f5224636f7e7849649f26c7987ab8eb326 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Fri, 11 Sep 2026 13:35:12 +0200 Subject: [PATCH 01/10] =?UTF-8?q?fix(BIDC-8):=20keep=20both=20aliases=20of?= =?UTF-8?q?=20a=20self-join=20=E2=80=94=20lossless=20From.aliasesToTable,?= =?UTF-8?q?=20alias-first=20resolution?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs SOFTNETWORK-APP/softclient4es-arrow#144 (closes from the arrow PR, not from here). `From.tableAliases` is `ListMap[tableKey -> alias]`, so by construction it holds ONE alias per table: for `FROM idx a JOIN idx b` both legs share the bare key `idx` and the map kept `idx -> b`. `aliasesToTable` was its `.swap`, so alias `a` was gone from BOTH maps, `Identifier.update` never resolved `a.id`, `JoinKey.apply` produced no key and `On.joinKeyMatches` was EMPTY — the malformed `INNER JOIN "sq_b"` (no ON) that arrow#144 saw as DuckDB's "syntax error at end of input" (reproduced at b37ef940 before any fix; captures in the PR body). - `From.aliasesToTable` is built DIRECTLY and losslessly (alias -> table key, same `aliasKey` on the table side): a strict superset of the old `.swap`; `tableAliases` keeps its one-alias-per-table contract byte-for-byte (extensions read it forward). The two in-core reverse-lookup consumers move to it: `Identifier.update` and `FieldSort.update` (#159's `bareTableAlias`, whose canary now also covers `ORDER BY a` / `ORDER BY b` on a self-join). - `TemporalLiterals`: join-source set minus the new `From.mainTableKey` — a self-join's join source IS the main index, whose mapping is the one in hand. - Tripwire 2 (lead ruling "keep or reject loudly"): `FROM t a, t b` — a table listed twice in a comma-separated FROM under DIFFERENT aliases — is rejected in `From.validate()` naming the JOIN spelling with the user's aliases; `FROM t, t` and 21.2's `FROM "a".orders o, "b".orders p` are untouched. Pinned in the BEFORE form at the baseline, then flipped. - Found by the review by measurement, fixed here: `CreateTable` and `CreateMaterializedView` had no `validate()` override, so `CREATE TABLE … AS SELECT` / `CREATE MATERIALIZED VIEW … AS SELECT` skipped every SELECT-level rule (`INSERT … SELECT` delegated). Both now delegate; blast radius 0 over the sql corpus (1054) and extensions' unit suites (162). - 21.2's AD-6 scope pins are changed ON PURPOSE (LR-4): the "identically qualified tables keep collapsing" pin now asserts the lossless map and the comma rejection; the watcher `FROM orders o, orders p WHERE o.x = 1` flips from accepted to rejected by #191's guard (a qualifier over a doubled single index is still one a multi-index search cannot scope). - `documentation/sql/joins.md`: new Self-join section; the arrow#137 "must be written bare" limit note now describes the named error (MDX twin in softclient4es-web feature/BIDC-8). Release notes (0.23.0): self-joins resolve both legs (qualify every column); newly rejected — `FROM t a, t b`, the qualified watcher self-join, and a CTAS / MV whose SELECT violates a SELECT-level rule. No binary-incompatible change (lazy vals + two overrides). Tests: sql 1054 (+10: SelfJoinAliasSpec x9, TemporalLiteralsSpec +1), core 979, `+ sql/compile`, `+ core/compile`, `++ 2.12.20 sql/Test/compile`; arrow JOIN integration on real ES 6.8 / 7.17 / 8.18 / 9.0 against this branch's publishLocal (see the arrow branch). Co-Authored-By: Claude Fable 5.1 --- documentation/sql/joins.md | 21 ++- .../app/softnetwork/elastic/sql/package.scala | 8 +- .../elastic/sql/parser/Parser.scala | 12 +- .../softnetwork/elastic/sql/query/From.scala | 83 ++++++++- .../elastic/sql/query/OrderBy.scala | 6 +- .../elastic/sql/query/TemporalLiterals.scala | 11 +- .../elastic/sql/query/package.scala | 23 +++ .../elastic/sql/parser/ParserSpec.scala | 6 +- .../sql/parser/QuotedTableNameSpec.scala | 38 ++-- .../elastic/sql/query/SelfJoinAliasSpec.scala | 165 ++++++++++++++++++ .../sql/query/TemporalLiteralsSpec.scala | 12 ++ 11 files changed, 353 insertions(+), 32 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala diff --git a/documentation/sql/joins.md b/documentation/sql/joins.md index 458b724f5..3639426eb 100644 --- a/documentation/sql/joins.md +++ b/documentation/sql/joins.md @@ -111,10 +111,29 @@ ORDER BY COUNT(*) DESC; -- Engineering (3), Marketing (2) survive HAVING ``` -> **SELECT aliases and ordinals work here** — since arrow-extensions **0.2.5** (REPL bundle `0.20.4`, JDBC / ADBC / Flight SQL driver `0.2.5`). `ORDER BY cnt`, `HAVING cnt > 1`, `GROUP BY` on an alias and ordinal forms such as `ORDER BY 2` all resolve against the final SELECT list *after* the join, and alias matching is case-insensitive. Two limits remain: an alias is **not** legal in `SELECT`, `ON` or `WHERE` — nothing has been computed at that point — and it must be written **bare**, since `ORDER BY d.cnt` qualifies a name no table owns and fails inside DuckDB. Before 0.2.5 all of these were rejected with `Ambiguous column`. +> **SELECT aliases and ordinals work here** — since arrow-extensions **0.2.5** (REPL bundle `0.20.4`, JDBC / ADBC / Flight SQL driver `0.2.5`). `ORDER BY cnt`, `HAVING cnt > 1`, `GROUP BY` on an alias and ordinal forms such as `ORDER BY 2` all resolve against the final SELECT list *after* the join, and alias matching is case-insensitive. Two limits remain: an alias is **not** legal in `SELECT`, `ON` or `WHERE` — nothing has been computed at that point — and it must be written **bare** — `ORDER BY d.cnt` qualifies a name no table owns, and since arrow-extensions **0.3.3** it is rejected with an error that says exactly that and names the bare spelling (before 0.3.3 it failed inside DuckDB with an internal `Invalid Input Error`). Before 0.2.5 all of these were rejected with `Ambiguous column`. > > **The remaining ORDER BY gotcha:** you cannot `ORDER BY` a column that exists on **both** sides of the JOIN — order by a column unique to one side, e.g. `d.dept_name`, not the shared join key `d.dept_id`. +### Self-join — one index under two aliases + +A table can be joined to **itself**: give the index two aliases and qualify every column. Each alias becomes its own ES sub-query (with its own pushed-down `WHERE`), exactly as two distinct indices would: + +```sql +SELECT a.id, b.amount +FROM orders a +JOIN orders b ON a.id = b.id +WHERE a.id <= 5 AND b.id <= 5; +-- 5 rows: each order paired with itself + +SELECT a.id AS left_id, b.id AS right_id +FROM orders a +JOIN orders b ON a.customer_id = b.customer_id; +-- every pair of orders sharing a customer (fan-out, see below) +``` + +> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Two rules that follow from the fix: **qualify every column** (a bare `id` is ambiguous between the two legs and is rejected as such), and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is now rejected with a message pointing at the `JOIN` spelling. + ### JOIN cardinality — fan-out on a non-unique key A JOIN on a key that is **not unique** on the other side multiplies rows. That is standard SQL and the engine is doing it correctly, but it is the easiest way to get plausible-looking wrong numbers, because the row multiplication is invisible in the output. 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 b5c06a59e..dbca470dc 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -1556,8 +1556,14 @@ package object sql { // name follows it. Without the arity check a column that happens to share its table's name // — `FROM status WHERE status = 'done'` — matched `tableAliases` and was rewritten to // `parts.tail.mkString(".")`, i.e. the empty string, silently querying a nameless field. + // + // 🔴 `aliasesToTable`, never a REVERSE lookup over `tableAliases` (story BIDC-8, + // softclient4es-arrow#144): that map is keyed by TABLE and holds one alias per table, so on a + // self-join (`FROM idx a JOIN idx b`) the reverse lookup found `b` and never `a` — `a.id` + // stayed a literal dotted field name with no `table`, and the ON clause lost its join key. + // The value is still a `tableAliases` KEY (same `aliasKey`), so `table` keeps its language. val table = - if (parts.size > 1) request.tableAliases.find(t => t._2 == tableAlias).map(_._1) + if (parts.size > 1) request.aliasesToTable.get(tableAlias) else None /** The schema for THIS column's own table. diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala index fdd5304ae..1dc23e1fd 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala @@ -947,16 +947,18 @@ object Parser * * Testing for "any qualifier" rather than "qualifiers from two tables" is deliberate: the * narrower test lets a correlation through whenever one side fails to resolve — a function - * argument (`WHERE o.id = LOWER(c.id)`), or a self-join through duplicate table names, where - * `From.tableAliases` keeps only the last alias. + * argument (`WHERE o.id = LOWER(c.id)`), or — before story BIDC-8 — a self-join through + * duplicate table names, where the alias map kept only the last alias. * - * ⚠️ That last example is now conditional, and the guard WIDENED because of it (story 21.2 + * ⚠️ That last example became conditional, and the guard WIDENED because of it (story 21.2 * AD-6'): when two tables differ only by qualifier the alias map keeps BOTH entries, so both * qualifiers resolve and this guard fires where it used to be defeated. MEASURED: `CREATE OR * REPLACE WATCHER w AS EVERY 5 MINUTES FROM "a".orders o, "b".orders p WHERE o.x = 1 WITHIN 2 * MINUTES ALWAYS DO … END` is accepted before 21.2 and rejected after — #191's guard finally - * firing on a statement it always meant to catch. A WHOLLY unqualified self-join (`FROM orders - * o, orders p`) still defeats it, exactly as before. + * firing on a statement it always meant to catch. Story BIDC-8 closed the last gap: qualifiers + * resolve through the lossless `From.aliasesToTable`, so a WHOLLY unqualified self-join (`FROM + * orders o, orders p WHERE o.x = 1`) now resolves `o` too and is rejected here as well — a + * qualifier over a doubled single index is still a qualifier this search cannot scope. */ private def qualifiedOverManyIndices(f: From, criteria: Option[Criteria]): Boolean = f.tables.size > 1 && criteria.exists(_.referencedIdentifiers.exists(_.table.isDefined)) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala index 3acfa3e8e..acb3b542c 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala @@ -445,7 +445,50 @@ case class From(tables: Seq[Table]) extends Updateable { lazy val joinSourceKeys: Set[String] = joinReferences.values.map { case (name, qualified) => aliasKey(name, qualified) }.toSet - lazy val aliasesToTable: ListMap[String, String] = tableAliases.map(_.swap) + /** The alias a table is referenced by in this statement: its SQL alias when it has a non-empty + * one, its bare name otherwise — the VALUE `tableAliases` records for it. + */ + private def effectiveAlias(table: Table): String = + table.tableAlias.map(_.alias).filter(_.nonEmpty).getOrElse(table.name) + + /** The alias-map key of this FROM's main table — `aliasKey` applied to `mainTable` — so a + * consumer holding an `Identifier.table` can tell "the main index" from a join source in the + * SAME key language (story BIDC-8; `TemporalLiterals` needs it for a self-join, whose join + * source IS the main index). + */ + lazy val mainTableKey: String = aliasKey(mainTable.name, mainTable.qualifiedName) + + /** 🔴 FIX (story BIDC-8, softclient4es-arrow#144) — alias -> table key, built DIRECTLY and + * LOSSLESS. It used to be `tableAliases.map(_.swap)`. + * + * `tableAliases` is keyed by the TABLE, so by construction it holds exactly ONE alias per key: + * for a self-join — `FROM idx a JOIN idx b` — both legs have the IDENTICAL qualified reference, + * `aliasKey` returns the bare `idx` for both, and the `ListMap` keeps `idx -> b`. MEASURED at + * the baseline (b37ef940): `aliasesToTable` was `ListMap(b -> idx)`, alias `a` was gone from + * BOTH maps, `Identifier.update` could not resolve `a.id` (it stayed a literal dotted field name + * with no `table`), `JoinKey.apply` therefore produced no key for it and `On.joinKeyMatches` was + * EMPTY — which is the malformed `INNER JOIN "sq_b"` with no `ON` that the arrow issue saw as + * DuckDB's "syntax error at end of input". + * + * The alias is the only identity that tells the two legs apart, so the lossless direction is + * alias -> table. Every consumer that asks "which table does this alias name?" — + * `Identifier.update` (`sql/package.scala`) and `FieldSort.update` (#159's `bareTableAlias`) — + * reads THIS map. `tableAliases` keeps its documented single-alias-per-key semantics UNTOUCHED: + * softclient4es-extensions' `JoinDependencyGraph` reads it FORWARD (table -> alias) and would + * break under any other key language. The table side of every entry below uses the SAME + * `aliasKey`, so the values here ARE `tableAliases` keys — `Identifier.table` stays a + * `tableAliases` key (story 21.2 AD-6) and `schemas` / `joinSourceKeys` / `TemporalLiterals` + * need no new language. Whenever no two aliases share a key this map equals the old `.swap`. + * + * Two legs sharing an ALIAS still collapse here (last wins), exactly as `joinReferences` does. + */ + lazy val aliasesToTable: ListMap[String, String] = ListMap( + (tables.map(table => effectiveAlias(table) -> aliasKey(table.name, table.qualifiedName)) ++ + unnestAliases.map { case (alias, (name, _)) => alias -> name } ++ + joinReferences.map { case (alias, (name, qualified)) => + alias -> aliasKey(name, qualified) + }): _* + ) lazy val joins: Seq[Join] = tables.flatMap(_.joins) @@ -489,12 +532,38 @@ case class From(tables: Seq[Table]) extends Updateable { } else if (tables.count(_.joins.nonEmpty) > 1) { Left("Only one table with joins is supported in FROM clause") } else { - for { - _ <- tables.map(_.validate()).filter(_.isLeft) match { - case Nil => Right(()) - case errors => Left(errors.map { case Left(err) => err }.mkString("\n")) - } - } yield () + // 🔴 Story BIDC-8, tripwire 2 (lead ruling: keep the behaviour or reject LOUDLY, never + // change it silently). A comma-separated FROM is a MULTI-INDEX SEARCH — one query over every + // index it names, no join engine behind it — so two aliases on the SAME table have no meaning + // it can honour. MEASURED at the baseline: `SELECT a.x, b.y FROM t a, t b` resolved `b.y` and + // left `a.x` a literal field name (the alias map kept only the last alias); with the alias + // map made lossless both would resolve against ONE index — a DIFFERENT wrong answer, still + // silent. The same alias twice (`FROM t, t`) changes nothing and stays accepted; two + // DIFFERENT qualified references to one bare name (`FROM "a".orders o, "b".orders p`, story + // 21.2) are distinct tables, not duplicates. A JOIN hangs off ONE table, so a self-JOIN can + // never reach this branch. + val duplicated: Option[Table] = tables.find { table => + tables.filter(_.qualifiedName == table.qualifiedName).map(effectiveAlias).distinct.size > 1 + } + duplicated match { + case Some(table) => + val rendered = Table.render(table.parts, table.name) + val aliases = + tables.filter(_.qualifiedName == table.qualifiedName).map(effectiveAlias).distinct + val (a, b) = (aliases.head, aliases(1)) + Left( + s"Table $rendered is listed more than once in FROM under different aliases ($a, $b); " + + "a comma-separated FROM searches several indices and cannot join a table to itself. " + + s"Write a self-join as FROM $rendered $a JOIN $rendered $b ON $a. = $b." + ) + case None => + for { + _ <- tables.map(_.validate()).filter(_.isLeft) match { + case Nil => Right(()) + case errors => Left(errors.map { case Left(err) => err }.mkString("\n")) + } + } yield () + } } } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala index df459152a..8274a3e40 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala @@ -128,9 +128,11 @@ case class FieldSort( // caught downstream, because `Identifier.update` rewrote any bare name matching an alias // to the empty string; that rewrite also broke a column legitimately sharing its table's // name, so it is gone and the collision is recorded here, where the FROM aliases are - // still in scope. + // still in scope. `aliasesToTable`, not a reverse scan of `tableAliases` (story BIDC-8): + // on a self-join the table-keyed map holds only the LAST alias, so `ORDER BY a` over + // `FROM idx a JOIN idx b` slipped past this check. bareTableAlias = - if (!field.name.contains('.') && request.tableAliases.exists(_._2 == field.name)) + if (!field.name.contains('.') && request.aliasesToTable.contains(field.name)) Some(field.name) else None diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/TemporalLiterals.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/TemporalLiterals.scala index 7e2c9a5c7..12f702da9 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/TemporalLiterals.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/TemporalLiterals.scala @@ -301,6 +301,11 @@ object TemporalLiterals { * since story 21.2 that key is the QUALIFIED reference whenever a bare index name is ambiguous * inside one FROM. Comparing a qualified `table` against a bare set silently stops this guard * firing, and the literal is then resolved against the wrong index's schema. + * + * And `joinSources` must NOT contain the main table's own key (story BIDC-8): on a self-join + * (`FROM idx a JOIN idx b`) the join source IS the main index, whose mapping IS the one in hand, + * so every column of BOTH legs resolves against it. Before BIDC-8 the first leg's qualifier did + * not resolve at all, so the question never arose. */ private def temporalColumn( identifier: GenericIdentifier, @@ -324,8 +329,10 @@ object TemporalLiterals { search.where.flatMap(_.criteria) match { case Some(criteria) => // `joinSourceKeys`, NOT `joinAliases.values.map(_._1)`: the comparison below is against - // `Identifier.table`, which is a `tableAliases` KEY (story 21.2 AD-6'). See `temporalColumn`. - val joinSources: Set[String] = search.from.joinSourceKeys + // `Identifier.table`, which is a `tableAliases` KEY (story 21.2 AD-6'). Minus the main + // table's own key, so a self-join's second leg resolves against the mapping in hand (story + // BIDC-8). See `temporalColumn`. + val joinSources: Set[String] = search.from.joinSourceKeys - search.from.mainTableKey rewrite(criteria, schema, joinSources).map { rewritten => if (rewritten eq criteria) search else search.copy(where = Some(Where(Some(rewritten)))) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala index cb1789cbd..4d9e44e98 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala @@ -118,6 +118,11 @@ package object query { lazy val fieldAliases: ListMap[String, String] = select.fieldAliases lazy val tableAliases: ListMap[String, String] = from.tableAliases + + /** alias -> table KEY, lossless (story BIDC-8): the map to consult when resolving a qualifier. + * `tableAliases` (table -> alias) cannot hold two aliases of one table. + */ + lazy val aliasesToTable: ListMap[String, String] = from.aliasesToTable lazy val unnestAliases: ListMap[String, (String, Option[Limit])] = from.unnestAliases /** Bucket lookup, keyed by EVERY spelling a bucket can be referenced with. @@ -1207,6 +1212,12 @@ package object query { parts: Seq[NamePart] = Nil ) extends MaterializedViewStatement with DdlStatement { + + /** Same reasoning as `CreateTable.validate()` (story BIDC-8): the view's query is validated by + * the rules that govern any SELECT — this statement used to inherit the no-op default. + */ + override def validate(): Either[String, Unit] = dql.validate() + override def sql: String = { // The leading space belongs HERE, not to `Frequency.sql`: `TransformConfig` renders the same // value on a line of its own and supplies its own indentation. Without it the render read @@ -1329,6 +1340,18 @@ package object query { lazy val partitioned: Boolean = partitionBy.isDefined + /** The AS-SELECT is a full `SearchStatement` and every rule `SingleSearch.validate()` enforces + * applies to it unchanged; before story BIDC-8 this statement inherited the no-op default, so + * a CTAS carried its query past `Parser.apply` UNVALIDATED (measured: `CREATE TABLE t AS + * SELECT a.x, b.y FROM t a, t b` was accepted while the bare SELECT was rejected). The column + * form keeps its previous acceptance — nothing validated it before and nothing new does now. + */ + override def validate(): Either[String, Unit] = + ddl match { + case Left(select) => select.validate() + case Right(_) => Right(()) + } + override def sql: String = { val replaceClause = if (orReplace) " OR REPLACE" else "" val ineClause = if (!orReplace && ifNotExists) " IF NOT EXISTS" else "" diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserSpec.scala index 4b90f1b5b..5967e7a6d 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserSpec.scala @@ -3281,8 +3281,10 @@ class ParserSpec extends AnyFlatSpec with Matchers { Parser(sql).swap.toOption.get.msg should include(multiIndexQualifierRejection) } - // A self-join through duplicate table names: `From.tableAliases` is keyed by table name and - // keeps only the last alias, so `o.id` never resolves and a "two distinct tables" test sees one. + // A self-join through duplicate table names. Before story BIDC-8 the alias map kept only the + // last alias, so `o.id` never resolved and only `p.parent_id` carried a table; since BIDC-8 both + // resolve through `From.aliasesToTable`. Either way the guard fires — it tests "any qualifier", + // not "two distinct tables", which a one-sided resolution would have defeated. it should "reject a self-correlation through duplicate table names in a watcher input" in { val sql = """CREATE OR REPLACE WATCHER my_watcher AS diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala index acc019463..c4444bbe3 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala @@ -341,14 +341,25 @@ class QuotedTableNameSpec extends AnyFlatSpec with Matchers { ListMap("orders" -> "orders") } - it should "leave two IDENTICALLY qualified same-name tables collapsing, as before (AD-6 scope)" in { - // AD-6 fixes the case where the bare name is AMBIGUOUS. Two references to the SAME qualified - // table are not ambiguous — there is no second reading to disambiguate — so this keeps today's - // behaviour exactly, as does the wholly unqualified self-join below. Pinned so the scope of the - // fix is explicit rather than inferred. - single("""SELECT a FROM "elastic".orders o, "elastic".orders p""").from.tableAliases shouldBe - ListMap("orders" -> "p") - single("SELECT a FROM orders o, orders p").from.tableAliases shouldBe ListMap("orders" -> "p") + it should "keep BOTH aliases of two IDENTICALLY qualified same-name tables in aliasesToTable (BIDC-8)" in { + // Story 21.2 (AD-6) pinned this shape as "collapsing, as before": two references to the SAME + // qualified table are not ambiguous, so `tableAliases` — keyed by TABLE — kept `orders -> p` + // and alias `o` was lost from BOTH maps (`aliasesToTable` was its `.swap`). Story BIDC-8 + // (softclient4es-arrow#144) changes that pin ON PURPOSE: `tableAliases` still holds one alias + // per key (its contract is unchanged — extensions read it forward), while `aliasesToTable` is + // now built directly and keeps every alias. The COMMA spelling is rejected by `From.validate()` + // (a multi-index search cannot join a table to itself — BIDC-8 tripwire 2), so the JOIN + // spelling carries the assertion. + val qualified = single( + """SELECT o.id FROM "elastic".orders o JOIN "elastic".orders p ON o.id = p.parent_id""" + ) + qualified.from.tableAliases shouldBe ListMap("orders" -> "p") + qualified.from.aliasesToTable shouldBe ListMap("o" -> "orders", "p" -> "orders") + val bare = single("SELECT o.id FROM orders o JOIN orders p ON o.id = p.parent_id") + bare.from.tableAliases shouldBe ListMap("orders" -> "p") + bare.from.aliasesToTable shouldBe ListMap("o" -> "orders", "p" -> "orders") + rejected("""SELECT a FROM "elastic".orders o, "elastic".orders p""") + rejected("SELECT a FROM orders o, orders p") } it should "disambiguate only the ambiguous half of a MIXED qualified/bare FROM (AD-6)" in { @@ -448,12 +459,15 @@ class QuotedTableNameSpec extends AnyFlatSpec with Matchers { """CREATE OR REPLACE WATCHER my_watcher AS EVERY 5 MINUTES FROM "a".orders o, """ + """"b".orders p WHERE o.x = 1 WITHIN 2 MINUTES ALWAYS DO LOG_ACTION LOG 'x' END""" ) - // A WHOLLY unqualified self-join still defeats it, exactly as before — the alias map has - // nothing to disambiguate, so `o` does not resolve and no identifier carries a table. - Parser( + // 21.2 pinned the WHOLLY unqualified self-join as still defeating the guard, "exactly as + // before" (the alias map had nothing to disambiguate, so `o` did not resolve). Story BIDC-8 + // flips that pin ON PURPOSE: qualifiers resolve through the lossless `From.aliasesToTable`, so + // `o` resolves here too and the guard fires — a qualifier over a doubled single index is still + // a qualifier a multi-index search cannot scope. + rejected( "CREATE OR REPLACE WATCHER my_watcher AS EVERY 5 MINUTES FROM orders o, orders p " + "WHERE o.x = 1 WITHIN 2 MINUTES ALWAYS DO LOG_ACTION LOG 'x' END" - ).isRight shouldBe true + ) } /** RETARGETED by story 21.7, not deleted: these rows pinned a PENDING CAPABILITY ("the DML and diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala new file mode 100644 index 000000000..16cd8445e --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala @@ -0,0 +1,165 @@ +/* + * 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 org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import scala.collection.immutable.ListMap + +/** Story BIDC-8 — a self-join (`FROM idx a JOIN idx b`) keeps BOTH aliases + * (softclient4es-arrow#144). + * + * MEASURED at the baseline (`origin/main` b37ef940), and pinned in the BEFORE form of this spec + * before the re-key landed (T4): + * + * {{{ + * SELECT a.id, b.amount FROM idx a JOIN idx b ON a.id = b.id + * tableAliases = ListMap(idx -> b) + * aliasesToTable = ListMap(b -> idx) -- alias `a` gone from BOTH maps + * a.id -> name=a.id table=None tableAlias=None -- never resolved + * b.amount -> name=amount table=Some(idx) tableAlias=Some(b) + * joinKeyMatches = Some(List()) -- the ON clause lost its join key + * SELECT a.x, b.y FROM t a, t b -- tripwire 2 (comma-FROM, core path) + * a.x -> name=a.x table=None ; b.y -> name=y table=Some(t) -- a silent wrong answer + * }}} + * + * `tableAliases` is keyed by TABLE and therefore holds ONE alias per table by construction — that + * map's contract is UNCHANGED here (softclient4es-extensions reads it forward). The lossless + * direction is alias -> table: `From.aliasesToTable` is now built directly, and the two resolvers + * (`Identifier.update`, `FieldSort.update`) read it. + */ +class SelfJoinAliasSpec extends AnyFlatSpec with Matchers { + + private def single(sql: String): SingleSearch = + Parser(sql) match { + case Right(ss: SingleSearch) => ss + case Right(other) => fail(s"Expected SingleSearch for [$sql], got $other") + case Left(err) => fail(s"Expected [$sql] to parse, got: ${err.msg}") + } + + /** A GRAMMAR / validation rejection — not story 21.4's boundary catch, which would make a bare + * `isLeft` unfalsifiable. + */ + private def rejectedWith(sql: String, fragment: String): Unit = { + withClue(s"[$sql] ") { noException should be thrownBy Parser(sql) } + val msg = Parser(sql).swap.toOption.map(_.msg).getOrElse(fail(s"[$sql] was accepted")) + withClue(s"[$sql] msg=[$msg] ") { + msg should not startWith Parser.InternalParseFailure + msg should include(fragment) + } + () + } + + private val selfJoin = "SELECT a.id, b.amount FROM idx a JOIN idx b ON a.id = b.id" + + behavior of "From.aliasesToTable on a self-join" + + it should "keep BOTH aliases while tableAliases keeps its one-alias-per-table contract" in { + val from = single(selfJoin).from + from.aliasesToTable shouldBe ListMap("a" -> "idx", "b" -> "idx") + // UNCHANGED, on purpose: keyed by table, one alias per key, the forward-lookup contract every + // consumer outside `sql` speaks. It is `aliasesToTable` that is lossless, not this. + from.tableAliases shouldBe ListMap("idx" -> "b") + from.mainTableKey shouldBe "idx" + from.joinSourceKeys shouldBe Set("idx") + } + + it should "resolve BOTH legs' columns, each tagged with its own alias and the shared index" in { + val ss = single(selfJoin) + val a = ss.select.fields.head.identifier + a.name shouldBe "id" + a.table shouldBe Some("idx") + a.tableAlias shouldBe Some("a") + val b = ss.select.fields(1).identifier + b.name shouldBe "amount" + b.table shouldBe Some("idx") + b.tableAlias shouldBe Some("b") + } + + it should "carry the join key on BOTH sides of the ON clause" in { + val sj = single(selfJoin).from.mainTable.joins.collect { case s: StandardJoin => s }.head + sj.on.map(_.joinKeyMatches) shouldBe Some( + Seq(JoinKeyMatch(JoinKey("idx", "a", "id"), JoinKey("idx", "b", "id"))) + ) + } + + it should "render to SQL that re-parses to the same statement" in { + val ss = single(selfJoin) + Parser(ss.sql) shouldBe Right(ss) + } + + it should "equal the old .swap wherever no two aliases share a key (no regression)" in { + Seq( + "SELECT o.id FROM orders o JOIN customers c ON o.customer_id = c.id", + "SELECT * FROM orders LEFT JOIN customers ON orders.customer_id = customers.id", + "SELECT o.id, i.qty FROM orders o JOIN UNNEST(o.items) AS i", + """SELECT o.id FROM "prod_us".orders o JOIN "prod_eu".orders p ON o.cid = p.id""", + "SELECT a FROM t" + ).foreach { sql => + val from = single(sql).from + withClue(sql)(from.aliasesToTable shouldBe from.tableAliases.map(_.swap)) + } + } + + behavior of "#159's bare-alias canary on a self-join" + + it should "reject ORDER BY on EITHER bare table alias" in { + // Before BIDC-8 `tableAliases` was `ListMap(idx -> b)`, so `ORDER BY a` was not recognised as + // a bare table alias and went through as a sort on a column named `a`. + rejectedWith(s"$selfJoin ORDER BY a", "Column name expected after table alias 'a'") + rejectedWith(s"$selfJoin ORDER BY b", "Column name expected after table alias 'b'") + } + + behavior of "the comma-FROM duplicate shape (tripwire 2)" + + it should "be REJECTED loudly under different aliases, naming the JOIN spelling" in { + // Lead ruling: keep the (silent, wrong) baseline behaviour or reject loudly — never change it + // silently. With the alias map lossless both `a.x` and `b.y` would have resolved against ONE + // doubled index; a comma-separated FROM has no join engine behind it. + rejectedWith( + "SELECT a.x, b.y FROM t a, t b", + "Table t is listed more than once in FROM under different aliases" + ) + rejectedWith("SELECT x FROM t, t a", "FROM t t JOIN t a ON t. = a.") + rejectedWith( + """SELECT a FROM "elastic".orders o, "elastic".orders p""", + """Table "elastic".orders is listed more than once""" + ) + } + + it should "be REJECTED inside every statement kind that embeds a query (INSERT, CTAS, MV)" in { + // `From.validate()` is reached only through the embedding statement's own `validate()`; a + // kind that did not delegate would keep the silently-changed resolution the lead ruled out. + val comma = "SELECT a.x, b.y FROM t a, t b" + rejectedWith(s"INSERT INTO target $comma", "listed more than once in FROM") + rejectedWith(s"CREATE TABLE target AS $comma", "listed more than once in FROM") + rejectedWith( + s"CREATE MATERIALIZED VIEW target REFRESH EVERY 30 SECONDS AS $comma", + "listed more than once in FROM" + ) + } + + it should "leave the same-alias duplicate and the cross-qualifier shape untouched" in { + // `FROM t, t`: key and value identical, nothing was ever lost, nothing changes. + single("SELECT x FROM t, t").from.tableAliases shouldBe ListMap("t" -> "t") + // Story 21.2's shape: two DIFFERENT qualified references, i.e. two tables, not a duplicate. + single("""SELECT o.a FROM "prod_us".orders o, "prod_eu".orders p""").from.tableAliases shouldBe + ListMap("prod_us.orders" -> "o", "prod_eu.orders" -> "p") + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/TemporalLiteralsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/TemporalLiteralsSpec.scala index 292de7f91..aa24fa76e 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/TemporalLiteralsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/TemporalLiteralsSpec.scala @@ -415,6 +415,18 @@ class TemporalLiteralsSpec extends AnyFlatSpec with Matchers { where should include("c.event_ts >= '2026-06-04 00:00:00'") } + it should "resolve BOTH legs of a self-join against the mapping in hand (story BIDC-8)" in { + // The join source IS the main index, so its mapping IS the one in hand: the cross-index guard + // above must not fire on it. Before BIDC-8 the first leg's qualifier did not resolve at all, + // so neither literal was touched. + val sql = + "SELECT a.id FROM events a JOIN events b ON a.id = b.id " + + "WHERE a.event_ts >= '2026-06-04 00:00:00' AND b.event_ts >= '2026-06-04 00:00:00'" + val where = whereSql(resolved(sql)) + where should include("a.event_ts >= '2026-06-04T00:00:00'") + where should include("b.event_ts >= '2026-06-04T00:00:00'") + } + it should "resolve columns from a real Elasticsearch mapping, excluding date_nanos by construction" in { val json = """{"events":{"aliases":{},"mappings":{"properties":{ From 3d7518d1059755f92c33d8ba500bd8c14854beee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Fri, 11 Sep 2026 18:51:17 +0200 Subject: [PATCH 02/10] fix(BIDC-8): reject one alias for two sources; pin the review round's findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 518da2f5 after the independent review round (references SOFTNETWORK-APP/softclient4es-arrow#144; the arrow PR carries the closing reference). - `From.validate()` rejects one alias used for more than one source: `FROM orders o JOIN customers o` and `FROM t JOIN t ON t.id = t.parent` both parsed — every `o.x` resolved against the LAST source and the arrow planner registered two legs as the same `sq_o` (DuckDB: "Attempting to execute an unsuccessful or closed pending query result"). The exempt shape stays `FROM t, t` under ONE alias (a multi-index search, nothing resolves differently). - The comma-FROM remedy text no longer invents an alias token for an alias-less occurrence. - The AD-6 scaladoc names `aliasesToTable` as the resolver's map (it used to say a reverse scan). - Pins: the watcher-flip case asserts #191's exact message, not merely a rejection; the UNNEST whose nested field is named like its table (`FROM orders o JOIN UNNEST(o.orders) AS i`) now resolves `o.id` where the lossy `.swap` left it a literal field name — two aliases share a key there without a self-join; one-alias-for-two-sources rejections. - `documentation/sql/joins.md`: the self-join section states the four rules that ship (qualify every column; alias a column selected from both legs; one alias per leg; JOIN not comma) and the alias-in-ORDER-BY note describes the behaviour the planner and the connection now have. Release notes (0.23.0), in addition to 518da2f5: one alias for two sources is rejected; an UNNEST named like its table resolves the table qualifier. Tests: sql 1056 (+2), core 979, `+ sql/compile`, `++ 2.12.20 sql/Test/compile`. Co-Authored-By: Claude Fable 5.1 --- documentation/sql/joins.md | 4 +- .../softnetwork/elastic/sql/query/From.scala | 66 ++++++++++++++----- .../sql/parser/QuotedTableNameSpec.scala | 6 +- .../elastic/sql/query/SelfJoinAliasSpec.scala | 39 ++++++++++- 4 files changed, 96 insertions(+), 19 deletions(-) diff --git a/documentation/sql/joins.md b/documentation/sql/joins.md index 3639426eb..3c77be52f 100644 --- a/documentation/sql/joins.md +++ b/documentation/sql/joins.md @@ -111,7 +111,7 @@ ORDER BY COUNT(*) DESC; -- Engineering (3), Marketing (2) survive HAVING ``` -> **SELECT aliases and ordinals work here** — since arrow-extensions **0.2.5** (REPL bundle `0.20.4`, JDBC / ADBC / Flight SQL driver `0.2.5`). `ORDER BY cnt`, `HAVING cnt > 1`, `GROUP BY` on an alias and ordinal forms such as `ORDER BY 2` all resolve against the final SELECT list *after* the join, and alias matching is case-insensitive. Two limits remain: an alias is **not** legal in `SELECT`, `ON` or `WHERE` — nothing has been computed at that point — and it must be written **bare** — `ORDER BY d.cnt` qualifies a name no table owns, and since arrow-extensions **0.3.3** it is rejected with an error that says exactly that and names the bare spelling (before 0.3.3 it failed inside DuckDB with an internal `Invalid Input Error`). Before 0.2.5 all of these were rejected with `Ambiguous column`. +> **SELECT aliases and ordinals work here** — since arrow-extensions **0.2.5** (REPL bundle `0.20.4`, JDBC / ADBC / Flight SQL driver `0.2.5`). `ORDER BY cnt`, `HAVING cnt > 1`, `GROUP BY` on an alias and ordinal forms such as `ORDER BY 2` all resolve against the final SELECT list *after* the join, and alias matching is case-insensitive. Two limits remain: an alias is **not** legal in `SELECT`, `ON` or `WHERE` — nothing has been computed at that point — and it must be written **bare** — `ORDER BY d.cnt` qualifies a name no table owns. Since arrow-extensions **0.3.3** the planner rejects the shapes it can recognise (a table-alias-qualified name that is a computed alias or a renamed column, such as `d.cnt` or `d.dname` for `d.dept_name AS dname`) with an error naming the bare spelling, and a leg that asks Elasticsearch for a column its index does not have fails with an error naming the column and the table alias — never DuckDB's internal `Invalid Input Error`, which is what both shapes produced before 0.3.3. A qualified name that really is a column keeps working even when it spells an alias (`o.id AS id … ORDER BY o.id`, `ROUND(o.amount) AS amount … ORDER BY o.amount`). Before 0.2.5 all of these were rejected with `Ambiguous column`. > > **The remaining ORDER BY gotcha:** you cannot `ORDER BY` a column that exists on **both** sides of the JOIN — order by a column unique to one side, e.g. `d.dept_name`, not the shared join key `d.dept_id`. @@ -132,7 +132,7 @@ JOIN orders b ON a.customer_id = b.customer_id; -- every pair of orders sharing a customer (fan-out, see below) ``` -> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Two rules that follow from the fix: **qualify every column** (a bare `id` is ambiguous between the two legs and is rejected as such), and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is now rejected with a message pointing at the `JOIN` spelling. +> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value); **give each leg its own alias** (`FROM orders a JOIN orders a` and `FROM orders JOIN orders` are rejected); and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. ### JOIN cardinality — fan-out on a non-unique key diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala index acb3b542c..ef5587124 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala @@ -402,13 +402,14 @@ case class From(tables: Seq[Table]) extends Updateable { * regression" is half of the ruling, not a footnote to it. * * ⚠️ **Narrowing the rule does not by itself keep the rest of `sql` in step — a companion change - * was needed and is `joinSourceKeys` below.** `Identifier.update` (`sql/package.scala`) reverse- - * looks-up this map and puts the KEY into `Identifier.table`, so in the ambiguous branch that - * value is the qualified reference. Anything inside `sql` that compares `Identifier.table` - * against an index name must therefore speak the same key language: `TemporalLiterals` did not, - * and its cross-index join-leg guard stopped firing for `FROM orders o JOIN "prod_eu".orders p` - * until it was routed through `joinSourceKeys`. **Any new consumer of `Identifier.table` - * inherits that obligation.** + * was needed and is `joinSourceKeys` below.** `Identifier.update` (`sql/package.scala`) resolves + * a qualifier through `aliasesToTable` (since story BIDC-8; it used to reverse-scan this map) + * and puts the resulting KEY — the same `aliasKey` language — into `Identifier.table`, so in the + * ambiguous branch that value is the qualified reference. Anything inside `sql` that compares + * `Identifier.table` against an index name must therefore speak the same key language: + * `TemporalLiterals` did not, and its cross-index join-leg guard stopped firing for `FROM orders + * o JOIN "prod_eu".orders p` until it was routed through `joinSourceKeys`. **Any new consumer of + * `Identifier.table` inherits that obligation.** */ private def aliasKey(name: String, qualified: String): String = if (ambiguousTableNames.contains(name)) qualified else name @@ -478,9 +479,14 @@ case class From(tables: Seq[Table]) extends Updateable { * break under any other key language. The table side of every entry below uses the SAME * `aliasKey`, so the values here ARE `tableAliases` keys — `Identifier.table` stays a * `tableAliases` key (story 21.2 AD-6) and `schemas` / `joinSourceKeys` / `TemporalLiterals` - * need no new language. Whenever no two aliases share a key this map equals the old `.swap`. + * need no new language. Whenever no two aliases share a key this map equals the old `.swap`. Two + * aliases share a key WITHOUT a self-join in one more shape (review L-2): an UNNEST whose nested + * field is named like its table — `FROM orders o JOIN UNNEST(o.orders) AS i` put `orders -> o` + * and `orders -> i` under one key, so `.swap` lost `o` and `o.id` stayed a literal field name; + * it now resolves. Pinned in `SelfJoinAliasSpec`. * - * Two legs sharing an ALIAS still collapse here (last wins), exactly as `joinReferences` does. + * Two legs sharing an ALIAS still collapse here (last wins), exactly as `joinReferences` does; + * `validate()` rejects that shape since BIDC-8. */ lazy val aliasesToTable: ListMap[String, String] = ListMap( (tables.map(table => effectiveAlias(table) -> aliasKey(table.name, table.qualifiedName)) ++ @@ -550,19 +556,49 @@ case class From(tables: Seq[Table]) extends Updateable { val rendered = Table.render(table.parts, table.name) val aliases = tables.filter(_.qualifiedName == table.qualifiedName).map(effectiveAlias).distinct + // An alias-less occurrence has no alias token to repeat in the remedy (review N-7). val (a, b) = (aliases.head, aliases(1)) + def ref(alias: String): String = + if (alias == table.name) rendered else s"$rendered $alias" Left( s"Table $rendered is listed more than once in FROM under different aliases ($a, $b); " + "a comma-separated FROM searches several indices and cannot join a table to itself. " + - s"Write a self-join as FROM $rendered $a JOIN $rendered $b ON $a. = $b." + s"Write a self-join as FROM ${ref(a)} JOIN ${ref(b)} ON $a. = $b." ) case None => - for { - _ <- tables.map(_.validate()).filter(_.isLeft) match { - case Nil => Right(()) - case errors => Left(errors.map { case Left(err) => err }.mkString("\n")) + // Story BIDC-8 (review M1) — one alias for two SOURCES. `FROM orders o JOIN customers o` + // and `FROM t JOIN t ON t.id = t.parent` both parsed: the alias maps kept the LAST + // source under `o`/`t`, every `o.x` resolved against it, and the arrow planner + // registered two legs as the same `sq_o` (DuckDB: "Attempting to execute an + // unsuccessful or closed pending query result"). Comma-listed duplicates of the SAME + // table under the SAME alias (`FROM t, t`) are deliberately exempt — a multi-index + // search de-duplicates them and nothing resolves differently. + val sources: Seq[(String, String, Boolean)] = + tables.map(t => (effectiveAlias(t), t.qualifiedName, false)) ++ + joins.collect { case sj: StandardJoin => + (sj.alias.map(_.alias).getOrElse(sj.source.name), sj.qualifiedName, true) } - } yield () + val reusedAlias = sources + .groupBy(_._1) + .collectFirst { + case (alias, uses) + if uses.size > 1 && !(uses.forall(u => !u._3 && u._2 == uses.head._2)) => + (alias, uses.map(_._2)) + } + reusedAlias match { + case Some((alias, names)) => + Left( + s"Alias '$alias' is used for more than one table (${names.mkString(", ")}); give " + + "each table its own alias" + ) + case None => + for { + _ <- tables.map(_.validate()).filter(_.isLeft) match { + case Nil => Right(()) + case errors => Left(errors.map { case Left(err) => err }.mkString("\n")) + } + } yield () + } } } } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala index c4444bbe3..3d5922592 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala @@ -464,9 +464,13 @@ class QuotedTableNameSpec extends AnyFlatSpec with Matchers { // flips that pin ON PURPOSE: qualifiers resolve through the lossless `From.aliasesToTable`, so // `o` resolves here too and the guard fires — a qualifier over a doubled single index is still // a qualifier a multi-index search cannot scope. - rejected( + val watcherSelfJoin = "CREATE OR REPLACE WATCHER my_watcher AS EVERY 5 MINUTES FROM orders o, orders p " + "WHERE o.x = 1 WITHIN 2 MINUTES ALWAYS DO LOG_ACTION LOG 'x' END" + rejected(watcherSelfJoin) + // It must be #191's guard that fires, not just any grammar rejection (review M-1). + Parser(watcherSelfJoin).swap.toOption.map(_.msg).getOrElse("") should include( + "cannot qualify a column by table when it searches several indices" ) } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala index 16cd8445e..edcf045f6 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala @@ -136,7 +136,8 @@ class SelfJoinAliasSpec extends AnyFlatSpec with Matchers { "SELECT a.x, b.y FROM t a, t b", "Table t is listed more than once in FROM under different aliases" ) - rejectedWith("SELECT x FROM t, t a", "FROM t t JOIN t a ON t. = a.") + // An alias-less occurrence carries no alias token in the remedy (review N-7). + rejectedWith("SELECT x FROM t, t a", "FROM t JOIN t a ON t. = a.") rejectedWith( """SELECT a FROM "elastic".orders o, "elastic".orders p""", """Table "elastic".orders is listed more than once""" @@ -155,6 +156,42 @@ class SelfJoinAliasSpec extends AnyFlatSpec with Matchers { ) } + behavior of "one alias for two sources (review M1)" + + it should "be REJECTED for a JOIN leg reusing the main table's alias, and for two legs under one alias" in { + // Before BIDC-8 both parsed: `o.x` resolved against the LAST source and the arrow planner + // registered two legs as the same `sq_o`. + rejectedWith( + "SELECT o.x FROM orders o JOIN customers o ON o.id = o.cid", + "Alias 'o' is used for more than one table (orders, customers)" + ) + rejectedWith( + "SELECT t.x FROM t JOIN t ON t.id = t.parent", + "Alias 't' is used for more than one table" + ) + rejectedWith( + "SELECT a.x FROM orders a JOIN orders a ON a.id = a.parent_id", + "Alias 'a' is used for more than one table" + ) + // Distinct aliases keep planning; the exempt comma shape stays accepted. + single("SELECT a.x FROM orders a JOIN orders b ON a.id = b.parent_id") + single("SELECT x FROM t, t") + } + + behavior of "an UNNEST whose nested field is named like its table (review L-2)" + + it should "now resolve the table's own alias, which the lossy .swap used to lose" in { + // `tableAliases` puts `orders -> o` (the table) and `orders -> i` (the unnest, keyed by its + // nested field name) under ONE key, so before BIDC-8 the `.swap` kept only `i` and `o.id` + // stayed a literal field name. Two aliases share a key here without any self-join. + val ss = single("SELECT o.id, i.qty FROM orders o JOIN UNNEST(o.orders) AS i") + ss.from.aliasesToTable shouldBe ListMap("o" -> "orders", "i" -> "orders") + val id = ss.select.fields.head.identifier + id.name shouldBe "id" + id.table shouldBe Some("orders") + id.tableAlias shouldBe Some("o") + } + it should "leave the same-alias duplicate and the cross-qualifier shape untouched" in { // `FROM t, t`: key and value identical, nothing was ever lost, nothing changes. single("SELECT x FROM t, t").from.tableAliases shouldBe ListMap("t" -> "t") From bc65b8c0c6de10d367353a83bae371ee3b8d122e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Fri, 11 Sep 2026 19:18:33 +0200 Subject: [PATCH 03/10] fix(BIDC-8): duplicate-alias guard compares explicit aliases only, case-insensitively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 follow-up (delta review of 3d7518d1); references SOFTNETWORK-APP/softclient4es-arrow#144, the arrow branch carries the closing reference. - NEW-4: `From.validate()`'s one-alias-for-two-sources check treated an alias-less table's bare name as its alias and rejected 21.2's preserve-don't-interpret multi-index searches (`FROM "prod_us".orders, "prod_eu".orders`, `FROM "a".orders, orders`) with an alias nobody wrote. Only EXPLICIT aliases are compared now; `FROM t, t` stays accepted and the alias-less `FROM t JOIN t` is left to the join planner's own named guard. `FROM t a, t a` and `FROM orders a JOIN orders a` stay rejected. - NEW-3: the comparison is case-insensitive — DuckDB's catalog folds `sq_A` and `sq_a`, so `FROM orders A JOIN orders a` passed both guards and died with the forbidden "closed pending query result" string. - `documentation/sql/joins.md`: the alias rule states the case-insensitivity; the alias-in-ORDER-BY note says a column used only in a leg's own pushed-down WHERE is accepted even when the mapping does not have it (the arrow executor backstop's WHERE-only exemption). Tests: sql 1058 (+2), core 979, `+ sql/compile`, `++ 2.12.20 sql/Test/compile`. Co-Authored-By: Claude Fable 5.1 --- documentation/sql/joins.md | 4 +- .../softnetwork/elastic/sql/query/From.scala | 37 ++++++++++--------- .../sql/parser/QuotedTableNameSpec.scala | 9 +++++ .../elastic/sql/query/SelfJoinAliasSpec.scala | 24 ++++++++---- 4 files changed, 47 insertions(+), 27 deletions(-) diff --git a/documentation/sql/joins.md b/documentation/sql/joins.md index 3c77be52f..fc1c7846e 100644 --- a/documentation/sql/joins.md +++ b/documentation/sql/joins.md @@ -111,7 +111,7 @@ ORDER BY COUNT(*) DESC; -- Engineering (3), Marketing (2) survive HAVING ``` -> **SELECT aliases and ordinals work here** — since arrow-extensions **0.2.5** (REPL bundle `0.20.4`, JDBC / ADBC / Flight SQL driver `0.2.5`). `ORDER BY cnt`, `HAVING cnt > 1`, `GROUP BY` on an alias and ordinal forms such as `ORDER BY 2` all resolve against the final SELECT list *after* the join, and alias matching is case-insensitive. Two limits remain: an alias is **not** legal in `SELECT`, `ON` or `WHERE` — nothing has been computed at that point — and it must be written **bare** — `ORDER BY d.cnt` qualifies a name no table owns. Since arrow-extensions **0.3.3** the planner rejects the shapes it can recognise (a table-alias-qualified name that is a computed alias or a renamed column, such as `d.cnt` or `d.dname` for `d.dept_name AS dname`) with an error naming the bare spelling, and a leg that asks Elasticsearch for a column its index does not have fails with an error naming the column and the table alias — never DuckDB's internal `Invalid Input Error`, which is what both shapes produced before 0.3.3. A qualified name that really is a column keeps working even when it spells an alias (`o.id AS id … ORDER BY o.id`, `ROUND(o.amount) AS amount … ORDER BY o.amount`). Before 0.2.5 all of these were rejected with `Ambiguous column`. +> **SELECT aliases and ordinals work here** — since arrow-extensions **0.2.5** (REPL bundle `0.20.4`, JDBC / ADBC / Flight SQL driver `0.2.5`). `ORDER BY cnt`, `HAVING cnt > 1`, `GROUP BY` on an alias and ordinal forms such as `ORDER BY 2` all resolve against the final SELECT list *after* the join, and alias matching is case-insensitive. Two limits remain: an alias is **not** legal in `SELECT`, `ON` or `WHERE` — nothing has been computed at that point — and it must be written **bare** — `ORDER BY d.cnt` qualifies a name no table owns. Since arrow-extensions **0.3.3** the planner rejects the shapes it can recognise (a table-alias-qualified name that is a computed alias or a renamed column, such as `d.cnt` or `d.dname` for `d.dept_name AS dname`) with an error naming the bare spelling, and a leg that asks Elasticsearch for a column its index does not have fails with an error naming the column and the table alias — never DuckDB's internal `Invalid Input Error`, which is what both shapes produced before 0.3.3. A qualified name that really is a column keeps working even when it spells an alias (`o.id AS id … ORDER BY o.id`, `ROUND(o.amount) AS amount … ORDER BY o.amount`), and a column used only in a leg's own pushed-down `WHERE` (`WHERE o.deleted_at IS NULL` on an index no document has carried the field into yet) is still accepted even when the mapping does not have it. Before 0.2.5 all of these were rejected with `Ambiguous column`. > > **The remaining ORDER BY gotcha:** you cannot `ORDER BY` a column that exists on **both** sides of the JOIN — order by a column unique to one side, e.g. `d.dept_name`, not the shared join key `d.dept_id`. @@ -132,7 +132,7 @@ JOIN orders b ON a.customer_id = b.customer_id; -- every pair of orders sharing a customer (fan-out, see below) ``` -> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value); **give each leg its own alias** (`FROM orders a JOIN orders a` and `FROM orders JOIN orders` are rejected); and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. +> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value); **give each leg its own alias** — aliases compare case-insensitively, so `FROM orders a JOIN orders a`, `FROM orders A JOIN orders a` and `FROM orders JOIN orders` are all rejected; and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. ### JOIN cardinality — fan-out on a non-unique key diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala index ef5587124..77be732b0 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/From.scala @@ -566,25 +566,26 @@ case class From(tables: Seq[Table]) extends Updateable { s"Write a self-join as FROM ${ref(a)} JOIN ${ref(b)} ON $a. = $b." ) case None => - // Story BIDC-8 (review M1) — one alias for two SOURCES. `FROM orders o JOIN customers o` - // and `FROM t JOIN t ON t.id = t.parent` both parsed: the alias maps kept the LAST - // source under `o`/`t`, every `o.x` resolved against it, and the arrow planner - // registered two legs as the same `sq_o` (DuckDB: "Attempting to execute an - // unsuccessful or closed pending query result"). Comma-listed duplicates of the SAME - // table under the SAME alias (`FROM t, t`) are deliberately exempt — a multi-index - // search de-duplicates them and nothing resolves differently. - val sources: Seq[(String, String, Boolean)] = - tables.map(t => (effectiveAlias(t), t.qualifiedName, false)) ++ - joins.collect { case sj: StandardJoin => - (sj.alias.map(_.alias).getOrElse(sj.source.name), sj.qualifiedName, true) - } - val reusedAlias = sources - .groupBy(_._1) - .collectFirst { - case (alias, uses) - if uses.size > 1 && !(uses.forall(u => !u._3 && u._2 == uses.head._2)) => - (alias, uses.map(_._2)) + // Story BIDC-8 (review M1) — one EXPLICIT alias for two SOURCES. `FROM orders o JOIN + // customers o` parsed: the alias maps kept the LAST source under `o`, every `o.x` + // resolved against it, and the arrow planner registered two legs as the same `sq_o` + // (DuckDB: "Attempting to execute an unsuccessful or closed pending query result"). + // Only aliases somebody WROTE are compared (review NEW-4): an alias-less table's bare + // name is not an alias, so `FROM "prod_us".orders, "prod_eu".orders` and `FROM t, t` + // keep their multi-index-search acceptance (21.2 preserves, it does not interpret), and + // the alias-less `FROM t JOIN t` is left to the join planner's own guard. The comparison + // is case-INSENSITIVE (review NEW-3): DuckDB's catalog folds `sq_A` and `sq_a`. + val explicitAliases: Seq[(String, String)] = + tables.flatMap(t => + t.tableAlias.map(_.alias).filter(_.nonEmpty).map(_ -> t.qualifiedName) + ) ++ + joins.collect { + case sj: StandardJoin if sj.alias.exists(_.alias.nonEmpty) => + sj.alias.get.alias -> sj.qualifiedName } + val reusedAlias = explicitAliases + .groupBy(_._1.toLowerCase(java.util.Locale.ROOT)) + .collectFirst { case (_, uses) if uses.size > 1 => (uses.head._1, uses.map(_._2)) } reusedAlias match { case Some((alias, names)) => Left( diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala index 3d5922592..e74a01618 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedTableNameSpec.scala @@ -362,6 +362,15 @@ class QuotedTableNameSpec extends AnyFlatSpec with Matchers { rejected("SELECT a FROM orders o, orders p") } + it should "keep alias-less multi-index searches over same-name tables accepted (BIDC-8 review NEW-4)" in { + // The duplicate-alias guard compares EXPLICIT aliases only; a bare table name is not an alias + // anybody wrote, so these 21.2 shapes parse exactly as before. + single("""SELECT x FROM "prod_us".orders, "prod_eu".orders""").from.tableAliases shouldBe + ListMap("prod_us.orders" -> "orders", "prod_eu.orders" -> "orders") + single("""SELECT x FROM "a".orders, orders""").from.tableAliases shouldBe + ListMap("a.orders" -> "orders", "orders" -> "orders") + } + it should "disambiguate only the ambiguous half of a MIXED qualified/bare FROM (AD-6)" in { // One leg qualified, one bare, same index name: the bare name IS ambiguous (two distinct // qualified references, `orders` and `prod_eu.orders`), so both keys survive and the bare leg diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala index edcf045f6..f5b521423 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala @@ -158,24 +158,34 @@ class SelfJoinAliasSpec extends AnyFlatSpec with Matchers { behavior of "one alias for two sources (review M1)" - it should "be REJECTED for a JOIN leg reusing the main table's alias, and for two legs under one alias" in { - // Before BIDC-8 both parsed: `o.x` resolved against the LAST source and the arrow planner + it should "be REJECTED for an EXPLICIT alias written for two sources, case-insensitively" in { + // Before BIDC-8 these parsed: `o.x` resolved against the LAST source and the arrow planner // registered two legs as the same `sq_o`. rejectedWith( "SELECT o.x FROM orders o JOIN customers o ON o.id = o.cid", "Alias 'o' is used for more than one table (orders, customers)" ) - rejectedWith( - "SELECT t.x FROM t JOIN t ON t.id = t.parent", - "Alias 't' is used for more than one table" - ) rejectedWith( "SELECT a.x FROM orders a JOIN orders a ON a.id = a.parent_id", "Alias 'a' is used for more than one table" ) - // Distinct aliases keep planning; the exempt comma shape stays accepted. + rejectedWith("SELECT a.x FROM t a, t a", "is used for more than one table") + // DuckDB's catalog is case-insensitive (`sq_A` == `sq_a`), so the guard is too (review NEW-3). + rejectedWith( + "SELECT a.x FROM orders A JOIN orders a ON A.id = a.parent_id", + "is used for more than one table" + ) + // Distinct aliases keep planning. single("SELECT a.x FROM orders a JOIN orders b ON a.id = b.parent_id") + } + + it should "leave alias-less duplicates alone: a bare name is not an alias anybody wrote (review NEW-4)" in { + // 21.2's preserve-don't-interpret multi-index searches must keep parsing; the alias-less + // self-JOIN is left to the join planner's own duplicate-alias guard. single("SELECT x FROM t, t") + single("""SELECT x FROM "prod_us".orders, "prod_eu".orders""") + single("""SELECT x FROM "a".orders, orders""") + single("SELECT t.x FROM t JOIN t ON t.id = t.parent") } behavior of "an UNNEST whose nested field is named like its table (review L-2)" From 4e1ebe5ea2e20911aab66feca8a5e9fe4ee18ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 00:08:19 +0200 Subject: [PATCH 04/10] =?UTF-8?q?docs(BIDC-8):=20joins.md=20=E2=80=94=20th?= =?UTF-8?q?e=20REPL=20names=20colliding=20columns=20.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lead ruling NEW-1b (2026-09-11): over two different indices sharing a column name the REPL returns every row and names only the colliding columns `.` (`o.id`, `c.id`), leaving the others bare; JDBC / ADBC / Flight SQL keep Arrow's duplicate labels. Identical wording in softclient4es-web joins.mdx. References SOFTNETWORK-APP/softclient4es-arrow#144; the arrow branch carries the closing reference. Co-Authored-By: Claude Fable 5.1 --- documentation/sql/joins.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/sql/joins.md b/documentation/sql/joins.md index fc1c7846e..c04dd8677 100644 --- a/documentation/sql/joins.md +++ b/documentation/sql/joins.md @@ -132,7 +132,7 @@ JOIN orders b ON a.customer_id = b.customer_id; -- every pair of orders sharing a customer (fan-out, see below) ``` -> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value); **give each leg its own alias** — aliases compare case-insensitively, so `FROM orders a JOIN orders a`, `FROM orders A JOIN orders a` and `FROM orders JOIN orders` are all rejected; and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. +> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value — over two *different* indices that share a column name the REPL names the colliding columns `.` (`o.id`, `c.id`) and leaves the others bare, while JDBC / ADBC / Flight SQL keep Arrow's duplicate labels); **give each leg its own alias** — aliases compare case-insensitively, so `FROM orders a JOIN orders a`, `FROM orders A JOIN orders a` and `FROM orders JOIN orders` are all rejected; and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. ### JOIN cardinality — fan-out on a non-unique key From 9daa5c2a79f39f88df31f87bd72028f7b5dd0ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 00:32:26 +0200 Subject: [PATCH 05/10] =?UTF-8?q?docs(BIDC-8):=20joins.md=20=E2=80=94=20SE?= =?UTF-8?q?LECT=20*=20over=20a=20JOIN=20omits=20object=20and=20nested=20co?= =?UTF-8?q?lumns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 follow-up (review MEDIUM-4): the self-join paragraph states that `SELECT *` over a JOIN omits object and nested columns and their sub-fields — list them explicitly to select sub-fields. Identical wording in softclient4es-web joins.mdx. References SOFTNETWORK-APP/softclient4es-arrow#144; the arrow branch carries the closing reference. Co-Authored-By: Claude Fable 5.1 --- documentation/sql/joins.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/sql/joins.md b/documentation/sql/joins.md index c04dd8677..faa617f96 100644 --- a/documentation/sql/joins.md +++ b/documentation/sql/joins.md @@ -132,7 +132,7 @@ JOIN orders b ON a.customer_id = b.customer_id; -- every pair of orders sharing a customer (fan-out, see below) ``` -> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value — over two *different* indices that share a column name the REPL names the colliding columns `.` (`o.id`, `c.id`) and leaves the others bare, while JDBC / ADBC / Flight SQL keep Arrow's duplicate labels); **give each leg its own alias** — aliases compare case-insensitively, so `FROM orders a JOIN orders a`, `FROM orders A JOIN orders a` and `FROM orders JOIN orders` are all rejected; and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. +> **Since engine `0.23.0` with arrow-extensions `0.3.3`.** Before that, the alias map kept only the *last* alias of a repeated index, so the first leg's columns never resolved, the `ON` clause lost its join key and DuckDB answered `Parser Error: syntax error at end of input` (softclient4es-arrow#144). Four rules follow, each enforced with a message that says why: **qualify every column** (a bare `id` is ambiguous between the two legs); **alias a column you select from both legs** (`SELECT a.id AS left_id, b.id AS right_id` — `SELECT a.id, b.id`, `SELECT *` and `a.*` over a self-join are rejected, because the result would carry the same column name twice and keep only one value — over two *different* indices that share a column name the REPL names the colliding columns `.` (`o.id`, `c.id`) and leaves the others bare, while JDBC / ADBC / Flight SQL keep Arrow's duplicate labels; and `SELECT *` over a JOIN omits object and nested columns and their sub-fields — list them explicitly to select sub-fields); **give each leg its own alias** — aliases compare case-insensitively, so `FROM orders a JOIN orders a`, `FROM orders A JOIN orders a` and `FROM orders JOIN orders` are all rejected; and write a self-join with `JOIN … ON` — the comma form `FROM orders a, orders b` is a multi-index *search* with no join engine behind it and is rejected with a message pointing at the `JOIN` spelling. ### JOIN cardinality — fan-out on a non-unique key From 3acc094407ee8c96f50a4470eb2a6c31c27d22d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 01:58:08 +0200 Subject: [PATCH 06/10] fix(BIDC-8): a WHERE predicate with a function now compiles and follows ANSI three-valued logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AD-9, lead ruling option (a). References SOFTNETWORK-APP/softclient4es-arrow#144; the arrow branch carries the closing reference. MEASURED on live Elasticsearch 8.18.3 before the change: `WHERE UPPER(status) <> 'ZZZ'` emitted `(param1 == null) ? null : param1.toUpperCase().compareTo("ZZZ") != 0`, whose ternary branches are `null` (Object) and a primitive `boolean`. Elasticsearch rejected the WHOLE query at compile time — `script_exception: compile error`, caused by `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]` — and the failure is data-independent: it fires on an EMPTY index. The numeric family failed identically (`Cannot cast from [java.lang.Double] to [int]`); the date family escaped only because `YEAR(...)` folds into the parameter assignment and took the already-correct guarded branch. So a leg-local function predicate had never executed. Two further defects sat on the same path, both silent: - a composed predicate was NOT parenthesised, and `?:` is the loosest operator in Painless, so `a == null ? false : (x) && b == null ? false : (y)` parsed as `a == null ? false : (((x) && b == null) ? false : (y))` — the sibling became part of the first guard's condition (the rule `bucketPipelinePainless`'s own scaladoc states for HAVING, never applied to WHERE); - `check` chose its hard-coded spelling (`compareTo(...) == 0`, `isEqual(...)`) from the RAW operator while only the generic fallback consulted `painlessOp`, so a `NOT` was dropped. The shape shipped, in a QUERY context only: a left-hand side that reads a document field (`nullable || dependencies.nonEmpty`) is bound to a local declared in the script PROLOGUE — a Painless `def x = …;` is a statement and an operand of a composed predicate must be an expression — and the comparison lands INSIDE the guard, `( == null ? false : ())`; every Predicate operand is parenthesised; `check` dispatches on the effective, NOT-folded operator. `script_fields`, sorts, aggregations and ingest processors keep `null` (21.8 Part C): their expectations are byte-identical. ANSI three-valued logic holds because `NOT` is folded into the operator before the guard and `NOT ()` is not expressible in this dialect (measured) — the ruling's `Boolean`-boxing sketch was therefore unnecessary and is recorded as the rejected alternative. Fixtures: three filter expectations in BOTH bridge copies (template + hand-maintained es6), each justified old-bytes/new-bytes/why in the PR body, plus two re-spacing rules for the whitespace-stripped expectation chains. No script_fields expectation moved. Behaviour pins (row sets, never bytes): `GatewayApiIntegrationSpec` gains 11 assertions on real ES with id3's field ABSENT — seeded via COPY INTO because an INSERT omitting a column writes an empty string — including the two un-scripted shapes that show the other route (`NOT status = 'A'` is ES `must_not`, which INCLUDES a document lacking the field: pre-existing, unchanged, now visible). Release notes (0.23.0): a `WHERE` predicate applying a function to a column now runs (it previously failed the whole query with the compile error above) and follows ANSI three-valued logic for absent fields; a `NOT` in a scripted predicate is no longer dropped. Documented in `documentation/sql/dql_statements.md` (its MDX twin needs the same paragraph). Tests: sql 1058, core 979, `+ sql/compile`, `++ 2.12.20 sql/Test/compile`; bridge suites 197/197 on es6, es7, es8, es9; es8 client integration 412 (+ the new pin, 75/75 on JavaClientGatewayApiSpec). Co-Authored-By: Claude Fable 5.1 --- .../elastic/sql/SQLQuerySpec.scala | 35 +++++++- documentation/sql/dql_statements.md | 15 ++++ .../elastic/sql/SQLQuerySpec.scala | 35 +++++++- .../app/softnetwork/elastic/sql/package.scala | 42 +++++++--- .../softnetwork/elastic/sql/query/Where.scala | 60 ++++++++++++- .../client/GatewayApiIntegrationSpec.scala | 84 +++++++++++++++++++ 6 files changed, 252 insertions(+), 19 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 ded8dd920..32fcc25f6 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -1652,6 +1652,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defe", "def e") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -1705,6 +1707,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defe", "def e") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -2194,6 +2198,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defv", " def v") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -2372,6 +2378,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defv", " def v") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -2668,7 +2676,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "script": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().get(ChronoField.YEAR); (param1 == null) ? null : (param1 * (param2 - 10)) > 10000", + | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().get(ChronoField.YEAR); def left1 = (param1 == null) ? null : (param1 * (param2 - 10)); (left1 == null ? false : ((left1 > 10000)))", | "params": { | "__now__": 1767139200000 | } @@ -2730,6 +2738,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("defr", "def r") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") // .replaceAll("(\\d)=", "$1 =") @@ -2764,7 +2773,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "script": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); (param1 == null) ? null : Double.valueOf(Math.sqrt(param1)) > 100.0" + | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def left1 = (param1 == null) ? null : Double.valueOf(Math.sqrt(param1)); (left1 == null ? false : ((left1 > 100.0)))" | } | } | } @@ -2893,6 +2902,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defp", "def p") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") @@ -2937,7 +2948,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "script": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); (param1 == null) ? null : param1.trim().length() > 10" + | "source": "def param1 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); def left1 = (param1 == null) ? null : param1.trim().length(); (left1 == null ? false : ((left1 > 10)))" | } | } | } @@ -3042,6 +3053,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3174,6 +3187,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3246,6 +3261,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3386,6 +3403,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3508,6 +3527,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defv", " def v") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3599,6 +3620,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3712,6 +3735,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3827,6 +3852,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3931,6 +3958,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 228facf02..2c5d3f071 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -410,6 +410,21 @@ The `WHERE` clause supports: - `LIKE`, `RLIKE` (regex) - conditions on nested fields (`profile.city`, `profile.followers`) +> **A function in a `WHERE` predicate, and documents that do not carry the field.** A predicate that +> applies a function to a column (`WHERE UPPER(status) = 'A'`, `WHERE ABS(amount) > 10`) is executed +> by Elasticsearch as a Painless script. Since engine **0.23.0** such a predicate follows ANSI +> three-valued logic for a document in which the field is **absent**: the comparison is NULL, so the +> document does not match — and it does not match the negated form either (`WHERE NOT UPPER(status) +> = 'A'` leaves it out, because `NOT NULL` is NULL, not TRUE). Before 0.23.0 the emitted script did +> not compile at all and Elasticsearch rejected the whole query (`script_exception: compile error`, +> caused by `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]`), so no such +> predicate ever ran. +> +> A predicate with **no** function is not scripted — it becomes a term/range query — and `NOT` over +> it is Elasticsearch's `must_not`, which **does** return documents that lack the field. The two +> routes therefore differ for absent fields; use `IS NULL` / `IS NOT NULL` when that distinction +> matters. + **Example** ```sql 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 4c2ee99e3..c67885515 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 @@ -1652,6 +1652,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defe", "def e") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -1705,6 +1707,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defe", "def e") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -2194,6 +2198,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defv", " def v") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -2363,6 +2369,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defp", "def p") .replaceAll("defv", " def v") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") .replaceAll("\\?", " ? ") @@ -2659,7 +2667,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "script": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().get(ChronoField.YEAR); (param1 == null) ? null : (param1 * (param2 - 10)) > 10000", + | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def param2 = ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).toLocalDate().get(ChronoField.YEAR); def left1 = (param1 == null) ? null : (param1 * (param2 - 10)); (left1 == null ? false : ((left1 > 10000)))", | "params": { | "__now__": 1767139200000 | } @@ -2721,6 +2729,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("defr", "def r") + .replaceAll("false:", "false : ") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") // .replaceAll("(\\d)=", "$1 =") @@ -2755,7 +2764,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "script": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); (param1 == null) ? null : Double.valueOf(Math.sqrt(param1)) > 100.0" + | "source": "def param1 = (doc['identifier'].size() == 0 ? null : doc['identifier'].value); def left1 = (param1 == null) ? null : Double.valueOf(Math.sqrt(param1)); (left1 == null ? false : ((left1 > 100.0)))" | } | } | } @@ -2884,6 +2893,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defp", "def p") .replaceAll("if\\(", "if (") .replaceAll("=\\(", " = (") @@ -2928,7 +2939,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "script": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); (param1 == null) ? null : param1.trim().length() > 10" + | "source": "def param1 = (doc['identifier2'].size() == 0 ? null : doc['identifier2'].value); def left1 = (param1 == null) ? null : param1.trim().length(); (left1 == null ? false : ((left1 > 10)))" | } | } | } @@ -3033,6 +3044,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3165,6 +3178,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3237,6 +3252,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3377,6 +3394,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3499,6 +3518,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defv", " def v") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3590,6 +3611,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3703,6 +3726,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3818,6 +3843,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") @@ -3922,6 +3949,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replace("\\\"/\\\",\\\"-\\\"", "\\\"/\\\", \\\"-\\\"") .replaceAll("defp", "def p") .replaceAll("defa", "def a") + .replaceAll("defleft", "def left") + .replaceAll("false:", "false : ") .replaceAll("defe", "def e") .replaceAll("defl", "def l") .replaceAll("def_", "def _") 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 dbca470dc..b818758c7 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -468,6 +468,25 @@ package object sql { } } + // Unique local-variable names for a script (story BIDC-8, AD-9). A guarded boolean binds its + // operand to a local so the comparison lands INSIDE the null guard; two such operands in one + // script must not declare the same name. + private[this] var _locals: collection.mutable.Seq[(String, String)] = + collection.mutable.Seq.empty + + /** Bind `expr` to a fresh local declared in this script's PROLOGUE and return its name. + * + * A Painless `def x = …;` is a STATEMENT: it cannot sit inside a parenthesised operand, so a + * guarded boolean cannot declare its own local inline once predicates are composed (story + * BIDC-8, AD-9). Declaring it beside the `param` assignments keeps every operand a pure + * expression and evaluates it once. + */ + def bindLocal(expr: String, prefix: String = "left"): String = { + val name = s"$prefix${_locals.size + 1}" + _locals = _locals :+ (name -> expr) + name + } + def exists(token: Token): Boolean = { token match { case param: PainlessParam => _keys.contains(param) @@ -476,7 +495,7 @@ package object sql { } } - def isEmpty: Boolean = _keys.isEmpty + def isEmpty: Boolean = _keys.isEmpty && _locals.isEmpty def nonEmpty: Boolean = _keys.nonEmpty @@ -495,16 +514,19 @@ package object sql { s"${param.param}${param.painlessMethods.mkString("")}" override def toString: String = { - if (isEmpty) "" - else - _keys - .flatMap { param => - get(param) match { - case Some(v) => Some(s"def $v = ${paramValue(param)}; ") - case None => None // should not happen + val params = + if (isEmpty) "" + else + _keys + .flatMap { param => + get(param) match { + case Some(v) => Some(s"def $v = ${paramValue(param)}; ") + case None => None // should not happen + } } - } - .mkString("") + .mkString("") + // Locals bound by guarded booleans (AD-9) follow the params they read. + params + _locals.map { case (name, expr) => s"def $name = $expr; " }.mkString("") } } 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 fca076ab3..f9f14e15b 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 @@ -206,8 +206,15 @@ sealed trait Criteria extends Updateable with PainlessScript { override def painless(context: Option[PainlessContext]): String = this match { case Predicate(left, op, right, maybeNot, group) => - val leftStr = left.painless(context) - val rightStr = right.painless(context) + // 🔴 Story BIDC-8 AD-9 — each operand is PARENTHESISED. An operand that carries a null guard + // renders as a ternary, and `?:` has the LOWEST precedence in Painless, so the unparenthesised + // composition `a == null ? false : (x) && b == null ? false : (y)` parses as + // `a == null ? false : (((x) && b == null) ? false : (y))` — the second predicate silently + // became part of the first one's condition. MEASURED at the baseline on `WHERE UPPER(status) = + // 'A' AND id = 1` and on the all-bare-column twin; a filter cannot be composed from ternaries + // without parentheses. + val leftStr = s"(${left.painless(context)})" + val rightStr = s"(${right.painless(context)})" val opStr = op match { case AND | OR => op.painless(context) case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") @@ -511,7 +518,18 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { */ protected def check(context: Option[PainlessContext], param: String, value: String): String = { operator match { - case comparison: ComparisonOperator => + case _: ComparisonOperator => + // 🔴 Story BIDC-8 AD-9 — dispatch on the EFFECTIVE operator, i.e. with a `NOT` folded in, + // exactly as `painlessOp` does for the generic fallback below. Each arm here returns a + // HARD-CODED spelling (`compareTo(...) == 0`, `isEqual(...)`, …) chosen from the RAW + // operator, so `WHERE NOT status = 'A'` and `WHERE NOT UPPER(status) = 'A'` both emitted + // the same Painless as their un-negated twins and the NOT was silently lost — a wrong + // answer, not an error (MEASURED at the baseline: `maybeNot = Some(NOT)` on the AST, + // `compareTo("A") == 0` in the script). + val comparison = operator match { + case o: ComparisonOperator if maybeNot.isDefined => o.not + case o => o + } comparison match { case LT => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { @@ -638,6 +656,42 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } case _ => } + // 🔴 Story BIDC-8 AD-9 — a left-hand side that can be NULL must be bound to a local so the + // comparison lands INSIDE the guard, and the guard must collapse to `false` exactly once, at the + // top of THIS predicate. + // + // MEASURED at the baseline: `WHERE UPPER(status) <> 'ZZZ'` emitted + // `(param1 == null) ? null : param1.toUpperCase().compareTo("ZZZ") != 0` — the function's own + // null guard with the comparison appended OUTSIDE it. `?:` binds loosest, so the ternary's + // branches became `null` (Object) and a primitive `boolean`, and Elasticsearch refused the whole + // query at COMPILE time: `script_exception / compile error`, caused by + // `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]` (live ES 8.18.3, + // data-independent — it fails on an empty index). The same held for the numeric family + // (`ABS(amount) > 10`); the date family escaped it only because `YEAR(...)` folds into the + // parameter assignment and reports `nullable`. + // + // The `false` collapse is ANSI-safe here because NOT is already folded into the operator at this + // point (`painlessOp` uses `o.not`), so `NOT UPPER(x) = 'A'` over a MISSING field yields `false` + // = no match, which is what three-valued logic requires (`NOT NULL` is NULL, not TRUE). It is + // applied per predicate, never to a composite: `NULL OR true` stays true and `NULL AND true` + // stays false under `||`/`&&` composition. Only a QUERY (filter) context collapses — script + // fields, sorts, aggregations and ingest processors keep `null`, where it is a legitimate value + // (story 21.8 Part C). + // What must be guarded is a rendering that READS A DOCUMENT FIELD, since that is what can be + // missing. MEASURED (probe over the real parser): a bare column reports `name = status`, + // `nullable = true`, no dependencies; a function-wrapped column reports an EMPTY name, + // `nullable = false` and `dependencies = [status]`; a literal (`CASE WHEN 1 = 1`, which parses + // to an identifier with an empty name and a ONE-element function chain — story 20.9) reports + // neither. So `nullable || dependencies.nonEmpty` is the discriminator: a `functions.nonEmpty` + // test alone guards constants and broke `BooleanCastSpec`'s 21.8 restoration pin. + val guardInQuery = context.exists(_.context == PainlessContextType.Query) + val readsDocumentField = identifier.nullable || identifier.dependencies.nonEmpty + if (guardInQuery && readsDocumentField) { + // The local is declared in the script PROLOGUE, never inline: `def x = …;` is a STATEMENT and + // an operand of a composed predicate must be a pure expression. + val v = context.map(_.bindLocal(innerLeft)).getOrElse(innerLeft) + return s"($v == null ? false : ($painlessNot(${check(context, v)})))" + } if (identifier.nullable) { return s"def left = $innerLeft; left == null ? false : $painlessNot(${check(context, "left")})" } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index 4501c95d3..2debb5de0 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -2418,6 +2418,90 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { // 8. POLICIES — CREATE / DROP / EXECUTE // =========================================================================== + behavior of "a WHERE predicate that must be scripted (story BIDC-8, AD-9)" + + /** 🔴 Row-set pins for the Painless filter emission, on a REAL cluster. + * + * Before BIDC-8 a function-wrapped predicate emitted `(param == null) ? null : `, whose + * ternary branches are `null` (Object) and a primitive `boolean`: Elasticsearch refused the + * whole query at COMPILE time (`script_exception / compile error`, caused by + * `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]`) — data-independent, + * it failed even against an empty index. Two more defects sat on the same path: a composed + * predicate was not parenthesised, so `?:` (the loosest operator in Painless) swallowed the + * sibling — measured `WHERE status = 'A' OR id = 1` returning NO rows where one matches — and + * `check` dispatched on the RAW operator, so a NOT was silently dropped (`WHERE NOT status = + * 'A'` returned exactly the rows that DO equal 'A'). + * + * The assertions below are ROW SETS, never script bytes, and every one of them is what ANSI + * three-valued logic requires of a MISSING field: a predicate over NULL is NULL, so the row does + * not match — and `NOT` over it stays NULL, so it does not match either. + */ + it should "apply ANSI three-valued logic to a scripted WHERE, including NOT and composites" in { + val create = + """CREATE TABLE IF NOT EXISTS tvl_orders ( + | id INT, + | status KEYWORD, + | amount DOUBLE, + | PRIMARY KEY (id) + |)""".stripMargin + assertDdl(System.nanoTime(), client.run(create).futureValue) + // id 1: status = 'A'; id 2: status = 'B'; id 3: the `status` FIELD IS ABSENT from the document. + // 🔴 Seeded through COPY INTO, not INSERT: `INSERT INTO tvl_orders (id, amount)` writes the + // omitted column as an EMPTY STRING (measured — row 3 then matched `NOT UPPER(status) = 'A'` + // legitimately), so it cannot express "this document never carried the field", which is the + // only state that exercises the null guard. + val tvlJsonl = java.io.File.createTempFile("tvl_orders_", ".jsonl") + tvlJsonl.deleteOnExit() + val tvlWriter = new java.io.PrintWriter(tvlJsonl) + try { + tvlWriter.println("""{"id": 1, "status": "A", "amount": 5.0}""") + tvlWriter.println("""{"id": 2, "status": "B", "amount": 50.0}""") + tvlWriter.println("""{"id": 3, "amount": 50.0}""") + } finally tvlWriter.close() + assertDml( + System.nanoTime(), + client.run(s"""COPY INTO tvl_orders FROM "${tvlJsonl.getAbsolutePath}";""").futureValue + ) + // Fixture guard: the premise of every assertion below is that row 3 has NO `status` field. + collectRows( + System.nanoTime(), + client.run("SELECT id FROM tvl_orders WHERE status IS NOT NULL").futureValue + ).flatMap(_.get("id").map(_.toString.toDouble.toInt)).sorted shouldBe Seq(1, 2) + + // `collectRows`, not `assertQueryRows`: an un-LIMITed row query comes back as a QueryStream on + // the licensed gateway since #209, and `assertQueryRows` accepts `QueryRows` only. + def ids(where: String): Seq[Int] = + collectRows( + System.nanoTime(), + client.run(s"SELECT id FROM tvl_orders $where").futureValue + ).flatMap(_.get("id").map(_.toString.toDouble.toInt)).sorted + + // A function over a present field matches only the row it names; the MISSING row never matches. + ids("WHERE UPPER(status) = 'A'") shouldBe Seq(1) + // NOT over a MISSING field is NULL, not TRUE: row 3 must NOT appear. + ids("WHERE NOT UPPER(status) = 'A'") shouldBe Seq(2) + ids("WHERE UPPER(status) <> 'A'") shouldBe Seq(2) + // Composition: the guard of one operand must not swallow the other. + ids("WHERE UPPER(status) = 'A' OR id = 1") shouldBe Seq(1) + ids("WHERE UPPER(status) = 'A' AND id = 1") shouldBe Seq(1) + ids("WHERE NOT UPPER(status) = 'A' AND id = 1") shouldBe Seq.empty[Int] + ids("WHERE NOT UPPER(status) = 'A' OR id = 1") shouldBe Seq(1, 2) + // 🔴 With NO function the engine does not script at all, and the contrast is worth pinning: + // a bare column becomes a TERM query, so `OR` is a `bool.should` of two clauses … + ids("WHERE status = 'A' OR id = 1") shouldBe Seq(1) + // … and `NOT` becomes Elasticsearch's `must_not`, whose semantics INCLUDE a document that does + // not carry the field — row 3 matches. That diverges from ANSI (`NOT NULL` is NULL), it is + // PRE-EXISTING, structural, and BIDC-8 does not change it: this story fixes what the Painless + // FILTER emits, and a bare-column predicate never reaches Painless here. Pinned so the + // difference between the two routes is visible rather than discovered. + ids("WHERE NOT status = 'A'") shouldBe Seq(2, 3) + // A numeric function family, so the rule is not string-specific (`amount` is present on all). + ids("WHERE ABS(amount) > 10") shouldBe Seq(2, 3) + ids("WHERE LOWER(status) = 'a' AND ABS(amount) > 10") shouldBe Seq.empty[Int] + + assertDdl(System.nanoTime(), client.run("DROP TABLE IF EXISTS tvl_orders").futureValue) + } + behavior of "POLICIES statements" it should "create, show, execute and drop a policy" in { From 2848d1426eccf2be6e80bf4ce49493a3a28e0929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 08:28:32 +0200 Subject: [PATCH 07/10] =?UTF-8?q?fix(BIDC-8):=20finish=20AD-9=20=E2=80=94?= =?UTF-8?q?=20one=20guarded,=20total=20Painless=20emission=20for=20every?= =?UTF-8?q?=20WHERE=20comparison?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 of the independent AD-9 audit. Every item was measured on live Elasticsearch 8.18.3 over four documents (status A / status B / status ABSENT / status C) BEFORE the fix and again after, and every fix carries a gate that fails without it. B-2, M-5 — a NOT written after AND / OR used to give DIFFERENT ROWS depending on writing order. `WhereParser.predicate` is `criteria ~ (and|or) ~ not.? ~ criteria`, so the NOT lands on the PREDICATE, and the predicate emitted it two ways: `asFilter` wrapped the un-negated right criterion in a `must_not` (which MATCHES a document lacking the field) while `painless` rendered `!(left && right)` (which negates the whole composite). Measured: `NOT UPPER(status)='A' AND amount>10` -> [2] versus `amount>10 AND NOT UPPER(status)='A'` -> [2,3]. `Criteria.negated` now folds the NOT into the criterion it qualifies, so both paths agree and `check` folds it into the operator. Same fold inside a CASE (audit case C2): the absent row went to THEN where ANSI takes ELSE. M-6 — `ComparisonOperator.not` covered six of thirteen operators, so `UPPER(status) NOT LIKE 'A%'` died with a bare `scala.MatchError: LIKE` escaping the query builder. It is now `maybeNegated: Option[ComparisonOperator]`, exhaustive over the sealed set (so a new operator cannot be added without answering), and the three callers that cannot represent a missing negation decline instead of crashing: `painlessNot` renders the `!` INSIDE the null guard, the geo shortcut falls through, and the date-math range falls back to a script. L-10 — `LIKE` over a function emitted `left1 .matches "A%"`, which is not Painless. Two repaired spellings were measured and REFUTED before the shipped one: `String.matches(String)` is not whitelisted, and neither is `Pattern.compile` (which also condemns REGEXP_LIKE's emission — pre-existing, untouched, reported). A `%`-only pattern now decomposes into whitelisted String methods; only a pattern that needs a real regex uses a `/…/` literal. That is also the only form that works on ES 6.8, where script regexes are disabled by default. L-8 — `IN` and `BETWEEN` over a function fell through to `termsQuery`/`rangeQuery` keyed on `identifier.name`, which is the EMPTY STRING for a function wrapper: `{"terms":{"":[…]}}` rejected as `[bool] failed to parse field [filter]`. The "this operand needs a script" test is now one named predicate used by all three conversions, and both criteria render through the SHARED guarded path by overriding `check` rather than `painless` — which also repaired `[…].contains` being applied to the element instead of the list, a chained `a <= x <= b` Painless rejects, and a `'A'` bound rendered in SQL quotes inside a script. H-3 — the audit asked for a context discriminator; measurement says the discriminator is already structural, and the story is better without one. An `Expression` IS a boolean: all four consumers of a criteria rendering (script query, predicate composition, CASE condition, HAVING bucket_selector — enumerated in `PainlessNullSurvivalSpec`) are boolean positions, so the `false` collapse is right in every one. A projected value goes through `Identifier.painless`, a different renderer, which still emits `? null :` — pinned for script_fields, script sort and terms script. L-11 — the statement form `def left = …; ` is legal only with no context; spliced into a predicate or a CASE it is a compile-time 400. The two latent sites now hoist through `bindLocal` when there is a context, the invariant is named at each site, and `PainlessOperandFormSpec` gates it: inlining `bindLocal` reddens it on 14 shapes. Also: `atomic` now asks the honest question (is there an operator at paren depth 0), which keeps a literal and a method chain out of a needless local — two pins measure both sides of that line. Green: sql 1061, core 979, bridge 202 x 4 majors (es6/es7/es8/es9), `+ sql/compile`, `++ 2.12.20` for sql and core test sources, scalafmt, headerCheck. On REAL Elasticsearch the extended `GatewayApiIntegrationSpec` ANSI test passes on 6.8.23, 7.17.29, 8.18.3 and 9.0.3, and the full es8 GatewayApi suite is 75/75. Co-Authored-By: Claude Opus 5 --- .../elastic/sql/bridge/package.scala | 61 ++- .../elastic/sql/AggregationNamingSpec.scala | 6 +- .../sql/PainlessNullSurvivalSpec.scala | 95 ++++ documentation/sql/dql_statements.md | 17 + .../elastic/sql/bridge/package.scala | 65 ++- .../elastic/sql/AggregationNamingSpec.scala | 6 +- .../sql/PainlessNullSurvivalSpec.scala | 95 ++++ .../elastic/sql/operator/package.scala | 31 +- .../app/softnetwork/elastic/sql/package.scala | 50 +- .../elastic/sql/parser/Parser.scala | 54 ++- .../softnetwork/elastic/sql/query/Where.scala | 430 +++++++++++++----- .../sql/query/PainlessOperandFormSpec.scala | 152 +++++++ .../client/GatewayApiIntegrationSpec.scala | 67 +++ 13 files changed, 934 insertions(+), 195 deletions(-) create mode 100644 bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala create mode 100644 es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.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 b720d45c2..f8d5c38ac 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 @@ -654,6 +654,34 @@ package object bridge { ) } + /** True when the ES query DSL cannot address this operand by field NAME -- it is a computed + * value, so the predicate has to run as a script. + * + * 🔴 Story BIDC-8 (review L-8). This condition was inline in `expressionToQuery` only, so `IN` + * and `BETWEEN` over a function-wrapped operand fell through to `termsQuery` / `rangeQuery` + * keyed on `identifier.name` -- which is the EMPTY STRING for a function wrapper. MEASURED on + * live ES 8.18.3: `WHERE UPPER(status) IN ('A','B')` emitted `{"terms":{"":["A","B"]}}` and + * `WHERE ABS(amount) BETWEEN 1 AND 100` emitted `{"range":{"":{...}}}`, both rejected with the + * opaque `x_content_parse_exception: [bool] failed to parse field [filter]`. `Distance` is + * excluded because the geo-distance query DOES address it natively. + */ + private[bridge] def requiresScript(identifier: Identifier): Boolean = + identifier.functions.nonEmpty && (identifier.functions.size > 1 || (identifier.functions.head match { + case _: Distance => false + case _ => true + })) + + private[bridge] def scriptQueryOf(criteria: Criteria)(implicit + timestamp: Long, + contextType: PainlessContextType + ): Query = { + val context = PainlessContext(context = contextType) + val script = criteria.painless(Some(context)) + scriptQuery( + now(Script(script = s"$context$script").lang("painless").scriptType("source")) + ) + } + def applyNumericOp[A](n: NumericValue[_])( longOp: Long => A, doubleOp: Double => A @@ -666,18 +694,7 @@ package object bridge { import expression._ if (isAggregation) return matchAllQuery() - if ( - identifier.functions.nonEmpty && (identifier.functions.size > 1 || (identifier.functions.head match { - case _: Distance => false - case _ => true - })) - ) { - val context = PainlessContext(context = contextType) - val script = painless(Some(context)) - return scriptQuery( - now(Script(script = s"$context$script").lang("painless").scriptType("source")) - ) - } + if (requiresScript(identifier)) return scriptQueryOf(expression) // Geo distance special case identifier.functions.headOption match { case Some(d: Distance) => @@ -692,11 +709,14 @@ package object bridge { }) match { case Some(g) => maybeNot match { - case Some(_) => + // `maybeNegated` is total (story BIDC-8, review M-6): a comparison with no + // negated spelling declines the geo shortcut and falls through to the generic + // path rather than dying with a `MatchError`. + case Some(_) if o.maybeNegated.isDefined => return geoDistanceToQuery( DistanceCriteria( d, - o.not, + o.maybeNegated.get, g ) ) @@ -880,7 +900,10 @@ package object bridge { case op: ComparisonOperator => i.script match { case Some(script) => - val o = if (maybeNot.isDefined) op.not else op + // `maybeNegated` is total (story BIDC-8, review M-6). A comparison the range + // query cannot express -- one with no negated spelling, or any operator outside + // the six below -- runs as a script instead of reaching a `MatchError`. + val o = if (maybeNot.isDefined) op.maybeNegated.getOrElse(op) else op o match { case GT => rangeQuery(identifier.name) gt script case GE => rangeQuery(identifier.name) gte script @@ -888,6 +911,7 @@ package object bridge { case LE => rangeQuery(identifier.name) lte script case EQ => rangeQuery(identifier.name) gte script lte script case NE | DIFF => not(rangeQuery(identifier.name) gte script lte script) + case _ => scriptQueryOf(expression) } case _ => val context = PainlessContext(context = contextType) @@ -943,7 +967,11 @@ package object bridge { existsQuery(identifier.name) } - implicit def inToQuery[R, T <: Value[R]](in: InExpr[R, T]): Query = { + implicit def inToQuery[R, T <: Value[R]](in: InExpr[R, T])(implicit + timestamp: Long, + contextType: PainlessContextType = PainlessContextType.Query + ): Query = { + if (requiresScript(in.identifier)) return scriptQueryOf(in) import in._ val _values: Seq[Any] = values.innerValues val t = @@ -969,6 +997,7 @@ package object bridge { contextType: PainlessContextType = PainlessContextType.Query ): Query = { import between._ + if (requiresScript(identifier)) return scriptQueryOf(between) // Geo distance special case identifier.functions.headOption match { case Some(d: Distance) => diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index 921d832f4..24f2eb96c 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -245,7 +245,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", - """"script":{"source":"(params.count_x == null ? false : (!(params.count_x >= 1 && params.count_x <= 5)))"}}}}}}}""" + """"script":{"source":"(params.count_x == null ? false : !(params.count_x >= 1 && params.count_x <= 5))"}}}}}}}""" ).mkString } @@ -265,7 +265,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (!(params.max_x == 1 || params.max_x == 2)))"}}}}}}}""" + """"script":{"source":"(params.max_x == null ? false : !(params.max_x == 1 || params.max_x == 2))"}}}}}}}""" ).mkString } @@ -290,7 +290,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", - """"script":{"source":"(params.count_x == null ? false : (!(params.count_x == 1 || params.count_x == 2)))"}}}}}}}""" + """"script":{"source":"(params.count_x == null ? false : !(params.count_x == 1 || params.count_x == 2))"}}}}}}}""" ).mkString } diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala new file mode 100644 index 000000000..189c9ba2d --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -0,0 +1,95 @@ +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 org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime + +/** Story BIDC-8, AD-9 (review H-3) — WHERE it is right to collapse an absent field to `false`, and + * WHERE `null` must survive. + * + * The audit asked for a "context discriminator" that would keep the `false` collapse to filter + * scripts only. MEASURED, that discriminator is not needed, because it already exists + * STRUCTURALLY: the collapse lives in `Expression.painless` / `Criteria.painless`, and an + * `Expression` IS a boolean — it is consumed as a condition, never projected as a value. A + * projected value goes through a different renderer entirely, `Identifier.painless`, which still + * emits `(paramN == null) ? null : …`. + * + * POSITIVE PROOF OF TOTALITY — every consumer of a `Criteria` rendering in this repository, + * enumerated (`grep -rn "criteria.painless\|\.painless(Some(" sql/src/main core/src/main + * bridge/src/main`), and what each does with it: + * + * 1. `bridge/package.scala:680` `scriptQueryOf` -> `scriptQuery` — Elasticsearch requires a + * BOOLEAN; a `null` is a runtime `class_cast_exception`. + * 1. `Criteria.painless`'s own `Predicate` arm (`Where.scala:236-250`) — joins two renderings + * with `&&` / `||`. Painless refuses `null && x`. + * 1. `function/cond/package.scala:360` — a `CASE` condition, coerced to `SQLTypes.Boolean` and + * spliced as `$c ? $r`. A `null` there does not even compile. + * 1. `Expression.bucketPipelinePainless` (HAVING `bucket_selector`) — Elasticsearch requires a + * boolean. + * + * There is no fifth. Every one is a boolean position, so the collapse is right in all of them, and + * a context flag would be a parameter with one legal value. + * + * This spec pins the OTHER half — the three value-projecting renderers, which must keep `null` — + * so a future "simplification" that routes them through the criteria renderer reddens here rather + * than silently turning every absent field into `false` in a projection. + */ +class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { + + import scala.language.implicitConversions + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + private def bodyOf(sql: String): String = + Parser(sql) match { + case Right(ss: SingleSearch) => requestToElasticSearchRequest(ss).query + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + "a projected function (script_fields)" should "keep null for an absent field" in { + val body = bodyOf("SELECT id, UPPER(status) AS u FROM probe") + body should include("script_fields") + body should include("? null :") + body should not include "? false :" + } + + "a sort key (script sort)" should "keep null for an absent field" in { + val body = bodyOf("SELECT id FROM probe ORDER BY UPPER(status)") + body should include("_script") + body should include("? null :") + body should not include "? false :" + } + + "a grouping key (terms script)" should "keep null for an absent field" in { + val body = bodyOf("SELECT UPPER(status) AS u, COUNT(*) AS n FROM probe GROUP BY UPPER(status)") + body should include("? null :") + body should not include "? false :" + } + + "a WHERE predicate (filter script)" should "collapse an absent field to false, not null" in { + val body = bodyOf("SELECT id FROM probe WHERE UPPER(status) = 'A'") + body should include("? false :") + // The PARAMETER binding still carries its own `? null :` — that is the operand's rendering. + // What must not survive is a `null` in the CONDITION, i.e. as the last thing the script yields. + body should include("""left1 == null ? false : ((left1.compareTo(\"A\") == 0))""") + } + + /** The B-1 regression, byte-pinned in both bridge copies. A `CASE` over a function used to emit + * `cannot resolve symbol [left1]`: the condition's local was bound on a THROWAWAY context while + * the prologue came from another, so the script referenced a name it never declared. Executed on + * live Elasticsearch 8.18.3 over docs A / B / field-absent / A, this returns 1 / 0 / 0 / 1. + */ + "a CASE over a function" should "declare every local it references" in { + val body = bodyOf("SELECT id, CASE WHEN UPPER(status) = 'A' THEN 1 ELSE 0 END AS c FROM probe") + val declared = + """def (left\d+|param\d+|right\d+)""".r.findAllMatchIn(body).map(_.group(1)).toSet + val referenced = """\b(left\d+|right\d+)\b""".r.findAllIn(body).toSet + withClue(s"declared=$declared referenced=$referenced in\n$body\n") { + (referenced -- declared) shouldBe empty + } + } +} diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 2c5d3f071..23e1c98f8 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -420,10 +420,27 @@ The `WHERE` clause supports: > caused by `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]`), so no such > predicate ever ran. > +> This holds for **every** comparison the clause accepts, not only `=`: `<`, `>`, `<>`, `LIKE`, +> `NOT LIKE`, `IN`, `NOT IN`, `BETWEEN` and `NOT BETWEEN` over a function all follow the same rule, +> and a `NOT` written after `AND` / `OR` (`WHERE ABS(amount) > 10 AND NOT UPPER(status) = 'A'`) +> negates the criterion it qualifies, not the whole composite — so the same predicate returns the +> same rows whichever way round you write it. Before **0.23.0** several of these did not run at all: +> `LIKE` over a function produced an uncompilable script, `NOT LIKE` failed inside the engine, and +> `IN` / `BETWEEN` over a function were sent to Elasticsearch with an empty field name and rejected. +> > A predicate with **no** function is not scripted — it becomes a term/range query — and `NOT` over > it is Elasticsearch's `must_not`, which **does** return documents that lack the field. The two > routes therefore differ for absent fields; use `IS NULL` / `IS NOT NULL` when that distinction > matters. +> +> A **projected** function keeps its `NULL`: `SELECT UPPER(status) AS u` returns `u = NULL` for a +> document with no `status`, and a `GROUP BY UPPER(status)` has no bucket for it. The collapse to +> "no match" applies to a **condition**, never to a value. +> +> ⚠️ `LIKE` / `NOT LIKE` with `%` and `_` in the same pattern (`'A_%'`) compiles to a Painless +> regular expression, and Elasticsearch **6.8** disables regular expressions in scripts by default +> (`script.painless.regex.enabled`). Patterns using only `%` — which is what a BI tool emits — need +> no regex and work on every supported version. **Example** diff --git a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala index 050100a88..d2726986c 100644 --- a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala +++ b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/package.scala @@ -647,6 +647,34 @@ package object bridge { ) } + /** True when the ES query DSL cannot address this operand by field NAME -- it is a computed + * value, so the predicate has to run as a script. + * + * 🔴 Story BIDC-8 (review L-8). This condition was inline in `expressionToQuery` only, so `IN` + * and `BETWEEN` over a function-wrapped operand fell through to `termsQuery` / `rangeQuery` + * keyed on `identifier.name` -- which is the EMPTY STRING for a function wrapper. MEASURED on + * live ES 8.18.3: `WHERE UPPER(status) IN ('A','B')` emitted `{"terms":{"":["A","B"]}}` and + * `WHERE ABS(amount) BETWEEN 1 AND 100` emitted `{"range":{"":{...}}}`, both rejected with the + * opaque `x_content_parse_exception: [bool] failed to parse field [filter]`. `Distance` is + * excluded because the geo-distance query DOES address it natively. + */ + private[bridge] def requiresScript(identifier: Identifier): Boolean = + identifier.functions.nonEmpty && (identifier.functions.size > 1 || (identifier.functions.head match { + case _: Distance => false + case _ => true + })) + + private[bridge] def scriptQueryOf(criteria: Criteria)(implicit + timestamp: Long, + contextType: PainlessContextType + ): Query = { + val context = PainlessContext(context = contextType) + val script = criteria.painless(Some(context)) + scriptQuery( + now(Script(script = s"$context$script").lang("painless").scriptType("source")) + ) + } + def applyNumericOp[A](n: NumericValue[_])( longOp: Long => A, doubleOp: Double => A @@ -659,22 +687,7 @@ package object bridge { import expression._ if (isAggregation) return matchAllQuery() - if ( - identifier.functions.nonEmpty && (identifier.functions.size > 1 || (identifier.functions.head match { - case _: Distance => false - case _ => true - })) - ) { - val context = PainlessContext(context = contextType) - val script = painless(Some(context)) - return scriptQuery( - now( - Script(script = s"$context$script") - .lang("painless") - .scriptType("source") - ) - ) - } + if (requiresScript(identifier)) return scriptQueryOf(expression) // Geo distance special case identifier.functions.headOption match { case Some(d: Distance) => @@ -689,11 +702,14 @@ package object bridge { }) match { case Some(g) => maybeNot match { - case Some(_) => + // `maybeNegated` is total (story BIDC-8, review M-6): a comparison with no + // negated spelling declines the geo shortcut and falls through to the generic + // path rather than dying with a `MatchError`. + case Some(_) if o.maybeNegated.isDefined => return geoDistanceToQuery( DistanceCriteria( d, - o.not, + o.maybeNegated.get, g ) ) @@ -877,7 +893,10 @@ package object bridge { case op: ComparisonOperator => i.script match { case Some(script) => - val o = if (maybeNot.isDefined) op.not else op + // `maybeNegated` is total (story BIDC-8, review M-6). A comparison the range + // query cannot express -- one with no negated spelling, or any operator outside + // the six below -- runs as a script instead of reaching a `MatchError`. + val o = if (maybeNot.isDefined) op.maybeNegated.getOrElse(op) else op o match { case GT => rangeQuery(identifier.name) gt script case GE => rangeQuery(identifier.name) gte script @@ -885,6 +904,7 @@ package object bridge { case LE => rangeQuery(identifier.name) lte script case EQ => rangeQuery(identifier.name) gte script lte script case NE | DIFF => not(rangeQuery(identifier.name) gte script lte script) + case _ => scriptQueryOf(expression) } case _ => val context = PainlessContext(context = contextType) @@ -940,7 +960,11 @@ package object bridge { existsQuery(identifier.name) } - implicit def inToQuery[R, T <: Value[R]](in: InExpr[R, T]): Query = { + implicit def inToQuery[R, T <: Value[R]](in: InExpr[R, T])(implicit + timestamp: Long, + contextType: PainlessContextType = PainlessContextType.Query + ): Query = { + if (requiresScript(in.identifier)) return scriptQueryOf(in) import in._ val _values: Seq[Any] = values.innerValues val t = @@ -966,6 +990,7 @@ package object bridge { contextType: PainlessContextType = PainlessContextType.Query ): Query = { import between._ + if (requiresScript(identifier)) return scriptQueryOf(between) // Geo distance special case identifier.functions.headOption match { case Some(d: Distance) => diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index 63acdbc5e..576fb2c5f 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -245,7 +245,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", - """"script":{"source":"(params.count_x == null ? false : (!(params.count_x >= 1 && params.count_x <= 5)))"}}}}}}}""" + """"script":{"source":"(params.count_x == null ? false : !(params.count_x >= 1 && params.count_x <= 5))"}}}}}}}""" ).mkString } @@ -265,7 +265,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (!(params.max_x == 1 || params.max_x == 2)))"}}}}}}}""" + """"script":{"source":"(params.max_x == null ? false : !(params.max_x == 1 || params.max_x == 2))"}}}}}}}""" ).mkString } @@ -290,7 +290,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", - """"script":{"source":"(params.count_x == null ? false : (!(params.count_x == 1 || params.count_x == 2)))"}}}}}}}""" + """"script":{"source":"(params.count_x == null ? false : !(params.count_x == 1 || params.count_x == 2))"}}}}}}}""" ).mkString } diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala new file mode 100644 index 000000000..189c9ba2d --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -0,0 +1,95 @@ +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 org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime + +/** Story BIDC-8, AD-9 (review H-3) — WHERE it is right to collapse an absent field to `false`, and + * WHERE `null` must survive. + * + * The audit asked for a "context discriminator" that would keep the `false` collapse to filter + * scripts only. MEASURED, that discriminator is not needed, because it already exists + * STRUCTURALLY: the collapse lives in `Expression.painless` / `Criteria.painless`, and an + * `Expression` IS a boolean — it is consumed as a condition, never projected as a value. A + * projected value goes through a different renderer entirely, `Identifier.painless`, which still + * emits `(paramN == null) ? null : …`. + * + * POSITIVE PROOF OF TOTALITY — every consumer of a `Criteria` rendering in this repository, + * enumerated (`grep -rn "criteria.painless\|\.painless(Some(" sql/src/main core/src/main + * bridge/src/main`), and what each does with it: + * + * 1. `bridge/package.scala:680` `scriptQueryOf` -> `scriptQuery` — Elasticsearch requires a + * BOOLEAN; a `null` is a runtime `class_cast_exception`. + * 1. `Criteria.painless`'s own `Predicate` arm (`Where.scala:236-250`) — joins two renderings + * with `&&` / `||`. Painless refuses `null && x`. + * 1. `function/cond/package.scala:360` — a `CASE` condition, coerced to `SQLTypes.Boolean` and + * spliced as `$c ? $r`. A `null` there does not even compile. + * 1. `Expression.bucketPipelinePainless` (HAVING `bucket_selector`) — Elasticsearch requires a + * boolean. + * + * There is no fifth. Every one is a boolean position, so the collapse is right in all of them, and + * a context flag would be a parameter with one legal value. + * + * This spec pins the OTHER half — the three value-projecting renderers, which must keep `null` — + * so a future "simplification" that routes them through the criteria renderer reddens here rather + * than silently turning every absent field into `false` in a projection. + */ +class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { + + import scala.language.implicitConversions + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + private def bodyOf(sql: String): String = + Parser(sql) match { + case Right(ss: SingleSearch) => requestToElasticSearchRequest(ss).query + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + "a projected function (script_fields)" should "keep null for an absent field" in { + val body = bodyOf("SELECT id, UPPER(status) AS u FROM probe") + body should include("script_fields") + body should include("? null :") + body should not include "? false :" + } + + "a sort key (script sort)" should "keep null for an absent field" in { + val body = bodyOf("SELECT id FROM probe ORDER BY UPPER(status)") + body should include("_script") + body should include("? null :") + body should not include "? false :" + } + + "a grouping key (terms script)" should "keep null for an absent field" in { + val body = bodyOf("SELECT UPPER(status) AS u, COUNT(*) AS n FROM probe GROUP BY UPPER(status)") + body should include("? null :") + body should not include "? false :" + } + + "a WHERE predicate (filter script)" should "collapse an absent field to false, not null" in { + val body = bodyOf("SELECT id FROM probe WHERE UPPER(status) = 'A'") + body should include("? false :") + // The PARAMETER binding still carries its own `? null :` — that is the operand's rendering. + // What must not survive is a `null` in the CONDITION, i.e. as the last thing the script yields. + body should include("""left1 == null ? false : ((left1.compareTo(\"A\") == 0))""") + } + + /** The B-1 regression, byte-pinned in both bridge copies. A `CASE` over a function used to emit + * `cannot resolve symbol [left1]`: the condition's local was bound on a THROWAWAY context while + * the prologue came from another, so the script referenced a name it never declared. Executed on + * live Elasticsearch 8.18.3 over docs A / B / field-absent / A, this returns 1 / 0 / 0 / 1. + */ + "a CASE over a function" should "declare every local it references" in { + val body = bodyOf("SELECT id, CASE WHEN UPPER(status) = 'A' THEN 1 ELSE 0 END AS c FROM probe") + val declared = + """def (left\d+|param\d+|right\d+)""".r.findAllMatchIn(body).map(_.group(1)).toSet + val referenced = """\b(left\d+|right\d+)\b""".r.findAllIn(body).toSet + withClue(s"declared=$declared referenced=$referenced in\n$body\n") { + (referenced -- declared) shouldBe empty + } + } +} diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/operator/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/operator/package.scala index 5569800c8..43a6ac652 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/operator/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/operator/package.scala @@ -38,13 +38,30 @@ package object operator { trait ExpressionOperator extends Operator sealed trait ComparisonOperator extends ExpressionOperator with PainlessScript { - def not: ComparisonOperator = this match { - case EQ => NE - case NE | DIFF => EQ - case GE => LT - case GT => LE - case LE => GT - case LT => GE + + /** The operator that MEANS this one negated, when this operator set has a spelling for it. + * + * 🔴 Story BIDC-8 (review M-6). This was a total-looking `def not: ComparisonOperator` whose + * match covered only the six arithmetic comparisons, so every OTHER comparison reached it as a + * `scala.MatchError` escaping the query builder: MEASURED, `WHERE UPPER(status) NOT LIKE 'A%'` + * died with `scala.MatchError: LIKE (of class ...operator.package$LIKE$)` — an internal error + * where a user typed valid SQL. `IN`, `LIKE`, `RLIKE`, `BETWEEN` and `MATCH` have NO negated + * operator in this set; the honest answer is `None`, and the caller negates the whole check + * instead (`Expression.painlessNot` renders the `!` INSIDE the null guard, so ANSI + * three-valued logic is preserved: an absent field still collapses to `false` rather than + * being inverted into a match). The match is exhaustive over the sealed set, so a new + * comparison operator cannot be added without answering here. + */ + def maybeNegated: Option[ComparisonOperator] = this match { + case EQ => Some(NE) + case NE | DIFF => Some(EQ) + case GE => Some(LT) + case GT => Some(LE) + case LE => Some(GT) + case LT => Some(GE) + case IS_NULL => Some(IS_NOT_NULL) + case IS_NOT_NULL => Some(IS_NULL) + case IN | LIKE | RLIKE | BETWEEN | MATCH => None } } 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 b818758c7..05ee37b5a 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -444,6 +444,14 @@ package object sql { val paramName = s"param${_keys.size + 1}" _keys = _keys :+ param _values = _values :+ paramName + // 🔴 Declaration ORDER is creation ORDER (story BIDC-8, review B-1). A param can be + // created AFTER a local — `function/cond/package.scala` captures an already-rendered + // CASE condition into a new `LiteralParam` — and emitting all params before all + // locals then produced a FORWARD REFERENCE (`def param2 = (left1 …); def left1 = …`), + // which Elasticsearch refuses with `cannot resolve symbol [left1]` on EVERY index. + // There are 18 `addParam(LiteralParam(...))` sites, so the ordering must be + // structural, not a special case. + _declarations = _declarations :+ Left(param) _lastParam = Some(paramName) _lastParam } @@ -471,7 +479,13 @@ package object sql { // Unique local-variable names for a script (story BIDC-8, AD-9). A guarded boolean binds its // operand to a local so the comparison lands INSIDE the null guard; two such operands in one // script must not declare the same name. - private[this] var _locals: collection.mutable.Seq[(String, String)] = + private[this] var _locals: Int = 0 + + /** Every declaration this script emits, in CREATION order: a parameter (rendered from its + * `PainlessParam`) or a local bound by [[bindLocal]]. See the B-1 note in `addParam`. + */ + private[this] var _declarations + : collection.mutable.Seq[Either[PainlessParam, (String, String)]] = collection.mutable.Seq.empty /** Bind `expr` to a fresh local declared in this script's PROLOGUE and return its name. @@ -482,8 +496,9 @@ package object sql { * expression and evaluates it once. */ def bindLocal(expr: String, prefix: String = "left"): String = { - val name = s"$prefix${_locals.size + 1}" - _locals = _locals :+ (name -> expr) + _locals += 1 + val name = s"$prefix${_locals}" + _declarations = _declarations :+ Right(name -> expr) name } @@ -495,9 +510,10 @@ package object sql { } } - def isEmpty: Boolean = _keys.isEmpty && _locals.isEmpty + def isEmpty: Boolean = _declarations.isEmpty - def nonEmpty: Boolean = _keys.nonEmpty + // Review M-7: the complement of `isEmpty`, so a locals-only context cannot be both. + def nonEmpty: Boolean = !isEmpty def last: Option[String] = _lastParam @@ -513,21 +529,17 @@ package object sql { else s"${param.param}${param.painlessMethods.mkString("")}" - override def toString: String = { - val params = - if (isEmpty) "" - else - _keys - .flatMap { param => - get(param) match { - case Some(v) => Some(s"def $v = ${paramValue(param)}; ") - case None => None // should not happen - } + override def toString: String = + _declarations + .flatMap { + case Left(param) => + get(param) match { + case Some(v) => Some(s"def $v = ${paramValue(param)}; ") + case None => None // should not happen } - .mkString("") - // Locals bound by guarded booleans (AD-9) follow the params they read. - params + _locals.map { case (name, expr) => s"def $name = $expr; " }.mkString("") - } + case Right((name, expr)) => Some(s"def $name = $expr; ") + } + .mkString("") } trait PainlessParams extends PainlessScript { diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala index 1dc23e1fd..98963f2af 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala @@ -842,32 +842,36 @@ object Parser keyword("WHEN") ~> opt(not) ~ identName ~ comparison_operator ~ opt(value) ~ opt( dateMathScript ) >> { case n ~ field ~ op ~ v ~ fun => - val target_op = - n match { - case Some(_) => op.not - case None => op - } - v match { - case Some(value) => - success(CompareWatcherCondition(field, target_op, Left(value))) - case None => - fun match { - case Some(f) if f.identifier.dependencies.isEmpty => - success( - CompareWatcherCondition( - field, - target_op, - Right(f.identifier.withFunctions(f +: f.identifier.functions)) + // A `NOT` is folded into the operator. `comparison_operator` admits only the six arithmetic + // comparisons, every one of which HAS a negated spelling, so `maybeNegated` is `Some` here + // today -- but the answer is total (story BIDC-8, review M-6: `ComparisonOperator.not` used + // to `MatchError` on every other comparison), so a widening of that production rejects the + // combination instead of emitting a silently un-negated condition. + val target_op = if (n.isDefined) op.maybeNegated.getOrElse(op) else op + if (n.isDefined && op.maybeNegated.isEmpty) + err(s"NOT is not supported with the $op operator in a watcher condition") + else + v match { + case Some(value) => + success(CompareWatcherCondition(field, target_op, Left(value))) + case None => + fun match { + case Some(f) if f.identifier.dependencies.isEmpty => + success( + CompareWatcherCondition( + field, + target_op, + Right(f.identifier.withFunctions(f +: f.identifier.functions)) + ) ) - ) - case Some(_) => - err( - "Date/datetime functions with field dependencies are not supported for comparison" - ) - case None => - err("A value or a date/datetime function must be provided for comparison") - } - } + case Some(_) => + err( + "Date/datetime functions with field dependencies are not supported for comparison" + ) + case None => + err("A value or a date/datetime function must be provided for comparison") + } + } } private def scriptParams: PackratParser[ListMap[String, Value[_]]] = 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 f9f14e15b..d201b427a 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 @@ -38,6 +38,21 @@ case object Where extends Expr("WHERE") with TokenRegex sealed trait Criteria extends Updateable with PainlessScript { def operator: Operator + /** This criterion with its own `NOT` flipped, when it can carry one. + * + * 🔴 Story BIDC-8 (review B-2/M-5). `WhereParser.predicate` is `criteria ~ (and|or) ~ not.? ~ + * criteria`, so a `NOT` written AFTER a predicate operator lands on the PREDICATE, and the + * predicate used to emit it two different ways: `asFilter` wrapped the UN-negated right + * criterion in an Elasticsearch `must_not` (which MATCHES a document that lacks the field, + * inverting the per-predicate `false` collapse), while `painless` rendered `!(left && right)` + * (which negates the WHOLE composite). MEASURED on live ES 8.18.3 over docs A / B / + * field-absent: `WHERE NOT UPPER(x) = 'A' AND a = 1` returned `[2]` while the mirror order + * `WHERE a = 1 AND NOT UPPER(x) = 'A'` returned `[2, 3]` — the same predicate, different rows by + * writing order. Folding the NOT into the criterion it actually qualifies makes both paths agree + * and puts the negation where `check` can fold it into the operator. + */ + def negated: Option[Criteria] = None + def dependencies: Seq[Identifier] = this match { case Predicate(left, _, right, _, _) => left.dependencies ++ right.dependencies case c: Expression => c.dependencies @@ -205,7 +220,12 @@ sealed trait Criteria extends Updateable with PainlessScript { override def baseType: SQLType = SQLTypes.Boolean override def painless(context: Option[PainlessContext]): String = this match { - case Predicate(left, op, right, maybeNot, group) => + case p @ Predicate(left, op, _, maybeNot, group) => + // The same fold as `asFilter` (B-2): `A AND NOT B` renders as `A && `, never as + // `!(A && B)` — which negates the whole composite and, for an absent field, flipped a CASE + // condition into its THEN branch where ANSI takes ELSE (review B-2 C2, measured). + val right = p.emittedRight + val notHandled = maybeNot.isEmpty || p.notConsumed // 🔴 Story BIDC-8 AD-9 — each operand is PARENTHESISED. An operand that carries a null guard // renders as a ternary, and `?:` has the LOWEST precedence in Painless, so the unparenthesised // composition `a == null ? false : (x) && b == null ? false : (y)` parses as @@ -219,9 +239,9 @@ sealed trait Criteria extends Updateable with PainlessScript { case AND | OR => op.painless(context) case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") } - val not = maybeNot.nonEmpty + val not = maybeNot.nonEmpty && !notHandled if (group || not) - s"${maybeNot.map(_.painless(context)).getOrElse("")}($leftStr $opStr $rightStr)" + s"${if (not) maybeNot.map(_.painless(context)).getOrElse("") else ""}($leftStr $opStr $rightStr)" else s"$leftStr $opStr $rightStr" case relation: ElasticRelation => asGroup(relation.criteria.painless(context)) @@ -266,19 +286,33 @@ case class Predicate( case _ => criteria } + /** The right criterion as it must be EMITTED, with this predicate's own `NOT` folded into it when + * the criterion can carry one (story BIDC-8, review B-2). `sql` keeps rendering the `NOT` where + * the user wrote it; only emission changes. When the criterion cannot carry a `NOT` (a nested + * group, a relation) the old routing stands and `notConsumed` stays false, so nothing silently + * changes shape. + */ + private[query] lazy val (emittedRight: Criteria, notConsumed: Boolean) = + not match { + case Some(_) => rightCriteria.negated.map(_ -> true).getOrElse(rightCriteria -> false) + case None => rightCriteria -> false + } + override def asFilter(currentQuery: Option[ElasticBoolQuery]): ElasticFilter = { val query = asBoolQuery(currentQuery) + // A folded NOT is part of the right criterion now, so it must NOT be wrapped in `must_not`: + // `must_not` over a script MATCHES documents the script rejects, including those that lack the + // field, which is the opposite of the three-valued reading the criterion itself emits (B-2). + val negate = not.isDefined && !notConsumed operator match { case AND => - (not match { - case Some(_) => query.not(rightCriteria.asFilter(Option(query))) - case _ => query.filter(rightCriteria.asFilter(Option(query))) - }).filter(leftCriteria.asFilter(Option(query))) + (if (negate) query.not(emittedRight.asFilter(Option(query))) + else query.filter(emittedRight.asFilter(Option(query)))) + .filter(leftCriteria.asFilter(Option(query))) case OR => - (not match { - case Some(_) => query.not(rightCriteria.asFilter(Option(query))) - case _ => query.should(rightCriteria.asFilter(Option(query))) - }).should(leftCriteria.asFilter(Option(query))) + (if (negate) query.not(emittedRight.asFilter(Option(query))) + else query.should(emittedRight.asFilter(Option(query)))) + .should(leftCriteria.asFilter(Option(query))) } } @@ -369,6 +403,7 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { def maybeValue: Option[Token] def maybeNot: Option[NOT.type] def notAsString: String = maybeNot.map(v => s"$v ").getOrElse("") + def valueAsString: String = maybeValue.map(v => s" $v").getOrElse("") override def sql = s"$identifier $notAsString$operator$valueAsString" @@ -460,22 +495,84 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { case _ => false } - def painlessNot: String = operator match { - case _: ComparisonOperator => "" - case _ => maybeNot.map(_.painless(None)).getOrElse("") + /** This predicate's operator with its own `NOT` folded in, where the operator set HAS a negated + * spelling for it. `NOT x = 1` becomes `x != 1`; `NOT x LIKE 'a%'` stays `LIKE` and the negation + * is rendered by [[painlessNot]] instead (review M-6). One derivation, so `check`, `painlessOp` + * and the bridge cannot disagree about which operator is actually being emitted. + */ + protected def effectiveOperator: Operator = operator match { + case o: ComparisonOperator if maybeNot.isDefined => o.maybeNegated.getOrElse(o) + case o => o } - def painlessOp: String = operator match { - case o: ComparisonOperator if maybeNot.isDefined => o.not.painless(None) - case _ => operator.painless(None) + /** The `!` to render in front of the check, if any. + * + * A comparison normally folds its NOT into the operator, so there is nothing left to render -- + * UNLESS the operator has no negated spelling (`LIKE`, `RLIKE`, `IN`, `BETWEEN`, `MATCH`), in + * which case the whole check is negated here. The `!` lands INSIDE the null guard `painless` + * emits, so an absent field still collapses to `false` instead of being inverted into a match: + * ANSI three-valued logic, not `must_not` semantics. + */ + def painlessNot: String = operator match { + case o: ComparisonOperator if o.maybeNegated.isDefined => "" + case _ => maybeNot.map(_.painless(None)).getOrElse("") } - def painlessValue(context: Option[PainlessContext]): String = maybeValue - .map { + def painlessOp: String = effectiveOperator.painless(None) + + def painlessValue(context: Option[PainlessContext]): String = + maybeValue.map(painlessOperand(_, context)).getOrElse("") + + /** One operand's Painless rendering. A `Token` that is not a [[PainlessScript]] has only its SQL + * spelling to offer -- which is why `BetweenExpr` must go through this and not `toString`: a + * `StringValue` renders `'A'` in SQL and `"A"` in Painless, and the SQL form is a syntax error + * in a script. + */ + protected def painlessOperand(token: Token, context: Option[PainlessContext]): String = + token match { case v: PainlessScript => v.painless(context) case v => v.sql } - .getOrElse("") /*{ + + /** ` LIKE ''` (or RLIKE) as Painless. + * + * 🔴 Story BIDC-8 (review L-10). The generic `$param $painlessOp $value` fallback emitted `left1 + * .matches "A%"`, which is not Painless at all -- MEASURED on live ES 8.18.3: `invalid sequence + * of tokens near ['"A%"']`. Two further spellings were MEASURED and REFUTED before this one: + * `left1.matches("A.*")` gives `dynamic method [java.lang.String, matches/1] not found`, and + * `java.util.regex.Pattern.compile("A.*").matcher(left1).matches()` gives `static method + * [java.util.regex.Pattern, compile/1] not found` -- Painless deliberately whitelists NO + * `Pattern.compile`, so a regex may only enter through a `/.../` literal. (⚠️ That refutation + * also condemns `REGEXP_LIKE`'s emission at `function/string/package.scala:469`, which uses + * `Pattern.compile` -- PRE-EXISTING, untouched here, recorded for the lead.) + * + * So: the `%`-only patterns -- which is what a BI tool emits -- decompose into whitelisted + * `String` methods, and only a pattern that genuinely needs a regex falls back to the literal. + * That is not only faster, it is the only form that works on ES 6.8, where + * `script.painless.regex.enabled` defaults to `false`. `_` (the single-character wildcard) + * always needs the regex. The regex itself comes from the SHARED `toRegex`, the same translation + * the query-DSL path feeds to `regexQuery` (`bridge/package.scala:826-828`), so the scripted and + * non-scripted spellings of one SQL pattern cannot diverge -- including `toRegex`'s own + * limitation, that it does not escape regex metacharacters. + */ + private def likePainless(param: String, pattern: String, sqlWildcards: Boolean): String = { + def lit(value: String): String = s""""${escapePainlessString(value)}"""" + if (sqlWildcards && !pattern.contains("_")) { + val core = pattern.stripPrefix("%").stripSuffix("%") + if (core.nonEmpty && !core.contains("%")) { + val leading = pattern.startsWith("%") + val trailing = pattern.endsWith("%") && pattern.length > 1 + return (leading, trailing) match { + case (false, false) => s"$param.equals(${lit(core)})" + case (false, true) => s"$param.startsWith(${lit(core)})" + case (true, false) => s"$param.endsWith(${lit(core)})" + case (true, true) => s"$param.contains(${lit(core)})" + } + } + } + val regex = if (sqlWildcards) toRegex(pattern) else pattern + s"($param ==~ /${regex.replace("/", "\\/")}/)" + } /*{ operator match { case IsNull | IsNotNull => "null" case _ => "" @@ -516,8 +613,20 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * right-hand side, so a caller may adapt it first (the bucket-pipeline rendering converts a * temporal literal to epoch millis, the unit a date metric arrives in). */ - protected def check(context: Option[PainlessContext], param: String, value: String): String = { - operator match { + protected def check(context: Option[PainlessContext], param: String, value: String): String = + check(context, param, value, effectiveOperator) + + /** Same, for an EXPLICIT operator. `BetweenExpr` renders as two comparisons and needs the `GE` / + * `LE` per-type dispatch (`compareTo` for strings, `isBefore` / `isAfter` for temporals) that + * this method owns -- duplicating it there is how the two spellings drift apart. + */ + protected def check( + context: Option[PainlessContext], + param: String, + value: String, + op: Operator + ): String = { + op match { case _: ComparisonOperator => // 🔴 Story BIDC-8 AD-9 — dispatch on the EFFECTIVE operator, i.e. with a `NOT` folded in, // exactly as `painlessOp` does for the generic fallback below. Each arm here returns a @@ -526,11 +635,14 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { // the same Painless as their un-negated twins and the NOT was silently lost — a wrong // answer, not an error (MEASURED at the baseline: `maybeNot = Some(NOT)` on the AST, // `compareTo("A") == 0` in the script). - val comparison = operator match { - case o: ComparisonOperator if maybeNot.isDefined => o.not - case o => o - } + val comparison = op comparison match { + case LIKE | RLIKE => + maybeValue match { + case Some(sv: StringValue) => + return likePainless(param, sv.value, sqlWildcards = comparison == LIKE) + case _ => + } case LT => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => @@ -581,8 +693,8 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } case _ => } - s"$param $painlessOp $value" - case _ => s"$param$painlessOp($value)" + s"$param ${op.painless(None)} $value" + case _ => s"$param${op.painless(None)}($value)" } } @@ -644,58 +756,126 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { // A context-free rendering of an aggregate predicate is a bucket-pipeline rendering. if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless val innerLeft = left(context) + // The right-hand side is rendered ONCE: `painlessValue` can register a parameter on the context, + // so calling it twice would declare it twice (review H-4). + val innerRight = painlessValue(context) + + /** An operand of a comparison must be ATOMIC. A rendering that is not a bare name carries + * operators whose precedence swallows what is appended to it — `param2 ? 1 : 0` with `== 1` + * appended parses as `param2 ? 1 : (0 == 1)` (review L-9, measured: `Cannot cast from [int] to + * [java.lang.Object]`), and a null-guarded function rendering has the same shape (AD-9). + * + * What makes a rendering dangerous is an operator at PAREN DEPTH 0, so that is exactly what + * this asks: a name, a literal and a method CHAIN (`p.toLocalDate().get(X)`) are all atomic -- + * every operator inside them is already bracketed -- while a null-guarded ternary is not. Two + * pins measure both sides of that line: `BooleanCastSpec`'s 21.8 restoration pin fails if the + * literal of `CASE WHEN 1 = 1` is bound to a local, and `SQLQuerySpec`'s `LAST_DAY` fixture + * fails if the method chain is. + */ + def atomic(rendered: String): Boolean = { + var depth = 0 + var inString = false + var i = 0 + var result = true + while (i < rendered.length && result) { + val c = rendered.charAt(i) + if (inString) { + if (c == '\\') i += 1 + else if (c == '"') inString = false + } else + c match { + case '"' => inString = true + case '(' | '[' => depth += 1 + case ')' | ']' => depth -= 1 + case _ if depth == 0 && "?:+-*/%<>=!&|,".indexOf(c) >= 0 => result = false + case _ => + } + i += 1 + } + result + } + + /** Can this rendering evaluate to NULL at run time? A document field can be missing; a literal + * cannot. MEASURED over the real parser: a bare column reports `nullable = true`; a + * function-wrapped column reports `nullable = false` with `dependencies = [status]`; a literal + * (`CASE WHEN 1 = 1`, an identifier with an empty name and a one-element function chain — + * story 20.9) reports neither, which is why `functions.nonEmpty` is the wrong test and broke + * `BooleanCastSpec`'s 21.8 restoration pin. + */ + def readsDocumentField(token: Option[Token]): Boolean = token match { + case Some(id: Identifier) => id.nullable || id.dependencies.nonEmpty + case _ => false + } + + val leftNullable = readsDocumentField(Some(identifier)) + val rightNullable = readsDocumentField(maybeValue) + context match { case Some(ctx) => ctx.get(identifier) match { - case Some(p) => + case Some(p) if !rightNullable => if (identifier.nullable) - return s"$p == null ? false : $painlessNot(${check(context, p)})" + return s"$p == null ? false : $painlessNot(${check(context, p, innerRight)})" else - return s"$painlessNot(${check(context, p)})" + return s"$painlessNot(${check(context, p, innerRight)})" case _ => } case _ => } - // 🔴 Story BIDC-8 AD-9 — a left-hand side that can be NULL must be bound to a local so the - // comparison lands INSIDE the guard, and the guard must collapse to `false` exactly once, at the - // top of THIS predicate. + + // 🔴 Story BIDC-8 AD-9 — an operand that can be NULL is bound to a local so the comparison lands + // INSIDE the guard, and the guard collapses to `false` exactly once, at the top of THIS + // predicate. // // MEASURED at the baseline: `WHERE UPPER(status) <> 'ZZZ'` emitted // `(param1 == null) ? null : param1.toUpperCase().compareTo("ZZZ") != 0` — the function's own // null guard with the comparison appended OUTSIDE it. `?:` binds loosest, so the ternary's // branches became `null` (Object) and a primitive `boolean`, and Elasticsearch refused the whole - // query at COMPILE time: `script_exception / compile error`, caused by - // `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]` (live ES 8.18.3, - // data-independent — it fails on an empty index). The same held for the numeric family - // (`ABS(amount) > 10`); the date family escaped it only because `YEAR(...)` folds into the - // parameter assignment and reports `nullable`. + // query at COMPILE time: `class_cast_exception: Cannot cast from [boolean] to + // [java.lang.Object]` (live ES 8.18.3, data-independent — it fails on an empty index). The + // numeric family failed identically; the date family escaped only because `YEAR(...)` folds into + // the parameter assignment and reports `nullable`. // - // The `false` collapse is ANSI-safe here because NOT is already folded into the operator at this - // point (`painlessOp` uses `o.not`), so `NOT UPPER(x) = 'A'` over a MISSING field yields `false` - // = no match, which is what three-valued logic requires (`NOT NULL` is NULL, not TRUE). It is - // applied per predicate, never to a composite: `NULL OR true` stays true and `NULL AND true` - // stays false under `||`/`&&` composition. Only a QUERY (filter) context collapses — script - // fields, sorts, aggregations and ingest processors keep `null`, where it is a legitimate value - // (story 21.8 Part C). - // What must be guarded is a rendering that READS A DOCUMENT FIELD, since that is what can be - // missing. MEASURED (probe over the real parser): a bare column reports `name = status`, - // `nullable = true`, no dependencies; a function-wrapped column reports an EMPTY name, - // `nullable = false` and `dependencies = [status]`; a literal (`CASE WHEN 1 = 1`, which parses - // to an identifier with an empty name and a ONE-element function chain — story 20.9) reports - // neither. So `nullable || dependencies.nonEmpty` is the discriminator: a `functions.nonEmpty` - // test alone guards constants and broke `BooleanCastSpec`'s 21.8 restoration pin. - val guardInQuery = context.exists(_.context == PainlessContextType.Query) - val readsDocumentField = identifier.nullable || identifier.dependencies.nonEmpty - if (guardInQuery && readsDocumentField) { - // The local is declared in the script PROLOGUE, never inline: `def x = …;` is a STATEMENT and - // an operand of a composed predicate must be a pure expression. - val v = context.map(_.bindLocal(innerLeft)).getOrElse(innerLeft) - return s"($v == null ? false : ($painlessNot(${check(context, v)})))" - } - if (identifier.nullable) { - return s"def left = $innerLeft; left == null ? false : $painlessNot(${check(context, "left")})" - } - s"$painlessNot${check(context, innerLeft)}" + // 🔴 BOTH sides are guarded (review H-4): `WHERE UPPER(status) = UPPER(other)` over a document + // carrying `status` but NOT `other` used to reach `left.compareTo(null)` and fail the shard with + // `null_pointer_exception`. + // + // 🔴 WHY THE `false` COLLAPSE IS THE RIGHT ANSWER HERE, IN EVERY CONTEXT: an `Expression` IS a + // boolean — it is consumed as a condition, never projected as a value. Its three consumers are + // the filter script (`bridge/package.scala`, `scriptQuery`), a CASE condition + // (`function/cond/package.scala`, which captures this rendering into `paramN` and uses it as + // `paramN ? … : …`) and HAVING (`bucketPipelinePainless`, which returns above). A `null` in any + // of them is not a value, it is a condition that does not hold — and in a CASE it does not even + // compile (`def paramN = ; paramN ? 1 : 0`). MEASURED: `SELECT CASE WHEN UPPER(status) = + // 'A' THEN 1 ELSE 0 END` now returns 1/0/0/1 over docs A/B/absent/A, which is ANSI. What keeps + // its `null` is the rendering of an IDENTIFIER or a FUNCTION (`SELECT UPPER(status) AS u`, a + // sort key, a `terms` script) — a different code path that this method never enters, and which + // still emits `(param1 == null) ? null : …` (measured: `u` is `null` for the absent row). + // That is the 21.8 Part C rule, preserved. + val needsLeftLocal = leftNullable || !atomic(innerLeft) + val leftRef = + if (needsLeftLocal) context.map(_.bindLocal(innerLeft)).getOrElse(innerLeft) else innerLeft + val rightRef = + if (rightNullable && !atomic(innerRight)) + context.map(_.bindLocal(innerRight, "right")).getOrElse(innerRight) + else innerRight + val guards = + Seq( + if (leftNullable) Some(s"$leftRef == null") else None, + if (rightNullable) Some(s"$rightRef == null") else None + ).flatten + if (guards.nonEmpty && context.nonEmpty) + return s"(${guards.mkString(" || ")} ? false : ($painlessNot(${check(context, leftRef, rightRef)})))" + // 🔴 INVARIANT (story BIDC-8, review L-11): a statement sequence is legal ONLY with no + // `PainlessContext`. `Criteria.painless` is spliced into larger expressions -- a `Predicate` + // joins two renderings with `&&`/`||`, and a `CASE` condition captures one as + // `def paramN = ;` -- so a `def x = …;` in an operand is a compile-time 400. With a + // context the binding belongs in the PROLOGUE (`PainlessContext.bindLocal`). The context-free + // route is the HAVING / bucket-pipeline one, which is never spliced. Gated by + // `PainlessOperandFormSpec`, which FAILS (14 shapes) if `bindLocal` stops hoisting. + if (identifier.nullable) + return s"def left = $leftRef; left == null ? false : $painlessNot(${check(context, "left", innerRight)})" + s"$painlessNot${check(context, leftRef, innerRight)}" } override def validate(): Either[String, Unit] = { @@ -728,6 +908,9 @@ case class GenericExpression( ) extends Expression { override def maybeValue: Option[Token] = Option(value) + override def negated: Option[Criteria] = + Some(this.copy(maybeNot = if (maybeNot.isDefined) None else Some(NOT))) + override def update(request: SingleSearch): Criteria = { val updated = value match { @@ -752,6 +935,9 @@ case class Comparison( ) extends Expression { override def maybeValue: Option[Token] = Option(value) + override def negated: Option[Criteria] = + Some(this.copy(maybeNot = if (maybeNot.isDefined) None else Some(NOT))) + override def update(request: SingleSearch): Criteria = { val updated = this.copy(identifier = identifier.update(request)) if (updated.nested) { @@ -770,6 +956,8 @@ case class IsNullExpr(identifier: Identifier) extends Expression { override def maybeNot: Option[NOT.type] = None + override def negated: Option[Criteria] = Some(IsNotNullExpr(identifier)) + override def update(request: SingleSearch): Criteria = { val updated = this.copy(identifier = identifier.update(request)) if (updated.nested) { @@ -788,6 +976,8 @@ case class IsNotNullExpr(identifier: Identifier) extends Expression { override def maybeNot: Option[NOT.type] = None + override def negated: Option[Criteria] = Some(IsNullExpr(identifier)) + override def update(request: SingleSearch): Criteria = { val updated = this.copy(identifier = identifier.update(request)) if (updated.nested) { @@ -836,7 +1026,16 @@ case class IsNullCriteria(identifier: Identifier) extends CriteriaWithConditiona case _ => } if (identifier.nullable) { - return s"def left = ${left(context)}; left == null" + // See the L-11 invariant on `Expression.painless`: the statement form is legal only with no + // context. `IS NULL` takes a BARE name in the grammar today, so `addParam` above always + // answers and this line is unreachable under a context -- but a grammar widening must not be + // able to ship the statement form, so the binding is hoisted when there IS somewhere to + // hoist it to. + val rendered = left(context) + return context match { + case Some(ctx) => s"${ctx.bindLocal(rendered)} == null" + case None => s"def left = $rendered; left == null" + } } s"${left(context)} == null" } @@ -867,7 +1066,16 @@ case class IsNotNullCriteria(identifier: Identifier) case _ => } if (identifier.nullable) { - return s"def left = ${left(context)}; left != null" + // See the L-11 invariant on `Expression.painless`: the statement form is legal only with no + // context. `IS NULL` takes a BARE name in the grammar today, so `addParam` above always + // answers and this line is unreachable under a context -- but a grammar widening must not be + // able to ship the statement form, so the binding is hoisted when there IS somewhere to + // hoist it to. + val rendered = left(context) + return context match { + case Some(ctx) => s"${ctx.bindLocal(rendered)} != null" + case None => s"def left = $rendered; left != null" + } } s"${left(context)} != null" } @@ -879,6 +1087,10 @@ case class InExpr[R, +T <: Value[R]]( values: Values[R, T], maybeNot: Option[NOT.type] = None ) extends Expression { this: InExpr[R, T] => + + override def negated: Option[Criteria] = + Some(this.copy(maybeNot = if (maybeNot.isDefined) None else Some(NOT))) + private[this] lazy val id = functions.headOption match { case Some(f) => s"$f($identifier)" case _ => s"$identifier" @@ -936,21 +1148,33 @@ case class InExpr[R, +T <: Value[R]]( } } yield () - override def painless(context: Option[PainlessContext]): String = { - if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless - s"$painlessNot${identifier.painless(context)}$painlessOp(${painlessValue(context)})" - } + /** `[v1, v2].contains()` -- the LIST owns `contains`, not the element. + * + * 🔴 Story BIDC-8 (review L-8). The old override rendered + * `${identifier.painless}$painlessOp(${values})` = `left.contains(["A","B"])`, which asks a + * String whether it contains a List. It also bypassed [[Expression.painless]] entirely, so a + * function-wrapped operand got NO null guard, and `painlessNot` was "" for `IN` so `NOT IN` lost + * its negation in the document form. Overriding `check` instead puts IN on the ONE guarded + * emission path: `painless` binds the operand, collapses an absent field to `false`, and + * `painlessNot` renders the `!` (`IN` has no negated operator -- see + * `ComparisonOperator.maybeNegated`). + */ + override protected def check( + context: Option[PainlessContext], + param: String, + value: String, + op: Operator + ): String = s"$value.contains($param)" // `params. == v1 || params. == v2` -- the guarded bucket form of // ` IN (v1, v2)`. NOT `[v1,v2].contains(p)`: a buckets_path value arrives as a boxed // Double and the literals are Integers, so `List.contains` (Java `equals`) never matched -- // measured live, `MAX(age) IN (40, 50)` returned no bucket. Painless `==` promotes numerics and - // uses `equals` for strings. IN is a ComparisonOperator, so `painlessNot` is "" (a comparison - // folds its NOT into the operator, which this form never uses): the negation is rendered here. - override protected def bucketPipelineCheck(param: String): String = { - val membership = values.values.map(v => s"$param == ${v.painless(None)}").mkString(" || ") - if (maybeNot.isDefined) s"!($membership)" else membership - } + // uses `equals` for strings. The NOT is NOT rendered here: `IN` has no negated operator, so + // `painlessNot` renders the `!` for it (review M-6) and `bucketPipelinePainless` already wraps + // this call in it -- negating twice was a double negation. + override protected def bucketPipelineCheck(param: String): String = + values.values.map(v => s"$param == ${v.painless(None)}").mkString(" || ") } @@ -981,33 +1205,35 @@ case class BetweenExpr( } yield () } - override def painless(context: Option[PainlessContext]): String = { - if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless - context match { - case Some(ctx) => - ctx.addParam(identifier) match { - case Some(p) => - if (identifier.nullable) - return s"$p == null ? false : $painlessNot($p >= ${fromTo.from} && $p <= ${fromTo.to})" - else - return s"$painlessNot($p >= ${fromTo.from} && $p <= ${fromTo.to})" - case _ => - } - case _ => - } - if (identifier.nullable) { - return s"def left = ${left(context)}; left == null ? false : $painlessNot(${fromTo.from} <= left <= ${fromTo.to})" - } - s"$painlessNot(${fromTo.from} <= ${left(context)} <= ${fromTo.to})" + /** Two comparisons, `param >= from && param <= to`, each rendered by the SHARED per-type + * dispatch. + * + * 🔴 Story BIDC-8 (review L-8). The old override emitted three things Painless rejects. + * `${fromTo.from}` is `Token.toString` = the SQL spelling, so a string bound rendered `'A'` -- a + * syntax error in a script; the un-parameterised branch emitted the CHAINED `a <= left <= b`, + * which Painless refuses (`boolean <= int`) exactly as this class's own `bucketPipelineCheck` + * comment already recorded; and a temporal bound needs `isBefore` / `isAfter`, which only + * [[Expression.check]] knows. Overriding `check` puts BETWEEN on the ONE guarded emission path + * and reuses that dispatch via the explicit `GE` / `LE` operators. The NOT is rendered by + * `painlessNot` (`BETWEEN` has no negated operator). + */ + override protected def check( + context: Option[PainlessContext], + param: String, + value: String, + op: Operator + ): String = { + val from = painlessOperand(fromTo.from, context) + val to = painlessOperand(fromTo.to, context) + s"(${super.check(context, param, from, GE)} && ${super.check(context, param, to, LE)})" } - // The guarded bucket form of ` BETWEEN a AND b` -- two comparisons, not the chained - // `a <= p <= b` the document form used (Painless rejects `boolean <= int`). BETWEEN is a - // ComparisonOperator, so `painlessNot` is "": the negation is rendered here. - override protected def bucketPipelineCheck(param: String): String = { - val range = s"$param >= ${fromTo.from} && $param <= ${fromTo.to}" - if (maybeNot.isDefined) s"!($range)" else range - } + // The guarded bucket form of ` BETWEEN a AND b` -- two comparisons, not a chained + // `a <= p <= b` (Painless rejects `boolean <= int`). The NOT is NOT rendered here: `BETWEEN` has + // no negated operator, so `painlessNot` renders the `!` (review M-6) and + // `bucketPipelinePainless` already wraps this call in it. + override protected def bucketPipelineCheck(param: String): String = + s"$param >= ${fromTo.from} && $param <= ${fromTo.to}" } 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 new file mode 100644 index 000000000..ad7d4900d --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.scala @@ -0,0 +1,152 @@ +/* + * 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 +// 🔴 ALIASED: this spec lives in `…sql.query`, which declares its OWN `Table` (the FROM +// clause's). Scala 2.13 let the explicit import win; 2.12 does not, and only an explicit +// `++ 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 org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Story BIDC-8, AD-9 (review L-11) — a criteria rendered WITH a `PainlessContext` must be one + * EXPRESSION, never a statement sequence. + * + * 🔴 Why this is a gate and not a style rule. `Criteria.painless` has three consumers and two of + * them splice the rendering INTO a larger expression: `Criteria.painless`'s own `Predicate` arm + * joins two renderings with `&&` / `||`, and a `CASE` condition captures one as `def paramN = + * ;`. A rendering of the form `def left = X; left == null` is valid only at the TOP of + * a script: spliced, it produces `def param1 = def left = ...` or `A && def left = ...`, which + * Elasticsearch refuses at COMPILE time — a 400 on a statement the user typed correctly, and the + * exact failure class AD-9 was opened for. + * + * The context is what makes the statement form unnecessary: `PainlessContext.bindLocal` hoists the + * binding into the script PROLOGUE and hands back a bare name, so the criteria stays an + * expression. A context-free rendering (`painless(None)`) has nowhere to hoist to and keeps the + * statement form — that path is the HAVING / bucket-pipeline route, which is never spliced. + * + * REFUTES the reading that "no expectation moved" is evidence: the whole unit estate was green + * while `WHERE UPPER(status) IS NULL` emitted a statement form under a context. This spec fails on + * that, by construction, with no allow-list. + */ +class PainlessOperandFormSpec extends AnyFlatSpec with Matchers { + + private val schema: SchemaTable = SchemaTable( + "t", + columns = List( + Column("status", SQLTypes.Keyword), + Column("other", SQLTypes.Keyword), + Column("amount", SQLTypes.Double), + Column("n", SQLTypes.Int), + Column("ts", SQLTypes.Timestamp) + ) + ) + + /** Every criteria shape that can carry a function-wrapped operand — which is what forces the + * binding — plus the plain-column twin of each, so a fix that only guards the function case is + * visible. + */ + private val shapes: Seq[String] = Seq( + "SELECT id FROM t WHERE UPPER(status) = 'A'", + "SELECT id FROM t WHERE NOT UPPER(status) = 'A'", + "SELECT id FROM t WHERE UPPER(status) = UPPER(other)", + "SELECT id FROM t WHERE UPPER(status) <> 'A'", + "SELECT id FROM t WHERE ABS(amount) > 10", + "SELECT id FROM t WHERE UPPER(status) LIKE 'A%'", + "SELECT id FROM t WHERE UPPER(status) NOT LIKE 'A%'", + "SELECT id FROM t WHERE UPPER(status) IN ('A','B')", + "SELECT id FROM t WHERE UPPER(status) NOT IN ('A','B')", + "SELECT id FROM t WHERE ABS(amount) BETWEEN 1 AND 100", + "SELECT id FROM t WHERE ABS(amount) NOT BETWEEN 1 AND 100", + "SELECT id FROM t WHERE status IS NULL", + "SELECT id FROM t WHERE status IS NOT NULL", + "SELECT id FROM t WHERE UPPER(status) = 'A' AND ABS(amount) > 10", + "SELECT id FROM t WHERE UPPER(status) = 'A' OR ABS(amount) > 10", + "SELECT id FROM t WHERE ABS(amount) > 10 AND NOT UPPER(status) = 'A'" + ) + + private def renderedOf(sql: String): String = + Parser(sql) match { + case Right(ss: SingleSearch) => + val ctx = PainlessContext(context = PainlessContextType.Query) + val criteria = ss + .update(Some(schema)) + .where + .flatMap(_.criteria) + .getOrElse(fail(s"[$sql] has no WHERE criteria")) + criteria.painless(Some(ctx)) + 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. + */ + 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 + } + + "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" + } + withClue( + s"${offenders.size} criteria rendered a STATEMENT SEQUENCE under a context; a `def x = …;` " + + "binding belongs in the context prologue (PainlessContext.bindLocal), not in the operand:\n " + + offenders.mkString("\n ") + "\n" + )(offenders shouldBe empty) + } + + it should "hoist every binding it needs into the context prologue instead" in { + // Positive proof that the expression form is not achieved by dropping the binding: the shapes + // whose operand is function-wrapped DO declare something, and it is in the prologue. + Parser("SELECT id FROM t WHERE UPPER(status) = 'A'") match { + case Right(ss: SingleSearch) => + val ctx = PainlessContext(context = PainlessContextType.Query) + val rendered = ss.update(Some(schema)).where.flatMap(_.criteria).get.painless(Some(ctx)) + ctx.nonEmpty shouldBe true + ctx.toString should include("def ") + rendered should not include "def " + case other => fail(s"expected a SingleSearch, got $other") + } + } + + "a context-free rendering" should "keep the statement form, which is what HAVING consumes" in { + // The counterpart of the gate above: `painless(None)` has no prologue to hoist into, and the + // bucket-pipeline consumer never splices it into another expression. Pinning this keeps the + // gate honest — it must be the CONTEXT that removes the statement, not a blanket ban. + Parser("SELECT id FROM t WHERE UPPER(status) = 'A'") match { + case Right(ss: SingleSearch) => + ss.update(Some(schema)).where.flatMap(_.criteria).get.painless(None) should include("def ") + case other => fail(s"expected a SingleSearch, got $other") + } + } +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index 2debb5de0..0b35531e9 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -2499,6 +2499,73 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { ids("WHERE ABS(amount) > 10") shouldBe Seq(2, 3) ids("WHERE LOWER(status) = 'a' AND ABS(amount) > 10") shouldBe Seq.empty[Int] + // 🔴 Review B-2 / M-5 — the SAME predicate must give the SAME rows in either writing order. + // `WhereParser.predicate` is `criteria ~ (and|or) ~ not.? ~ criteria`, so a `NOT` written AFTER + // the operator lands on the PREDICATE, and the predicate used to emit it two different ways: + // `asFilter` wrapped the UN-negated right criterion in a `must_not` (which MATCHES a document + // that lacks the field) while `painless` rendered `!(left && right)` (which negates the whole + // composite). MEASURED on live ES 8.18.3 BEFORE the fix: the first line returned [2], the + // second [2, 3] — the same query, different rows by writing order, and row 3 is the one with no + // `status` field at all. The NOT is now folded into the criterion it qualifies, so both paths + // agree and `check` folds it into the operator. + ids("WHERE NOT UPPER(status) = 'A' AND ABS(amount) > 10") shouldBe Seq(2) + ids("WHERE ABS(amount) > 10 AND NOT UPPER(status) = 'A'") shouldBe Seq(2) + + // 🔴 Review M-6 / L-10 — `LIKE` over a function. + // BEFORE: `NOT LIKE` died with `scala.MatchError: LIKE` out of the query builder (there was no + // negated spelling for it and `ComparisonOperator.not` was a partial function), and plain + // `LIKE` emitted the un-parseable `left1 .matches "A%"` (`invalid sequence of tokens`). Two + // repaired spellings were then REFUTED on live ES before the shipped one: + // `left1.matches("A.*")` is `dynamic method [java.lang.String, matches/1] not found` and + // `Pattern.compile("A.*")` is `static method [java.util.regex.Pattern, compile/1] not found`. + // A `%`-only pattern now decomposes into whitelisted String methods, which is also the only + // form that works on ES 6.8 (regex literals are disabled there by default). + ids("WHERE UPPER(status) LIKE 'A%'") shouldBe Seq(1) + ids("WHERE UPPER(status) NOT LIKE 'A%'") shouldBe Seq(2) // NOT the absent row — ANSI + ids("WHERE UPPER(status) LIKE '%A%'") shouldBe Seq(1) + + // 🔴 Review L-8 — `IN` and `BETWEEN` over a function. + // BEFORE: both fell through to `termsQuery` / `rangeQuery` keyed on `identifier.name`, which is + // the EMPTY STRING for a function wrapper, so Elasticsearch rejected `{"terms":{"":[…]}}` and + // `{"range":{"":{…}}}` with the opaque `[bool] failed to parse field [filter]`. + ids("WHERE UPPER(status) IN ('A','B')") shouldBe Seq(1, 2) + ids("WHERE UPPER(status) NOT IN ('A','B')") shouldBe Seq.empty[Int] // not row 3 — ANSI + ids("WHERE ABS(amount) BETWEEN 1 AND 100") shouldBe Seq(1, 2, 3) + ids("WHERE ABS(amount) BETWEEN 1 AND 10") shouldBe Seq(1) + + // 🔴 Review B-2 case C2 — the same fold inside a CASE, where the NOT used to negate the WHOLE + // composite: `CASE WHEN a AND NOT b` emitted `!((a) && (b))`, so an absent `b` took the THEN + // branch where ANSI takes ELSE. Row 3 (no `status`) must be 0; row 2 must be 1, so this is not + // satisfied by collapsing everything to 0. + def caseValue(expr: String): Seq[(Int, String)] = + collectRows( + System.nanoTime(), + client.run(s"SELECT id, $expr AS c FROM tvl_orders").futureValue + ).flatMap { row => + row.get("id").map(_.toString.toDouble.toInt).map { id => + // A script field comes back wrapped in Elasticsearch's per-field array on every path, + // and its single element is `null` for the absent row -- so every unwrap here is + // null-safe, and an ABSENT `c` reads the same as a null one. + id -> row + .get("c") + .map { + case seq: Seq[_] => + seq.headOption.map(v => if (v == null) "null" else v.toString).getOrElse("null") + case other => if (other == null) "null" else other.toString + } + .getOrElse("null") + } + }.sortBy(_._1) + + caseValue("CASE WHEN amount > 10 AND NOT status = 'A' THEN 1 ELSE 0 END").map { case (id, v) => + id -> v.toDouble.toInt + } shouldBe Seq(1 -> 0, 2 -> 1, 3 -> 0) + + // 🔴 Review H-3 — the `false` collapse belongs to a CONDITION. A PROJECTED function keeps its + // `null`, because it goes through `Identifier.painless`, a different renderer. Without this the + // fix could have been "read" as turning every absent field into a value. + caseValue("UPPER(status)") shouldBe Seq(1 -> "A", 2 -> "B", 3 -> "null") + assertDdl(System.nanoTime(), client.run("DROP TABLE IF EXISTS tvl_orders").futureValue) } From ec6833843209f6134881a4bbafb1b304f686bf94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 09:29:37 +0200 Subject: [PATCH 08/10] fix(BIDC-8): fold NOT for every criteria class, and make LIKE mean one thing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10, the final audit. Each item was measured on live Elasticsearch 8.18.3 before and after, and each fix carries a gate that fails without it. BLOCKING — this story had turned a loud error into a silent wrong answer. `BetweenExpr` had no `negated` override, so a predicate's NOT fell back to wrapping the un-negated criterion in a `must_not`, whose semantics INCLUDE a document lacking the field: the guard's `false` came back as a match. Measured over a document with `status` but no `amount`, `WHERE status = 'A' AND NOT ABS(amount) BETWEEN 1 AND 10` returned it and the mirror order returned nothing; at the previous head that statement was a loud HTTP 400. It survived round 9 because the mirror-order pins there used only `UPPER(status) = 'A'`, the one criteria class that already had the override — a pin that exercises the class you just fixed proves nothing about its siblings. The new pins cover BETWEEN, IN and IS NULL in both writing orders. Two negation tables existed, `Criteria.negated` and a private copy in `MetricSelectorScript`; the private one had the BETWEEN arm right. Collapsed into one. `LIKE` meant three different things at once. `toRegex` translated `%` and `_` and nothing else, so every other regex metacharacter kept its regex meaning wherever a pattern ended up as a regex. Measured over `A.B` and `AXB1`: the native `regexp` query matched both, the string-method path matched only `A.B`, and the Painless regex matched only `AXB1`. In SQL only `%` and `_` are wildcards, so the escape belongs in the SHARED translation — putting it in the Painless emitter alone would have produced a fourth reading. USER-VISIBLE: `LIKE 'A.B%'` no longer matches `AXB1`. An audit prescription REFUTED and not applied. It asked for `ElasticBridge`'s nested arm to fold its NOT like the other three consumers. Tried, and `SQLQuerySpec`'s "predicate with distinct nested" fixture caught the flip: `must_not[nested] + filter` became `must[nested, nested(… == false)]`. Under a nested relation the criterion runs over a CHILD document inside an existential, so NOT outside and NOT inside ask different questions — "no reply is before that date", which a blog with no replies satisfies, versus "some reply is not before it", which it does not. The fold is valid exactly where the negated criterion is evaluated over the SAME document. What the audit was right about is that nothing ENUMERATED these consumers: all four are now listed on `Predicate.emittedRight`, each classified fold or `NOT-FOLD:`, and a source scan fails if a fifth states neither. That scan strips comments first — measured: without it the comment explaining the gate satisfied the gate. Smaller: `LIKE ''` over a function emitted `left1 ==~ //`, and `//` opens a Painless comment; the empty core now takes the equality path, matching the native form. A CASE with no ELSE emitted a truncated ternary, and once completed with SQL's implicit NULL it needed the result boxed — measured that it is the absence of a `return` that makes `cond ? 1 : null` fatal, and that `(def)` compiles in both positions. The sort pin is annotated with the live 500 it does NOT cover, so nobody reads it as "the sort path works", and both doc twins now carry that limitation, the real LIKE fast-path rule, and the release note. Green: sql 1065, core 979, bridge 203 x es6/es7/es8/es9, `+ sql/compile`, `++ 2.12.20` for sql, core and both bridge test trees, scalafmt, headerCheck; es8 JavaClientGatewayApiSpec 75/75 on real ES 8.18.3 with sixteen new round-10 pins. Core re-published serialised and content-verified; arrow re-run against it (the shared `toRegex` change reaches its pushdown): arrowJoin 228/228, `+ compile`, JoinExtensionIntegrationSpec 38/38 on ES 8.18.3 and 7.17.29. Co-Authored-By: Claude Opus 5 --- .../elastic/sql/bridge/ElasticBridge.scala | 28 ++-- .../sql/PainlessNullSurvivalSpec.scala | 68 ++++++++++ documentation/sql/dql_statements.md | 22 +++- .../elastic/sql/bridge/ElasticBridge.scala | 28 ++-- .../sql/PainlessNullSurvivalSpec.scala | 68 ++++++++++ .../elastic/sql/function/cond/package.scala | 29 ++++- .../app/softnetwork/elastic/sql/package.scala | 29 ++++- .../elastic/sql/query/GroupBy.scala | 13 +- .../softnetwork/elastic/sql/query/Where.scala | 84 ++++++++++-- .../elastic/sql/query/LikePatternSpec.scala | 123 ++++++++++++++++++ .../client/GatewayApiIntegrationSpec.scala | 43 ++++++ 11 files changed, 487 insertions(+), 48 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/LikePatternSpec.scala diff --git a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala index c3b2893f6..3f57345f9 100644 --- a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala +++ b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala @@ -121,22 +121,34 @@ case class ElasticBridge(filter: ElasticFilter) { val leftQuery = ElasticBridge(leftNested) .query(innerHitsNames /*++ leftNested.innerHitsName.toSet*/, leftBoolQuery) + // 🔴 NOT-FOLD: this consumer of `Predicate.not` deliberately does NOT fold. + // + // Round 10 (LOW-1) proposed aligning this site with the other three by reading + // `Predicate.emittedRight`. That was tried and MEASURED WRONG: under a nested + // relation the right criterion is evaluated over a CHILD document and the query + // wraps it in an EXISTENTIAL, so `NOT` outside and `NOT` inside are different + // questions. `WHERE MATCH(comments.content) AGAINST ('Nice') AND NOT + // replies.lastUpdated < LAST_DAY(…)` means "no reply is before that date", which + // includes a blog with NO replies; folded, it became "SOME reply is not before + // it", which excludes that blog and admits one with replies on both sides. The + // `SQLQuerySpec` "predicate with distinct nested" fixture caught the flip: + // `must_not[nested(…)] + filter[…]` became `must[nested(…), nested(… == false)]`. + // + // So the fold applies where the negated criterion is evaluated over the SAME + // document, and not here. Enumerated on `Predicate.emittedRight`. val rightNested = ElasticNested(p.rightCriteria, p.rightCriteria.limit) val rightBoolQuery = Option(ElasticBoolQuery(group = true)) val rightQuery = ElasticBridge(rightNested) .query(innerHitsNames /*++ rightNested.innerHitsName.toSet*/, rightBoolQuery) + val negate = p.not.isDefined p.operator match { case AND => - p.not match { - case Some(_) => not(rightQuery).filter(leftQuery) - case _ => must(leftQuery, rightQuery) - } + if (negate) not(rightQuery).filter(leftQuery) + else must(leftQuery, rightQuery) case _ => - p.not match { - case Some(_) => not(rightQuery).should(leftQuery) - case _ => should(leftQuery, rightQuery) - } + if (negate) not(rightQuery).should(leftQuery) + else should(leftQuery, rightQuery) } case _ => val boolQuery = Option(ElasticBoolQuery(group = true)) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index 189c9ba2d..b3d65659d 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -57,6 +57,19 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { body should not include "? false :" } + /** ⚠️ ROUND 10, MEDIUM-2 — READ THIS BEFORE READING THE ASSERTION AS AN ENDORSEMENT. + * + * The EMISSION below is correct: the sort script preserves `null`, which is what this story + * owns. Elasticsearch nevertheless REFUSES to run it when any document in the index lacks the + * field — MEASURED live on 8.18.3: `SELECT id FROM probe ORDER BY UPPER(status)` answers `HTTP + * 500 null_pointer_exception: Cannot invoke "java.lang.CharSequence.length()" because "text" is + * null`, so an ORDER BY over a function of a sparse column is unusable. That is PRE-EXISTING, + * downstream of this emission (the sort path builds the comparator), and NOT fixed here — a + * null-safe sort emission would change ordering semantics for present values, which is a product + * decision, not a dev one. It is disclosed in the PR body and in both doc twins. + * + * This pin therefore says "the emission still carries `null`", never "the sort path works". + */ "a sort key (script sort)" should "keep null for an absent field" in { val body = bodyOf("SELECT id FROM probe ORDER BY UPPER(status)") body should include("_script") @@ -92,4 +105,59 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { (referenced -- declared) shouldBe empty } } + + /** 🔴 Round 10, LOW-1 — a SOURCE SCAN, with no allow-list, over every consumer of + * `Predicate.not`. + * + * The fold of a predicate's `NOT` into its right criterion has FOUR consumers (enumerated on + * `Predicate.emittedRight`). Two of this story's defects were a consumer that did not take part: + * BLOCKING-1 was a criteria class with no `negated` override, LOW-1 a site still reading + * `rightCriteria` + `not` directly. Nothing listed them, so nothing noticed. This fails if a + * source file reads a predicate's `not` without also consulting `notConsumed` — the only honest + * way to use it. + */ + "every consumer of a predicate's NOT" should "say which side of the fold it is on" in { + def root: java.io.File = { + var d = new java.io.File(".").getAbsoluteFile + while (d != null && !new java.io.File(d, "build.sbt").isFile) d = d.getParentFile + if (d == null) fail("could not locate the build root") else d + } + def scalaFilesUnder(dir: java.io.File): Seq[java.io.File] = + if (!dir.isDirectory) Nil + else + Option(dir.listFiles).toSeq.flatten.flatMap { f => + if (f.isDirectory) scalaFilesUnder(f) + else if (f.getName.endsWith(".scala")) Seq(f) + else Nil + } + val sources = Seq("sql/src/main", "core/src/main", "bridge/src/main", "es6/bridge/src/main") + .map(new java.io.File(root, _)) + .flatMap(scalaFilesUnder) + sources should not be empty + val offenders = sources.flatMap { f => + // 🔴 COMMENTS STRIPPED FIRST. Measured: without this the gate is defeated by the very comment + // that explains it — the file naming `notConsumed` in prose passes while the CODE below has + // stopped consulting it. A guard that its own documentation satisfies is not a guard. + val raw = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") + val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") + val readsNot = """(? document with no `status`, and a `GROUP BY UPPER(status)` has no bucket for it. The collapse to > "no match" applies to a **condition**, never to a value. > -> ⚠️ `LIKE` / `NOT LIKE` with `%` and `_` in the same pattern (`'A_%'`) compiles to a Painless -> regular expression, and Elasticsearch **6.8** disables regular expressions in scripts by default -> (`script.painless.regex.enabled`). Patterns using only `%` — which is what a BI tool emits — need -> no regex and work on every supported version. +> ⚠️ **`ORDER BY` over a function of a column some documents do not carry does not work**, on any +> version. The engine emits a null-preserving sort script, and Elasticsearch then fails the search +> with `null_pointer_exception` while building the comparator. Sort by the bare column, or keep the +> field present on every document. +> +> ⚠️ **When a `LIKE` over a function needs a regular expression.** The engine compiles such a +> predicate to whitelisted string operations when the pattern contains **no `_`** and uses `%` +> **only at the ends** (`'A%'`, `'%A'`, `'%A%'`, `'A'`, `''`, `'%'`). Every other pattern — including +> one made only of `%`, such as `'A%B'` — compiles to a Painless regular expression, and +> Elasticsearch **6.8** disables those by default (`script.painless.regex.enabled`), answering +> `Regexes are disabled`. On 7.x and later every pattern works. +> +> 🔴 **Changed in 0.23.0 — `LIKE` reads only `%` and `_` as wildcards.** Every other character in a +> pattern is now matched literally, on the scripted **and** the native path. `WHERE status LIKE +> 'A.B%'` previously matched `AXB1`, because `.` reached Elasticsearch as a regular-expression +> wildcard; it now matches only values that really begin with `A.B`. Patterns that relied on the old +> reading must be rewritten with `_` (any single character) or `%` (any sequence). `RLIKE` is +> unaffected — its operand is a regular expression by definition. **Example** diff --git a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala index b5856a65a..c39e9bc52 100644 --- a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala +++ b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticBridge.scala @@ -119,22 +119,34 @@ case class ElasticBridge(filter: ElasticFilter) { val leftQuery = ElasticBridge(leftNested) .query(innerHitsNames /*++ leftNested.innerHitsName.toSet*/, leftBoolQuery) + // 🔴 NOT-FOLD: this consumer of `Predicate.not` deliberately does NOT fold. + // + // Round 10 (LOW-1) proposed aligning this site with the other three by reading + // `Predicate.emittedRight`. That was tried and MEASURED WRONG: under a nested + // relation the right criterion is evaluated over a CHILD document and the query + // wraps it in an EXISTENTIAL, so `NOT` outside and `NOT` inside are different + // questions. `WHERE MATCH(comments.content) AGAINST ('Nice') AND NOT + // replies.lastUpdated < LAST_DAY(…)` means "no reply is before that date", which + // includes a blog with NO replies; folded, it became "SOME reply is not before + // it", which excludes that blog and admits one with replies on both sides. The + // `SQLQuerySpec` "predicate with distinct nested" fixture caught the flip: + // `must_not[nested(…)] + filter[…]` became `must[nested(…), nested(… == false)]`. + // + // So the fold applies where the negated criterion is evaluated over the SAME + // document, and not here. Enumerated on `Predicate.emittedRight`. val rightNested = ElasticNested(p.rightCriteria, p.rightCriteria.limit) val rightBoolQuery = Option(ElasticBoolQuery(group = true)) val rightQuery = ElasticBridge(rightNested) .query(innerHitsNames /*++ rightNested.innerHitsName.toSet*/, rightBoolQuery) + val negate = p.not.isDefined p.operator match { case AND => - p.not match { - case Some(_) => not(rightQuery).filter(leftQuery) - case _ => must(leftQuery, rightQuery) - } + if (negate) not(rightQuery).filter(leftQuery) + else must(leftQuery, rightQuery) case _ => - p.not match { - case Some(_) => not(rightQuery).should(leftQuery) - case _ => should(leftQuery, rightQuery) - } + if (negate) not(rightQuery).should(leftQuery) + else should(leftQuery, rightQuery) } case _ => val boolQuery = Option(ElasticBoolQuery(group = true)) diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index 189c9ba2d..b3d65659d 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -57,6 +57,19 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { body should not include "? false :" } + /** ⚠️ ROUND 10, MEDIUM-2 — READ THIS BEFORE READING THE ASSERTION AS AN ENDORSEMENT. + * + * The EMISSION below is correct: the sort script preserves `null`, which is what this story + * owns. Elasticsearch nevertheless REFUSES to run it when any document in the index lacks the + * field — MEASURED live on 8.18.3: `SELECT id FROM probe ORDER BY UPPER(status)` answers `HTTP + * 500 null_pointer_exception: Cannot invoke "java.lang.CharSequence.length()" because "text" is + * null`, so an ORDER BY over a function of a sparse column is unusable. That is PRE-EXISTING, + * downstream of this emission (the sort path builds the comparator), and NOT fixed here — a + * null-safe sort emission would change ordering semantics for present values, which is a product + * decision, not a dev one. It is disclosed in the PR body and in both doc twins. + * + * This pin therefore says "the emission still carries `null`", never "the sort path works". + */ "a sort key (script sort)" should "keep null for an absent field" in { val body = bodyOf("SELECT id FROM probe ORDER BY UPPER(status)") body should include("_script") @@ -92,4 +105,59 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { (referenced -- declared) shouldBe empty } } + + /** 🔴 Round 10, LOW-1 — a SOURCE SCAN, with no allow-list, over every consumer of + * `Predicate.not`. + * + * The fold of a predicate's `NOT` into its right criterion has FOUR consumers (enumerated on + * `Predicate.emittedRight`). Two of this story's defects were a consumer that did not take part: + * BLOCKING-1 was a criteria class with no `negated` override, LOW-1 a site still reading + * `rightCriteria` + `not` directly. Nothing listed them, so nothing noticed. This fails if a + * source file reads a predicate's `not` without also consulting `notConsumed` — the only honest + * way to use it. + */ + "every consumer of a predicate's NOT" should "say which side of the fold it is on" in { + def root: java.io.File = { + var d = new java.io.File(".").getAbsoluteFile + while (d != null && !new java.io.File(d, "build.sbt").isFile) d = d.getParentFile + if (d == null) fail("could not locate the build root") else d + } + def scalaFilesUnder(dir: java.io.File): Seq[java.io.File] = + if (!dir.isDirectory) Nil + else + Option(dir.listFiles).toSeq.flatten.flatMap { f => + if (f.isDirectory) scalaFilesUnder(f) + else if (f.getName.endsWith(".scala")) Seq(f) + else Nil + } + val sources = Seq("sql/src/main", "core/src/main", "bridge/src/main", "es6/bridge/src/main") + .map(new java.io.File(root, _)) + .flatMap(scalaFilesUnder) + sources should not be empty + val offenders = sources.flatMap { f => + // 🔴 COMMENTS STRIPPED FIRST. Measured: without this the gate is defeated by the very comment + // that explains it — the file naming `notConsumed` in prose passes while the CODE below has + // stopped consulting it. A guard that its own documentation satisfies is not a guard. + val raw = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") + val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") + val readsNot = """(? @@ -307,7 +320,7 @@ package object cond { } val c = SQLTypeUtils.coerce(cond, out, context) val r = - res match { + boxWhenNoDefault(res match { case i: Identifier if i.name == name && name.nonEmpty => i.withNullable(false) SQLTypeUtils.coerce( @@ -317,7 +330,7 @@ package object cond { ) case _ => SQLTypeUtils.coerce(res, out, context) - } + }) expParam match { case Some(e) => if (cond.nullable) { @@ -348,7 +361,7 @@ package object cond { } val c = SQLTypeUtils.coerce(cond, SQLTypes.Boolean, context) val r = - res match { + boxWhenNoDefault(res match { case i: Identifier if i.name == name && name.nonEmpty => i.withNullable(false) SQLTypeUtils.coerce( @@ -358,7 +371,7 @@ package object cond { ) case _ => SQLTypeUtils.coerce(res, out, context) - } + }) if (!cond.isInstanceOf[CriteriaWithConditionalFunction[_]] && cond.nullable) { ctx.addParam(LiteralParam(c)) match { case Some(c) => s"$c ? $r" @@ -381,7 +394,13 @@ package object cond { } else { cases = s"$cases : $d" } - case _ => + // 🔴 Story BIDC-8 (round 10, MEDIUM-3). A `CASE … THEN x END` with NO `ELSE` used to + // leave the ternary truncated — `param2 ? 1` — which Elasticsearch rejects at compile + // time: `unexpected token [''] was expecting one of [':']` (MEASURED live on + // 8.18.3). SQL says the missing ELSE is NULL, so that is what the else-branch emits. + // Pre-existing; it sits on the surface this story's CASE gate exercises, and the gate + // missed it because every fixture supplied an ELSE. + case _ => cases = s"$cases : null" } cases 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 05ee37b5a..b73cca9d7 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -1100,8 +1100,35 @@ package object sql { override def nullable: Boolean = true } + /** A SQL `LIKE` pattern as a regular expression. + * + * 🔴 Story BIDC-8 (round 10, HIGH-2). This used to replace `%` and `_` and NOTHING else, so + * every other regex metacharacter in the pattern kept its REGEX meaning — and since a pattern + * only becomes a regex on some paths, the same SQL then meant different things depending on + * where it ran. MEASURED on live ES 8.18.3 over `A.B` and `AXB1`: `status LIKE 'A.B%'` (native + * `regexp`) matched BOTH, `UPPER(status) LIKE 'A.B%'` (string methods) matched only `A.B`, and + * `UPPER(status) LIKE 'A.B%1'` (Painless regex) matched only `AXB1` — three readings of one + * pattern. In SQL only `%` and `_` are wildcards; `.` is a literal. Escaping here fixes all of + * them in ONE place, which is the point: the native `regexp` query and the scripted forms read + * the shared translation, so they cannot disagree. + * + * ⚠️ USER-VISIBLE: `LIKE 'A.B%'` no longer matches `AXB1`. That is the correct SQL reading, and + * it is a 0.23.0 release note. + * + * The escaped set is the union of the Java and Lucene regex metacharacters. Every one of them is + * a legal backslash escape in BOTH engines (Java only forbids escaping an ALPHABETIC character + * that is not a known construct), so one escaping rule serves the query DSL and Painless alike. + */ def toRegex(value: String): String = { - value.replaceAll("%", ".*").replaceAll("_", ".") + val metacharacters = "\\.[]{}()*+-?^$|#@&<>~\"" + val out = new StringBuilder(value.length * 2) + value.foreach { + case '%' => out.append(".*") + case '_' => out.append('.') + case c if metacharacters.indexOf(c) >= 0 => out.append('\\').append(c) + case c => out.append(c) + } + out.toString } case object Alias extends Expr("AS") with TokenRegex diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala index c2edb7c19..70af12b1c 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala @@ -375,7 +375,7 @@ object MetricSelectorScript { // falls back to `!( ... )`. maybeNot match { case Some(_) => - negated(right) match { + right.negated match { case Some(n) => s"($leftStr) $opStr ${metricSelector(n)}" // Grammar-unreachable today (`NOT (A AND B)` in HAVING is a parse rejection); kept // as the total fallback for a compound right side. @@ -401,17 +401,6 @@ object MetricSelectorScript { case _ => "1 == 1" } - /** The single expression `c` with its own NOT toggled, when `c` is one that carries a NOT. */ - private def negated(c: Criteria): Option[Criteria] = { - def toggle(not: Option[NOT.type]): Option[NOT.type] = if (not.isDefined) None else Some(NOT) - c match { - case e: GenericExpression => Some(e.copy(maybeNot = toggle(e.maybeNot))) - case e: Comparison => Some(e.copy(maybeNot = toggle(e.maybeNot))) - case e: BetweenExpr => Some(e.copy(maybeNot = toggle(e.maybeNot))) - case e: InExpr[_, _] => Some(e.copy(maybeNot = toggle(e.maybeNot))) - case _ => None - } - } } case class BucketIncludesExcludes(values: Set[String] = Set.empty, regex: Option[String] = None) 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 d201b427a..08aa54c11 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 @@ -292,7 +292,33 @@ case class Predicate( * group, a relation) the old routing stands and `notConsumed` stays false, so nothing silently * changes shape. */ - private[query] lazy val (emittedRight: Criteria, notConsumed: Boolean) = + /** 🔴 EVERY CONSUMER OF `Predicate.not`, ENUMERATED (round 10, LOW-1). Nothing listed them, which + * is why BLOCKING-1 (a missing `negated` override) and LOW-1 (a site still reading + * `rightCriteria` + `not`) both slipped through a round that was specifically about this fold: + * + * 1. `Criteria.painless`'s `Predicate` arm (`Where.scala`, ~:227) — the script form; + * 1. `Predicate.asFilter` (just below) — the query-DSL form; + * 1. `MetricSelectorScript.metricSelector` (`GroupBy.scala`, ~:378) — HAVING, via + * `right.negated`; it kept its OWN copy of the negation table until round 10 collapsed it + * into `Criteria.negated`, and that copy already had the `BetweenExpr` arm the shared one + * was missing — two tables, one right, one wrong; + * 1. `ElasticBridge`'s nested/relation arm (`ElasticBridge.scala`, ~:135, and the es6 copy) — + * which deliberately does NOT fold, and that is the point of enumerating them. + * + * 🔴 THE FOLD IS NOT UNIFORM, and round 10 MEASURED why. It is valid exactly where the negated + * criterion is evaluated over the SAME document. Under a NESTED relation the criterion runs over + * a CHILD document inside an EXISTENTIAL, so `NOT` outside and `NOT` inside ask different + * questions: `AND NOT replies.lastUpdated < d` means "no reply is before d" (a blog with no + * replies qualifies), while the folded form asks "some reply is not before d" (it does not). + * Aligning that fourth site by folding was tried and reverted; `SQLQuerySpec`'s "predicate with + * distinct nested" fixture is what caught the flip, turning `must_not[nested] + filter` into + * `must[nested, nested(… == false)]`. + * + * So the first three fold and the fourth does not, each for a stated reason. + * `PainlessNullSurvivalSpec`'s source scan fails if a consumer appears that states NEITHER — + * silence is the failure mode both round-10 defects had in common. + */ + private[sql] lazy val (emittedRight: Criteria, notConsumed: Boolean) = not match { case Some(_) => rightCriteria.negated.map(_ -> true).getOrElse(rightCriteria -> false) case None => rightCriteria -> false @@ -546,20 +572,44 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * also condemns `REGEXP_LIKE`'s emission at `function/string/package.scala:469`, which uses * `Pattern.compile` -- PRE-EXISTING, untouched here, recorded for the lead.) * - * So: the `%`-only patterns -- which is what a BI tool emits -- decompose into whitelisted - * `String` methods, and only a pattern that genuinely needs a regex falls back to the literal. - * That is not only faster, it is the only form that works on ES 6.8, where - * `script.painless.regex.enabled` defaults to `false`. `_` (the single-character wildcard) - * always needs the regex. The regex itself comes from the SHARED `toRegex`, the same translation - * the query-DSL path feeds to `regexQuery` (`bridge/package.scala:826-828`), so the scripted and - * non-scripted spellings of one SQL pattern cannot diverge -- including `toRegex`'s own - * limitation, that it does not escape regex metacharacters. + * So the common patterns -- which is what a BI tool emits -- decompose into whitelisted `String` + * methods, and only a pattern that genuinely needs a regex falls back to the literal. The regex + * itself comes from the SHARED `toRegex`, the same translation the query-DSL path feeds to + * `regexQuery` (`bridge/package.scala:805-814`), so the scripted and non-scripted spellings of + * one SQL pattern cannot diverge -- a claim that was FALSE until round 10 escaped the regex + * metacharacters there. + * + * 🔴 THE EXACT RULE, because "only `%` patterns are safe" was MEASURED WRONG (round 10, HIGH-1): + * the string-method fast path is taken when the pattern has NO `_` and `%` appears ONLY at the + * ends. Everything else compiles to a Painless regex literal -- including patterns made only of + * `%`, such as `'A%B'`. A regex literal needs `script.painless.regex.enabled`, which stock **ES + * 6.8** leaves at `false`: there, `UPPER(status) LIKE 'A%B'` answers `illegal_state_exception: + * Regexes are disabled` while `LIKE 'A%'` returns 200 on the same cluster. + * + * 📌 FUTURE IMPROVEMENT (recorded, not a defect; no issue filed). This decomposition is reached + * ONLY for a function-wrapped identifier. A plain `LIKE` / `RLIKE` / `NOT LIKE` never enters + * Painless at all -- it becomes a native `regexp` query (`bridge/package.scala:805-814`) -- and + * the script path is chosen at `bridge/package.scala:668-679` (the identifier carries functions, + * the single geo-`Distance` case excepted). For the case-folding functions, `UPPER(x)` / + * `LOWER(x)` over a PLAIN column, the predicate is really a case-insensitive match, and + * Elasticsearch has supported `case_insensitive: true` on `regexp` and `wildcard` queries since + * **7.10**: that would be native, index-accelerated and free of the Painless whitelist entirely. + * It is not available on ES 6.8, which would still need this decomposition or a loud rejection. + * And it replaces nothing: a genuinely computed operand (`SUBSTRING(x, 1, 3) LIKE 'AB%'`, + * `CONCAT`, `TRIM`, arithmetic) cannot be expressed as a native query at all, so the scripted + * path and this decomposition are required for the general case. The improvement is an + * optimisation for a recognisable subset. */ private def likePainless(param: String, pattern: String, sqlWildcards: Boolean): String = { def lit(value: String): String = s""""${escapePainlessString(value)}"""" if (sqlWildcards && !pattern.contains("_")) { val core = pattern.stripPrefix("%").stripSuffix("%") - if (core.nonEmpty && !core.contains("%")) { + // 🔴 Round 10 (MEDIUM-1): NO `core.nonEmpty` precondition. `LIKE ''` fell through to the + // regex branch and emitted `left1 ==~ //`, where `//` opens a Painless COMMENT — + // `unexpected character [//))))]` on live ES 8.18.3, while the native `status LIKE ''` + // answers `[]`. An empty core is meaningful in all four shapes: `''` is `equals("")`, + // `'%'` is `endsWith("")` and `'%%'` is `contains("")`, both true for any non-null value. + if (!core.contains("%")) { val leading = pattern.startsWith("%") val trailing = pattern.endsWith("%") && pattern.length > 1 return (leading, trailing) match { @@ -1183,6 +1233,20 @@ case class BetweenExpr( fromTo: FromTo, maybeNot: Option[NOT.type] ) extends Expression { + + /** \U0001f534 Story BIDC-8 (round 10, BLOCKING-1). Without this override `Predicate.emittedRight` + * fell back to wrapping the UN-negated criterion in an Elasticsearch `must_not`, whose semantics + * INCLUDE a document lacking the field — so the guarded script's `false` was inverted into a + * match. MEASURED on live ES 8.18.3 over a document carrying `status` but NO `amount`: `WHERE + * status = 'A' AND NOT ABS(amount) BETWEEN 1 AND 10` returned `[8]` while the mirror `WHERE + * ABS(amount) NOT BETWEEN 1 AND 10 AND status = 'A'` returned `[]` — the same predicate, + * different rows by writing order, and the row it added is the one ANSI excludes. It is the + * defect `Criteria.negated` exists to prevent, and it survived because the round-9 pins only + * exercised `GenericExpression`, the one class that already had the override. + */ + override def negated: Option[Criteria] = + Some(this.copy(maybeNot = if (maybeNot.isDefined) None else Some(NOT))) + override def sql = s"$identifier $notAsString$operator $fromTo" override def operator: Operator = BETWEEN override def update(request: SingleSearch): Criteria = { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/LikePatternSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/LikePatternSpec.scala new file mode 100644 index 000000000..039604fe6 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/LikePatternSpec.scala @@ -0,0 +1,123 @@ +/* + * 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.{toRegex, PainlessContext, PainlessContextType} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Story BIDC-8, round 10 — what a SQL `LIKE` pattern MEANS, in one place. + * + * 🔴 HIGH-2. `toRegex` used to translate `%` and `_` and nothing else, so every other regex + * metacharacter kept its REGEX meaning — and since a pattern only becomes a regex on SOME paths, + * one SQL statement had three different readings. MEASURED on live ES 8.18.3 over documents `A.B` + * and `AXB1`: + * + * - `status LIKE 'A.B%'` (native `regexp`) matched BOTH — `.` was any character; + * - `UPPER(status) LIKE 'A.B%'` (string-method fast path) matched only `A.B` — `.` was literal; + * - `UPPER(status) LIKE 'A.B%1'` (Painless regex) matched only `AXB1`. + * + * In SQL only `%` and `_` are wildcards. Escaping in the SHARED translation makes all three agree, + * which is why the fix belongs there and not in the Painless emitter: fixing it only there would + * have produced a THIRD semantics. + * + * ⚠️ USER-VISIBLE, 0.23.0 release note: `LIKE 'A.B%'` no longer matches `AXB1`. + */ +class LikePatternSpec extends AnyFlatSpec with Matchers { + + private val schema: SchemaTable = SchemaTable( + "t", + columns = List(Column("status", SQLTypes.Keyword), Column("amount", SQLTypes.Double)) + ) + + private def scriptOf(sql: String): String = + Parser(sql) match { + case Right(ss: SingleSearch) => + val ctx = PainlessContext(context = PainlessContextType.Query) + val body = ss + .update(Some(schema)) + .where + .flatMap(_.criteria) + .getOrElse(fail(s"[$sql] has no WHERE criteria")) + .painless(Some(ctx)) + s"$ctx$body" + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + "toRegex" should "treat %% and _ as the ONLY wildcards" in { + toRegex("A%") shouldBe "A.*" + toRegex("A_B") shouldBe "A.B" + toRegex("%A%") shouldBe ".*A.*" + } + + it should "escape every other regex metacharacter, so a literal stays a literal" in { + // Each of these was a WILDCARD before the fix, in the native `regexp` query as well as in the + // scripted form — `A.B` matched `AXB`. + toRegex("A.B") shouldBe "A\\.B" + toRegex("A+B") shouldBe "A\\+B" + toRegex("A*B") shouldBe "A\\*B" + toRegex("A|B") shouldBe "A\\|B" + toRegex("A(B)") shouldBe "A\\(B\\)" + toRegex("A[B]") shouldBe "A\\[B\\]" + toRegex("A{2}") shouldBe "A\\{2\\}" + toRegex("A^B$") shouldBe "A\\^B\\$" + toRegex("A?B") shouldBe "A\\?B" + toRegex("A-B") shouldBe "A\\-B" + toRegex("""A\B""") shouldBe """A\\B""" + // Lucene's regexp grammar has metacharacters Java's does not; every one of these is also a + // legal Java backslash escape, so ONE escaping rule serves both engines. + toRegex("A#B@C&DF~G") shouldBe "A\\#B\\@C\\&D\\F\\~G" + // … and a wildcard still survives beside them. + toRegex("A.B%") shouldBe "A\\.B.*" + } + + /** 🔴 HIGH-1 — the documented rule used to be "only `%` patterns are safe", which is wrong in + * both directions: `'A%B'` is pure `%` and still needs a regex, and a pure `_` pattern needs one + * too. The real rule is: NO `_`, and `%` only at the ends. + */ + "the string-method fast path" should "be taken exactly when the pattern has no _ and % only at the ends" in { + def usesRegex(pattern: String): Boolean = + scriptOf(s"SELECT id FROM t WHERE UPPER(status) LIKE '$pattern'").contains("==~") + + usesRegex("A%") shouldBe false // startsWith + usesRegex("%A") shouldBe false // endsWith + usesRegex("%A%") shouldBe false // contains + usesRegex("A") shouldBe false // equals + usesRegex("") shouldBe false // equals("") — MEDIUM-1 + usesRegex("%") shouldBe false // endsWith("") + usesRegex("%%") shouldBe false // contains("") + + usesRegex("A%B") shouldBe true // pure `%`, still a regex — the falsified claim + usesRegex("A_B") shouldBe true // `_` always needs one + usesRegex("_") shouldBe true + usesRegex("%A%B%") shouldBe true + } + + /** 🔴 MEDIUM-1 — the empty pattern used to emit `left1 ==~ //`, where `//` opens a Painless + * COMMENT: `unexpected character [//))))]` on live ES 8.18.3, while the native `status LIKE ''` + * answered `[]`. + */ + "an empty LIKE pattern" should "compare for equality, not open a comment" in { + val script = scriptOf("SELECT id FROM t WHERE UPPER(status) LIKE ''") + script should include("""equals("")""") + script should not include "==~" + script should not include "//" + } +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index 0b35531e9..e995d75a6 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -2566,6 +2566,49 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { // fix could have been "read" as turning every absent field into a value. caseValue("UPPER(status)") shouldBe Seq(1 -> "A", 2 -> "B", 3 -> "null") + // 🔴 Round 10, BLOCKING-1 — the round-9 mirror-order pair used `UPPER(status) = 'A'`, a + // `GenericExpression`, which is the ONE criteria class that already carried `negated`. That is + // exactly why the hole survived, so the pins below cover every OTHER class that can carry a + // `NOT`. MEASURED on live ES 8.18.3 before the fix, over a document with `status` but NO + // `amount`: `WHERE status = 'A' AND NOT ABS(amount) BETWEEN 1 AND 10` returned that row while + // the mirror `WHERE ABS(amount) NOT BETWEEN 1 AND 10 AND status = 'A'` returned none — the + // `must_not` wrapping the guarded script INCLUDED the document the guard had excluded. Row 3 is + // the one with no `status`; rows 1 and 2 carry `amount` 5 and 50. + ids("WHERE status = 'A' AND NOT ABS(amount) BETWEEN 1 AND 100") shouldBe Seq.empty[Int] + ids("WHERE ABS(amount) NOT BETWEEN 1 AND 100 AND status = 'A'") shouldBe Seq.empty[Int] + ids("WHERE status = 'B' AND NOT UPPER(status) IN ('A')") shouldBe Seq(2) + ids("WHERE UPPER(status) NOT IN ('A') AND status = 'B'") shouldBe Seq(2) + ids("WHERE status = 'A' AND NOT amount IS NULL") shouldBe Seq(1) + ids("WHERE amount IS NOT NULL AND status = 'A'") shouldBe Seq(1) + + // 🔴 Round 10, HIGH-2 — in SQL only `%` and `_` are wildcards. Before the fix the SAME pattern + // meant three different things: the native `regexp` read `.` as any character, the + // string-method path read it literally, and the Painless regex read it as any character again. + // `status` here is 'A' / 'B' / absent, so a literal-dot pattern matches nothing and a + // regex-dot one would match 'A' — the assertion distinguishes them. + ids("WHERE status LIKE 'A.'") shouldBe Seq.empty[Int] + ids("WHERE UPPER(status) LIKE 'A.'") shouldBe Seq.empty[Int] + ids("WHERE UPPER(status) LIKE 'A_'") shouldBe Seq + .empty[Int] // `_` IS a wildcard, but 'A' is 1 char + ids("WHERE UPPER(status) LIKE 'A'") shouldBe Seq(1) + + // 🔴 Round 10, MEDIUM-1 — `LIKE ''` over a function emitted `left1 ==~ //`, and `//` opens a + // Painless COMMENT: `unexpected character [//))))]`. The native form always answered `[]`. + ids("WHERE UPPER(status) LIKE ''") shouldBe Seq.empty[Int] + ids("WHERE status LIKE ''") shouldBe Seq.empty[Int] + // … and the all-wildcard patterns, which share the empty core, match every NON-NULL value — + // row 3 has no `status`, so it is absent from both. + ids("WHERE UPPER(status) LIKE '%'") shouldBe Seq(1, 2) + ids("WHERE UPPER(status) LIKE '%%'") shouldBe Seq(1, 2) + + // 🔴 Round 10, MEDIUM-3 — a `CASE … THEN x END` with NO `ELSE`. SQL says the missing branch is + // NULL; Painless types a ternary from its branches, so the emission used to be the truncated + // `param2 ? 1` (`unexpected token ['']`) and then, once the `: null` was added, + // `Cannot cast from [int] to [java.lang.Object]`. Both were live 400s. + caseValue("CASE WHEN UPPER(status) = 'A' THEN 1 END").map { case (id, v) => + id -> v + } shouldBe Seq(1 -> "1", 2 -> "null", 3 -> "null") + assertDdl(System.nanoTime(), client.run("DROP TABLE IF EXISTS tvl_orders").futureValue) } From d55ee7ae52c37c334636ce1081f03ad64617fcee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 10:16:53 +0200 Subject: [PATCH 09/10] fix(BIDC-8): guard a NULL-valued CASE, collapse % runs, and gate the class axis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 11, the last fix round. Each item measured on live Elasticsearch 8.18.3 before and after. The `ORDER BY` disclosure was describing a DATA-LOSS mode as a loud error, in four places. Sorting by a function of a column some documents do not carry fails the shard building the comparator — and only a SINGLE-shard index turns that into an error. Measured on three shards with seven documents, one lacking the field: HTTP 200, `_shards.failed: 1`, `hits.total: 5`, the failing shard's two documents simply absent. Nothing surfaces it; `grep -rn "_shards" core/src/main` finds no consumer on the search path. Corrected in both doc twins, the PR body and the pin's own comment, and named as the #205 / #209 / #253 family. Making core raise on `_shards.failed` is a separate issue, deliberately not started here. A `CASE … END` with no `ELSE` is NULL-valued and reached comparisons unguarded. `= 1` hid it, because Painless tolerates `null == 1`; `> 0` gave `Cannot invoke "Object.getClass()" because "leftObject" is null` and a string `=` gave `cannot access method/field [compareTo] from a null def reference`. The operand discriminator now has a third disjunct — a nullable FUNCTION makes its operand nullable — which is what the round-10 pin's chosen shape had hidden. It does not re-open the `functions.nonEmpty` trap round 9 measured: a `CASE` with an `ELSE` reports `nullable = false`, so the literal shapes stay unguarded and byte-identical. `%%` means what `%` means, but the fast-path test stripped only one leading and one trailing `%`, so `'%%A'` fell to a regex and the rule the code documents was false as written — measured, `circuit_breaking_exception: Regular expression considered too many characters` on 8.18 and `Regexes are disabled` on 6.8. Runs are collapsed first, which makes the documented rule true and keeps more patterns off the regex path everywhere. A NEW GATE ON THE AXIS THE BLOCKING DEFECT LIVED ON. Round 10's gate enumerated the CONSUMERS of a predicate's NOT; BLOCKING-1 was a criteria CLASS with no `negated`, which no consumer-side check could ever see. The class-axis gate has no allow-list — declare a `maybeNot` field, override `negated` — and was shown failing by deleting one override. ⚠️ Two things stated rather than fixed, both measured. `MatchCriteria` and `MultiMatchCriteria` declare no `maybeNot` at all, so `NOT match(x) AGAINST ('y')` still takes the `must_not` route and matches a document lacking the field — the same deviation the bare-column `NOT status = 'A'` route has. Fixing it needs a new field (arity, and a `.sql` render that `MaterializedViewExtension` persists) plus a bridge arm emitting `must_not(match) + exists(field)`: the lead's call, not this round's. It is pinned as it behaves and named by the gate's scaladoc rather than left silent. And the consumer-axis gate's limits are now written down: it is anchored on the `Predicate` TYPE rather than a receiver name, so both measured evasions (`pr.not`, a destructured fourth field) are caught, but classification stays per FILE — counting uses against classifications is defeated because `Where.scala` legitimately says `negated` many times, and a line-window version cried wolf on `Predicate(l, _, r, _, _)`. A text scan cannot type a receiver; the class-axis gate is the proof, this one is the reminder. Also: `(def)` now parenthesises its operand (it was casting a nested condition), three stale line citations corrected, a literal escape sequence in a scaladoc fixed, and `emittedRight` / `notConsumed` returned to `private[query]` — the consumer that needed them wider was reverted in round 10. Green: sql 1066, core 979, bridge 203 x es6/es7/es8/es9, `+ sql/compile`, `++ 2.12.20` for sql, core and both bridge test trees, scalafmt, headerCheck; es8 JavaClientGatewayApiSpec 75/75 on real ES 8.18.3 with the round-11 pins. Core re-published serialised and content-verified; arrow re-run against it because the shared `toRegex` and the operand discriminator both reach its pushdown: arrowJoin 228/228, JoinExtensionIntegrationSpec 38/38 on ES 8.18.3. Co-Authored-By: Claude Opus 5 --- .../sql/PainlessNullSurvivalSpec.scala | 84 ++++++++++++++----- documentation/sql/dql_statements.md | 28 +++++-- .../sql/PainlessNullSurvivalSpec.scala | 84 ++++++++++++++----- .../elastic/sql/function/cond/package.scala | 6 +- .../softnetwork/elastic/sql/query/Where.scala | 44 +++++++--- .../sql/query/PainlessOperandFormSpec.scala | 64 ++++++++++++++ .../client/GatewayApiIntegrationSpec.scala | 28 +++++++ 7 files changed, 282 insertions(+), 56 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index b3d65659d..1232ec711 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -60,13 +60,20 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { /** ⚠️ ROUND 10, MEDIUM-2 — READ THIS BEFORE READING THE ASSERTION AS AN ENDORSEMENT. * * The EMISSION below is correct: the sort script preserves `null`, which is what this story - * owns. Elasticsearch nevertheless REFUSES to run it when any document in the index lacks the - * field — MEASURED live on 8.18.3: `SELECT id FROM probe ORDER BY UPPER(status)` answers `HTTP - * 500 null_pointer_exception: Cannot invoke "java.lang.CharSequence.length()" because "text" is - * null`, so an ORDER BY over a function of a sparse column is unusable. That is PRE-EXISTING, - * downstream of this emission (the sort path builds the comparator), and NOT fixed here — a - * null-safe sort emission would change ordering semantics for present values, which is a product - * decision, not a dev one. It is disclosed in the PR body and in both doc twins. + * owns. Elasticsearch then fails the SHARD while building the comparator, and what the CALLER + * sees depends on the shard count — the dangerous case being the normal one. MEASURED live on + * 8.18.3: a SINGLE-shard index rejects the search (`HTTP 500 null_pointer_exception: Cannot + * invoke "java.lang.CharSequence.length()" because "text" is null`), but a THREE-shard index + * holding 7 documents, one of them lacking the field, answers **HTTP 200** with `_shards.failed: + * 1` and `hits.total: 5` — the failing shard's two documents SILENTLY ABSENT, no error anywhere. + * Nothing surfaces it: `grep -rn "_shards" core/src/main` finds no consumer on the search path. + * That is the #205 / #209 / #253 silent-wrong-answer family, and an earlier draft of this + * comment called it a loud error, which is the opposite of what it is. + * + * PRE-EXISTING and NOT fixed here — a null-safe sort emission changes ordering semantics for + * values that ARE present, which is a product decision; and surfacing `_shards.failures` is its + * own issue, deliberately not started in this story. Disclosed in the PR body and both doc + * twins. * * This pin therefore says "the emission still carries `null`", never "the sort path works". */ @@ -134,21 +141,60 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { .map(new java.io.File(root, _)) .flatMap(scalaFilesUnder) sources should not be empty + // 🔴 Round 11 (M-4). The first version keyed on a receiver literally named `p` or `predicate` + // and asked only whether the WHOLE FILE mentioned `notConsumed`. Both were measured evadable: + // `pr.not` passed green, so did `case Predicate(_, _, _, n, _) => n.isDefined`, and a new + // mis-use anywhere in `Where.scala` — which holds two of the four consumers — could never + // redden because the file mentions `notConsumed` elsewhere. + // + // The anchor is now the TYPE, recovered from the binding rather than from a name convention: + // every identifier the file binds to a `Predicate` (`case x @ Predicate`, `case x: Predicate`, + // `x: Predicate` in a parameter list) is collected, and `.not` counts as a use — as + // does a destructured fourth field BOUND TO A NAME (`Predicate(l, _, r, _, _)` discards it on + // purpose and is not a consumer). Every use must be PAID FOR: the count of uses may not exceed + // the count of classifications, so one mention no longer covers a file. Receivers that are not + // predicates — elastic4s's own `boolQuery.not(...)`, for instance — are invisible to it, which + // is the point: a gate that cries wolf is a gate someone silences. + // + // ⚠️ WHAT THIS GATE DOES NOT DO, stated because round 11 MEASURED it rather than assumed it. + // Classification is per FILE, so a NEW mis-use inside a file that already classifies one will + // not redden. Two stronger rules were tried and both failed: counting uses against + // classifications is defeated because `Where.scala` legitimately says `negated` many times, and + // a line-window version either cried wolf on `Predicate(l, _, r, _, _)` (which discards the NOT + // on purpose) or mis-numbered lines once block comments were stripped. A text scan cannot type + // a receiver; closing this axis properly needs a typed check (a Scalafix rule), recorded as a + // follow-up. THE GATE THAT ACTUALLY CATCHES THIS DEFECT CLASS IS THE CLASS-AXIS ONE in + // `PainlessOperandFormSpec` — BLOCKING-1 was a criteria CLASS with no `negated`, which no + // consumer-side check could ever have seen. This one is a reminder, not a proof. + val destructured = """Predicate\(\s*[^)]*?,\s*[^)]*?,\s*[^)]*?,\s*[a-zA-Z]\w*\s*,""".r + val boundToPredicate = Seq( + """case\s+(\w+)\s*@\s*Predicate""".r, + """case\s+(\w+)\s*:\s*Predicate""".r, + """(\w+)\s*:\s*Predicate""".r + ) val offenders = sources.flatMap { f => - // 🔴 COMMENTS STRIPPED FIRST. Measured: without this the gate is defeated by the very comment - // that explains it — the file naming `notConsumed` in prose passes while the CODE below has - // stopped consulting it. A guard that its own documentation satisfies is not a guard. val raw = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") - val readsNot = """(? + s"""(? classifications) + Some( + s"${f.getPath.substring(root.getPath.length)} ($uses uses, $classifications classified)" + ) else None } withClue( diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 5f9690a25..d5a6ccb20 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -420,8 +420,8 @@ The `WHERE` clause supports: > caused by `class_cast_exception: Cannot cast from [boolean] to [java.lang.Object]`), so no such > predicate ever ran. > -> This holds for **every** comparison the clause accepts, not only `=`: `<`, `>`, `<>`, `LIKE`, -> `NOT LIKE`, `IN`, `NOT IN`, `BETWEEN` and `NOT BETWEEN` over a function all follow the same rule, +> This holds for the comparisons listed here, not only `=`: `<`, `>`, `<>`, `LIKE`, `NOT LIKE`, +> `IN`, `NOT IN`, `BETWEEN` and `NOT BETWEEN` over a function all follow the same rule, > and a `NOT` written after `AND` / `OR` (`WHERE ABS(amount) > 10 AND NOT UPPER(status) = 'A'`) > negates the criterion it qualifies, not the whole composite — so the same predicate returns the > same rows whichever way round you write it. Before **0.23.0** several of these did not run at all: @@ -437,10 +437,26 @@ The `WHERE` clause supports: > document with no `status`, and a `GROUP BY UPPER(status)` has no bucket for it. The collapse to > "no match" applies to a **condition**, never to a value. > -> ⚠️ **`ORDER BY` over a function of a column some documents do not carry does not work**, on any -> version. The engine emits a null-preserving sort script, and Elasticsearch then fails the search -> with `null_pointer_exception` while building the comparator. Sort by the bare column, or keep the -> field present on every document. +> 🔴 **`ORDER BY` over a function of a column some documents do not carry LOSES ROWS SILENTLY.** The +> engine emits a null-preserving sort script; Elasticsearch then fails the shard while building the +> comparator (`null_pointer_exception`). What you see depends on the shard count, and the dangerous +> case is the normal one: +> +> - on a **single-shard** index the whole search is rejected — you get an error; +> - on a **multi-shard** index the search returns **HTTP 200** and the failing shard's documents are +> simply **absent from the result**. MEASURED on Elasticsearch 8.18.3, 3 shards, 7 documents with +> one lacking the field: `_shards.failed: 1`, `hits.total: 5` — two rows gone, no error anywhere. +> The engine does not surface `_shards.failures`, so nothing reaches the caller. +> +> Until that is fixed, sort by the bare column, or keep the field present on every document. Do not +> rely on getting an error. The same applies to `ORDER BY` over a `CASE … END` with no `ELSE`, which +> is NULL-valued for the rows no branch matches. +> +> ⚠️ Two limits of the rule above, stated rather than implied. `NOT (x) IS NULL` is a +> PARSE rejection — the grammar takes a bare name after `NOT` there — so the rule covers the +> comparisons listed, not literally every clause you can write. And on a **multi-valued** field the +> scripted and non-scripted routes differ for a reason that has nothing to do with NULL: the native +> query matches if ANY value matches, while the script reads a single value. > > ⚠️ **When a `LIKE` over a function needs a regular expression.** The engine compiles such a > predicate to whitelisted string operations when the pattern contains **no `_`** and uses `%` diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index b3d65659d..1232ec711 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -60,13 +60,20 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { /** ⚠️ ROUND 10, MEDIUM-2 — READ THIS BEFORE READING THE ASSERTION AS AN ENDORSEMENT. * * The EMISSION below is correct: the sort script preserves `null`, which is what this story - * owns. Elasticsearch nevertheless REFUSES to run it when any document in the index lacks the - * field — MEASURED live on 8.18.3: `SELECT id FROM probe ORDER BY UPPER(status)` answers `HTTP - * 500 null_pointer_exception: Cannot invoke "java.lang.CharSequence.length()" because "text" is - * null`, so an ORDER BY over a function of a sparse column is unusable. That is PRE-EXISTING, - * downstream of this emission (the sort path builds the comparator), and NOT fixed here — a - * null-safe sort emission would change ordering semantics for present values, which is a product - * decision, not a dev one. It is disclosed in the PR body and in both doc twins. + * owns. Elasticsearch then fails the SHARD while building the comparator, and what the CALLER + * sees depends on the shard count — the dangerous case being the normal one. MEASURED live on + * 8.18.3: a SINGLE-shard index rejects the search (`HTTP 500 null_pointer_exception: Cannot + * invoke "java.lang.CharSequence.length()" because "text" is null`), but a THREE-shard index + * holding 7 documents, one of them lacking the field, answers **HTTP 200** with `_shards.failed: + * 1` and `hits.total: 5` — the failing shard's two documents SILENTLY ABSENT, no error anywhere. + * Nothing surfaces it: `grep -rn "_shards" core/src/main` finds no consumer on the search path. + * That is the #205 / #209 / #253 silent-wrong-answer family, and an earlier draft of this + * comment called it a loud error, which is the opposite of what it is. + * + * PRE-EXISTING and NOT fixed here — a null-safe sort emission changes ordering semantics for + * values that ARE present, which is a product decision; and surfacing `_shards.failures` is its + * own issue, deliberately not started in this story. Disclosed in the PR body and both doc + * twins. * * This pin therefore says "the emission still carries `null`", never "the sort path works". */ @@ -134,21 +141,60 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { .map(new java.io.File(root, _)) .flatMap(scalaFilesUnder) sources should not be empty + // 🔴 Round 11 (M-4). The first version keyed on a receiver literally named `p` or `predicate` + // and asked only whether the WHOLE FILE mentioned `notConsumed`. Both were measured evadable: + // `pr.not` passed green, so did `case Predicate(_, _, _, n, _) => n.isDefined`, and a new + // mis-use anywhere in `Where.scala` — which holds two of the four consumers — could never + // redden because the file mentions `notConsumed` elsewhere. + // + // The anchor is now the TYPE, recovered from the binding rather than from a name convention: + // every identifier the file binds to a `Predicate` (`case x @ Predicate`, `case x: Predicate`, + // `x: Predicate` in a parameter list) is collected, and `.not` counts as a use — as + // does a destructured fourth field BOUND TO A NAME (`Predicate(l, _, r, _, _)` discards it on + // purpose and is not a consumer). Every use must be PAID FOR: the count of uses may not exceed + // the count of classifications, so one mention no longer covers a file. Receivers that are not + // predicates — elastic4s's own `boolQuery.not(...)`, for instance — are invisible to it, which + // is the point: a gate that cries wolf is a gate someone silences. + // + // ⚠️ WHAT THIS GATE DOES NOT DO, stated because round 11 MEASURED it rather than assumed it. + // Classification is per FILE, so a NEW mis-use inside a file that already classifies one will + // not redden. Two stronger rules were tried and both failed: counting uses against + // classifications is defeated because `Where.scala` legitimately says `negated` many times, and + // a line-window version either cried wolf on `Predicate(l, _, r, _, _)` (which discards the NOT + // on purpose) or mis-numbered lines once block comments were stripped. A text scan cannot type + // a receiver; closing this axis properly needs a typed check (a Scalafix rule), recorded as a + // follow-up. THE GATE THAT ACTUALLY CATCHES THIS DEFECT CLASS IS THE CLASS-AXIS ONE in + // `PainlessOperandFormSpec` — BLOCKING-1 was a criteria CLASS with no `negated`, which no + // consumer-side check could ever have seen. This one is a reminder, not a proof. + val destructured = """Predicate\(\s*[^)]*?,\s*[^)]*?,\s*[^)]*?,\s*[a-zA-Z]\w*\s*,""".r + val boundToPredicate = Seq( + """case\s+(\w+)\s*@\s*Predicate""".r, + """case\s+(\w+)\s*:\s*Predicate""".r, + """(\w+)\s*:\s*Predicate""".r + ) val offenders = sources.flatMap { f => - // 🔴 COMMENTS STRIPPED FIRST. Measured: without this the gate is defeated by the very comment - // that explains it — the file naming `notConsumed` in prose passes while the CODE below has - // stopped consulting it. A guard that its own documentation satisfies is not a guard. val raw = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") - val readsNot = """(? + s"""(? classifications) + Some( + s"${f.getPath.substring(root.getPath.length)} ($uses uses, $classifications classified)" + ) else None } withClue( 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 76b100eef..bd8fc4f84 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 @@ -294,7 +294,11 @@ package object cond { * default, so every existing emission is byte-identical. */ private def boxWhenNoDefault(rendered: String): String = - if (default.isEmpty) s"(def)$rendered" else rendered + // 🔴 Round 11 — PARENTHESISED. `(def)$rendered` casts only the first token, so a result that + // is itself a ternary came out as `param4 ? (def)param3 ? (def)1 : null : null`, casting the + // nested CONDITION rather than the value. Benign today (a `def` condition still works) and + // wrong as written. + if (default.isEmpty) s"(def)($rendered)" else rendered override def painless(context: Option[PainlessContext] = None): String = { context match { 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 08aa54c11..6fe06906e 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 @@ -318,7 +318,7 @@ case class Predicate( * `PainlessNullSurvivalSpec`'s source scan fails if a consumer appears that states NEITHER — * silence is the failure mode both round-10 defects had in common. */ - private[sql] lazy val (emittedRight: Criteria, notConsumed: Boolean) = + private[query] lazy val (emittedRight: Criteria, notConsumed: Boolean) = not match { case Some(_) => rightCriteria.negated.map(_ -> true).getOrElse(rightCriteria -> false) case None => rightCriteria -> false @@ -575,9 +575,12 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * So the common patterns -- which is what a BI tool emits -- decompose into whitelisted `String` * methods, and only a pattern that genuinely needs a regex falls back to the literal. The regex * itself comes from the SHARED `toRegex`, the same translation the query-DSL path feeds to - * `regexQuery` (`bridge/package.scala:805-814`), so the scripted and non-scripted spellings of - * one SQL pattern cannot diverge -- a claim that was FALSE until round 10 escaped the regex - * metacharacters there. + * `regexQuery` (`bridge/package.scala:825-827`), so the scripted and non-scripted spellings of + * one SQL pattern read the same PATTERN -- a claim that was false until round 10 escaped the + * regex metacharacters there. ⚠️ It is a claim about the pattern, not about the whole predicate: + * on a MULTI-VALUED field the two routes still differ, because the native query matches if ANY + * value matches while the script reads `doc['x'].value`. That is orthogonal to this method and + * is recorded, not fixed. * * 🔴 THE EXACT RULE, because "only `%` patterns are safe" was MEASURED WRONG (round 10, HIGH-1): * the string-method fast path is taken when the pattern has NO `_` and `%` appears ONLY at the @@ -588,8 +591,8 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * * 📌 FUTURE IMPROVEMENT (recorded, not a defect; no issue filed). This decomposition is reached * ONLY for a function-wrapped identifier. A plain `LIKE` / `RLIKE` / `NOT LIKE` never enters - * Painless at all -- it becomes a native `regexp` query (`bridge/package.scala:805-814`) -- and - * the script path is chosen at `bridge/package.scala:668-679` (the identifier carries functions, + * Painless at all -- it becomes a native `regexp` query (`bridge/package.scala:825-827`) -- and + * the script path is chosen at `bridge/package.scala:657-697` (the identifier carries functions, * the single geo-`Distance` case excepted). For the case-folding functions, `UPPER(x)` / * `LOWER(x)` over a PLAIN column, the predicate is really a case-insensitive match, and * Elasticsearch has supported `case_insensitive: true` on `regexp` and `wildcard` queries since @@ -600,8 +603,16 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * path and this decomposition are required for the general case. The improvement is an * optimisation for a recognisable subset. */ - private def likePainless(param: String, pattern: String, sqlWildcards: Boolean): String = { + private def likePainless(param: String, rawPattern: String, sqlWildcards: Boolean): String = { def lit(value: String): String = s""""${escapePainlessString(value)}"""" + // 🔴 Round 11 (M-2). `%%` means exactly what `%` means, but the fast-path test strips only ONE + // leading and ONE trailing `%`, so `'%%A'` fell through to a regex — and the rule this method + // documents ("no `_`, `%` only at the ends") was then false as written. MEASURED on live ES + // 8.18.3, `UPPER(status) LIKE '%%A'` answered + // `circuit_breaking_exception: Regular expression considered too many characters`, and on 6.8 it + // is `Regexes are disabled`. Collapsing the runs first makes the documented rule TRUE and keeps + // more patterns off the regex path on every version. The collapse is identity for LIKE. + val pattern = if (sqlWildcards) rawPattern.replaceAll("%+", "%") else rawPattern if (sqlWildcards && !pattern.contains("_")) { val core = pattern.stripPrefix("%").stripSuffix("%") // 🔴 Round 10 (MEDIUM-1): NO `core.nonEmpty` precondition. `LIKE ''` fell through to the @@ -853,8 +864,19 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * `BooleanCastSpec`'s 21.8 restoration pin. */ def readsDocumentField(token: Option[Token]): Boolean = token match { - case Some(id: Identifier) => id.nullable || id.dependencies.nonEmpty - case _ => false + case Some(id: Identifier) => + // 🔴 Round 11 (M-1) — `id.functions.exists(_.nullable)` is the third disjunct, and it is + // what makes a `CASE … END` with no `ELSE` guardable. SQL says the missing branch is NULL, + // `Case.nullable` reports that faithfully, but the wrapping identifier reports neither + // `nullable` nor dependencies for it, so the operand reached the comparison unguarded. + // MEASURED on live ES 8.18.3: `= 1` survived only because Painless tolerates `null == 1`, + // while `> 0` gave `Cannot invoke "Object.getClass()" because "leftObject" is null` and a + // string `=` gave `cannot access method/field [compareTo] from a null def reference`. + // This does NOT re-open the `functions.nonEmpty` trap round 9 measured: a `CASE` WITH an + // `ELSE` reports `nullable = false`, so the literal shapes `BooleanCastSpec` pins stay + // unguarded and byte-identical. + id.nullable || id.dependencies.nonEmpty || id.functions.exists(_.nullable) + case _ => false } val leftNullable = readsDocumentField(Some(identifier)) @@ -1234,8 +1256,8 @@ case class BetweenExpr( maybeNot: Option[NOT.type] ) extends Expression { - /** \U0001f534 Story BIDC-8 (round 10, BLOCKING-1). Without this override `Predicate.emittedRight` - * fell back to wrapping the UN-negated criterion in an Elasticsearch `must_not`, whose semantics + /** 🔴 Story BIDC-8 (round 10, BLOCKING-1). Without this override `Predicate.emittedRight` fell + * back to wrapping the UN-negated criterion in an Elasticsearch `must_not`, whose semantics * INCLUDE a document lacking the field — so the guarded script's `false` was inverted into a * match. MEASURED on live ES 8.18.3 over a document carrying `status` but NO `amount`: `WHERE * status = 'A' AND NOT ABS(amount) BETWEEN 1 AND 10` returned `[8]` while the mirror `WHERE 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 ad7d4900d..367a941f8 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 @@ -149,4 +149,68 @@ class PainlessOperandFormSpec extends AnyFlatSpec with Matchers { case other => fail(s"expected a SingleSearch, got $other") } } + + /** 🔴 Round 11 (M-3) — the CLASS axis, which is the axis BLOCKING-1 actually lived on. + * + * Round 10 added a gate over the CONSUMERS of `Predicate.not`. That is orthogonal to the defect + * that blocked: `BetweenExpr` was a criteria CLASS with no `negated` override, and no + * consumer-side check could ever see it. A class that can carry its own `NOT` and does not say + * how to flip it silently falls back to an Elasticsearch `must_not`, which MATCHES a document + * lacking the field — the semantics this story exists to remove. + * + * The rule has no allow-list: if the constructor declares a `maybeNot` field, the body must + * override `negated`. Shown failing by deleting one override (measured: removing + * `BetweenExpr.negated` reddens this with that class named). + * + * ⚠️ SCOPE, stated rather than implied. `MatchCriteria` and `MultiMatchCriteria` declare NO + * `maybeNot` field — a full-text match has no negated spelling of its own — so they are outside + * this rule by construction, and `NOT match(x) AGAINST ('y')` still takes the `must_not` route. + * MEASURED live on ES 8.18.3: it matches a document that does not carry the field. That is the + * same deviation the bare-column `NOT status = 'A'` route has, it is pinned in + * `GatewayApiIntegrationSpec`, and it is NOT fixed here: giving `MatchCriteria` a `maybeNot` + * changes its arity and its `.sql` render (which `MaterializedViewExtension` persists) and needs + * a new bridge arm emitting `must_not(match) + exists(field)` — too much for the round that + * closes this story, and the lead's call, not the dev's. + */ + "every criteria class that can carry its own NOT" should "say how to flip it" in { + def root: java.io.File = { + var d = new java.io.File(".").getAbsoluteFile + while (d != null && !new java.io.File(d, "build.sbt").isFile) d = d.getParentFile + if (d == null) fail("could not locate the build root") else d + } + def scalaFilesUnder(dir: java.io.File): Seq[java.io.File] = + if (!dir.isDirectory) Nil + else + Option(dir.listFiles).toSeq.flatten.flatMap { f => + if (f.isDirectory) scalaFilesUnder(f) + else if (f.getName.endsWith(".scala")) Seq(f) + else Nil + } + val sources = scalaFilesUnder(new java.io.File(root, "sql/src/main")) + sources should not be empty + val offenders = sources.flatMap { f => + val text = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") + .replaceAll("(?s)/\\*.*?\\*/", " ") + .replaceAll("(?m)//.*$", " ") + // Each top-level `case class` body, up to the next top-level definition. + val starts = """(?m)^case class (\w+)\(""".r.findAllMatchIn(text).toSeq + starts.zipWithIndex.flatMap { case (m, i) => + val end = if (i + 1 < starts.size) starts(i + 1).start else text.length + val body = text.substring(m.start, end) + val declaresMaybeNot = + """(?m)^\s*(override\s+)?(val\s+)?maybeNot:\s*Option\[NOT\.type\]""".r + .findFirstIn(body) + .isDefined + if (declaresMaybeNot && !body.contains("override def negated")) + Some(s"${m.group(1)} (${f.getName})") + else None + } + } + withClue( + "these criteria classes declare a `maybeNot` field but never override `negated`, so a " + + "predicate's NOT over them falls back to an Elasticsearch `must_not`, which MATCHES a " + + "document lacking the field:\n " + offenders.mkString("\n ") + "\n" + )(offenders shouldBe empty) + } + } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index e995d75a6..dd772becf 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -2609,6 +2609,34 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { id -> v } shouldBe Seq(1 -> "1", 2 -> "null", 3 -> "null") + // 🔴 Round 11, M-2 — `%%` means what `%` means, but the fast-path test stripped only ONE + // leading and ONE trailing `%`, so `'%%A'` fell through to a regex. MEASURED on live ES 8.18.3 + // before the fix: `circuit_breaking_exception: Regular expression considered too many + // characters`; on 6.8 the same shape is `Regexes are disabled`. Collapsing the runs first makes + // the documented rule ("no `_`, `%` only at the ends") TRUE as written. + ids("WHERE UPPER(status) LIKE '%%A'") shouldBe Seq(1) + ids("WHERE UPPER(status) LIKE 'A%%'") shouldBe Seq(1) + ids("WHERE UPPER(status) LIKE '%%'") shouldBe Seq(1, 2) + + // 🔴 Round 11, M-1 — a `CASE … END` with no `ELSE` is NULL-valued, and until now it reached a + // comparison unguarded. `= 1` hid it (Painless tolerates `null == 1`); everything else did not. + // MEASURED before the fix: `> 0` gave `Cannot invoke "Object.getClass()" because "leftObject" + // is null` and the string form `cannot access method/field [compareTo] from a null def + // reference`. Row 3 carries no `status`, so its CASE is NULL and it must not match either. + ids("WHERE (CASE WHEN UPPER(status) = 'A' THEN 1 END) > 0") shouldBe Seq(1) + ids("WHERE (CASE WHEN UPPER(status) = 'A' THEN 'y' END) = 'y'") shouldBe Seq(1) + ids("WHERE (CASE WHEN UPPER(status) = 'A' THEN 1 END) = 1") shouldBe Seq(1) + + // 🔴 Round 11, M-3 — A DEVIATION PINNED AS IT IS, NOT AS IT SHOULD BE. A full-text `MATCH` has + // no negated spelling of its own, so `NOT match(...)` takes Elasticsearch's `must_not`, which + // INCLUDES a document lacking the field: row 3 has no `status`, and ANSI would leave it out + // (`NOT UNKNOWN` is UNKNOWN). Same family as the bare-column `NOT status = 'A'` pinned above. + // NOT fixed in this story: it needs a `maybeNot` field on `MatchCriteria` (arity + `.sql` + // render, which `MaterializedViewExtension` persists) and a bridge arm emitting + // `must_not(match) + exists(field)` — the lead's call. `PainlessOperandFormSpec`'s class-axis + // gate names the exemption explicitly rather than leaving it silent. + ids("WHERE amount = 50 AND NOT match(status) against ('A')") shouldBe Seq(2, 3) + assertDdl(System.nanoTime(), client.run("DROP TABLE IF EXISTS tvl_orders").futureValue) } From 4a51f245a5a1bb91d98dfc9f9a71e07c38227096 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sat, 12 Sep 2026 12:49:19 +0200 Subject: [PATCH 10/10] test(BIDC-8): gate the LIKE shapes that need a Painless regex on ES 7+ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this, not me (run 34683904367): the ANSI three-valued test asserted `WHERE UPPER(status) LIKE 'A_'` on every major, and it fails on BOTH ES 6 clients with `illegal_state_exception: Regexes are disabled. Set [script.painless.regex.enabled] to [true]`. The product behaviour is right and already documented — a LIKE compiles to whitelisted String methods only when the pattern has no `_` and uses `%` at the ends alone, everything else becomes a regex, and 6.x ships that setting off. The defect was the TEST: it certified a shape on a configuration no 6.8 user has. ⚠️ And a correction to this story's own record. Round 9 reported this suite green on 6.8.23 / 7.17.29 / 8.18.3 / 9.0.3, which was true OF ROUND 9's CONTENT — every LIKE shape then took the string-method path. Round 10 ADDED the `_` shape and re-ran only 8.18.3, so the 6.8 claim was carried forward stale rather than re-derived. Re-derive after the content changes; CI is the authority. The fix is the capability idiom this suite already uses for enrich policies: `ElasticsearchVersion.supportsPainlessRegex` (false below 7.0), a `supportsPainlessRegex` accessor beside `supportsEnrichPolicies`, and the test SPLIT — every shape that takes the string-method fast path stays asserted on all majors, and only the regex-requiring ones move to a capability-gated test that cancels with its reason visible in the log. NOT done, deliberately: enabling `script.painless.regex.enabled` in the ES 6 fixture. That would make the suite certify a cluster nobody runs — the "gate satisfied by the shape you chose" failure this story has already repeated twice. The version boundary is also pinned in `ElasticsearchVersionSpec`, because a live run only ever covers the majors someone remembered to run. Verified on the failing surface only: es6rest and es6jest — the previously red test passes, the gated one cancels with its reason; es8java — BOTH pass, 0 cancelled, so the gate does not skip where the capability holds. `core/testOnly *ElasticsearchVersionSpec` 14/14. The other `==~`-emitting specs (PainlessNullSurvivalSpec, PainlessOperandFormSpec, LikePatternSpec) assert on the EMISSION and never reach Elasticsearch, so they have no exposure — checked, not assumed. Co-Authored-By: Claude Opus 5 --- .../elastic/client/ElasticsearchVersion.scala | 19 ++++++ .../client/ElasticsearchVersionSpec.scala | 20 ++++++ .../client/GatewayApiIntegrationSpec.scala | 66 ++++++++++++++++++- .../client/GatewayIntegrationTestKit.scala | 9 +++ 4 files changed, 112 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ElasticsearchVersion.scala b/core/src/main/scala/app/softnetwork/elastic/client/ElasticsearchVersion.scala index 7a4e10ec4..ecd3ea419 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ElasticsearchVersion.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ElasticsearchVersion.scala @@ -146,6 +146,25 @@ object ElasticsearchVersion { isAtLeast(version, 7, 16) } + /** Painless REGULAR EXPRESSIONS are usable in a script (ES >= 7.0). + * + * 🔴 A CONFIGURATION gate, not a feature one, and that is why it is easy to get wrong. Regex + * literals exist in every supported Painless, but `script.painless.regex.enabled` defaults to + * `false` on Elasticsearch 6.x and to `limited` from 7.0. So on a STOCK 6.8 cluster a script + * carrying `==~ /…/` is rejected at COMPILE time with `illegal_state_exception: Regexes are + * disabled. Set [script.painless.regex.enabled] to [true]`, while the identical script runs on + * 7.x and later. A cluster whose operator has turned the setting on would accept it — this + * predicate deliberately answers for the DEFAULT, because a test suite must certify the + * configuration users actually have. + * + * Story BIDC-8: `WHERE UPPER(x) LIKE 'A_'` is the shape that needs one — the engine compiles a + * `LIKE` to whitelisted `String` methods only when the pattern has no `_` and uses `%` at the + * ends alone; everything else becomes a regex. + */ + def supportsPainlessRegex(version: String): Boolean = { + isAtLeast(version, 7, 0) + } + /** Check if Composable Templates are supported (ES >= 7.8) */ def supportsComposableTemplates(version: String): Boolean = { diff --git a/core/src/test/scala/app/softnetwork/elastic/client/ElasticsearchVersionSpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/ElasticsearchVersionSpec.scala index c4514d7c1..60db3d92a 100644 --- a/core/src/test/scala/app/softnetwork/elastic/client/ElasticsearchVersionSpec.scala +++ b/core/src/test/scala/app/softnetwork/elastic/client/ElasticsearchVersionSpec.scala @@ -43,6 +43,26 @@ class ElasticsearchVersionSpec extends AnyWordSpec with Matchers { } } + "ElasticsearchVersion.supportsPainlessRegex" should { + // 🔴 Story BIDC-8. CI (run 34683904367) failed BOTH ES 6 clients on a `LIKE` pattern that + // compiles to a Painless regex: `illegal_state_exception: Regexes are disabled. Set + // [script.painless.regex.enabled] to [true]`. The boundary is a DEFAULT, not a feature — + // 6.x ships the setting `false`, 7.0 ships it `limited` — so it is pinned here as well as + // exercised live, because a live run only ever sees the majors someone remembered to run. + "return false for ES 6, where regexes are disabled by default" in { + ElasticsearchVersion.supportsPainlessRegex("6.8.23") shouldBe false + ElasticsearchVersion.supportsPainlessRegex("6.0.0") shouldBe false + ElasticsearchVersion.supportsPainlessRegex("5.6.0") shouldBe false + } + + "return true from ES 7.0 on" in { + ElasticsearchVersion.supportsPainlessRegex("7.0.0") shouldBe true + ElasticsearchVersion.supportsPainlessRegex("7.17.29") shouldBe true + ElasticsearchVersion.supportsPainlessRegex("8.18.3") shouldBe true + ElasticsearchVersion.supportsPainlessRegex("9.0.3") shouldBe true + } + } + "ElasticsearchVersion.supportsPit" should { "return true for ES >= 7.12" in { ElasticsearchVersion.supportsPit("7.12.0") shouldBe true diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index dd772becf..97589acfb 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -2588,9 +2588,10 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { // regex-dot one would match 'A' — the assertion distinguishes them. ids("WHERE status LIKE 'A.'") shouldBe Seq.empty[Int] ids("WHERE UPPER(status) LIKE 'A.'") shouldBe Seq.empty[Int] - ids("WHERE UPPER(status) LIKE 'A_'") shouldBe Seq - .empty[Int] // `_` IS a wildcard, but 'A' is 1 char ids("WHERE UPPER(status) LIKE 'A'") shouldBe Seq(1) + // ⚠️ The `_` wildcard is NOT asserted here — it compiles to a Painless regex, which stock + // Elasticsearch 6.8 refuses. It has its own capability-gated test below; everything in THIS + // test takes the whitelisted string-method path and therefore holds on every supported major. // 🔴 Round 10, MEDIUM-1 — `LIKE ''` over a function emitted `left1 ==~ //`, and `//` opens a // Painless COMMENT: `unexpected character [//))))]`. The native form always answered `[]`. @@ -2640,6 +2641,67 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { assertDdl(System.nanoTime(), client.run("DROP TABLE IF EXISTS tvl_orders").futureValue) } + /** 🔴 CI CAUGHT THIS, not me (run 34683904367): the shape below was asserted on every major and + * FAILS on both ES 6 clients with `illegal_state_exception: Regexes are disabled. Set + * [script.painless.regex.enabled] to [true]`. + * + * The product behaviour is correct and documented — a `LIKE` compiles to whitelisted `String` + * methods only when the pattern has no `_` and uses `%` at the ends alone, and everything else + * becomes a Painless regex, which 6.x disables BY DEFAULT. The defect was the TEST, which + * certified a shape on a configuration users of 6.8 do not have. + * + * ⚠️ And a correction to this story's own record: round 9 reported this ANSI suite green on + * 6.8.23 / 7.17.29 / 8.18.3 / 9.0.3, which was true OF ROUND 9's CONTENT — every `LIKE` shape + * then took the string-method path. Round 10 added the `_` shape and re-ran only 8.18.3, so the + * 6.8 claim was carried forward STALE rather than re-derived. CI is the authority. + * + * The fix is a capability gate, NOT enabling `script.painless.regex.enabled` in the ES 6 + * fixture: a suite that turns a non-default setting on certifies a cluster nobody runs. + */ + it should "apply the same rule to a LIKE pattern that needs a Painless regex" in { + assume( + supportsPainlessRegex, + "Painless regexes are disabled by default before Elasticsearch 7 " + + "(script.painless.regex.enabled), so a LIKE pattern that compiles to one cannot run here" + ) + val create = + """CREATE TABLE IF NOT EXISTS tvl_regex ( + | id INT, + | status KEYWORD, + | PRIMARY KEY (id) + |)""".stripMargin + assertDdl(System.nanoTime(), client.run(create).futureValue) + val jsonl = java.io.File.createTempFile("tvl_regex_", ".jsonl") + jsonl.deleteOnExit() + val writer = new java.io.PrintWriter(jsonl) + try { + writer.println("""{"id": 1, "status": "A"}""") + writer.println("""{"id": 2, "status": "AB"}""") + writer.println("""{"id": 3}""") // the `status` field is ABSENT + } finally writer.close() + assertDml( + System.nanoTime(), + client.run(s"""COPY INTO tvl_regex FROM "${jsonl.getAbsolutePath}";""").futureValue + ) + + def ids(where: String): Seq[Int] = + collectRows( + System.nanoTime(), + client.run(s"SELECT id FROM tvl_regex $where").futureValue + ).flatMap(_.get("id").map(_.toString.toDouble.toInt)).sorted + + // `_` is exactly ONE character, so `'A_'` matches `AB` and not `A` — and the row with no + // `status` matches neither, which is the ANSI rule this whole suite is about. + ids("WHERE UPPER(status) LIKE 'A_'") shouldBe Seq(2) + ids("WHERE UPPER(status) LIKE 'A'") shouldBe Seq(1) + // A `%` that is NOT at an end also needs the regex, and this is the shape whose "pure `%` + // patterns are safe everywhere" claim round 10 had to retract. + ids("WHERE UPPER(status) LIKE 'A%B'") shouldBe Seq(2) + ids("WHERE UPPER(status) NOT LIKE 'A_'") shouldBe Seq(1) // NOT the absent row — ANSI + + assertDdl(System.nanoTime(), client.run("DROP TABLE IF EXISTS tvl_regex").futureValue) + } + behavior of "POLICIES statements" it should "create, show, execute and drop a policy" in { diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayIntegrationTestKit.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayIntegrationTestKit.scala index 1823f2a75..ee54eafbd 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayIntegrationTestKit.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayIntegrationTestKit.scala @@ -68,6 +68,15 @@ trait GatewayIntegrationTestKit extends AnyFlatSpecLike with Matchers with Scala } } + def supportsPainlessRegex: Boolean = { + client.asInstanceOf[VersionApi].version match { + case ElasticSuccess(v) => ElasticsearchVersion.supportsPainlessRegex(v) + case ElasticFailure(error) => + log.error(s"❌ Failed to retrieve Elasticsearch version: ${error.message}") + false + } + } + def supportsQueryWatchers: Boolean = { client.asInstanceOf[VersionApi].version match { case ElasticSuccess(v) => ElasticsearchVersion.supportsQueryWatchers(v)