From af41d01339cee22c82e560aae5ebbea4d78fe5f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Fri, 25 Sep 2026 12:51:37 +0200 Subject: [PATCH] fix(sql): a MATERIALIZED VIEW refuses a HAVING its transform cannot express A materialized view SILENTLY DROPPED a HAVING clause its transform has no mechanism for - or deployed a bucket_selector that rejects every group - and answered HTTP 200. Seven rules: six specific ones and a unified backstop, each measured at RENDER level on the generated TransformConfig. 1 a condition on a GROUPING key. A search applies one through the terms aggregation's include / exclude list; a transform's group_by has no such channel, so the condition vanished. With a metric conjunct beside it only the KEY half vanished - a partial filter answering 200 with wrong groups. 2 a HAVING with no GROUP BY. The pivot is built from the GROUP BY alone, so there was nothing for a bucket_selector to hang on. 3 an aggregate a view's transform cannot compute. toTransformAggregation has arms for MIN / MAX / SUM / AVG / COUNT and answers None otherwise; buildAggregations flatMaps that None away while extractAggregatePaths keys on *is this an aggregate*, so the two DIVERGE. STDDEV, VARIANCE and PERCENTILE_CONT - even WITH the aggregate in the SELECT list - produced a buckets_path naming an aggregation that is never created. 4 a SELECT bucket_script alias (MAX(x) - MIN(x) AS d). This repo's pivot model has a bucketSelector field and no bucketScript one, and such an item is not an aggregate, so buckets_path came back empty. 5 an aggregate on the RIGHT of a comparison THAT NO LEAF NAMES ON ITS LEFT. extractAggregatePaths walks expr.identifier only and never expr.maybeValue, while core's own extractAllMetricsPath walks both - so an aggregate reached only through a value side lands in buildAggregations and NEVER in buckets_path. The selector IS built, reads a null parameter, and every bucket is rejected: EMPTY view, 200. Both halves of that sentence are load-bearing and each was measured after a gate found the rule too wide: extractAggregatePaths declares the LEFT identifier of EVERY leaf in the whole clause, so a sibling conjunct naming the same aggregate DECLARES it and the view runs correctly (firing on mere presence over-refused 74 cells that are correct on main); and when nothing names it on the left but the SELECT list does not publish it either, adding the SELECT alias is what makes the selector declare it, so rule 6 owns that family and this rule stands down. HAVING MIN(amount) > 1 AND MAX(amount) > MIN(amount) is ACCEPTED once MIN(amount) AS mn is published; HAVING MAX(amount) > MIN(amount) is not. 6 an aggregate the transform could compute but the SELECT list does not publish in the spelling the HAVING uses. 7 THE BACKSTOP - a HAVING leaf naming no aggregation the pivot creates. Two families the six are structurally blind to. (a) An aggregate with NO SELECT alias: core mints a generated alias for an unaliased item and the script reads it, while RequiredField.apply names the aggregation after the USER alias - the two never meet, buckets_path is EMPTY and the clause is silently dropped. Invariant across every computable aggregate, every connective, one and two grouping keys and a JOIN body. (b) A nested / child / parent predicate beside a metric conjunct: extractAggregatePaths has case _ => acc for a relation and havingLeaves excludes them, so the selector is emitted reading the metric alone. The refusal lives in CreateMaterializedView.validate(), beside the three arms that already refuse a set operation, a derived table and a WHERE subquery: one place, a parse-time 400 with a reason and a remedy instead of a 500 from the extension, and every venue rather than one. It is chained AFTER dql.validate() because these are additional constraints on an already valid SELECT. MV-ONLY by construction, asserted in both directions: every refused body is CORRECT as a plain search and stays accepted, still reaching the terms channel and still creating its auxiliary aggregation. The backstop is LAST and the rule set is deliberately NOT collapsed into it. It subsumes rules 1, 3 and 4 entirely and 5 and 6 in part, but those rules exist for their SPECIFIC REMEDIES and one generic message would lose all of them. Rule order is load-bearing and measured: the rules above the SELECT-list rule run first because its remedy is "add it to the SELECT list", and for each of their populations applying that remedy lands somewhere WORSE. The converse is equally measured and is why rule 5 asks a counterfactual rather than "is this parameter declared today": where some leaf names the right-hand aggregate on its left, that remedy DOES end the journey, so rule 5 stands down and rule 6 speaks. Precedence is not a fixed order between two rules - it is whichever remedy reaches a view that deploys. Every remedy that puts an aggregate in the SELECT list says AS , and that is not politeness: a view's HAVING can only read an aggregate that carries a SELECT alias, so "spell it exactly as the HAVING spells it" was - on its own - the instruction that produced the broken artefact, because a HAVING never spells an alias. Applying each remedy literally is a test. The derivations ask the real thing rather than copying it: notPublishedBySelect is auxiliaryAggs' own filter hoisted; the unbuildable rule calls toTransformAggregation itself; the right-operand rule is expressed as the VALUE SIDE, the operand the consumer omits; and the backstop's names come from Field.outputName, which is the consumer's formula character for character - what matters there is the INPUT, raw select.fields rather than the computed aliases. Attribution is deliberate: these are limitations of this engine's transform model, not of Elasticsearch. TransformPivot carries a bucketSelector, so a pipeline aggregation reaches a pivot; MIN/MAX/SUM/AVG/COUNT is toTransformAggregation's arm set. The messages say so. There is deliberately NO rule relaying Having.unrepresentable: it would run after dql.validate(), which already refuses every un-expressible HAVING unconditionally, so such an arm is unsatisfiable for every possible input. Release note: a CREATE MATERIALIZED VIEW now fails at CREATE, where it previously succeeded and materialized every group - or deployed a selector that rejects every group - when its HAVING filters on a grouping key, has no GROUP BY, names an aggregate a view's transform cannot compute (only MIN, MAX, SUM, AVG and COUNT including COUNT DISTINCT are computable), names an expression over aggregates, compares an aggregate with an aggregate NO OTHER CONDITION names on its left, names an aggregate the SELECT list does not publish in the same spelling, reads any aggregate that does not carry a SELECT alias, or contains a nested / child / parent predicate. The last is the one most users will meet: a view's HAVING can only read an aggregate written in the SELECT list with an alias. Comparing two aggregates is NOT refused when some other condition in the same HAVING names the right-hand one on its left (HAVING SUM(x) > MAX(x) AND MAX(x) > 1) - that view deploys and runs correctly on every version, and it is accepted. The identical statement as a plain SELECT is unaffected in every case. Co-Authored-By: Claude Opus 5 (1M context) --- .../MaterializedViewHavingGatewaySpec.scala | 159 +++ .../elastic/sql/query/package.scala | 435 ++++++- .../query/MaterializedViewHavingSpec.scala | 1081 +++++++++++++++++ 3 files changed, 1668 insertions(+), 7 deletions(-) create mode 100644 core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala diff --git a/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala new file mode 100644 index 00000000..2a3e47d2 --- /dev/null +++ b/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala @@ -0,0 +1,159 @@ +/* + * 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.client + +import akka.actor.ActorSystem +import app.softnetwork.elastic.client.result._ +import org.scalatest.BeforeAndAfterAll +import org.scalatest.concurrent.ScalaFutures +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.slf4j.{Logger, LoggerFactory} + +import scala.concurrent.duration._ + +/** The STATUS a materialized view carrying an unmaterializable `HAVING` answers with, OBSERVED + * through `GatewayApi.run(sql)` rather than assumed from the `Parser` verdict. + * + * πŸ”΄ `operation` is `"sql"`, not `"mv"`, and that is forced rather than chosen: the refusal is a + * parse-time rejection, and `GatewayApi.run`'s rejection branch cannot classify a statement it has + * by construction failed to parse. MEASURED: the three materialized-view arms that already ship + * (set operation / derived table / WHERE subquery) answer exactly the same way, so this pins + * CONSISTENCY with them -- and the part that matters is the negative, asserted below: never a 500, + * never a 404, never a 402 (the two defects extensions had to fix for reporting a user error as a + * server fault and a permissions problem as a quota upsell). + */ +class MaterializedViewHavingGatewaySpec + extends AnyFlatSpec + with Matchers + with ScalaFutures + with BeforeAndAfterAll { + + implicit private val system: ActorSystem = ActorSystem("mv-having-gateway") + override implicit val patienceConfig: PatienceConfig = + PatienceConfig(timeout = scaled(10.seconds)) + + override def afterAll(): Unit = { + system.terminate() + super.afterAll() + } + + private class Nope extends NopeClientApi { + override protected def logger: Logger = LoggerFactory.getLogger(getClass) + } + + private val client = new Nope + + private def failureOf(sql: String): ElasticError = + client.run(sql).futureValue match { + case ElasticFailure(error) => error + case other => fail(s"expected a failure for [$sql], got $other") + } + + private val refused: Seq[(String, String)] = Seq( + "a GROUP BY key condition" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'", + "a partial key condition beside a metric" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1 AND city = 'Paris'", + "a HAVING with no GROUP BY" -> + "CREATE MATERIALIZED VIEW mv AS SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1", + "an aggregate the view does not publish" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1", + "an aggregate no transform can compute" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, STDDEV(amount) AS sd FROM customers GROUP BY city HAVING STDDEV(amount) > 1", + "an expression over aggregates" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", + "two aggregates compared" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)" + ) + + "an unmaterializable HAVING" should "answer 400, naming the clause" in { + refused.foreach { case (label, sql) => + withClue(s"[$label] ") { + val error = failureOf(sql) + error.statusCode shouldBe Some(400) + error.message should include("MATERIALIZED VIEW") + error.message should include("HAVING") + } + } + } + + it should "never answer 500, 404 or 402" in { + refused.foreach { case (label, sql) => + withClue(s"[$label] ") { + val code = failureOf(sql).statusCode + code should not be Some(500) + code should not be Some(404) + code should not be Some(402) + } + } + } + + it should "answer exactly as the materialized-view arms that already ship" in { + // The contract asserted is SAMENESS with the sibling refusals, not a literal `"sql"`: the three + // below are the set-operation, derived-table and WHERE-subquery arms, all decided in the same + // `CreateMaterializedView.validate()` and all surfaced through the same rejection branch. + val siblings = Seq( + "CREATE MATERIALIZED VIEW mv AS SELECT city FROM customers UNION ALL SELECT city FROM prospects", + "CREATE MATERIALIZED VIEW mv AS SELECT d.city FROM (SELECT city FROM customers) AS d", + "CREATE MATERIALIZED VIEW mv AS SELECT city FROM customers WHERE id IN (SELECT customer_id FROM orders)" + ).map(failureOf) + siblings.map(_.statusCode).distinct shouldBe Seq(Some(400)) + val siblingOperation = siblings.map(_.operation).distinct + siblingOperation should have size 1 + refused.foreach { case (label, sql) => + withClue(s"[$label] ") { + failureOf(sql).operation shouldBe siblingOperation.head + } + } + } + + "a materialized view whose HAVING a transform CAN express" should "not be refused at parse" in { + // No materialized-view extension is registered on any elasticsql classpath, so the furthest a + // valid CREATE gets here is the DDL router's "unsupported" answer. That it reaches the ROUTER + // at all is the assertion: the new rules did not refuse it. + Seq( + "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1", + "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > 1", + "CREATE OR REPLACE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1", + "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city" + ).foreach { sql => + withClue(s"[$sql] ") { + val error = failureOf(sql) + error.message should not include "MATERIALIZED VIEW cannot" + error.message should not include "MATERIALIZED VIEW with a HAVING" + error.operation shouldBe Some("table") + } + } + } + + "the same HAVING outside a view" should "still be routed to the search executor" in { + // The MV-only separation, at the gateway: the identical body as a plain SELECT is never + // refused by these rules -- it reaches DQL routing and fails only for want of a real cluster. + Seq( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'", + "SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1" + ).foreach { sql => + withClue(s"[$sql] ") { + val error = failureOf(sql) + error.operation shouldBe Some("dql") + error.message should not include "MATERIALIZED VIEW" + } + } + } +} 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 fa5061c5..92d9c515 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 @@ -40,6 +40,7 @@ import app.softnetwork.elastic.sql.function.aggregate.WindowFunction import app.softnetwork.elastic.sql.policy.{EnrichPolicy, EnrichPolicyType} import app.softnetwork.elastic.sql.serialization._ import app.softnetwork.elastic.sql.transform.{ + AggregateConversion, Delay, Frequency, TransformTimeInterval, @@ -138,6 +139,219 @@ package object query { def whereSubqueriesPresent(statement: Statement): Boolean = closureSearches(statement).exists(_.hasWhereSubqueries) + /** Why a `HAVING` an ordinary search expresses cannot be materialized, or `None` when the + * Elasticsearch transform a view deploys can express it. + * + * πŸ”΄ MATERIALIZED-VIEW ONLY, and that is the whole point. Every shape refused here is CORRECT in + * a search: the engine has FIVE mechanisms for a `HAVING` and a transform's pivot offers only + * one of them, the `bucket_selector`. Read by [[CreateMaterializedView.validate]] alone; a + * watcher renders its SELECT into a SEARCH body and therefore keeps all five, so it deliberately + * does NOT read this. + * + * SIX rules. Each was MEASURED at RENDER level (the generated `TransformConfig`) either silently + * dropping the clause or deploying a `bucket_selector` that cannot run, before it was refused. + * They are stated as paragraphs rather than a numbered list because the formatter rewraps list + * items into the previous item's prose. + * + * RULE 1 -- NO `GROUP BY`. A transform's pivot is built from the GROUP BY alone, and with no + * pivot there is nothing for a `bucket_selector` to hang on: the clause was never even + * attempted. (A search has its own whole-table mechanism and is unaffected.) + * + * RULE 2 -- a condition on a GROUPING KEY. A search applies one through the `terms` + * aggregation's `include` / `exclude` list; a transform's `group_by` has no such channel, since + * `TermsGroupBy` emits `{"terms":{"field":…}}` and nothing else. With a metric conjunct beside + * it only the KEY half vanished, which is worse: a partial filter that answers 200 with the + * wrong groups. + * + * RULE 3 -- an aggregate a view's transform cannot compute at all. + * `AggregateConversion.toTransformAggregation` has arms for MIN / MAX / SUM / AVG / COUNT and + * answers `None` for every other aggregate; `Stage.buildAggregations()` `flatMap`s that `None` + * away while `Stage.extractAggregatePaths` keys on *is this an aggregate*, so the two DIVERGE. + * MEASURED: `STDDEV`, `VARIANCE` and `PERCENTILE_CONT` in the SELECT list AND the HAVING + * produced a `buckets_path` naming an aggregation the transform never creates. + * + * RULE 4 -- a SELECT `bucket_script` alias (`MAX(x) - MIN(x) AS d … HAVING d > 3`). This repo's + * transform pivot model has a `bucketSelector` field and NO `bucketScript` one + * (`TransformPivot`), and such a SELECT item is not an aggregate, so it never reaches the + * stage's aggregate list either: MEASURED `buckets_path` EMPTY, clause dropped. + * + * RULE 5 -- an aggregate on the RIGHT of a comparison THAT NO LEAF NAMES ON ITS LEFT. + * `Stage.extractAggregatePaths` walks `expr.identifier` ONLY and never `expr.maybeValue`, while + * core's own `Expression.extractAllMetricsPath` walks BOTH. So an aggregate reached only through + * a value side lands in `buildAggregations` and NEVER in `buckets_path`. MEASURED: the selector + * IS built, reads `params.` which is null, the null guard short-circuits, and EVERY + * bucket is rejected -- an EMPTY view at HTTP 200. + * + * πŸ”΄ Both halves of that sentence are load-bearing, and each was measured after a gate found the + * rule too wide. `extractAggregatePaths` declares the LEFT identifier of EVERY leaf in the whole + * clause, so (i) a sibling conjunct naming the same aggregate DECLARES it and the view runs + * correctly -- firing on mere presence over-refused 74 cells that are correct on `main`; and + * (ii) when nothing names it on the left but the SELECT list does not publish it either, adding + * the SELECT alias is what makes the selector declare it, so RULE 6 owns that family and this + * rule stands down. `HAVING MIN(amount) > 1 AND MAX(amount) > MIN(amount)` is ACCEPTED once + * `MIN(amount) AS mn` is published; `HAVING MAX(amount) > MIN(amount)` is not. See + * [[SingleSearch.havingLeftHandNames]]. + * + * RULE 6 -- an aggregate a transform COULD compute but the SELECT list does not publish, in the + * spelling the HAVING uses. The selector would read a metric the view never creates: either + * `buckets_path` comes back empty and the whole clause is dropped, or -- with a published metric + * beside it -- the path is PARTIAL and the script reads a `params.*` the path never declares. + * + * RULE 7 -- THE BACKSTOP: a `HAVING` leaf whose name matches no aggregation the pivot creates. + * It covers two families the six specific rules are structurally blind to. (a) An aggregate with + * NO SELECT alias: `Select.fieldAliases` mints a generated internal alias and + * `Identifier.update` copies it, so the script reads THAT name, while `RequiredField.apply` + * names the aggregation `fieldAlias.getOrElse(sourceField)` -- the USER alias only. The two + * never match, `buckets_path` comes back EMPTY and the clause is SILENTLY DROPPED; MEASURED + * invariant across every computable aggregate, every connective, one and two grouping keys, and + * a JOIN body. (b) A relation predicate (`NESTED` / `CHILD` / `PARENT`) beside a metric + * conjunct: `extractAggregatePaths` falls to its `case _ => acc` for a relation and + * `havingLeaves` excludes them, so the selector is emitted reading the metric alone -- a PARTIAL + * filter answering 200 with the wrong groups. + * + * πŸ”΄ The backstop is LAST, and the rule set is deliberately NOT collapsed into it. It subsumes + * rules 2, 3 and 4 entirely and 5 and 6 in part, so deleting them is tempting -- do not. Those + * rules exist for their SPECIFIC REMEDIES (filter the key in WHERE; this aggregate exists in no + * view; compare with a constant; publish it under an alias), and a single generic "names no + * aggregation this view creates" would lose every one of them. Specific first, backstop behind. + * + * ⚠️ MEASURED over 5,145 product points: rules 2, 3 and 4 have ZERO sole-owner cells -- every + * statement they refuse would be refused by a later rule anyway -- while rule 5 owns 25 (all of + * them genuinely broken), rule 6 owns 2, rule 1 owns 83 and the backstop owns 134. Their + * coverage contribution is nil ON PURPOSE and it is not a reason to delete them: the remedy is + * the deliverable, not the verdict. + * + * πŸ”΄ Every remedy that puts an aggregate in the SELECT list says `AS `, and that is not + * politeness: a view's HAVING can only read an aggregate that carries a SELECT alias, so *"spell + * it exactly as the HAVING spells it"* was -- on its own -- the instruction that produced the + * broken artefact, because a HAVING never spells an alias. Applying each remedy literally is a + * test. + * + * πŸ”΄ Rules 3, 4 and 5 run BEFORE rule 6 deliberately, and the reason is measured rather than + * aesthetic. Rule 6's remedy is *"add it to the SELECT list"*, and for the populations those + * three carry, applying it LANDS SOMEWHERE WORSE: adding `STDDEV(amount)` produces the broken + * `buckets_path` rule 3 refuses, and adding the right-hand `MIN(amount)` produces the + * every-bucket-rejected selector rule 5 refuses. A refusal whose remedy makes things worse is + * the issue-#389 trap ("verify the remedy a message prescribes, the same way you verify the + * defect"). Each precedence has its own test and its own mutation. + * + * πŸ”΄ The converse is equally measured, and it is why rule 5 asks a counterfactual rather than + * *"is this parameter declared today"*: where SOME leaf names the right-hand aggregate on its + * left, rule 6's remedy DOES end the journey, so rule 5 must stand down and let rule 6 speak. + * Precedence is not a fixed order between two rules -- it is whichever remedy reaches a view + * that deploys. W1-W8 pin both directions, with the remedy applied literally in W5 and W8. + * + * πŸ”΄ Rules 3 and 5 ask the real thing, never a copy of it: rule 3 calls `toTransformAggregation` + * ITSELF rather than matching a list of function names, and rule 5 is expressed as *the value + * side of the comparison*, which is exactly the operand `extractAggregatePaths` omits. A name + * list would be a second derivation of what the transform emitter does and would drift the first + * time that mapping changed -- the same reason [[SingleSearch.notPublishedBySelect]] is hoisted + * rather than copied. + * + * ⚠️ Attribution: these are limitations of THIS ENGINE's transform model, not of Elasticsearch. + * A `bucket_selector` demonstrably reaches a transform pivot -- `TransformPivot` carries one -- + * and MIN/MAX/SUM/AVG/COUNT is `toTransformAggregation`'s arm set, not Elasticsearch's + * capability list. The messages say so, because a user told *"Elasticsearch cannot"* will never + * ask us for the feature. + * + * πŸ”΄ There is deliberately NO rule relaying `Having.unrepresentable`. This function runs only + * AFTER `dql.validate()` has succeeded, and `SingleSearch.validate()` already refuses every + * un-expressible `HAVING` unconditionally -- so such an arm would be unsatisfiable for every + * possible input, which is dead code beside live code (the defect story 21.2 had to reduce). It + * is not an omission; do not add it. + */ + private[query] def materializedViewHavingRefusal(statement: Statement): Option[String] = + statement match { + case search: SingleSearch if search.having.flatMap(_.criteria).isDefined => + if (search.groupBy.isEmpty) + Some( + "MATERIALIZED VIEW with a HAVING but no GROUP BY is not supported: a materialized " + + "view's transform filters groups with a bucket_selector on its pivot, and a view " + + "with no " + + "GROUP BY has no pivot, so the condition would be silently ignored. Add a GROUP BY " + + "and give every aggregate the HAVING reads a SELECT alias (SUM(x) AS s), or " + + "materialize the aggregate without the condition and apply the condition when " + + "querying the view." + ) + else + search.havingLeaves + .find(e => search.havingScopeOf(e) == search.HavingScope.GroupKey) + .map { e => + // πŸ”΄ "grouping key", not "GROUP BY key". `buckets` is `bucketTree.allBuckets`, which + // also carries every window `PARTITION BY` key -- MEASURED: `… MAX(amount) OVER + // (PARTITION BY status) … GROUP BY city HAVING status = 'a'` reaches this arm, and + // `status` is not a GROUP BY key. The refusal is right (a transform expresses + // neither channel); only the noun was. + s"MATERIALIZED VIEW cannot filter on a grouping key in HAVING (${e.sql}): a search " + + "applies that condition through the terms aggregation's include / exclude list, " + + "and a materialized view's transform group_by has no such channel, so the " + + "condition " + + "would be silently dropped. Filter on the key in the view's WHERE clause, keeping " + + "a SELECT alias on every aggregate the HAVING reads (SUM(x) AS s)." + } + .orElse { + search.havingAggsNoTransformCanCompute.headOption.map { f => + s"MATERIALIZED VIEW cannot filter on ${f.identifier.sql} in HAVING: a " + + "materialized view's transform computes only MIN, MAX, SUM, AVG and COUNT " + + "(including COUNT DISTINCT), so this aggregate exists in no materialized view " + + "and the group filter would read a metric that is never created. Apply the " + + "condition when querying the view." + } + } + .orElse { + search.havingBucketScriptRefs.headOption.map { id => + s"MATERIALIZED VIEW cannot filter on ${id.sql} in HAVING: it is an expression " + + "over aggregates, which a search evaluates with a bucket_script and this " + + "engine's transform pivot has no bucket_script channel for, so the condition " + + "would be silently dropped. Apply the condition when querying the view." + } + } + .orElse { + search.havingValueSideAggs.headOption.map { id => + s"MATERIALIZED VIEW cannot compare two aggregates in HAVING (${id.sql} is on the " + + "right of the comparison): a materialized view's transform declares only the " + + "metric on the LEFT of each comparison in its bucket_selector buckets_path, so " + + "the right-hand aggregate is read as an undeclared parameter, the null guard " + + "rejects every group and the view comes out EMPTY. Compare the aggregate with a " + + "constant -- keeping its SELECT alias (SUM(x) AS s) -- or apply the condition " + + "when querying the view." + } + } + .orElse { + search.havingOnlyAggs.headOption.map { f => + // ⚠️ The aggregate is rendered in the HAVING's OWN spelling, which can differ from + // the SELECT's (`SELECT COUNT(id) AS c … HAVING COUNT(`id`)` renders + // `COUNT("id")`). The refusal is right -- the two spellings really do build two + // different metrics, and the selector really is dropped -- but a bare "add it" + // would read as nonsense, so the message says what to compare instead. + s"MATERIALIZED VIEW cannot filter on ${f.identifier.sql} in HAVING: a " + + "materialized view's transform computes only the aggregations the SELECT list " + + "publishes, so the group filter would read a metric the view does not compute " + + s"and the condition would be silently dropped. Add ${f.identifier.sql} AS " + + " to the SELECT list, spelling the aggregate exactly as the HAVING " + + "spells it -- a view's HAVING can only read an aggregate that carries a SELECT " + + "alias." + } + } + .orElse { + // πŸ”΄ THE BACKSTOP. Last on purpose; see the rule list above. + search.havingLeafWithNoMatchingAgg.map { case (leaf, _) => + val created = search.transformAggregationNames.toSeq.sorted + val names = + if (created.isEmpty) "this view creates none" + else created.mkString(", ") + s"MATERIALIZED VIEW cannot filter on ${leaf.sql} in HAVING: a materialized " + + "view's transform names each aggregation after its SELECT alias, and this " + + s"condition matches none of the aggregations this view creates ($names), so the " + + "group filter would be silently dropped or applied only in part. Every " + + "aggregate a view's HAVING reads must be written in the SELECT list with an " + + "alias (SUM(x) AS s), and a nested, child or parent predicate cannot be part of " + + "a view's HAVING at all." + } + } + case _ => None + } + sealed trait Statement extends Token sealed trait DqlStatement extends Statement @@ -659,12 +873,210 @@ package object query { .filter(f => f.isAggregation || f.isBucketScript) .filterNot(_.identifier.hasWindow) ++ windowFields + /** The members of `fields` the SELECT list does NOT already publish. + * + * Dedup against SELECT by EXPRESSION only. Matching by alias too let a user alias hijack a + * derived metric name -- `SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3` read MIN under + * `params.max_x`; such a collision is now rejected by `validate()` instead. + * + * πŸ”΄ ONE derivation, hoisted so it has two readers and cannot drift: [[auxiliaryAggs]], which + * CREATES an aggregation for everything a SEARCH needs and the SELECT does not publish, and + * [[havingOnlyAggs]], which NAMES those same aggregates for the materialized-view refusal -- + * because a TRANSFORM creates no such aggregation. Two copies of this rule would be the + * story-21.3 desync class. + */ + private[query] def notPublishedBySelect(fields: Seq[Field]): Seq[Field] = { + val selectAggNames = selectAggs.map(_.identifier.identifierName).toSet + fields.filterNot(f => selectAggNames.contains(f.identifier.identifierName)) + } + + /** The aggregates this statement's `HAVING` reads that the SELECT list does NOT publish. + * + * A SEARCH creates each of them as an auxiliary aggregation (see [[auxiliaryAggs]]); an + * Elasticsearch TRANSFORM computes only the aggregations the SELECT list carries, so a + * materialized view whose `HAVING` names one of these cannot express the filter at all. + */ + private[query] lazy val havingOnlyAggs: Seq[Field] = + notPublishedBySelect( + having.flatMap(_.criteria).map(_.extractAggregationFields).getOrElse(Seq.empty) + ) + + /** The aggregates this statement's `HAVING` reads that NO Elasticsearch transform can compute. + * + * πŸ”΄ Asked of `AggregateConversion.toTransformAggregation` ITSELF -- the function the + * transform emitter calls -- never of a list of function names. A name list would be a second + * derivation of the same mapping and would drift the first time it gained an entry, which is + * the desync class this file keeps paying for. Today that mapping answers `Some` for MIN / MAX + * / SUM / AVG / COUNT (cardinality included) and `None` for everything else, and + * `Stage.buildAggregations()` DROPS a `None` while `Stage.extractAggregatePaths` still names + * the field -- so a view over such an aggregate deploys a `buckets_path` pointing at an + * aggregation that does not exist. + */ + private[query] lazy val havingAggsNoTransformCanCompute: Seq[Field] = + having + .flatMap(_.criteria) + .map(_.extractAggregationFields) + .getOrElse(Seq.empty) + .filter(_.identifier.aggregateFunction.flatMap(_.toTransformAggregation).isEmpty) + + /** The `HAVING` references that are an EXPRESSION over aggregates rather than an aggregate -- + * what a search emits as a `bucket_script` (`MAX(x) - MIN(x) AS d ... HAVING d > 3`, resolved + * to its SELECT item by `Having.resolveAggregateAliases`). + * + * A transform's pivot has no `bucket_script` channel, and such a SELECT item is not an + * aggregate, so it never enters the stage's aggregate list either: MEASURED, `buckets_path` + * comes back EMPTY and the whole clause is dropped. + * + * The predicate is the one `SingleSearch.validate()` already uses to find inline arithmetic + * over aggregates, minus its `fieldAlias.isEmpty` guard -- there it refuses the UNALIASED form + * for every venue; here the ALIASED form is the one a transform cannot honour. + */ + private[query] lazy val havingBucketScriptRefs: Seq[Identifier] = + having + .flatMap(_.criteria) + .map(_.referencedIdentifiers) + .getOrElse(Nil) + .filter(id => !id.isAggregation && id.hasAggregation) + + /** The aggregates this statement's `HAVING` compares AGAINST -- the VALUE side of a comparison. + * + * πŸ”΄ The operand `Stage.extractAggregatePaths` omits. It walks `expr.identifier` only and + * never `expr.maybeValue`, while core's own [[Expression.extractAllMetricsPath]] walks BOTH, + * so a right-hand aggregate reaches `buildAggregations` (the view really does compute it) and + * never reaches `buckets_path`. MEASURED: the `bucket_selector` IS built, its script reads + * `params.`, that parameter is undeclared and therefore null, the null guard + * short-circuits and EVERY bucket is rejected -- the view materialises EMPTY at HTTP 200. + * + * Expressed as *the value side*, deliberately: that is the same operand the consumer omits, so + * the rule and the defect cannot drift apart. Where publishing the aggregate does not help, + * this must be decided before the "not published by the SELECT list" rule -- and where it DOES + * help, [[havingLeftHandNames]] hands the statement to that rule instead (see below). + * + * πŸ”΄ NARROWED after the coverage gate: PRESENCE of an aggregate on the value side is NOT the + * defect -- being UNDECLARED is. `extractAggregatePaths` declares a parameter for the left + * identifier of EVERY leaf in the whole clause, so a sibling conjunct naming the same + * aggregate declares it and the transform deploys and runs correctly. MEASURED on the control: + * `HAVING SUM(amount) > MAX(amount) AND MAX(amount) > 1` has UNDECLARED = none, and the same + * aggregate on both sides (`MAX(x) > MAX(x)`) declares itself. Firing on presence over-refused + * 74 cells that are CORRECT on main -- a regression, and its remedy ("compare with a + * constant") would have changed the meaning of a working query. + * + * πŸ”΄ NARROWED a second time, by the same argument one step further out: the set of declaring + * names is [[havingLeftHandNames]], NOT the parameters the pivot declares TODAY. The question + * this rule must answer is the COUNTERFACTUAL -- would publishing the right-hand aggregate + * make the selector declare it? MEASURED: `HAVING MIN(amount) > 1 AND MAX(amount) > + * MIN(amount)` is ACCEPTED once `MIN(amount) AS mn` joins the SELECT list, so rule 6 owns it + * and its remedy ends the journey; `HAVING MAX(amount) > MIN(amount)`, where no leaf names + * `MIN(amount)` on the left, is refused again after publishing it, so this rule owns that one. + * Asking the today-question instead sent the first family here, whose remedy ("compare with a + * constant") would have changed the MEANING of a query a one-line SELECT alias fixes. Pinned + * by the W cells, remedy applied literally in W5 / W8. + * + * ⚠️ The `isAggregation` guard is a SECOND LINE OF DEFENCE and is unreachable from SQL, which + * is stated because it was MEASURED rather than assumed: a value side that is NOT an aggregate + * is already refused by the shared rules inside `dql.validate()`, before this runs + * -- a plain column (`HAVING COUNT(*) > amount`) and a grouping key (`… > city`) by + * `Having.unrepresentable` ("its rendering can evaluate to NULL"), and `HAVING city = status` + * by the key-predicate rule. A SELECT alias of an aggregate is SUBSTITUTED by + * `Having.resolveAggregateAliases` before it gets here, so it passes the guard as the + * aggregate it is. The guard therefore keeps the derivation's NAME true for a `SingleSearch` + * assembled in code -- the same role `MetricSelectorScript.metricSelector`'s throw plays -- + * and its mutation is GREEN for that reason, not for want of a test. + */ + /** The aggregation NAMES a materialized view's pivot actually creates. + * + * `Stage.buildAggregations()` keeps a SELECT aggregate only when + * `AggregateConversion.toTransformAggregation` answers `Some`, and `RequiredField.apply` names + * it `field.fieldAlias.map(_.alias).getOrElse(field.sourceField)` -- which is + * [[Field.outputName]] character for character, so core's own derivation is used rather than a + * copy of the consumer's formula (verified: writing either spells the same set). + * + * πŸ”΄ What matters is the INPUT, not the formula: `select.fields`, RAW. The generated alias + * `select.fieldsWithComputedAliases` mints for an unaliased item never reaches the consumer, + * while the HAVING script DOES read it -- and that disagreement is the whole of rule 7's first + * family. + * + * ⚠️ The `toTransformAggregation` filter is unreachable from SQL, because rule 3 refuses an + * un-computable aggregate in a HAVING before the backstop runs; it is kept so the set is + * honestly "what the pivot creates" for a `SingleSearch` assembled in code. Its mutation is + * GREEN for that reason (see `havingValueSideAggs` for the same situation). + */ + /** The name `Stage.extractAggregatePaths` computes for an identifier -- ONE derivation, read by + * [[havingLeafAggNames]] (the declaring side) and by [[havingValueSideAggs]] (the value side), + * so the two sides of a comparison cannot be named by two different rules. + */ + private[query] def transformFieldName(id: Identifier): String = + id.fieldAlias match { + case Some(alias) => alias + case None if id.name.nonEmpty => id.name + case _ => AliasUtils.normalize(id.identifierName) + } + + private[query] lazy val transformAggregationNames: Set[String] = + select.fields.flatMap { f => + f.aggregateFunction.flatMap(_.toTransformAggregation).map(_ => f.outputName) + }.toSet + + /** The name each `HAVING` leaf would be looked up under, paired with the leaf itself. + * + * πŸ”΄ This walks the WHOLE criteria tree, `ElasticRelation` INCLUDED -- unlike + * [[havingLeaves]], which deliberately excludes relation predicates. That difference is the + * whole point of the backstop: `Stage.extractAggregatePaths` falls to its `case _ => acc` for + * a relation, so a relation conjunct beside a metric one is invisible to every rule built on + * `havingLeaves` and the selector is emitted reading the metric alone -- a PARTIAL filter + * answering 200 with the wrong groups. + * + * The name is computed exactly as `extractAggregatePaths` computes it, so the two cannot + * disagree about which leaf resolves to which aggregation. + */ + private[query] lazy val havingLeafAggNames: Seq[(Criteria, String)] = { + def walk(c: Criteria): Seq[(Criteria, String)] = c match { + case p: Predicate => walk(p.leftCriteria) ++ walk(p.rightCriteria) + case relation: ElasticRelation => walk(relation.criteria) + case e: Expression => Seq(c -> transformFieldName(e.identifier)) + case _ => Nil + } + having.flatMap(_.criteria).toSeq.flatMap(walk) + } + + /** Every name some `HAVING` leaf carries on its LEFT -- the names the selector's `buckets_path` + * declares, or WOULD declare once the SELECT list publishes them. + * + * πŸ”΄ `Stage.extractAggregatePaths` declares a parameter for the LEFT identifier of EVERY leaf + * in the WHOLE clause, keeping the ones the pivot creates. So a name is declared as soon as + * ANY leaf's identifier side carries it -- a sibling conjunct counts -- AND the SELECT list + * publishes it. + * + * πŸ”΄ Deliberately NOT intersected with [[transformAggregationNames]], because rule 5 asks a + * COUNTERFACTUAL, not what the pivot declares today: would publishing this aggregate make the + * selector declare it? MEASURED both ways -- `HAVING MIN(amount) > 1 AND MAX(amount) > + * MIN(amount)` becomes ACCEPTED once `MIN(amount) AS mn` joins the SELECT list, so rule 6's + * remedy ends the journey and rule 6 must speak; `HAVING MAX(amount) > MIN(amount)`, where no + * leaf names `MIN(amount)` on the left, is still refused after publishing it, so rule 5 must + * speak for that one instead of steering the user into a second refusal. Intersecting here + * routes the first family to rule 5, whose remedy ("compare with a constant") changes the + * MEANING of a query a one-line SELECT alias would have fixed. + */ + private[query] lazy val havingLeftHandNames: Set[String] = + havingLeafAggNames.map(_._2).toSet + + /** The first `HAVING` leaf whose name matches no aggregation the pivot creates, or `None`. + * + * The UNIFIED backstop (see `materializedViewHavingRefusal`): it subsumes several of the + * specific rules, which are deliberately kept IN FRONT of it for their remedies. + */ + private[query] lazy val havingLeafWithNoMatchingAgg: Option[(Criteria, String)] = + havingLeafAggNames.find { case (_, name) => !transformAggregationNames.contains(name) } + + private[query] lazy val havingValueSideAggs: Seq[Identifier] = + havingLeaves.flatMap(_.maybeValue).collect { + case id: Identifier + if id.isAggregation && !havingLeftHandNames.contains(transformFieldName(id)) => + id + } + // Aggregations referenced only in HAVING, WHERE, or ORDER BY clauses (not in SELECT) private lazy val auxiliaryAggs: Seq[Field] = { - // Dedup against SELECT by EXPRESSION only. Matching by alias too let a user alias hijack a - // derived metric name -- `SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3` read MIN under - // `params.max_x`; such a collision is now rejected by `validate()` instead. - val selectAggNames = selectAggs.map(_.identifier.identifierName).toSet val havingAggs = having .flatMap(_.criteria) .map(_.extractAggregationFields) @@ -689,8 +1101,7 @@ package object query { .flatMap(id => id.metricName.map(name => Field(id, Some(Alias(name))))) // Dedup by name, keeping the first occurrence IN ORDER -- a `groupBy` here hashed the order, // so the emitted `aggs` shuffled between runs and could not be pinned. - (havingAggs ++ whereAggs ++ orderByAggs ++ bucketScriptAggs) - .filterNot(f => selectAggNames.contains(f.identifier.identifierName)) + notPublishedBySelect(havingAggs ++ whereAggs ++ orderByAggs ++ bucketScriptAggs) .foldLeft(Seq.empty[Field]) { (acc, f) => if (acc.exists(_.fieldAlias.map(_.alias) == f.fieldAlias.map(_.alias))) acc else acc :+ f } @@ -2415,7 +2826,17 @@ package object query { "scalar or quantified subquery) is not supported: an Elasticsearch transform cannot run " + "the inner query. Materialize the subquery's values first and reference them." ) - else dql.validate() + else + // The three arms above are structural: they refuse a BODY SHAPE, so they run first and the + // DERIVED / set-operation message keeps winning whenever both apply. What follows is a + // different species -- ADDITIONAL constraints on an otherwise VALID select -- so + // `dql.validate()` runs first and `materializedViewHavingRefusal` only ever sees a + // statement every ordinary rule already accepted. That ordering is also what makes a + // materialized-view arm for `Having.unrepresentable` unreachable; see there. + for { + _ <- dql.validate() + _ <- materializedViewHavingRefusal(dql).map(Left(_)).getOrElse(Right(())) + } yield () override def sql: String = { // The leading space belongs HERE, not to `Frequency.sql`: `TransformConfig` renders the same diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala new file mode 100644 index 00000000..5e4add4a --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala @@ -0,0 +1,1081 @@ +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.parser.Parser +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** A MATERIALIZED VIEW may not carry a `HAVING` an Elasticsearch TRANSFORM cannot express. + * + * SIX shapes were MEASURED at RENDER level (the generated `TransformConfig`) either silently + * dropping the clause or deploying a `bucket_selector` that cannot run, on `origin/main` at + * `04b1c694`: + * + * - a condition on a GROUPING key -- a search rides the `terms` `include` / `exclude` channel, a + * transform's `group_by` has none (`TermsGroupBy` emits `{"terms":{"field":…}}` and nothing + * else), so the whole condition vanished. With a metric conjunct beside it, only the key half + * vanished: a PARTIAL filter, which is a wrong answer that answers 200; + * - a `HAVING` with no `GROUP BY` -- `FinalTransformStage.toTransformConfig` builds no pivot, so + * the `bucket_selector` is never even attempted; + * - an aggregate NO transform can compute (`STDDEV`, `VARIANCE`, `PERCENTILE_CONT`, …) -- even + * WITH the aggregate in the SELECT list, `Stage.buildAggregations()` drops it because + * `AggregateConversion.toTransformAggregation` answers `None`, while + * `Stage.extractAggregatePaths` still names it: a `buckets_path` pointing at an aggregation + * that does not exist; + * - a SELECT `bucket_script` alias (`MAX(x) - MIN(x) AS d`) -- a transform's pivot has no + * `bucket_script` channel and such an item is not an aggregate, so `buckets_path` came back + * EMPTY and the clause was dropped; + * - an aggregate on the RIGHT of a comparison -- `Stage.extractAggregatePaths` walks + * `expr.identifier` ONLY and never `expr.maybeValue`, while core's own + * `Expression.extractAllMetricsPath` walks BOTH, so publishing it puts it in + * `buildAggregations` and NEVER in `buckets_path`: the selector IS built, reads a null + * parameter, the guard short-circuits and EVERY bucket is rejected -- an EMPTY view at 200; + * - an aggregate a transform COULD compute but the SELECT list does not publish -- the selector + * reads a metric the view never creates: `buckets_path` empty (dropped) or, beside a published + * metric, PARTIAL, so the script reads a `params.*` the path never declares. + * + * πŸ”΄ MV-ONLY. Every one of those bodies is CORRECT as a plain search and stays accepted -- + * asserted here in both directions, and executed against real Elasticsearch by the testkit's + * `GroupByCompletenessSpec`. + */ +class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { + + /** What a statement's verdict is, and why. `Refused` carries the reason so a row can assert the + * RULE that fired and not merely that something failed. + */ + private sealed trait Verdict + private case object Accepted extends Verdict + private case class Refused(reason: String) extends Verdict + + private def verdict(sql: String): Verdict = Parser(sql) match { + case Right(_) => Accepted + case Left(err) => Refused(err.msg) + } + + private def asView(body: String): String = s"CREATE MATERIALIZED VIEW mv_probe AS $body" + + /** Which of the six rules a refusal came from -- keyed on the PROPERTY the message carries, never + * on an internal identifier, so message wording stays free to change + * (`feedback_assert_the_mechanism_not_a_proxy`). + */ + private object Rule { + val NoGroupBy = "no GROUP BY" + val GroupKey = "grouping key" + val NoTransformAgg = "computes only MIN, MAX, SUM, AVG" + val BucketScript = "expression over aggregates" + val TwoAggregates = "compare two aggregates" + val Unpublished = "to the SELECT list" + val NoMatchingAgg = "matches none of the aggregations this view creates" + val all: Seq[String] = + Seq( + NoGroupBy, + GroupKey, + NoTransformAgg, + BucketScript, + TwoAggregates, + Unpublished, + NoMatchingAgg + ) + } + + private def refusalOf(sql: String): String = verdict(sql) match { + case Refused(reason) => reason + case Accepted => fail(s"expected a refusal, got ACCEPTED: [$sql]") + } + + private def parsedSearch(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"expected a SingleSearch, got $other: [$sql]") + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // The POPULATION. Every cell of the spec's Β§3 carries a verdict for BOTH venues, so the MV-only + // separation is read off the table rather than argued. `mv` is the verdict of + // `CREATE MATERIALIZED VIEW … AS `; `select` is the verdict of `` on its own. + // ───────────────────────────────────────────────────────────────────────────────────────────── + private case class Cell(id: String, body: String, mvRefusedBy: Option[String]) + + private val population: Seq[Cell] = Seq( + // (A) a metric the SELECT publishes: the `bucket_selector` a transform CAN build. + Cell( + "A1 metric in SELECT", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1", + None + ), + Cell( + "A2 metric via SELECT alias", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING c > 1", + None + ), + Cell( + "A3 SUM in SELECT", + "SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING SUM(amount) > 1", + None + ), + Cell( + "A4 WHERE + GROUP BY + metric", + "SELECT city, COUNT(*) AS c FROM customers WHERE amount > 0 GROUP BY city HAVING COUNT(*) > 1", + None + ), + // (B) a condition on the GROUP BY key -- the terms include/exclude channel, which a transform lacks. + Cell( + "B1 key =", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'", + Some(Rule.GroupKey) + ), + Cell( + "B2 key <>", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city <> 'Paris'", + Some(Rule.GroupKey) + ), + Cell( + "B3 key IN", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city IN ('Paris','Lyon')", + Some(Rule.GroupKey) + ), + Cell( + "B4 key LIKE", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city LIKE 'Par%'", + Some(Rule.GroupKey) + ), + Cell( + "B5 key OR key", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris' OR city = 'Lyon'", + Some(Rule.GroupKey) + ), + Cell( + "B6 key on 2nd grouping level", + "SELECT city, status, COUNT(*) AS c FROM customers GROUP BY city, status HAVING status = 'a'", + Some(Rule.GroupKey) + ), + Cell( + "B7 key NOT IN", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city NOT IN ('Paris','Lyon')", + Some(Rule.GroupKey) + ), + // (C) the PARTIAL shape: only the key half vanished, which answers 200 with wrong groups. + Cell( + "C1 metric AND key", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1 AND city = 'Paris'", + Some(Rule.GroupKey) + ), + Cell( + "C2 key AND metric", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris' AND COUNT(*) > 1", + Some(Rule.GroupKey) + ), + // (D) an aggregate the view does not publish. + Cell( + "D1 MAX not in SELECT", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1", + Some(Rule.Unpublished) + ), + Cell( + "D2 MIN not in SELECT", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MIN(amount) > 1", + Some(Rule.Unpublished) + ), + Cell( + "D3 one published, one not", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1 AND MAX(amount) > 2", + Some(Rule.Unpublished) + ), + Cell( + "D4 aggregate IS in SELECT", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > 1", + None + ), + Cell( + "D5 remedy: add it to SELECT", + "SELECT city, COUNT(*) AS c, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > 1", + None + ), + // (E) a HAVING with no GROUP BY -- no pivot, so no selector. + Cell( + "E1 no GROUP BY, metric", + "SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1", + Some(Rule.NoGroupBy) + ), + Cell( + "E2 no GROUP BY, metric alias", + "SELECT COUNT(*) AS c FROM customers HAVING c > 1", + Some(Rule.NoGroupBy) + ), + Cell( + "E3 no GROUP BY, SUM", + "SELECT SUM(amount) AS s FROM customers HAVING SUM(amount) > 1", + Some(Rule.NoGroupBy) + ), + // (H) no HAVING at all: the shapes a transform has always been able to build. + Cell("H1 no HAVING, GROUP BY", "SELECT city, COUNT(*) AS c FROM customers GROUP BY city", None), + Cell("H2 no HAVING, no GROUP BY", "SELECT id, name FROM customers", None), + Cell("H3 WHERE only", "SELECT id, name FROM customers WHERE city = 'Paris'", None), + // (J) the repo's one existing materialized-view HAVING fixture, and its neighbours. + Cell( + "J1 JOIN, metric HAVING (existing fixture)", + "SELECT c.city, SUM(o.amount) AS total FROM customers c INNER JOIN orders o ON c.id = o.customer_id GROUP BY c.city HAVING SUM(o.amount) > 10000", + None + ), + Cell( + "J2 JOIN, no HAVING", + "SELECT c.city, SUM(o.amount) AS total FROM customers c INNER JOIN orders o ON c.id = o.customer_id GROUP BY c.city", + None + ), + Cell( + "J3 JOIN, unpublished aggregate", + "SELECT c.city, SUM(o.amount) AS total FROM customers c INNER JOIN orders o ON c.id = o.customer_id GROUP BY c.city HAVING MAX(o.amount) > 1", + Some(Rule.Unpublished) + ), + // (N) an aggregate NO transform can compute. `AggregateConversion.toTransformAggregation` + // maps MIN / MAX / SUM / AVG / COUNT and answers None for everything else, and + // `Stage.buildAggregations()` drops that None while `Stage.extractAggregatePaths` still names + // the field -- so these deployed a `buckets_path` pointing at an aggregation that is never + // created, even though the SELECT list "publishes" them. + Cell( + "N1 STDDEV in SELECT and HAVING", + "SELECT city, STDDEV(amount) AS sd FROM customers GROUP BY city HAVING STDDEV(amount) > 1", + Some(Rule.NoTransformAgg) + ), + Cell( + "N2 VARIANCE in SELECT and HAVING", + "SELECT city, VARIANCE(amount) AS vr FROM customers GROUP BY city HAVING VARIANCE(amount) > 1", + Some(Rule.NoTransformAgg) + ), + Cell( + "N3 PERCENTILE_CONT in SELECT and HAVING", + "SELECT city, PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY amount) AS p95 FROM customers " + + "GROUP BY city HAVING PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY amount) > 1", + Some(Rule.NoTransformAgg) + ), + Cell( + "N4 STDDEV via SELECT alias", + "SELECT city, STDDEV(amount) AS sd FROM customers GROUP BY city HAVING sd > 1", + Some(Rule.NoTransformAgg) + ), + Cell( + "N5 STDDEV beside a published COUNT", + "SELECT city, COUNT(*) AS c, STDDEV(amount) AS sd FROM customers GROUP BY city " + + "HAVING STDDEV(amount) > 1 AND COUNT(*) > 2", + Some(Rule.NoTransformAgg) + ), + Cell( + "N6 STDDEV NOT in SELECT", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING STDDEV(amount) > 1", + Some(Rule.NoTransformAgg) + ), + Cell( + "N7 bucket_script alias", + "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", + Some(Rule.BucketScript) + ), + // πŸ”΄ The non-over-reach row: an aggregate no transform can compute is fine in the SELECT list + // as long as the HAVING does not read it. Without this, rule 3 could refuse the whole family. + Cell( + "N8 STDDEV in SELECT, HAVING reads only COUNT", + "SELECT city, COUNT(*) AS c, STDDEV(amount) AS sd FROM customers GROUP BY city HAVING COUNT(*) > 1", + None + ), + Cell( + "N9 MIN is computable", + "SELECT city, MIN(amount) AS mn FROM customers GROUP BY city HAVING MIN(amount) > 1", + None + ), + Cell( + "N10 COUNT DISTINCT is computable", + "SELECT city, COUNT(DISTINCT id) AS cd FROM customers GROUP BY city HAVING COUNT(DISTINCT id) > 1", + None + ), + // (R) an aggregate on the RIGHT of the comparison. πŸ”΄ The population had NO row comparing two + // aggregates -- every earlier cell is ` ` or ` ` -- which + // is why a whole rule could be added without one assertion moving. + // + // extensions' `Stage.extractAggregatePaths` walks `expr.identifier` ONLY and never + // `expr.maybeValue`, while core's own `Expression.extractAllMetricsPath` walks BOTH. So + // publishing the right-hand aggregate puts it in `buildAggregations` and NEVER in + // `buckets_path`. MEASURED: the selector IS built, reads `params.` which is null, the + // null guard short-circuits, and EVERY bucket is rejected -- an EMPTY view at HTTP 200. + Cell( + "R1 MAX > MIN, both published", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) > MIN(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "R2 same, alias spelling", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING mx > mn", + Some(Rule.TwoAggregates) + ), + Cell( + "R3 reversed operand order", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MIN(amount) < MAX(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "R4 inequality", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) <> MIN(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "R5 SUM > AVG", + "SELECT city, SUM(amount) AS sm, AVG(amount) AS av FROM customers GROUP BY city " + + "HAVING SUM(amount) > AVG(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "R6 beside a well-formed metric, AND", + "SELECT city, COUNT(*) AS c, MAX(amount) AS mx, MIN(amount) AS mn FROM customers " + + "GROUP BY city HAVING COUNT(*) > 1 AND MAX(amount) > MIN(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "R7 beside a well-formed metric, OR", + "SELECT city, COUNT(*) AS c, MAX(amount) AS mx, MIN(amount) AS mn FROM customers " + + "GROUP BY city HAVING COUNT(*) > 1 OR MAX(amount) > MIN(amount)", + Some(Rule.TwoAggregates) + ), + // πŸ”΄ The other direction: the right-hand aggregate is NOT published either. Both rules apply; + // the ordering test below pins which one speaks, and it must be this one -- rule 5's remedy + // ("add it to the SELECT list") is MEASURED to produce R1. + Cell( + "R8 right-hand aggregate NOT published", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", + Some(Rule.TwoAggregates) + ), + // (V) πŸ”΄ A right-hand aggregate is undeclared ONLY when its name appears on no leaf's LEFT + // side anywhere in the clause -- `Stage.extractAggregatePaths` declares a parameter for the + // left identifier of EVERY leaf in the whole tree. A sibling conjunct that names the same + // aggregate therefore declares it, and the transform deploys and runs correctly. + // MEASURED on the control: UNDECLARED = none, selector built, both aggregations created. + // These MUST stay accepted; refusing them is a regression against main. + Cell( + "V1 sibling conjunct declares it, AND", + "SELECT city, SUM(amount) AS m, MAX(amount) AS m2 FROM customers GROUP BY city " + + "HAVING SUM(amount) > MAX(amount) AND MAX(amount) > 1", + None + ), + Cell( + "V2 sibling conjunct declares it, OR", + "SELECT city, SUM(amount) AS m, MAX(amount) AS m2 FROM customers GROUP BY city " + + "HAVING SUM(amount) > MAX(amount) OR MAX(amount) > 1", + None + ), + Cell( + "V3 alias spelling on the left", + "SELECT city, SUM(amount) AS m, MAX(amount) AS m2 FROM customers GROUP BY city " + + "HAVING m > MAX(amount) AND m2 > 1", + None + ), + Cell( + "V4 two grouping keys", + "SELECT city, status, SUM(amount) AS m, MAX(amount) AS m2 FROM customers " + + "GROUP BY city, status HAVING SUM(amount) > MAX(amount) AND MAX(amount) > 1", + None + ), + Cell( + "V5 MIN / AVG pair", + "SELECT city, MIN(amount) AS mn, AVG(amount) AS av FROM customers GROUP BY city " + + "HAVING MIN(amount) > AVG(amount) AND AVG(amount) > 1", + None + ), + Cell( + "V6 the declaring sibling comes FIRST", + "SELECT city, SUM(amount) AS m, MAX(amount) AS m2 FROM customers GROUP BY city " + + "HAVING MAX(amount) > 1 AND SUM(amount) > MAX(amount)", + None + ), + // family B: both operands are the SAME aggregate. Tautologically false, but that is what the + // SQL means and the plain search renders the identical script -- the path declares it. + Cell( + "V7 the same aggregate on both sides", + "SELECT city, MAX(amount) AS m2 FROM customers GROUP BY city HAVING MAX(amount) > MAX(amount)", + None + ), + Cell( + "V8 the same aggregate, alias spelling", + "SELECT city, MAX(amount) AS m2 FROM customers GROUP BY city HAVING m2 > m2", + None + ), + // …and the negatives that keep rule 5 alive: nothing declares the right-hand name. + Cell( + "V9 no sibling declares it", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) > MIN(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "V10 right-hand aggregate not published at all", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", + Some(Rule.TwoAggregates) + ), + // (W) πŸ”΄ Which of rule 5 and rule 6 speaks when BOTH apply -- the right-hand aggregate is + // undeclared AND the SELECT list does not publish it. The discriminator is the COUNTERFACTUAL: + // would publishing it make the selector declare it? `Stage.extractAggregatePaths` declares the + // LEFT identifier of EVERY leaf, so the answer is yes exactly when some leaf names it on the + // left. When it does, rule 6's remedy ends the journey (W5 / W8 are W1 / W3 with the remedy + // applied literally, and they are ACCEPTED); when nothing names it on the left, publishing it + // lands on V9 and rule 5 must speak instead (W6, R8/V10 above). + Cell( + "W1 a sibling leaf names it on the left, unpublished", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + + "HAVING MIN(amount) > 1 AND MAX(amount) > MIN(amount)", + Some(Rule.Unpublished) + ), + Cell( + "W2 the same, with a further well-formed conjunct", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + + "HAVING MIN(amount) > 1 AND MAX(amount) > 2 AND MAX(amount) > MIN(amount)", + Some(Rule.Unpublished) + ), + Cell( + "W3 the leaf that names it on the left is the leaf itself", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MIN(amount) > MIN(amount)", + Some(Rule.Unpublished) + ), + Cell( + "W4 COUNT(*) named on the left by a sibling", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + + "HAVING COUNT(*) > 1 AND MAX(amount) > COUNT(*)", + Some(Rule.Unpublished) + ), + Cell( + "W5 W1 with rule 6's remedy applied literally", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MIN(amount) > 1 AND MAX(amount) > MIN(amount)", + None + ), + Cell( + "W6 nothing names it on the left: publishing it would not help", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + + "HAVING MIN(amount) > 1 AND MIN(amount) > MAX(amount)", + Some(Rule.TwoAggregates) + ), + Cell( + "W7 both published, alias spelling on the right", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) > mn", + Some(Rule.TwoAggregates) + ), + Cell( + "W8 W3 with rule 6's remedy applied literally", + "SELECT city, COUNT(*) AS c, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MIN(amount) > MIN(amount)", + None + ), + // (U) the BACKSTOP population: a HAVING leaf whose selector parameter names no aggregation the + // pivot creates. Two families, both PRE-EXISTING (accepted on the control too) and both still + // correct as a plain SELECT. + // + // U-1: the aggregate carries NO SELECT alias. `Select.fieldAliases` mints a generated internal + // alias for an unaliased item and `Identifier.update` copies it, so the script reads that name + // -- while `RequiredField.apply` names the aggregation `fieldAlias.getOrElse(sourceField)`, the + // USER alias only. The two never match, `buckets_path` comes back EMPTY and the clause is + // SILENTLY DROPPED. MEASURED invariant across every computable aggregate, every connective, + // one and two grouping keys, and a JOIN body. It survived because the estate's only MV+HAVING + // fixture aliases everything. + Cell( + "U1 SUM with no SELECT alias", + "SELECT city, SUM(amount) FROM customers GROUP BY city HAVING SUM(amount) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U2 COUNT(*) with no SELECT alias", + "SELECT city, COUNT(*) FROM customers GROUP BY city HAVING COUNT(*) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U3 COUNT(col) with no SELECT alias", + "SELECT city, COUNT(id) FROM customers GROUP BY city HAVING COUNT(id) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U4 COUNT DISTINCT with no SELECT alias", + "SELECT city, COUNT(DISTINCT id) FROM customers GROUP BY city HAVING COUNT(DISTINCT id) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U5 MIN with no SELECT alias", + "SELECT city, MIN(qty) FROM customers GROUP BY city HAVING MIN(qty) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U6 two unaliased aggregates, AND", + "SELECT city, SUM(amount), MIN(qty) FROM customers GROUP BY city " + + "HAVING SUM(amount) > 5 AND MIN(qty) > 1", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U7 two grouping keys, unaliased aggregate", + "SELECT city, status, SUM(amount) FROM customers GROUP BY city, status HAVING SUM(amount) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U8 JOIN body, unaliased aggregate", + "SELECT c.city, SUM(o.amount) FROM customers c INNER JOIN orders o ON c.id = o.customer_id " + + "GROUP BY c.city HAVING SUM(o.amount) > 5", + Some(Rule.NoMatchingAgg) + ), + Cell( + "U9 BETWEEN, unaliased aggregate", + "SELECT city, SUM(amount) FROM customers GROUP BY city HAVING SUM(amount) BETWEEN 1 AND 9", + Some(Rule.NoMatchingAgg) + ), + // U-2: a relation predicate BESIDE a metric conjunct. `extractAggregatePaths` falls to its + // `case _ => acc` for an `ElasticRelation`, and `havingLeaves` deliberately excludes relation + // predicates -- so the grouping-key and two-aggregate rules are STRUCTURALLY blind to it. The + // selector IS built and reads the metric only: a PARTIAL filter answering 200 with the wrong + // groups, which is exactly the harm the grouping-key rule's own docstring names. + Cell( + "U10 relation predicate beside a metric", + "SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING s > 5 AND child(c.x = 1)", + Some(Rule.NoMatchingAgg) + ), + // (P) the ONE materialized-view example in the shipped documentation that carries a HAVING -- + // `documentation/sql/materialized_views.md` "Materialized View with Aggregations", verbatim. + // A published example the engine rejects is the issue-#302 family, so it is PINNED rather than + // reasoned about: every aggregate its HAVING names is in the SELECT list, so it still creates. + Cell( + "P1 published MV example", + "SELECT c.city, c.country, COUNT(*) AS order_count, SUM(o.amount) AS total_amount, " + + "AVG(o.amount) AS avg_amount, MAX(o.amount) AS max_amount " + + "FROM orders o JOIN customers c ON o.customer_id = c.id " + + "WHERE o.status = 'completed' GROUP BY c.city, c.country " + + "HAVING SUM(o.amount) > 10000 ORDER BY total_amount DESC LIMIT 100", + None + ) + ) + + "every cell of the population" should "carry the verdict the spec names, in BOTH venues" in { + population.foreach { cell => + withClue(s"[${cell.id}] MATERIALIZED VIEW: ") { + (verdict(asView(cell.body)), cell.mvRefusedBy) match { + case (Accepted, None) => succeed + case (Refused(reason), Some(property)) => reason should include(property) + case (Accepted, Some(property)) => + fail(s"expected a refusal naming '$property', got ACCEPTED") + case (Refused(reason), None) => fail(s"expected ACCEPTED, got refused: $reason") + } + } + // πŸ”΄ The other direction of AC-2: the SAME body, as a plain search, is ALWAYS accepted. + withClue(s"[${cell.id}] plain SELECT: ") { + verdict(cell.body) shouldBe Accepted + } + } + } + + it should "exercise every rule and both outcomes" in { + // Non-vacuity, computed over the table the rows are drawn from -- not a literal count. + val refusals = population.flatMap(_.mvRefusedBy).distinct + refusals should contain theSameElementsAs Rule.all + population.count(_.mvRefusedBy.isEmpty) should be > 0 + } + + it should "key each rule on a property no OTHER rule's message carries" in { + // πŸ”΄ The population above reads a rule off a substring. If two messages shared a key, a row + // could pass while a DIFFERENT rule fired -- so the keys are proved mutually exclusive + // against the real messages, not eyeballed. + val canonical: Map[String, String] = Map( + Rule.NoGroupBy -> refusalOf( + asView("SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1") + ), + Rule.GroupKey -> refusalOf( + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'") + ), + Rule.NoTransformAgg -> refusalOf( + asView( + "SELECT city, STDDEV(amount) AS sd FROM customers GROUP BY city HAVING STDDEV(amount) > 1" + ) + ), + Rule.BucketScript -> refusalOf( + asView( + "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3" + ) + ), + Rule.TwoAggregates -> refusalOf( + asView( + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) > MIN(amount)" + ) + ), + Rule.Unpublished -> refusalOf( + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1") + ), + Rule.NoMatchingAgg -> refusalOf( + asView("SELECT city, SUM(amount) FROM customers GROUP BY city HAVING SUM(amount) > 5") + ) + ) + canonical.foreach { case (key, message) => + withClue(s"[$key] ") { + message should include(key) + Rule.all.filterNot(_ == key).foreach { other => + withClue(s"also carries the key of another rule ($other): ") { + message should not include other + } + } + } + } + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // Rule 3's oracle: the verdict must agree with `AggregateConversion.toTransformAggregation` + // ITSELF -- the function the transform emitter calls -- never with a list of function names. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "the unbuildable-aggregate rule" should "agree with toTransformAggregation on every spelling" in { + import app.softnetwork.elastic.sql.transform.AggregateConversion + // The SPELLINGS are written (one must write SQL); no EXPECTATION is. Each row's verdict is + // computed by asking the emitter's own mapping, so the rule and the emitter cannot disagree. + val spellings = Seq( + "COUNT(*)", + "COUNT(id)", + "COUNT(DISTINCT id)", + "MIN(amount)", + "MAX(amount)", + "SUM(amount)", + "AVG(amount)", + "STDDEV(amount)", + "STDDEV_POP(amount)", + "STDDEV_SAMP(amount)", + "VARIANCE(amount)", + "VAR_POP(amount)", + "VAR_SAMP(amount)", + "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY amount)", + "PERCENTILE_DISC(0.5) WITHIN GROUP (ORDER BY amount)" + ) + var computable, refused, notExercised = 0 + val skipped = Seq.newBuilder[String] + spellings.foreach { agg => + val body = s"SELECT city, $agg AS m FROM customers GROUP BY city HAVING $agg > 1" + val oracle = Parser(body) match { + case Right(s: SingleSearch) => + s.having + .flatMap(_.criteria) + .map(_.extractAggregationFields) + .getOrElse(Seq.empty) + .flatMap(_.identifier.aggregateFunction) + .headOption + .map(_.toTransformAggregation.isDefined) + case _ => None + } + oracle match { + case None => + // Three outcomes, never two: an unparseable or non-aggregate spelling is REPORTED. + notExercised += 1 + skipped += agg + case Some(true) => + computable += 1 + withClue(s"[$agg] toTransformAggregation says computable, so the view must create: ") { + verdict(asView(body)) shouldBe Accepted + } + case Some(false) => + refused += 1 + withClue(s"[$agg] toTransformAggregation says None, so the view must refuse: ") { + refusalOf(asView(body)) should include(Rule.NoTransformAgg) + } + } + } + info(s"computable=$computable refused=$refused notExercised=$notExercised ${skipped.result()}") + // Both outcomes must actually occur, or the oracle proved nothing. + computable should be > 0 + refused should be > 0 + } + + it should "leave no AggregateFunction leaf silently unconsidered" in { + // Completeness, walked off the SEALED hierarchy rather than a hand list. Leaves the spellings + // above do not reach are REPORTED with a count -- the third outcome that otherwise becomes + // "fine" by default. Window ranking functions legitimately live here and are not aggregates a + // pivot ever computes. + import scala.reflect.runtime.{universe => ru} + def leaves(sym: ru.ClassSymbol): Set[ru.ClassSymbol] = { + val subs = sym.knownDirectSubclasses.map(_.asClass) + if (subs.isEmpty) Set(sym) else subs.flatMap(leaves) + } + val names = + leaves( + ru.typeOf[app.softnetwork.elastic.sql.function.aggregate.AggregateFunction] + .typeSymbol + .asClass + ) + .map(_.name.toString) + names.size should be > 5 + info(s"AggregateFunction leaves (${names.size}): ${names.toSeq.sorted.mkString(", ")}") + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // The MV-only separation, at the MECHANISM the emission reads (AC-2, RENDER level). + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "a key condition refused in a view" should "still reach the terms include/exclude channel in a search" in { + // `Expression.includes` / `excludes` IS what the bridge calls to build the `terms` filter -- + // the same function `SingleSearch.keyExpressibleByTerms` asks, never a copy of its rules. + val empty = BucketIncludesExcludes() + Seq( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'" -> Set( + "Paris" + ), + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city IN ('Paris','Lyon')" -> Set( + "Paris", + "Lyon" + ), + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris' OR city = 'Lyon'" -> Set( + "Paris", + "Lyon" + ) + ).foreach { case (sql, expected) => + withClue(s"[$sql] ") { + val s = parsedSearch(sql) + val bucket = s.buckets.headOption.getOrElse(fail("no bucket")) + val criteria = s.having.flatMap(_.criteria).getOrElse(fail("no HAVING")) + criteria.includes(bucket, not = false, empty).values shouldBe expected + } + } + val excluded = parsedSearch( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city <> 'Paris'" + ) + val bucket = excluded.buckets.headOption.getOrElse(fail("no bucket")) + excluded.having + .flatMap(_.criteria) + .getOrElse(fail("no HAVING")) + .excludes(bucket, not = false, empty) + .values shouldBe Set("Paris") + } + + "an unpublished aggregate refused in a view" should "still become an auxiliary aggregation in a search" in { + // A search CREATES the aggregation the HAVING needs; a transform does not. That asymmetry is + // the whole reason the rule is MV-only, so it is asserted rather than described. + val s = parsedSearch( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1" + ) + s.havingOnlyAggs.map(_.identifier.sql) shouldBe Seq("MAX(amount)") + s.sqlAggregations.keys.toSeq should contain("max_amount") + } + + "a whole-table HAVING refused in a view" should "still be evaluated by a search" in { + parsedSearch( + "SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1" + ).wholeTableHaving shouldBe true + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // Message contract and rule precedence. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "each refusal" should "name the clause, the reason and a remedy" in { + val key = refusalOf( + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'") + ) + key should include("MATERIALIZED VIEW") + key should include("city = 'Paris'") + key should include("include") + key should include("WHERE") + + val noGroupBy = refusalOf(asView("SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1")) + noGroupBy should include("MATERIALIZED VIEW") + noGroupBy should include("no GROUP BY") + noGroupBy should include("Add a GROUP BY") + + val unpublished = refusalOf( + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1") + ) + unpublished should include("MATERIALIZED VIEW") + unpublished should include("MAX(amount)") + unpublished should include("SELECT list") + } + + it should "carry no internal work-item identifier" in { + Seq( + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'"), + asView("SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 1"), + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1") + ).foreach { sql => + withClue(s"[$sql] ") { + val reason = refusalOf(sql) + reason should not include "#" + reason.toLowerCase should not include "story" + } + } + } + + "the no-GROUP-BY rule" should "win over every rule that needs a pivot" in { + // The structural reason is the more useful one, and the precedence is pinned so it cannot + // drift silently. Two rows, because a single assertion is the whole guard otherwise. + refusalOf(asView("SELECT COUNT(*) AS c FROM customers HAVING MAX(amount) > 1")) should + include(Rule.NoGroupBy) + refusalOf(asView("SELECT STDDEV(amount) AS sd FROM customers HAVING STDDEV(amount) > 1")) should + include(Rule.NoGroupBy) + } + + "the grouping-key rule" should "win over the aggregate rules" in { + refusalOf( + asView( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris' AND MAX(amount) > 1" + ) + ) should include(Rule.GroupKey) + refusalOf( + asView( + "SELECT city, STDDEV(amount) AS sd FROM customers GROUP BY city " + + "HAVING city = 'Paris' AND STDDEV(amount) > 1" + ) + ) should include(Rule.GroupKey) + } + + "the unbuildable-aggregate rule" should "win over the unpublished-aggregate rule" in { + // πŸ”΄ Order matters for a REASON, not for taste: the unpublished rule's remedy is "add it to + // the SELECT list", and for an aggregate no transform can compute that remedy DOES NOT WORK -- + // MEASURED, adding it produces the buckets_path-names-a-missing-aggregation state that the + // unbuildable rule refuses. A refusal whose remedy leads somewhere worse is the #389 trap. + val notInSelect = + asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING STDDEV(amount) > 1") + refusalOf(notInSelect) should include(Rule.NoTransformAgg) + refusalOf(notInSelect) should not include Rule.Unpublished + // and the remedy the OTHER rule would have prescribed is itself refused, which is the proof + // that the ordering is load-bearing rather than cosmetic. + refusalOf( + asView( + "SELECT city, COUNT(*) AS c, STDDEV(amount) AS sd FROM customers GROUP BY city " + + "HAVING STDDEV(amount) > 1" + ) + ) should include(Rule.NoTransformAgg) + } + + "the two-aggregate rule" should "win over the unpublished-aggregate rule" in { + // πŸ”΄ Same reason as rule 3's precedence, one operand over: rule 5's remedy is "add it to the + // SELECT list", and MEASURED, applying it to this statement produces R1 -- a selector that + // reads an undeclared parameter and rejects every group. A refusal whose remedy leads + // somewhere worse is a defect, not a nicety. + val reason = refusalOf( + asView( + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)" + ) + ) + reason should include(Rule.TwoAggregates) + reason should not include Rule.Unpublished + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // Each message must name what actually fired. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "the grouping-key message" should "not call a window PARTITION BY key a GROUP BY key" in { + // `SingleSearch.buckets` is `bucketTree.allBuckets`, which carries window PARTITION BY keys + // too, so this reaches the key arm with `status` -- which is NOT a GROUP BY key. The refusal + // is right (a transform expresses neither channel); the noun had to stop being wrong. + val reason = refusalOf( + asView( + "SELECT city, COUNT(*) AS c, MAX(amount) OVER (PARTITION BY status) AS mo " + + "FROM customers GROUP BY city HAVING status = 'a'" + ) + ) + reason should include("grouping key") + reason should not include "GROUP BY key" + } + + "the unpublished-aggregate message" should "not tell the user to add something already there" in { + // `SELECT COUNT(id) AS c … HAVING COUNT(`id`)` really does build a second metric, so the + // refusal is right -- but a bare "add COUNT(\"id\") to the SELECT list" reads as nonsense + // when COUNT(id) is visibly in the SELECT list. The message has to say WHY. + refusalOf( + asView( + "SELECT `category`, COUNT(id) AS c FROM customers GROUP BY `category` HAVING COUNT(`id`) > 1" + ) + ) should include("spelling the aggregate exactly as the HAVING spells it") + } + + /** πŸ”΄ A remedy must not steer the user into another refusal. Three rounds in a row a remedy WAS + * the defect, so each one is now applied LITERALLY here and its result asserted. + * + * The trap this closes: every remedy that puts an aggregate in the SELECT list has to say `AS + * `, because a view's HAVING can only read an aggregate that carries a SELECT alias + * -- and a HAVING never spells an alias, so "spell it exactly as the HAVING spells it" was, on + * its own, the instruction that produced the broken artefact. + */ + "every remedy" should "name the SELECT alias, and be accepted when applied literally" in { + Seq( + "R1 add a GROUP BY" -> + ("SELECT COUNT(*) AS c FROM customers HAVING COUNT(*) > 5", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 5"), + "R2 filter the key in WHERE" -> + ("SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING city = 'Paris'", + "SELECT city, SUM(amount) AS s FROM customers WHERE city = 'Paris' GROUP BY city HAVING SUM(amount) > 5"), + "R5 compare with a constant" -> + ("SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > 5"), + "R6 add it to the SELECT list" -> + ("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MIN(qty) > 5", + "SELECT city, COUNT(*) AS c, MIN(qty) AS mq FROM customers GROUP BY city HAVING MIN(qty) > 5") + ).foreach { case (label, (refusedBody, remedyApplied)) => + withClue(s"[$label] the refusal must prescribe a SELECT alias: ") { + // The PROPERTY, not one spelling: the message tells the user to write `AS ` + // and says the word alias. Both halves matter -- an `AS` with no explanation reads as + // noise, and "alias" with no `AS` does not show what to type. + val reason = refusalOf(asView(refusedBody)) + // πŸ”΄ `include("alias")` alone is satisfied by the `AS ` PLACEHOLDER itself, so a + // mutation that deletes the explanation stayed green. Assert the EXPLANATION: the message + // has to say that a view's HAVING can only read an aliased aggregate. + reason should include("AS ") + reason should include("SELECT alias") + } + withClue(s"[$label] the remedy applied literally must be ACCEPTED: ") { + verdict(asView(remedyApplied)) shouldBe Accepted + } + } + } + + "the backstop message" should "teach the alias rule and name the relation case" in { + // πŸ”΄ Same weakness M14 exposed one rule over: the population keys on "matches none of the + // aggregations this view creates", which survives deleting everything that TEACHES. The + // backstop is the message most users will see for the commonest mistake (no SELECT alias), + // so its remedy is asserted, not just its diagnosis. + val noAlias = refusalOf( + asView("SELECT city, SUM(amount) FROM customers GROUP BY city HAVING SUM(amount) > 5") + ) + noAlias should include("SELECT list with an alias") + noAlias should include("SUM(x) AS s") + val relation = refusalOf( + asView( + "SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING s > 5 AND child(c.x = 1)" + ) + ) + relation should include("nested, child or parent predicate") + } + + it should "refuse the SAME remedy applied WITHOUT an alias" in { + // The paired negative: drop the `AS` the remedy prescribes and the statement is refused by the + // backstop rather than silently deployed. This is what makes the alias clause load-bearing. + Seq( + "SELECT city, COUNT(*) FROM customers GROUP BY city HAVING COUNT(*) > 5", + "SELECT city, SUM(amount) FROM customers WHERE city = 'Paris' GROUP BY city HAVING SUM(amount) > 5", + "SELECT city, MAX(amount) FROM customers GROUP BY city HAVING MAX(amount) > 5", + "SELECT city, COUNT(*) AS c, MIN(qty) FROM customers GROUP BY city HAVING MIN(qty) > 5" + ).foreach { body => + withClue(s"[$body] ") { refusalOf(asView(body)) should include(Rule.NoMatchingAgg) } + withClue(s"[$body] still correct as a plain SELECT: ") { verdict(body) shouldBe Accepted } + } + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // The existing three arms keep their precedence, and the existing HAVING rules keep theirs. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "the set-operation, derived-table and WHERE-subquery arms" should "still win" in { + refusalOf( + "CREATE MATERIALIZED VIEW mv_probe AS SELECT city FROM customers UNION ALL SELECT city FROM prospects" + ) should include("set operation") + refusalOf( + "CREATE MATERIALIZED VIEW mv_probe AS SELECT d.city FROM (SELECT city FROM customers) AS d" + ) should include("derived table") + refusalOf( + "CREATE MATERIALIZED VIEW mv_probe AS SELECT city FROM customers WHERE id IN (SELECT customer_id FROM orders)" + ) should include("WHERE subquery") + } + + /** πŸ”΄ The reason there is no fourth arm relaying `Having.unrepresentable`. + * + * `SingleSearch.validate()` already refuses every un-expressible HAVING, and it runs inside + * `dql.validate()`, i.e. BEFORE the materialized-view rules. An MV arm reading `unrepresentable` + * would therefore be unsatisfiable for every possible input -- dead code beside live code. These + * rows are what makes that claim falsifiable: if the shared rule ever stopped firing, they would + * report the MV message (or an acceptance) instead. + */ + "an un-expressible HAVING" should "be refused by the shared rule, not by a view-specific one" in { + Seq( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING ROUND(COUNT(*), 2) > 1", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING ABS(COUNT(*)) > 1", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING NULLIF(COUNT(*), 0) > 1" + ).foreach { body => + withClue(s"[$body] ") { + val viewReason = refusalOf(asView(body)) + viewReason should include("HAVING cannot be applied to") + viewReason should not include "MATERIALIZED VIEW" + // The SAME reason in both venues: the rule is shared, not duplicated. + refusalOf(body) shouldBe viewReason + } + } + } + + /** πŸ”΄ Why the `isAggregation` guard on `havingValueSideAggs` has a GREEN mutation. + * + * A value side that is NOT an aggregate never reaches the view rules: the shared rules inside + * `dql.validate()` refuse it first. These rows pin THAT, which is what makes the guard dead from + * SQL -- if a shared rule ever stopped firing, the guard would become live and its mutation + * would start to matter. Recording the reason beats leaving a green cell unexplained. + */ + "a non-aggregate on the value side" should "be refused by the shared rules, before the view rules" in { + Seq( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > amount" -> + "HAVING cannot be applied to", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > city" -> + "HAVING cannot be applied to", + "SELECT city, status, COUNT(*) AS c FROM customers GROUP BY city, status HAVING city = status" -> + "HAVING cannot filter on" + ).foreach { case (body, sharedRule) => + withClue(s"[$body] ") { + val reason = refusalOf(asView(body)) + reason should include(sharedRule) + reason should not include "MATERIALIZED VIEW" + refusalOf(body) shouldBe reason + } + } + // …and a SELECT alias of an aggregate IS substituted before the guard sees it, so the guard + // admits it as the aggregate it is rather than excluding it. + refusalOf( + asView( + "SELECT city, COUNT(*) AS c, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > c" + ) + ) should include(Rule.TwoAggregates) + } + + "the shared HAVING rules" should "still fire inside a view, unchanged" in { + Seq( + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING UPPER(city) = 'PARIS'" -> + "HAVING cannot filter on", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING amount > 1" -> + "HAVING can only filter on", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1 OR city = 'Paris'" -> + "HAVING cannot OR conditions", + "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris' AND city = 'Lyon'" -> + "HAVING cannot combine the conditions" + ).foreach { case (body, expected) => + withClue(s"[$body] ") { + refusalOf(asView(body)) should include(expected) + refusalOf(body) should include(expected) + } + } + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // The refusal reaches every CREATE spelling. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "the refusal" should "reach OR REPLACE, IF NOT EXISTS, REFRESH EVERY and WITH options" in { + val body = "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'" + Seq( + s"CREATE OR REPLACE MATERIALIZED VIEW mv_probe AS $body", + s"CREATE MATERIALIZED VIEW IF NOT EXISTS mv_probe AS $body", + s"CREATE MATERIALIZED VIEW mv_probe REFRESH EVERY 60 SECONDS AS $body", + s"CREATE MATERIALIZED VIEW mv_probe WITH (delay = '1s') AS $body" + ).foreach { sql => + withClue(s"[$sql] ") { refusalOf(sql) should include(Rule.GroupKey) } + } + } + + "a view that still creates" should "keep every CREATE spelling too" in { + val body = "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1" + Seq( + s"CREATE OR REPLACE MATERIALIZED VIEW mv_probe AS $body", + s"CREATE MATERIALIZED VIEW IF NOT EXISTS mv_probe AS $body", + s"CREATE MATERIALIZED VIEW mv_probe REFRESH EVERY 60 SECONDS AS $body" + ).foreach { sql => withClue(s"[$sql] ") { verdict(sql) shouldBe Accepted } } + } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // The rules are MV-only by construction: no OTHER statement kind inherits them. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + "a CREATE TABLE AS SELECT over the same body" should "not inherit the view rules" in { + // CTAS renders through the ordinary search path, which HAS the include/exclude channel. + verdict( + "CREATE TABLE t2 AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'" + ) shouldBe Accepted + } +}