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 + } +}