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/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..1232ec711 --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -0,0 +1,209 @@ +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 :" + } + + /** ⚠️ 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 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". + */ + "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 + } + } + + /** 🔴 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 + // 🔴 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 => + val raw = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") + val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") + val names = boundToPredicate.flatMap(_.findAllMatchIn(code).map(_.group(1))).toSet + val namedUses = names.toSeq.map { n => + s"""(? classifications) + Some( + s"${f.getPath.substring(root.getPath.length)} ($uses uses, $classifications classified)" + ) + else None + } + withClue( + "these files read a predicate's `not` without saying which side of the fold they are on. " + + "Consult `notConsumed` (fold: the criterion is evaluated over the SAME document), or write " + + "a `NOT-FOLD:` comment saying why it must not (round 10 measured one such case: under a " + + "nested relation the negation is outside an EXISTENTIAL and folding it changes the " + + "question). Silence is the bug this gate exists for:\n " + offenders.mkString("\n ") + "\n" + )(offenders shouldBe empty) + } + +} 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/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/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 228facf02..d5a6ccb20 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -410,6 +410,68 @@ 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. +> +> 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: +> `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. +> +> 🔴 **`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 `%` +> **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** ```sql diff --git a/documentation/sql/joins.md b/documentation/sql/joins.md index 458b724f5..faa617f96 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. 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`. +### 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). 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 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/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/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..1232ec711 --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -0,0 +1,209 @@ +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 :" + } + + /** ⚠️ 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 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". + */ + "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 + } + } + + /** 🔴 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 + // 🔴 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 => + val raw = new String(java.nio.file.Files.readAllBytes(f.toPath), "UTF-8") + val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") + val names = boundToPredicate.flatMap(_.findAllMatchIn(code).map(_.group(1))).toSet + val namedUses = names.toSeq.map { n => + s"""(? classifications) + Some( + s"${f.getPath.substring(root.getPath.length)} ($uses uses, $classifications classified)" + ) + else None + } + withClue( + "these files read a predicate's `not` without saying which side of the fold they are on. " + + "Consult `notConsumed` (fold: the criterion is evaluated over the SAME document), or write " + + "a `NOT-FOLD:` comment saying why it must not (round 10 measured one such case: under a " + + "nested relation the negation is outside an EXISTENTIAL and folding it changes the " + + "question). Silence is the bug this gate exists for:\n " + offenders.mkString("\n ") + "\n" + )(offenders shouldBe empty) + } + +} 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/function/cond/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala index 5a4382e32..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 @@ -283,6 +283,23 @@ package object cond { } } + /** A result rendering that can sit opposite a `null` branch. + * + * 🔴 Story BIDC-8 (round 10, MEDIUM-3). With no `ELSE`, SQL says the missing branch is NULL — + * but Painless types a ternary from its BRANCHES, so `param2 ? 1 : null` is + * `class_cast_exception: Cannot cast from [int] to [java.lang.Object]` (MEASURED live on ES + * 8.18.3, and MEASURED again to confirm it is the absence of a `return` that makes it fatal: + * the same expression compiles behind an explicit `return`). `(def)` on the result side is the + * spelling that compiles in BOTH positions — also measured. Applied only when there is no + * default, so every existing emission is byte-identical. + */ + private def boxWhenNoDefault(rendered: String): String = + // 🔴 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 { case Some(ctx) => @@ -307,7 +324,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 +334,7 @@ package object cond { ) case _ => SQLTypeUtils.coerce(res, out, context) - } + }) expParam match { case Some(e) => if (cond.nullable) { @@ -348,7 +365,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 +375,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 +398,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/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 b5c06a59e..b73cca9d7 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 } @@ -468,6 +476,32 @@ 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: 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. + * + * 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 = { + _locals += 1 + val name = s"$prefix${_locals}" + _declarations = _declarations :+ Right(name -> expr) + name + } + def exists(token: Token): Boolean = { token match { case param: PainlessParam => _keys.contains(param) @@ -476,9 +510,10 @@ package object sql { } } - def isEmpty: Boolean = _keys.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 @@ -494,18 +529,17 @@ package object sql { else s"${param.param}${param.painlessMethods.mkString("")}" - override def toString: String = { - if (isEmpty) "" - else - _keys - .flatMap { param => + 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("") - } + case Right((name, expr)) => Some(s"def $name = $expr; ") + } + .mkString("") } trait PainlessParams extends PainlessScript { @@ -1066,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 @@ -1556,8 +1617,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..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[_]]] = @@ -947,16 +951,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..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 @@ -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 @@ -445,7 +446,55 @@ 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 + * 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; + * `validate()` rejects that shape since BIDC-8. + */ + 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 +538,69 @@ 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 + // 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 ${ref(a)} JOIN ${ref(b)} ON $a. = $b." + ) + case None => + // 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( + 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/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/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/Where.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala index fca076ab3..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 @@ -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,16 +220,28 @@ 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) => - val leftStr = left.painless(context) - val rightStr = right.painless(context) + 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 + // `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") } - 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)) @@ -259,19 +286,59 @@ 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. + */ + /** 🔴 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[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))) } } @@ -362,6 +429,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" @@ -453,22 +521,119 @@ 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 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: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 + * 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: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 + * **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, 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 + // 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 { + 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 _ => "" @@ -509,10 +674,36 @@ 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 { - case comparison: ComparisonOperator => + 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 + // 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 = 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 => @@ -563,8 +754,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)" } } @@ -626,22 +817,137 @@ 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) => + // 🔴 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)) + 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 _ => } - if (identifier.nullable) { - return s"def left = $innerLeft; left == null ? false : $painlessNot(${check(context, "left")})" - } - s"$painlessNot${check(context, innerLeft)}" + + // 🔴 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: `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`. + // + // 🔴 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] = { @@ -674,6 +980,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 { @@ -698,6 +1007,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) { @@ -716,6 +1028,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) { @@ -734,6 +1048,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) { @@ -782,7 +1098,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" } @@ -813,7 +1138,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" } @@ -825,6 +1159,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" @@ -882,21 +1220,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(" || ") } @@ -905,6 +1255,20 @@ case class BetweenExpr( fromTo: FromTo, maybeNot: Option[NOT.type] ) extends Expression { + + /** 🔴 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 = { @@ -927,33 +1291,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/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..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 @@ -341,14 +341,34 @@ 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 "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 { @@ -448,12 +468,19 @@ 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. + 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" - ).isRight shouldBe true + 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" + ) } /** 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/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/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..367a941f8 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/PainlessOperandFormSpec.scala @@ -0,0 +1,216 @@ +/* + * 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") + } + } + + /** 🔴 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/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..f5b521423 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/SelfJoinAliasSpec.scala @@ -0,0 +1,212 @@ +/* + * 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" + ) + // 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""" + ) + } + + 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" + ) + } + + behavior of "one alias for two sources (review M1)" + + 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 a.x FROM orders a JOIN orders a ON a.id = a.parent_id", + "Alias 'a' is used for more than one table" + ) + 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)" + + 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") + // 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":{ 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..97589acfb 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,290 @@ 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] + + // 🔴 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") + + // 🔴 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(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 `[]`. + 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") + + // 🔴 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) + } + + /** 🔴 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)