From 14d7a16592c46c01c182cef30bc37e5c75dc156f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Thu, 24 Sep 2026 10:19:21 +0200 Subject: [PATCH 1/6] =?UTF-8?q?fix(sql):=20a=20HAVING=20over=20a=20functio?= =?UTF-8?q?n=20of=20an=20aggregate=20must=20filter,=20or=20fail=20?= =?UTF-8?q?=E2=80=94=20never=20vanish?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HAVING ABS(COUNT(*)) > 1` parsed, ran, and returned EVERY group with HTTP 200 — and so did `COALESCE`, `NULLIF`, `ROUND`, `GREATEST`, a `CASE`, an aggregate on the RIGHT of the comparison, and the whole-table form. The #205/#209/#253 silent-wrong-answer family. Cause, in one sentence: a function never looks inside its own ARGUMENTS. `FunctionChain .hasAggregation` reads the chain's OWN links, so `COUNT(*)` inside `ABS(...)` made the tree answer "no aggregate here" — no aggregation was created, `buckets_path` published nothing, and the selector fell to its `1 == 1` catch-all, which the caller reads as "nothing to filter". ONE derivation, `Identifier.bucketMetrics`, now feeds all three consumers that used to disagree: what `buckets_path` publishes, which aggregations get created, and the `bucket_selector` null guard. It needs TWO walks — `funIdentifiers` reaches a function argument and a `CASE`'s THEN result, while a `CASE`'s WHEN conditions are reachable only through `Case.conditionsOf`. Everything the engine can express is emitted; everything it cannot is REFUSED BY NAME at `SingleSearch.validate()`, inside `Parser.apply`, so every venue sees it. A partial emission counts as a wrong answer: one un-expressible conjunct refuses the whole statement. The representability gate decides on the RENDERING, never on a list of function names — a name list is a fourth derivation of what the renderer does, and it could not have expressed the rule that only EXECUTION produced: the bucket-pipeline Painless context does NOT compile `Double.valueOf(…)`, so `ABS`/`FLOOR`/`CEIL`/`SQRT`/`EXP`/`LOG`/`POWER` over an aggregate are refused, even though the same function works in WHERE, in the SELECT list and in ORDER BY. The spec asserted the opposite; eighteen shapes were rendered and executed against a real cluster to settle it. Fixed on the way: `HAVING COUNT(*) > ABS(MAX(x))` emitted a script reading `params.max_x` with no `max_x` aggregation anywhere; the nested form emitted NO aggregation at all and returned raw documents; an OR-disjunct was dropped as well as an AND-conjunct; and `WHERE ABS(COUNT(*)) > 1` now hits the existing aggregates-in-WHERE rejection. No bridge file changed — the whole fix is in `sql`, proved by holding the hand-maintained es6 twin to the same emission matrix. Verified: 19/19 mutations RED · sql 1607 / core 1141 / bridge 225 / es6bridge 221 · `+ compile` both Scala legs · GrammarDiffProbe over 18,165 inputs = 4 differing rows, every one intended · integration 32/32 on real Elasticsearch 8.18.3 and 32/32 on 6.8.23 · cost, 5 interleaved rounds vs a 455433ae control: parse+emit −0.39 % against a must-be-zero baseline reading −0.08 % ⇒ no signal. Closed Issue #389 Co-Authored-By: Claude Opus 5 (1M context) --- .../sql/HavingFunctionEmissionSpec.scala | 284 ++++++++++++ documentation/sql/dql_statements.md | 29 +- .../sql/HavingFunctionEmissionSpec.scala | 291 ++++++++++++ .../app/softnetwork/elastic/sql/package.scala | 67 ++- .../elastic/sql/query/GroupBy.scala | 302 +++++++++++-- .../elastic/sql/query/Having.scala | 29 +- .../softnetwork/elastic/sql/query/Where.scala | 62 ++- .../elastic/sql/query/package.scala | 47 ++ .../HavingOverAggregateFunctionSpec.scala | 413 ++++++++++++++++++ .../client/GroupByCompletenessSpec.scala | 156 +++++++ 10 files changed, 1620 insertions(+), 60 deletions(-) create mode 100644 bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala create mode 100644 es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala new file mode 100644 index 000000000..78b134bb2 --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -0,0 +1,284 @@ +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 com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime +import scala.jdk.CollectionConverters._ + +/** Issue #389 -- the EMITTED Elasticsearch query for a `HAVING` over a FUNCTION of an aggregate. + * + * The `sql` half (detection, the representability gate, the refusals) is pinned in + * `HavingOverAggregateFunctionSpec`. What is measured HERE is what Elasticsearch is actually sent: + * the `bucket_selector`, its `buckets_path`, and the metric aggregation the script reads. Those + * are the three things that used to disagree -- the defect was invisible to a parse, because every + * one of these statements parsed, ran, and returned EVERY group with HTTP 200. + * + * 🔴 Every shape named in `emitted` below was EXECUTED against a real Elasticsearch 8.18.3 before + * it was written here, and every shape in `refused` was measured to FAIL there. That is not + * ceremony: the spec asserted that `Double.valueOf(Math.abs(params.c)) > 1` was a legal + * `bucket_selector` source, and Elasticsearch rejects it at COMPILE time. A byte pin would have + * certified it (`feedback_assert_the_mechanism_not_a_proxy` -- PARSES != RENDERS != RUNS). + * + * The file closes with a structural invariant over every shape it names: a `bucket_selector` may + * read no `params.` its own `buckets_path` does not declare, and must null-test every metric it + * declares. `metricSelectorForBucket`'s own comment asserts the first half; it is now load-bearing + * for a new population, so it is asserted rather than assumed. + */ +class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { + + import scala.language.implicitConversions + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + implicit def sqlQueryToRequest(sqlQuery: SelectStatement): ElasticSearchRequest = + sqlQuery.statement match { + case Some(value: SingleSearch) => value.copy(score = sqlQuery.score) + case other => throw new IllegalArgumentException(s"Not a single search: $other") + } + + private val mapper = new ObjectMapper() + + private def queryOf(sql: String): String = { + val select: ElasticSearchRequest = SelectStatement(sql) + select.query + } + + private val terms = """"terms":{"field":"status","size":65536,"min_doc_count":1}""" + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " + + /** Every shape this file emits -- the input of the structural invariant at the bottom, so the + * guard is computed over the material it guards rather than over a hand-picked subset. + */ + private val emitted: Seq[String] = Seq( + group + "COUNT(*) > 1", + group + "COALESCE(COUNT(*), 0) > 1", + group + "GREATEST(COUNT(*), 0) > 1", + group + "LEAST(COUNT(*), 99) > 1", + group + "SIGN(COUNT(*)) > 0", + group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2", + group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5", + group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5", + group + "GREATEST(COUNT(*), 0) IN (1, 2)", + group + "NOT GREATEST(COUNT(*), 0) > 1", + group + "1 < GREATEST(COUNT(*), 0)", + group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'", + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)", + "SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING COALESCE(COUNT(*), 0) > 1", + "SELECT status, MAX(x) - MIN(x) AS d FROM t GROUP BY status HAVING d > 3", + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) + + // --------------------------------------------------------------------------------------------- + // S1 / S2 -- the shapes the engine CAN express now emit a selector (AC-1) + // --------------------------------------------------------------------------------------------- + + "COALESCE over an aggregate" should "emit a bucket_selector reading the published metric" in { + queryOf(group + "COALESCE(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c != null ? params.c : 0) > 1"}}}}}}}""" + ).mkString + } + + "GREATEST over an aggregate" should "emit a bucket_selector, null-guarded" in { + queryOf(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. + // --------------------------------------------------------------------------------------------- + + "a conjunction of a bare and a wrapped aggregate" should "emit both conditions" in { + queryOf(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") should include( + """"script":{"source":"(params.c == null ? false : (params.c > 1)) && """ + + """(params.c == null ? false : (Math.max(params.c, 0) > 2))"}""" + ) + } + + "a disjunction" should "emit both conditions" in { + // Dropping a disjunct makes the filter STRICTER than written: rows disappear silently. + queryOf(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") should include( + """(params.c == null ? false : (Math.max(params.c, 0) > 1)) || """ + + """(params.c == null ? false : (params.c > 5))""" + ) + } + + "a wrapped aggregate beside a bucket-key predicate" should "honour BOTH mechanisms" in { + // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may + // cost the other. + val q = queryOf(group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'") + q should include(""""exclude":["x"]""") + q should include("""(params.c != null ? params.c : 0) > 1""") + } + + // --------------------------------------------------------------------------------------------- + // S9 -- the aggregation reached ONLY through the HAVING function must be CREATED (AC-5) + // --------------------------------------------------------------------------------------------- + + "an aggregate reached only through a HAVING function" should "create its aggregation" in { + queryOf("SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"count_all":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_all":"count_all"},""", + """"script":{"source":"(params.count_all == null ? false : """, + """(Math.max(params.count_all, 0) > 1))"}}}}}}}""" + ).mkString + } + + it should "create it inside a NESTED relation too" in { + // 🔴 MEASURED on `main`: this statement emitted `{"query":{"match_all":{}},"_source":true}` -- + // no aggregation AT ALL. The GROUP BY itself vanished and raw documents came back. + queryOf( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.name":{"terms":{"field":"emails.name","size":65536,"min_doc_count":1},""", + """"aggs":{"count_e_address":{"value_count":{"field":"emails.address"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_e_address":"count_e_address"},""", + """"script":{"source":"(params.count_e_address == null ? false : """, + """(Math.max(params.count_e_address, 0) > 1))"}}}}}}}}}""" + ).mkString + } + + "an aggregate reached only through the RIGHT operand" should "create and guard its aggregation" in { + // 🔴 MEASURED on `main`: the script read `params.max_x`, `buckets_path` declared only `c`, and + // no `max_x` aggregation existed -- so Elasticsearch ran `Math.abs(null)`. + queryOf( + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c","max_x":"max_x"},""", + """"script":{"source":"(params.c == null || params.max_x == null ? false : """, + """(params.c > Math.max(params.max_x, 0)))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // S8 -- the whole-table HAVING follows the same rule (AC-4) + // --------------------------------------------------------------------------------------------- + + "a whole-table HAVING over a function of an aggregate" should "hang the selector off the synthetic bucket" in { + queryOf("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,""", + """"aggs":{"__whole_table_having__":{"filters":{"filters":{"_all":{"match_all":{}}}},""", + """"aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // AC-2 -- nothing in the matrix emits a query with the predicate missing + // --------------------------------------------------------------------------------------------- + + private val refused: Seq[String] = Seq( + group + "ABS(COUNT(*)) > 1", + group + "FLOOR(COUNT(*)) > 1", + group + "NULLIF(COUNT(*), 0) > 1", + group + "NULLIF(c, 0) > 1", + group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1", + group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1", + group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2", + "SELECT status, MAX(amount) AS m FROM t GROUP BY status HAVING ROUND(MAX(amount), 2) > 1", + "SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1" + ) + + "a HAVING the engine cannot express" should "never reach emission" in { + refused.foreach { sql => + withClue(s"[$sql] ") { + Parser(sql) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(s) => + fail(s"expected a rejection; it emitted ${queryOf(sql)} from $s") + } + } + } + } + + "every statement that DOES emit" should "carry a having_filter" in { + // Non-vacuity: the invariant below would pass on an empty set, which is exactly the shape the + // defect produced. + emitted.foreach { sql => + withClue(s"[$sql] ") { + valuesOf(mapper.readTree(queryOf(sql)), "bucket_selector") should not be empty + } + } + } + + // --------------------------------------------------------------------------------------------- + // The structural invariant (AD-2 item 3) + // --------------------------------------------------------------------------------------------- + + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { + val here = Option(node.get(key)).toSeq + val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) + here ++ below + } + + "every emitted bucket pipeline" should "read exactly the metrics its buckets_path declares" in { + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") + pipelines should not be empty + pipelines.foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = Option(pipeline.get("script")) + .map { s => + if (s.isTextual) s.asText() else s.get("source").asText() + } + .getOrElse(fail("a bucket pipeline with no script")) + val read = paramRef.findAllMatchIn(source).map(_.group(1)).toSet - "__now__" + read shouldBe declared + } + } + } + } + + it should "null-test every metric a bucket_selector declares" in { + // The AC-4b contract, restated for the #389 population: a selector either guards the metric + // (`params.x == null ? false : ...`) or handles the null itself (`params.x != null ? ... : k`, + // which is what COALESCE means). Dereferencing it unguarded fails the whole search. + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = pipeline.get("script").get("source").asText() + declared.foreach { key => + withClue(s"metric '$key' in [$source] ") { + assert( + source.contains(s"params.$key == null") || source.contains(s"params.$key != null"), + "is never null-tested" + ) + } + } + } + } + } + } +} diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index d5a6ccb20..7b59433f5 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -778,8 +778,33 @@ ORDER BY COUNT(*) DESC; price_range > 10`); `BETWEEN`, `IN` and `NOT` apply to aggregates as to columns. - Rejected with an explicit error: arithmetic over aggregates written inline in `HAVING` (`HAVING MAX(price) - MIN(price) > 10` — alias it in `SELECT` and reference the alias), an - aggregate function inside `WHERE` (use `HAVING`), and an alias that names one aggregate in - `SELECT` and a different one in `HAVING` / `ORDER BY`. + aggregate function inside `WHERE` (use `HAVING`, and this covers a wrapped one such as + `WHERE ABS(COUNT(*)) > 1`), and an alias that names one aggregate in `SELECT` and a different one + in `HAVING` / `ORDER BY`. +- A **function of an aggregate** in `HAVING` is applied to the group, or the statement is rejected + by name — it is never ignored. `COALESCE`, `GREATEST`, `LEAST` and `SIGN` over an aggregate filter + the groups (`HAVING COALESCE(COUNT(*), 0) > 30`, `HAVING GREATEST(MAX(price), 0) > 100`), on + either side of the comparison and under `BETWEEN`, `IN` and `NOT`. Everything Elasticsearch + cannot evaluate as a group filter is refused with the reason: + - a rendering that can be NULL — `HAVING NULLIF(COUNT(*), 0) > 1`; + - a rendering that needs a local variable — `HAVING ROUND(SUM(price), 2) > 10`; + - a rendering that boxes a number — `HAVING ABS(COUNT(*)) > 1` and the rest of the numeric + function family (`FLOOR`, `CEIL`, `SQRT`, `EXP`, `LOG`, `POWER`); Elasticsearch's group-filter + script context does not compile the conversion, even though the same function works in `WHERE`, + in the `SELECT` list and in `ORDER BY`; + - `CASE ... END` in `HAVING`, which needs a document and a group filter has none; + - a function applied to a `SELECT` aggregate **alias** — `COUNT(*) AS c ... HAVING NULLIF(c, 0) > 1`. + + In every refused case the remedy is the same: **compare the aggregate itself** — + `HAVING SUM(price) > 10` rather than `HAVING ROUND(SUM(price), 2) > 10` — and apply the function + to the result outside the query. Aliasing the expression in `SELECT` does NOT help: a function of + an aggregate is not a valid `SELECT` item under a `GROUP BY` either + (`SELECT ABS(COUNT(*)) AS a ... GROUP BY city` is rejected as a non-aggregated field). That + differs from arithmetic over aggregates, which IS a valid `SELECT` item + (`MAX(price) - MIN(price) AS price_range`) and is the reason the rule above tells you to alias + THAT one. +- One un-expressible condition rejects the whole `HAVING`, never just its own half: a statement is + answered with every condition it was written with, or with an error. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter script null-checks every metric before comparing it. diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala new file mode 100644 index 000000000..06ed3afb5 --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -0,0 +1,291 @@ +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 com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime +import scala.jdk.CollectionConverters._ + +/** Issue #389 -- the EMITTED Elasticsearch query for a `HAVING` over a FUNCTION of an aggregate, + * measured against the HAND-MAINTAINED es6 bridge (AC-7). + * + * 🔴 The whole fix lives in the `sql` module: no line of either `ElasticAggregation.scala` moved. + * This twin is what PROVES that -- it is the same matrix, asserted against a different elastic4s + * major, and the only expectation that differs is elastic4s 6's bare-string `terms` `exclude`. + * + * The `sql` half (detection, the representability gate, the refusals) is pinned in + * `HavingOverAggregateFunctionSpec`. What is measured HERE is what Elasticsearch is actually sent: + * the `bucket_selector`, its `buckets_path`, and the metric aggregation the script reads. Those + * are the three things that used to disagree -- the defect was invisible to a parse, because every + * one of these statements parsed, ran, and returned EVERY group with HTTP 200. + * + * 🔴 Every shape named in `emitted` below was EXECUTED against a real Elasticsearch 8.18.3 before + * it was written here, and every shape in `refused` was measured to FAIL there. That is not + * ceremony: the spec asserted that `Double.valueOf(Math.abs(params.c)) > 1` was a legal + * `bucket_selector` source, and Elasticsearch rejects it at COMPILE time. A byte pin would have + * certified it (`feedback_assert_the_mechanism_not_a_proxy` -- PARSES != RENDERS != RUNS). + * + * The file closes with a structural invariant over every shape it names: a `bucket_selector` may + * read no `params.` its own `buckets_path` does not declare, and must null-test every metric it + * declares. `metricSelectorForBucket`'s own comment asserts the first half; it is now load-bearing + * for a new population, so it is asserted rather than assumed. + */ +class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { + + import scala.language.implicitConversions + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + implicit def sqlQueryToRequest(sqlQuery: SelectStatement): ElasticSearchRequest = + sqlQuery.statement match { + case Some(value: SingleSearch) => value.copy(score = sqlQuery.score) + case other => throw new IllegalArgumentException(s"Not a single search: $other") + } + + private val mapper = new ObjectMapper() + + private def queryOf(sql: String): String = { + val select: ElasticSearchRequest = SelectStatement(sql) + select.query + } + + private val terms = """"terms":{"field":"status","size":65536,"min_doc_count":1}""" + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " + + /** Every shape this file emits -- the input of the structural invariant at the bottom, so the + * guard is computed over the material it guards rather than over a hand-picked subset. + */ + private val emitted: Seq[String] = Seq( + group + "COUNT(*) > 1", + group + "COALESCE(COUNT(*), 0) > 1", + group + "GREATEST(COUNT(*), 0) > 1", + group + "LEAST(COUNT(*), 99) > 1", + group + "SIGN(COUNT(*)) > 0", + group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2", + group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5", + group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5", + group + "GREATEST(COUNT(*), 0) IN (1, 2)", + group + "NOT GREATEST(COUNT(*), 0) > 1", + group + "1 < GREATEST(COUNT(*), 0)", + group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'", + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)", + "SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING COALESCE(COUNT(*), 0) > 1", + "SELECT status, MAX(x) - MIN(x) AS d FROM t GROUP BY status HAVING d > 3", + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) + + // --------------------------------------------------------------------------------------------- + // S1 / S2 -- the shapes the engine CAN express now emit a selector (AC-1) + // --------------------------------------------------------------------------------------------- + + "COALESCE over an aggregate" should "emit a bucket_selector reading the published metric" in { + queryOf(group + "COALESCE(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c != null ? params.c : 0) > 1"}}}}}}}""" + ).mkString + } + + "GREATEST over an aggregate" should "emit a bucket_selector, null-guarded" in { + queryOf(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. + // --------------------------------------------------------------------------------------------- + + "a conjunction of a bare and a wrapped aggregate" should "emit both conditions" in { + queryOf(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") should include( + """"script":{"source":"(params.c == null ? false : (params.c > 1)) && """ + + """(params.c == null ? false : (Math.max(params.c, 0) > 2))"}""" + ) + } + + "a disjunction" should "emit both conditions" in { + // Dropping a disjunct makes the filter STRICTER than written: rows disappear silently. + queryOf(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") should include( + """(params.c == null ? false : (Math.max(params.c, 0) > 1)) || """ + + """(params.c == null ? false : (params.c > 5))""" + ) + } + + "a wrapped aggregate beside a bucket-key predicate" should "honour BOTH mechanisms" in { + // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may + // cost the other. + val q = queryOf(group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'") + // elastic4s 6 renders a single-value `exclude` as a bare string, 7+ as an array -- the ONE + // documented emission difference this matrix touches. + q should include(""""exclude":"x"""") + q should include("""(params.c != null ? params.c : 0) > 1""") + } + + // --------------------------------------------------------------------------------------------- + // S9 -- the aggregation reached ONLY through the HAVING function must be CREATED (AC-5) + // --------------------------------------------------------------------------------------------- + + "an aggregate reached only through a HAVING function" should "create its aggregation" in { + queryOf("SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"count_all":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_all":"count_all"},""", + """"script":{"source":"(params.count_all == null ? false : """, + """(Math.max(params.count_all, 0) > 1))"}}}}}}}""" + ).mkString + } + + it should "create it inside a NESTED relation too" in { + // 🔴 MEASURED on `main`: this statement emitted `{"query":{"match_all":{}},"_source":true}` -- + // no aggregation AT ALL. The GROUP BY itself vanished and raw documents came back. + queryOf( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.name":{"terms":{"field":"emails.name","size":65536,"min_doc_count":1},""", + """"aggs":{"count_e_address":{"value_count":{"field":"emails.address"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_e_address":"count_e_address"},""", + """"script":{"source":"(params.count_e_address == null ? false : """, + """(Math.max(params.count_e_address, 0) > 1))"}}}}}}}}}""" + ).mkString + } + + "an aggregate reached only through the RIGHT operand" should "create and guard its aggregation" in { + // 🔴 MEASURED on `main`: the script read `params.max_x`, `buckets_path` declared only `c`, and + // no `max_x` aggregation existed -- so Elasticsearch ran `Math.abs(null)`. + queryOf( + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c","max_x":"max_x"},""", + """"script":{"source":"(params.c == null || params.max_x == null ? false : """, + """(params.c > Math.max(params.max_x, 0)))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // S8 -- the whole-table HAVING follows the same rule (AC-4) + // --------------------------------------------------------------------------------------------- + + "a whole-table HAVING over a function of an aggregate" should "hang the selector off the synthetic bucket" in { + queryOf("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,""", + """"aggs":{"__whole_table_having__":{"filters":{"filters":{"_all":{"match_all":{}}}},""", + """"aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // AC-2 -- nothing in the matrix emits a query with the predicate missing + // --------------------------------------------------------------------------------------------- + + private val refused: Seq[String] = Seq( + group + "ABS(COUNT(*)) > 1", + group + "FLOOR(COUNT(*)) > 1", + group + "NULLIF(COUNT(*), 0) > 1", + group + "NULLIF(c, 0) > 1", + group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1", + group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1", + group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2", + "SELECT status, MAX(amount) AS m FROM t GROUP BY status HAVING ROUND(MAX(amount), 2) > 1", + "SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1" + ) + + "a HAVING the engine cannot express" should "never reach emission" in { + refused.foreach { sql => + withClue(s"[$sql] ") { + Parser(sql) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(s) => + fail(s"expected a rejection; it emitted ${queryOf(sql)} from $s") + } + } + } + } + + "every statement that DOES emit" should "carry a having_filter" in { + // Non-vacuity: the invariant below would pass on an empty set, which is exactly the shape the + // defect produced. + emitted.foreach { sql => + withClue(s"[$sql] ") { + valuesOf(mapper.readTree(queryOf(sql)), "bucket_selector") should not be empty + } + } + } + + // --------------------------------------------------------------------------------------------- + // The structural invariant (AD-2 item 3) + // --------------------------------------------------------------------------------------------- + + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { + val here = Option(node.get(key)).toSeq + val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) + here ++ below + } + + "every emitted bucket pipeline" should "read exactly the metrics its buckets_path declares" in { + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") + pipelines should not be empty + pipelines.foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = Option(pipeline.get("script")) + .map { s => + if (s.isTextual) s.asText() else s.get("source").asText() + } + .getOrElse(fail("a bucket pipeline with no script")) + val read = paramRef.findAllMatchIn(source).map(_.group(1)).toSet - "__now__" + read shouldBe declared + } + } + } + } + + it should "null-test every metric a bucket_selector declares" in { + // The AC-4b contract, restated for the #389 population: a selector either guards the metric + // (`params.x == null ? false : ...`) or handles the null itself (`params.x != null ? ... : k`, + // which is what COALESCE means). Dereferencing it unguarded fails the whole search. + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = pipeline.get("script").get("source").asText() + declared.foreach { key => + withClue(s"metric '$key' in [$source] ") { + assert( + source.contains(s"params.$key == null") || source.contains(s"params.$key != null"), + "is never null-tested" + ) + } + } + } + } + } + } +} 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 69a13d0b4..45037666d 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -1564,16 +1564,61 @@ package object sql { def bucketPath: String - lazy val allMetricsPath: Map[String, String] = { - metricName match { - case Some(name) => Map(name -> name) - // The alias of a SELECT `bucket_script` item referenced from HAVING (`... AS d ... HAVING - // d > 3`): the selector reads the sibling pipeline aggregation by that name. - case _ if hasAggregation && fieldAlias.isDefined => Map(aliasOrName -> aliasOrName) - case _ => Map.empty + /** Every aggregate this identifier's expression references ANYWHERE -- its own chain, the + * ARGUMENTS of every function in it, a `CASE`'s `THEN` results and its `WHEN` conditions + * (issue #389). + * + * 🔴 A function never looked inside its own arguments: [[FunctionChain.hasAggregation]] is + * `functions.exists(_.hasAggregation)` over the chain's OWN links, so `COUNT(*)` written + * inside `ABS(...)` / `COALESCE(...)` / `NULLIF(...)` made the whole tree answer "no aggregate + * here". That one fact is why the aggregation was never created, why `buckets_path` published + * nothing, and why the `bucket_selector` degenerated to `1 == 1` and every group came back. + * + * Two walks, because the grammar hides the aggregate in two different places and neither walk + * sees the other's: [[FunctionUtils.funIdentifiers]] descends `FunctionN.args` and the chain + * (so it reaches a `CASE`'s `THEN` result, which IS an argument), while a `CASE`'s `WHEN` + * conditions are deliberately excluded from `Case.args` and are reachable only through + * [[app.softnetwork.elastic.sql.function.cond.Case.conditionsOf]]. MEASURED: `CASE WHEN + * COUNT(*) > 1 THEN 1 ELSE 0 END` yields NO aggregate from `funIdentifiers` alone. + * + * Deduplicated by [[metricPathKey]] -- which for every element here IS [[metricName]], since + * `isAggregation` means `aggregateFunction.isDefined` and `metricName` is defined exactly + * then. Stating it as `metricPathKey` keeps ONE notion of metric identity across this + * derivation and `Expression.bucketMetrics`. + */ + lazy val referencedAggregates: Seq[Identifier] = { + val fromChain = FunctionUtils.funIdentifiers(this).filter(_.isAggregation) + val fromCaseConditions = function.cond.Case + .conditionsOf(this) + .flatMap(_.referencedIdentifiers) + .flatMap(id => if (id.isAggregation) Seq(id) else id.referencedAggregates) + // Deduplicated by [[metricPathKey]] -- the SAME key `Expression.bucketMetrics` dedups by, + // so the two cannot disagree about what "the same metric" means. + (fromChain ++ fromCaseConditions).foldLeft(Seq.empty[Identifier]) { (acc, id) => + if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id } } + /** The aggregations a bucket pipeline reading THIS identifier addresses, in order -- the ONE + * derivation behind [[allMetricsPath]] (what `buckets_path` publishes), behind + * `Criteria.extractAggregationFields` (which aggregations are created) and behind the + * `bucket_selector` null guard. Three answers to one question would be three answers that + * drift (`project_self_join_alias_resolution`). + * + * Three arms, and the third is issue #389's: + * 1. the identifier IS an aggregate -- one metric, itself; 2. it is the alias of a SELECT + * `bucket_script` item referenced from HAVING (`MAX(x) - MIN(x) AS d ... HAVING d > 3`) + * -- the selector reads that sibling pipeline aggregation by name and NOT its operands, so the + * operands must not be published here; 3. otherwise, every aggregate it references through a + * function argument or a `CASE`. + */ + lazy val bucketMetrics: Seq[Identifier] = + if (metricName.isDefined || (hasAggregation && fieldAlias.isDefined)) Seq(this) + else referencedAggregates + + lazy val allMetricsPath: Map[String, String] = + bucketMetrics.map(id => id.metricPathKey -> id.metricPathKey).toMap + override def sql: String = { var parts: Seq[String] = name.split("\\.").toSeq tableAlias match { @@ -1666,10 +1711,16 @@ package object sql { } } + /** The `buckets_path` key this identifier is addressed by: its derived [[metricName]] when it + * is an aggregate, else the alias a SELECT `bucket_script` item published it under. ONE + * derivation, read by [[metricParam]] and by [[allMetricsPath]]. + */ + lazy val metricPathKey: String = metricName.getOrElse(aliasOrName) + /** How a bucket pipeline script reads this aggregate: the metric Elasticsearch already * computed, published under `buckets_path` as [[metricName]]. */ - lazy val metricParam: String = s"params.${metricName.getOrElse(aliasOrName)}" + lazy val metricParam: String = s"params.$metricPathKey" lazy val script: Option[String] = if (isTemporal) { 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 70af12b1c..afaf06057 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 @@ -18,6 +18,8 @@ package app.softnetwork.elastic.sql.query import app.softnetwork.elastic.sql.`type`.SQLType import app.softnetwork.elastic.sql.operator._ +import scala.util.Try +import scala.util.matching.Regex import app.softnetwork.elastic.sql.{ quoteIdentifier, Expr, @@ -347,48 +349,119 @@ case class BucketPath(buckets: Seq[Bucket]) { override def toString: String = path } +/** What a `HAVING` criterion contributes to a bucket pipeline. THREE values, because `""` used to + * mean two things and the caller could not tell them apart (issue #389): "this level has nothing + * to filter" and "I cannot express this filter" both came back as the empty string, and the second + * was read as the first -- so a predicate the engine could not emit was silently DROPPED and every + * group came back with HTTP 200. + */ +sealed trait MetricSelector + +object MetricSelector { + + /** Nothing for the bucket pipeline to do at this level. Legitimate and common: a predicate over + * the bucket KEY is honoured by the `terms` `include` / `exclude` instead (`HAVING status = + * 'A'`), and a condition reading another level's metric belongs to that level. + */ + case object NoFilter extends MetricSelector + + /** A `bucket_selector` `script.source`: ONE boolean expression reading published metrics. */ + case class Filter(script: String) extends MetricSelector + + /** The predicate references an aggregate and the engine cannot express it as a `bucket_selector`. + * It is REFUSED by name at `Having.validate()`, never dropped. + */ + case class Unrepresentable(expression: String, reason: String) extends MetricSelector { + + /** 🔴 The remedy is "compare the aggregate itself", NOT "alias it in SELECT". The sibling rule + * on inline arithmetic (`HAVING MAX(x) - MIN(x) > 10`) does say to alias it, and that advice + * does NOT transfer: MEASURED, `SELECT ABS(COUNT(*)) AS a ... GROUP BY city` is itself + * rejected ("Non-aggregated fields ... cannot be selected when GROUP BY is present") for every + * member of this family, so the borrowed wording would send the user to a dead end. + */ + def message: String = + s"HAVING cannot be applied to $expression: $reason. " + + "Compare the aggregate itself (HAVING SUM(x) > 10 rather than HAVING ROUND(SUM(x), 2) > 10), " + + "or apply the function to the result outside the query." + } +} + object MetricSelectorScript { - def metricSelector(expr: Criteria): String = expr match { + import MetricSelector._ + + /** The bucket-pipeline script of a `HAVING` criterion, or `"1 == 1"` when there is nothing to + * filter at this level. + * + * 🔴 It THROWS on an [[MetricSelector.Unrepresentable]] criterion, and that is the point: + * returning `""` is what let a predicate vanish. `Having.validate()` refuses every such + * statement inside `Parser.apply`, so no venue can reach this throw with a parsed statement -- + * it is the invariant's second line of defence, for a `SingleSearch` assembled in code and + * emitted without validation. + */ + def metricSelector(expr: Criteria): String = selector(expr) match { + case NoFilter => "1 == 1" + case Filter(script) => script + case u: Unrepresentable => throw new IllegalStateException(u.message) + } + + /** Every `HAVING` criterion of this tree the engine cannot express, in statement order. Read by + * `Having.validate()`; empty for every criterion tree [[metricSelector]] can render. + */ + def unrepresentable(expr: Criteria): Seq[MetricSelector.Unrepresentable] = expr match { + case Predicate(left, _, right, maybeNot, _) => + unrepresentable(left) ++ (maybeNot match { + case Some(_) => right.negated.map(unrepresentable).getOrElse(unrepresentable(right)) + case None => unrepresentable(right) + }) + case relation: ElasticRelation => unrepresentable(relation.criteria) + case _ => + selector(expr) match { + case u: Unrepresentable => Seq(u) + case _ => Nil + } + } + + private def selector(expr: Criteria): MetricSelector = expr match { case Predicate(left, op, right, maybeNot, group) => - val leftStr = metricSelector(left) - val rightStr = metricSelector(right) - - // Filtering all "1 == 1" - if (leftStr == "1 == 1" && rightStr == "1 == 1") { - "1 == 1" - } else if (leftStr == "1 == 1") { - rightStr - } else if (rightStr == "1 == 1") { - leftStr - } else { - val opStr = op match { - case AND | OR => op.painless(None) - case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") - } - // `A AND NOT B`: the parser attaches the NOT to the RIGHT criteria (`Predicate.sql` and - // `asFilter` agree); this used to prefix the LEFT one -- the exact complement of what was - // asked. The negation is pushed INTO the right-hand expression when it is a single one - // (`NOT MAX(x) > 45` renders `(params.max_x == null ? false : (params.max_x <= 45))`), so a - // bucket whose metric is missing still fails the test -- `!(guard ? false : ...)` would let - // it through, against SQL's three-valued NOT and the AC 4b contract. A compound right side - // falls back to `!( ... )`. - maybeNot match { - case Some(_) => - 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. - case None => s"($leftStr) $opStr !($rightStr)" - } - case None if group => s"($leftStr) $opStr ($rightStr)" - case None => s"$leftStr $opStr $rightStr" - } + // The RIGHT criterion as it is really rendered: a `NOT` on the predicate is pushed INTO it + // when it can carry one, so the verdict must be taken on the SAME node the script is built + // from -- otherwise a refusal could be decided on a criterion nothing emits. + val effectiveRight = maybeNot.flatMap(_ => right.negated).getOrElse(right) + val leftSel = selector(left) + val rightSel = selector(effectiveRight) + (leftSel, rightSel) match { + case (u: Unrepresentable, _) => u + case (_, u: Unrepresentable) => u + // Filtering all "1 == 1" + case (NoFilter, NoFilter) => NoFilter + case (NoFilter, r) => r + case (l, NoFilter) => l + case (Filter(leftStr), Filter(rightStr)) => + val opStr = op match { + case AND | OR => op.painless(None) + case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") + } + // `A AND NOT B`: the parser attaches the NOT to the RIGHT criteria (`Predicate.sql` and + // `asFilter` agree); this used to prefix the LEFT one -- the exact complement of what was + // asked. The negation is pushed INTO the right-hand expression when it is a single one + // (`NOT MAX(x) > 45` renders `(params.max_x == null ? false : (params.max_x <= 45))`), so + // a bucket whose metric is missing still fails the test -- `!(guard ? false : ...)` would + // let it through, against SQL's three-valued NOT and the AC 4b contract. A compound right + // side falls back to `!( ... )`. + Filter(maybeNot match { + case Some(_) if right.negated.isDefined => s"($leftStr) $opStr $rightStr" + // Grammar-unreachable today (`NOT (A AND B)` in HAVING is a parse rejection); kept + // as the total fallback for a compound right side. + case Some(_) => s"($leftStr) $opStr !($rightStr)" + case None if group => s"($leftStr) $opStr ($rightStr)" + case None => s"$leftStr $opStr $rightStr" + }) } - case relation: ElasticRelation => metricSelector(relation.criteria) + case relation: ElasticRelation => selector(relation.criteria) - case _: MultiMatchCriteria => "1 == 1" + case _: MultiMatchCriteria => NoFilter case e: Expression if e.isAggregation || e.referencesBucketMetric => // NO FILTERING: the script is generated for all metrics. The context-free rendering of an @@ -397,10 +470,163 @@ object MetricSelectorScript { // already converted to epoch millis. It used to be converted HERE by appending // `.toInstant().toEpochMilli()` to the rendered predicate -- which only reached the literal // because the predicate happened to end with it. - e.painless(None) - case _ => "1 == 1" + Filter(e.painless(None)) + + // 🔴 Issue #389 -- the predicate reads an aggregate through a FUNCTION (`ABS(COUNT(*)) > 1`, + // `COALESCE(COUNT(*), 0) > 1`, `1 < ABS(COUNT(*))`). Neither flag above sees it, because a + // function never looks inside its own arguments, so it fell to the `1 == 1` catch-all below + // and the whole condition VANISHED. Emitted when the rendering is provably a single boolean + // expression; refused BY NAME when it is not. Never dropped. + case e: Expression if e.bucketMetrics.nonEmpty => + representable(e) match { + case Left(reason) => Unrepresentable(e.sql, reason) + case Right(script) => Filter(script) + } + + case _ => NoFilter } + /** This predicate's `bucket_selector` script, or the reason the engine cannot express it. + * + * ONE rendering: the verdict and the script it authorises come out of the same call, so the + * emission can never be the second render of a tree whose FIRST render was the one vetted. + * + * 🔴 The verdict is taken on the RENDERING, never on a list of function names. A name list is a + * fourth derivation of what the renderer does and would disagree with it the first time a + * rendering changed (`project_type_derivations_runtime_declared_reported`); the rendering is the + * thing Elasticsearch actually compiles. And it is a POSITIVE proof -- everything this method + * cannot account for is refused, because a `HAVING` that cannot be expressed must fail, never + * silently return every group (`feedback_nonsense_input_fails_loudly`). + * + * The four disqualifiers, each MEASURED on a real rendering: + * 1. it cannot be rendered at all without a document context -- `CASE WHEN ... END` throws; 2. + * it carries a STATEMENT, not an expression -- `ROUND(MAX(x), 2)` renders `def arg1 = + * Math.pow(10, 2); ...`, and a `bucket_selector` source is one expression; 3. it can + * evaluate to NULL -- `NULLIF(COUNT(*), 0) > 1` renders `(params.c) == 0 ? null : + * (params.c) > 1`, which hands Elasticsearch a `null` where a boolean is required. `? null + * :` is the renderer's OWN null-producing idiom and is already the marker the repo's + * null-safety guard keys on; 4. its rendering BOXES a number -- `ABS(COUNT(*)) > 1` renders + * `Double.valueOf(Math.abs(params.c)) > 1`, and the bucket-pipeline script context does not + * compile it; 5. it reads a local nothing binds -- `NULLIF(c, 0) > 1` over a SELECT alias + * renders `arg0 == 0 ? null : arg0 > 1`, and `arg0` is a `PainlessContext` parameter that + * no bucket pipeline declares. Every free lower-case word is refused unless it is a + * Painless literal (`null` / `true` / `false`) or the `params` map itself: a Painless + * expression has no free functions, so a method call is always preceded by a `.` and a + * class is capitalised. + * + * 🔴 Rule 4 is the one NO amount of reading could have produced, and it is why the acceptance + * bar for this issue was EXECUTION and not a byte pin + * (`feedback_assert_the_mechanism_not_a_proxy` + * -- PARSES != RENDERS != RUNS). The spec asserted that `Double.valueOf(Math.abs(params.c)) > 1` + * was "legal as a bucket_selector source"; MEASURED on a real Elasticsearch 8.18.3 it is a + * `class_cast_exception: Cannot cast from [java.lang.Double] to [int]` at COMPILE time, and the + * whole search fails. The rule is DERIVED, not guessed: eighteen shapes were rendered and + * executed, and `.valueOf(` separates the nine that run from the nine that do not, exactly. The + * same call compiles in the QUERY script context (`WHERE ABS(x) > 1` runs), so this is a + * property of the bucket-pipeline context, not of the rendering alone -- which is precisely why + * a syntactic gate needs a measured rule here and cannot derive one. + * + * Rule 5's complement also proves rule 3 is not the only null route, and rules 2-5 are decided + * on the rendering with every STRING LITERAL blanked, so a `;`, a `?` or a word inside one is + * never read as code. + */ + private[query] def representable(e: Expression): Either[String, String] = + Try(e.functionBucketPipelinePainless).toOption match { + case None => + Left("it cannot be rendered without a document, and a bucket pipeline has none") + case Some(rendering) => disqualifyRendering(rendering).toLeft(rendering) + } + + /** Rules 2-6 of [[representable]], asked of a RENDERING rather than of an expression. + * + * Exposed as its own function because it is a pure function of a string and is therefore the + * only surface on which each rule can be falsified INDIVIDUALLY + * (`feedback_assert_the_mechanism_not_a_proxy`): the rules overlap on real SQL -- every shape + * that trips the top-level-conditional rule also renders a `? null :` -- so a mutation matrix + * driven by statements alone reports a live rule as unguarded. The end-to-end rows stay; these + * are what keep each rule honest. + */ + private[query] def disqualifyRendering(rendering: String): Option[String] = + disqualify(blankStringLiterals(rendering)) + + private def disqualify(code: String): Option[String] = { + if (code.contains(";") || code.contains("def ")) + Some( + "its rendering needs a local declaration, and a bucket_selector source is one expression" + ) + else if (code.contains("? null :")) + Some("its rendering can evaluate to NULL, and a bucket_selector source must be a boolean") + else if (conditionalAtTopLevel(code)) + Some( + "its outermost operator is a conditional rather than a comparison, so it does not render a boolean" + ) + else if (code.contains(".valueOf(")) + Some( + "its rendering boxes a number, which the bucket-pipeline script context does not compile" + ) + else + freeLocal(code).map(name => + s"its rendering reads '$name', which nothing binds in a bucket pipeline" + ) + } + + /** The same string with the CONTENT of every Painless string literal replaced by spaces, so the + * scanners above read code and only code. Length-preserving, so an index into the result is an + * index into the original. + */ + private def blankStringLiterals(s: String): String = { + val out = new StringBuilder(s) + var i = 0 + while (i < s.length) { + val c = s.charAt(i) + if (c == '"' || c == '\'') { + var j = i + 1 + while (j < s.length && s.charAt(j) != c) { + if (s.charAt(j) == '\\') { out.setCharAt(j, ' '); j += 1 } + if (j < s.length) { out.setCharAt(j, ' '); j += 1 } + } + i = j + 1 + } else i += 1 + } + out.toString + } + + /** Is there a `?` outside every parenthesis? Then the comparison is not the outermost operator + * and the expression's value is whatever the branches are -- not necessarily a boolean. + */ + private def conditionalAtTopLevel(code: String): Boolean = { + var depth = 0 + var i = 0 + while (i < code.length) { + code.charAt(i) match { + case '(' => depth += 1 + case ')' => depth -= 1 + case '?' => if (depth <= 0) return true + case _ => + } + i += 1 + } + false + } + + private val FreeWord: Regex = """(? + m.group(1) + } + + private val PainlessLiterals: Set[String] = Set("null", "true", "false") + } case class BucketIncludesExcludes(values: Set[String] = Set.empty, regex: Option[String] = None) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala index 44fad1a36..7f6cc37cc 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala @@ -20,6 +20,21 @@ import app.softnetwork.elastic.sql.{Expr, Identifier, TokenRegex, Updateable} case object Having extends Expr("HAVING") with TokenRegex { + /** The SELECT items a bare name in a `HAVING` may resolve to: aggregates AND arithmetic over + * aggregates (`MAX(x) - MIN(x) AS d`), the latter being a `bucket_script` that a + * `bucket_selector` may read as a sibling pipeline aggregation by name. + * + * ONE derivation, read by [[resolveAggregateAliases]] (which SUBSTITUTES a bare reference) and + * by `SingleSearch.validate()` (which REFUSES a reference substitution cannot reach, issue #389: + * `HAVING NULLIF(c, 0) > 1` hides the alias inside a function ARGUMENT, where the substitution + * below -- deliberately scoped to an operand -- does not go). + */ + private[query] def aggregateAliases(request: SingleSearch): Map[String, Identifier] = + request.select.fields.collect { + case f if (f.isAggregation || f.isBucketScript) && f.fieldAlias.isDefined => + f.fieldAlias.get.alias -> f.identifier + }.toMap + /** `HAVING cnt > 1` where `cnt` aliases a SELECT aggregate (`COUNT(name) AS cnt`). The bare * identifier carries no aggregate function of its own, so the selector rendering saw no metric * in the condition and the whole HAVING degenerated to `1 == 1`: every group came back — the @@ -34,12 +49,7 @@ case object Having extends Expr("HAVING") with TokenRegex { criteria: Criteria, request: SingleSearch ): Criteria = { - // Aggregates AND arithmetic over aggregates (`MAX(x) - MIN(x) AS d`): the latter is a - // `bucket_script`, and a `bucket_selector` may read a sibling pipeline aggregation by name. - val aliased: Map[String, Identifier] = request.select.fields.collect { - case f if (f.isAggregation || f.isBucketScript) && f.fieldAlias.isDefined => - f.fieldAlias.get.alias -> f.identifier - }.toMap + val aliased: Map[String, Identifier] = aggregateAliases(request) if (aliased.isEmpty) return criteria def substitute(id: Identifier): Identifier = @@ -105,6 +115,13 @@ case class Having(criteria: Option[Criteria]) extends Updateable { def nestedElements: Seq[NestedElement] = criteria.map(_.nestedElements).getOrElse(Seq.empty).groupBy(_.path).map(_._2.head).toList + /** Every condition of this clause the engine cannot express as a `bucket_selector` (issue #389), + * in statement order. Refused by name in `SingleSearch.validate()`; EMPTY is the only shape that + * reaches emission. + */ + private[query] def unrepresentable: Seq[MetricSelector.Unrepresentable] = + criteria.toSeq.flatMap(MetricSelectorScript.unrepresentable) + def script: Option[String] = criteria.flatMap { criteria => val fullScript = MetricSelectorScript .metricSelector(criteria) 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 3228a8163..3ad1f8e45 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 @@ -217,10 +217,15 @@ sealed trait Criteria extends Updateable with PainlessScript { case Predicate(left, _, right, _, _) => left.extractAggregationFields ++ right.extractAggregationFields case relation: ElasticRelation => relation.criteria.extractAggregationFields + // 🔴 Issue #389 -- `bucketMetrics`, not `aggregations.nonEmpty` + `metricName`. An aggregate + // written inside a function ARGUMENT (`HAVING ABS(COUNT(*)) > 1`) is not the identifier's own + // aggregate, so `metricName` was `None` and NO aggregation was created for it: the selector + // had no `params.count_all` to read even once it was taught to read one. It is also the + // aggregate on the RIGHT of the comparison (`HAVING COUNT(*) > ABS(MAX(x))` emitted a script + // reading `params.max_x` with no `max_x` aggregation anywhere -- MEASURED on `main`). case e: Expression => - val identifiers = Seq(e.identifier) ++ e.maybeValue.collect { case id: Identifier => id } - identifiers - .filter(_.aggregations.nonEmpty) + (Seq(e.identifier) ++ e.maybeValue.collect { case id: Identifier => id }) + .flatMap(_.bucketMetrics) .flatMap { id => id.metricName.map(name => Field(id, Some(Alias(name)))) } @@ -1253,13 +1258,58 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { case IS_NULL => s"$param == null" case IS_NOT_NULL => s"$param != null" case _ => - val metrics: Seq[Identifier] = - identifier +: maybeValue.collect { case id: Identifier if id.isAggregation => id }.toSeq - val guard = metrics.map(id => s"${id.metricParam} == null").mkString(" || ") + val guard = bucketMetrics.map(id => s"${id.metricParam} == null").mkString(" || ") s"($guard ? false : $painlessNot(${bucketPipelineCheck(param)}))" } } + /** Every metric THIS predicate reads, left operand and right operand alike, deduplicated and in + * order -- the guard set of [[bucketPipelinePainless]] and of [[functionBucketPipelinePainless]] + * alike, and the same derivation `Criteria.extractAggregationFields` creates the aggregations + * from and `extractAllMetricsPath` publishes. + * + * 🔴 It used to be `identifier +: maybeValue.collect { case id if id.isAggregation }`, which + * misses an aggregate reached through a function: `HAVING COUNT(*) > ABS(MAX(x))` emitted + * `params.max_x` UNGUARDED (issue #389, measured on `main` -- `Math.abs(null)` fails the + * search). + */ + private[query] def bucketMetrics: Seq[Identifier] = + (identifier.bucketMetrics ++ maybeValue.toSeq + .collect { case id: Identifier => + id + } + .flatMap(_.bucketMetrics)).foldLeft(Seq.empty[Identifier]) { (acc, id) => + if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id + } + + /** The bucket-pipeline rendering of a predicate that reads an aggregate through a FUNCTION + * (`HAVING COALESCE(COUNT(*), 0) > 1`, issue #389). + * + * Unlike [[bucketPipelinePainless]] there is no `params.` to compare directly: the + * aggregate is an operand of a function, and the CONTEXT-FREE rendering of that function already + * reads it as `params.` (measured: `ABS(COUNT(*)) > 1` renders + * `Double.valueOf(Math.abs(params.c)) > 1`). So the rendering IS the script -- once it has been + * proved to be a single boolean expression ([[MetricSelectorScript.representable]]) and once + * every metric it dereferences is null-guarded. + * + * 🔴 The guard is added only for a metric the rendering does not already test. `COALESCE` exists + * precisely to decide what a null means, and forcing `false` on it would make `COALESCE(MAX(x), + * 99) > 1` answer `false` where SQL says `true`. A rendering that does NOT test the metric + * (`Math.abs(params.c)`) would throw on a null instead, and SQL's answer for it is UNKNOWN -- + * which is the `false` this guard supplies, exactly as `bucketPipelinePainless` supplies it for + * a bare aggregate. + */ + private[query] def functionBucketPipelinePainless: String = { + val rendering = painless(None) + val unguarded = bucketMetrics.filterNot { id => + rendering.contains(s"${id.metricParam} == null") || + rendering.contains(s"${id.metricParam} != null") + } + if (unguarded.isEmpty) rendering + else + s"(${unguarded.map(id => s"${id.metricParam} == null").mkString(" || ")} ? false : ($rendering))" + } + /** The comparison body of the bucket-pipeline rendering, `param` (= `params.`) against * the right-hand side. BETWEEN and IN, whose `painless` never goes through `check`, override it. */ 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 fe319c045..42426b7cb 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 @@ -908,6 +908,53 @@ package object query { case None => Right(()) } } + _ <- { + // 🔴 Issue #389 -- a HAVING over a FUNCTION of an aggregate must FILTER, or FAIL. It used + // to VANISH: a function never looks inside its own arguments, so `ABS(COUNT(*)) > 1` was + // invisible to the bucket-metric detection, the selector degenerated to `1 == 1` and + // every group came back with HTTP 200 (the #205 / #209 / #253 silent-wrong-answer + // family). Everything the engine can express is now emitted; everything it cannot is + // refused HERE, inside `Parser.apply`, so every venue sees it -- the REPL, JDBC, Flight, + // the materialized-view extension and the bridge's own emission path alike. + // + // ⚠️ A PARTIAL emission counts as a wrong answer: one un-expressible conjunct refuses the + // whole statement rather than silently filtering on the other half. + having.map(_.unrepresentable).getOrElse(Nil).headOption match { + case Some(u) => Left(u.message) + case None => Right(()) + } + } + _ <- { + // The residual of the rule above, and the reason it needs the STATEMENT rather than the + // clause: `HAVING NULLIF(c, 0) > 1` names a SELECT aggregate by its ALIAS from inside a + // function ARGUMENT. `Having.resolveAggregateAliases` substitutes an OPERAND only, so `c` + // stays a bare column, the predicate references no aggregate at all, and the rule above + // cannot see it -- it renders `arg0 == 0 ? null : arg0 > 1`, reading a context parameter + // no bucket pipeline binds. MEASURED: dropped silently on `main`. + // + // ⚠️ The alias set is read from `Having.aggregateAliases`, the SAME map the substitution + // uses, so the two cannot disagree about which bare names are aggregate references. A + // COLUMN that happens to share a SELECT aggregate's alias is therefore refused here for + // exactly the reason it is SUBSTITUTED there -- one ambiguity, one reading. + having.flatMap(_.criteria) match { + case None => Right(()) + case Some(criteria) => + val aliases = Having.aggregateAliases(this).keySet + criteria.referencedIdentifiers + .filter(id => id.functions.nonEmpty && id.bucketMetrics.isEmpty) + .flatMap(id => + FunctionUtils.funIdentifiers(id).map(_.name).filter(aliases.contains).map(id -> _) + ) + .headOption match { + case Some((id, alias)) => + Left( + s"HAVING cannot apply a function to the aggregate alias '$alias' (${id.sql}); " + + "compare the aggregate itself" + ) + case None => Right(()) + } + } + } _ <- { // A HAVING / ORDER BY aggregate whose derived name equals a SELECT alias of a DIFFERENT // aggregate (`SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3`) would read the wrong metric diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala new file mode 100644 index 000000000..cb6280fbd --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -0,0 +1,413 @@ +/* + * 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.OptionValues +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Issue #389 -- a `HAVING` over a FUNCTION of an aggregate must FILTER, or FAIL, never vanish. + * + * This file measures the `sql` half: the DETECTION (does the engine see the aggregate hidden in a + * function argument, a `CASE` branch, or the right-hand operand?), the REPRESENTABILITY gate (is + * the rendering a single boolean expression?) and the REFUSAL each un-expressible shape earns. The + * emission itself is pinned on the GENERATED query, in the bridge suites. + * + * 🔴 Every assertion here is at the PARSE/RENDER level. `Parser.apply` runs `update()` and then + * `validate()`, so a `Left` here is what every venue sees -- REPL, JDBC, Flight, the + * materialized-view extension and the bridge's own `SelectStatement(sql).query` alike. + */ +class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with OptionValues { + + private def parsed(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + private def rejection(sql: String): String = Parser(sql) match { + case Left(e) => e.msg + case Right(s) => fail(s"[$sql] expected a rejection, got $s") + } + + /** The AST of a statement `validate()` may REFUSE -- `Parser.single` runs `update()` inside its + * own combinator action, so everything the detection reads is populated exactly as `apply` would + * have populated it. Needed because half the shapes below are (correctly) rejected. + */ + private def unvalidated(sql: String): SingleSearch = Parser.parseUnvalidated(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + private def having(sql: String): Criteria = + parsed(sql).having.flatMap(_.criteria).getOrElse(fail(s"[$sql] has no HAVING criteria")) + + private def havingOf(s: SingleSearch, sql: String): Criteria = + s.having.flatMap(_.criteria).getOrElse(fail(s"[$sql] has no HAVING criteria")) + + private def script(sql: String): String = + MetricSelectorScript.metricSelector(having(sql)) + + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " + + // ------------------------------------------------------------------------------------------- + // Detection -- `Identifier.bucketMetrics` is the ONE derivation behind the selector, the + // aggregations that get created, and the `buckets_path` that publishes them. + // ------------------------------------------------------------------------------------------- + + private def metricKeys(sql: String): Seq[String] = + havingOf(unvalidated(sql), sql).referencedIdentifiers + .flatMap(_.bucketMetrics) + .flatMap(_.metricName) + .distinct + + "the aggregate inside a function argument" should "be seen through every wrapper" in { + // Measured on `main`: every one of these answered `hasAggregation = false` and the whole + // condition degenerated to `1 == 1`. + metricKeys(group + "ABS(COUNT(*)) > 1") shouldBe Seq("c") + metricKeys(group + "COALESCE(COUNT(*), 0) > 1") shouldBe Seq("c") + metricKeys(group + "NULLIF(COUNT(*), 0) > 1") shouldBe Seq("c") + metricKeys(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq("c") + metricKeys(group + "FLOOR(COUNT(*)) > 1") shouldBe Seq("c") + metricKeys(group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1") shouldBe Seq("c") + } + + it should "be seen in a CASE branch and in a CASE condition" in { + // 🔴 The two live in DIFFERENT places and no single walk reaches both: `Case.args` excludes the + // WHEN conditions on purpose, so `funIdentifiers` finds the THEN result and misses the + // condition. Measured: the condition form yielded NO aggregate from `funIdentifiers` alone. + metricKeys(group + "CASE WHEN status = 'a' THEN COUNT(*) ELSE 0 END = 1") shouldBe Seq("c") + metricKeys(group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1") shouldBe Seq("c") + } + + it should "be seen on the RIGHT operand of the comparison" in { + // `1 < ABS(COUNT(*))` reads the metric through the VALUE, not the identifier. + metricKeys(group + "1 < ABS(COUNT(*))") shouldBe Seq("c") + metricKeys( + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > ABS(MAX(x))" + ) shouldBe Seq("c", "max_x") + } + + "a predicate that references no aggregate" should "still contribute no metric" in { + metricKeys(group + "status = 'A'") shouldBe empty + metricKeys(group + "UPPER(status) = 'A'") shouldBe empty + } + + "a SELECT bucket_script referenced by its alias" should "publish the alias, never its operands" in { + // The control the third arm must not swallow: the selector reads the sibling pipeline + // aggregation `d` by name; publishing `max_x` / `min_x` here would change a shipped query. + val sql = "SELECT status, MAX(x) - MIN(x) AS d FROM t GROUP BY status HAVING d > 3" + having(sql).extractAllMetricsPath shouldBe Map("d" -> "d") + script(sql) shouldBe "(params.d == null ? false : (params.d > 3))" + } + + // ------------------------------------------------------------------------------------------- + // Emission -- what the selector renders once the aggregate is seen + // + // 🔴 The emittable population is the one that MEASURABLY RUNS. `GREATEST` / `LEAST` / `COALESCE` + // emit; `ABS` and the rest of the boxing family are REFUSED, because their rendering does not + // compile in the bucket-pipeline script context -- see the disqualifier tests below. + // ------------------------------------------------------------------------------------------- + + "a null-PROPAGATING function of an aggregate" should "render guarded, exactly as a bare aggregate does" in { + // SQL says `GREATEST(NULL, 0) > 1` is UNKNOWN, so the group is excluded -- which is the `false` + // this guard supplies. Without it `Math.max(null, 0)` fails the whole search. + script(group + "GREATEST(COUNT(*), 0) > 1") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) > 1))" + script(group + "LEAST(COUNT(*), 99) > 1") shouldBe + "(params.c == null ? false : (Math.min(params.c, 99) > 1))" + script(group + "1 < GREATEST(COUNT(*), 0)") shouldBe + "(params.c == null ? false : (1 < Math.max(params.c, 0)))" + } + + "a null-ABSORBING function of an aggregate" should "render UNguarded" in { + // 🔴 `COALESCE` exists precisely to decide what a null means. Forcing `false` on it would make + // `COALESCE(MAX(x), 99) > 1` answer `false` where SQL says `true`. + script(group + "COALESCE(COUNT(*), 0) > 1") shouldBe "(params.c != null ? params.c : 0) > 1" + } + + "BETWEEN and IN over a function of an aggregate" should "render as one guarded boolean" in { + script(group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5") shouldBe + "(params.c == null ? false : ((Math.max(params.c, 0) >= 1 && Math.max(params.c, 0) <= 5)))" + script(group + "GREATEST(COUNT(*), 0) IN (1, 2)") shouldBe + "(params.c == null ? false : ((Math.max(params.c, 0) == 1 || Math.max(params.c, 0) == 2)))" + } + + "NOT over a function of an aggregate" should "push the negation into the comparison" in { + script(group + "NOT GREATEST(COUNT(*), 0) > 1") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) <= 1))" + } + + "a conjunction of two expressible predicates" should "emit BOTH" in { + // 🔴 On `main` the second conjunct was DROPPED and the answer looked filtered while being + // wrong -- a partial filter is a wrong answer, not a lesser fix. + script(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") shouldBe + "(params.c == null ? false : (params.c > 1)) && " + + "(params.c == null ? false : (Math.max(params.c, 0) > 2))" + } + + "a disjunction of two expressible predicates" should "emit BOTH" in { + // The OR form is WORSE than the AND form when a branch is dropped: the surviving filter is + // STRICTER than what was written, so rows silently disappear. + script(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) > 1)) || " + + "(params.c == null ? false : (params.c > 5))" + } + + "an aggregate reached only through a HAVING function" should "be created and published" in { + // Issue #389 AC-5: with no SELECT alias there was no aggregation for the script to read. + val sql = "SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1" + parsed(sql).sqlAggregations.keys.toList shouldBe List("count_all") + having(sql).extractAllMetricsPath shouldBe Map("count_all" -> "count_all") + } + + "an aggregate reached only through the RIGHT operand" should "be created and guarded" in { + // Measured on `main`: the script read `params.max_x`, `buckets_path` declared only `c`, and NO + // `max_x` aggregation existed anywhere. + val sql = + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)" + parsed(sql).sqlAggregations.keys.toList shouldBe List("c", "max_x") + having(sql).extractAllMetricsPath shouldBe Map("c" -> "c", "max_x" -> "max_x") + script(sql) shouldBe + "(params.c == null || params.max_x == null ? false : " + + "(params.c > Math.max(params.max_x, 0)))" + } + + // ------------------------------------------------------------------------------------------- + // The representability gate -- one refusal per disqualifier, each naming its own reason + // ------------------------------------------------------------------------------------------- + + "a rendering that can evaluate to NULL" should "be refused, naming the boolean requirement" in { + val msg = rejection(group + "NULLIF(COUNT(*), 0) > 1") + msg should include("HAVING cannot be applied to NULLIF(COUNT(*), 0) > 1") + msg should include("can evaluate to NULL") + } + + it should "be refused through an outer function too" in { + rejection(group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1") should include("can evaluate to NULL") + } + + "a rendering that needs a local declaration" should "be refused, naming the expression requirement" in { + val msg = rejection( + "SELECT status, MAX(amount) AS m FROM t GROUP BY status HAVING ROUND(MAX(amount), 2) > 1" + ) + msg should include("HAVING cannot be applied to ROUND(MAX(amount), 2) > 1") + msg should include("needs a local declaration") + } + + "a rendering that BOXES a number" should "be refused, naming the script context" in { + // 🔴 The disqualifier no amount of reading could have produced, and the reason this issue's + // acceptance bar was EXECUTION. `Double.valueOf(Math.abs(params.c)) > 1` is syntactically a + // perfectly good boolean expression -- and on a real Elasticsearch 8.18.3 it is a COMPILE error + // (`Cannot cast from [java.lang.Double] to [int]`) that fails the whole search. The same call + // compiles in the QUERY script context, so it is the bucket-pipeline context that refuses it. + // Derived, not guessed: eighteen shapes were rendered and executed, and `.valueOf(` separates + // the nine that run from the nine that do not, exactly. + Seq("ABS", "FLOOR", "CEIL", "SQRT", "EXP", "LOG").foreach { fn => + withClue(s"[$fn] ") { + rejection(group + s"$fn(COUNT(*)) > 1") should include("boxes a number") + } + } + rejection(group + "POWER(COUNT(*), 2) > 1") should include("boxes a number") + } + + "a function whose rendering boxes nothing" should "still emit" in { + // The complement that proves the rule above is not simply "refuse every math function": + // `SIGN` renders a bare ternary, and it runs. + script(group + "SIGN(COUNT(*)) > 0") shouldBe + "(params.c == null ? false : ((params.c > 0 ? 1 : (params.c < 0 ? -1 : 0)) > 0))" + } + + "a rendering that needs a document" should "be refused" in { + // A `CASE` renders only against a Painless context; a bucket pipeline has no document. + rejection(group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1") should include( + "cannot be rendered without a document" + ) + } + + "an aggregate ALIAS used inside a function argument" should "be refused by name" in { + // `Having.resolveAggregateAliases` substitutes an OPERAND only, so `c` inside `NULLIF(c, 0)` + // stays a bare column and renders `arg0`, a context parameter nothing binds. + val msg = rejection(group + "NULLIF(c, 0) > 1") + msg should include("aggregate alias 'c'") + msg should include("NULLIF(c, 0)") + } + + "a conjunction mixing an expressible and an un-expressible predicate" should "refuse the whole statement" in { + // 🔴 The alternative is a PARTIAL filter, which is a wrong answer dressed as a right one. + rejection(group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2") should include( + "can evaluate to NULL" + ) + rejection(group + "NULLIF(COUNT(*), 0) > 2 AND COUNT(*) > 1") should include( + "can evaluate to NULL" + ) + rejection(group + "COUNT(*) > 1 OR NULLIF(COUNT(*), 0) > 2") should include( + "can evaluate to NULL" + ) + } + + "the whole-table HAVING" should "follow the same rule" in { + script("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) > 1))" + rejection("SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1") should include( + "can evaluate to NULL" + ) + } + + "a nested-relation HAVING" should "follow the same rule" in { + val sql = "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + parsed(sql).sqlAggregations.keys.toList shouldBe List("e.filtered_agg.count_e_address") + script(sql) shouldBe + "(params.count_e_address == null ? false : (Math.max(params.count_e_address, 0) > 1))" + } + + it should "be REFUSED inside the relation too, not only at the flat level" in { + // The walk recurses through `ElasticRelation`, so a nested predicate the engine cannot express + // is refused rather than silently dropped -- the nested level is where the ORIGINAL drop was + // most damaging (measured on `main`: the whole aggregation tree vanished and raw documents came + // back). + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING NULLIF(COUNT(e.address), 0) > 1" + ) should include("can evaluate to NULL") + } + + // ------------------------------------------------------------------------------------------- + // Each gate rule, falsified on its OWN input + // + // 🔴 The rules OVERLAP on real SQL -- every statement that trips the top-level-conditional rule + // also renders a `? null :`, and every statement carrying a free local also carries one. A + // mutation matrix driven by statements alone therefore reports live rules as unguarded, which is + // the "a guard never seen RED is a hypothesis" trap. `disqualifyRendering` is a pure function of + // a string, so each rule gets an input only IT can classify. + // ------------------------------------------------------------------------------------------- + + private def reason(rendering: String): Option[String] = + MetricSelectorScript.disqualifyRendering(rendering) + + "the statement rule" should "refuse a declaration and accept a call chain" in { + reason("def x = 1; x > 1").value should include("local declaration") + reason("params.c > 1") shouldBe None + } + + "the NULL rule" should "refuse a null-producing ternary anywhere in the rendering" in { + reason("(params.c) == 0 ? null : (params.c) > 1").value should include("evaluate to NULL") + reason("((params.c) == 0 ? null : 5) > 1").value should include("evaluate to NULL") + } + + "the top-level-conditional rule" should "refuse a rendering whose outermost operator is `?`" in { + // No SQL statement reaches this rule today -- every conditional the renderer emits is either + // parenthesised (`SIGN`) or null-producing (`NULLIF`), so the two rules above catch them + // first. It is kept because the gate's contract is to prove a BOOLEAN, and a bare ternary is + // not one; this row is the only thing that can hold it honest. + reason("params.c > 0 ? 1 : 2").value should include("outermost operator is a conditional") + reason("(params.c > 0 ? 1 : 2) > 1") shouldBe None + } + + "the boxing rule" should "refuse a valueOf and accept the same call without it" in { + reason("Double.valueOf(Math.abs(params.c)) > 1").value should include("boxes a number") + reason("Math.abs(params.c) > 1") shouldBe None + } + + "the free-local rule" should "refuse an unbound name and accept methods, classes and literals" in { + reason("arg0 > 1").value should include("reads 'arg0'") + reason("Double.parseDouble(params.c) > 1") shouldBe None + // `null` / `true` / `false` are Painless LITERALS, not free names -- allowing them is not a + // whitelist of the renderer's vocabulary, which is the thing this rule must not become. + reason("(params.c == null ? false : true) && params.c != null") shouldBe None + } + + "every rule" should "read CODE and never the inside of a string literal" in { + // 🔴 Without the literal blanking, each of these is refused by a DIFFERENT rule, and each of + // them is a perfectly good script. Measured on a real Elasticsearch 8.18.3: the first form + // runs. + reason("""(params.c != null ? params.c : "a; b").compareTo("x") == 0""") shouldBe None + reason("""params.c.compareTo("a ? null : b") == 0""") shouldBe None + reason("""params.c.compareTo("def x") == 0""") shouldBe None + reason("""params.c.compareTo('arg0') == 0""") shouldBe None + } + + "an un-expressible criterion" should "make the selector THROW, never return a partial script" in { + // 🔴 The second line of defence. `validate()` refuses every such statement inside + // `Parser.apply`, so this is reachable only for a `SingleSearch` assembled in code and emitted + // without validation -- exactly the route on which `""` used to mean "nothing to filter". + val sql = group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2" + val criteria = havingOf(unvalidated(sql), sql) + val thrown = intercept[IllegalStateException] { + MetricSelectorScript.metricSelector(criteria) + } + thrown.getMessage should include("HAVING cannot be applied to") + // ... and it must not have quietly emitted the OTHER half instead. + thrown.getMessage should not include "params.c > 1" + } + + it should "be reported by name from either side of a conjunction" in { + Seq( + group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2", + group + "NULLIF(COUNT(*), 0) > 2 AND COUNT(*) > 1", + group + "COUNT(*) > 1 OR NULLIF(COUNT(*), 0) > 2" + ).foreach { sql => + withClue(s"[$sql] ") { + val u = unvalidated(sql).having.map(_.unrepresentable).getOrElse(Nil) + u should have size 1 + u.head.reason should include("evaluate to NULL") + } + } + } + + "a function of an aggregate over a plain COLUMN" should "be refused by name" in { + // The free-local rule's own SQL witness: the second operand is a document field, which a + // bucket pipeline cannot read at all. + rejection(group + "GREATEST(COUNT(*), status) > 1") should include("reads 'arg1'") + } + + // ------------------------------------------------------------------------------------------- + // Controls -- shapes that must NOT move + // ------------------------------------------------------------------------------------------- + + "a bare aggregate predicate" should "render exactly as it always did" in { + script(group + "COUNT(*) > 1") shouldBe "(params.c == null ? false : (params.c > 1))" + script(group + "c > 1") shouldBe "(params.c == null ? false : (params.c > 1))" + } + + "a predicate over the bucket KEY" should "still contribute nothing to the selector" in { + // It is honoured by the `terms` include / exclude, which is a DIFFERENT mechanism. Refusing it + // here would break every `HAVING status = 'A'` shipped today. + script(group + "status = 'A'") shouldBe "1 == 1" + Parser(group + "status = 'A'").isRight shouldBe true + } + + "a HAVING over a function of a NON-aggregate" should "keep its pre-existing behaviour" in { + // 🔴 Recorded, deliberately NOT changed by this story: `UPPER(status) = 'A'` is honoured by no + // mechanism and is silently dropped -- measured identically on `main`, with and without an + // aggregate conjunct. It references NO aggregate, so it is outside this issue's population; + // refusing it needs a second derivation of the include/exclude rule. + script(group + "UPPER(status) = 'A'") shouldBe "1 == 1" + script(group + "COUNT(*) > 1 AND UPPER(status) = 'A'") shouldBe + "(params.c == null ? false : (params.c > 1))" + } + + "an aggregate written in WHERE" should "still be rejected, now through the wider walk" in { + Parser("SELECT status FROM t WHERE COUNT(*) > 1 GROUP BY status").isLeft shouldBe true + // NEW: the function-wrapped form is rejected too. It used to reach the bridge, where an + // aggregate has no document-level query form at all. + rejection("SELECT status FROM t WHERE ABS(COUNT(*)) > 1 GROUP BY status") should include( + "Aggregate functions are not allowed in WHERE" + ) + } +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index 187b02d14..fb97a179b 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -29,6 +29,8 @@ import org.scalatest.flatspec.AnyFlatSpecLike import org.scalatest.matchers.should.Matchers import org.slf4j.{Logger, LoggerFactory} +import scala.concurrent.Await +import scala.concurrent.duration._ import scala.language.implicitConversions case class CategoryCount(category: String, cnt: Long) @@ -697,6 +699,160 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + // ------------------------------------------------------------------ + // Issue #389 -- a HAVING over a FUNCTION of an aggregate must FILTER, or FAIL. + // + // MEASURED on `main`: every shape below parsed, ran, and returned EVERY group with HTTP 200 -- + // the `bucket_selector` was never emitted because a function does not look inside its own + // arguments. These rows EXECUTE the emitted script against a real cluster, because the claim is + // that Elasticsearch COMPILES and RUNS it: `cat_i` holds exactly `i` documents, so the filtered + // answer and the unfiltered one differ by construction (7 groups against 37). + // + // 🔴 EXECUTION is what set the emittable population, not reading. `ABS(COUNT(*)) > 1` renders a + // perfectly good-looking boolean expression and Elasticsearch REFUSES to compile it + // (`Double.valueOf` in a bucket-pipeline script). It is in the refusal list below, where the + // cluster put it. + // ------------------------------------------------------------------ + + private val over30 = 7 // cat_31 .. cat_37 + + "a HAVING over a function of an aggregate" should "filter the groups it names" in { + // ⚠️ Unrolled, not looped: `searchAs` is a macro and needs a compile-time constant SQL string. + def check(label: String, result: ElasticResult[Seq[CategoryCount]], expected: Int): Unit = + result match { + case ElasticSuccess(rows) => + withClue(s"[$label] rows=${rows.map(_.category).sorted}: ") { + rows should have size expected.toLong + rows.map(_.category).toSet shouldBe + (categories - expected + 1 to categories).map(c => f"cat_$c%02d").toSet + } + case ElasticFailure(error) => fail(s"[$label] Query failed: ${error.message}") + } + + check( + "GREATEST over an aggregate", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING GREATEST(COUNT(*), 0) > 30" + ), + over30 + ) + check( + "COALESCE over an aggregate", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COALESCE(COUNT(*), 0) > 30" + ), + over30 + ) + check( + "the aggregate on the RIGHT of the comparison", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING 30 < GREATEST(COUNT(*), 0)" + ), + over30 + ) + check( + "BETWEEN over a function of an aggregate", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING GREATEST(COUNT(*), 0) BETWEEN 31 AND 37" + ), + over30 + ) + } + + it should "emit BOTH halves of a conjunction" in { + // 🔴 On `main` the second conjunct was DROPPED, so this returned 37 groups: a PARTIAL filter, + // which looks filtered and is wrong. The oracle (cat_31..cat_34) differs from BOTH the + // unfiltered answer (37) and from either conjunct alone (7 and 34). + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 30 AND GREATEST(COUNT(*), 0) < 35" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category).toSet shouldBe Set("cat_31", "cat_32", "cat_33", "cat_34") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "create the aggregation when the aggregate appears ONLY in the HAVING function" in { + // No SELECT aggregate at all: on `main` there was no `count_all` aggregation for the script to + // read, so even a corrected script would have had nothing to compare. + client.searchAs[CategoryOnly]( + "SELECT category FROM group_by_completeness GROUP BY category HAVING GREATEST(COUNT(*), 0) > 30" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category).toSet shouldBe + (categories - over30 + 1 to categories).map(c => f"cat_$c%02d").toSet + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "filter the implicit whole-table group too" in { + implicit val wholeCtx: ConversionContext = NativeContext + // 703 documents in total: the TRUE predicate keeps the single row, the FALSE one removes it. + client.search( + SelectStatement( + "SELECT COUNT(*) AS c FROM group_by_completeness HAVING GREATEST(COUNT(*), 0) > 700" + ) + ) match { + case ElasticSuccess(response) => response.results should have size 1 + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + client.search( + SelectStatement( + "SELECT COUNT(*) AS c FROM group_by_completeness HAVING GREATEST(COUNT(*), 0) > 9999" + ) + ) match { + case ElasticSuccess(response) => response.results shouldBe empty + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "REFUSE the shapes it cannot express, rather than return every group" in { + // 🔴 The whole point of the issue: a predicate the engine cannot emit used to come back as + // HTTP 200 over UNFILTERED groups. Each of these must now be an error. + // + // ⚠️ Asserted through `run`, not `search(SelectStatement(...))`: `SelectStatement` yields + // `statement = None` on a rejection and the client reports its own generic "does not contain a + // valid search request" message, which would hide WHICH rule fired. `run` is the REPL / JDBC / + // Flight route and relays the parser's reason (#262). + Seq( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING ABS(COUNT(*)) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING NULLIF(COUNT(*), 0) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING NULLIF(cnt, 0) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING CASE WHEN COUNT(*) > 30 THEN 1 ELSE 0 END = 1", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING ROUND(SUM(amount), 2) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 30 AND NULLIF(COUNT(*), 0) > 1" + ).foreach { sql => + Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(result) => fail(s"[$sql] was accepted and answered $result") + case ElasticFailure(error) => + withClue(s"[$sql] ") { error.message should include("HAVING cannot") } + } + } + } + + it should "still return every group when the un-expressible predicate is NOT there" in { + // 🔴 Non-vacuity for the refusals above: the same statements WITHOUT the offending function + // are accepted, so the rejection is about the predicate and not about the fixture. + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 30" + ) match { + case ElasticSuccess(rows) => rows should have size over30.toLong + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + it should "refuse -- never discard -- a HAVING it cannot evaluate over the implicit group" in { implicit val havingCtx: ConversionContext = NativeContext // `category` is not aggregated and there is no GROUP BY, so there is no group to filter. The From 7acce66a62d26635931211bcf88a0f7c000f8ee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Thu, 24 Sep 2026 12:09:16 +0200 Subject: [PATCH 2/6] fix(sql): a HAVING over the GROUP BY key filters, and the gate covers both operands (#389 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 on an independent review that executed the branch against a 455433ae control and a real Elasticsearch 8.18.3. LEAD RULING — `HAVING ()` must WORK, not be refused. It does now. The equivalence it rests on was verified before building on it: a function of the GROUP BY key is CONSTANT within a bucket, so filtering the documents by it keeps exactly the satisfying buckets and changes nothing in a surviving one. Executed on real data, a surviving bucket's doc_count and every metric are byte-identical to the unfiltered run, including for a multi-level grouping. The predicate is carried by `SingleSearch.searchCriteria` and read by ONE line in each bridge; the AST and the render are untouched, because `MaterializedViewExtension` persists that render. What the equivalence does not license is refused by name: a predicate reading neither an aggregate nor a key, a NESTED key (a document filter keeps the whole parent), and an OR mixing a key with an aggregate — which closes a PRE-EXISTING silent wrong answer, measured on `main`: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the disjunction is two of them, because the key became a terms `include` and the metric a `bucket_selector`, so the OR executed as an AND. 🔴 The review also found that the representability gate was bypassed whenever the LEFT operand was a bare aggregate — so everything refused on the left was EMITTED on the right. Measured and executed: `> ROUND(MAX(a),2)`, `> NULLIF(MAX(a),0)` and `BETWEEN 1 AND ABS(MAX(a))` (which leaked RAW SQL into the Painless source) each returned HTTP 400, and `> CASE WHEN …` surfaced `Internal parser error` to the user. The gate now runs on both arms. That also CORRECTS this branch's own record: the `COUNT(*) > ABS(MAX(x))` row was documented as refused and was in fact running — the test for it asserted detection and never a verdict. Also closed: a metric compared as TEXT is refused rather than reaching Elasticsearch as a raw class-cast; a metric computed outside a nested grouping is refused rather than answering HTTP 200 with no grouping at all; the gate is re-asked at `validateResolved()` so a post-resolution divergence stays a 400 naming the clause; and the refusal reason now reaches the caller instead of the client's generic sentence. Two defects in this branch's own new code, both found by the mutation matrix reporting a refusal as unguarded: HAVING has FOUR mechanisms and scoping past the nested filter refused a shipped fixture; and `Identifier.bucket` is a stale copy reporting `nestedElement = None`, which made the nested refusal silent and pushed the one predicate the equivalence forbids. The emission matrix is now DERIVED (9 wrappers x 3 aggregates x 8 operand positions), each cell asserted for a verdict — the hand-written lists it replaces excluded the whole family above. Verified, 247-cell derived matrix EXECUTED on ES 8.18.3, control vs branch: runs with the filter applied 74 -> 111 silently dropped (200, wrong) 149 -> 0 HTTP 400 at the user 16 -> 0 `Internal parser error` label 3 -> 0 refused by name 5 -> 136 13/13 mutations RED · GrammarDiffProbe 18,062 control inputs, 0 differing · sql 1618 / core 1141 / bridge 231 / es6bridge 227 · `+ compile` both legs · the exact CI lint line · es8java *GroupByCompleteness* 37/37 on real ES 8.18.3. es6/es7/es9 NOT re-run after round 2. Cost: parse+emit +1.69% inside a +1.28% must-be-zero floor; `validateResolved` costs +2.6 us on the one `group by + having` shape and ±0.1 us on the other sixteen. Closed Issue #389 Co-Authored-By: Claude Opus 5 (1M context) --- .../elastic/sql/bridge/package.scala | 9 +- .../sql/HavingFunctionEmissionSpec.scala | 137 ++++++- .../elastic/client/SearchApi.scala | 18 +- documentation/sql/dql_statements.md | 34 +- .../elastic/sql/bridge/package.scala | 9 +- .../sql/HavingFunctionEmissionSpec.scala | 144 +++++++- .../elastic/sql/query/GroupBy.scala | 40 ++- .../elastic/sql/query/Having.scala | 10 - .../elastic/sql/query/package.scala | 335 +++++++++++++++++- .../sql/parser/InPredicateRenderSpec.scala | 9 +- .../HavingOverAggregateFunctionSpec.scala | 131 ++++++- .../client/GroupByCompletenessSpec.scala | 92 ++++- 12 files changed, 893 insertions(+), 75 deletions(-) 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 bb88acb44..6b4af308c 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 @@ -540,7 +540,14 @@ package object bridge { val nestedWithoutCriteriaQuery: Option[Query] = requestToNestedWithoutCriteriaQuery(request) var _search: SearchRequest = search("") query { - where.flatMap(_.criteria.map(_.asQuery())) match { + // 🔴 `searchCriteria`, NOT `where.criteria` (issue #389, lead ruling 2026-09-24): the WHERE + // clause conjoined with every HAVING predicate over a GROUP BY KEY. A function of the key is + // CONSTANT within a bucket, so filtering the DOCUMENTS by it keeps exactly the buckets that + // satisfy it and changes no surviving bucket's metrics -- which is what the `terms` + // `include` / `exclude` already does for the shapes it can spell, and this generalises it to + // the ones it cannot (`HAVING UPPER(status) = 'A'`). `sql` owns the rule and `validate()` + // refuses what the equivalence does not cover, so this is ONE line in each bridge copy. + searchCriteria.map(_.asQuery()) match { case Some(c) => val baseQuery = c nestedWithoutCriteriaQuery match { diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 78b134bb2..582a18852 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -48,11 +48,104 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { select.query } + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { + val here = Option(node.get(key)).toSeq + val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) + here ++ below + } + private val terms = """"terms":{"field":"status","size":65536,"min_doc_count":1}""" private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " - /** Every shape this file emits -- the input of the structural invariant at the bottom, so the - * guard is computed over the material it guards rather than over a hand-picked subset. + // --------------------------------------------------------------------------------------------- + // The matrix is DERIVED, not enumerated + // + // 🔴 The first version of this file carried two hand-written literal lists, and BOTH excluded the + // whole family in which the left operand is a bare aggregate and the RIGHT one carries the + // function -- which is exactly the family that emitted four HTTP-400 scripts and one + // `Internal parser error`. A count over a hand-written list is a literal asserting itself + // (`feedback_assert_the_mechanism_not_a_proxy`): the population has to be derived, and every cell + // has to get a VERDICT. + // --------------------------------------------------------------------------------------------- + + private val wrappers: Seq[String => String] = Seq( + a => s"COALESCE($a, 0)", + a => s"GREATEST($a, 0)", + a => s"LEAST($a, 99)", + a => s"SIGN($a)", + a => s"ABS($a)", + a => s"FLOOR($a)", + a => s"ROUND($a, 2)", + a => s"NULLIF($a, 0)", + a => s"CASE WHEN $a > 1 THEN 1 ELSE 0 END" + ) + + private val aggregates = Seq("COUNT(*)", "MAX(amount)", "SUM(amount)") + + /** Every operand POSITION a wrapped aggregate can occupy. The second and third are the ones the + * hand-written lists missed. + */ + private val positions: Seq[String => String] = Seq( + w => s"$w > 1", + w => s"1 < $w", + w => s"COUNT(*) > $w", + w => s"NOT $w > 1", + w => s"$w BETWEEN 1 AND 5", + w => s"$w IN (1, 2)", + w => s"COUNT(*) > 1 AND $w > 2", + w => s"COUNT(*) > 1 OR $w > 2" + ) + + private val matrix: Seq[String] = + for { + wrap <- wrappers + agg <- aggregates + pos <- positions + } yield group + pos(wrap(agg)) + + "every cell of the derived matrix" should "either emit a filter or be refused by name -- never both, never neither" in { + // 🔴 The verdict, not the shape. On `main` 149 of the 247 statements of this family answered + // HTTP 200 with the predicate SILENTLY DROPPED; the contract is that NONE does. + val silent = matrix.filter { sql => + Parser(sql) match { + case Left(_) => false + case Right(_) => + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").isEmpty + } + } + withClue( + s"${silent.size} statements emit NO filter and no refusal:\n${silent.take(8).mkString("\n")}\n" + )( + silent shouldBe empty + ) + } + + it should "name the HAVING clause in every refusal" in { + val refusals = matrix.flatMap(sql => Parser(sql).left.toOption.map(sql -> _.msg)) + // Non-vacuity computed over the material: the matrix MUST contain refusals, or the assertion + // above could pass on an all-emitting set. + refusals.size should be >= 100 + refusals.foreach { case (sql, msg) => + withClue(s"[$sql] ") { + msg should startWith("HAVING cannot") + msg should not startWith Parser.InternalParseFailure + } + } + } + + it should "cover both operand sides, so the right-hand family cannot go missing again" in { + // The guard on the guard: if `positions` ever loses the right-operand rows, this reddens. + val rightHand = matrix.filter(_.contains("COUNT(*) > COALESCE")) + rightHand should not be empty + val bounds = matrix.filter(_.contains("BETWEEN 1 AND 5")) + bounds should not be empty + } + + /** The shapes whose EXACT emission is pinned below, each executed against a real cluster. The + * structural invariants at the bottom run over these AND over the derived matrix above. */ private val emitted: Seq[String] = Seq( group + "COUNT(*) > 1", @@ -101,6 +194,38 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { ).mkString } + // --------------------------------------------------------------------------------------------- + // The GROUP BY key push-down (lead ruling 2026-09-24) + // + // 🔴 These assert the WIRING, not the derivation. `SingleSearch.searchCriteria` computing the + // right thing proves nothing if the bridge still reads `where.criteria` -- a mutation doing + // exactly that left the whole `sql` suite green. + // --------------------------------------------------------------------------------------------- + + "a HAVING over a function of the GROUP BY key" should "reach the QUERY, not the aggregation" in { + val q = queryOf(group + "UPPER(status) = 'A'") + q should not include """"query":{"match_all":{}}""" + q should include("toUpperCase") + // ... and it must NOT become a bucket_selector: it is a different mechanism. + q should not include "having_filter" + } + + it should "AND with the WHERE clause rather than replace it" in { + val q = queryOf( + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status " + + "HAVING UPPER(status) = 'A'" + ) + q should include(""""amount"""") + q should include("toUpperCase") + } + + it should "leave a key predicate the terms filter already expresses in the terms filter" in { + // Byte-identical to `main` for every shipped `HAVING = `: no query filter appears. + val q = queryOf(group + "status = 'a'") + q should include(""""include":["a"]""") + q should include(""""query":{"match_all":{}}""") + } + // --------------------------------------------------------------------------------------------- // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. // --------------------------------------------------------------------------------------------- @@ -231,14 +356,6 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { // The structural invariant (AD-2 item 3) // --------------------------------------------------------------------------------------------- - private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r - - private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { - val here = Option(node.get(key)).toSeq - val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) - here ++ below - } - "every emitted bucket pipeline" should "read exactly the metrics its buckets_path declares" in { emitted.foreach { sql => withClue(s"[$sql] ") { diff --git a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala index 3020e3a27..e76420d5e 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala @@ -477,6 +477,20 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC // PUBLIC METHODS // ======================================================================== + /** The client's refusal for a statement that did not parse -- WITH the parser's own reason when + * there is one (issue #389 / F7). + * + * 🔴 `SelectStatement.statement` is an `Option`, so a rejection arriving through it used to be + * reported as the generic sentence below and the reason -- computed, formatted, and naming the + * clause and its remedy -- was thrown away. Tolerable while the rejections were syntax errors + * the user can see; a whole family of SEMANTIC refusals now lands here. + */ + private def invalidSearchRequest(statement: SearchStatement, query: String): String = + (statement match { + case s: SelectStatement => s.parseError + case _ => None + }).getOrElse(s"SQL query does not contain a valid search request\n$query") + /** Search for documents / aggregations matching the SQL query. * * @param statement @@ -502,7 +516,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC ) ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request\n$query", + message = invalidSearchRequest(statement, query), operation = Some("search") ) ) @@ -559,7 +573,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC ) ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request\n$query", + message = invalidSearchRequest(statement, query), operation = Some("search") ) ) diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 7b59433f5..6e73a5ddc 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -784,16 +784,24 @@ ORDER BY COUNT(*) DESC; - A **function of an aggregate** in `HAVING` is applied to the group, or the statement is rejected by name — it is never ignored. `COALESCE`, `GREATEST`, `LEAST` and `SIGN` over an aggregate filter the groups (`HAVING COALESCE(COUNT(*), 0) > 30`, `HAVING GREATEST(MAX(price), 0) > 100`), on - either side of the comparison and under `BETWEEN`, `IN` and `NOT`. Everything Elasticsearch - cannot evaluate as a group filter is refused with the reason: + either side of the comparison and under `NOT`; `BETWEEN` and `IN` support them in the tested + POSITION (`GREATEST(COUNT(*), 0) BETWEEN 1 AND 5`) but not as a BOUND + (`COUNT(*) BETWEEN 1 AND ABS(MAX(price))` is refused). Everything the engine cannot evaluate as a + group filter is refused with the reason: - a rendering that can be NULL — `HAVING NULLIF(COUNT(*), 0) > 1`; - a rendering that needs a local variable — `HAVING ROUND(SUM(price), 2) > 10`; - a rendering that boxes a number — `HAVING ABS(COUNT(*)) > 1` and the rest of the numeric - function family (`FLOOR`, `CEIL`, `SQRT`, `EXP`, `LOG`, `POWER`); Elasticsearch's group-filter - script context does not compile the conversion, even though the same function works in `WHERE`, - in the `SELECT` list and in `ORDER BY`; + function family (`FLOOR`, `CEIL`, `SQRT`, `EXP`, `LOG`, `POWER`). This is a deliberate + over-approximation: the boxing conversion compiles in *some* positions of a group-filter script + and not others, so the engine refuses it in all of them rather than guess. The same functions + work normally in `WHERE`, in the `SELECT` list and in `ORDER BY`; + - a rendering that compares a metric as text — `HAVING COALESCE(MAX(created), '2020-01-01') > + '2019-01-01'`. A group filter reads every metric as a number (a date as epoch milliseconds); - `CASE ... END` in `HAVING`, which needs a document and a group filter has none; - - a function applied to a `SELECT` aggregate **alias** — `COUNT(*) AS c ... HAVING NULLIF(c, 0) > 1`. + - a function applied to a `SELECT` aggregate **alias** — `COUNT(*) AS c ... HAVING NULLIF(c, 0) > 1`; + - an aggregate computed outside a nested grouping — `... JOIN UNNEST(t.emails) AS e GROUP BY + e.name HAVING COALESCE(MAX(amount), 0) > 1`, where no level of the aggregation can read the + metric. In every refused case the remedy is the same: **compare the aggregate itself** — `HAVING SUM(price) > 10` rather than `HAVING ROUND(SUM(price), 2) > 10` — and apply the function @@ -803,6 +811,20 @@ ORDER BY COUNT(*) DESC; differs from arithmetic over aggregates, which IS a valid `SELECT` item (`MAX(price) - MIN(price) AS price_range`) and is the reason the rule above tells you to alias THAT one. +- A **function of the `GROUP BY` key** in `HAVING` filters the groups: `HAVING UPPER(city) = 'PARIS'`, + `HAVING LENGTH(status) = 1`, `HAVING SUBSTRING(code, 1, 1) = 'A'`. A function of the key has the + same value for every document of a group, so the condition is applied to the documents and whole + groups are kept or dropped — every surviving group's counts and metrics are exactly what they + would have been without it. + - A predicate naming a column that is **neither** the `GROUP BY` key **nor** an aggregate is + refused (`HAVING UPPER(name) = 'X'` when the grouping is by `city`): it is not constant within a + group, so it would have to filter documents and silently change every metric. Put it in `WHERE`, + or add the column to the `GROUP BY`. + - ⚠️ An `OR` between a key condition and an aggregate condition is refused + (`HAVING COUNT(*) > 1 OR status = 'b'`). The two are different mechanisms — a key filter and a + group filter — and their disjunction cannot be expressed; before this was refused it was + **executed as an `AND`**. An `OR` of key conditions (`HAVING status = 'a' OR status = 'b'`) is + supported. - One un-expressible condition rejects the whole `HAVING`, never just its own half: a statement is answered with every condition it was written with, or with an error. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents 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 ade951743..61390b829 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 @@ -533,7 +533,14 @@ package object bridge { val nestedWithoutCriteriaQuery: Option[Query] = requestToNestedWithoutCriteriaQuery(request) var _search: SearchRequest = search("") query { - where.flatMap(_.criteria.map(_.asQuery())) match { + // 🔴 `searchCriteria`, NOT `where.criteria` (issue #389, lead ruling 2026-09-24): the WHERE + // clause conjoined with every HAVING predicate over a GROUP BY KEY. A function of the key is + // CONSTANT within a bucket, so filtering the DOCUMENTS by it keeps exactly the buckets that + // satisfy it and changes no surviving bucket's metrics -- which is what the `terms` + // `include` / `exclude` already does for the shapes it can spell, and this generalises it to + // the ones it cannot (`HAVING UPPER(status) = 'A'`). `sql` owns the rule and `validate()` + // refuses what the equivalence does not cover, so this is ONE line in each bridge copy. + searchCriteria.map(_.asQuery()) match { case Some(c) => val baseQuery = c nestedWithoutCriteriaQuery match { diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 06ed3afb5..b5eaef426 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -13,9 +13,10 @@ import scala.jdk.CollectionConverters._ /** Issue #389 -- the EMITTED Elasticsearch query for a `HAVING` over a FUNCTION of an aggregate, * measured against the HAND-MAINTAINED es6 bridge (AC-7). * - * 🔴 The whole fix lives in the `sql` module: no line of either `ElasticAggregation.scala` moved. - * This twin is what PROVES that -- it is the same matrix, asserted against a different elastic4s - * major, and the only expectation that differs is elastic4s 6's bare-string `terms` `exclude`. + * 🔴 One line of each bridge changed (the query seam now reads `searchCriteria`); everything else + * lives in `sql`. This twin is what PROVES it -- the same derived matrix and the same pins against + * a different elastic4s major, with two expectations differing (elastic4s 6 renders a single-value + * `terms` `include` / `exclude` as a bare string). * * The `sql` half (detection, the representability gate, the refusals) is pinned in * `HavingOverAggregateFunctionSpec`. What is measured HERE is what Elasticsearch is actually sent: @@ -53,11 +54,104 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { select.query } + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { + val here = Option(node.get(key)).toSeq + val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) + here ++ below + } + private val terms = """"terms":{"field":"status","size":65536,"min_doc_count":1}""" private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " - /** Every shape this file emits -- the input of the structural invariant at the bottom, so the - * guard is computed over the material it guards rather than over a hand-picked subset. + // --------------------------------------------------------------------------------------------- + // The matrix is DERIVED, not enumerated + // + // 🔴 The first version of this file carried two hand-written literal lists, and BOTH excluded the + // whole family in which the left operand is a bare aggregate and the RIGHT one carries the + // function -- which is exactly the family that emitted four HTTP-400 scripts and one + // `Internal parser error`. A count over a hand-written list is a literal asserting itself + // (`feedback_assert_the_mechanism_not_a_proxy`): the population has to be derived, and every cell + // has to get a VERDICT. + // --------------------------------------------------------------------------------------------- + + private val wrappers: Seq[String => String] = Seq( + a => s"COALESCE($a, 0)", + a => s"GREATEST($a, 0)", + a => s"LEAST($a, 99)", + a => s"SIGN($a)", + a => s"ABS($a)", + a => s"FLOOR($a)", + a => s"ROUND($a, 2)", + a => s"NULLIF($a, 0)", + a => s"CASE WHEN $a > 1 THEN 1 ELSE 0 END" + ) + + private val aggregates = Seq("COUNT(*)", "MAX(amount)", "SUM(amount)") + + /** Every operand POSITION a wrapped aggregate can occupy. The second and third are the ones the + * hand-written lists missed. + */ + private val positions: Seq[String => String] = Seq( + w => s"$w > 1", + w => s"1 < $w", + w => s"COUNT(*) > $w", + w => s"NOT $w > 1", + w => s"$w BETWEEN 1 AND 5", + w => s"$w IN (1, 2)", + w => s"COUNT(*) > 1 AND $w > 2", + w => s"COUNT(*) > 1 OR $w > 2" + ) + + private val matrix: Seq[String] = + for { + wrap <- wrappers + agg <- aggregates + pos <- positions + } yield group + pos(wrap(agg)) + + "every cell of the derived matrix" should "either emit a filter or be refused by name -- never both, never neither" in { + // 🔴 The verdict, not the shape. On `main` 149 of the 247 statements of this family answered + // HTTP 200 with the predicate SILENTLY DROPPED; the contract is that NONE does. + val silent = matrix.filter { sql => + Parser(sql) match { + case Left(_) => false + case Right(_) => + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").isEmpty + } + } + withClue( + s"${silent.size} statements emit NO filter and no refusal:\n${silent.take(8).mkString("\n")}\n" + )( + silent shouldBe empty + ) + } + + it should "name the HAVING clause in every refusal" in { + val refusals = matrix.flatMap(sql => Parser(sql).left.toOption.map(sql -> _.msg)) + // Non-vacuity computed over the material: the matrix MUST contain refusals, or the assertion + // above could pass on an all-emitting set. + refusals.size should be >= 100 + refusals.foreach { case (sql, msg) => + withClue(s"[$sql] ") { + msg should startWith("HAVING cannot") + msg should not startWith Parser.InternalParseFailure + } + } + } + + it should "cover both operand sides, so the right-hand family cannot go missing again" in { + // The guard on the guard: if `positions` ever loses the right-operand rows, this reddens. + val rightHand = matrix.filter(_.contains("COUNT(*) > COALESCE")) + rightHand should not be empty + val bounds = matrix.filter(_.contains("BETWEEN 1 AND 5")) + bounds should not be empty + } + + /** The shapes whose EXACT emission is pinned below, each executed against a real cluster. The + * structural invariants at the bottom run over these AND over the derived matrix above. */ private val emitted: Seq[String] = Seq( group + "COUNT(*) > 1", @@ -106,6 +200,38 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { ).mkString } + // --------------------------------------------------------------------------------------------- + // The GROUP BY key push-down (lead ruling 2026-09-24) + // + // 🔴 These assert the WIRING, not the derivation. `SingleSearch.searchCriteria` computing the + // right thing proves nothing if the bridge still reads `where.criteria` -- a mutation doing + // exactly that left the whole `sql` suite green. + // --------------------------------------------------------------------------------------------- + + "a HAVING over a function of the GROUP BY key" should "reach the QUERY, not the aggregation" in { + val q = queryOf(group + "UPPER(status) = 'A'") + q should not include """"query":{"match_all":{}}""" + q should include("toUpperCase") + // ... and it must NOT become a bucket_selector: it is a different mechanism. + q should not include "having_filter" + } + + it should "AND with the WHERE clause rather than replace it" in { + val q = queryOf( + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status " + + "HAVING UPPER(status) = 'A'" + ) + q should include(""""amount"""") + q should include("toUpperCase") + } + + it should "leave a key predicate the terms filter already expresses in the terms filter" in { + // Byte-identical to `main` for every shipped `HAVING = `: no query filter appears. + val q = queryOf(group + "status = 'a'") + q should include(""""include":"a"""") + q should include(""""query":{"match_all":{}}""") + } + // --------------------------------------------------------------------------------------------- // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. // --------------------------------------------------------------------------------------------- @@ -238,14 +364,6 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { // The structural invariant (AD-2 item 3) // --------------------------------------------------------------------------------------------- - private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r - - private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { - val here = Option(node.get(key)).toSeq - val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) - here ++ below - } - "every emitted bucket pipeline" should "read exactly the metrics its buckets_path declares" in { emitted.foreach { sql => withClue(s"[$sql] ") { 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 afaf06057..8238ada09 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 @@ -463,14 +463,25 @@ object MetricSelectorScript { case _: MultiMatchCriteria => NoFilter + // The context-free rendering of an aggregate predicate IS the bucket-pipeline rendering + // (`Expression.bucketPipelinePainless`): `params.` reads, null-guarded, one + // parenthesised expression, temporal literal already converted to epoch millis. + // + // 🔴 It goes through the SAME gate as the arm below, and that is not symmetry for its own sake. + // This arm keys on the LEFT operand being an aggregate and says nothing about the RIGHT one, so + // every disqualifier refused below was EMITTED here. MEASURED and EXECUTED on Elasticsearch + // 8.18.3, all four with a bare `COUNT(*)` on the left: + // `> ROUND(MAX(a), 2)` -> `invalid sequence of tokens near ['def']` HTTP 400 + // `> NULLIF(MAX(a), 0)` -> `Cannot cast from [boolean] to [int]` HTTP 400 + // `BETWEEN 1 AND ABS(MAX(a))` -> `Unknown call [ABS] with [1] arguments` HTTP 400 + // (the BOUND leaked RAW SQL into the Painless source) + // `> CASE WHEN MAX(a) > 1 …` -> `painless(None)` THREW inside `validate()`, surfacing + // `Internal parser error: …` to the user (issue #250's family) case e: Expression if e.isAggregation || e.referencesBucketMetric => - // NO FILTERING: the script is generated for all metrics. The context-free rendering of an - // aggregate predicate IS the bucket-pipeline rendering (`Expression.bucketPipelinePainless`): - // `params.` reads, null-guarded, one parenthesised expression, temporal literal - // already converted to epoch millis. It used to be converted HERE by appending - // `.toInstant().toEpochMilli()` to the rendered predicate -- which only reached the literal - // because the predicate happened to end with it. - Filter(e.painless(None)) + representable(e) match { + case Left(reason) => Unrepresentable(e.sql, reason) + case Right(script) => Filter(script) + } // 🔴 Issue #389 -- the predicate reads an aggregate through a FUNCTION (`ABS(COUNT(*)) > 1`, // `COALESCE(COUNT(*), 0) > 1`, `1 < ABS(COUNT(*))`). Neither flag above sees it, because a @@ -564,6 +575,21 @@ object MetricSelectorScript { Some( "its rendering boxes a number, which the bucket-pipeline script context does not compile" ) + else if (code.contains(".compareTo(\"") && code.contains("params.")) + // 🔴 A metric arrives from `buckets_path` as a NUMBER -- a date metric as epoch millis -- so a + // rendering that compares it as a STRING cannot run. MEASURED on Elasticsearch 8.18.3: + // `HAVING COALESCE(MAX(d), '2020-01-01') > '2019-01-01'` renders + // `(params.max_d != null ? params.max_d : "2020-01-01").compareTo("2019-01-01") > 0` and + // fails with `cannot explicitly cast def [java.lang.String] to java.lang.Double`. + // + // The RENDERER defect is pre-existing and wider than this issue -- the bare + // `HAVING MAX(d) > '2019-01-01'` renders byte-identically on the control and fails the same + // way -- but a raw Painless class-cast reaching the user is not "refused by name", so the + // shape is refused here until the rendering is fixed. + Some( + "its rendering compares a metric as text, and a bucket pipeline reads every metric as a " + + "number (a date metric as epoch millis)" + ) else freeLocal(code).map(name => s"its rendering reads '$name', which nothing binds in a bucket pipeline" diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala index 7f6cc37cc..31a15f987 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala @@ -121,14 +121,4 @@ case class Having(criteria: Option[Criteria]) extends Updateable { */ private[query] def unrepresentable: Seq[MetricSelector.Unrepresentable] = criteria.toSeq.flatMap(MetricSelectorScript.unrepresentable) - - def script: Option[String] = criteria.flatMap { criteria => - val fullScript = MetricSelectorScript - .metricSelector(criteria) - .replaceAll("1 == 1 &&", "") - .replaceAll("&& 1 == 1", "") - .replaceAll("1 == 1", "") - .trim - if (fullScript.nonEmpty) Some(fullScript) else None - } } 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 42426b7cb..137da62e3 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 @@ -17,7 +17,7 @@ package app.softnetwork.elastic.sql import app.softnetwork.elastic.sql.`type`.{SQLType, SQLTypeUtils, SQLTypes} -import app.softnetwork.elastic.sql.operator.{SetOperator, UNION} +import app.softnetwork.elastic.sql.operator.{AND, OR, SetOperator, UNION} import app.softnetwork.elastic.sql.schema.{ sqlConfig, validateScriptReferences, @@ -162,13 +162,26 @@ package object query { override def withoutNestedExplosion: SelectStatement = this.copy(explodeNested = false) - lazy val statement: Option[SearchStatement] = { - queryToStatement(query) match { - case Some(s: SearchStatement) => Some(if (!explodeNested) s.withoutNestedExplosion else s) - case _ => None - } + /** ONE parse. `statement` and [[parseError]] are two readings of it, so asking for the reason + * costs nothing and cannot disagree with the verdict. + */ + private lazy val parsed: Either[parser.ParserError, Statement] = parser.Parser(query) + + lazy val statement: Option[SearchStatement] = parsed match { + case Right(s: SearchStatement) => Some(if (!explodeNested) s.withoutNestedExplosion else s) + case _ => None } + /** Why this statement did not parse, when it did not (issue #389 / F7). + * + * 🔴 `statement` is an `Option`, so every rejection reaching a caller through it used to be + * reported as the client's generic "SQL query does not contain a valid search request" -- the + * reason was computed, formatted and then thrown away. That was tolerable while the rejections + * were syntax errors the user could see for themselves; a whole family of SEMANTIC refusals + * now lands here, each naming a clause and a remedy, and none of them could reach the caller. + */ + lazy val parseError: Option[String] = parsed.left.toOption.map(_.msg) + override def sql: SQL = statement match { case Some(value) => value.sql @@ -697,6 +710,158 @@ package object query { ) } + /** Where a `HAVING` leaf predicate can be evaluated (issue #389, lead ruling 2026-09-24). + * + * 🔴 The whole design rests on one property: **a function of a GROUP BY key is CONSTANT within + * a bucket**, because every document in that bucket has the same key value. So `HAVING f(key) + * v` selects whole buckets, and filtering the DOCUMENTS by the same predicate keeps + * exactly those buckets and removes no document from a surviving one -- which is why every + * metric of a surviving bucket is unchanged. That is what the `terms` `include` / `exclude` + * already is: a key filter. The push-down generalises it to the predicates `include` / + * `exclude` cannot spell. + */ + private[query] sealed trait HavingScope + private[query] object HavingScope { + + /** Reads an aggregate: the `bucket_selector` population. */ + case object Metric extends HavingScope + + /** Reads only GROUP BY keys: constant within a bucket, so it can be pushed into the query. */ + case object GroupKey extends HavingScope + + /** Neither -- it names a column that is not a grouping key, so it is not constant within a + * bucket and NO mechanism can honour it. Refused; it used to be silently dropped. + */ + case object Unscoped extends HavingScope + } + + /** The GROUP BY bucket a leaf identifier reads, when it reads one. + * + * 🔴 TWO shapes, because the grammar builds a function over a column in two different ways and + * only one of them attaches `Identifier.bucket`. MEASURED over `GROUP BY a`: + * - `UPPER(a)` / `ABS(a)` build a WRAPPER identifier (`name = ""`) whose argument is a + * separate identifier `a` carrying `bucket = Some(a)`; + * - `DAY(a)` / `YEAR(a)` / `EPOCHDAY(a)` put the function on the identifier ITSELF, so the + * leaf IS `a` with `bucket = None` -- the bucket lookup keys on `identifierName`, which is + * `DAY(a)` there, not `a`. + * + * Matching the leaf's COLUMN name against each bucket's own column covers the second shape and + * is sound for the push-down: if the leaf names the column the `terms` aggregation groups by, + * its value is by construction the same for every document in a bucket. A false NEGATIVE only + * refuses; a false POSITIVE would push a non-constant predicate and silently change metrics, + * which is why the comparison is on the physical column and on nothing else. + */ + private[query] def keyBucketOf(leaf: Identifier): Option[Bucket] = { + // 🔴 Resolve through THIS statement's bucket list, never through `Identifier.bucket`. + // `Identifier.bucket` is a COPY attached during `update()` and deliberately NOT re-updated + // (the story-21.3 / #253 desync class), and MEASURED here it reports `nestedElement = None` + // for a bucket the statement lists as `Some(e)`. Reading the copy made `readsNestedKey` + // answer `false` for `GROUP BY e.name HAVING UPPER(e.name) = 'X'`, so the predicate was + // PUSHED INTO THE QUERY -- the one case the equivalence does NOT license, because a document + // filter on a nested field keeps the whole parent and its non-matching objects keep their + // buckets. Found by the mutation matrix reporting the nested refusal as unguarded. + val byName = buckets.find(b => b.identifier.name.nonEmpty && b.identifier.name == leaf.name) + byName.orElse(leaf.bucket.flatMap(attached => buckets.find(_.name == attached.name))) + } + + private[query] def namedLeavesOf(e: Expression): Seq[Identifier] = + e.referencedIdentifiers + .flatMap(id => FunctionUtils.funIdentifiers(id)) + .filter(_.name.nonEmpty) + .distinct + + private[query] def havingScopeOf(e: Expression): HavingScope = + if (e.referencedIdentifiers.exists(_.bucketMetrics.nonEmpty)) HavingScope.Metric + else { + val leaves = namedLeavesOf(e) + if (leaves.nonEmpty && leaves.forall(l => keyBucketOf(l).isDefined)) HavingScope.GroupKey + else HavingScope.Unscoped + } + + /** Does this key predicate read a NESTED bucket? Asked of the bucket the leaf RESOLVES to, not + * of `Identifier.bucket` -- which is `None` for the date-function shape above, so a check on + * it would silently miss exactly the statements this refuses. + */ + private[query] def readsNestedKey(e: Expression): Boolean = + e.nested || namedLeavesOf(e).exists(l => keyBucketOf(l).exists(_.nestedElement.isDefined)) + + /** Does the `terms` `include` / `exclude` path ALREADY express this key predicate? + * + * 🔴 Asked of the REAL function (`Expression.includes` / `excludes`), never of a copy of its + * rules. A second derivation of "which key predicates the terms filter can spell" is the + * story-21.3 desync class, and it would silently change every shipped `HAVING status = 'A'` + * the first time the two disagreed. + */ + private[query] def keyExpressibleByTerms(e: Expression): Boolean = { + val empty = BucketIncludesExcludes() + buckets.exists(b => + e.includes(b, not = false, empty) != empty || e.excludes(b, not = false, empty) != empty + ) + } + + /** Every leaf `Expression` of the FLAT part of the HAVING tree, in statement order. + * + * 🔴 A predicate scoped to a NESTED relation is deliberately NOT here, and that is the same + * boundary the whole-table block below already draws: such a predicate has its OWN + * long-standing mechanism (`requestToNestedFilterAggregation` turns it into a `filter` + * aggregation scoped to the inner-hits path), it is exercised by the bridge fixtures, and it + * is not discarded. MEASURED: without this exclusion the repo's own "complex query" fixture -- + * `... JOIN UNNEST(store.products) AS inner_products ... HAVING inner_products.deleted = + * false` + * -- is refused, which would have broken a shipped, WORKING statement. HAVING has FOUR + * mechanisms, not two: the `bucket_selector`, the `terms` include/exclude, this nested filter, + * and the query push-down added here. + */ + private[query] def havingLeaves: Seq[Expression] = { + def leaves(c: Criteria): Seq[Expression] = c match { + case Predicate(l, _, r, _, _) => leaves(l) ++ leaves(r) + case _: ElasticRelation => Nil + case e: Expression => Seq(e).filterNot(_.nested) + case _ => Nil + } + having.flatMap(_.criteria).toSeq.flatMap(leaves) + } + + /** The group-key predicates the query must carry: every `GroupKey` leaf the `terms` `include` / + * `exclude` path cannot already spell, and that is not scoped to a nested bucket. + * + * ⚠️ NOT applied to a key the terms filter already expresses -- pushing it as well would be + * correct (the same predicate ANDed twice) but would move the emitted bytes of every shipped + * `HAVING = ` statement for nothing. + * + * ⚠️ NOT applied under a NESTED relation: there the bucket is an inner object and a + * document-level filter keeps the whole PARENT, so a parent with one matching and one + * non-matching object would keep both buckets. `validate()` refuses that shape instead. + */ + private[query] lazy val pushedHavingKeys: Seq[Expression] = + if (groupBy.isEmpty) Nil + else + havingLeaves.filter(e => + havingScopeOf(e) == HavingScope.GroupKey && !keyExpressibleByTerms(e) && + !readsNestedKey(e) + ) + + /** The DOCUMENT-level criteria this search applies: the `WHERE` clause conjoined with every + * pushed-down group-key `HAVING` predicate (issue #389). + * + * 🔴 Read by the bridges INSTEAD of `where.criteria`. It is a derivation, never a rewrite of + * the AST: moving the predicate into `where` would change what [[sql]] renders, and + * `MaterializedViewExtension` PERSISTS that render and re-runs it -- the story-21.3 / #318 + * fixed-point lesson. The statement still renders exactly as it was written. + */ + lazy val searchCriteria: Option[Criteria] = { + val pushed = pushedHavingKeys + val whereCriteria = where.flatMap(_.criteria) + if (pushed.isEmpty) whereCriteria + else { + val conjunction = pushed.reduceLeft[Criteria]((acc, e) => Predicate(acc, AND, e)) + whereCriteria match { + case Some(w) => Some(Predicate(w, AND, conjunction)) + case None => Some(conjunction) + } + } + } + lazy val excludes: Seq[String] = select.except.map(_.fields.map(_.sourceField)).getOrElse(Nil) lazy val sources: Seq[String] = from.tables.map(_.name) @@ -847,14 +1012,27 @@ package object query { * measured, and each one names the shape it refuses. */ def validateResolved(): Either[String, Unit] = - ((where.flatMap(_.criteria).toSeq ++ having.flatMap(_.criteria).toSeq ++ - select.fields.flatMap(f => Case.conditionsOf(f.identifier)) ++ - orderBy.toSeq.flatMap(_.sorts.flatMap(s => Case.conditionsOf(s.field))) ++ - groupBy.toSeq.flatMap(_.buckets.flatMap(b => Case.conditionsOf(b.identifier)))) - .flatMap(_.temporalComparisonErrors) ++ + // 🔴 Issue #389 / F4 -- the representability gate runs AGAIN here, on the RESOLVED statement. + // `validate()` runs schema-less inside `Parser.apply`, and `update(Some(schema))` CAN change + // what a HAVING renders (measured divergence on three shapes, e.g. + // `COALESCE(MAX(d), CURRENT_DATE) > CURRENT_DATE` gains `.atStartOfDay(ZoneId.of('Z'))`). + // No shape was found that trips a disqualifier only after resolution, so this is latent + // rather than demonstrated -- and it is closed the cheap way: re-asking the question at the + // ONE seam that produces a resolved statement turns any such divergence into the same 400 + // every other refusal takes. The alternative (letting `MetricSelectorScript.metricSelector`'s + // `IllegalStateException` escape into the bridge) would surface a 500 with no clause named, + // which is issue #250's family. That throw stays as the last-resort invariant; this is what + // makes it unreachable in practice. + having.map(_.unrepresentable).getOrElse(Nil).headOption.map(u => Left(u.message)).getOrElse { + ((where.flatMap(_.criteria).toSeq ++ having.flatMap(_.criteria).toSeq ++ + select.fields.flatMap(f => Case.conditionsOf(f.identifier)) ++ + orderBy.toSeq.flatMap(_.sorts.flatMap(s => Case.conditionsOf(s.field))) ++ + groupBy.toSeq.flatMap(_.buckets.flatMap(b => Case.conditionsOf(b.identifier)))) + .flatMap(_.temporalComparisonErrors) ++ scriptedExpressions.flatMap(NullIf.mismatchesOf)).headOption - .map(Left(_)) - .getOrElse(Right(())) + .map(Left(_)) + .getOrElse(Right(())) + } /** Every expression of this statement that can be emitted as Painless, as the chain it is. * @@ -955,6 +1133,137 @@ package object query { } } } + _ <- { + // 🔴 Issue #389, lead ruling 2026-09-24 -- a HAVING over a FUNCTION of the GROUP BY KEY + // must WORK. It is pushed into the query (see `pushedHavingKeys`); what cannot be pushed + // is refused HERE. Two shapes, both MEASURED as silent wrong answers on `main` over a + // 4-document fixture (`a` x2, `b`, `cc`): + // + // HAVING UPPER(status) = 'A' -> buckets [a, b, cc] (should be [a]) + // HAVING LENGTH(status) = 1 -> buckets [a, b, cc] (should be [a, b]) + // + // (a) A predicate that reads neither an aggregate NOR a grouping key is not constant + // within a bucket, so NO mechanism can honour it -- the terms filter cannot spell it, + // the selector cannot read a document, and pushing it into the query would filter + // DOCUMENTS and silently change every metric of every surviving group. + val unscoped = havingLeaves + .filter(e => groupBy.isDefined && havingScopeOf(e) == HavingScope.Unscoped) + unscoped.headOption match { + case Some(e) => + Left( + s"HAVING can only filter on a GROUP BY key or on an aggregate; ${e.sql} is " + + "neither. Move it to WHERE, or add its column to the GROUP BY." + ) + case None => Right(()) + } + } + _ <- { + // (b) ... and a key predicate scoped to a NESTED bucket cannot be pushed either: there + // the bucket is an inner object, so a document-level filter keeps the whole PARENT + // and a parent holding one matching and one non-matching object would still produce + // BOTH buckets. The equivalence that licenses the push-down does not hold, so the + // shape is refused rather than answered wrongly. What the terms `include` / `exclude` + // already expresses is untouched -- it filters the BUCKET, not the document. + havingLeaves + .find(e => + groupBy.isDefined && havingScopeOf(e) == HavingScope.GroupKey && + !keyExpressibleByTerms(e) && readsNestedKey(e) + ) + .map(e => + Left( + s"HAVING cannot apply ${e.sql} to a nested grouping key: the filter would keep the " + + "whole parent document. Filter it in WHERE, or compare the key directly." + ) + ) + .getOrElse(Right(())) + } + _ <- { + // (c) 🔴 An OR that mixes the two populations is NOT expressible, and today it is + // executed as an AND. MEASURED on `main` over the same fixture: + // + // HAVING COUNT(*) > 1 OR status = 'b' -> buckets [] (should be [a, b]) + // + // `status = 'b'` becomes a terms `include` (which REMOVES bucket `a` outright) while + // `COUNT(*) > 1` becomes a `bucket_selector` (which removes `b`), so the disjunction + // answers the empty conjunction. A `bucket_selector` script cannot read the bucket + // KEY and a query filter cannot be OR-ed with a bucket filter, so no emission can + // express it. This is PRE-EXISTING in the include/exclude path; the ruling closes it. + // + // An OR of key predicates the terms filter can spell stays supported -- that path + // UNIONS them, which is the disjunction (`HAVING status = 'a' OR status = 'b'` + // emits `include: [a, b]`). + // The SAME boundary as `havingLeaves`: a relation-scoped predicate belongs to the nested + // filter mechanism and is neither a key filter nor a group filter here. + def leavesUnder(c: Criteria): Seq[Expression] = c match { + case Predicate(l, _, r, _, _) => leavesUnder(l) ++ leavesUnder(r) + case _: ElasticRelation => Nil + case e: Expression => Seq(e).filterNot(_.nested) + case _ => Nil + } + def scopesUnder(c: Criteria): Seq[HavingScope] = leavesUnder(c).map(havingScopeOf) + def firstBadOr(c: Criteria): Option[Criteria] = c match { + case p @ Predicate(l, op, r, _, _) => + val here = + if (op == OR) { + val leaves = leavesUnder(p) + val scopes = scopesUnder(p).distinct + // heterogeneous, or a key disjunct the terms filter cannot union + if (scopes.size > 1) Some(p: Criteria) + else if ( + scopes == Seq(HavingScope.GroupKey) && !leaves.forall(keyExpressibleByTerms) + ) Some(p: Criteria) + else None + } else None + here.orElse(firstBadOr(l)).orElse(firstBadOr(r)) + case _: ElasticRelation => None + case _ => None + } + having.flatMap(_.criteria).filter(_ => groupBy.isDefined).flatMap(firstBadOr) match { + case Some(p) => + Left( + s"HAVING cannot OR a GROUP BY key with an aggregate (${p.sql}): the key filter and " + + "the group filter are different mechanisms and their disjunction is not " + + "expressible. Split the query, or repeat the condition as an AND." + ) + case None => Right(()) + } + } + _ <- { + // 🔴 Issue #389 / F5 -- a representable HAVING that NO bucket level can address still + // vanished. MEASURED on the branch: + // + // SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name + // HAVING COALESCE(MAX(amount), 0) > 1 + // -> {"size":0,"_source":false,"aggs":{"max_amount":{"max":{"field":"amount"}}}} + // HTTP 200, no `having_filter` -- and the GROUP BY itself is gone. + // + // The metric is computed at the ROOT while the bucket lives inside the `nested` + // aggregation, so `resolveBucketMetric` answers `OutOfScope` at every level, every + // condition is filtered out, and `metricSelectorForBucket` returns the `""` that means + // "nothing to filter". That is the issue's SECOND conflation site. + // + // The vanished GROUP BY is a SEPARATE pre-existing defect on a different mechanism (the + // control answers `{"query":{"match_all":{}},"_source":true}` -- raw documents -- for the + // same statement) and is deliberately NOT fixed here. What this rule guarantees is the + // contract: the statement no longer answers HTTP 200 with a silently unfiltered result. + // + // A metric that DOES share the bucket's nesting level keeps working + // (`... GROUP BY e.name HAVING COUNT(e.address) > 1` emits the full nested tree). + val innermostNested = buckets.lastOption.flatMap(_.nestedElement).map(_.innerHitsName) + val strandedMetric = havingLeaves + .filter(e => havingScopeOf(e) == HavingScope.Metric) + .flatMap(_.bucketMetrics) + .find(m => innermostNested.isDefined && m.innerHitsName != innermostNested) + strandedMetric match { + case Some(m) if groupBy.isDefined => + Left( + s"HAVING cannot filter on ${m.sql}: it is computed outside the nested grouping " + + s"'${innermostNested.getOrElse("")}', so no level of the aggregation can read it. " + + "Aggregate a field of the nested object, or group by a field of the parent." + ) + case _ => Right(()) + } + } _ <- { // A HAVING / ORDER BY aggregate whose derived name equals a SELECT alias of a DIFFERENT // aggregate (`SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3`) would read the wrong metric diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala index 227e9e1c0..0ea0b533b 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala @@ -70,8 +70,13 @@ class InPredicateRenderSpec extends AnyFlatSpec with Matchers { s"SELECT x FROM t WHERE $name(a) IN (1, 2)", s"SELECT x FROM t WHERE $name(a) IN ('u', 'v')", s"SELECT x FROM t WHERE $name(a) NOT IN (1, 2)", - s"SELECT id FROM t GROUP BY id HAVING $name(a) IN (1, 2)", - s"SELECT id FROM t GROUP BY id HAVING $name(a) NOT IN (1, 2)" + // 🔴 `GROUP BY a`, not `GROUP BY id`: since issue #389 a HAVING predicate must read either an + // aggregate or a GROUP BY KEY -- `HAVING f(a) ...` over a non-grouped `a` is not constant + // within a bucket and is now refused by name instead of being silently dropped. Grouping by the + // column the function is applied to keeps this sweep exercising the HAVING venue, which is the + // half that carried #365's loud failure (`COUNT(x)(COUNT(x))`). + s"SELECT a FROM t GROUP BY a HAVING $name(a) IN (1, 2)", + s"SELECT a FROM t GROUP BY a HAVING $name(a) NOT IN (1, 2)" ) /** A statement is IN SCOPE only if it parses; a function that needs other arity or types simply diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index cb6280fbd..df1099bc5 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -324,6 +324,17 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt reason("Math.abs(params.c) > 1") shouldBe None } + "the text-comparison rule" should "refuse a metric compared as text and accept a numeric one" in { + // 🔴 MEASURED on Elasticsearch 8.18.3: a date metric arrives from `buckets_path` as epoch + // millis, so `(params.max_d != null ? params.max_d : "2020-01-01").compareTo("2019-01-01") > 0` + // fails with `cannot explicitly cast def [java.lang.String] to java.lang.Double`. + reason( + """(params.max_d != null ? params.max_d : "2020-01-01").compareTo("2019-01-01") > 0""" + ).value should + include("compares a metric as text") + reason("params.c > 1") shouldBe None + } + "the free-local rule" should "refuse an unbound name and accept methods, classes and literals" in { reason("arg0 > 1").value should include("reads 'arg0'") reason("Double.parseDouble(params.c) > 1") shouldBe None @@ -336,10 +347,12 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt // 🔴 Without the literal blanking, each of these is refused by a DIFFERENT rule, and each of // them is a perfectly good script. Measured on a real Elasticsearch 8.18.3: the first form // runs. - reason("""(params.c != null ? params.c : "a; b").compareTo("x") == 0""") shouldBe None - reason("""params.c.compareTo("a ? null : b") == 0""") shouldBe None - reason("""params.c.compareTo("def x") == 0""") shouldBe None - reason("""params.c.compareTo('arg0') == 0""") shouldBe None + // ⚠️ Deliberately NOT a `.compareTo("...")` payload: that is itself a disqualifier now (a + // metric arrives as a number), so such a row could never isolate the blanking. + reason("""params.c > 1 && params.c != "a; b"""") shouldBe None + reason("""params.c > 1 && params.c != "a ? null : b"""") shouldBe None + reason("""params.c > 1 && params.c != "def x"""") shouldBe None + reason("""params.c > 1 && params.c != 'arg0'""") shouldBe None } "an un-expressible criterion" should "make the selector THROW, never return a partial script" in { @@ -392,14 +405,114 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt Parser(group + "status = 'A'").isRight shouldBe true } - "a HAVING over a function of a NON-aggregate" should "keep its pre-existing behaviour" in { - // 🔴 Recorded, deliberately NOT changed by this story: `UPPER(status) = 'A'` is honoured by no - // mechanism and is silently dropped -- measured identically on `main`, with and without an - // aggregate conjunct. It references NO aggregate, so it is outside this issue's population; - // refusing it needs a second derivation of the include/exclude rule. + "a HAVING over a function of the GROUP BY KEY" should "be PUSHED into the query, not dropped" in { + // 🔴 This test used to PIN the silent drop. Lead ruling 2026-09-24: make it WORK. A function of + // the key is CONSTANT within a bucket, so filtering the documents by it keeps exactly the + // buckets that satisfy it -- EXECUTED on Elasticsearch 8.18.3 over `a` x2 / `b` / `cc`: + // `UPPER(status) = 'A'` returned [a, b, cc] before and returns [a] now, with `a`'s doc_count + // and every metric byte-identical to the unfiltered run. + // + // It contributes NOTHING to the bucket selector -- that is the point: it is a different + // mechanism, and the selector is left alone. script(group + "UPPER(status) = 'A'") shouldBe "1 == 1" + val pushed = parsed(group + "UPPER(status) = 'A'").pushedHavingKeys + pushed.map(_.sql) shouldBe Seq("UPPER(status) = 'A'") + parsed(group + "UPPER(status) = 'A'").searchCriteria.map(_.sql) shouldBe + Some("UPPER(status) = 'A'") + } + + it should "AND with the WHERE clause rather than replace it" in { + val s = parsed( + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status " + + "HAVING UPPER(status) = 'A'" + ) + s.searchCriteria.map(_.sql) shouldBe Some("amount > 1 AND UPPER(status) = 'A'") + } + + it should "leave the key predicates the terms filter already expresses alone" in { + // ⚠️ `HAVING status = 'A'` is a `terms` include TODAY. Pushing it as well would be correct and + // would move the emitted bytes of every shipped statement of that shape for nothing, so the + // eligibility test ASKS the real `Expression.includes` rather than re-deriving its rules. + parsed(group + "status = 'a'").pushedHavingKeys shouldBe empty + parsed(group + "status LIKE 'a%'").pushedHavingKeys shouldBe empty + parsed(group + "status = 'a'").searchCriteria shouldBe None + } + + it should "still leave the selector to the aggregate half of a conjunction" in { script(group + "COUNT(*) > 1 AND UPPER(status) = 'A'") shouldBe "(params.c == null ? false : (params.c > 1))" + parsed(group + "COUNT(*) > 1 AND UPPER(status) = 'A'").pushedHavingKeys.map(_.sql) shouldBe + Seq("UPPER(status) = 'A'") + } + + "a HAVING that reads neither an aggregate nor a GROUP BY key" should "be refused by name" in { + // It is not constant within a bucket, so no mechanism can honour it -- and pushing it into the + // query would filter DOCUMENTS and silently change every metric of every surviving group. + rejection(group + "UPPER(name) = 'X'") should include("can only filter on a GROUP BY key") + rejection(group + "name = 'x'") should include("can only filter on a GROUP BY key") + } + + "an OR that mixes a GROUP BY key with an aggregate" should "be refused, because it executes as an AND" in { + // 🔴 PRE-EXISTING silent wrong answer, closed by the ruling. MEASURED on `main` over the same + // fixture: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the disjunction is + // [a, b] -- the key became a terms `include` (removing `a`) and the metric a `bucket_selector` + // (removing `b`), so the OR answered the empty AND. + rejection(group + "COUNT(*) > 1 OR status = 'b'") should include("cannot OR a GROUP BY key") + rejection(group + "COUNT(*) > 1 OR UPPER(status) = 'B'") should include( + "cannot OR a GROUP BY key" + ) + } + + it should "still accept an OR of key predicates the terms filter can union" in { + // The complement: `include: [a, b]` IS the disjunction, so this must not be swept up. + Parser(group + "status = 'a' OR status = 'b'").isRight shouldBe true + parsed(group + "status = 'a' OR status = 'b'").pushedHavingKeys shouldBe empty + } + + "a HAVING inside a NESTED relation" should "keep its own mechanism" in { + // 🔴 HAVING has FOUR mechanisms, not two. A relation-scoped predicate is turned into a `filter` + // aggregation by the bridge, and scoping the new rules past it refused the repo's own shipped + // "complex query" fixture -- caught by that fixture going red. + Parser( + "SELECT inner_products.category AS cat, MIN(inner_products.price) AS p FROM stores store " + + "JOIN UNNEST(store.products) AS inner_products GROUP BY inner_products.category " + + "HAVING inner_products.deleted = false" + ).isRight shouldBe true + } + + "a function of a NESTED grouping key" should "be refused, because the push-down is not equivalent there" in { + // 🔴 The equivalence that licenses the push-down -- "the key is constant within a bucket, so + // filtering documents keeps whole buckets" -- does NOT hold for a nested key: the bucket is an + // inner object and a document filter keeps the whole PARENT, so a parent holding one matching + // and one non-matching object would still produce BOTH buckets. + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name HAVING UPPER(e.name) = 'X'" + ) should include("nested grouping key") + } + + "the representability gate" should "be re-asked of the RESOLVED statement" in { + // 🔴 `validate()` runs schema-less inside `Parser.apply`, and `update(Some(schema))` can change + // what a HAVING renders. Re-asking at `validateResolved()` is what keeps a post-resolution + // divergence a 400 naming the clause instead of an `IllegalStateException` escaping into the + // bridge (issue #250's family). Asserted on the ONE surface core calls. + val refused = unvalidated(group + "NULLIF(COUNT(*), 0) > 1") + refused.validateResolved() match { + case Left(msg) => msg should include("HAVING cannot be applied to") + case Right(_) => fail("the resolved statement accepted a HAVING the parser refuses") + } + // ... and it does not fire on a statement the gate accepts. + unvalidated(group + "GREATEST(COUNT(*), 0) > 1").validateResolved() shouldBe Right(()) + } + + "a metric computed outside the nested grouping" should "be refused rather than vanish" in { + // 🔴 The issue's SECOND conflation site. MEASURED on the branch before this rule: the statement + // answered HTTP 200 with `{"aggs":{"max_amount":{"max":{"field":"amount"}}}}` -- no + // `having_filter` AND no grouping at all. (The vanished GROUP BY is a separate pre-existing + // defect: the control answers `{"query":{"match_all":{}},"_source":true}` for the same input.) + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING COALESCE(MAX(amount), 0) > 1" + ) should include("computed outside the nested grouping") } "an aggregate written in WHERE" should "still be rejected, now through the wider walk" in { diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index fb97a179b..e2f87e047 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -831,7 +831,17 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + "HAVING ROUND(SUM(amount), 2) > 30", "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + - "HAVING COUNT(*) > 30 AND NULLIF(COUNT(*), 0) > 1" + "HAVING COUNT(*) > 30 AND NULLIF(COUNT(*), 0) > 1", + // 🔴 The RIGHT-operand family (F1): the left operand is a bare aggregate, so the gate used to + // be skipped entirely and these reached Elasticsearch as HTTP 400 / an internal-error label. + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > ROUND(SUM(amount), 2)", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > NULLIF(SUM(amount), 0)", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) BETWEEN 1 AND ABS(SUM(amount))", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > CASE WHEN SUM(amount) > 1 THEN 1 ELSE 0 END" ).foreach { sql => Await.result(client.run(sql), 60.seconds) match { case ElasticSuccess(result) => fail(s"[$sql] was accepted and answered $result") @@ -841,6 +851,86 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + it should "filter on a FUNCTION of the GROUP BY key, not ignore it" in { + // 🔴 Lead ruling 2026-09-24. A function of the key is CONSTANT within a group, so the condition + // is pushed into the query and whole groups are kept or dropped. MEASURED on `main`: this + // returned all 37 groups with HTTP 200 -- the condition was honoured by no mechanism at all. + // `cat_i` holds exactly `i` documents and every key is `cat_NN`, so `LENGTH(category) = 6` is + // every group and `SUBSTRING(category, 5, 2) = '37'` is exactly one. + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING SUBSTRING(category, 5, 2) = '37'" + ) match { + case ElasticSuccess(rows) => + rows.map(r => r.category -> r.cnt) shouldBe Seq("cat_37" -> categories.toLong) + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "leave a surviving group's metrics EXACTLY as they were without the key filter" in { + // 🔴 THE property the push-down rests on. Filtering documents may not change what a surviving + // group reports, or the optimisation would be a silent wrong answer. Asserted against the + // UNFILTERED run of the same grouping, not against a literal. + def counts(res: ElasticResult[Seq[CategoryCount]]): Map[String, Long] = res match { + case ElasticSuccess(rows) => rows.map(r => r.category -> r.cnt).toMap + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + val unfiltered = counts( + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category" + ) + ) + val filtered = counts( + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING UPPER(category) = 'CAT_37'" + ) + ) + filtered.keySet shouldBe Set("cat_37") + filtered("cat_37") shouldBe unfiltered("cat_37") + } + + it should "combine a key condition with an aggregate condition as an AND" in { + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 35 AND SUBSTRING(category, 5, 1) = '3'" + ) match { + case ElasticSuccess(rows) => rows.map(_.category).toSet shouldBe Set("cat_36", "cat_37") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "REFUSE an OR between a key condition and an aggregate condition" in { + // 🔴 PRE-EXISTING silent wrong answer: the key became a terms `include` and the aggregate a + // `bucket_selector`, so the OR was EXECUTED AS AN AND. MEASURED on `main` over a 4-document + // fixture: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO groups where the disjunction is + // two of them. + Await.result( + client.run( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 35 OR category = 'cat_01'" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"the OR was accepted and answered $result") + case ElasticFailure(error) => error.message should include("cannot OR a GROUP BY key") + } + } + + it should "REFUSE a HAVING on a column that is neither grouped nor aggregated" in { + Await.result( + client.run( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING UPPER(id) = 'X'" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"accepted and answered $result") + case ElasticFailure(error) => + error.message should include("can only filter on a GROUP BY key") + } + } + it should "still return every group when the un-expressible predicate is NOT there" in { // 🔴 Non-vacuity for the refusals above: the same statements WITHOUT the offending function // are accepted, so the rejection is about the predicate and not about the fixture. From 74b99e0fa52dc0cba425c7f8d5b693dee7c18776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Thu, 24 Sep 2026 15:19:16 +0200 Subject: [PATCH 3/6] fix(sql): a HAVING over the GROUP BY key is honoured at BUCKET level, or refused (#389 round 3) Round 2 honoured `HAVING ()` by pushing the predicate into the QUERY, on the argument that a function of the key is constant within a bucket. That argument holds only when the key is single-valued AND the bucket key is the column itself, and both premises fail in the shipped grammar. Two HIGH silent wrong answers, both reproduced on real ES 8.18.3: * a MULTI-VALUED key filtered on `doc['tags'].value` reads the FIRST value only, so a document is kept by one of its values and every OTHER value still forms its own bucket -- bucket `b` survived a filter naming only `A`; * `GROUP BY DAY(d) HAVING YEAR(d) = 2025` removes DOCUMENTS from buckets it does not remove, so a SURVIVING bucket's `doc_count` and every metric in it change. A document filter is not a bucket filter. The push-down (`pushedHavingKeys`, `searchCriteria`) is DELETED and both bridges' query seam is byte-identical to `455433ae` again. What replaces it is ONE classifier, `SingleSearch.keyPredicateOutcome`, with three outcomes -- `TermsFilter`, `Refused(reason)` and a `ScriptedTerms` that is specified but NOT BUILT, so its population is refused by name rather than silently dropped. Both `validate()` and `validateResolved()` read that one value, and `keyExpressibleByTerms` CALLS the real `Expression.includes` / `excludes` instead of re-deriving what the terms filter can spell, so the classifier and the emission cannot disagree by construction. Also in this scope: * `Having.script` is NOT dead code -- it has a production caller in softclient4es-extensions (`graph/Stage.scala:291`), which builds a materialized view's `bucket_selector` from it. Restored `@deprecated`, now gated on `unrepresentable`, which is made public so that caller can read the REASON. The extensions-side repair needs a published core and is NOT made here. * the Unscoped rule is UNGATED: a HAVING over a column that is neither an aggregate nor a GROUP BY key is refused whether or not the statement groups. * the OR-homogeneity rule is re-derived over leaves that INCLUDE relation leaves, and its dead `unsupported-key` answer -- unreachable because the key rule refuses that population first -- is deleted rather than kept beside a live copy. * the refusal reason reaches the caller at every venue: `search`, `searchAsync`, `searchWithInnerHits` and `ScrollApi`. * `havingLeaves` is a `lazy val`; four validation rules walk it. Measured: 8/8 mutations RED; sql 1620 / core 1141 / bridge 231 / es6bridge 227; cross compile 2.12.20 + 2.13.16; the exact CI lint line; GrammarDiffProbe over 18,062 corpus inputs against the `455433ae` control with 0 differing rows; `GroupByCompletenessSpec` 38/38 on real ES 8.18.3 and 38/38 on ES 6.8.23. Cost isolates to one shape -- `group by + having` validate 0.40 -> 3.10 us, resolve 2.00 -> 6.10 us -- with parse and emission showing overlapping ranges, i.e. no signal. Closed Issue #389 Co-Authored-By: Claude Opus 5 (1M context) --- .../elastic/sql/bridge/package.scala | 9 +- .../sql/HavingFunctionEmissionSpec.scala | 43 +-- .../elastic/client/ScrollApi.scala | 10 +- .../elastic/client/SearchApi.scala | 6 +- documentation/sql/dql_statements.md | 53 +-- .../elastic/sql/bridge/package.scala | 9 +- .../sql/HavingFunctionEmissionSpec.scala | 54 ++-- .../elastic/sql/query/Having.scala | 35 +- .../elastic/sql/query/package.scala | 301 ++++++++---------- .../sql/parser/InPredicateRenderSpec.scala | 30 +- .../HavingOverAggregateFunctionSpec.scala | 171 +++++----- .../client/GroupByCompletenessSpec.scala | 76 +++-- 12 files changed, 428 insertions(+), 369 deletions(-) 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 6b4af308c..bb88acb44 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 @@ -540,14 +540,7 @@ package object bridge { val nestedWithoutCriteriaQuery: Option[Query] = requestToNestedWithoutCriteriaQuery(request) var _search: SearchRequest = search("") query { - // 🔴 `searchCriteria`, NOT `where.criteria` (issue #389, lead ruling 2026-09-24): the WHERE - // clause conjoined with every HAVING predicate over a GROUP BY KEY. A function of the key is - // CONSTANT within a bucket, so filtering the DOCUMENTS by it keeps exactly the buckets that - // satisfy it and changes no surviving bucket's metrics -- which is what the `terms` - // `include` / `exclude` already does for the shapes it can spell, and this generalises it to - // the ones it cannot (`HAVING UPPER(status) = 'A'`). `sql` owns the rule and `validate()` - // refuses what the equivalence does not cover, so this is ONE line in each bridge copy. - searchCriteria.map(_.asQuery()) match { + where.flatMap(_.criteria.map(_.asQuery())) match { case Some(c) => val baseQuery = c nestedWithoutCriteriaQuery match { diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 582a18852..b498208c3 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -195,35 +195,40 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { } // --------------------------------------------------------------------------------------------- - // The GROUP BY key push-down (lead ruling 2026-09-24) + // The GROUP BY key population -- BUCKET level only (round 3) // - // 🔴 These assert the WIRING, not the derivation. `SingleSearch.searchCriteria` computing the - // right thing proves nothing if the bridge still reads `where.criteria` -- a mutation doing - // exactly that left the whole `sql` suite green. + // 🔴 The round-2 document push-down is GONE. Its licence ("a function of the key is constant + // within a bucket") is false when the KEY is a function of the column and the predicate reads the + // column, and false again on a MULTI-VALUED field where one document belongs to several buckets. + // Both were measured as HTTP 200 wrong answers on Elasticsearch 8.18.3. // --------------------------------------------------------------------------------------------- - "a HAVING over a function of the GROUP BY key" should "reach the QUERY, not the aggregation" in { - val q = queryOf(group + "UPPER(status) = 'A'") - q should not include """"query":{"match_all":{}}""" - q should include("toUpperCase") - // ... and it must NOT become a bucket_selector: it is a different mechanism. + "a key predicate the terms filter expresses" should "stay in the terms filter and touch nothing else" in { + val q = queryOf(group + "status = 'a'") + q should include(""""include":["a"]""") + // 🔴 No query filter: the round-2 push-down put one here, and that is what made a multi-valued + // grouping wrong. + q should include(""""query":{"match_all":{}}""") q should not include "having_filter" } - it should "AND with the WHERE clause rather than replace it" in { + it should "leave the WHERE clause exactly as written" in { val q = queryOf( - "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status " + - "HAVING UPPER(status) = 'A'" + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status HAVING status = 'a'" ) - q should include(""""amount"""") - q should include("toUpperCase") + q should include(""""include":["a"]""") + q should not include "toUpperCase" } - it should "leave a key predicate the terms filter already expresses in the terms filter" in { - // Byte-identical to `main` for every shipped `HAVING = `: no query filter appears. - val q = queryOf(group + "status = 'a'") - q should include(""""include":["a"]""") - q should include(""""query":{"match_all":{}}""") + "a key predicate the terms filter cannot express" should "never reach emission at all" in { + Seq("UPPER(status) = 'A'", "LENGTH(status) = 1").foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } } // --------------------------------------------------------------------------------------------- diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala index 5ecf72dfa..cd1334e55 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala @@ -282,8 +282,16 @@ trait ScrollApi extends ElasticClientHelpers with SchemaCacheTtlApi { scroll(multiple, config) case None => + // Issue #389 / F7 -- carry the parser's OWN reason. A whole family of SEMANTIC refusals + // reaches this branch, each naming a clause and a remedy; the generic sentence threw + // every one of them away. Source.failed( - new IllegalArgumentException("SQL query does not contain a valid search request") + new IllegalArgumentException( + (statement match { + case s: SelectStatement => s.parseError + case _ => None + }).getOrElse("SQL query does not contain a valid search request") + ) ) } diff --git a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala index e76420d5e..28fa6f259 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala @@ -884,7 +884,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC Future.successful( ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request: ${statement.sql}", + message = invalidSearchRequest(statement, statement.sql), operation = Some("searchAsync") ) ) @@ -933,7 +933,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC Future.successful( ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request: $query", + message = invalidSearchRequest(statement, query), operation = Some("searchAsync") ) ) @@ -1462,7 +1462,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC ) ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request: ${sql.query}", + message = invalidSearchRequest(sql, sql.query), operation = Some("searchWithInnerHits") ) ) diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 6e73a5ddc..351255c9e 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -795,8 +795,6 @@ ORDER BY COUNT(*) DESC; over-approximation: the boxing conversion compiles in *some* positions of a group-filter script and not others, so the engine refuses it in all of them rather than guess. The same functions work normally in `WHERE`, in the `SELECT` list and in `ORDER BY`; - - a rendering that compares a metric as text — `HAVING COALESCE(MAX(created), '2020-01-01') > - '2019-01-01'`. A group filter reads every metric as a number (a date as epoch milliseconds); - `CASE ... END` in `HAVING`, which needs a document and a group filter has none; - a function applied to a `SELECT` aggregate **alias** — `COUNT(*) AS c ... HAVING NULLIF(c, 0) > 1`; - an aggregate computed outside a nested grouping — `... JOIN UNNEST(t.emails) AS e GROUP BY @@ -811,22 +809,41 @@ ORDER BY COUNT(*) DESC; differs from arithmetic over aggregates, which IS a valid `SELECT` item (`MAX(price) - MIN(price) AS price_range`) and is the reason the rule above tells you to alias THAT one. -- A **function of the `GROUP BY` key** in `HAVING` filters the groups: `HAVING UPPER(city) = 'PARIS'`, - `HAVING LENGTH(status) = 1`, `HAVING SUBSTRING(code, 1, 1) = 'A'`. A function of the key has the - same value for every document of a group, so the condition is applied to the documents and whole - groups are kept or dropped — every surviving group's counts and metrics are exactly what they - would have been without it. - - A predicate naming a column that is **neither** the `GROUP BY` key **nor** an aggregate is - refused (`HAVING UPPER(name) = 'X'` when the grouping is by `city`): it is not constant within a - group, so it would have to filter documents and silently change every metric. Put it in `WHERE`, - or add the column to the `GROUP BY`. - - ⚠️ An `OR` between a key condition and an aggregate condition is refused - (`HAVING COUNT(*) > 1 OR status = 'b'`). The two are different mechanisms — a key filter and a - group filter — and their disjunction cannot be expressed; before this was refused it was - **executed as an `AND`**. An `OR` of key conditions (`HAVING status = 'a' OR status = 'b'`) is - supported. -- One un-expressible condition rejects the whole `HAVING`, never just its own half: a statement is - answered with every condition it was written with, or with an error. + +#### Comparing a DATE aggregate + +⚠️ **A comparison between a date aggregate and a date literal is refused**, function or not: +`HAVING MAX(created) > '2019-01-01'` and `HAVING MIN(created) < '2020-01-01'` are rejected at parse +time. A group filter reads every metric as a number — a date as epoch milliseconds — so the +generated comparison is text against a number and Elasticsearch fails the whole search with a +`class_cast_exception`. Compare in `WHERE` instead, or filter the result outside the query. + +### Conditions on the GROUP BY key + +A `HAVING` condition over the grouping key filters GROUPS, and it is applied by the `terms` filter — +so it is correct for a multi-valued field, where one document belongs to several groups. + +```sql +SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city = 'Paris'; +SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city LIKE 'P%'; +SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city <> 'Lyon'; +``` + +- Supported: a direct comparison of the key — `=`, `<>`, `IN`, `LIKE` / `RLIKE` — and an `OR` of + them, which the filter unions into one list (`HAVING city = 'Paris' OR city = 'Lyon'`). +- ⚠️ **A FUNCTION of the key is refused** (`HAVING UPPER(city) = 'PARIS'`, + `HAVING LENGTH(status) = 1`). The terms filter can only express a direct comparison, and the + alternatives are unsound: filtering documents instead would keep or drop a multi-valued document + WHOLE, and would change the counts of surviving groups whenever the key is itself a function of + the column (`GROUP BY DAY(d) HAVING YEAR(d) = 2025`). Compare the key itself, or filter in + `WHERE`. +- A predicate naming a column that is **neither** the `GROUP BY` key **nor** an aggregate is refused + (`HAVING UPPER(name) = 'X'` when the grouping is by `city`) — with or without a `GROUP BY`. +- ⚠️ An `OR` whose branches need **different mechanisms** is refused: a group filter + (`bucket_selector`), a key filter (`terms`) and a nested filter are separate stages, so + `HAVING COUNT(*) > 1 OR city = 'Paris'` would be executed as a conjunction. An `OR` within one + mechanism is supported. +- An `AND` across mechanisms is fine — each stage applies its own half. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter script null-checks every metric before comparing it. 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 61390b829..ade951743 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 @@ -533,14 +533,7 @@ package object bridge { val nestedWithoutCriteriaQuery: Option[Query] = requestToNestedWithoutCriteriaQuery(request) var _search: SearchRequest = search("") query { - // 🔴 `searchCriteria`, NOT `where.criteria` (issue #389, lead ruling 2026-09-24): the WHERE - // clause conjoined with every HAVING predicate over a GROUP BY KEY. A function of the key is - // CONSTANT within a bucket, so filtering the DOCUMENTS by it keeps exactly the buckets that - // satisfy it and changes no surviving bucket's metrics -- which is what the `terms` - // `include` / `exclude` already does for the shapes it can spell, and this generalises it to - // the ones it cannot (`HAVING UPPER(status) = 'A'`). `sql` owns the rule and `validate()` - // refuses what the equivalence does not cover, so this is ONE line in each bridge copy. - searchCriteria.map(_.asQuery()) match { + where.flatMap(_.criteria.map(_.asQuery())) match { case Some(c) => val baseQuery = c nestedWithoutCriteriaQuery match { diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index b5eaef426..47f76d585 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -13,10 +13,10 @@ import scala.jdk.CollectionConverters._ /** Issue #389 -- the EMITTED Elasticsearch query for a `HAVING` over a FUNCTION of an aggregate, * measured against the HAND-MAINTAINED es6 bridge (AC-7). * - * 🔴 One line of each bridge changed (the query seam now reads `searchCriteria`); everything else - * lives in `sql`. This twin is what PROVES it -- the same derived matrix and the same pins against - * a different elastic4s major, with two expectations differing (elastic4s 6 renders a single-value - * `terms` `include` / `exclude` as a bare string). + * 🔴 NO bridge file changed in round 3 — the whole rule lives in `sql`. This twin is what PROVES + * it: the same derived matrix and the same pins against a different elastic4s major, with two + * expectations differing (elastic4s 6 renders a single-value `terms` `include` / `exclude` as a + * bare string). * * The `sql` half (detection, the representability gate, the refusals) is pinned in * `HavingOverAggregateFunctionSpec`. What is measured HERE is what Elasticsearch is actually sent: @@ -201,35 +201,40 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { } // --------------------------------------------------------------------------------------------- - // The GROUP BY key push-down (lead ruling 2026-09-24) + // The GROUP BY key population -- BUCKET level only (round 3) // - // 🔴 These assert the WIRING, not the derivation. `SingleSearch.searchCriteria` computing the - // right thing proves nothing if the bridge still reads `where.criteria` -- a mutation doing - // exactly that left the whole `sql` suite green. + // 🔴 The round-2 document push-down is GONE. Its licence ("a function of the key is constant + // within a bucket") is false when the KEY is a function of the column and the predicate reads the + // column, and false again on a MULTI-VALUED field where one document belongs to several buckets. + // Both were measured as HTTP 200 wrong answers on Elasticsearch 8.18.3. // --------------------------------------------------------------------------------------------- - "a HAVING over a function of the GROUP BY key" should "reach the QUERY, not the aggregation" in { - val q = queryOf(group + "UPPER(status) = 'A'") - q should not include """"query":{"match_all":{}}""" - q should include("toUpperCase") - // ... and it must NOT become a bucket_selector: it is a different mechanism. + "a key predicate the terms filter expresses" should "stay in the terms filter and touch nothing else" in { + val q = queryOf(group + "status = 'a'") + q should include(""""include":"a"""") + // 🔴 No query filter: the round-2 push-down put one here, and that is what made a multi-valued + // grouping wrong. + q should include(""""query":{"match_all":{}}""") q should not include "having_filter" } - it should "AND with the WHERE clause rather than replace it" in { + it should "leave the WHERE clause exactly as written" in { val q = queryOf( - "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status " + - "HAVING UPPER(status) = 'A'" + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status HAVING status = 'a'" ) - q should include(""""amount"""") - q should include("toUpperCase") + q should include(""""include":"a"""") + q should not include "toUpperCase" } - it should "leave a key predicate the terms filter already expresses in the terms filter" in { - // Byte-identical to `main` for every shipped `HAVING = `: no query filter appears. - val q = queryOf(group + "status = 'a'") - q should include(""""include":"a"""") - q should include(""""query":{"match_all":{}}""") + "a key predicate the terms filter cannot express" should "never reach emission at all" in { + Seq("UPPER(status) = 'A'", "LENGTH(status) = 1").foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } } // --------------------------------------------------------------------------------------------- @@ -255,8 +260,7 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may // cost the other. val q = queryOf(group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'") - // elastic4s 6 renders a single-value `exclude` as a bare string, 7+ as an array -- the ONE - // documented emission difference this matrix touches. + // elastic4s 6 renders a single-value `exclude` as a bare string, 7+ as an array. q should include(""""exclude":"x"""") q should include("""(params.c != null ? params.c : 0) > 1""") } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala index 31a15f987..fd1294953 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala @@ -118,7 +118,40 @@ case class Having(criteria: Option[Criteria]) extends Updateable { /** Every condition of this clause the engine cannot express as a `bucket_selector` (issue #389), * in statement order. Refused by name in `SingleSearch.validate()`; EMPTY is the only shape that * reaches emission. + * + * 🔴 PUBLIC on purpose. `softclient4es-extensions` builds the materialized-view transform's + * `TransformBucketSelectorConfig` from [[script]] and has no other way to tell "this clause has + * nothing to filter" from "this clause cannot be expressed" -- which is #389's conflation, live + * inside MV enrichment. This is the reason it needs, and the only thing core can give it until + * it is rebuilt against a published `0.24.0`. */ - private[query] def unrepresentable: Seq[MetricSelector.Unrepresentable] = + def unrepresentable: Seq[MetricSelector.Unrepresentable] = criteria.toSeq.flatMap(MetricSelectorScript.unrepresentable) + + /** The `bucket_selector` source for this clause, or `None` when there is nothing to filter. + * + * 🔴 NOT dead code -- round 1 recorded it as having no production caller and that was WRONG: + * `softclient4es-extensions`'s `graph/Stage.scala:291` reads it to build a materialized view's + * `TransformBucketSelectorConfig`. That is a FIFTH HAVING mechanism, outside this repo. + * + * ⚠️ It still answers `None` for a clause the engine cannot express, which is exactly the + * conflation #389 closed everywhere else -- so a materialized view over such a HAVING is + * enriched with NO filter. Core cannot fix that from here: switching this to + * `MetricSelectorScript.metricSelector` would THROW inside the extension (a 500), and the + * extension must decide for itself. [[unrepresentable]] is public so it can. See `§9` of the + * story artifact (§10.B) for the exact change extensions needs once `0.24.0` is published. + */ + @deprecated("read `unrepresentable` first, then `MetricSelectorScript.metricSelector`", "0.24.0") + def script: Option[String] = criteria.flatMap { criteria => + if (unrepresentable.nonEmpty) None + else { + val fullScript = MetricSelectorScript + .metricSelector(criteria) + .replaceAll("1 == 1 &&", "") + .replaceAll("&& 1 == 1", "") + .replaceAll("1 == 1", "") + .trim + if (fullScript.nonEmpty) Some(fullScript) else None + } + } } 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 137da62e3..4dde0452d 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 @@ -710,15 +710,24 @@ package object query { ) } - /** Where a `HAVING` leaf predicate can be evaluated (issue #389, lead ruling 2026-09-24). + /** Where a `HAVING` leaf predicate can be evaluated (issue #389). * - * 🔴 The whole design rests on one property: **a function of a GROUP BY key is CONSTANT within - * a bucket**, because every document in that bucket has the same key value. So `HAVING f(key) - * v` selects whole buckets, and filtering the DOCUMENTS by the same predicate keeps - * exactly those buckets and removes no document from a surviving one -- which is why every - * metric of a surviving bucket is unchanged. That is what the `terms` `include` / `exclude` - * already is: a key filter. The push-down generalises it to the predicates `include` / - * `exclude` cannot spell. + * 🔴 ROUND 3 -- the DOCUMENT push-down this used to authorise is GONE. Its licence was "a + * function of the GROUP BY key is constant within a bucket, so filtering the documents keeps + * whole buckets", and that is false twice over, both MEASURED on Elasticsearch 8.18.3: + * + * - the key can be a FUNCTION of the column, and the predicate can read the COLUMN. Over + * `2024-01-03, 2024-02-03, 2025-03-03, 2025-04-05`: `GROUP BY DAY(d)` -> dd=3 c=3 · dd=5 + * c=1 `GROUP BY DAY(d) HAVING YEAR(d) = 2025` -> dd=3 c=1 · dd=5 c=1 HTTP 200 A SURVIVING + * bucket's count changed -- the exact wrong answer the push-down was supposed to be immune + * to. + * - a MULTI-VALUED field puts one document in several buckets, and a document filter keeps + * or drops it WHOLE. Over `tags = ["a","b"], ["b"], ["c"], ["a"]`: `GROUP BY tags HAVING + * UPPER(tags) = 'A'` kept bucket `b`, which fails the predicate. The `terms` `include` + * path answers `a` alone, so the push-down was strictly LESS sound than the mechanism it + * was meant to generalise. + * + * A key predicate is therefore honoured at the BUCKET level or not at all. */ private[query] sealed trait HavingScope private[query] object HavingScope { @@ -726,7 +735,8 @@ package object query { /** Reads an aggregate: the `bucket_selector` population. */ case object Metric extends HavingScope - /** Reads only GROUP BY keys: constant within a bucket, so it can be pushed into the query. */ + /** Reads only GROUP BY keys. Honoured by [[keyPredicateOutcome]], never by a document filter. + */ case object GroupKey extends HavingScope /** Neither -- it names a column that is not a grouping key, so it is not constant within a @@ -735,31 +745,38 @@ package object query { case object Unscoped extends HavingScope } - /** The GROUP BY bucket a leaf identifier reads, when it reads one. + /** How a GROUP BY key predicate is honoured -- the ONE classifier both emission paths read. * - * 🔴 TWO shapes, because the grammar builds a function over a column in two different ways and - * only one of them attaches `Identifier.bucket`. MEASURED over `GROUP BY a`: - * - `UPPER(a)` / `ABS(a)` build a WRAPPER identifier (`name = ""`) whose argument is a - * separate identifier `a` carrying `bucket = Some(a)`; - * - `DAY(a)` / `YEAR(a)` / `EPOCHDAY(a)` put the function on the identifier ITSELF, so the - * leaf IS `a` with `bucket = None` -- the bucket lookup keys on `identifierName`, which is - * `DAY(a)` there, not `a`. - * - * Matching the leaf's COLUMN name against each bucket's own column covers the second shape and - * is sound for the push-down: if the leaf names the column the `terms` aggregation groups by, - * its value is by construction the same for every document in a bucket. A false NEGATIVE only - * refuses; a false POSITIVE would push a non-constant predicate and silently change metrics, - * which is why the comparison is on the physical column and on nothing else. + * 🔴 Two mechanisms for one feature is two derivations that can disagree + * (`project_self_join_alias_resolution`), so neither emission path re-decides: they read this + * value. `TermsFilter` means "the `terms` `include` / `exclude` path already expresses it" and + * is answered by the REAL `Expression.includes` / `excludes`, never by a copy of their rules. */ + private[query] sealed trait KeyPredicateOutcome + private[query] object KeyPredicateOutcome { + + /** The `terms` `include` / `exclude` filter expresses this predicate. Emission is unchanged: + * the bridge's existing include/exclude call is what applies it. + */ + case object TermsFilter extends KeyPredicateOutcome + + /** Neither bucket-level mechanism can express it, so the statement is refused by name. + * + * ⚠️ The lead's round-3 ruling specifies a THIRD outcome here -- a scripted `terms` source + * whose script returns only the key values satisfying the predicate, which is correct on a + * multi-valued field and on a function-of-column key by construction. It is NOT BUILT: see + * `§10.A.3` of the story artifact for the design, the Painless it must emit (verified + * running on a real cluster) and what remains. Until it exists this arm carries that + * population, so the predicate is REFUSED rather than silently dropped or wrongly applied. + */ + case class Refused(reason: String) extends KeyPredicateOutcome + } + private[query] def keyBucketOf(leaf: Identifier): Option[Bucket] = { - // 🔴 Resolve through THIS statement's bucket list, never through `Identifier.bucket`. - // `Identifier.bucket` is a COPY attached during `update()` and deliberately NOT re-updated - // (the story-21.3 / #253 desync class), and MEASURED here it reports `nestedElement = None` - // for a bucket the statement lists as `Some(e)`. Reading the copy made `readsNestedKey` - // answer `false` for `GROUP BY e.name HAVING UPPER(e.name) = 'X'`, so the predicate was - // PUSHED INTO THE QUERY -- the one case the equivalence does NOT license, because a document - // filter on a nested field keeps the whole parent and its non-matching objects keep their - // buckets. Found by the mutation matrix reporting the nested refusal as unguarded. + // 🔴 Resolve through THIS statement's bucket list, never through `Identifier.bucket`, which + // is a COPY attached during `update()` and deliberately NOT re-updated (the story-21.3 / #253 + // desync class) -- MEASURED reporting `nestedElement = None` for a bucket the statement lists + // as nested. val byName = buckets.find(b => b.identifier.name.nonEmpty && b.identifier.name == leaf.name) byName.orElse(leaf.bucket.flatMap(attached => buckets.find(_.name == attached.name))) } @@ -778,19 +795,11 @@ package object query { else HavingScope.Unscoped } - /** Does this key predicate read a NESTED bucket? Asked of the bucket the leaf RESOLVES to, not - * of `Identifier.bucket` -- which is `None` for the date-function shape above, so a check on - * it would silently miss exactly the statements this refuses. - */ - private[query] def readsNestedKey(e: Expression): Boolean = - e.nested || namedLeavesOf(e).exists(l => keyBucketOf(l).exists(_.nestedElement.isDefined)) - - /** Does the `terms` `include` / `exclude` path ALREADY express this key predicate? + /** Does the `terms` `include` / `exclude` path express this key predicate? * - * 🔴 Asked of the REAL function (`Expression.includes` / `excludes`), never of a copy of its - * rules. A second derivation of "which key predicates the terms filter can spell" is the - * story-21.3 desync class, and it would silently change every shipped `HAVING status = 'A'` - * the first time the two disagreed. + * 🔴 Asked of the REAL function, never of a copy of its rules -- a second derivation of "which + * key predicates the terms filter can spell" is the story-21.3 desync class and would silently + * change every shipped `HAVING = ` the first time the two disagreed. */ private[query] def keyExpressibleByTerms(e: Expression): Boolean = { val empty = BucketIncludesExcludes() @@ -799,20 +808,37 @@ package object query { ) } + private[query] def keyPredicateOutcome(e: Expression): KeyPredicateOutcome = + if (keyExpressibleByTerms(e)) KeyPredicateOutcome.TermsFilter + else if (readsNestedKey(e)) + KeyPredicateOutcome.Refused( + s"HAVING cannot filter on ${e.sql}: the grouping key is a nested object, and a bucket " + + "filter over it is not expressible. Filter it in WHERE, or group by a field of the parent." + ) + else + KeyPredicateOutcome.Refused( + s"HAVING cannot filter on ${e.sql}: a condition over the GROUP BY key is applied by the " + + "terms filter, which can only express a direct comparison of the key (=, <>, LIKE, IN). " + + "Compare the key itself, or filter in WHERE." + ) + + /** Does this key predicate read a NESTED bucket? Asked of the bucket the leaf RESOLVES to. */ + private[query] def readsNestedKey(e: Expression): Boolean = + e.nested || namedLeavesOf(e).exists(l => keyBucketOf(l).exists(_.nestedElement.isDefined)) + /** Every leaf `Expression` of the FLAT part of the HAVING tree, in statement order. * - * 🔴 A predicate scoped to a NESTED relation is deliberately NOT here, and that is the same - * boundary the whole-table block below already draws: such a predicate has its OWN - * long-standing mechanism (`requestToNestedFilterAggregation` turns it into a `filter` - * aggregation scoped to the inner-hits path), it is exercised by the bridge fixtures, and it - * is not discarded. MEASURED: without this exclusion the repo's own "complex query" fixture -- - * `... JOIN UNNEST(store.products) AS inner_products ... HAVING inner_products.deleted = - * false` - * -- is refused, which would have broken a shipped, WORKING statement. HAVING has FOUR - * mechanisms, not two: the `bucket_selector`, the `terms` include/exclude, this nested filter, - * and the query push-down added here. + * 🔴 A predicate scoped to a NESTED relation is deliberately NOT here, the same boundary the + * whole-table block below draws: it has its OWN mechanism (`requestToNestedFilterAggregation` + * turns it into a `filter` aggregation scoped to the inner-hits path). MEASURED: without this + * exclusion the repo's own "complex query" fixture is refused. HAVING has FIVE mechanisms -- + * the `bucket_selector`, the `terms` include/exclude, this nested filter, the extensions' + * materialized-view transform (see `Having.script`), and the scripted `terms` source that the + * round-3 ruling specifies and that is not built yet. + * + * A `lazy val`: three validation rules and the key logic all walk it (review LOW-9). */ - private[query] def havingLeaves: Seq[Expression] = { + private[query] lazy val havingLeaves: Seq[Expression] = { def leaves(c: Criteria): Seq[Expression] = c match { case Predicate(l, _, r, _, _) => leaves(l) ++ leaves(r) case _: ElasticRelation => Nil @@ -822,46 +848,6 @@ package object query { having.flatMap(_.criteria).toSeq.flatMap(leaves) } - /** The group-key predicates the query must carry: every `GroupKey` leaf the `terms` `include` / - * `exclude` path cannot already spell, and that is not scoped to a nested bucket. - * - * ⚠️ NOT applied to a key the terms filter already expresses -- pushing it as well would be - * correct (the same predicate ANDed twice) but would move the emitted bytes of every shipped - * `HAVING = ` statement for nothing. - * - * ⚠️ NOT applied under a NESTED relation: there the bucket is an inner object and a - * document-level filter keeps the whole PARENT, so a parent with one matching and one - * non-matching object would keep both buckets. `validate()` refuses that shape instead. - */ - private[query] lazy val pushedHavingKeys: Seq[Expression] = - if (groupBy.isEmpty) Nil - else - havingLeaves.filter(e => - havingScopeOf(e) == HavingScope.GroupKey && !keyExpressibleByTerms(e) && - !readsNestedKey(e) - ) - - /** The DOCUMENT-level criteria this search applies: the `WHERE` clause conjoined with every - * pushed-down group-key `HAVING` predicate (issue #389). - * - * 🔴 Read by the bridges INSTEAD of `where.criteria`. It is a derivation, never a rewrite of - * the AST: moving the predicate into `where` would change what [[sql]] renders, and - * `MaterializedViewExtension` PERSISTS that render and re-runs it -- the story-21.3 / #318 - * fixed-point lesson. The statement still renders exactly as it was written. - */ - lazy val searchCriteria: Option[Criteria] = { - val pushed = pushedHavingKeys - val whereCriteria = where.flatMap(_.criteria) - if (pushed.isEmpty) whereCriteria - else { - val conjunction = pushed.reduceLeft[Criteria]((acc, e) => Predicate(acc, AND, e)) - whereCriteria match { - case Some(w) => Some(Predicate(w, AND, conjunction)) - case None => Some(conjunction) - } - } - } - lazy val excludes: Seq[String] = select.except.map(_.fields.map(_.sourceField)).getOrElse(Nil) lazy val sources: Seq[String] = from.tables.map(_.name) @@ -1134,21 +1120,19 @@ package object query { } } _ <- { - // 🔴 Issue #389, lead ruling 2026-09-24 -- a HAVING over a FUNCTION of the GROUP BY KEY - // must WORK. It is pushed into the query (see `pushedHavingKeys`); what cannot be pushed - // is refused HERE. Two shapes, both MEASURED as silent wrong answers on `main` over a - // 4-document fixture (`a` x2, `b`, `cc`): - // - // HAVING UPPER(status) = 'A' -> buckets [a, b, cc] (should be [a]) - // HAVING LENGTH(status) = 1 -> buckets [a, b, cc] (should be [a, b]) + // 🔴 Issue #389 -- (a) a predicate that reads neither an aggregate NOR a grouping key is + // not constant within a bucket, so NO mechanism can honour it: the terms filter cannot + // spell it, the selector cannot read a document, and filtering documents would silently + // change every metric of every surviving group. MEASURED as a silent wrong answer on + // `main` and refused since. // - // (a) A predicate that reads neither an aggregate NOR a grouping key is not constant - // within a bucket, so NO mechanism can honour it -- the terms filter cannot spell it, - // the selector cannot read a document, and pushing it into the query would filter - // DOCUMENTS and silently change every metric of every surviving group. - val unscoped = havingLeaves - .filter(e => groupBy.isDefined && havingScopeOf(e) == HavingScope.Unscoped) - unscoped.headOption match { + // ⚠️ NOT gated on `groupBy.isDefined` (review MEDIUM-4). With no GROUP BY there are no + // buckets, so every named leaf is `Unscoped` and every aggregate leaf is `Metric` -- the + // whole-table block below refuses a BARE column (`HAVING status = 'a'`) but its + // `filter(_.name.nonEmpty)` probe cannot see a FUNCTION of one, which is S8's original + // hole. MEASURED: `SELECT COUNT(*) AS c FROM t HAVING UPPER(status) = 'A'` answered + // `{"c":{"value":4}}` with the predicate gone. + havingLeaves.filter(e => havingScopeOf(e) == HavingScope.Unscoped).headOption match { case Some(e) => Left( s"HAVING can only filter on a GROUP BY key or on an aggregate; ${e.sql} is " + @@ -1158,72 +1142,69 @@ package object query { } } _ <- { - // (b) ... and a key predicate scoped to a NESTED bucket cannot be pushed either: there - // the bucket is an inner object, so a document-level filter keeps the whole PARENT - // and a parent holding one matching and one non-matching object would still produce - // BOTH buckets. The equivalence that licenses the push-down does not hold, so the - // shape is refused rather than answered wrongly. What the terms `include` / `exclude` - // already expresses is untouched -- it filters the BUCKET, not the document. + // (b) ... and a key predicate neither bucket-level mechanism can express is refused by + // the ONE classifier both emission paths read. havingLeaves - .find(e => - groupBy.isDefined && havingScopeOf(e) == HavingScope.GroupKey && - !keyExpressibleByTerms(e) && readsNestedKey(e) - ) - .map(e => - Left( - s"HAVING cannot apply ${e.sql} to a nested grouping key: the filter would keep the " + - "whole parent document. Filter it in WHERE, or compare the key directly." - ) - ) + .filter(e => groupBy.isDefined && havingScopeOf(e) == HavingScope.GroupKey) + .map(keyPredicateOutcome) + .collectFirst { case KeyPredicateOutcome.Refused(reason) => Left(reason) } .getOrElse(Right(())) } _ <- { - // (c) 🔴 An OR that mixes the two populations is NOT expressible, and today it is - // executed as an AND. MEASURED on `main` over the same fixture: - // - // HAVING COUNT(*) > 1 OR status = 'b' -> buckets [] (should be [a, b]) + // (c) 🔴 An OR is honoured by ONE mechanism or by none: a `bucket_selector` script cannot + // read the bucket KEY, the `terms` filter cannot read a metric, and the nested filter + // aggregation is a third scope entirely. An OR whose branches need different mechanisms + // is therefore EXECUTED AS AN AND -- measured on `main` over a 4-document fixture, + // `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the disjunction is two + // of them, because the key became a terms `include` (removing one) and the metric a + // `bucket_selector` (removing the other). // - // `status = 'b'` becomes a terms `include` (which REMOVES bucket `a` outright) while - // `COUNT(*) > 1` becomes a `bucket_selector` (which removes `b`), so the disjunction - // answers the empty conjunction. A `bucket_selector` script cannot read the bucket - // KEY and a query filter cannot be OR-ed with a bucket filter, so no emission can - // express it. This is PRE-EXISTING in the include/exclude path; the ruling closes it. + // ⚠️ Review MEDIUM-5: the rule is stated as HOMOGENEITY, and its message names the real + // cause. It used to say "cannot OR a GROUP BY key with an aggregate" even when no + // aggregate was present. An OR of key predicates the terms filter UNIONS + // (`status = 'a' OR status = 'b'` -> `include: [a, b]`) is supported; an OR mixing a + // TermsFilter key with a scripted-or-refused one is not, because only the first is a + // union. // - // An OR of key predicates the terms filter can spell stays supported -- that path - // UNIONS them, which is the disjunction (`HAVING status = 'a' OR status = 'b'` - // emits `include: [a, b]`). - // The SAME boundary as `havingLeaves`: a relation-scoped predicate belongs to the nested - // filter mechanism and is neither a key filter nor a group filter here. + // ⚠️ Relation-scoped leaves are counted HERE, unlike everywhere else: an OR spanning the + // nested filter and any other mechanism has exactly the same defect, and excluding them + // would leave `HAVING COUNT(*) > 1 OR ` executing as an AND. + def mechanismOf(e: Expression): String = + if (e.nested) "nested" + else + havingScopeOf(e) match { + case HavingScope.Metric => "metric" + case HavingScope.Unscoped => "unscoped" + // 🔴 Every GroupKey leaf reaching here is a `TermsFilter`: rule (b) above refuses + // the rest before this rule runs. Distinguishing them was DEAD -- deleting the + // distinction reddened nothing (round-3 mutation T5, and the same mutation was + // green in round 2). + case HavingScope.GroupKey => "key" + } def leavesUnder(c: Criteria): Seq[Expression] = c match { - case Predicate(l, _, r, _, _) => leavesUnder(l) ++ leavesUnder(r) - case _: ElasticRelation => Nil - case e: Expression => Seq(e).filterNot(_.nested) - case _ => Nil + case Predicate(l, _, r, _, _) => leavesUnder(l) ++ leavesUnder(r) + case relation: ElasticRelation => leavesUnder(relation.criteria) + case e: Expression => Seq(e) + case _ => Nil } - def scopesUnder(c: Criteria): Seq[HavingScope] = leavesUnder(c).map(havingScopeOf) - def firstBadOr(c: Criteria): Option[Criteria] = c match { + def firstMixedOr(c: Criteria): Option[(Criteria, Seq[String])] = c match { case p @ Predicate(l, op, r, _, _) => val here = if (op == OR) { - val leaves = leavesUnder(p) - val scopes = scopesUnder(p).distinct - // heterogeneous, or a key disjunct the terms filter cannot union - if (scopes.size > 1) Some(p: Criteria) - else if ( - scopes == Seq(HavingScope.GroupKey) && !leaves.forall(keyExpressibleByTerms) - ) Some(p: Criteria) - else None + val kinds = leavesUnder(p).map(mechanismOf).distinct + if (kinds.size > 1) Some((p: Criteria, kinds)) else None } else None - here.orElse(firstBadOr(l)).orElse(firstBadOr(r)) - case _: ElasticRelation => None - case _ => None + here.orElse(firstMixedOr(l)).orElse(firstMixedOr(r)) + case relation: ElasticRelation => firstMixedOr(relation.criteria) + case _ => None } - having.flatMap(_.criteria).filter(_ => groupBy.isDefined).flatMap(firstBadOr) match { - case Some(p) => + having.flatMap(_.criteria).filter(_ => groupBy.isDefined).flatMap(firstMixedOr) match { + case Some((p, kinds)) => Left( - s"HAVING cannot OR a GROUP BY key with an aggregate (${p.sql}): the key filter and " + - "the group filter are different mechanisms and their disjunction is not " + - "expressible. Split the query, or repeat the condition as an AND." + s"HAVING cannot OR conditions that Elasticsearch applies with different mechanisms " + + s"(${p.sql} mixes ${kinds.sorted.mkString(" and ")}): a group filter, a key filter " + + "and a nested filter are separate stages, so their disjunction would be executed " + + "as a conjunction. Split the query, or restate the condition as an AND." ) case None => Right(()) } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala index 0ea0b533b..7a5b25824 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala @@ -70,30 +70,34 @@ class InPredicateRenderSpec extends AnyFlatSpec with Matchers { s"SELECT x FROM t WHERE $name(a) IN (1, 2)", s"SELECT x FROM t WHERE $name(a) IN ('u', 'v')", s"SELECT x FROM t WHERE $name(a) NOT IN (1, 2)", - // 🔴 `GROUP BY a`, not `GROUP BY id`: since issue #389 a HAVING predicate must read either an - // aggregate or a GROUP BY KEY -- `HAVING f(a) ...` over a non-grouped `a` is not constant - // within a bucket and is now refused by name instead of being silently dropped. Grouping by the - // column the function is applied to keeps this sweep exercising the HAVING venue, which is the - // half that carried #365's loud failure (`COUNT(x)(COUNT(x))`). - s"SELECT a FROM t GROUP BY a HAVING $name(a) IN (1, 2)", - s"SELECT a FROM t GROUP BY a HAVING $name(a) NOT IN (1, 2)" + s"SELECT id FROM t GROUP BY id HAVING $name(a) IN (1, 2)", + s"SELECT id FROM t GROUP BY id HAVING $name(a) NOT IN (1, 2)" ) - /** A statement is IN SCOPE only if it parses; a function that needs other arity or types simply - * is not exercised by this shape. That makes the sweep partial, which is why the next test - * asserts what it must contain. + /** A statement is IN SCOPE only if the GRAMMAR accepts it; a function that needs other arity or + * types simply is not exercised by this shape. That makes the sweep partial, which is why the + * next test asserts what it must contain. + * + * 🔴 `parseUnvalidated`, not `apply` (issue #389, round 3). This file measures a RENDER property + * -- does a statement survive its own `.sql`? -- and that is a property of the AST, not of + * whether a validation rule admits the statement. Keying it on `apply` coupled it to + * `validate()`: when #389 taught HAVING which key predicates a bucket filter can express, ~120 + * rows silently dropped OUT of the sweep and the HAVING venue -- the half that carried #365's + * loud failure (`COUNT(x)(COUNT(x))`) -- stopped being exercised at all, while the file stayed + * green. `parseUnvalidated` is `Parser.single`'s own action, so the AST is built exactly as + * `apply` builds it; only the validation pass is skipped. */ private def accepted: List[(String, String)] = for { name <- spellings sql <- statements(name) - if Parser(sql).isRight + if Parser.parseUnvalidated(sql).isRight } yield (name, sql) "the render of a function on the left of IN" should "re-parse to the same statement" in { val broken = accepted.flatMap { case (_, sql) => - val rendered = Parser(sql).map(_.sql).getOrElse("") - Parser(rendered) match { + val rendered = Parser.parseUnvalidated(sql).map(_.sql).getOrElse("") + Parser.parseUnvalidated(rendered) match { case Left(e) => Some(s" $sql\n renders $rendered\n re-parse REJECTED: ${e.toString.take(70)}") case Right(again) if again.sql != rendered => diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index df1099bc5..16c0094b6 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -405,103 +405,118 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt Parser(group + "status = 'A'").isRight shouldBe true } - "a HAVING over a function of the GROUP BY KEY" should "be PUSHED into the query, not dropped" in { - // 🔴 This test used to PIN the silent drop. Lead ruling 2026-09-24: make it WORK. A function of - // the key is CONSTANT within a bucket, so filtering the documents by it keeps exactly the - // buckets that satisfy it -- EXECUTED on Elasticsearch 8.18.3 over `a` x2 / `b` / `cc`: - // `UPPER(status) = 'A'` returned [a, b, cc] before and returns [a] now, with `a`'s doc_count - // and every metric byte-identical to the unfiltered run. - // - // It contributes NOTHING to the bucket selector -- that is the point: it is a different - // mechanism, and the selector is left alone. - script(group + "UPPER(status) = 'A'") shouldBe "1 == 1" - val pushed = parsed(group + "UPPER(status) = 'A'").pushedHavingKeys - pushed.map(_.sql) shouldBe Seq("UPPER(status) = 'A'") - parsed(group + "UPPER(status) = 'A'").searchCriteria.map(_.sql) shouldBe - Some("UPPER(status) = 'A'") - } - - it should "AND with the WHERE clause rather than replace it" in { - val s = parsed( - "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status " + - "HAVING UPPER(status) = 'A'" - ) - s.searchCriteria.map(_.sql) shouldBe Some("amount > 1 AND UPPER(status) = 'A'") + // ------------------------------------------------------------------------------------------- + // The GROUP BY key population (round 3 -- the document push-down is GONE) + // ------------------------------------------------------------------------------------------- + + "a key predicate the terms filter expresses" should "be classified TermsFilter and left to it" in { + // The ONE classifier both emission paths read. `TermsFilter` means the existing `terms` + // include/exclude call applies it, so the selector sees nothing. + val st = parsed(group + "status = 'a'") + val leaf = st.havingLeaves.head + st.keyPredicateOutcome(leaf) shouldBe st.KeyPredicateOutcome.TermsFilter + script(group + "status = 'a'") shouldBe "1 == 1" + } + + "a key predicate neither bucket mechanism expresses" should "be REFUSED, never applied to documents" in { + // 🔴 ROUND 3. This used to be pushed into the QUERY as a document filter, and that is unsound + // twice over -- MEASURED on Elasticsearch 8.18.3: + // `GROUP BY DAY(d) HAVING YEAR(d) = 2025` kept bucket `3` and changed its count 3 -> 1; + // `GROUP BY tags HAVING UPPER(tags) = 'A'` kept bucket `b`, which FAILS the predicate, + // because a multi-valued document is kept or dropped whole. + // The second is strictly less sound than the `include` path it was meant to generalise. + Seq("UPPER(status) = 'A'", "LENGTH(status) = 1", "SUBSTRING(status, 1, 1) = 'a'").foreach { p => + withClue(s"[$p] ") { + rejection(group + p) should include("terms filter, which can only express") + } + } } - it should "leave the key predicates the terms filter already expresses alone" in { - // ⚠️ `HAVING status = 'A'` is a `terms` include TODAY. Pushing it as well would be correct and - // would move the emitted bytes of every shipped statement of that shape for nothing, so the - // eligibility test ASKS the real `Expression.includes` rather than re-deriving its rules. - parsed(group + "status = 'a'").pushedHavingKeys shouldBe empty - parsed(group + "status LIKE 'a%'").pushedHavingKeys shouldBe empty - parsed(group + "status = 'a'").searchCriteria shouldBe None + it should "be refused on a FUNCTION-of-column key too, where the push-down silently miscounted" in { + rejection( + "SELECT DAY(d) AS dd, COUNT(*) AS c FROM t GROUP BY DAY(d) HAVING YEAR(d) = 2025" + ) should include("HAVING cannot filter on") } - it should "still leave the selector to the aggregate half of a conjunction" in { - script(group + "COUNT(*) > 1 AND UPPER(status) = 'A'") shouldBe - "(params.c == null ? false : (params.c > 1))" - parsed(group + "COUNT(*) > 1 AND UPPER(status) = 'A'").pushedHavingKeys.map(_.sql) shouldBe - Seq("UPPER(status) = 'A'") + it should "name the nested case separately" in { + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name HAVING UPPER(e.name) = 'X'" + ) should include("nested object") } "a HAVING that reads neither an aggregate nor a GROUP BY key" should "be refused by name" in { - // It is not constant within a bucket, so no mechanism can honour it -- and pushing it into the - // query would filter DOCUMENTS and silently change every metric of every surviving group. rejection(group + "UPPER(name) = 'X'") should include("can only filter on a GROUP BY key") rejection(group + "name = 'x'") should include("can only filter on a GROUP BY key") } - "an OR that mixes a GROUP BY key with an aggregate" should "be refused, because it executes as an AND" in { - // 🔴 PRE-EXISTING silent wrong answer, closed by the ruling. MEASURED on `main` over the same - // fixture: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the disjunction is - // [a, b] -- the key became a terms `include` (removing `a`) and the metric a `bucket_selector` - // (removing `b`), so the OR answered the empty AND. - rejection(group + "COUNT(*) > 1 OR status = 'b'") should include("cannot OR a GROUP BY key") - rejection(group + "COUNT(*) > 1 OR UPPER(status) = 'B'") should include( - "cannot OR a GROUP BY key" + it should "be refused with NO GROUP BY as well" in { + // 🔴 Review MEDIUM-4: the rule was gated on `groupBy.isDefined`, so S8's original hole stayed + // open for a FUNCTION of a column. MEASURED: `SELECT COUNT(*) AS c FROM t HAVING + // UPPER(status) = 'A'` answered `{"c":{"value":4}}` with the predicate gone, while the bare + // `HAVING status = 'a'` was correctly refused. + rejection("SELECT COUNT(*) AS c FROM t HAVING UPPER(status) = 'A'") should include( + "can only filter on a GROUP BY key" ) } - it should "still accept an OR of key predicates the terms filter can union" in { - // The complement: `include: [a, b]` IS the disjunction, so this must not be swept up. + // ------------------------------------------------------------------------------------------- + // The OR rule -- homogeneity of MECHANISM (round 3) + // ------------------------------------------------------------------------------------------- + + "an OR whose branches need different mechanisms" should "be refused, because it executes as an AND" in { + // 🔴 MEASURED on `main`: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the + // disjunction is two of them -- the key became a terms `include` (removing one) and the metric + // a `bucket_selector` (removing the other), so the OR answered the empty AND. + val msg = rejection(group + "COUNT(*) > 1 OR status = 'b'") + msg should include("different mechanisms") + msg should include("key") + msg should include("metric") + } + + it should "name the mechanisms it actually found, not an aggregate that is not there" in { + // 🔴 Review MEDIUM-5: the old message said "cannot OR a GROUP BY key with an aggregate" even + // for a statement containing NO aggregate. The message now names what it found. + rejection(group + "COUNT(*) > 1 OR status = 'b'") should include("mixes key and metric") + // ... and a key predicate neither mechanism expresses is refused for its OWN reason first, + // which is the more actionable one. + rejection(group + "status = 'a' OR UPPER(status) = 'B'") should include( + "terms filter, which can only express" + ) + } + + it should "close the NESTED pair too, which the round-2 rule left executing as an AND" in { + // 🔴 The nested filter aggregation is a THIRD mechanism. The round-2 rule dropped `_.nested` + // leaves before counting, so this pair was invisible to it and still executed as an AND. + rejection( + "SELECT e.name, COUNT(*) AS c FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING COUNT(*) > 1 OR e.name = 'x'" + ) should include("mixes metric and nested") + } + + it should "accept an OR the terms filter UNIONS into one include list" in { + // 🔴 The complement, and the mutation-killer for the rule's second clause: `include: [a, b]` + // IS the disjunction. Parser(group + "status = 'a' OR status = 'b'").isRight shouldBe true - parsed(group + "status = 'a' OR status = 'b'").pushedHavingKeys shouldBe empty } - "a HAVING inside a NESTED relation" should "keep its own mechanism" in { - // 🔴 HAVING has FOUR mechanisms, not two. A relation-scoped predicate is turned into a `filter` - // aggregation by the bridge, and scoping the new rules past it refused the repo's own shipped - // "complex query" fixture -- caught by that fixture going red. - Parser( - "SELECT inner_products.category AS cat, MIN(inner_products.price) AS p FROM stores store " + - "JOIN UNNEST(store.products) AS inner_products GROUP BY inner_products.category " + - "HAVING inner_products.deleted = false" - ).isRight shouldBe true + it should "accept an OR of two metric conditions" in { + Parser(group + "COUNT(*) > 1 OR COUNT(*) > 5").isRight shouldBe true } - "a function of a NESTED grouping key" should "be refused, because the push-down is not equivalent there" in { - // 🔴 The equivalence that licenses the push-down -- "the key is constant within a bucket, so - // filtering documents keeps whole buckets" -- does NOT hold for a nested key: the bucket is an - // inner object and a document filter keeps the whole PARENT, so a parent holding one matching - // and one non-matching object would still produce BOTH buckets. - rejection( - "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name HAVING UPPER(e.name) = 'X'" - ) should include("nested grouping key") - } - - "the representability gate" should "be re-asked of the RESOLVED statement" in { - // 🔴 `validate()` runs schema-less inside `Parser.apply`, and `update(Some(schema))` can change - // what a HAVING renders. Re-asking at `validateResolved()` is what keeps a post-resolution - // divergence a 400 naming the clause instead of an `IllegalStateException` escaping into the - // bridge (issue #250's family). Asserted on the ONE surface core calls. - val refused = unvalidated(group + "NULLIF(COUNT(*), 0) > 1") - refused.validateResolved() match { - case Left(msg) => msg should include("HAVING cannot be applied to") - case Right(_) => fail("the resolved statement accepted a HAVING the parser refuses") - } - // ... and it does not fire on a statement the gate accepts. - unvalidated(group + "GREATEST(COUNT(*), 0) > 1").validateResolved() shouldBe Right(()) + "Having.script" should "answer None for a clause the gate refuses, never a partial filter" in { + // 🔴 NOT dead code: `softclient4es-extensions` reads it to build a materialized view's + // `TransformBucketSelectorConfig` (`graph/Stage.scala:291`). Round 1 recorded it as having no + // production caller and that was WRONG -- it is a FIFTH HAVING mechanism, outside this repo. + // + // The contract core can offer: never hand the extension a script for a clause the engine + // cannot express (a PARTIAL filter inside an MV), and never THROW (a 500 inside the extension). + // `unrepresentable` is public so the extension can refuse for itself. + val refused = unvalidated(group + "NULLIF(COUNT(*), 0) > 1").having.get + refused.unrepresentable should not be empty + refused.script shouldBe None + val ok = unvalidated(group + "COUNT(*) > 1").having.get + ok.unrepresentable shouldBe empty + ok.script shouldBe Some("(params.c == null ? false : (params.c > 1))") } "a metric computed outside the nested grouping" should "be refused rather than vanish" in { diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index e2f87e047..63aca1473 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -851,58 +851,53 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } - it should "filter on a FUNCTION of the GROUP BY key, not ignore it" in { - // 🔴 Lead ruling 2026-09-24. A function of the key is CONSTANT within a group, so the condition - // is pushed into the query and whole groups are kept or dropped. MEASURED on `main`: this - // returned all 37 groups with HTTP 200 -- the condition was honoured by no mechanism at all. - // `cat_i` holds exactly `i` documents and every key is `cat_NN`, so `LENGTH(category) = 6` is - // every group and `SUBSTRING(category, 5, 2) = '37'` is exactly one. - client.searchAs[CategoryCount]( + it should "REFUSE a key condition the terms filter cannot express, rather than answer wrongly" in { + // 🔴 ROUND 3. Round 2 pushed these into the QUERY as a document filter. That is unsound twice, + // both MEASURED on real Elasticsearch: with a key that is a FUNCTION of the column a SURVIVING + // bucket's count changed (`GROUP BY DAY(d) HAVING YEAR(d) = 2025` kept bucket 3 and moved its + // count 3 -> 1), and on a MULTI-VALUED field a bucket that FAILS the predicate survived, + // because a document in several buckets is kept or dropped whole. The `terms` `include` path + // answers such a grouping correctly, so the push-down was strictly LESS sound than the + // mechanism it was meant to generalise. + Seq( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING UPPER(category) = 'CAT_37'", "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + "HAVING SUBSTRING(category, 5, 2) = '37'" - ) match { - case ElasticSuccess(rows) => - rows.map(r => r.category -> r.cnt) shouldBe Seq("cat_37" -> categories.toLong) - case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + ).foreach { sql => + Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(result) => fail(s"[$sql] was accepted and answered $result") + case ElasticFailure(error) => + withClue(s"[$sql] ") { error.message should include("HAVING cannot filter on") } + } } } - it should "leave a surviving group's metrics EXACTLY as they were without the key filter" in { - // 🔴 THE property the push-down rests on. Filtering documents may not change what a surviving - // group reports, or the optimisation would be a silent wrong answer. Asserted against the - // UNFILTERED run of the same grouping, not against a literal. - def counts(res: ElasticResult[Seq[CategoryCount]]): Map[String, Long] = res match { - case ElasticSuccess(rows) => rows.map(r => r.category -> r.cnt).toMap + it should "still apply a key condition the terms filter DOES express" in { + // The complement: this is the mechanism the refusal above defers to, and it must keep working. + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING category = 'cat_37'" + ) match { + case ElasticSuccess(rows) => + rows.map(r => r.category -> r.cnt) shouldBe Seq("cat_37" -> categories.toLong) case ElasticFailure(error) => fail(s"Query failed: ${error.message}") } - val unfiltered = counts( - client.searchAs[CategoryCount]( - "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category" - ) - ) - val filtered = counts( - client.searchAs[CategoryCount]( - "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + - "HAVING UPPER(category) = 'CAT_37'" - ) - ) - filtered.keySet shouldBe Set("cat_37") - filtered("cat_37") shouldBe unfiltered("cat_37") } it should "combine a key condition with an aggregate condition as an AND" in { client.searchAs[CategoryCount]( "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + - "HAVING COUNT(*) > 35 AND SUBSTRING(category, 5, 1) = '3'" + "HAVING COUNT(*) > 35 AND category <> 'cat_36'" ) match { - case ElasticSuccess(rows) => rows.map(_.category).toSet shouldBe Set("cat_36", "cat_37") + case ElasticSuccess(rows) => rows.map(_.category).toSet shouldBe Set("cat_37") case ElasticFailure(error) => fail(s"Query failed: ${error.message}") } } - it should "REFUSE an OR between a key condition and an aggregate condition" in { + it should "REFUSE an OR whose branches need different mechanisms" in { // 🔴 PRE-EXISTING silent wrong answer: the key became a terms `include` and the aggregate a - // `bucket_selector`, so the OR was EXECUTED AS AN AND. MEASURED on `main` over a 4-document + // `bucket_selector`, so the OR was EXECUTED AS AN AND. Measured on `main` over a 4-document // fixture: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO groups where the disjunction is // two of them. Await.result( @@ -913,7 +908,18 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit 60.seconds ) match { case ElasticSuccess(result) => fail(s"the OR was accepted and answered $result") - case ElasticFailure(error) => error.message should include("cannot OR a GROUP BY key") + case ElasticFailure(error) => error.message should include("different mechanisms") + } + } + + it should "still accept an OR the terms filter unions into one include list" in { + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING category = 'cat_36' OR category = 'cat_37'" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category).toSet shouldBe Set("cat_36", "cat_37") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") } } From e6740149471a433bbfc375c9526af5eec50e6e8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Thu, 24 Sep 2026 18:20:43 +0200 Subject: [PATCH 4/6] fix(sql): a HAVING on the GROUP BY key unions, it never intersects (#389) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Elasticsearch applies a `terms` filter through ONE list of kept values and ONE list of removed values, and each is a UNION. So the kept list can express a DISJUNCTION and the removed list a CONJUNCTION -- and a combination needing the other one is not expressible at all. Three shapes were emitting a plausible-looking WRONG answer, measured on `455433ae` and on real Elasticsearch: HAVING k <> 'a' OR k <> 'b' emitted exclude:["a","b"] -- the SAME filter as the AND, though the disjunction is true for EVERY group, so two groups were dropped. HTTP 200. HAVING k = 'a' AND k = 'b' emitted include:["a","b"] -- two groups returned where the SQL means none. HAVING k = 'a' OR k <> 'b' emitted include:["a"] AND exclude:["b"] -- a conjunction where the SQL says disjunction. All three are now refused by name. What already worked is untouched, byte for byte: `= a OR = b`, `<> a AND <> b` (including `NOT IN`, and chains of three), and one kept list beside one removed list. 🔴 `Criteria.excludes` IS `Criteria.includes(bucket, !not, …)` -- one method read in two senses -- so `SingleSearch.keyChannelConflict` is ONE derivation that asks BOTH methods about BOTH sides of every predicate, per bucket, with the polarity the emission itself threads. A rule about these two channels that consults only one of them is wrong by construction. The polarity is now named, `Predicate.includePolarityOfRight`, and the emission and the rule call the same member -- the repo's own `PainlessNullSurvivalSpec` source gate reddened on the first draft, which is exactly what it exists for. 🔴 No De Morgan arm, and that is MEASURED rather than assumed: `Predicate.maybeNot` negates the RIGHT OPERAND only and `NOT ( … )` around a group is rejected by the grammar (here and on `455433ae`), so no `Predicate` is ever reached with a flipped polarity. An arm dualising the operator would be unreachable for every possible input -- the trap this issue already sprang once. Falsification, because the reason the defect class was invisible is the POPULATION and not the assertions: no test in this branch or in the repository combined two EXCLUDE-side key predicates under AND or OR, though `excludes` is literally the same method. The fix ships a DERIVED matrix -- `=` / `IN` / `LIKE` against `<>` / `NOT IN` / `NOT LIKE`, times AND / OR, same-channel and mixed, each verdict computed from the principle rather than listed -- at three levels: `sql` validation, both bridges' emission, and the testkit EXECUTED on a real index against bucket sets computed by hand from the fixture. Measured: 5/5 mutations RED, including "the rule reads only the include sense"; sql 1628 / core 1141 / bridge 235 / es6bridge 231; cross compile 2.12.20 + 2.13.16; the exact CI lint line; an emission diff over 20 shapes chosen to EXERCISE the two channels against the `455433ae` control -- 6 moved, every one from a silent wrong answer to a refusal, 14 byte-identical; `GroupByCompletenessSpec` 44/44 on real ES 8.18.3 and 44/44 on ES 6.8.23. ⚠️ The 18,062-input corpus replay is deliberately NOT cited as the guard for this surface: the corpus holds 4 HAVING rows and none with `<>`, `NOT IN`, `LIKE` or `OR`. Closed Issue #389 Co-Authored-By: Claude Opus 5 (1M context) --- .../sql/HavingFunctionEmissionSpec.scala | 51 ++++++++ documentation/sql/dql_statements.md | 17 ++- .../sql/HavingFunctionEmissionSpec.scala | 57 +++++++++ .../softnetwork/elastic/sql/query/Where.scala | 17 ++- .../elastic/sql/query/package.scala | 83 ++++++++++++ .../HavingOverAggregateFunctionSpec.scala | 120 ++++++++++++++++++ .../client/GroupByCompletenessSpec.scala | 86 +++++++++++++ 7 files changed, 427 insertions(+), 4 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index b498208c3..b34fa150c 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -231,6 +231,57 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { } } + // --------------------------------------------------------------------------------------------- + // The include/exclude CHANNEL -- it unions, it never intersects (rule b2) + // + // 🔴 `excludes` IS `includes(bucket, !not, …)`. Every assertion above this point is include-side, + // which is how an inverted rule once shipped: it reasoned about the include sense and was applied + // by a method that is also the exclude sense. These cells are the EXCLUDE side, derived. + // --------------------------------------------------------------------------------------------- + + "an AND of inequalities" should "union into ONE exclude list, exactly as on 455433ae" in { + queryOf(group + "status <> 'a' AND status <> 'b'") should include( + """"exclude":["a","b"]""" + ) + queryOf(group + "status NOT IN ('a','b') AND status <> 'c'") should include( + """"exclude":["a","b","c"]""" + ) + } + + it should "keep working beside a metric, which is a different mechanism" in { + val q = queryOf(group + "status <> 'a' AND COUNT(*) > 1") + q should include(""""exclude":["a"]""") + q should include("bucket_selector") + } + + "a combination the channel cannot express" should "never reach emission" in { + // Each of these emitted a SILENT WRONG ANSWER on `455433ae` -- measured: + // `<> a OR <> b` -> exclude:["a","b"], though the disjunction is true for EVERY bucket + // `= a AND = b` -> include:["a","b"], though the SQL means NO bucket + // `= a OR <> b` -> include:["a"] AND exclude:["b"], a conjunction where SQL says OR + // `LIKE a% AND LIKE b%` -> include:"a.*" only, the second pattern dropped + Seq( + "status <> 'a' OR status <> 'b'", + "status NOT IN ('a','b') OR status <> 'c'", + "status = 'a' AND status = 'b'", + "status = 'a' OR status <> 'b'", + "status LIKE 'a%' AND status LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + } + + it should "still allow one include and one exclude under a conjunction" in { + val q = queryOf(group + "status IN ('a','b') AND status <> 'c'") + q should include(""""include":["a","b"]""") + q should include(""""exclude":["c"]""") + } + // --------------------------------------------------------------------------------------------- // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. // --------------------------------------------------------------------------------------------- diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 351255c9e..19b5ec940 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -829,8 +829,21 @@ SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city LIKE 'P%'; SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city <> 'Lyon'; ``` -- Supported: a direct comparison of the key — `=`, `<>`, `IN`, `LIKE` / `RLIKE` — and an `OR` of - them, which the filter unions into one list (`HAVING city = 'Paris' OR city = 'Lyon'`). +- Supported: a direct comparison of the key — `=`, `<>`, `IN`, `LIKE` / `RLIKE`. +- ⚠️ **Which COMBINATIONS are supported follows from how Elasticsearch applies them.** The `terms` + filter carries one list of kept values and one list of removed values, and each is a UNION: + + | combination | supported | why | + |---|---|---| + | `city = 'Paris' OR city = 'Lyon'` | ✅ | the kept list is a union, i.e. a disjunction | + | `city <> 'Paris' AND city <> 'Lyon'` | ✅ | not-in-A and not-in-B is not-in-(A ∪ B) | + | `city = 'Paris' AND city <> 'Lyon'` | ✅ | one kept list and one removed list, applied together | + | `city <> 'Paris' OR city <> 'Lyon'` | ❌ refused | a union of removals is a conjunction, so this would be executed as one | + | `city = 'Paris' AND city = 'Lyon'` | ❌ refused | a union of kept values is a disjunction, so this would be executed as one | + | `city = 'Paris' OR city <> 'Lyon'` | ❌ refused | the two lists are applied together, i.e. ANDed | + + The refused rows previously returned a plausible-looking but WRONG set of groups. Split the query, + or restate the condition as an `OR` of equalities or an `AND` of inequalities. - ⚠️ **A FUNCTION of the key is refused** (`HAVING UPPER(city) = 'PARIS'`, `HAVING LENGTH(status) = 1`). The terms filter can only express a direct comparison, and the alternatives are unsound: filtering documents instead would keep or drop a multi-valued document diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 47f76d585..d960b5948 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -237,6 +237,63 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { } } + // --------------------------------------------------------------------------------------------- + // The include/exclude CHANNEL -- it unions, it never intersects (rule b2) + // + // 🔴 `excludes` IS `includes(bucket, !not, …)`. Every assertion above this point is include-side, + // which is how an inverted rule once shipped: it reasoned about the include sense and was applied + // by a method that is also the exclude sense. These cells are the EXCLUDE side, derived. + // --------------------------------------------------------------------------------------------- + + "an AND of inequalities" should "union into ONE exclude list, exactly as on 455433ae" in { + queryOf(group + "status <> 'a' AND status <> 'b'") should include( + """"exclude":["a","b"]""" + ) + queryOf(group + "status NOT IN ('a','b') AND status <> 'c'") should include( + """"exclude":["a","b","c"]""" + ) + } + + it should "keep working beside a metric, which is a different mechanism" in { + val q = queryOf(group + "status <> 'a' AND COUNT(*) > 1") + // ⚠️ ES6 RENDERING, and it is a RESIDUAL, not a choice: this bridge renders a SINGLE-element + // value list as a bare string, which Elasticsearch 6.8 reads as a REGEX. It happens to select + // the right terms here (anchored, no metacharacters) but it is why a set-based include beside + // a single-valued exclude is rejected outright by 6.8 -- recorded, pre-existing, not fixed. + q should include(""""exclude":"a"""") + q should include("bucket_selector") + } + + "a combination the channel cannot express" should "never reach emission" in { + // Each of these emitted a SILENT WRONG ANSWER on `455433ae` -- measured: + // `<> a OR <> b` -> exclude:["a","b"], though the disjunction is true for EVERY bucket + // `= a AND = b` -> include:["a","b"], though the SQL means NO bucket + // `= a OR <> b` -> include:["a"] AND exclude:["b"], a conjunction where SQL says OR + // `LIKE a% AND LIKE b%` -> include:"a.*" only, the second pattern dropped + Seq( + "status <> 'a' OR status <> 'b'", + "status NOT IN ('a','b') OR status <> 'c'", + "status = 'a' AND status = 'b'", + "status = 'a' OR status <> 'b'", + "status LIKE 'a%' AND status LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + } + + it should "still allow one include and one exclude under a conjunction" in { + // Both lists MULTI-valued, so this bridge renders both as arrays -- see the residual note + // above; ES 6.8 rejects a set-based include beside a regex-rendered exclude. + val q = queryOf(group + "status IN ('a','b') AND status NOT IN ('c','d')") + q should include(""""include":["a","b"]""") + q should include(""""exclude":["c","d"]""") + } + // --------------------------------------------------------------------------------------------- // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. // --------------------------------------------------------------------------------------------- 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 3ad1f8e45..29518f7a0 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 @@ -244,10 +244,10 @@ sealed trait Criteria extends Updateable with PainlessScript { bucketIncludesExcludes: BucketIncludesExcludes ): BucketIncludesExcludes = this match { - case Predicate(left, _, right, n, _) => + case p @ Predicate(left, _, right, _, _) => right.includes( bucket, - (!not && n.isDefined) || (not && n.isEmpty), + p.includePolarityOfRight(not), left.includes(bucket, not, bucketIncludesExcludes) ) case relation: ElasticRelation => @@ -367,6 +367,19 @@ case class Predicate( else leftCriteria} $operator${not .map(_ => " NOT") .getOrElse("")} ${if (group) s"$rightCriteria)" else rightCriteria}" + + /** The polarity the RIGHT criterion inherits when a `terms` include/exclude traversal walks this + * predicate. The predicate's own `NOT` binds the RIGHT operand, and this IS its fold: the + * criterion is evaluated over the same bucket with the sense flipped, so `notConsumed` holds by + * construction and nothing downstream has to negate it again. + * + * 🔴 Named because it has TWO readers -- `Criteria.includes` and + * `SingleSearch.keyChannelConflict` -- and the round-5 regression this replaces was exactly one + * derivation written for one of two call sites that are the same function. + */ + private[query] def includePolarityOfRight(not: Boolean): Boolean = + (!not && this.not.isDefined) || (not && this.not.isEmpty) + override def update(request: SingleSearch): Criteria = { val updatedPredicate = this.copy( leftCriteria = leftCriteria.update(request), 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 4dde0452d..42bc8508f 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 @@ -822,6 +822,65 @@ package object query { "Compare the key itself, or filter in WHERE." ) + /** The first `HAVING` combination whose halves cannot BOTH reach the `terms` filter, or `None` + * when every combination is a union the channel expresses. See rule (b2) in `validate()`. + * + * The analysis is per bucket and follows `Criteria.includes` exactly -- same polarity + * threading, same contribution test, both senses: + * - the INCLUDE list is a union, so it expresses a DISJUNCTION. Two halves that both + * contribute includes under a conjunction are not expressible. + * - the EXCLUDE list is a union of negations, so it expresses a CONJUNCTION. Anything + * excluded under a disjunction is not expressible -- including the mixed shape `= 'a' OR + * <> 'b'`, where the emission would AND an include with an exclude. + * + * The polarity is still threaded, because a `NOT` before a LEAF decides which channel that + * leaf feeds -- `= 'a' AND NOT = 'b'` is an include and an exclude, and expressible. + */ + private[query] def keyChannelConflict: Option[String] = { + val empty = BucketIncludesExcludes() + def conflict(c: Criteria, bucket: Bucket, not: Boolean): Option[Criteria] = c match { + case p @ Predicate(left, op, right, _, _) => + // ONE derivation of the polarity, shared with `Criteria.includes` -- see + // `Predicate.includePolarityOfRight`. + val rightNot = p.includePolarityOfRight(not) + val leftIncludes = left.includes(bucket, not, empty) + val leftExcludes = left.excludes(bucket, not, empty) + val rightIncludes = right.includes(bucket, rightNot, empty) + val rightExcludes = right.excludes(bucket, rightNot, empty) + // 🔴 No De Morgan arm here, and that is MEASURED, not assumed: `Predicate.maybeNot` + // negates the RIGHT OPERAND only, and `NOT ( … )` around a group is rejected by the + // grammar (`end of input expected`, on this branch and on `455433ae`). So no `Predicate` + // is ever reached with a flipped polarity -- probed over the parseable shapes -- and an + // arm dualising `op` would be unreachable for every possible input. Round 3 shipped + // exactly such an arm and had to delete it; one is enough. + val leftContributes = leftIncludes != empty || leftExcludes != empty + val rightContributes = rightIncludes != empty || rightExcludes != empty + val here = + if (op == AND && leftIncludes != empty && rightIncludes != empty) Some(p: Criteria) + else if ( + op == OR && leftContributes && rightContributes && + (leftExcludes != empty || rightExcludes != empty) + ) Some(p: Criteria) + else None + here + .orElse(conflict(left, bucket, not)) + .orElse(conflict(right, bucket, rightNot)) + case relation: ElasticRelation => conflict(relation.criteria, bucket, not) + case _ => None + } + having.flatMap(_.criteria).flatMap { criteria => + buckets.view + .flatMap(bucket => conflict(criteria, bucket, not = false)) + .headOption + .map(c => + s"HAVING cannot combine the conditions in ${c.sql} on one GROUP BY key: Elasticsearch " + + "applies one list of kept values and one list of removed values, and each is a union, " + + "so this combination would be executed as a different one. Split the query, or " + + "restate it as an OR of equalities or an AND of inequalities." + ) + } + } + /** Does this key predicate read a NESTED bucket? Asked of the bucket the leaf RESOLVES to. */ private[query] def readsNestedKey(e: Expression): Boolean = e.nested || namedLeavesOf(e).exists(l => keyBucketOf(l).exists(_.nestedElement.isDefined)) @@ -1150,6 +1209,30 @@ package object query { .collectFirst { case KeyPredicateOutcome.Refused(reason) => Left(reason) } .getOrElse(Right(())) } + _ <- { + // (b2) 🔴 The `terms` filter carries ONE include list and ONE exclude list, and each is a + // UNION of its members. So the channel can express a union and NEVER an intersection -- + // and which of the two a combination needs depends on the OPERATOR *and* on the + // POLARITY, because `excludes` IS `includes(bucket, !not, …)`: the same method, read in + // the other sense. + // + // MEASURED on `455433ae` and on real ES 8.18.3: + // `HAVING city <> 'Paris' AND city <> 'Lyon'` -> `exclude:["Lyon","Paris"]`, CORRECT: + // NOT-in-A and NOT-in-B is NOT-in-(A∪B). + // `HAVING city <> 'Paris' OR city <> 'Lyon'` -> the SAME `exclude:["Lyon","Paris"]`, + // a SILENT WRONG ANSWER: the disjunction is true for every bucket, and two are + // dropped. HTTP 200. + // `HAVING city = 'a' AND city = 'b'` -> `include:["a","b"]`, also a silent wrong + // answer: the include list means `a OR b`, the SQL means no bucket at all. + // `HAVING city = 'a' OR city = 'b'` -> `include:["a","b"]`, CORRECT. + // + // 🔴 This rule is ONE derivation that asks BOTH methods about BOTH sides. The defect it + // replaces (a round-5 regression, reverted with the rest of that work) was a rule + // written for the include sense only and applied by a method that is also the exclude + // sense -- so it inverted every `<>`. A rule about these two channels that consults only + // one of them is wrong by construction. + keyChannelConflict.map(Left(_)).getOrElse(Right(())) + } _ <- { // (c) 🔴 An OR is honoured by ONE mechanism or by none: a `bucket_selector` script cannot // read the bucket KEY, the `terms` filter cannot read a metric, and the nested filter diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index 16c0094b6..b0f9ffcdc 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -459,6 +459,126 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt ) } + // ------------------------------------------------------------------------------------------- + // The include/exclude CHANNEL -- it unions, it never intersects (rule b2) + // + // 🔴 `Criteria.excludes` IS `Criteria.includes(bucket, !not, …)`: one method read in two senses. + // A rule about these channels written for one sense is wrong for the other by construction -- + // that is exactly how an abandoned round shipped an inverted rule that turned `<> a OR <> b` + // from a dropped predicate into a WRONG answer. So the matrix below is DERIVED and covers both + // senses; the population, not the assertion, is what was missing. + // ------------------------------------------------------------------------------------------- + + /** Both halves of the matrix, as the predicate text and which channel the leaf feeds. */ + private val includeSide = Seq( + "status = 'a'" -> "status = 'b'", + "status IN ('a','x')" -> "status = 'b'", + "status LIKE 'a%'" -> "status LIKE 'b%'" + ) + private val excludeSide = Seq( + "status <> 'a'" -> "status <> 'b'", + "status NOT IN ('a','x')" -> "status <> 'b'", + "status NOT LIKE 'a%'" -> "status NOT LIKE 'b%'" + ) + + "the terms channel" should "express every DERIVED combination of key predicates, or refuse it" in { + // The verdict is DERIVED from the principle, never listed by hand: the include list is a union + // and therefore a DISJUNCTION; the exclude list is a union of negations and therefore a + // CONJUNCTION. So two leaves are expressible only as an OR of includes or an AND of excludes. + val cells = + for { + (leftSide, leftName) <- Seq(includeSide -> "include", excludeSide -> "exclude") + (left, right) <- leftSide + (rightSide, rightName) <- Seq(includeSide -> "include", excludeSide -> "exclude") + combiner <- Seq("AND", "OR") + } yield { + // pair each left with the OTHER side's matching right, so mixed cells exist too + val other = rightSide(leftSide.indexOf((left, right)))._2 + val rhs = if (rightName == leftName) right else other + val sql = s"$left $combiner $rhs" + val sameChannel = leftName == rightName + val expressible = + // same channel: the include list is a disjunction, the exclude list a conjunction ... + (sameChannel && leftName == "include" && combiner == "OR") || + (sameChannel && leftName == "exclude" && combiner == "AND") || + // ... and the two channels are themselves ANDed by Elasticsearch, so one of each under + // a conjunction is exactly what they mean together. + (!sameChannel && combiner == "AND") + (sql, expressible) + } + cells.distinct.size should be >= 20 + cells.distinct.foreach { case (predicate, expressible) => + withClue(s"[$predicate] expressible=$expressible ") { + Parser(group + predicate).isRight shouldBe expressible + } + } + } + + it should "leave a SINGLE key predicate of either sense alone" in { + (includeSide ++ excludeSide).flatMap { case (l, r) => Seq(l, r) }.distinct.foreach { p => + withClue(s"[$p] ")(Parser(group + p).isRight shouldBe true) + } + } + + it should "keep the AND of inequalities that works on `455433ae`, byte for byte" in { + // 🔴 The row this rule must NOT move: `exclude:["a","b"]` is CORRECT, because NOT-in-A and + // NOT-in-B is NOT-in-(A ∪ B). An earlier, inverted rule refused it -- a regression against + // `main` that the docs then contradicted. + val st = parsed(group + "status <> 'a' AND status <> 'b'") + st.buckets + .map(b => havingOf(st, "and").excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("a", "b")) + // ... and the three-leaf chain, which is the same union one level deeper. + val three = parsed(group + "status <> 'a' AND status <> 'b' AND status <> 'c'") + three.buckets + .map(b => havingOf(three, "and3").excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("a", "b", "c")) + } + + it should "refuse the OR of inequalities that is WRONG on `455433ae`" in { + // 🔴 MEASURED on `455433ae` and on ES 8.18.3: `<> 'a' OR <> 'b'` emitted the SAME + // `exclude:["a","b"]` as the AND. The disjunction is true for EVERY bucket, and two were + // dropped, HTTP 200. Live on `main` today. + rejection(group + "status <> 'a' OR status <> 'b'") should include("cannot combine") + rejection(group + "status NOT IN ('a','b') OR status <> 'c'") should include("cannot combine") + } + + it should "refuse the AND of equalities, whose include list means OR" in { + // `include:["a","b"]` keeps two buckets; the SQL means none. DECISION: refused, not + // "corrected" to an empty result -- a loud refusal is the contract this issue is about. + rejection(group + "status = 'a' AND status = 'b'") should include("cannot combine") + } + + it should "refuse a MIXED pair under OR, which the emission would AND" in { + // `= 'a' OR <> 'b'` emitted `include:["a"]` AND `exclude:["b"]` on `455433ae` -- a conjunction + // where the SQL says disjunction. + rejection(group + "status = 'a' OR status <> 'b'") should include("cannot combine") + } + + it should "not refuse a conjunction the channels express SEPARATELY" in { + // One include and one exclude under AND is exactly what the two lists mean together. + Parser(group + "status = 'a' AND status <> 'b'").isRight shouldBe true + Parser(group + "status IN ('a','b') AND status <> 'c'").isRight shouldBe true + // ... and a key predicate beside a METRIC is a different mechanism, not a second channel. + Parser(group + "status <> 'a' AND COUNT(*) > 1").isRight shouldBe true + } + + it should "still place a NOT before a LEAF in the right channel" in { + // The polarity threading that IS reachable: `NOT = 'b'` feeds the EXCLUDE list, so the pair is + // one include and one exclude under a conjunction -- expressible. + val st = parsed(group + "status = 'a' AND NOT status = 'b'") + st.buckets + .map(b => havingOf(st, "notleaf").includes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("a")) + st.buckets + .map(b => havingOf(st, "notleaf").excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("b")) + // 🔴 And the shape that would need De Morgan does not exist: `NOT ( … )` around a group is + // rejected by the grammar, here and on `455433ae`. MEASURED -- which is why the rule carries + // no arm for it. + Parser(group + "NOT (status = 'x' OR status = 'y')").isLeft shouldBe true + } + // ------------------------------------------------------------------------------------------- // The OR rule -- homogeneity of MECHANISM (round 3) // ------------------------------------------------------------------------------------------- diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index 63aca1473..19b59ddc9 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -923,6 +923,92 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + // ----------------------------------------------------------------------------------------- + // The include/exclude CHANNEL, EXECUTED (rule b2) + // + // 🔴 `excludes` IS `includes(bucket, !not, …)` -- one method, two senses -- and every other row + // in this file exercises the INCLUDE sense. These are the exclude-side cells, each against a + // bucket set computed by hand from the fixture: `cat_i` holds exactly `i` documents, 37 groups. + // ----------------------------------------------------------------------------------------- + + private def categoriesOf(having: String): Seq[String] = + client.searchAsUnchecked[CategoryCount]( + SelectStatement( + s"SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category $having" + ) + ) match { + case ElasticSuccess(rows) => rows.map(_.category).sorted + case ElasticFailure(error) => fail(s"[$having] failed: ${error.message}") + } + + private def categoryRefusal(having: String): String = + Await.result( + client.run( + s"SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category $having" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"[$having] was accepted and answered $result") + case ElasticFailure(error) => error.message + } + + private lazy val allCategories: Seq[String] = (1 to categories).map(c => f"cat_$c%02d").sorted + + "an AND of inequalities on the GROUP BY key" should "remove exactly those groups" in { + // NOT-in-A and NOT-in-B is NOT-in-(A ∪ B): the exclude list IS that union, so this is correct + // on `455433ae` and must stay byte-identical. + categoriesOf("HAVING category <> 'cat_36' AND category <> 'cat_37'") shouldBe + allCategories.filterNot(Set("cat_36", "cat_37")) + categoriesOf( + "HAVING category NOT IN ('cat_36','cat_37') AND category <> 'cat_35'" + ) shouldBe allCategories.filterNot(Set("cat_35", "cat_36", "cat_37")) + // three leaves, the same union one level deeper + categoriesOf( + "HAVING category <> 'cat_35' AND category <> 'cat_36' AND category <> 'cat_37'" + ) shouldBe allCategories.filterNot(Set("cat_35", "cat_36", "cat_37")) + } + + it should "still combine with an aggregate condition, which is a different mechanism" in { + categoriesOf("HAVING category <> 'cat_37' AND COUNT(*) > 35") shouldBe Seq("cat_36") + } + + "an OR of inequalities" should "be REFUSED, because the exclude list would execute it as an AND" in { + // 🔴 MEASURED on `455433ae` and on this cluster: it emitted the SAME `exclude:["cat_36", + // "cat_37"]` as the AND above. The disjunction is true for EVERY one of the 37 groups, and two + // were dropped -- HTTP 200, no error. Live on `main` today. + categoryRefusal( + "HAVING category <> 'cat_36' OR category <> 'cat_37'" + ) should include("cannot combine") + categoryRefusal( + "HAVING category NOT IN ('cat_36','cat_37') OR category <> 'cat_35'" + ) should include("cannot combine") + } + + "an AND of equalities" should "be REFUSED, because the include list means OR" in { + // `include:["cat_36","cat_37"]` keeps two groups; the SQL means none. + categoryRefusal( + "HAVING category = 'cat_36' AND category = 'cat_37'" + ) should include("cannot combine") + } + + "a MIXED pair under OR" should "be REFUSED, because the two lists are ANDed" in { + categoryRefusal( + "HAVING category = 'cat_37' OR category <> 'cat_36'" + ) should include("cannot combine") + } + + it should "still honour one include and one exclude under AND" in { + // ⚠️ Both lists carry SEVERAL values on purpose. RESIDUAL, pre-existing and ES-6-ONLY, found + // by this matrix: the es6 bridge renders a SINGLE-element list as a bare string + // (`"exclude":"c"`), which ES 6.8 reads as a REGEX, and mixing a set-based include with a + // regex-based exclude is rejected with HTTP 400 `Cannot mix a set-based include with a + // regex-based method`. The emission is byte-identical to `455433ae`; it is recorded, not fixed + // here. + categoriesOf( + "HAVING category IN ('cat_35','cat_36','cat_37') AND category NOT IN ('cat_35','cat_36')" + ) shouldBe Seq("cat_37") + } + it should "REFUSE a HAVING on a column that is neither grouped nor aggregated" in { Await.result( client.run( From 893e2ce203757d13644514d6432e03aa9f19287d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Thu, 24 Sep 2026 19:09:53 +0200 Subject: [PATCH 5/6] fix(sql): a pattern replaces the list, and two grouping levels are nested (#389) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The final review of `74b99e0f..e6740149` found the commit's CODE sound and its own DOCUMENTATION false in three places. Both defects it measured are fixed here, and each false claim is corrected where it was written. HIGH-2 -- a pattern silently replaces a value list on the same key. The emission matches the regex arm FIRST and discards the values, and a second pattern is lost to `orElse`. MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: HAVING status = 'a' OR status LIKE 'b%' -> {"terms":{"include":"b.*"}} -> ['b1'] and the SQL means ['a','b1'] 🔴 That shape is an OR of two KEPT-value contributions -- exactly the row this branch's own documentation table blessed as supported. **Being a union is necessary, not sufficient**: each list holds either a set of values or ONE pattern. A new arm refuses any same-channel collision, with its own remedy: a single RLIKE, or splitting the query. HIGH-1 -- an OR across TWO GROUP BY keys executed as an AND. `keyChannelConflict` is per bucket, so each level only ever sees one contributing side, and `mechanismOf` collapsed every grouping level to one label, so the OR rule saw a single mechanism and passed. Elasticsearch NESTS the two `terms` aggregations, which IS the conjunction that rule exists to catch. MEASURED over (a,a) (a,b) (x,b) (x,y): GROUP BY status, city HAVING status = 'a' OR city = 'b' -> ONE group (a,b), and the SQL means THREE A leaf's STAGE is now its mechanism plus, for a key predicate, the bucket it addresses, and the two-level case has its own message -- "a group filter, a key filter and a nested filter are separate stages" is not what two grouping levels are. 🔴 The suite PINNED HIGH-2 as supported, and the axis is why: the derived matrix paired each operator with a FIXED partner and varied only channel and combiner, so `=` was never crossed with `LIKE`. The operators are CROSSED now -- 72 cells, each verdict derived from BOTH properties of the channel. Every defect found on this branch was a hole in the population, never in the assertions. Also: `PainlessNullSurvivalSpec`'s probe no longer saw the site it caught two rounds ago (`p.includePolarityOfRight(not)` does not match `.not`), so that file passed vacuously; the named fold now counts as a use and as a classification, and a raw `p.not` reddens it again. Measured, re-derived rather than applied on trust: a generated cross-product of 366 statements -- every operator against every operator, AND and OR, one key and two, two leaves and three -- each cell carrying its own oracle evaluated independently and executed on ES 8.18.3 through the real emission: WRONG 64 -> 0. Cross-tabulated against `455433ae`: 229 were wrong on main and are now refused, 91 are correct and byte-identical, and 46 were correct on main by COINCIDENCE (23 repeat a leaf, 21 are contradictions, 2 genuinely distinct) and are now refused -- accepted knowingly, because their emission cannot be told apart from the wrong one. 9/9 mutations RED; sql 1632 / core 1141 / bridge 237 / es6bridge 233; cross compile 2.12.20 + 2.13.16; the exact CI lint line; `GroupByCompletenessSpec` 46/46 on real ES 8.18.3 and 46/46 on ES 6.8.23. ⚠️ Corrects the previous commit's release note "everything that worked keeps working, byte-identical": false, as the 46 above show. The 20-shape sample it rested on was not representative of the moved population. Closes #389 Co-Authored-By: Claude Opus 5 (1M context) --- .../sql/HavingFunctionEmissionSpec.scala | 37 +++++ .../sql/PainlessNullSurvivalSpec.scala | 10 +- documentation/sql/dql_statements.md | 25 +++- .../sql/HavingFunctionEmissionSpec.scala | 39 +++++ .../sql/PainlessNullSurvivalSpec.scala | 10 +- .../elastic/sql/query/package.scala | 139 ++++++++++++------ .../HavingOverAggregateFunctionSpec.scala | 116 +++++++++++---- .../client/GroupByCompletenessSpec.scala | 39 +++++ 8 files changed, 331 insertions(+), 84 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index b34fa150c..b28783d98 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -282,6 +282,43 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { q should include(""""exclude":["c"]""") } + it should "refuse a pattern meeting a value list in the SAME channel" in { + // 🔴 MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: `= 'a' OR LIKE 'b%'` emitted + // `include:"b.*"` and returned ['b1'] where the SQL means ['a','b1']. The emission keeps the + // PATTERN and discards the value list, and a second pattern is lost to `orElse`. + Seq( + "status = 'a' OR status LIKE 'b%'", + "status LIKE 'a%' OR status LIKE 'b%'", + "status <> 'a' AND status NOT LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should include("a pattern replaces the list") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + // ... while a pattern in EACH channel is fine: they are different lists. + val q = queryOf(group + "status LIKE 'a%' AND status NOT LIKE 'b%'") + q should include(""""include":"a.*"""") + q should include(""""exclude":"b.*"""") + } + + "an OR across TWO grouping levels" should "never reach emission" in { + // 🔴 MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): Elasticsearch NESTS the two `terms` + // aggregations, so `status = 'a' OR city = 'b'` emitted + // `terms status include:["a"] > terms city include:["b"]` and returned ONE group where the + // SQL means THREE. The AND is exactly what the nesting means, and still emits. + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + Parser(two + "status = 'a' OR city = 'b'") match { + case Left(e) => e.msg should include("DIFFERENT GROUP BY keys") + case Right(_) => fail(s"accepted; it emitted ${queryOf(two + "status = 'a' OR city = 'b'")}") + } + val q = queryOf(two + "status = 'a' AND city = 'b'") + q should include(""""include":["a"]""") + q should include(""""include":["b"]""") + } + // --------------------------------------------------------------------------------------------- // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. // --------------------------------------------------------------------------------------------- diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index 1232ec711..535c63c81 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -177,7 +177,14 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") val names = boundToPredicate.flatMap(_.findAllMatchIn(code).map(_.group(1))).toSet val namedUses = names.toSeq.map { n => - s"""(?\.not`, so + // `p.includePolarityOfRight(not)` -- the member that exists PRECISELY to consume this + // fold -- was invisible, and its file scored 0 uses / 0 classifications and passed + // VACUOUSLY. A gate that stops seeing the site it has just caught is worse than no gate. + // The named fold counts as a use AND, like `negated`, as a classification: calling it IS + // the fold. Replace that call with a raw `p.not` and the file reddens, which is the point. + (s"""(? classifications) Some( diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 19b5ec940..f8c72c607 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -838,12 +838,21 @@ SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city <> 'Lyon'; | `city = 'Paris' OR city = 'Lyon'` | ✅ | the kept list is a union, i.e. a disjunction | | `city <> 'Paris' AND city <> 'Lyon'` | ✅ | not-in-A and not-in-B is not-in-(A ∪ B) | | `city = 'Paris' AND city <> 'Lyon'` | ✅ | one kept list and one removed list, applied together | + | `city LIKE 'P%' AND city NOT LIKE 'L%'` | ✅ | one pattern in each of the two lists | | `city <> 'Paris' OR city <> 'Lyon'` | ❌ refused | a union of removals is a conjunction, so this would be executed as one | | `city = 'Paris' AND city = 'Lyon'` | ❌ refused | a union of kept values is a disjunction, so this would be executed as one | | `city = 'Paris' OR city <> 'Lyon'` | ❌ refused | the two lists are applied together, i.e. ANDed | + | `city = 'Paris' OR city LIKE 'L%'` | ❌ refused | ⚠️ a pattern REPLACES the list — see below | + | `city LIKE 'P%' OR city LIKE 'L%'` | ❌ refused | one list holds one pattern, so the second is lost | - The refused rows previously returned a plausible-looking but WRONG set of groups. Split the query, - or restate the condition as an `OR` of equalities or an `AND` of inequalities. + ⚠️ **Being a union is necessary but not sufficient.** Each of the two lists holds either a set of + values or ONE pattern (`LIKE` / `RLIKE`), and a pattern replaces the set — so a pattern meeting + anything else in the SAME list loses a side, even where the combination itself is a disjunction. + `HAVING city = 'Paris' OR city LIKE 'L%'` used to return only the `L…` groups. Use a single + `RLIKE` covering both alternatives, or split the query. + + The refused rows previously returned a plausible-looking but WRONG set of groups. Otherwise: + split the query, or restate the condition as an `OR` of equalities or an `AND` of inequalities. - ⚠️ **A FUNCTION of the key is refused** (`HAVING UPPER(city) = 'PARIS'`, `HAVING LENGTH(status) = 1`). The terms filter can only express a direct comparison, and the alternatives are unsound: filtering documents instead would keep or drop a multi-valued document @@ -852,10 +861,14 @@ SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city <> 'Lyon'; `WHERE`. - A predicate naming a column that is **neither** the `GROUP BY` key **nor** an aggregate is refused (`HAVING UPPER(name) = 'X'` when the grouping is by `city`) — with or without a `GROUP BY`. -- ⚠️ An `OR` whose branches need **different mechanisms** is refused: a group filter - (`bucket_selector`), a key filter (`terms`) and a nested filter are separate stages, so - `HAVING COUNT(*) > 1 OR city = 'Paris'` would be executed as a conjunction. An `OR` within one - mechanism is supported. +- ⚠️ An `OR` whose branches need **different stages** is refused, because Elasticsearch applies + the stages one inside the other, which is a conjunction: + - different MECHANISMS — a group filter (`bucket_selector`), a key filter (`terms`) and a nested + filter: `HAVING COUNT(*) > 1 OR city = 'Paris'`; + - different GROUPING KEYS — the two `terms` aggregations are NESTED, so + `GROUP BY country, city HAVING country = 'FR' OR city = 'Paris'` would return only the groups + matching BOTH. An `OR` on ONE key, within one mechanism, is supported subject to the table + above; the corresponding `AND` is always fine, because the nesting IS the conjunction. - An `AND` across mechanisms is fine — each stage applies its own half. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index d960b5948..151c67fea 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -294,6 +294,45 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { q should include(""""exclude":["c","d"]""") } + it should "refuse a pattern meeting a value list in the SAME channel" in { + // 🔴 MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: `= 'a' OR LIKE 'b%'` emitted + // `include:"b.*"` and returned ['b1'] where the SQL means ['a','b1']. The emission keeps the + // PATTERN and discards the value list, and a second pattern is lost to `orElse`. + Seq( + "status = 'a' OR status LIKE 'b%'", + "status LIKE 'a%' OR status LIKE 'b%'", + "status <> 'a' AND status NOT LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should include("a pattern replaces the list") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + // ... while a pattern in EACH channel is fine: they are different lists. + val q = queryOf(group + "status LIKE 'a%' AND status NOT LIKE 'b%'") + q should include(""""include":"a.*"""") + q should include(""""exclude":"b.*"""") + } + + "an OR across TWO grouping levels" should "never reach emission" in { + // 🔴 MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): Elasticsearch NESTS the two `terms` + // aggregations, so `status = 'a' OR city = 'b'` emitted + // `terms status include:["a"] > terms city include:["b"]` and returned ONE group where the + // SQL means THREE. The AND is exactly what the nesting means, and still emits. + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + Parser(two + "status = 'a' OR city = 'b'") match { + case Left(e) => e.msg should include("DIFFERENT GROUP BY keys") + case Right(_) => fail(s"accepted; it emitted ${queryOf(two + "status = 'a' OR city = 'b'")}") + } + val q = queryOf(two + "status = 'a' AND city = 'b'") + // ⚠️ ES6 RENDERING: this bridge writes a SINGLE-element list as a bare string. See the + // residual note above -- recorded, pre-existing, byte-identical to `455433ae`. + q should include(""""include":"a"""") + q should include(""""include":"b"""") + } + // --------------------------------------------------------------------------------------------- // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. // --------------------------------------------------------------------------------------------- diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index 1232ec711..535c63c81 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -177,7 +177,14 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") val names = boundToPredicate.flatMap(_.findAllMatchIn(code).map(_.group(1))).toSet val namedUses = names.toSeq.map { n => - s"""(?\.not`, so + // `p.includePolarityOfRight(not)` -- the member that exists PRECISELY to consume this + // fold -- was invisible, and its file scored 0 uses / 0 classifications and passed + // VACUOUSLY. A gate that stops seeing the site it has just caught is worse than no gate. + // The named fold counts as a use AND, like `negated`, as a classification: calling it IS + // the fold. Replace that call with a raw `p.not` and the file reddens, which is the point. + (s"""(? classifications) Some( 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 42bc8508f..4042e6d18 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 @@ -838,46 +838,70 @@ package object query { */ private[query] def keyChannelConflict: Option[String] = { val empty = BucketIncludesExcludes() - def conflict(c: Criteria, bucket: Bucket, not: Boolean): Option[Criteria] = c match { - case p @ Predicate(left, op, right, _, _) => - // ONE derivation of the polarity, shared with `Criteria.includes` -- see - // `Predicate.includePolarityOfRight`. - val rightNot = p.includePolarityOfRight(not) - val leftIncludes = left.includes(bucket, not, empty) - val leftExcludes = left.excludes(bucket, not, empty) - val rightIncludes = right.includes(bucket, rightNot, empty) - val rightExcludes = right.excludes(bucket, rightNot, empty) - // 🔴 No De Morgan arm here, and that is MEASURED, not assumed: `Predicate.maybeNot` - // negates the RIGHT OPERAND only, and `NOT ( … )` around a group is rejected by the - // grammar (`end of input expected`, on this branch and on `455433ae`). So no `Predicate` - // is ever reached with a flipped polarity -- probed over the parseable shapes -- and an - // arm dualising `op` would be unreachable for every possible input. Round 3 shipped - // exactly such an arm and had to delete it; one is enough. - val leftContributes = leftIncludes != empty || leftExcludes != empty - val rightContributes = rightIncludes != empty || rightExcludes != empty - val here = - if (op == AND && leftIncludes != empty && rightIncludes != empty) Some(p: Criteria) - else if ( - op == OR && leftContributes && rightContributes && - (leftExcludes != empty || rightExcludes != empty) - ) Some(p: Criteria) - else None - here - .orElse(conflict(left, bucket, not)) - .orElse(conflict(right, bucket, rightNot)) - case relation: ElasticRelation => conflict(relation.criteria, bucket, not) - case _ => None - } + def unionOnlyReason(c: Criteria): String = + s"HAVING cannot combine the conditions in ${c.sql} on one GROUP BY key: Elasticsearch " + + "applies one list of kept values and one list of removed values, and each is a union, " + + "so this combination would be executed as a different one. Split the query, or restate " + + "it as an OR of equalities or an AND of inequalities." + def conflict(c: Criteria, bucket: Bucket, not: Boolean): Option[(Criteria, String)] = + c match { + case p @ Predicate(left, op, right, _, _) => + // ONE derivation of the polarity, shared with `Criteria.includes` -- see + // `Predicate.includePolarityOfRight`. + val rightNot = p.includePolarityOfRight(not) + val leftIncludes = left.includes(bucket, not, empty) + val leftExcludes = left.excludes(bucket, not, empty) + val rightIncludes = right.includes(bucket, rightNot, empty) + val rightExcludes = right.excludes(bucket, rightNot, empty) + // 🔴 No De Morgan arm here, and that is MEASURED, not assumed: `Predicate.maybeNot` + // negates the RIGHT OPERAND only, and `NOT ( … )` around a group is rejected by the + // grammar (`end of input expected`, on this branch and on `455433ae`). So no `Predicate` + // is ever reached with a flipped polarity -- probed over the parseable shapes -- and an + // arm dualising `op` would be unreachable for every possible input. Round 3 shipped + // exactly such an arm and had to delete it; one is enough. + val leftContributes = leftIncludes != empty || leftExcludes != empty + val rightContributes = rightIncludes != empty || rightExcludes != empty + // 🔴 A channel holds ONE list of values and ONE pattern, and the emission keeps the + // PATTERN and discards the values (`ElasticAggregation`, both bridges), while a second + // pattern is lost to `orElse`. So two contributors COLLIDE whenever a pattern meets + // anything else in the same channel -- whatever the operator, and even where the + // combination itself is a union. MEASURED on ES 8.18.3 over `a`, `b1`, `c`: + // `HAVING status = 'a' OR status LIKE 'b%'` -> `include:"b.*"` -> ['b1'], and the + // SQL means ['a','b1']. + // ⚠️ This is the claim an earlier draft of §12.F got wrong: `= 'a' OR LIKE 'b%'` IS an + // OR of two kept-value contributions -- the row the documentation blesses -- so + // "the kept list is a union" is not sufficient on its own. + def collides(a: BucketIncludesExcludes, b: BucketIncludesExcludes): Boolean = + (a.regex.nonEmpty && b.regex.nonEmpty && a.regex != b.regex) || + (a.regex.nonEmpty && b.values.nonEmpty) || + (b.regex.nonEmpty && a.values.nonEmpty) + val here = + if (collides(leftIncludes, rightIncludes) || collides(leftExcludes, rightExcludes)) + Some( + (p: Criteria) -> + (s"HAVING cannot combine the conditions in ${p.sql} on one GROUP BY key: a key " + + "filter carries one list of values and one pattern, and a pattern replaces the " + + "list, so one side would be silently dropped. Use a single RLIKE that covers " + + "both, or split the query.") + ) + else if (op == AND && leftIncludes != empty && rightIncludes != empty) + Some((p: Criteria) -> unionOnlyReason(p)) + else if ( + op == OR && leftContributes && rightContributes && + (leftExcludes != empty || rightExcludes != empty) + ) Some((p: Criteria) -> unionOnlyReason(p)) + else None + here + .orElse(conflict(left, bucket, not)) + .orElse(conflict(right, bucket, rightNot)) + case relation: ElasticRelation => conflict(relation.criteria, bucket, not) + case _ => None + } having.flatMap(_.criteria).flatMap { criteria => buckets.view .flatMap(bucket => conflict(criteria, bucket, not = false)) .headOption - .map(c => - s"HAVING cannot combine the conditions in ${c.sql} on one GROUP BY key: Elasticsearch " + - "applies one list of kept values and one list of removed values, and each is a union, " + - "so this combination would be executed as a different one. Split the query, or " + - "restate it as an OR of equalities or an AND of inequalities." - ) + .map { case (_, reason) => reason } } } @@ -1270,24 +1294,51 @@ package object query { case e: Expression => Seq(e) case _ => Nil } - def firstMixedOr(c: Criteria): Option[(Criteria, Seq[String])] = c match { + // 🔴 The STAGE, not just the mechanism. `mechanismOf` collapses every grouping level + // to the single label "key", so an OR across TWO GROUP BY keys looked homogeneous -- + // and Elasticsearch NESTS the two `terms` aggregations, which IS the conjunction this + // rule exists to catch. MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): + // `GROUP BY status, city HAVING status = 'a' OR city = 'b'` + // -> terms status include:["a"] > terms city include:["b"] -> ONE group (a,b), + // and the SQL means THREE. + // A leaf's stage is its mechanism PLUS, for a key predicate, the bucket it addresses. + def levelOf(e: Expression): String = + if (e.nested || havingScopeOf(e) != HavingScope.GroupKey) "" + else namedLeavesOf(e).flatMap(keyBucketOf).map(_.name).distinct.sorted.mkString(",") + def firstMixedOr(c: Criteria): Option[(Criteria, Seq[String], Boolean)] = c match { case p @ Predicate(l, op, r, _, _) => val here = if (op == OR) { - val kinds = leavesUnder(p).map(mechanismOf).distinct - if (kinds.size > 1) Some((p: Criteria, kinds)) else None + val leaves = leavesUnder(p) + val kinds = leaves.map(mechanismOf).distinct + val stages = leaves.map(e => (mechanismOf(e), levelOf(e))).distinct + if (stages.size > 1) + Some( + ( + p: Criteria, + if (kinds.size > 1) kinds.sorted else stages.map(_._2).sorted, + kinds.size > 1 + ) + ) + else None } else None here.orElse(firstMixedOr(l)).orElse(firstMixedOr(r)) case relation: ElasticRelation => firstMixedOr(relation.criteria) case _ => None } having.flatMap(_.criteria).filter(_ => groupBy.isDefined).flatMap(firstMixedOr) match { - case Some((p, kinds)) => + case Some((p, names, differentMechanisms)) => Left( - s"HAVING cannot OR conditions that Elasticsearch applies with different mechanisms " + - s"(${p.sql} mixes ${kinds.sorted.mkString(" and ")}): a group filter, a key filter " + - "and a nested filter are separate stages, so their disjunction would be executed " + - "as a conjunction. Split the query, or restate the condition as an AND." + if (differentMechanisms) + s"HAVING cannot OR conditions that Elasticsearch applies with different " + + s"mechanisms (${p.sql} mixes ${names.mkString(" and ")}): a group filter, a key " + + "filter and a nested filter are separate stages, so their disjunction would be " + + "executed as a conjunction. Split the query, or restate the condition as an AND." + else + s"HAVING cannot OR conditions on DIFFERENT GROUP BY keys (${p.sql} spans " + + s"${names.mkString(" and ")}): Elasticsearch nests one grouping level inside " + + "the other, so filtering both would be executed as a conjunction. Split the " + + "query, or restate the condition as an AND." ) case None => Right(()) } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index b0f9ffcdc..6efcafaf3 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -469,44 +469,51 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt // senses; the population, not the assertion, is what was missing. // ------------------------------------------------------------------------------------------- - /** Both halves of the matrix, as the predicate text and which channel the leaf feeds. */ - private val includeSide = Seq( - "status = 'a'" -> "status = 'b'", - "status IN ('a','x')" -> "status = 'b'", - "status LIKE 'a%'" -> "status LIKE 'b%'" - ) - private val excludeSide = Seq( - "status <> 'a'" -> "status <> 'b'", - "status NOT IN ('a','x')" -> "status <> 'b'", - "status NOT LIKE 'a%'" -> "status NOT LIKE 'b%'" + /** Every key operator, with the CHANNEL it feeds and the FORM it puts there. + * + * 🔴 The first version of this matrix paired each operator with a fixed partner (`=` with `=`, + * `IN` with `=`, `LIKE` with `LIKE`) and varied only channel × combiner, so `=` was NEVER + * crossed with `LIKE` -- which is exactly where the defect lived. The operators are CROSSED now. + * Every defect found on this branch was a hole in the population, never in the assertions. + */ + private val keyOperators = Seq( + ("status = 'a'", "include", "values"), + ("status IN ('a','x')", "include", "values"), + ("status LIKE 'a%'", "include", "regex"), + ("status <> 'a'", "exclude", "values"), + ("status NOT IN ('a','x')", "exclude", "values"), + ("status NOT LIKE 'a%'", "exclude", "regex") ) - "the terms channel" should "express every DERIVED combination of key predicates, or refuse it" in { - // The verdict is DERIVED from the principle, never listed by hand: the include list is a union - // and therefore a DISJUNCTION; the exclude list is a union of negations and therefore a - // CONJUNCTION. So two leaves are expressible only as an OR of includes or an AND of excludes. + /** The second operand, so a crossed cell never repeats its partner verbatim. */ + private def partnerOf(predicate: String): String = + predicate.replace("'a'", "'b'").replace("'a%'", "'b%'").replace("('b','x')", "('b','x')") + + "the terms channel" should "express every CROSSED combination of key predicates, or refuse it" in { + // The verdict is DERIVED from two properties of the channel, never listed by hand: + // 1. the kept list is a union (a DISJUNCTION) and the removed list a union of negations (a + // CONJUNCTION), so two same-channel leaves need OR and AND respectively; + // 2. a channel holds ONE list and ONE pattern, and the pattern REPLACES the list -- so a + // pattern meeting anything else in the same channel loses a side whatever the operator. val cells = for { - (leftSide, leftName) <- Seq(includeSide -> "include", excludeSide -> "exclude") - (left, right) <- leftSide - (rightSide, rightName) <- Seq(includeSide -> "include", excludeSide -> "exclude") - combiner <- Seq("AND", "OR") + (left, leftChannel, leftForm) <- keyOperators + (right, rightChannel, rightForm) <- keyOperators + combiner <- Seq("AND", "OR") } yield { - // pair each left with the OTHER side's matching right, so mixed cells exist too - val other = rightSide(leftSide.indexOf((left, right)))._2 - val rhs = if (rightName == leftName) right else other - val sql = s"$left $combiner $rhs" - val sameChannel = leftName == rightName + val rhs = partnerOf(right) + val sameChannel = leftChannel == rightChannel + val collides = + sameChannel && (leftForm == "regex" || rightForm == "regex") val expressible = - // same channel: the include list is a disjunction, the exclude list a conjunction ... - (sameChannel && leftName == "include" && combiner == "OR") || - (sameChannel && leftName == "exclude" && combiner == "AND") || - // ... and the two channels are themselves ANDed by Elasticsearch, so one of each under - // a conjunction is exactly what they mean together. - (!sameChannel && combiner == "AND") - (sql, expressible) + !collides && ( + (sameChannel && leftChannel == "include" && combiner == "OR") || + (sameChannel && leftChannel == "exclude" && combiner == "AND") || + (!sameChannel && combiner == "AND") + ) + (s"$left $combiner $rhs", expressible) } - cells.distinct.size should be >= 20 + cells.distinct.size shouldBe 72 cells.distinct.foreach { case (predicate, expressible) => withClue(s"[$predicate] expressible=$expressible ") { Parser(group + predicate).isRight shouldBe expressible @@ -514,8 +521,53 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt } } + it should "refuse a pattern that meets a value list in the SAME channel" in { + // 🔴 MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: + // `HAVING status = 'a' OR status LIKE 'b%'` emitted `include:"b.*"` and returned ['b1']; + // the SQL means ['a','b1']. The emission keeps the PATTERN and discards the list. + // ⚠️ It is an OR of two KEPT-value contributions -- the very row "the kept list is a union" + // blesses -- so the union property alone is NOT sufficient, and an earlier draft of this + // branch's own documentation said it was. + Seq( + "status = 'a' OR status LIKE 'b%'", + "status LIKE 'b%' OR status IN ('a','c')", + "status <> 'a' AND status NOT LIKE 'b%'", + "status NOT LIKE 'b%' AND status NOT IN ('a','c')", + "status LIKE 'a%' OR status LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + rejection(group + p) should include("a pattern replaces the list") + } + } + } + + it should "still allow a pattern in EACH channel, which do not collide" in { + Parser(group + "status LIKE 'a%' AND status NOT LIKE 'b%'").isRight shouldBe true + } + + "an OR across TWO GROUP BY keys" should "be refused, because the levels are NESTED" in { + // 🔴 MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): + // `GROUP BY status, city HAVING status = 'a' OR city = 'b'` + // -> terms status include:["a"] > terms city include:["b"] -> ONE group (a,b), + // and the SQL means THREE. + // `mechanismOf` labels every grouping level "key", so the round-3 rule saw one mechanism and + // passed. Nesting two `terms` aggregations IS the conjunction that rule exists to catch. + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + val msg = rejection(two + "status = 'a' OR city = 'b'") + msg should include("DIFFERENT GROUP BY keys") + msg should include("nests one grouping level inside the other") + // ... and the message must NOT reuse the mechanism wording, which is about a group filter, + // a key filter and a nested filter -- not about two grouping levels. + msg should not include "different mechanisms" + } + + it should "still allow an AND across two grouping keys, which IS the nesting" in { + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + Parser(two + "status = 'a' AND city = 'b'").isRight shouldBe true + } + it should "leave a SINGLE key predicate of either sense alone" in { - (includeSide ++ excludeSide).flatMap { case (l, r) => Seq(l, r) }.distinct.foreach { p => + keyOperators.map(_._1).foreach { p => withClue(s"[$p] ")(Parser(group + p).isRight shouldBe true) } } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index 19b59ddc9..cf1f690de 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -1009,6 +1009,45 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit ) shouldBe Seq("cat_37") } + "a pattern meeting a value list on the same key" should "be REFUSED, not half-applied" in { + // 🔴 MEASURED on ES 8.18.3 over a three-bucket fixture: `= 'a' OR LIKE 'b%'` emitted + // `include:"b.*"` and returned only the pattern's bucket -- the value list is DISCARDED by the + // emission when a pattern is present, and a second pattern is lost to `orElse`. + // ⚠️ the client caps a relayed parse message at 200 characters (#262), so the assertion + // anchors on the TAIL of the reason, not its middle. + categoryRefusal( + "HAVING category = 'cat_37' OR category LIKE 'cat_3%'" + ) should include("Use a single RLIKE") + // ... and the pattern ALONE still works: cat_30 .. cat_37. + categoriesOf("HAVING category LIKE 'cat_3%'") shouldBe + (30 to 37).map(c => f"cat_$c%02d") + } + + "an OR across TWO grouping levels" should "be REFUSED, because Elasticsearch NESTS them" in { + // 🔴 The two `terms` aggregations are nested, which IS a conjunction: an OR across them + // returned the intersection. Measured on a two-key fixture; here the refusal is asserted + // end-to-end, and the AND -- which is exactly what the nesting means -- still answers. + Await.result( + client.run( + "SELECT category, id, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category, id " + + "HAVING category = 'cat_02' OR id = 'cat_02_1'" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"the OR was accepted and answered $result") + case ElasticFailure(error) => error.message should include("DIFFERENT GROUP BY keys") + } + client.searchAsUnchecked[CategoryCount]( + SelectStatement( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category, id " + + "HAVING category = 'cat_02' AND id = 'cat_02_1'" + ) + ) match { + case ElasticSuccess(rows) => rows.map(_.category) shouldBe Seq("cat_02") + case ElasticFailure(error) => fail(s"the AND failed: ${error.message}") + } + } + it should "REFUSE a HAVING on a column that is neither grouped nor aggregated" in { Await.result( client.run( From 1a6a23a1ff64a93614ba3b5e0391170e7558b592 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Thu, 24 Sep 2026 19:56:28 +0200 Subject: [PATCH 6/6] fix(sql): the terms channel uses the SHARED LIKE translation (#389) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delta review of `893e2ce2` returned ready-to-push and disclosed five findings. Two are fixed here. F1 -- the terms include/exclude channel was a THIRD derivation of the LIKE -> regex translation: `v.value.replaceAll("%", ".*")`. It neither translated `_` nor escaped a regex metacharacter, while the query-DSL path uses the shared `toRegex` -- and `metricSelector`'s own scaladoc asserts the shared one is used, so the code contradicted its own documentation. MEASURED on ES 8.18.3 over the buckets `a.bZ`, `axbZ`, `ab`, `a1`: status LIKE 'a_' WHERE -> [a1, ab] HAVING -> NO BUCKETS status LIKE 'a.b%' WHERE -> [a.bZ] HAVING -> [a.bZ, axbZ] Pre-existing and byte-identical to `455433ae`, so not a regression -- but a silent wrong answer in the very channel this branch reasons about, which the standing rule says is fixed in the branch. `RLIKE` is raw regex by definition and is untouched. ⚠️ This CHANGES EMISSIONS. Measured against the control over a LIKE population: five of thirteen move, all of them in the `_`/metacharacter class (`a_` -> `a.`, `a.b%` -> `a\.b.*`, `a+b%` -> `a\+b.*`, `a_b` -> `a.b`, `cat_3%` -> `cat.3.*`); `LIKE 'a%'`, `LIKE '%a%'`, every `RLIKE` form, `=` and `<>` are byte-identical. 🔴 PINS THAT MOVED: NONE -- no pin anywhere used a LIKE literal with `_` or a metacharacter on this channel, which is exactly why the defect survived. New pins in both bridges and in `sql`, plus an EXECUTED row: `HAVING category LIKE 'cat_0_'` selected nothing before (no category is named `cat_0_`) and now selects cat_01 .. cat_09. F4 -- two unpinned decisions in the previous commit. The equal-pattern exemption in `collides` (`LIKE 'a%' OR LIKE 'a%'` is one pattern twice, so nothing is dropped) is live and correct: PINNED, with its complement. The `e.nested ||` guard in `levelOf` is DELETED: it made two leaves on different NESTED grouping levels collapse to one stage while two flat levels are refused -- the very asymmetry that rule exists to remove -- and it could not be pinned, because with no schema attached `GROUP BY e.name` under `JOIN UNNEST` emits no `terms` at all on either tree. An unobservable guard in a symmetry rule is worse than none. F5/F6 -- a no-op `.replace` deleted from the matrix, and the matrix's `collides` model documented as COARSER than production (it cannot express the equal-pattern exemption and is sound over the 72 cells only because every crossed partner differs). 🔴 Also corrects a FALSE claim this branch shipped twice. The release note and §13.C characterised the newly-refused population from a generated sample whose alphabet -- six operators over one keyword column -- contains no `IS NULL`, no range comparison, no qualified column name and no empty-string literal. Both halves were wrong: `= 'a' AND <> 'a'` is ACCEPTED (one include, one exclude), and the real set contains `HAVING IS NOT NULL`, `HAVING age > 0` on a grouping key, and `HAVING t.status = 'a'` with `GROUP BY t.status`. The release notes now NAME those families, and the spec states the alphabet beside the population so the next reader can judge its reach. Recorded, not fixed (ES 6.8.23 only, byte-identical to `455433ae`, correct on 8.18.3): a pattern in one channel beside a MULTI-value list in the other is also inexpressible there -- `LIKE 'a%' AND NOT IN ('b','x')` and two siblings answer HTTP 400. Wider than the single-element residual already recorded. Measured: the 366-statement cross-product re-run against its independent oracle after the change -- WRONG 0; sql 1635 / core 1141 / bridge 238 / es6bridge 234; cross compile 2.12.20 + 2.13.16; the exact CI lint line; `GroupByCompletenessSpec` 47/47 on real ES 8.18.3 and 47/47 on ES 6.8.23. Closes #389 Co-Authored-By: Claude Opus 5 (1M context) --- .../sql/HavingFunctionEmissionSpec.scala | 13 +++++ .../sql/HavingFunctionEmissionSpec.scala | 13 +++++ .../softnetwork/elastic/sql/query/Where.scala | 12 +++- .../elastic/sql/query/package.scala | 10 +++- .../HavingOverAggregateFunctionSpec.scala | 55 ++++++++++++++++++- .../client/GroupByCompletenessSpec.scala | 9 +++ 6 files changed, 109 insertions(+), 3 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index b28783d98..6fa427d39 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -304,6 +304,19 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { q should include(""""exclude":"b.*"""") } + "a LIKE pattern on the key" should "use the SHARED translation" in { + // 🔴 The terms channel had a THIRD private LIKE -> regex translation that neither handled `_` + // nor escaped a metacharacter, while the query-DSL path used the shared `toRegex`. MEASURED + // on ES 8.18.3: `LIKE 'a_'` matched NO buckets where the WHERE form matched two, and + // `LIKE 'a.b%'` matched `axbZ` as well as `a.bZ`. + queryOf(group + "status LIKE 'a_'") should include(""""include":"a."""") + queryOf(group + "status LIKE 'a.b%'") should include(""""include":"a\\.b.*"""") + // ... RLIKE is RAW regex by definition and must NOT be translated. + queryOf(group + "status RLIKE 'a.b'") should include(""""include":"a.b"""") + // ... and a pattern with neither is byte-identical to `455433ae`. + queryOf(group + "status LIKE 'a%'") should include(""""include":"a.*"""") + } + "an OR across TWO grouping levels" should "never reach emission" in { // 🔴 MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): Elasticsearch NESTS the two `terms` // aggregations, so `status = 'a' OR city = 'b'` emitted diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 151c67fea..1c10886f1 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -316,6 +316,19 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { q should include(""""exclude":"b.*"""") } + "a LIKE pattern on the key" should "use the SHARED translation" in { + // 🔴 The terms channel had a THIRD private LIKE -> regex translation that neither handled `_` + // nor escaped a metacharacter, while the query-DSL path used the shared `toRegex`. MEASURED + // on ES 8.18.3: `LIKE 'a_'` matched NO buckets where the WHERE form matched two, and + // `LIKE 'a.b%'` matched `axbZ` as well as `a.bZ`. + queryOf(group + "status LIKE 'a_'") should include(""""include":"a."""") + queryOf(group + "status LIKE 'a.b%'") should include(""""include":"a\\.b.*"""") + // ... RLIKE is RAW regex by definition and must NOT be translated. + queryOf(group + "status RLIKE 'a.b'") should include(""""include":"a.b"""") + // ... and a pattern with neither is byte-identical to `455433ae`. + queryOf(group + "status LIKE 'a%'") should include(""""include":"a.*"""") + } + "an OR across TWO grouping levels" should "never reach emission" in { // 🔴 MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): Elasticsearch NESTS the two `terms` // aggregations, so `status = 'a' OR city = 'b'` emitted 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 29518f7a0..02f4c9501 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 @@ -600,8 +600,18 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { if ((!not && maybeNot.isEmpty) || (not && maybeNot.isDefined)) maybeValue match { case Some(v: StringValue) if v.value.nonEmpty => + // 🔴 The SHARED `toRegex`, not a third private translation. This line used to + // read `v.value.replaceAll("%", ".*")`, which neither translates `_` nor escapes + // a regex metacharacter -- while `metricSelector`'s scaladoc asserts the shared + // one is used and the query-DSL path really does use it. MEASURED on ES 8.18.3 + // over the buckets `a.bZ`, `axbZ`, `ab`, `a1`: + // `status LIKE 'a_'` WHERE -> [a1, ab] HAVING -> NO BUCKETS + // `status LIKE 'a.b%'` WHERE -> [a.bZ] HAVING -> [a.bZ, axbZ] + // Pre-existing and byte-identical to `455433ae`, so not a regression -- but a + // silent wrong answer found while working on this very channel. + // ⚠️ `RLIKE` below is RAW regex by definition and must NOT be translated. bucketIncludesExcludes.copy(regex = - bucketIncludesExcludes.regex.orElse(Option(v.value.replaceAll("%", ".*"))) + bucketIncludesExcludes.regex.orElse(Option(toRegex(v.value))) ) case _ => bucketIncludesExcludes } 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 4042e6d18..fa5061c58 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 @@ -1302,8 +1302,16 @@ package object query { // -> terms status include:["a"] > terms city include:["b"] -> ONE group (a,b), // and the SQL means THREE. // A leaf's stage is its mechanism PLUS, for a key predicate, the bucket it addresses. + // 🔴 No `e.nested ||` guard here, and its removal is DELIBERATE (final review F4). It + // made two leaves on DIFFERENT nested grouping levels collapse to one stage while two + // flat levels are refused -- the very asymmetry this rule exists to remove -- and it was + // unobservable: with no schema attached, `GROUP BY e.name` under `JOIN UNNEST` emits no + // `terms` at all, on this tree or on `455433ae`. An unobservable, unpinnable guard in a + // symmetry rule is worse than no guard, so the rule is symmetric by construction + // instead. A leaf that is not a GROUP BY key still has no level, which is what the + // second half says. def levelOf(e: Expression): String = - if (e.nested || havingScopeOf(e) != HavingScope.GroupKey) "" + if (havingScopeOf(e) != HavingScope.GroupKey) "" else namedLeavesOf(e).flatMap(keyBucketOf).map(_.name).distinct.sorted.mkString(",") def firstMixedOr(c: Criteria): Option[(Criteria, Seq[String], Boolean)] = c match { case p @ Predicate(l, op, r, _, _) => diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index 6efcafaf3..77b06c31f 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -62,6 +62,21 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt private def script(sql: String): String = MetricSelectorScript.metricSelector(having(sql)) + /** The pattern the `terms` filter would carry, asked of the REAL derivation. */ + private def includeOf(sql: String): Option[String] = { + val st = parsed(sql) + st.buckets.iterator + .map(b => havingOf(st, sql).includes(b, not = false, BucketIncludesExcludes()).regex) + .collectFirst { case Some(r) => r } + } + + private def excludeOf(sql: String): Option[String] = { + val st = parsed(sql) + st.buckets.iterator + .map(b => havingOf(st, sql).excludes(b, not = false, BucketIncludesExcludes()).regex) + .collectFirst { case Some(r) => r } + } + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " // ------------------------------------------------------------------------------------------- @@ -487,7 +502,7 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt /** The second operand, so a crossed cell never repeats its partner verbatim. */ private def partnerOf(predicate: String): String = - predicate.replace("'a'", "'b'").replace("'a%'", "'b%'").replace("('b','x')", "('b','x')") + predicate.replace("'a'", "'b'").replace("'a%'", "'b%'") "the terms channel" should "express every CROSSED combination of key predicates, or refuse it" in { // The verdict is DERIVED from two properties of the channel, never listed by hand: @@ -503,6 +518,10 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt } yield { val rhs = partnerOf(right) val sameChannel = leftChannel == rightChannel + // ⚠️ COARSER THAN PRODUCTION, deliberately. `keyChannelConflict` exempts two IDENTICAL + // patterns (one pattern expresses both); this model cannot express that, and is sound + // here only because `partnerOf` makes every crossed partner differ. Do not read it as the + // production rule -- the exemption has its own test above. val collides = sameChannel && (leftForm == "regex" || rightForm == "regex") val expressible = @@ -541,6 +560,40 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt } } + "a LIKE pattern on the key" should "go through the SHARED translation, not a third private one" in { + // 🔴 The terms channel was a THIRD derivation of LIKE -> regex: `value.replaceAll("%", ".*")`. + // It neither translated `_` nor escaped a metacharacter, while the query-DSL path uses the + // shared `toRegex` -- and `metricSelector`'s own scaladoc asserts the shared one is used. + // MEASURED on ES 8.18.3 over the buckets `a.bZ`, `axbZ`, `ab`, `a1`: + // `status LIKE 'a_'` WHERE -> [a1, ab] HAVING -> NO BUCKETS + // `status LIKE 'a.b%'` WHERE -> [a.bZ] HAVING -> [a.bZ, axbZ] + // Pre-existing and byte-identical to `455433ae`; fixed here because it is a silent wrong + // answer in the very channel this branch reasons about. + includeOf(group + "status LIKE 'a_'").value shouldBe "a." + includeOf(group + "status LIKE 'a.b%'").value shouldBe "a\\.b.*" + includeOf(group + "status LIKE 'a+b%'").value shouldBe "a\\+b.*" + excludeOf(group + "status NOT LIKE 'a_b'").value shouldBe "a.b" + // ... and the shapes with neither stay byte-identical. + includeOf(group + "status LIKE 'a%'").value shouldBe "a.*" + includeOf(group + "status LIKE '%a%'").value shouldBe ".*a.*" + } + + it should "leave RLIKE alone, which is RAW regex by definition" in { + includeOf(group + "status RLIKE 'a.b'").value shouldBe "a.b" + excludeOf(group + "status NOT RLIKE 'a.*'").value shouldBe "a.*" + } + + "two IDENTICAL patterns on one key" should "NOT collide, because one pattern expresses both" in { + // 🔴 F4: the `a.regex != b.regex` exemption in `collides` was live and correct but unpinned. + // `LIKE 'a%' OR LIKE 'a%'` is one pattern twice: nothing is dropped, so nothing is refused. + Parser(group + "status LIKE 'a%' OR status LIKE 'a%'").isRight shouldBe true + includeOf(group + "status LIKE 'a%' OR status LIKE 'a%'").value shouldBe "a.*" + // ... while two DIFFERENT patterns in one channel do collide. + rejection(group + "status LIKE 'a%' OR status LIKE 'b%'") should include( + "a pattern replaces the list" + ) + } + it should "still allow a pattern in EACH channel, which do not collide" in { Parser(group + "status LIKE 'a%' AND status NOT LIKE 'b%'").isRight shouldBe true } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index cf1f690de..62d81e38f 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -1023,6 +1023,15 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit (30 to 37).map(c => f"cat_$c%02d") } + "a LIKE pattern on the key" should "treat _ as a wildcard, like every other LIKE" in { + // 🔴 EXECUTED. The terms channel had its own LIKE translation that left `_` literal, so this + // matched NOTHING on `455433ae` -- there is no category named `cat_0_`. With the shared + // translation `_` is one character, so it selects cat_01 .. cat_09. + categoriesOf("HAVING category LIKE 'cat_0_'") shouldBe (1 to 9).map(c => f"cat_0$c%d") + // ... and the plain wildcard form is unchanged. + categoriesOf("HAVING category LIKE 'cat_3%'") shouldBe (30 to 37).map(c => f"cat_$c%02d") + } + "an OR across TWO grouping levels" should "be REFUSED, because Elasticsearch NESTS them" in { // 🔴 The two `terms` aggregations are nested, which IS a conjunction: an OR across them // returned the intersection. Measured on a two-key fixture; here the refusal is asserted