diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala new file mode 100644 index 00000000..6fa427d3 --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -0,0 +1,507 @@ +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query._ +import com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime +import scala.jdk.CollectionConverters._ + +/** Issue #389 -- the EMITTED Elasticsearch query for a `HAVING` over a FUNCTION of an aggregate. + * + * The `sql` half (detection, the representability gate, the refusals) is pinned in + * `HavingOverAggregateFunctionSpec`. What is measured HERE is what Elasticsearch is actually sent: + * the `bucket_selector`, its `buckets_path`, and the metric aggregation the script reads. Those + * are the three things that used to disagree -- the defect was invisible to a parse, because every + * one of these statements parsed, ran, and returned EVERY group with HTTP 200. + * + * šŸ”“ Every shape named in `emitted` below was EXECUTED against a real Elasticsearch 8.18.3 before + * it was written here, and every shape in `refused` was measured to FAIL there. That is not + * ceremony: the spec asserted that `Double.valueOf(Math.abs(params.c)) > 1` was a legal + * `bucket_selector` source, and Elasticsearch rejects it at COMPILE time. A byte pin would have + * certified it (`feedback_assert_the_mechanism_not_a_proxy` -- PARSES != RENDERS != RUNS). + * + * The file closes with a structural invariant over every shape it names: a `bucket_selector` may + * read no `params.` its own `buckets_path` does not declare, and must null-test every metric it + * declares. `metricSelectorForBucket`'s own comment asserts the first half; it is now load-bearing + * for a new population, so it is asserted rather than assumed. + */ +class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { + + import scala.language.implicitConversions + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + implicit def sqlQueryToRequest(sqlQuery: SelectStatement): ElasticSearchRequest = + sqlQuery.statement match { + case Some(value: SingleSearch) => value.copy(score = sqlQuery.score) + case other => throw new IllegalArgumentException(s"Not a single search: $other") + } + + private val mapper = new ObjectMapper() + + private def queryOf(sql: String): String = { + val select: ElasticSearchRequest = SelectStatement(sql) + select.query + } + + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { + val here = Option(node.get(key)).toSeq + val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) + here ++ below + } + + private val terms = """"terms":{"field":"status","size":65536,"min_doc_count":1}""" + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " + + // --------------------------------------------------------------------------------------------- + // The matrix is DERIVED, not enumerated + // + // šŸ”“ The first version of this file carried two hand-written literal lists, and BOTH excluded the + // whole family in which the left operand is a bare aggregate and the RIGHT one carries the + // function -- which is exactly the family that emitted four HTTP-400 scripts and one + // `Internal parser error`. A count over a hand-written list is a literal asserting itself + // (`feedback_assert_the_mechanism_not_a_proxy`): the population has to be derived, and every cell + // has to get a VERDICT. + // --------------------------------------------------------------------------------------------- + + private val wrappers: Seq[String => String] = Seq( + a => s"COALESCE($a, 0)", + a => s"GREATEST($a, 0)", + a => s"LEAST($a, 99)", + a => s"SIGN($a)", + a => s"ABS($a)", + a => s"FLOOR($a)", + a => s"ROUND($a, 2)", + a => s"NULLIF($a, 0)", + a => s"CASE WHEN $a > 1 THEN 1 ELSE 0 END" + ) + + private val aggregates = Seq("COUNT(*)", "MAX(amount)", "SUM(amount)") + + /** Every operand POSITION a wrapped aggregate can occupy. The second and third are the ones the + * hand-written lists missed. + */ + private val positions: Seq[String => String] = Seq( + w => s"$w > 1", + w => s"1 < $w", + w => s"COUNT(*) > $w", + w => s"NOT $w > 1", + w => s"$w BETWEEN 1 AND 5", + w => s"$w IN (1, 2)", + w => s"COUNT(*) > 1 AND $w > 2", + w => s"COUNT(*) > 1 OR $w > 2" + ) + + private val matrix: Seq[String] = + for { + wrap <- wrappers + agg <- aggregates + pos <- positions + } yield group + pos(wrap(agg)) + + "every cell of the derived matrix" should "either emit a filter or be refused by name -- never both, never neither" in { + // šŸ”“ The verdict, not the shape. On `main` 149 of the 247 statements of this family answered + // HTTP 200 with the predicate SILENTLY DROPPED; the contract is that NONE does. + val silent = matrix.filter { sql => + Parser(sql) match { + case Left(_) => false + case Right(_) => + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").isEmpty + } + } + withClue( + s"${silent.size} statements emit NO filter and no refusal:\n${silent.take(8).mkString("\n")}\n" + )( + silent shouldBe empty + ) + } + + it should "name the HAVING clause in every refusal" in { + val refusals = matrix.flatMap(sql => Parser(sql).left.toOption.map(sql -> _.msg)) + // Non-vacuity computed over the material: the matrix MUST contain refusals, or the assertion + // above could pass on an all-emitting set. + refusals.size should be >= 100 + refusals.foreach { case (sql, msg) => + withClue(s"[$sql] ") { + msg should startWith("HAVING cannot") + msg should not startWith Parser.InternalParseFailure + } + } + } + + it should "cover both operand sides, so the right-hand family cannot go missing again" in { + // The guard on the guard: if `positions` ever loses the right-operand rows, this reddens. + val rightHand = matrix.filter(_.contains("COUNT(*) > COALESCE")) + rightHand should not be empty + val bounds = matrix.filter(_.contains("BETWEEN 1 AND 5")) + bounds should not be empty + } + + /** The shapes whose EXACT emission is pinned below, each executed against a real cluster. The + * structural invariants at the bottom run over these AND over the derived matrix above. + */ + private val emitted: Seq[String] = Seq( + group + "COUNT(*) > 1", + group + "COALESCE(COUNT(*), 0) > 1", + group + "GREATEST(COUNT(*), 0) > 1", + group + "LEAST(COUNT(*), 99) > 1", + group + "SIGN(COUNT(*)) > 0", + group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2", + group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5", + group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5", + group + "GREATEST(COUNT(*), 0) IN (1, 2)", + group + "NOT GREATEST(COUNT(*), 0) > 1", + group + "1 < GREATEST(COUNT(*), 0)", + group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'", + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)", + "SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING COALESCE(COUNT(*), 0) > 1", + "SELECT status, MAX(x) - MIN(x) AS d FROM t GROUP BY status HAVING d > 3", + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) + + // --------------------------------------------------------------------------------------------- + // S1 / S2 -- the shapes the engine CAN express now emit a selector (AC-1) + // --------------------------------------------------------------------------------------------- + + "COALESCE over an aggregate" should "emit a bucket_selector reading the published metric" in { + queryOf(group + "COALESCE(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c != null ? params.c : 0) > 1"}}}}}}}""" + ).mkString + } + + "GREATEST over an aggregate" should "emit a bucket_selector, null-guarded" in { + queryOf(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // The GROUP BY key population -- BUCKET level only (round 3) + // + // šŸ”“ The round-2 document push-down is GONE. Its licence ("a function of the key is constant + // within a bucket") is false when the KEY is a function of the column and the predicate reads the + // column, and false again on a MULTI-VALUED field where one document belongs to several buckets. + // Both were measured as HTTP 200 wrong answers on Elasticsearch 8.18.3. + // --------------------------------------------------------------------------------------------- + + "a key predicate the terms filter expresses" should "stay in the terms filter and touch nothing else" in { + val q = queryOf(group + "status = 'a'") + q should include(""""include":["a"]""") + // šŸ”“ No query filter: the round-2 push-down put one here, and that is what made a multi-valued + // grouping wrong. + q should include(""""query":{"match_all":{}}""") + q should not include "having_filter" + } + + it should "leave the WHERE clause exactly as written" in { + val q = queryOf( + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status HAVING status = 'a'" + ) + q should include(""""include":["a"]""") + q should not include "toUpperCase" + } + + "a key predicate the terms filter cannot express" should "never reach emission at all" in { + Seq("UPPER(status) = 'A'", "LENGTH(status) = 1").foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + } + + // --------------------------------------------------------------------------------------------- + // The include/exclude CHANNEL -- it unions, it never intersects (rule b2) + // + // šŸ”“ `excludes` IS `includes(bucket, !not, ā€¦)`. Every assertion above this point is include-side, + // which is how an inverted rule once shipped: it reasoned about the include sense and was applied + // by a method that is also the exclude sense. These cells are the EXCLUDE side, derived. + // --------------------------------------------------------------------------------------------- + + "an AND of inequalities" should "union into ONE exclude list, exactly as on 455433ae" in { + queryOf(group + "status <> 'a' AND status <> 'b'") should include( + """"exclude":["a","b"]""" + ) + queryOf(group + "status NOT IN ('a','b') AND status <> 'c'") should include( + """"exclude":["a","b","c"]""" + ) + } + + it should "keep working beside a metric, which is a different mechanism" in { + val q = queryOf(group + "status <> 'a' AND COUNT(*) > 1") + q should include(""""exclude":["a"]""") + q should include("bucket_selector") + } + + "a combination the channel cannot express" should "never reach emission" in { + // Each of these emitted a SILENT WRONG ANSWER on `455433ae` -- measured: + // `<> a OR <> b` -> exclude:["a","b"], though the disjunction is true for EVERY bucket + // `= a AND = b` -> include:["a","b"], though the SQL means NO bucket + // `= a OR <> b` -> include:["a"] AND exclude:["b"], a conjunction where SQL says OR + // `LIKE a% AND LIKE b%` -> include:"a.*" only, the second pattern dropped + Seq( + "status <> 'a' OR status <> 'b'", + "status NOT IN ('a','b') OR status <> 'c'", + "status = 'a' AND status = 'b'", + "status = 'a' OR status <> 'b'", + "status LIKE 'a%' AND status LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + } + + it should "still allow one include and one exclude under a conjunction" in { + val q = queryOf(group + "status IN ('a','b') AND status <> 'c'") + q should include(""""include":["a","b"]""") + q should include(""""exclude":["c"]""") + } + + it should "refuse a pattern meeting a value list in the SAME channel" in { + // šŸ”“ MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: `= 'a' OR LIKE 'b%'` emitted + // `include:"b.*"` and returned ['b1'] where the SQL means ['a','b1']. The emission keeps the + // PATTERN and discards the value list, and a second pattern is lost to `orElse`. + Seq( + "status = 'a' OR status LIKE 'b%'", + "status LIKE 'a%' OR status LIKE 'b%'", + "status <> 'a' AND status NOT LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should include("a pattern replaces the list") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + // ... while a pattern in EACH channel is fine: they are different lists. + val q = queryOf(group + "status LIKE 'a%' AND status NOT LIKE 'b%'") + q should include(""""include":"a.*"""") + q should include(""""exclude":"b.*"""") + } + + "a LIKE pattern on the key" should "use the SHARED translation" in { + // šŸ”“ The terms channel had a THIRD private LIKE -> regex translation that neither handled `_` + // nor escaped a metacharacter, while the query-DSL path used the shared `toRegex`. MEASURED + // on ES 8.18.3: `LIKE 'a_'` matched NO buckets where the WHERE form matched two, and + // `LIKE 'a.b%'` matched `axbZ` as well as `a.bZ`. + queryOf(group + "status LIKE 'a_'") should include(""""include":"a."""") + queryOf(group + "status LIKE 'a.b%'") should include(""""include":"a\\.b.*"""") + // ... RLIKE is RAW regex by definition and must NOT be translated. + queryOf(group + "status RLIKE 'a.b'") should include(""""include":"a.b"""") + // ... and a pattern with neither is byte-identical to `455433ae`. + queryOf(group + "status LIKE 'a%'") should include(""""include":"a.*"""") + } + + "an OR across TWO grouping levels" should "never reach emission" in { + // šŸ”“ MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): Elasticsearch NESTS the two `terms` + // aggregations, so `status = 'a' OR city = 'b'` emitted + // `terms status include:["a"] > terms city include:["b"]` and returned ONE group where the + // SQL means THREE. The AND is exactly what the nesting means, and still emits. + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + Parser(two + "status = 'a' OR city = 'b'") match { + case Left(e) => e.msg should include("DIFFERENT GROUP BY keys") + case Right(_) => fail(s"accepted; it emitted ${queryOf(two + "status = 'a' OR city = 'b'")}") + } + val q = queryOf(two + "status = 'a' AND city = 'b'") + q should include(""""include":["a"]""") + q should include(""""include":["b"]""") + } + + // --------------------------------------------------------------------------------------------- + // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. + // --------------------------------------------------------------------------------------------- + + "a conjunction of a bare and a wrapped aggregate" should "emit both conditions" in { + queryOf(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") should include( + """"script":{"source":"(params.c == null ? false : (params.c > 1)) && """ + + """(params.c == null ? false : (Math.max(params.c, 0) > 2))"}""" + ) + } + + "a disjunction" should "emit both conditions" in { + // Dropping a disjunct makes the filter STRICTER than written: rows disappear silently. + queryOf(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") should include( + """(params.c == null ? false : (Math.max(params.c, 0) > 1)) || """ + + """(params.c == null ? false : (params.c > 5))""" + ) + } + + "a wrapped aggregate beside a bucket-key predicate" should "honour BOTH mechanisms" in { + // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may + // cost the other. + val q = queryOf(group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'") + q should include(""""exclude":["x"]""") + q should include("""(params.c != null ? params.c : 0) > 1""") + } + + // --------------------------------------------------------------------------------------------- + // S9 -- the aggregation reached ONLY through the HAVING function must be CREATED (AC-5) + // --------------------------------------------------------------------------------------------- + + "an aggregate reached only through a HAVING function" should "create its aggregation" in { + queryOf("SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"count_all":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_all":"count_all"},""", + """"script":{"source":"(params.count_all == null ? false : """, + """(Math.max(params.count_all, 0) > 1))"}}}}}}}""" + ).mkString + } + + it should "create it inside a NESTED relation too" in { + // šŸ”“ MEASURED on `main`: this statement emitted `{"query":{"match_all":{}},"_source":true}` -- + // no aggregation AT ALL. The GROUP BY itself vanished and raw documents came back. + queryOf( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.name":{"terms":{"field":"emails.name","size":65536,"min_doc_count":1},""", + """"aggs":{"count_e_address":{"value_count":{"field":"emails.address"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_e_address":"count_e_address"},""", + """"script":{"source":"(params.count_e_address == null ? false : """, + """(Math.max(params.count_e_address, 0) > 1))"}}}}}}}}}""" + ).mkString + } + + "an aggregate reached only through the RIGHT operand" should "create and guard its aggregation" in { + // šŸ”“ MEASURED on `main`: the script read `params.max_x`, `buckets_path` declared only `c`, and + // no `max_x` aggregation existed -- so Elasticsearch ran `Math.abs(null)`. + queryOf( + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c","max_x":"max_x"},""", + """"script":{"source":"(params.c == null || params.max_x == null ? false : """, + """(params.c > Math.max(params.max_x, 0)))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // S8 -- the whole-table HAVING follows the same rule (AC-4) + // --------------------------------------------------------------------------------------------- + + "a whole-table HAVING over a function of an aggregate" should "hang the selector off the synthetic bucket" in { + queryOf("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,""", + """"aggs":{"__whole_table_having__":{"filters":{"filters":{"_all":{"match_all":{}}}},""", + """"aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // AC-2 -- nothing in the matrix emits a query with the predicate missing + // --------------------------------------------------------------------------------------------- + + private val refused: Seq[String] = Seq( + group + "ABS(COUNT(*)) > 1", + group + "FLOOR(COUNT(*)) > 1", + group + "NULLIF(COUNT(*), 0) > 1", + group + "NULLIF(c, 0) > 1", + group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1", + group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1", + group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2", + "SELECT status, MAX(amount) AS m FROM t GROUP BY status HAVING ROUND(MAX(amount), 2) > 1", + "SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1" + ) + + "a HAVING the engine cannot express" should "never reach emission" in { + refused.foreach { sql => + withClue(s"[$sql] ") { + Parser(sql) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(s) => + fail(s"expected a rejection; it emitted ${queryOf(sql)} from $s") + } + } + } + } + + "every statement that DOES emit" should "carry a having_filter" in { + // Non-vacuity: the invariant below would pass on an empty set, which is exactly the shape the + // defect produced. + emitted.foreach { sql => + withClue(s"[$sql] ") { + valuesOf(mapper.readTree(queryOf(sql)), "bucket_selector") should not be empty + } + } + } + + // --------------------------------------------------------------------------------------------- + // The structural invariant (AD-2 item 3) + // --------------------------------------------------------------------------------------------- + + "every emitted bucket pipeline" should "read exactly the metrics its buckets_path declares" in { + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") + pipelines should not be empty + pipelines.foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = Option(pipeline.get("script")) + .map { s => + if (s.isTextual) s.asText() else s.get("source").asText() + } + .getOrElse(fail("a bucket pipeline with no script")) + val read = paramRef.findAllMatchIn(source).map(_.group(1)).toSet - "__now__" + read shouldBe declared + } + } + } + } + + it should "null-test every metric a bucket_selector declares" in { + // The AC-4b contract, restated for the #389 population: a selector either guards the metric + // (`params.x == null ? false : ...`) or handles the null itself (`params.x != null ? ... : k`, + // which is what COALESCE means). Dereferencing it unguarded fails the whole search. + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = pipeline.get("script").get("source").asText() + declared.foreach { key => + withClue(s"metric '$key' in [$source] ") { + assert( + source.contains(s"params.$key == null") || source.contains(s"params.$key != null"), + "is never null-tested" + ) + } + } + } + } + } + } +} diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index 1232ec71..535c63c8 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -177,7 +177,14 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") val names = boundToPredicate.flatMap(_.findAllMatchIn(code).map(_.group(1))).toSet val namedUses = names.toSeq.map { n => - s"""(?\.not`, so + // `p.includePolarityOfRight(not)` -- the member that exists PRECISELY to consume this + // fold -- was invisible, and its file scored 0 uses / 0 classifications and passed + // VACUOUSLY. A gate that stops seeing the site it has just caught is worse than no gate. + // The named fold counts as a use AND, like `negated`, as a classification: calling it IS + // the fold. Replace that call with a raw `p.not` and the file reddens, which is the point. + (s"""(? classifications) Some( diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala index 5ecf72df..cd1334e5 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ScrollApi.scala @@ -282,8 +282,16 @@ trait ScrollApi extends ElasticClientHelpers with SchemaCacheTtlApi { scroll(multiple, config) case None => + // Issue #389 / F7 -- carry the parser's OWN reason. A whole family of SEMANTIC refusals + // reaches this branch, each naming a clause and a remedy; the generic sentence threw + // every one of them away. Source.failed( - new IllegalArgumentException("SQL query does not contain a valid search request") + new IllegalArgumentException( + (statement match { + case s: SelectStatement => s.parseError + case _ => None + }).getOrElse("SQL query does not contain a valid search request") + ) ) } diff --git a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala index 3020e3a2..28fa6f25 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala @@ -477,6 +477,20 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC // PUBLIC METHODS // ======================================================================== + /** The client's refusal for a statement that did not parse -- WITH the parser's own reason when + * there is one (issue #389 / F7). + * + * šŸ”“ `SelectStatement.statement` is an `Option`, so a rejection arriving through it used to be + * reported as the generic sentence below and the reason -- computed, formatted, and naming the + * clause and its remedy -- was thrown away. Tolerable while the rejections were syntax errors + * the user can see; a whole family of SEMANTIC refusals now lands here. + */ + private def invalidSearchRequest(statement: SearchStatement, query: String): String = + (statement match { + case s: SelectStatement => s.parseError + case _ => None + }).getOrElse(s"SQL query does not contain a valid search request\n$query") + /** Search for documents / aggregations matching the SQL query. * * @param statement @@ -502,7 +516,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC ) ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request\n$query", + message = invalidSearchRequest(statement, query), operation = Some("search") ) ) @@ -559,7 +573,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC ) ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request\n$query", + message = invalidSearchRequest(statement, query), operation = Some("search") ) ) @@ -870,7 +884,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC Future.successful( ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request: ${statement.sql}", + message = invalidSearchRequest(statement, statement.sql), operation = Some("searchAsync") ) ) @@ -919,7 +933,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC Future.successful( ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request: $query", + message = invalidSearchRequest(statement, query), operation = Some("searchAsync") ) ) @@ -1448,7 +1462,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers with SchemaC ) ElasticResult.failure( ElasticError( - message = s"SQL query does not contain a valid search request: ${sql.query}", + message = invalidSearchRequest(sql, sql.query), operation = Some("searchWithInnerHits") ) ) diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index d5a6ccb2..f8c72c60 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -778,8 +778,98 @@ ORDER BY COUNT(*) DESC; price_range > 10`); `BETWEEN`, `IN` and `NOT` apply to aggregates as to columns. - Rejected with an explicit error: arithmetic over aggregates written inline in `HAVING` (`HAVING MAX(price) - MIN(price) > 10` ā€” alias it in `SELECT` and reference the alias), an - aggregate function inside `WHERE` (use `HAVING`), and an alias that names one aggregate in - `SELECT` and a different one in `HAVING` / `ORDER BY`. + aggregate function inside `WHERE` (use `HAVING`, and this covers a wrapped one such as + `WHERE ABS(COUNT(*)) > 1`), and an alias that names one aggregate in `SELECT` and a different one + in `HAVING` / `ORDER BY`. +- A **function of an aggregate** in `HAVING` is applied to the group, or the statement is rejected + by name ā€” it is never ignored. `COALESCE`, `GREATEST`, `LEAST` and `SIGN` over an aggregate filter + the groups (`HAVING COALESCE(COUNT(*), 0) > 30`, `HAVING GREATEST(MAX(price), 0) > 100`), on + either side of the comparison and under `NOT`; `BETWEEN` and `IN` support them in the tested + POSITION (`GREATEST(COUNT(*), 0) BETWEEN 1 AND 5`) but not as a BOUND + (`COUNT(*) BETWEEN 1 AND ABS(MAX(price))` is refused). Everything the engine cannot evaluate as a + group filter is refused with the reason: + - a rendering that can be NULL ā€” `HAVING NULLIF(COUNT(*), 0) > 1`; + - a rendering that needs a local variable ā€” `HAVING ROUND(SUM(price), 2) > 10`; + - a rendering that boxes a number ā€” `HAVING ABS(COUNT(*)) > 1` and the rest of the numeric + function family (`FLOOR`, `CEIL`, `SQRT`, `EXP`, `LOG`, `POWER`). This is a deliberate + over-approximation: the boxing conversion compiles in *some* positions of a group-filter script + and not others, so the engine refuses it in all of them rather than guess. The same functions + work normally in `WHERE`, in the `SELECT` list and in `ORDER BY`; + - `CASE ... END` in `HAVING`, which needs a document and a group filter has none; + - a function applied to a `SELECT` aggregate **alias** ā€” `COUNT(*) AS c ... HAVING NULLIF(c, 0) > 1`; + - an aggregate computed outside a nested grouping ā€” `... JOIN UNNEST(t.emails) AS e GROUP BY + e.name HAVING COALESCE(MAX(amount), 0) > 1`, where no level of the aggregation can read the + metric. + + In every refused case the remedy is the same: **compare the aggregate itself** ā€” + `HAVING SUM(price) > 10` rather than `HAVING ROUND(SUM(price), 2) > 10` ā€” and apply the function + to the result outside the query. Aliasing the expression in `SELECT` does NOT help: a function of + an aggregate is not a valid `SELECT` item under a `GROUP BY` either + (`SELECT ABS(COUNT(*)) AS a ... GROUP BY city` is rejected as a non-aggregated field). That + differs from arithmetic over aggregates, which IS a valid `SELECT` item + (`MAX(price) - MIN(price) AS price_range`) and is the reason the rule above tells you to alias + THAT one. + +#### Comparing a DATE aggregate + +āš ļø **A comparison between a date aggregate and a date literal is refused**, function or not: +`HAVING MAX(created) > '2019-01-01'` and `HAVING MIN(created) < '2020-01-01'` are rejected at parse +time. A group filter reads every metric as a number ā€” a date as epoch milliseconds ā€” so the +generated comparison is text against a number and Elasticsearch fails the whole search with a +`class_cast_exception`. Compare in `WHERE` instead, or filter the result outside the query. + +### Conditions on the GROUP BY key + +A `HAVING` condition over the grouping key filters GROUPS, and it is applied by the `terms` filter ā€” +so it is correct for a multi-valued field, where one document belongs to several groups. + +```sql +SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city = 'Paris'; +SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city LIKE 'P%'; +SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city <> 'Lyon'; +``` + +- Supported: a direct comparison of the key ā€” `=`, `<>`, `IN`, `LIKE` / `RLIKE`. +- āš ļø **Which COMBINATIONS are supported follows from how Elasticsearch applies them.** The `terms` + filter carries one list of kept values and one list of removed values, and each is a UNION: + + | combination | supported | why | + |---|---|---| + | `city = 'Paris' OR city = 'Lyon'` | āœ… | the kept list is a union, i.e. a disjunction | + | `city <> 'Paris' AND city <> 'Lyon'` | āœ… | not-in-A and not-in-B is not-in-(A āˆŖ B) | + | `city = 'Paris' AND city <> 'Lyon'` | āœ… | one kept list and one removed list, applied together | + | `city LIKE 'P%' AND city NOT LIKE 'L%'` | āœ… | one pattern in each of the two lists | + | `city <> 'Paris' OR city <> 'Lyon'` | āŒ refused | a union of removals is a conjunction, so this would be executed as one | + | `city = 'Paris' AND city = 'Lyon'` | āŒ refused | a union of kept values is a disjunction, so this would be executed as one | + | `city = 'Paris' OR city <> 'Lyon'` | āŒ refused | the two lists are applied together, i.e. ANDed | + | `city = 'Paris' OR city LIKE 'L%'` | āŒ refused | āš ļø a pattern REPLACES the list ā€” see below | + | `city LIKE 'P%' OR city LIKE 'L%'` | āŒ refused | one list holds one pattern, so the second is lost | + + āš ļø **Being a union is necessary but not sufficient.** Each of the two lists holds either a set of + values or ONE pattern (`LIKE` / `RLIKE`), and a pattern replaces the set ā€” so a pattern meeting + anything else in the SAME list loses a side, even where the combination itself is a disjunction. + `HAVING city = 'Paris' OR city LIKE 'L%'` used to return only the `Lā€¦` groups. Use a single + `RLIKE` covering both alternatives, or split the query. + + The refused rows previously returned a plausible-looking but WRONG set of groups. Otherwise: + split the query, or restate the condition as an `OR` of equalities or an `AND` of inequalities. +- āš ļø **A FUNCTION of the key is refused** (`HAVING UPPER(city) = 'PARIS'`, + `HAVING LENGTH(status) = 1`). The terms filter can only express a direct comparison, and the + alternatives are unsound: filtering documents instead would keep or drop a multi-valued document + WHOLE, and would change the counts of surviving groups whenever the key is itself a function of + the column (`GROUP BY DAY(d) HAVING YEAR(d) = 2025`). Compare the key itself, or filter in + `WHERE`. +- A predicate naming a column that is **neither** the `GROUP BY` key **nor** an aggregate is refused + (`HAVING UPPER(name) = 'X'` when the grouping is by `city`) ā€” with or without a `GROUP BY`. +- āš ļø An `OR` whose branches need **different stages** is refused, because Elasticsearch applies + the stages one inside the other, which is a conjunction: + - different MECHANISMS ā€” a group filter (`bucket_selector`), a key filter (`terms`) and a nested + filter: `HAVING COUNT(*) > 1 OR city = 'Paris'`; + - different GROUPING KEYS ā€” the two `terms` aggregations are NESTED, so + `GROUP BY country, city HAVING country = 'FR' OR city = 'Paris'` would return only the groups + matching BOTH. An `OR` on ONE key, within one mechanism, is supported subject to the table + above; the corresponding `AND` is always fine, because the nesting IS the conjunction. +- An `AND` across mechanisms is fine ā€” each stage applies its own half. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter script null-checks every metric before comparing it. diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala new file mode 100644 index 00000000..1c10886f --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -0,0 +1,522 @@ +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query._ +import com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.ZonedDateTime +import scala.jdk.CollectionConverters._ + +/** Issue #389 -- the EMITTED Elasticsearch query for a `HAVING` over a FUNCTION of an aggregate, + * measured against the HAND-MAINTAINED es6 bridge (AC-7). + * + * šŸ”“ NO bridge file changed in round 3 ā€” the whole rule lives in `sql`. This twin is what PROVES + * it: the same derived matrix and the same pins against a different elastic4s major, with two + * expectations differing (elastic4s 6 renders a single-value `terms` `include` / `exclude` as a + * bare string). + * + * The `sql` half (detection, the representability gate, the refusals) is pinned in + * `HavingOverAggregateFunctionSpec`. What is measured HERE is what Elasticsearch is actually sent: + * the `bucket_selector`, its `buckets_path`, and the metric aggregation the script reads. Those + * are the three things that used to disagree -- the defect was invisible to a parse, because every + * one of these statements parsed, ran, and returned EVERY group with HTTP 200. + * + * šŸ”“ Every shape named in `emitted` below was EXECUTED against a real Elasticsearch 8.18.3 before + * it was written here, and every shape in `refused` was measured to FAIL there. That is not + * ceremony: the spec asserted that `Double.valueOf(Math.abs(params.c)) > 1` was a legal + * `bucket_selector` source, and Elasticsearch rejects it at COMPILE time. A byte pin would have + * certified it (`feedback_assert_the_mechanism_not_a_proxy` -- PARSES != RENDERS != RUNS). + * + * The file closes with a structural invariant over every shape it names: a `bucket_selector` may + * read no `params.` its own `buckets_path` does not declare, and must null-test every metric it + * declares. `metricSelectorForBucket`'s own comment asserts the first half; it is now load-bearing + * for a new population, so it is asserted rather than assumed. + */ +class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { + + import scala.language.implicitConversions + + implicit def timestamp: Long = ZonedDateTime.parse("2025-12-31T00:00:00Z").toInstant.toEpochMilli + + implicit def sqlQueryToRequest(sqlQuery: SelectStatement): ElasticSearchRequest = + sqlQuery.statement match { + case Some(value: SingleSearch) => value.copy(score = sqlQuery.score) + case other => throw new IllegalArgumentException(s"Not a single search: $other") + } + + private val mapper = new ObjectMapper() + + private def queryOf(sql: String): String = { + val select: ElasticSearchRequest = SelectStatement(sql) + select.query + } + + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + private def valuesOf(node: JsonNode, key: String): Seq[JsonNode] = { + val here = Option(node.get(key)).toSeq + val below = node.elements().asScala.toSeq.flatMap(valuesOf(_, key)) + here ++ below + } + + private val terms = """"terms":{"field":"status","size":65536,"min_doc_count":1}""" + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " + + // --------------------------------------------------------------------------------------------- + // The matrix is DERIVED, not enumerated + // + // šŸ”“ The first version of this file carried two hand-written literal lists, and BOTH excluded the + // whole family in which the left operand is a bare aggregate and the RIGHT one carries the + // function -- which is exactly the family that emitted four HTTP-400 scripts and one + // `Internal parser error`. A count over a hand-written list is a literal asserting itself + // (`feedback_assert_the_mechanism_not_a_proxy`): the population has to be derived, and every cell + // has to get a VERDICT. + // --------------------------------------------------------------------------------------------- + + private val wrappers: Seq[String => String] = Seq( + a => s"COALESCE($a, 0)", + a => s"GREATEST($a, 0)", + a => s"LEAST($a, 99)", + a => s"SIGN($a)", + a => s"ABS($a)", + a => s"FLOOR($a)", + a => s"ROUND($a, 2)", + a => s"NULLIF($a, 0)", + a => s"CASE WHEN $a > 1 THEN 1 ELSE 0 END" + ) + + private val aggregates = Seq("COUNT(*)", "MAX(amount)", "SUM(amount)") + + /** Every operand POSITION a wrapped aggregate can occupy. The second and third are the ones the + * hand-written lists missed. + */ + private val positions: Seq[String => String] = Seq( + w => s"$w > 1", + w => s"1 < $w", + w => s"COUNT(*) > $w", + w => s"NOT $w > 1", + w => s"$w BETWEEN 1 AND 5", + w => s"$w IN (1, 2)", + w => s"COUNT(*) > 1 AND $w > 2", + w => s"COUNT(*) > 1 OR $w > 2" + ) + + private val matrix: Seq[String] = + for { + wrap <- wrappers + agg <- aggregates + pos <- positions + } yield group + pos(wrap(agg)) + + "every cell of the derived matrix" should "either emit a filter or be refused by name -- never both, never neither" in { + // šŸ”“ The verdict, not the shape. On `main` 149 of the 247 statements of this family answered + // HTTP 200 with the predicate SILENTLY DROPPED; the contract is that NONE does. + val silent = matrix.filter { sql => + Parser(sql) match { + case Left(_) => false + case Right(_) => + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").isEmpty + } + } + withClue( + s"${silent.size} statements emit NO filter and no refusal:\n${silent.take(8).mkString("\n")}\n" + )( + silent shouldBe empty + ) + } + + it should "name the HAVING clause in every refusal" in { + val refusals = matrix.flatMap(sql => Parser(sql).left.toOption.map(sql -> _.msg)) + // Non-vacuity computed over the material: the matrix MUST contain refusals, or the assertion + // above could pass on an all-emitting set. + refusals.size should be >= 100 + refusals.foreach { case (sql, msg) => + withClue(s"[$sql] ") { + msg should startWith("HAVING cannot") + msg should not startWith Parser.InternalParseFailure + } + } + } + + it should "cover both operand sides, so the right-hand family cannot go missing again" in { + // The guard on the guard: if `positions` ever loses the right-operand rows, this reddens. + val rightHand = matrix.filter(_.contains("COUNT(*) > COALESCE")) + rightHand should not be empty + val bounds = matrix.filter(_.contains("BETWEEN 1 AND 5")) + bounds should not be empty + } + + /** The shapes whose EXACT emission is pinned below, each executed against a real cluster. The + * structural invariants at the bottom run over these AND over the derived matrix above. + */ + private val emitted: Seq[String] = Seq( + group + "COUNT(*) > 1", + group + "COALESCE(COUNT(*), 0) > 1", + group + "GREATEST(COUNT(*), 0) > 1", + group + "LEAST(COUNT(*), 99) > 1", + group + "SIGN(COUNT(*)) > 0", + group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2", + group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5", + group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5", + group + "GREATEST(COUNT(*), 0) IN (1, 2)", + group + "NOT GREATEST(COUNT(*), 0) > 1", + group + "1 < GREATEST(COUNT(*), 0)", + group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'", + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)", + "SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1", + "SELECT COUNT(*) AS c FROM t HAVING COALESCE(COUNT(*), 0) > 1", + "SELECT status, MAX(x) - MIN(x) AS d FROM t GROUP BY status HAVING d > 3", + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) + + // --------------------------------------------------------------------------------------------- + // S1 / S2 -- the shapes the engine CAN express now emit a selector (AC-1) + // --------------------------------------------------------------------------------------------- + + "COALESCE over an aggregate" should "emit a bucket_selector reading the published metric" in { + queryOf(group + "COALESCE(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c != null ? params.c : 0) > 1"}}}}}}}""" + ).mkString + } + + "GREATEST over an aggregate" should "emit a bucket_selector, null-guarded" in { + queryOf(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // The GROUP BY key population -- BUCKET level only (round 3) + // + // šŸ”“ The round-2 document push-down is GONE. Its licence ("a function of the key is constant + // within a bucket") is false when the KEY is a function of the column and the predicate reads the + // column, and false again on a MULTI-VALUED field where one document belongs to several buckets. + // Both were measured as HTTP 200 wrong answers on Elasticsearch 8.18.3. + // --------------------------------------------------------------------------------------------- + + "a key predicate the terms filter expresses" should "stay in the terms filter and touch nothing else" in { + val q = queryOf(group + "status = 'a'") + q should include(""""include":"a"""") + // šŸ”“ No query filter: the round-2 push-down put one here, and that is what made a multi-valued + // grouping wrong. + q should include(""""query":{"match_all":{}}""") + q should not include "having_filter" + } + + it should "leave the WHERE clause exactly as written" in { + val q = queryOf( + "SELECT status, COUNT(*) AS c FROM t WHERE amount > 1 GROUP BY status HAVING status = 'a'" + ) + q should include(""""include":"a"""") + q should not include "toUpperCase" + } + + "a key predicate the terms filter cannot express" should "never reach emission at all" in { + Seq("UPPER(status) = 'A'", "LENGTH(status) = 1").foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + } + + // --------------------------------------------------------------------------------------------- + // The include/exclude CHANNEL -- it unions, it never intersects (rule b2) + // + // šŸ”“ `excludes` IS `includes(bucket, !not, ā€¦)`. Every assertion above this point is include-side, + // which is how an inverted rule once shipped: it reasoned about the include sense and was applied + // by a method that is also the exclude sense. These cells are the EXCLUDE side, derived. + // --------------------------------------------------------------------------------------------- + + "an AND of inequalities" should "union into ONE exclude list, exactly as on 455433ae" in { + queryOf(group + "status <> 'a' AND status <> 'b'") should include( + """"exclude":["a","b"]""" + ) + queryOf(group + "status NOT IN ('a','b') AND status <> 'c'") should include( + """"exclude":["a","b","c"]""" + ) + } + + it should "keep working beside a metric, which is a different mechanism" in { + val q = queryOf(group + "status <> 'a' AND COUNT(*) > 1") + // āš ļø ES6 RENDERING, and it is a RESIDUAL, not a choice: this bridge renders a SINGLE-element + // value list as a bare string, which Elasticsearch 6.8 reads as a REGEX. It happens to select + // the right terms here (anchored, no metacharacters) but it is why a set-based include beside + // a single-valued exclude is rejected outright by 6.8 -- recorded, pre-existing, not fixed. + q should include(""""exclude":"a"""") + q should include("bucket_selector") + } + + "a combination the channel cannot express" should "never reach emission" in { + // Each of these emitted a SILENT WRONG ANSWER on `455433ae` -- measured: + // `<> a OR <> b` -> exclude:["a","b"], though the disjunction is true for EVERY bucket + // `= a AND = b` -> include:["a","b"], though the SQL means NO bucket + // `= a OR <> b` -> include:["a"] AND exclude:["b"], a conjunction where SQL says OR + // `LIKE a% AND LIKE b%` -> include:"a.*" only, the second pattern dropped + Seq( + "status <> 'a' OR status <> 'b'", + "status NOT IN ('a','b') OR status <> 'c'", + "status = 'a' AND status = 'b'", + "status = 'a' OR status <> 'b'", + "status LIKE 'a%' AND status LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + } + + it should "still allow one include and one exclude under a conjunction" in { + // Both lists MULTI-valued, so this bridge renders both as arrays -- see the residual note + // above; ES 6.8 rejects a set-based include beside a regex-rendered exclude. + val q = queryOf(group + "status IN ('a','b') AND status NOT IN ('c','d')") + q should include(""""include":["a","b"]""") + q should include(""""exclude":["c","d"]""") + } + + it should "refuse a pattern meeting a value list in the SAME channel" in { + // šŸ”“ MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: `= 'a' OR LIKE 'b%'` emitted + // `include:"b.*"` and returned ['b1'] where the SQL means ['a','b1']. The emission keeps the + // PATTERN and discards the value list, and a second pattern is lost to `orElse`. + Seq( + "status = 'a' OR status LIKE 'b%'", + "status LIKE 'a%' OR status LIKE 'b%'", + "status <> 'a' AND status NOT LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + Parser(group + p) match { + case Left(e) => e.msg should include("a pattern replaces the list") + case Right(_) => fail(s"accepted; it emitted ${queryOf(group + p)}") + } + } + } + // ... while a pattern in EACH channel is fine: they are different lists. + val q = queryOf(group + "status LIKE 'a%' AND status NOT LIKE 'b%'") + q should include(""""include":"a.*"""") + q should include(""""exclude":"b.*"""") + } + + "a LIKE pattern on the key" should "use the SHARED translation" in { + // šŸ”“ The terms channel had a THIRD private LIKE -> regex translation that neither handled `_` + // nor escaped a metacharacter, while the query-DSL path used the shared `toRegex`. MEASURED + // on ES 8.18.3: `LIKE 'a_'` matched NO buckets where the WHERE form matched two, and + // `LIKE 'a.b%'` matched `axbZ` as well as `a.bZ`. + queryOf(group + "status LIKE 'a_'") should include(""""include":"a."""") + queryOf(group + "status LIKE 'a.b%'") should include(""""include":"a\\.b.*"""") + // ... RLIKE is RAW regex by definition and must NOT be translated. + queryOf(group + "status RLIKE 'a.b'") should include(""""include":"a.b"""") + // ... and a pattern with neither is byte-identical to `455433ae`. + queryOf(group + "status LIKE 'a%'") should include(""""include":"a.*"""") + } + + "an OR across TWO grouping levels" should "never reach emission" in { + // šŸ”“ MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): Elasticsearch NESTS the two `terms` + // aggregations, so `status = 'a' OR city = 'b'` emitted + // `terms status include:["a"] > terms city include:["b"]` and returned ONE group where the + // SQL means THREE. The AND is exactly what the nesting means, and still emits. + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + Parser(two + "status = 'a' OR city = 'b'") match { + case Left(e) => e.msg should include("DIFFERENT GROUP BY keys") + case Right(_) => fail(s"accepted; it emitted ${queryOf(two + "status = 'a' OR city = 'b'")}") + } + val q = queryOf(two + "status = 'a' AND city = 'b'") + // āš ļø ES6 RENDERING: this bridge writes a SINGLE-element list as a bare string. See the + // residual note above -- recorded, pre-existing, byte-identical to `455433ae`. + q should include(""""include":"a"""") + q should include(""""include":"b"""") + } + + // --------------------------------------------------------------------------------------------- + // S7 -- a conjunction emits BOTH halves (AC-3). On `main` the second one VANISHED. + // --------------------------------------------------------------------------------------------- + + "a conjunction of a bare and a wrapped aggregate" should "emit both conditions" in { + queryOf(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") should include( + """"script":{"source":"(params.c == null ? false : (params.c > 1)) && """ + + """(params.c == null ? false : (Math.max(params.c, 0) > 2))"}""" + ) + } + + "a disjunction" should "emit both conditions" in { + // Dropping a disjunct makes the filter STRICTER than written: rows disappear silently. + queryOf(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") should include( + """(params.c == null ? false : (Math.max(params.c, 0) > 1)) || """ + + """(params.c == null ? false : (params.c > 5))""" + ) + } + + "a wrapped aggregate beside a bucket-key predicate" should "honour BOTH mechanisms" in { + // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may + // cost the other. + val q = queryOf(group + "COALESCE(COUNT(*), 0) > 1 AND status <> 'x'") + // elastic4s 6 renders a single-value `exclude` as a bare string, 7+ as an array. + q should include(""""exclude":"x"""") + q should include("""(params.c != null ? params.c : 0) > 1""") + } + + // --------------------------------------------------------------------------------------------- + // S9 -- the aggregation reached ONLY through the HAVING function must be CREATED (AC-5) + // --------------------------------------------------------------------------------------------- + + "an aggregate reached only through a HAVING function" should "create its aggregation" in { + queryOf("SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"count_all":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_all":"count_all"},""", + """"script":{"source":"(params.count_all == null ? false : """, + """(Math.max(params.count_all, 0) > 1))"}}}}}}}""" + ).mkString + } + + it should "create it inside a NESTED relation too" in { + // šŸ”“ MEASURED on `main`: this statement emitted `{"query":{"match_all":{}},"_source":true}` -- + // no aggregation AT ALL. The GROUP BY itself vanished and raw documents came back. + queryOf( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.name":{"terms":{"field":"emails.name","size":65536,"min_doc_count":1},""", + """"aggs":{"count_e_address":{"value_count":{"field":"emails.address"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_e_address":"count_e_address"},""", + """"script":{"source":"(params.count_e_address == null ? false : """, + """(Math.max(params.count_e_address, 0) > 1))"}}}}}}}}}""" + ).mkString + } + + "an aggregate reached only through the RIGHT operand" should "create and guard its aggregation" in { + // šŸ”“ MEASURED on `main`: the script read `params.max_x`, `buckets_path` declared only `c`, and + // no `max_x` aggregation existed -- so Elasticsearch ran `Math.abs(null)`. + queryOf( + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"_index"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c","max_x":"max_x"},""", + """"script":{"source":"(params.c == null || params.max_x == null ? false : """, + """(params.c > Math.max(params.max_x, 0)))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // S8 -- the whole-table HAVING follows the same rule (AC-4) + // --------------------------------------------------------------------------------------------- + + "a whole-table HAVING over a function of an aggregate" should "hang the selector off the synthetic bucket" in { + queryOf("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,""", + """"aggs":{"__whole_table_having__":{"filters":{"filters":{"_all":{"match_all":{}}}},""", + """"aggs":{"c":{"value_count":{"field":"_index"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", + """"script":{"source":"(params.c == null ? false : """, + """(Math.max(params.c, 0) > 1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // AC-2 -- nothing in the matrix emits a query with the predicate missing + // --------------------------------------------------------------------------------------------- + + private val refused: Seq[String] = Seq( + group + "ABS(COUNT(*)) > 1", + group + "FLOOR(COUNT(*)) > 1", + group + "NULLIF(COUNT(*), 0) > 1", + group + "NULLIF(c, 0) > 1", + group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1", + group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1", + group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2", + "SELECT status, MAX(amount) AS m FROM t GROUP BY status HAVING ROUND(MAX(amount), 2) > 1", + "SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1" + ) + + "a HAVING the engine cannot express" should "never reach emission" in { + refused.foreach { sql => + withClue(s"[$sql] ") { + Parser(sql) match { + case Left(e) => e.msg should startWith("HAVING cannot") + case Right(s) => + fail(s"expected a rejection; it emitted ${queryOf(sql)} from $s") + } + } + } + } + + "every statement that DOES emit" should "carry a having_filter" in { + // Non-vacuity: the invariant below would pass on an empty set, which is exactly the shape the + // defect produced. + emitted.foreach { sql => + withClue(s"[$sql] ") { + valuesOf(mapper.readTree(queryOf(sql)), "bucket_selector") should not be empty + } + } + } + + // --------------------------------------------------------------------------------------------- + // The structural invariant (AD-2 item 3) + // --------------------------------------------------------------------------------------------- + + "every emitted bucket pipeline" should "read exactly the metrics its buckets_path declares" in { + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") + pipelines should not be empty + pipelines.foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = Option(pipeline.get("script")) + .map { s => + if (s.isTextual) s.asText() else s.get("source").asText() + } + .getOrElse(fail("a bucket pipeline with no script")) + val read = paramRef.findAllMatchIn(source).map(_.group(1)).toSet - "__now__" + read shouldBe declared + } + } + } + } + + it should "null-test every metric a bucket_selector declares" in { + // The AC-4b contract, restated for the #389 population: a selector either guards the metric + // (`params.x == null ? false : ...`) or handles the null itself (`params.x != null ? ... : k`, + // which is what COALESCE means). Dereferencing it unguarded fails the whole search. + emitted.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "bucket_selector").foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + val source = pipeline.get("script").get("source").asText() + declared.foreach { key => + withClue(s"metric '$key' in [$source] ") { + assert( + source.contains(s"params.$key == null") || source.contains(s"params.$key != null"), + "is never null-tested" + ) + } + } + } + } + } + } +} diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala index 1232ec71..535c63c8 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/PainlessNullSurvivalSpec.scala @@ -177,7 +177,14 @@ class PainlessNullSurvivalSpec extends AnyFlatSpec with Matchers { val code = raw.replaceAll("(?s)/\\*.*?\\*/", " ").replaceAll("(?m)//.*$", " ") val names = boundToPredicate.flatMap(_.findAllMatchIn(code).map(_.group(1))).toSet val namedUses = names.toSeq.map { n => - s"""(?\.not`, so + // `p.includePolarityOfRight(not)` -- the member that exists PRECISELY to consume this + // fold -- was invisible, and its file scored 0 uses / 0 classifications and passed + // VACUOUSLY. A gate that stops seeing the site it has just caught is worse than no gate. + // The named fold counts as a use AND, like `negated`, as a classification: calling it IS + // the fold. Replace that call with a raw `p.not` and the file reddens, which is the point. + (s"""(? classifications) Some( diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala index 69a13d0b..45037666 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -1564,16 +1564,61 @@ package object sql { def bucketPath: String - lazy val allMetricsPath: Map[String, String] = { - metricName match { - case Some(name) => Map(name -> name) - // The alias of a SELECT `bucket_script` item referenced from HAVING (`... AS d ... HAVING - // d > 3`): the selector reads the sibling pipeline aggregation by that name. - case _ if hasAggregation && fieldAlias.isDefined => Map(aliasOrName -> aliasOrName) - case _ => Map.empty + /** Every aggregate this identifier's expression references ANYWHERE -- its own chain, the + * ARGUMENTS of every function in it, a `CASE`'s `THEN` results and its `WHEN` conditions + * (issue #389). + * + * šŸ”“ A function never looked inside its own arguments: [[FunctionChain.hasAggregation]] is + * `functions.exists(_.hasAggregation)` over the chain's OWN links, so `COUNT(*)` written + * inside `ABS(...)` / `COALESCE(...)` / `NULLIF(...)` made the whole tree answer "no aggregate + * here". That one fact is why the aggregation was never created, why `buckets_path` published + * nothing, and why the `bucket_selector` degenerated to `1 == 1` and every group came back. + * + * Two walks, because the grammar hides the aggregate in two different places and neither walk + * sees the other's: [[FunctionUtils.funIdentifiers]] descends `FunctionN.args` and the chain + * (so it reaches a `CASE`'s `THEN` result, which IS an argument), while a `CASE`'s `WHEN` + * conditions are deliberately excluded from `Case.args` and are reachable only through + * [[app.softnetwork.elastic.sql.function.cond.Case.conditionsOf]]. MEASURED: `CASE WHEN + * COUNT(*) > 1 THEN 1 ELSE 0 END` yields NO aggregate from `funIdentifiers` alone. + * + * Deduplicated by [[metricPathKey]] -- which for every element here IS [[metricName]], since + * `isAggregation` means `aggregateFunction.isDefined` and `metricName` is defined exactly + * then. Stating it as `metricPathKey` keeps ONE notion of metric identity across this + * derivation and `Expression.bucketMetrics`. + */ + lazy val referencedAggregates: Seq[Identifier] = { + val fromChain = FunctionUtils.funIdentifiers(this).filter(_.isAggregation) + val fromCaseConditions = function.cond.Case + .conditionsOf(this) + .flatMap(_.referencedIdentifiers) + .flatMap(id => if (id.isAggregation) Seq(id) else id.referencedAggregates) + // Deduplicated by [[metricPathKey]] -- the SAME key `Expression.bucketMetrics` dedups by, + // so the two cannot disagree about what "the same metric" means. + (fromChain ++ fromCaseConditions).foldLeft(Seq.empty[Identifier]) { (acc, id) => + if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id } } + /** The aggregations a bucket pipeline reading THIS identifier addresses, in order -- the ONE + * derivation behind [[allMetricsPath]] (what `buckets_path` publishes), behind + * `Criteria.extractAggregationFields` (which aggregations are created) and behind the + * `bucket_selector` null guard. Three answers to one question would be three answers that + * drift (`project_self_join_alias_resolution`). + * + * Three arms, and the third is issue #389's: + * 1. the identifier IS an aggregate -- one metric, itself; 2. it is the alias of a SELECT + * `bucket_script` item referenced from HAVING (`MAX(x) - MIN(x) AS d ... HAVING d > 3`) + * -- the selector reads that sibling pipeline aggregation by name and NOT its operands, so the + * operands must not be published here; 3. otherwise, every aggregate it references through a + * function argument or a `CASE`. + */ + lazy val bucketMetrics: Seq[Identifier] = + if (metricName.isDefined || (hasAggregation && fieldAlias.isDefined)) Seq(this) + else referencedAggregates + + lazy val allMetricsPath: Map[String, String] = + bucketMetrics.map(id => id.metricPathKey -> id.metricPathKey).toMap + override def sql: String = { var parts: Seq[String] = name.split("\\.").toSeq tableAlias match { @@ -1666,10 +1711,16 @@ package object sql { } } + /** The `buckets_path` key this identifier is addressed by: its derived [[metricName]] when it + * is an aggregate, else the alias a SELECT `bucket_script` item published it under. ONE + * derivation, read by [[metricParam]] and by [[allMetricsPath]]. + */ + lazy val metricPathKey: String = metricName.getOrElse(aliasOrName) + /** How a bucket pipeline script reads this aggregate: the metric Elasticsearch already * computed, published under `buckets_path` as [[metricName]]. */ - lazy val metricParam: String = s"params.${metricName.getOrElse(aliasOrName)}" + lazy val metricParam: String = s"params.$metricPathKey" lazy val script: Option[String] = if (isTemporal) { diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala index 70af12b1..8238ada0 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala @@ -18,6 +18,8 @@ package app.softnetwork.elastic.sql.query import app.softnetwork.elastic.sql.`type`.SQLType import app.softnetwork.elastic.sql.operator._ +import scala.util.Try +import scala.util.matching.Regex import app.softnetwork.elastic.sql.{ quoteIdentifier, Expr, @@ -347,60 +349,310 @@ case class BucketPath(buckets: Seq[Bucket]) { override def toString: String = path } +/** What a `HAVING` criterion contributes to a bucket pipeline. THREE values, because `""` used to + * mean two things and the caller could not tell them apart (issue #389): "this level has nothing + * to filter" and "I cannot express this filter" both came back as the empty string, and the second + * was read as the first -- so a predicate the engine could not emit was silently DROPPED and every + * group came back with HTTP 200. + */ +sealed trait MetricSelector + +object MetricSelector { + + /** Nothing for the bucket pipeline to do at this level. Legitimate and common: a predicate over + * the bucket KEY is honoured by the `terms` `include` / `exclude` instead (`HAVING status = + * 'A'`), and a condition reading another level's metric belongs to that level. + */ + case object NoFilter extends MetricSelector + + /** A `bucket_selector` `script.source`: ONE boolean expression reading published metrics. */ + case class Filter(script: String) extends MetricSelector + + /** The predicate references an aggregate and the engine cannot express it as a `bucket_selector`. + * It is REFUSED by name at `Having.validate()`, never dropped. + */ + case class Unrepresentable(expression: String, reason: String) extends MetricSelector { + + /** šŸ”“ The remedy is "compare the aggregate itself", NOT "alias it in SELECT". The sibling rule + * on inline arithmetic (`HAVING MAX(x) - MIN(x) > 10`) does say to alias it, and that advice + * does NOT transfer: MEASURED, `SELECT ABS(COUNT(*)) AS a ... GROUP BY city` is itself + * rejected ("Non-aggregated fields ... cannot be selected when GROUP BY is present") for every + * member of this family, so the borrowed wording would send the user to a dead end. + */ + def message: String = + s"HAVING cannot be applied to $expression: $reason. " + + "Compare the aggregate itself (HAVING SUM(x) > 10 rather than HAVING ROUND(SUM(x), 2) > 10), " + + "or apply the function to the result outside the query." + } +} + object MetricSelectorScript { - def metricSelector(expr: Criteria): String = expr match { + import MetricSelector._ + + /** The bucket-pipeline script of a `HAVING` criterion, or `"1 == 1"` when there is nothing to + * filter at this level. + * + * šŸ”“ It THROWS on an [[MetricSelector.Unrepresentable]] criterion, and that is the point: + * returning `""` is what let a predicate vanish. `Having.validate()` refuses every such + * statement inside `Parser.apply`, so no venue can reach this throw with a parsed statement -- + * it is the invariant's second line of defence, for a `SingleSearch` assembled in code and + * emitted without validation. + */ + def metricSelector(expr: Criteria): String = selector(expr) match { + case NoFilter => "1 == 1" + case Filter(script) => script + case u: Unrepresentable => throw new IllegalStateException(u.message) + } + + /** Every `HAVING` criterion of this tree the engine cannot express, in statement order. Read by + * `Having.validate()`; empty for every criterion tree [[metricSelector]] can render. + */ + def unrepresentable(expr: Criteria): Seq[MetricSelector.Unrepresentable] = expr match { + case Predicate(left, _, right, maybeNot, _) => + unrepresentable(left) ++ (maybeNot match { + case Some(_) => right.negated.map(unrepresentable).getOrElse(unrepresentable(right)) + case None => unrepresentable(right) + }) + case relation: ElasticRelation => unrepresentable(relation.criteria) + case _ => + selector(expr) match { + case u: Unrepresentable => Seq(u) + case _ => Nil + } + } + + private def selector(expr: Criteria): MetricSelector = expr match { case Predicate(left, op, right, maybeNot, group) => - val leftStr = metricSelector(left) - val rightStr = metricSelector(right) - - // Filtering all "1 == 1" - if (leftStr == "1 == 1" && rightStr == "1 == 1") { - "1 == 1" - } else if (leftStr == "1 == 1") { - rightStr - } else if (rightStr == "1 == 1") { - leftStr - } else { - val opStr = op match { - case AND | OR => op.painless(None) - case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") - } - // `A AND NOT B`: the parser attaches the NOT to the RIGHT criteria (`Predicate.sql` and - // `asFilter` agree); this used to prefix the LEFT one -- the exact complement of what was - // asked. The negation is pushed INTO the right-hand expression when it is a single one - // (`NOT MAX(x) > 45` renders `(params.max_x == null ? false : (params.max_x <= 45))`), so a - // bucket whose metric is missing still fails the test -- `!(guard ? false : ...)` would let - // it through, against SQL's three-valued NOT and the AC 4b contract. A compound right side - // falls back to `!( ... )`. - maybeNot match { - case Some(_) => - right.negated match { - case Some(n) => s"($leftStr) $opStr ${metricSelector(n)}" - // Grammar-unreachable today (`NOT (A AND B)` in HAVING is a parse rejection); kept - // as the total fallback for a compound right side. - case None => s"($leftStr) $opStr !($rightStr)" - } - case None if group => s"($leftStr) $opStr ($rightStr)" - case None => s"$leftStr $opStr $rightStr" - } + // The RIGHT criterion as it is really rendered: a `NOT` on the predicate is pushed INTO it + // when it can carry one, so the verdict must be taken on the SAME node the script is built + // from -- otherwise a refusal could be decided on a criterion nothing emits. + val effectiveRight = maybeNot.flatMap(_ => right.negated).getOrElse(right) + val leftSel = selector(left) + val rightSel = selector(effectiveRight) + (leftSel, rightSel) match { + case (u: Unrepresentable, _) => u + case (_, u: Unrepresentable) => u + // Filtering all "1 == 1" + case (NoFilter, NoFilter) => NoFilter + case (NoFilter, r) => r + case (l, NoFilter) => l + case (Filter(leftStr), Filter(rightStr)) => + val opStr = op match { + case AND | OR => op.painless(None) + case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") + } + // `A AND NOT B`: the parser attaches the NOT to the RIGHT criteria (`Predicate.sql` and + // `asFilter` agree); this used to prefix the LEFT one -- the exact complement of what was + // asked. The negation is pushed INTO the right-hand expression when it is a single one + // (`NOT MAX(x) > 45` renders `(params.max_x == null ? false : (params.max_x <= 45))`), so + // a bucket whose metric is missing still fails the test -- `!(guard ? false : ...)` would + // let it through, against SQL's three-valued NOT and the AC 4b contract. A compound right + // side falls back to `!( ... )`. + Filter(maybeNot match { + case Some(_) if right.negated.isDefined => s"($leftStr) $opStr $rightStr" + // Grammar-unreachable today (`NOT (A AND B)` in HAVING is a parse rejection); kept + // as the total fallback for a compound right side. + case Some(_) => s"($leftStr) $opStr !($rightStr)" + case None if group => s"($leftStr) $opStr ($rightStr)" + case None => s"$leftStr $opStr $rightStr" + }) } - case relation: ElasticRelation => metricSelector(relation.criteria) + case relation: ElasticRelation => selector(relation.criteria) + + case _: MultiMatchCriteria => NoFilter + + // The context-free rendering of an aggregate predicate IS the bucket-pipeline rendering + // (`Expression.bucketPipelinePainless`): `params.` reads, null-guarded, one + // parenthesised expression, temporal literal already converted to epoch millis. + // + // šŸ”“ It goes through the SAME gate as the arm below, and that is not symmetry for its own sake. + // This arm keys on the LEFT operand being an aggregate and says nothing about the RIGHT one, so + // every disqualifier refused below was EMITTED here. MEASURED and EXECUTED on Elasticsearch + // 8.18.3, all four with a bare `COUNT(*)` on the left: + // `> ROUND(MAX(a), 2)` -> `invalid sequence of tokens near ['def']` HTTP 400 + // `> NULLIF(MAX(a), 0)` -> `Cannot cast from [boolean] to [int]` HTTP 400 + // `BETWEEN 1 AND ABS(MAX(a))` -> `Unknown call [ABS] with [1] arguments` HTTP 400 + // (the BOUND leaked RAW SQL into the Painless source) + // `> CASE WHEN MAX(a) > 1 ā€¦` -> `painless(None)` THREW inside `validate()`, surfacing + // `Internal parser error: ā€¦` to the user (issue #250's family) + case e: Expression if e.isAggregation || e.referencesBucketMetric => + representable(e) match { + case Left(reason) => Unrepresentable(e.sql, reason) + case Right(script) => Filter(script) + } - case _: MultiMatchCriteria => "1 == 1" + // šŸ”“ Issue #389 -- the predicate reads an aggregate through a FUNCTION (`ABS(COUNT(*)) > 1`, + // `COALESCE(COUNT(*), 0) > 1`, `1 < ABS(COUNT(*))`). Neither flag above sees it, because a + // function never looks inside its own arguments, so it fell to the `1 == 1` catch-all below + // and the whole condition VANISHED. Emitted when the rendering is provably a single boolean + // expression; refused BY NAME when it is not. Never dropped. + case e: Expression if e.bucketMetrics.nonEmpty => + representable(e) match { + case Left(reason) => Unrepresentable(e.sql, reason) + case Right(script) => Filter(script) + } - case e: Expression if e.isAggregation || e.referencesBucketMetric => - // NO FILTERING: the script is generated for all metrics. The context-free rendering of an - // aggregate predicate IS the bucket-pipeline rendering (`Expression.bucketPipelinePainless`): - // `params.` reads, null-guarded, one parenthesised expression, temporal literal - // already converted to epoch millis. It used to be converted HERE by appending - // `.toInstant().toEpochMilli()` to the rendered predicate -- which only reached the literal - // because the predicate happened to end with it. - e.painless(None) - case _ => "1 == 1" + case _ => NoFilter } + /** This predicate's `bucket_selector` script, or the reason the engine cannot express it. + * + * ONE rendering: the verdict and the script it authorises come out of the same call, so the + * emission can never be the second render of a tree whose FIRST render was the one vetted. + * + * šŸ”“ The verdict is taken on the RENDERING, never on a list of function names. A name list is a + * fourth derivation of what the renderer does and would disagree with it the first time a + * rendering changed (`project_type_derivations_runtime_declared_reported`); the rendering is the + * thing Elasticsearch actually compiles. And it is a POSITIVE proof -- everything this method + * cannot account for is refused, because a `HAVING` that cannot be expressed must fail, never + * silently return every group (`feedback_nonsense_input_fails_loudly`). + * + * The four disqualifiers, each MEASURED on a real rendering: + * 1. it cannot be rendered at all without a document context -- `CASE WHEN ... END` throws; 2. + * it carries a STATEMENT, not an expression -- `ROUND(MAX(x), 2)` renders `def arg1 = + * Math.pow(10, 2); ...`, and a `bucket_selector` source is one expression; 3. it can + * evaluate to NULL -- `NULLIF(COUNT(*), 0) > 1` renders `(params.c) == 0 ? null : + * (params.c) > 1`, which hands Elasticsearch a `null` where a boolean is required. `? null + * :` is the renderer's OWN null-producing idiom and is already the marker the repo's + * null-safety guard keys on; 4. its rendering BOXES a number -- `ABS(COUNT(*)) > 1` renders + * `Double.valueOf(Math.abs(params.c)) > 1`, and the bucket-pipeline script context does not + * compile it; 5. it reads a local nothing binds -- `NULLIF(c, 0) > 1` over a SELECT alias + * renders `arg0 == 0 ? null : arg0 > 1`, and `arg0` is a `PainlessContext` parameter that + * no bucket pipeline declares. Every free lower-case word is refused unless it is a + * Painless literal (`null` / `true` / `false`) or the `params` map itself: a Painless + * expression has no free functions, so a method call is always preceded by a `.` and a + * class is capitalised. + * + * šŸ”“ Rule 4 is the one NO amount of reading could have produced, and it is why the acceptance + * bar for this issue was EXECUTION and not a byte pin + * (`feedback_assert_the_mechanism_not_a_proxy` + * -- PARSES != RENDERS != RUNS). The spec asserted that `Double.valueOf(Math.abs(params.c)) > 1` + * was "legal as a bucket_selector source"; MEASURED on a real Elasticsearch 8.18.3 it is a + * `class_cast_exception: Cannot cast from [java.lang.Double] to [int]` at COMPILE time, and the + * whole search fails. The rule is DERIVED, not guessed: eighteen shapes were rendered and + * executed, and `.valueOf(` separates the nine that run from the nine that do not, exactly. The + * same call compiles in the QUERY script context (`WHERE ABS(x) > 1` runs), so this is a + * property of the bucket-pipeline context, not of the rendering alone -- which is precisely why + * a syntactic gate needs a measured rule here and cannot derive one. + * + * Rule 5's complement also proves rule 3 is not the only null route, and rules 2-5 are decided + * on the rendering with every STRING LITERAL blanked, so a `;`, a `?` or a word inside one is + * never read as code. + */ + private[query] def representable(e: Expression): Either[String, String] = + Try(e.functionBucketPipelinePainless).toOption match { + case None => + Left("it cannot be rendered without a document, and a bucket pipeline has none") + case Some(rendering) => disqualifyRendering(rendering).toLeft(rendering) + } + + /** Rules 2-6 of [[representable]], asked of a RENDERING rather than of an expression. + * + * Exposed as its own function because it is a pure function of a string and is therefore the + * only surface on which each rule can be falsified INDIVIDUALLY + * (`feedback_assert_the_mechanism_not_a_proxy`): the rules overlap on real SQL -- every shape + * that trips the top-level-conditional rule also renders a `? null :` -- so a mutation matrix + * driven by statements alone reports a live rule as unguarded. The end-to-end rows stay; these + * are what keep each rule honest. + */ + private[query] def disqualifyRendering(rendering: String): Option[String] = + disqualify(blankStringLiterals(rendering)) + + private def disqualify(code: String): Option[String] = { + if (code.contains(";") || code.contains("def ")) + Some( + "its rendering needs a local declaration, and a bucket_selector source is one expression" + ) + else if (code.contains("? null :")) + Some("its rendering can evaluate to NULL, and a bucket_selector source must be a boolean") + else if (conditionalAtTopLevel(code)) + Some( + "its outermost operator is a conditional rather than a comparison, so it does not render a boolean" + ) + else if (code.contains(".valueOf(")) + Some( + "its rendering boxes a number, which the bucket-pipeline script context does not compile" + ) + else if (code.contains(".compareTo(\"") && code.contains("params.")) + // šŸ”“ A metric arrives from `buckets_path` as a NUMBER -- a date metric as epoch millis -- so a + // rendering that compares it as a STRING cannot run. MEASURED on Elasticsearch 8.18.3: + // `HAVING COALESCE(MAX(d), '2020-01-01') > '2019-01-01'` renders + // `(params.max_d != null ? params.max_d : "2020-01-01").compareTo("2019-01-01") > 0` and + // fails with `cannot explicitly cast def [java.lang.String] to java.lang.Double`. + // + // The RENDERER defect is pre-existing and wider than this issue -- the bare + // `HAVING MAX(d) > '2019-01-01'` renders byte-identically on the control and fails the same + // way -- but a raw Painless class-cast reaching the user is not "refused by name", so the + // shape is refused here until the rendering is fixed. + Some( + "its rendering compares a metric as text, and a bucket pipeline reads every metric as a " + + "number (a date metric as epoch millis)" + ) + else + freeLocal(code).map(name => + s"its rendering reads '$name', which nothing binds in a bucket pipeline" + ) + } + + /** The same string with the CONTENT of every Painless string literal replaced by spaces, so the + * scanners above read code and only code. Length-preserving, so an index into the result is an + * index into the original. + */ + private def blankStringLiterals(s: String): String = { + val out = new StringBuilder(s) + var i = 0 + while (i < s.length) { + val c = s.charAt(i) + if (c == '"' || c == '\'') { + var j = i + 1 + while (j < s.length && s.charAt(j) != c) { + if (s.charAt(j) == '\\') { out.setCharAt(j, ' '); j += 1 } + if (j < s.length) { out.setCharAt(j, ' '); j += 1 } + } + i = j + 1 + } else i += 1 + } + out.toString + } + + /** Is there a `?` outside every parenthesis? Then the comparison is not the outermost operator + * and the expression's value is whatever the branches are -- not necessarily a boolean. + */ + private def conditionalAtTopLevel(code: String): Boolean = { + var depth = 0 + var i = 0 + while (i < code.length) { + code.charAt(i) match { + case '(' => depth += 1 + case ')' => depth -= 1 + case '?' => if (depth <= 0) return true + case _ => + } + i += 1 + } + false + } + + private val FreeWord: Regex = """(? + m.group(1) + } + + private val PainlessLiterals: Set[String] = Set("null", "true", "false") + } case class BucketIncludesExcludes(values: Set[String] = Set.empty, regex: Option[String] = None) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala index 44fad1a3..fd129495 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala @@ -20,6 +20,21 @@ import app.softnetwork.elastic.sql.{Expr, Identifier, TokenRegex, Updateable} case object Having extends Expr("HAVING") with TokenRegex { + /** The SELECT items a bare name in a `HAVING` may resolve to: aggregates AND arithmetic over + * aggregates (`MAX(x) - MIN(x) AS d`), the latter being a `bucket_script` that a + * `bucket_selector` may read as a sibling pipeline aggregation by name. + * + * ONE derivation, read by [[resolveAggregateAliases]] (which SUBSTITUTES a bare reference) and + * by `SingleSearch.validate()` (which REFUSES a reference substitution cannot reach, issue #389: + * `HAVING NULLIF(c, 0) > 1` hides the alias inside a function ARGUMENT, where the substitution + * below -- deliberately scoped to an operand -- does not go). + */ + private[query] def aggregateAliases(request: SingleSearch): Map[String, Identifier] = + request.select.fields.collect { + case f if (f.isAggregation || f.isBucketScript) && f.fieldAlias.isDefined => + f.fieldAlias.get.alias -> f.identifier + }.toMap + /** `HAVING cnt > 1` where `cnt` aliases a SELECT aggregate (`COUNT(name) AS cnt`). The bare * identifier carries no aggregate function of its own, so the selector rendering saw no metric * in the condition and the whole HAVING degenerated to `1 == 1`: every group came back ā€” the @@ -34,12 +49,7 @@ case object Having extends Expr("HAVING") with TokenRegex { criteria: Criteria, request: SingleSearch ): Criteria = { - // Aggregates AND arithmetic over aggregates (`MAX(x) - MIN(x) AS d`): the latter is a - // `bucket_script`, and a `bucket_selector` may read a sibling pipeline aggregation by name. - val aliased: Map[String, Identifier] = request.select.fields.collect { - case f if (f.isAggregation || f.isBucketScript) && f.fieldAlias.isDefined => - f.fieldAlias.get.alias -> f.identifier - }.toMap + val aliased: Map[String, Identifier] = aggregateAliases(request) if (aliased.isEmpty) return criteria def substitute(id: Identifier): Identifier = @@ -105,13 +115,43 @@ case class Having(criteria: Option[Criteria]) extends Updateable { def nestedElements: Seq[NestedElement] = criteria.map(_.nestedElements).getOrElse(Seq.empty).groupBy(_.path).map(_._2.head).toList + /** Every condition of this clause the engine cannot express as a `bucket_selector` (issue #389), + * in statement order. Refused by name in `SingleSearch.validate()`; EMPTY is the only shape that + * reaches emission. + * + * šŸ”“ PUBLIC on purpose. `softclient4es-extensions` builds the materialized-view transform's + * `TransformBucketSelectorConfig` from [[script]] and has no other way to tell "this clause has + * nothing to filter" from "this clause cannot be expressed" -- which is #389's conflation, live + * inside MV enrichment. This is the reason it needs, and the only thing core can give it until + * it is rebuilt against a published `0.24.0`. + */ + def unrepresentable: Seq[MetricSelector.Unrepresentable] = + criteria.toSeq.flatMap(MetricSelectorScript.unrepresentable) + + /** The `bucket_selector` source for this clause, or `None` when there is nothing to filter. + * + * šŸ”“ NOT dead code -- round 1 recorded it as having no production caller and that was WRONG: + * `softclient4es-extensions`'s `graph/Stage.scala:291` reads it to build a materialized view's + * `TransformBucketSelectorConfig`. That is a FIFTH HAVING mechanism, outside this repo. + * + * āš ļø It still answers `None` for a clause the engine cannot express, which is exactly the + * conflation #389 closed everywhere else -- so a materialized view over such a HAVING is + * enriched with NO filter. Core cannot fix that from here: switching this to + * `MetricSelectorScript.metricSelector` would THROW inside the extension (a 500), and the + * extension must decide for itself. [[unrepresentable]] is public so it can. See `Ā§9` of the + * story artifact (Ā§10.B) for the exact change extensions needs once `0.24.0` is published. + */ + @deprecated("read `unrepresentable` first, then `MetricSelectorScript.metricSelector`", "0.24.0") def script: Option[String] = criteria.flatMap { criteria => - val fullScript = MetricSelectorScript - .metricSelector(criteria) - .replaceAll("1 == 1 &&", "") - .replaceAll("&& 1 == 1", "") - .replaceAll("1 == 1", "") - .trim - if (fullScript.nonEmpty) Some(fullScript) else None + if (unrepresentable.nonEmpty) None + else { + val fullScript = MetricSelectorScript + .metricSelector(criteria) + .replaceAll("1 == 1 &&", "") + .replaceAll("&& 1 == 1", "") + .replaceAll("1 == 1", "") + .trim + if (fullScript.nonEmpty) Some(fullScript) else None + } } } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala index 3228a816..02f4c950 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala @@ -217,10 +217,15 @@ sealed trait Criteria extends Updateable with PainlessScript { case Predicate(left, _, right, _, _) => left.extractAggregationFields ++ right.extractAggregationFields case relation: ElasticRelation => relation.criteria.extractAggregationFields + // šŸ”“ Issue #389 -- `bucketMetrics`, not `aggregations.nonEmpty` + `metricName`. An aggregate + // written inside a function ARGUMENT (`HAVING ABS(COUNT(*)) > 1`) is not the identifier's own + // aggregate, so `metricName` was `None` and NO aggregation was created for it: the selector + // had no `params.count_all` to read even once it was taught to read one. It is also the + // aggregate on the RIGHT of the comparison (`HAVING COUNT(*) > ABS(MAX(x))` emitted a script + // reading `params.max_x` with no `max_x` aggregation anywhere -- MEASURED on `main`). case e: Expression => - val identifiers = Seq(e.identifier) ++ e.maybeValue.collect { case id: Identifier => id } - identifiers - .filter(_.aggregations.nonEmpty) + (Seq(e.identifier) ++ e.maybeValue.collect { case id: Identifier => id }) + .flatMap(_.bucketMetrics) .flatMap { id => id.metricName.map(name => Field(id, Some(Alias(name)))) } @@ -239,10 +244,10 @@ sealed trait Criteria extends Updateable with PainlessScript { bucketIncludesExcludes: BucketIncludesExcludes ): BucketIncludesExcludes = this match { - case Predicate(left, _, right, n, _) => + case p @ Predicate(left, _, right, _, _) => right.includes( bucket, - (!not && n.isDefined) || (not && n.isEmpty), + p.includePolarityOfRight(not), left.includes(bucket, not, bucketIncludesExcludes) ) case relation: ElasticRelation => @@ -362,6 +367,19 @@ case class Predicate( else leftCriteria} $operator${not .map(_ => " NOT") .getOrElse("")} ${if (group) s"$rightCriteria)" else rightCriteria}" + + /** The polarity the RIGHT criterion inherits when a `terms` include/exclude traversal walks this + * predicate. The predicate's own `NOT` binds the RIGHT operand, and this IS its fold: the + * criterion is evaluated over the same bucket with the sense flipped, so `notConsumed` holds by + * construction and nothing downstream has to negate it again. + * + * šŸ”“ Named because it has TWO readers -- `Criteria.includes` and + * `SingleSearch.keyChannelConflict` -- and the round-5 regression this replaces was exactly one + * derivation written for one of two call sites that are the same function. + */ + private[query] def includePolarityOfRight(not: Boolean): Boolean = + (!not && this.not.isDefined) || (not && this.not.isEmpty) + override def update(request: SingleSearch): Criteria = { val updatedPredicate = this.copy( leftCriteria = leftCriteria.update(request), @@ -582,8 +600,18 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { if ((!not && maybeNot.isEmpty) || (not && maybeNot.isDefined)) maybeValue match { case Some(v: StringValue) if v.value.nonEmpty => + // šŸ”“ The SHARED `toRegex`, not a third private translation. This line used to + // read `v.value.replaceAll("%", ".*")`, which neither translates `_` nor escapes + // a regex metacharacter -- while `metricSelector`'s scaladoc asserts the shared + // one is used and the query-DSL path really does use it. MEASURED on ES 8.18.3 + // over the buckets `a.bZ`, `axbZ`, `ab`, `a1`: + // `status LIKE 'a_'` WHERE -> [a1, ab] HAVING -> NO BUCKETS + // `status LIKE 'a.b%'` WHERE -> [a.bZ] HAVING -> [a.bZ, axbZ] + // Pre-existing and byte-identical to `455433ae`, so not a regression -- but a + // silent wrong answer found while working on this very channel. + // āš ļø `RLIKE` below is RAW regex by definition and must NOT be translated. bucketIncludesExcludes.copy(regex = - bucketIncludesExcludes.regex.orElse(Option(v.value.replaceAll("%", ".*"))) + bucketIncludesExcludes.regex.orElse(Option(toRegex(v.value))) ) case _ => bucketIncludesExcludes } @@ -1253,13 +1281,58 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { case IS_NULL => s"$param == null" case IS_NOT_NULL => s"$param != null" case _ => - val metrics: Seq[Identifier] = - identifier +: maybeValue.collect { case id: Identifier if id.isAggregation => id }.toSeq - val guard = metrics.map(id => s"${id.metricParam} == null").mkString(" || ") + val guard = bucketMetrics.map(id => s"${id.metricParam} == null").mkString(" || ") s"($guard ? false : $painlessNot(${bucketPipelineCheck(param)}))" } } + /** Every metric THIS predicate reads, left operand and right operand alike, deduplicated and in + * order -- the guard set of [[bucketPipelinePainless]] and of [[functionBucketPipelinePainless]] + * alike, and the same derivation `Criteria.extractAggregationFields` creates the aggregations + * from and `extractAllMetricsPath` publishes. + * + * šŸ”“ It used to be `identifier +: maybeValue.collect { case id if id.isAggregation }`, which + * misses an aggregate reached through a function: `HAVING COUNT(*) > ABS(MAX(x))` emitted + * `params.max_x` UNGUARDED (issue #389, measured on `main` -- `Math.abs(null)` fails the + * search). + */ + private[query] def bucketMetrics: Seq[Identifier] = + (identifier.bucketMetrics ++ maybeValue.toSeq + .collect { case id: Identifier => + id + } + .flatMap(_.bucketMetrics)).foldLeft(Seq.empty[Identifier]) { (acc, id) => + if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id + } + + /** The bucket-pipeline rendering of a predicate that reads an aggregate through a FUNCTION + * (`HAVING COALESCE(COUNT(*), 0) > 1`, issue #389). + * + * Unlike [[bucketPipelinePainless]] there is no `params.` to compare directly: the + * aggregate is an operand of a function, and the CONTEXT-FREE rendering of that function already + * reads it as `params.` (measured: `ABS(COUNT(*)) > 1` renders + * `Double.valueOf(Math.abs(params.c)) > 1`). So the rendering IS the script -- once it has been + * proved to be a single boolean expression ([[MetricSelectorScript.representable]]) and once + * every metric it dereferences is null-guarded. + * + * šŸ”“ The guard is added only for a metric the rendering does not already test. `COALESCE` exists + * precisely to decide what a null means, and forcing `false` on it would make `COALESCE(MAX(x), + * 99) > 1` answer `false` where SQL says `true`. A rendering that does NOT test the metric + * (`Math.abs(params.c)`) would throw on a null instead, and SQL's answer for it is UNKNOWN -- + * which is the `false` this guard supplies, exactly as `bucketPipelinePainless` supplies it for + * a bare aggregate. + */ + private[query] def functionBucketPipelinePainless: String = { + val rendering = painless(None) + val unguarded = bucketMetrics.filterNot { id => + rendering.contains(s"${id.metricParam} == null") || + rendering.contains(s"${id.metricParam} != null") + } + if (unguarded.isEmpty) rendering + else + s"(${unguarded.map(id => s"${id.metricParam} == null").mkString(" || ")} ? false : ($rendering))" + } + /** The comparison body of the bucket-pipeline rendering, `param` (= `params.`) against * the right-hand side. BETWEEN and IN, whose `painless` never goes through `check`, override it. */ diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala index fe319c04..fa5061c5 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala @@ -17,7 +17,7 @@ package app.softnetwork.elastic.sql import app.softnetwork.elastic.sql.`type`.{SQLType, SQLTypeUtils, SQLTypes} -import app.softnetwork.elastic.sql.operator.{SetOperator, UNION} +import app.softnetwork.elastic.sql.operator.{AND, OR, SetOperator, UNION} import app.softnetwork.elastic.sql.schema.{ sqlConfig, validateScriptReferences, @@ -162,13 +162,26 @@ package object query { override def withoutNestedExplosion: SelectStatement = this.copy(explodeNested = false) - lazy val statement: Option[SearchStatement] = { - queryToStatement(query) match { - case Some(s: SearchStatement) => Some(if (!explodeNested) s.withoutNestedExplosion else s) - case _ => None - } + /** ONE parse. `statement` and [[parseError]] are two readings of it, so asking for the reason + * costs nothing and cannot disagree with the verdict. + */ + private lazy val parsed: Either[parser.ParserError, Statement] = parser.Parser(query) + + lazy val statement: Option[SearchStatement] = parsed match { + case Right(s: SearchStatement) => Some(if (!explodeNested) s.withoutNestedExplosion else s) + case _ => None } + /** Why this statement did not parse, when it did not (issue #389 / F7). + * + * šŸ”“ `statement` is an `Option`, so every rejection reaching a caller through it used to be + * reported as the client's generic "SQL query does not contain a valid search request" -- the + * reason was computed, formatted and then thrown away. That was tolerable while the rejections + * were syntax errors the user could see for themselves; a whole family of SEMANTIC refusals + * now lands here, each naming a clause and a remedy, and none of them could reach the caller. + */ + lazy val parseError: Option[String] = parsed.left.toOption.map(_.msg) + override def sql: SQL = statement match { case Some(value) => value.sql @@ -697,6 +710,227 @@ package object query { ) } + /** Where a `HAVING` leaf predicate can be evaluated (issue #389). + * + * šŸ”“ ROUND 3 -- the DOCUMENT push-down this used to authorise is GONE. Its licence was "a + * function of the GROUP BY key is constant within a bucket, so filtering the documents keeps + * whole buckets", and that is false twice over, both MEASURED on Elasticsearch 8.18.3: + * + * - the key can be a FUNCTION of the column, and the predicate can read the COLUMN. Over + * `2024-01-03, 2024-02-03, 2025-03-03, 2025-04-05`: `GROUP BY DAY(d)` -> dd=3 c=3 Ā· dd=5 + * c=1 `GROUP BY DAY(d) HAVING YEAR(d) = 2025` -> dd=3 c=1 Ā· dd=5 c=1 HTTP 200 A SURVIVING + * bucket's count changed -- the exact wrong answer the push-down was supposed to be immune + * to. + * - a MULTI-VALUED field puts one document in several buckets, and a document filter keeps + * or drops it WHOLE. Over `tags = ["a","b"], ["b"], ["c"], ["a"]`: `GROUP BY tags HAVING + * UPPER(tags) = 'A'` kept bucket `b`, which fails the predicate. The `terms` `include` + * path answers `a` alone, so the push-down was strictly LESS sound than the mechanism it + * was meant to generalise. + * + * A key predicate is therefore honoured at the BUCKET level or not at all. + */ + private[query] sealed trait HavingScope + private[query] object HavingScope { + + /** Reads an aggregate: the `bucket_selector` population. */ + case object Metric extends HavingScope + + /** Reads only GROUP BY keys. Honoured by [[keyPredicateOutcome]], never by a document filter. + */ + case object GroupKey extends HavingScope + + /** Neither -- it names a column that is not a grouping key, so it is not constant within a + * bucket and NO mechanism can honour it. Refused; it used to be silently dropped. + */ + case object Unscoped extends HavingScope + } + + /** How a GROUP BY key predicate is honoured -- the ONE classifier both emission paths read. + * + * šŸ”“ Two mechanisms for one feature is two derivations that can disagree + * (`project_self_join_alias_resolution`), so neither emission path re-decides: they read this + * value. `TermsFilter` means "the `terms` `include` / `exclude` path already expresses it" and + * is answered by the REAL `Expression.includes` / `excludes`, never by a copy of their rules. + */ + private[query] sealed trait KeyPredicateOutcome + private[query] object KeyPredicateOutcome { + + /** The `terms` `include` / `exclude` filter expresses this predicate. Emission is unchanged: + * the bridge's existing include/exclude call is what applies it. + */ + case object TermsFilter extends KeyPredicateOutcome + + /** Neither bucket-level mechanism can express it, so the statement is refused by name. + * + * āš ļø The lead's round-3 ruling specifies a THIRD outcome here -- a scripted `terms` source + * whose script returns only the key values satisfying the predicate, which is correct on a + * multi-valued field and on a function-of-column key by construction. It is NOT BUILT: see + * `Ā§10.A.3` of the story artifact for the design, the Painless it must emit (verified + * running on a real cluster) and what remains. Until it exists this arm carries that + * population, so the predicate is REFUSED rather than silently dropped or wrongly applied. + */ + case class Refused(reason: String) extends KeyPredicateOutcome + } + + private[query] def keyBucketOf(leaf: Identifier): Option[Bucket] = { + // šŸ”“ Resolve through THIS statement's bucket list, never through `Identifier.bucket`, which + // is a COPY attached during `update()` and deliberately NOT re-updated (the story-21.3 / #253 + // desync class) -- MEASURED reporting `nestedElement = None` for a bucket the statement lists + // as nested. + val byName = buckets.find(b => b.identifier.name.nonEmpty && b.identifier.name == leaf.name) + byName.orElse(leaf.bucket.flatMap(attached => buckets.find(_.name == attached.name))) + } + + private[query] def namedLeavesOf(e: Expression): Seq[Identifier] = + e.referencedIdentifiers + .flatMap(id => FunctionUtils.funIdentifiers(id)) + .filter(_.name.nonEmpty) + .distinct + + private[query] def havingScopeOf(e: Expression): HavingScope = + if (e.referencedIdentifiers.exists(_.bucketMetrics.nonEmpty)) HavingScope.Metric + else { + val leaves = namedLeavesOf(e) + if (leaves.nonEmpty && leaves.forall(l => keyBucketOf(l).isDefined)) HavingScope.GroupKey + else HavingScope.Unscoped + } + + /** Does the `terms` `include` / `exclude` path express this key predicate? + * + * šŸ”“ Asked of the REAL function, never of a copy of its rules -- a second derivation of "which + * key predicates the terms filter can spell" is the story-21.3 desync class and would silently + * change every shipped `HAVING = ` the first time the two disagreed. + */ + private[query] def keyExpressibleByTerms(e: Expression): Boolean = { + val empty = BucketIncludesExcludes() + buckets.exists(b => + e.includes(b, not = false, empty) != empty || e.excludes(b, not = false, empty) != empty + ) + } + + private[query] def keyPredicateOutcome(e: Expression): KeyPredicateOutcome = + if (keyExpressibleByTerms(e)) KeyPredicateOutcome.TermsFilter + else if (readsNestedKey(e)) + KeyPredicateOutcome.Refused( + s"HAVING cannot filter on ${e.sql}: the grouping key is a nested object, and a bucket " + + "filter over it is not expressible. Filter it in WHERE, or group by a field of the parent." + ) + else + KeyPredicateOutcome.Refused( + s"HAVING cannot filter on ${e.sql}: a condition over the GROUP BY key is applied by the " + + "terms filter, which can only express a direct comparison of the key (=, <>, LIKE, IN). " + + "Compare the key itself, or filter in WHERE." + ) + + /** The first `HAVING` combination whose halves cannot BOTH reach the `terms` filter, or `None` + * when every combination is a union the channel expresses. See rule (b2) in `validate()`. + * + * The analysis is per bucket and follows `Criteria.includes` exactly -- same polarity + * threading, same contribution test, both senses: + * - the INCLUDE list is a union, so it expresses a DISJUNCTION. Two halves that both + * contribute includes under a conjunction are not expressible. + * - the EXCLUDE list is a union of negations, so it expresses a CONJUNCTION. Anything + * excluded under a disjunction is not expressible -- including the mixed shape `= 'a' OR + * <> 'b'`, where the emission would AND an include with an exclude. + * + * The polarity is still threaded, because a `NOT` before a LEAF decides which channel that + * leaf feeds -- `= 'a' AND NOT = 'b'` is an include and an exclude, and expressible. + */ + private[query] def keyChannelConflict: Option[String] = { + val empty = BucketIncludesExcludes() + def unionOnlyReason(c: Criteria): String = + s"HAVING cannot combine the conditions in ${c.sql} on one GROUP BY key: Elasticsearch " + + "applies one list of kept values and one list of removed values, and each is a union, " + + "so this combination would be executed as a different one. Split the query, or restate " + + "it as an OR of equalities or an AND of inequalities." + def conflict(c: Criteria, bucket: Bucket, not: Boolean): Option[(Criteria, String)] = + c match { + case p @ Predicate(left, op, right, _, _) => + // ONE derivation of the polarity, shared with `Criteria.includes` -- see + // `Predicate.includePolarityOfRight`. + val rightNot = p.includePolarityOfRight(not) + val leftIncludes = left.includes(bucket, not, empty) + val leftExcludes = left.excludes(bucket, not, empty) + val rightIncludes = right.includes(bucket, rightNot, empty) + val rightExcludes = right.excludes(bucket, rightNot, empty) + // šŸ”“ No De Morgan arm here, and that is MEASURED, not assumed: `Predicate.maybeNot` + // negates the RIGHT OPERAND only, and `NOT ( ā€¦ )` around a group is rejected by the + // grammar (`end of input expected`, on this branch and on `455433ae`). So no `Predicate` + // is ever reached with a flipped polarity -- probed over the parseable shapes -- and an + // arm dualising `op` would be unreachable for every possible input. Round 3 shipped + // exactly such an arm and had to delete it; one is enough. + val leftContributes = leftIncludes != empty || leftExcludes != empty + val rightContributes = rightIncludes != empty || rightExcludes != empty + // šŸ”“ A channel holds ONE list of values and ONE pattern, and the emission keeps the + // PATTERN and discards the values (`ElasticAggregation`, both bridges), while a second + // pattern is lost to `orElse`. So two contributors COLLIDE whenever a pattern meets + // anything else in the same channel -- whatever the operator, and even where the + // combination itself is a union. MEASURED on ES 8.18.3 over `a`, `b1`, `c`: + // `HAVING status = 'a' OR status LIKE 'b%'` -> `include:"b.*"` -> ['b1'], and the + // SQL means ['a','b1']. + // āš ļø This is the claim an earlier draft of Ā§12.F got wrong: `= 'a' OR LIKE 'b%'` IS an + // OR of two kept-value contributions -- the row the documentation blesses -- so + // "the kept list is a union" is not sufficient on its own. + def collides(a: BucketIncludesExcludes, b: BucketIncludesExcludes): Boolean = + (a.regex.nonEmpty && b.regex.nonEmpty && a.regex != b.regex) || + (a.regex.nonEmpty && b.values.nonEmpty) || + (b.regex.nonEmpty && a.values.nonEmpty) + val here = + if (collides(leftIncludes, rightIncludes) || collides(leftExcludes, rightExcludes)) + Some( + (p: Criteria) -> + (s"HAVING cannot combine the conditions in ${p.sql} on one GROUP BY key: a key " + + "filter carries one list of values and one pattern, and a pattern replaces the " + + "list, so one side would be silently dropped. Use a single RLIKE that covers " + + "both, or split the query.") + ) + else if (op == AND && leftIncludes != empty && rightIncludes != empty) + Some((p: Criteria) -> unionOnlyReason(p)) + else if ( + op == OR && leftContributes && rightContributes && + (leftExcludes != empty || rightExcludes != empty) + ) Some((p: Criteria) -> unionOnlyReason(p)) + else None + here + .orElse(conflict(left, bucket, not)) + .orElse(conflict(right, bucket, rightNot)) + case relation: ElasticRelation => conflict(relation.criteria, bucket, not) + case _ => None + } + having.flatMap(_.criteria).flatMap { criteria => + buckets.view + .flatMap(bucket => conflict(criteria, bucket, not = false)) + .headOption + .map { case (_, reason) => reason } + } + } + + /** Does this key predicate read a NESTED bucket? Asked of the bucket the leaf RESOLVES to. */ + private[query] def readsNestedKey(e: Expression): Boolean = + e.nested || namedLeavesOf(e).exists(l => keyBucketOf(l).exists(_.nestedElement.isDefined)) + + /** Every leaf `Expression` of the FLAT part of the HAVING tree, in statement order. + * + * šŸ”“ A predicate scoped to a NESTED relation is deliberately NOT here, the same boundary the + * whole-table block below draws: it has its OWN mechanism (`requestToNestedFilterAggregation` + * turns it into a `filter` aggregation scoped to the inner-hits path). MEASURED: without this + * exclusion the repo's own "complex query" fixture is refused. HAVING has FIVE mechanisms -- + * the `bucket_selector`, the `terms` include/exclude, this nested filter, the extensions' + * materialized-view transform (see `Having.script`), and the scripted `terms` source that the + * round-3 ruling specifies and that is not built yet. + * + * A `lazy val`: three validation rules and the key logic all walk it (review LOW-9). + */ + private[query] lazy val havingLeaves: Seq[Expression] = { + def leaves(c: Criteria): Seq[Expression] = c match { + case Predicate(l, _, r, _, _) => leaves(l) ++ leaves(r) + case _: ElasticRelation => Nil + case e: Expression => Seq(e).filterNot(_.nested) + case _ => Nil + } + having.flatMap(_.criteria).toSeq.flatMap(leaves) + } + lazy val excludes: Seq[String] = select.except.map(_.fields.map(_.sourceField)).getOrElse(Nil) lazy val sources: Seq[String] = from.tables.map(_.name) @@ -847,14 +1081,27 @@ package object query { * measured, and each one names the shape it refuses. */ def validateResolved(): Either[String, Unit] = - ((where.flatMap(_.criteria).toSeq ++ having.flatMap(_.criteria).toSeq ++ - select.fields.flatMap(f => Case.conditionsOf(f.identifier)) ++ - orderBy.toSeq.flatMap(_.sorts.flatMap(s => Case.conditionsOf(s.field))) ++ - groupBy.toSeq.flatMap(_.buckets.flatMap(b => Case.conditionsOf(b.identifier)))) - .flatMap(_.temporalComparisonErrors) ++ + // šŸ”“ Issue #389 / F4 -- the representability gate runs AGAIN here, on the RESOLVED statement. + // `validate()` runs schema-less inside `Parser.apply`, and `update(Some(schema))` CAN change + // what a HAVING renders (measured divergence on three shapes, e.g. + // `COALESCE(MAX(d), CURRENT_DATE) > CURRENT_DATE` gains `.atStartOfDay(ZoneId.of('Z'))`). + // No shape was found that trips a disqualifier only after resolution, so this is latent + // rather than demonstrated -- and it is closed the cheap way: re-asking the question at the + // ONE seam that produces a resolved statement turns any such divergence into the same 400 + // every other refusal takes. The alternative (letting `MetricSelectorScript.metricSelector`'s + // `IllegalStateException` escape into the bridge) would surface a 500 with no clause named, + // which is issue #250's family. That throw stays as the last-resort invariant; this is what + // makes it unreachable in practice. + having.map(_.unrepresentable).getOrElse(Nil).headOption.map(u => Left(u.message)).getOrElse { + ((where.flatMap(_.criteria).toSeq ++ having.flatMap(_.criteria).toSeq ++ + select.fields.flatMap(f => Case.conditionsOf(f.identifier)) ++ + orderBy.toSeq.flatMap(_.sorts.flatMap(s => Case.conditionsOf(s.field))) ++ + groupBy.toSeq.flatMap(_.buckets.flatMap(b => Case.conditionsOf(b.identifier)))) + .flatMap(_.temporalComparisonErrors) ++ scriptedExpressions.flatMap(NullIf.mismatchesOf)).headOption - .map(Left(_)) - .getOrElse(Right(())) + .map(Left(_)) + .getOrElse(Right(())) + } /** Every expression of this statement that can be emitted as Painless, as the chain it is. * @@ -908,6 +1155,238 @@ package object query { case None => Right(()) } } + _ <- { + // šŸ”“ Issue #389 -- a HAVING over a FUNCTION of an aggregate must FILTER, or FAIL. It used + // to VANISH: a function never looks inside its own arguments, so `ABS(COUNT(*)) > 1` was + // invisible to the bucket-metric detection, the selector degenerated to `1 == 1` and + // every group came back with HTTP 200 (the #205 / #209 / #253 silent-wrong-answer + // family). Everything the engine can express is now emitted; everything it cannot is + // refused HERE, inside `Parser.apply`, so every venue sees it -- the REPL, JDBC, Flight, + // the materialized-view extension and the bridge's own emission path alike. + // + // āš ļø A PARTIAL emission counts as a wrong answer: one un-expressible conjunct refuses the + // whole statement rather than silently filtering on the other half. + having.map(_.unrepresentable).getOrElse(Nil).headOption match { + case Some(u) => Left(u.message) + case None => Right(()) + } + } + _ <- { + // The residual of the rule above, and the reason it needs the STATEMENT rather than the + // clause: `HAVING NULLIF(c, 0) > 1` names a SELECT aggregate by its ALIAS from inside a + // function ARGUMENT. `Having.resolveAggregateAliases` substitutes an OPERAND only, so `c` + // stays a bare column, the predicate references no aggregate at all, and the rule above + // cannot see it -- it renders `arg0 == 0 ? null : arg0 > 1`, reading a context parameter + // no bucket pipeline binds. MEASURED: dropped silently on `main`. + // + // āš ļø The alias set is read from `Having.aggregateAliases`, the SAME map the substitution + // uses, so the two cannot disagree about which bare names are aggregate references. A + // COLUMN that happens to share a SELECT aggregate's alias is therefore refused here for + // exactly the reason it is SUBSTITUTED there -- one ambiguity, one reading. + having.flatMap(_.criteria) match { + case None => Right(()) + case Some(criteria) => + val aliases = Having.aggregateAliases(this).keySet + criteria.referencedIdentifiers + .filter(id => id.functions.nonEmpty && id.bucketMetrics.isEmpty) + .flatMap(id => + FunctionUtils.funIdentifiers(id).map(_.name).filter(aliases.contains).map(id -> _) + ) + .headOption match { + case Some((id, alias)) => + Left( + s"HAVING cannot apply a function to the aggregate alias '$alias' (${id.sql}); " + + "compare the aggregate itself" + ) + case None => Right(()) + } + } + } + _ <- { + // šŸ”“ Issue #389 -- (a) a predicate that reads neither an aggregate NOR a grouping key is + // not constant within a bucket, so NO mechanism can honour it: the terms filter cannot + // spell it, the selector cannot read a document, and filtering documents would silently + // change every metric of every surviving group. MEASURED as a silent wrong answer on + // `main` and refused since. + // + // āš ļø NOT gated on `groupBy.isDefined` (review MEDIUM-4). With no GROUP BY there are no + // buckets, so every named leaf is `Unscoped` and every aggregate leaf is `Metric` -- the + // whole-table block below refuses a BARE column (`HAVING status = 'a'`) but its + // `filter(_.name.nonEmpty)` probe cannot see a FUNCTION of one, which is S8's original + // hole. MEASURED: `SELECT COUNT(*) AS c FROM t HAVING UPPER(status) = 'A'` answered + // `{"c":{"value":4}}` with the predicate gone. + havingLeaves.filter(e => havingScopeOf(e) == HavingScope.Unscoped).headOption match { + case Some(e) => + Left( + s"HAVING can only filter on a GROUP BY key or on an aggregate; ${e.sql} is " + + "neither. Move it to WHERE, or add its column to the GROUP BY." + ) + case None => Right(()) + } + } + _ <- { + // (b) ... and a key predicate neither bucket-level mechanism can express is refused by + // the ONE classifier both emission paths read. + havingLeaves + .filter(e => groupBy.isDefined && havingScopeOf(e) == HavingScope.GroupKey) + .map(keyPredicateOutcome) + .collectFirst { case KeyPredicateOutcome.Refused(reason) => Left(reason) } + .getOrElse(Right(())) + } + _ <- { + // (b2) šŸ”“ The `terms` filter carries ONE include list and ONE exclude list, and each is a + // UNION of its members. So the channel can express a union and NEVER an intersection -- + // and which of the two a combination needs depends on the OPERATOR *and* on the + // POLARITY, because `excludes` IS `includes(bucket, !not, ā€¦)`: the same method, read in + // the other sense. + // + // MEASURED on `455433ae` and on real ES 8.18.3: + // `HAVING city <> 'Paris' AND city <> 'Lyon'` -> `exclude:["Lyon","Paris"]`, CORRECT: + // NOT-in-A and NOT-in-B is NOT-in-(AāˆŖB). + // `HAVING city <> 'Paris' OR city <> 'Lyon'` -> the SAME `exclude:["Lyon","Paris"]`, + // a SILENT WRONG ANSWER: the disjunction is true for every bucket, and two are + // dropped. HTTP 200. + // `HAVING city = 'a' AND city = 'b'` -> `include:["a","b"]`, also a silent wrong + // answer: the include list means `a OR b`, the SQL means no bucket at all. + // `HAVING city = 'a' OR city = 'b'` -> `include:["a","b"]`, CORRECT. + // + // šŸ”“ This rule is ONE derivation that asks BOTH methods about BOTH sides. The defect it + // replaces (a round-5 regression, reverted with the rest of that work) was a rule + // written for the include sense only and applied by a method that is also the exclude + // sense -- so it inverted every `<>`. A rule about these two channels that consults only + // one of them is wrong by construction. + keyChannelConflict.map(Left(_)).getOrElse(Right(())) + } + _ <- { + // (c) šŸ”“ An OR is honoured by ONE mechanism or by none: a `bucket_selector` script cannot + // read the bucket KEY, the `terms` filter cannot read a metric, and the nested filter + // aggregation is a third scope entirely. An OR whose branches need different mechanisms + // is therefore EXECUTED AS AN AND -- measured on `main` over a 4-document fixture, + // `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the disjunction is two + // of them, because the key became a terms `include` (removing one) and the metric a + // `bucket_selector` (removing the other). + // + // āš ļø Review MEDIUM-5: the rule is stated as HOMOGENEITY, and its message names the real + // cause. It used to say "cannot OR a GROUP BY key with an aggregate" even when no + // aggregate was present. An OR of key predicates the terms filter UNIONS + // (`status = 'a' OR status = 'b'` -> `include: [a, b]`) is supported; an OR mixing a + // TermsFilter key with a scripted-or-refused one is not, because only the first is a + // union. + // + // āš ļø Relation-scoped leaves are counted HERE, unlike everywhere else: an OR spanning the + // nested filter and any other mechanism has exactly the same defect, and excluding them + // would leave `HAVING COUNT(*) > 1 OR ` executing as an AND. + def mechanismOf(e: Expression): String = + if (e.nested) "nested" + else + havingScopeOf(e) match { + case HavingScope.Metric => "metric" + case HavingScope.Unscoped => "unscoped" + // šŸ”“ Every GroupKey leaf reaching here is a `TermsFilter`: rule (b) above refuses + // the rest before this rule runs. Distinguishing them was DEAD -- deleting the + // distinction reddened nothing (round-3 mutation T5, and the same mutation was + // green in round 2). + case HavingScope.GroupKey => "key" + } + def leavesUnder(c: Criteria): Seq[Expression] = c match { + case Predicate(l, _, r, _, _) => leavesUnder(l) ++ leavesUnder(r) + case relation: ElasticRelation => leavesUnder(relation.criteria) + case e: Expression => Seq(e) + case _ => Nil + } + // šŸ”“ The STAGE, not just the mechanism. `mechanismOf` collapses every grouping level + // to the single label "key", so an OR across TWO GROUP BY keys looked homogeneous -- + // and Elasticsearch NESTS the two `terms` aggregations, which IS the conjunction this + // rule exists to catch. MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): + // `GROUP BY status, city HAVING status = 'a' OR city = 'b'` + // -> terms status include:["a"] > terms city include:["b"] -> ONE group (a,b), + // and the SQL means THREE. + // A leaf's stage is its mechanism PLUS, for a key predicate, the bucket it addresses. + // šŸ”“ No `e.nested ||` guard here, and its removal is DELIBERATE (final review F4). It + // made two leaves on DIFFERENT nested grouping levels collapse to one stage while two + // flat levels are refused -- the very asymmetry this rule exists to remove -- and it was + // unobservable: with no schema attached, `GROUP BY e.name` under `JOIN UNNEST` emits no + // `terms` at all, on this tree or on `455433ae`. An unobservable, unpinnable guard in a + // symmetry rule is worse than no guard, so the rule is symmetric by construction + // instead. A leaf that is not a GROUP BY key still has no level, which is what the + // second half says. + def levelOf(e: Expression): String = + if (havingScopeOf(e) != HavingScope.GroupKey) "" + else namedLeavesOf(e).flatMap(keyBucketOf).map(_.name).distinct.sorted.mkString(",") + def firstMixedOr(c: Criteria): Option[(Criteria, Seq[String], Boolean)] = c match { + case p @ Predicate(l, op, r, _, _) => + val here = + if (op == OR) { + val leaves = leavesUnder(p) + val kinds = leaves.map(mechanismOf).distinct + val stages = leaves.map(e => (mechanismOf(e), levelOf(e))).distinct + if (stages.size > 1) + Some( + ( + p: Criteria, + if (kinds.size > 1) kinds.sorted else stages.map(_._2).sorted, + kinds.size > 1 + ) + ) + else None + } else None + here.orElse(firstMixedOr(l)).orElse(firstMixedOr(r)) + case relation: ElasticRelation => firstMixedOr(relation.criteria) + case _ => None + } + having.flatMap(_.criteria).filter(_ => groupBy.isDefined).flatMap(firstMixedOr) match { + case Some((p, names, differentMechanisms)) => + Left( + if (differentMechanisms) + s"HAVING cannot OR conditions that Elasticsearch applies with different " + + s"mechanisms (${p.sql} mixes ${names.mkString(" and ")}): a group filter, a key " + + "filter and a nested filter are separate stages, so their disjunction would be " + + "executed as a conjunction. Split the query, or restate the condition as an AND." + else + s"HAVING cannot OR conditions on DIFFERENT GROUP BY keys (${p.sql} spans " + + s"${names.mkString(" and ")}): Elasticsearch nests one grouping level inside " + + "the other, so filtering both would be executed as a conjunction. Split the " + + "query, or restate the condition as an AND." + ) + case None => Right(()) + } + } + _ <- { + // šŸ”“ Issue #389 / F5 -- a representable HAVING that NO bucket level can address still + // vanished. MEASURED on the branch: + // + // SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name + // HAVING COALESCE(MAX(amount), 0) > 1 + // -> {"size":0,"_source":false,"aggs":{"max_amount":{"max":{"field":"amount"}}}} + // HTTP 200, no `having_filter` -- and the GROUP BY itself is gone. + // + // The metric is computed at the ROOT while the bucket lives inside the `nested` + // aggregation, so `resolveBucketMetric` answers `OutOfScope` at every level, every + // condition is filtered out, and `metricSelectorForBucket` returns the `""` that means + // "nothing to filter". That is the issue's SECOND conflation site. + // + // The vanished GROUP BY is a SEPARATE pre-existing defect on a different mechanism (the + // control answers `{"query":{"match_all":{}},"_source":true}` -- raw documents -- for the + // same statement) and is deliberately NOT fixed here. What this rule guarantees is the + // contract: the statement no longer answers HTTP 200 with a silently unfiltered result. + // + // A metric that DOES share the bucket's nesting level keeps working + // (`... GROUP BY e.name HAVING COUNT(e.address) > 1` emits the full nested tree). + val innermostNested = buckets.lastOption.flatMap(_.nestedElement).map(_.innerHitsName) + val strandedMetric = havingLeaves + .filter(e => havingScopeOf(e) == HavingScope.Metric) + .flatMap(_.bucketMetrics) + .find(m => innermostNested.isDefined && m.innerHitsName != innermostNested) + strandedMetric match { + case Some(m) if groupBy.isDefined => + Left( + s"HAVING cannot filter on ${m.sql}: it is computed outside the nested grouping " + + s"'${innermostNested.getOrElse("")}', so no level of the aggregation can read it. " + + "Aggregate a field of the nested object, or group by a field of the parent." + ) + case _ => Right(()) + } + } _ <- { // A HAVING / ORDER BY aggregate whose derived name equals a SELECT alias of a DIFFERENT // aggregate (`SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3`) would read the wrong metric diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala index 227e9e1c..7a5b2582 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/InPredicateRenderSpec.scala @@ -74,21 +74,30 @@ class InPredicateRenderSpec extends AnyFlatSpec with Matchers { s"SELECT id FROM t GROUP BY id HAVING $name(a) NOT IN (1, 2)" ) - /** A statement is IN SCOPE only if it parses; a function that needs other arity or types simply - * is not exercised by this shape. That makes the sweep partial, which is why the next test - * asserts what it must contain. + /** A statement is IN SCOPE only if the GRAMMAR accepts it; a function that needs other arity or + * types simply is not exercised by this shape. That makes the sweep partial, which is why the + * next test asserts what it must contain. + * + * šŸ”“ `parseUnvalidated`, not `apply` (issue #389, round 3). This file measures a RENDER property + * -- does a statement survive its own `.sql`? -- and that is a property of the AST, not of + * whether a validation rule admits the statement. Keying it on `apply` coupled it to + * `validate()`: when #389 taught HAVING which key predicates a bucket filter can express, ~120 + * rows silently dropped OUT of the sweep and the HAVING venue -- the half that carried #365's + * loud failure (`COUNT(x)(COUNT(x))`) -- stopped being exercised at all, while the file stayed + * green. `parseUnvalidated` is `Parser.single`'s own action, so the AST is built exactly as + * `apply` builds it; only the validation pass is skipped. */ private def accepted: List[(String, String)] = for { name <- spellings sql <- statements(name) - if Parser(sql).isRight + if Parser.parseUnvalidated(sql).isRight } yield (name, sql) "the render of a function on the left of IN" should "re-parse to the same statement" in { val broken = accepted.flatMap { case (_, sql) => - val rendered = Parser(sql).map(_.sql).getOrElse("") - Parser(rendered) match { + val rendered = Parser.parseUnvalidated(sql).map(_.sql).getOrElse("") + Parser.parseUnvalidated(rendered) match { case Left(e) => Some(s" $sql\n renders $rendered\n re-parse REJECTED: ${e.toString.take(70)}") case Right(again) if again.sql != rendered => diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala new file mode 100644 index 00000000..77b06c31 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -0,0 +1,766 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.parser.Parser +import org.scalatest.OptionValues +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Issue #389 -- a `HAVING` over a FUNCTION of an aggregate must FILTER, or FAIL, never vanish. + * + * This file measures the `sql` half: the DETECTION (does the engine see the aggregate hidden in a + * function argument, a `CASE` branch, or the right-hand operand?), the REPRESENTABILITY gate (is + * the rendering a single boolean expression?) and the REFUSAL each un-expressible shape earns. The + * emission itself is pinned on the GENERATED query, in the bridge suites. + * + * šŸ”“ Every assertion here is at the PARSE/RENDER level. `Parser.apply` runs `update()` and then + * `validate()`, so a `Left` here is what every venue sees -- REPL, JDBC, Flight, the + * materialized-view extension and the bridge's own `SelectStatement(sql).query` alike. + */ +class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with OptionValues { + + private def parsed(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + private def rejection(sql: String): String = Parser(sql) match { + case Left(e) => e.msg + case Right(s) => fail(s"[$sql] expected a rejection, got $s") + } + + /** The AST of a statement `validate()` may REFUSE -- `Parser.single` runs `update()` inside its + * own combinator action, so everything the detection reads is populated exactly as `apply` would + * have populated it. Needed because half the shapes below are (correctly) rejected. + */ + private def unvalidated(sql: String): SingleSearch = Parser.parseUnvalidated(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + private def having(sql: String): Criteria = + parsed(sql).having.flatMap(_.criteria).getOrElse(fail(s"[$sql] has no HAVING criteria")) + + private def havingOf(s: SingleSearch, sql: String): Criteria = + s.having.flatMap(_.criteria).getOrElse(fail(s"[$sql] has no HAVING criteria")) + + private def script(sql: String): String = + MetricSelectorScript.metricSelector(having(sql)) + + /** The pattern the `terms` filter would carry, asked of the REAL derivation. */ + private def includeOf(sql: String): Option[String] = { + val st = parsed(sql) + st.buckets.iterator + .map(b => havingOf(st, sql).includes(b, not = false, BucketIncludesExcludes()).regex) + .collectFirst { case Some(r) => r } + } + + private def excludeOf(sql: String): Option[String] = { + val st = parsed(sql) + st.buckets.iterator + .map(b => havingOf(st, sql).excludes(b, not = false, BucketIncludesExcludes()).regex) + .collectFirst { case Some(r) => r } + } + + private val group = "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING " + + // ------------------------------------------------------------------------------------------- + // Detection -- `Identifier.bucketMetrics` is the ONE derivation behind the selector, the + // aggregations that get created, and the `buckets_path` that publishes them. + // ------------------------------------------------------------------------------------------- + + private def metricKeys(sql: String): Seq[String] = + havingOf(unvalidated(sql), sql).referencedIdentifiers + .flatMap(_.bucketMetrics) + .flatMap(_.metricName) + .distinct + + "the aggregate inside a function argument" should "be seen through every wrapper" in { + // Measured on `main`: every one of these answered `hasAggregation = false` and the whole + // condition degenerated to `1 == 1`. + metricKeys(group + "ABS(COUNT(*)) > 1") shouldBe Seq("c") + metricKeys(group + "COALESCE(COUNT(*), 0) > 1") shouldBe Seq("c") + metricKeys(group + "NULLIF(COUNT(*), 0) > 1") shouldBe Seq("c") + metricKeys(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq("c") + metricKeys(group + "FLOOR(COUNT(*)) > 1") shouldBe Seq("c") + metricKeys(group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1") shouldBe Seq("c") + } + + it should "be seen in a CASE branch and in a CASE condition" in { + // šŸ”“ The two live in DIFFERENT places and no single walk reaches both: `Case.args` excludes the + // WHEN conditions on purpose, so `funIdentifiers` finds the THEN result and misses the + // condition. Measured: the condition form yielded NO aggregate from `funIdentifiers` alone. + metricKeys(group + "CASE WHEN status = 'a' THEN COUNT(*) ELSE 0 END = 1") shouldBe Seq("c") + metricKeys(group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1") shouldBe Seq("c") + } + + it should "be seen on the RIGHT operand of the comparison" in { + // `1 < ABS(COUNT(*))` reads the metric through the VALUE, not the identifier. + metricKeys(group + "1 < ABS(COUNT(*))") shouldBe Seq("c") + metricKeys( + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > ABS(MAX(x))" + ) shouldBe Seq("c", "max_x") + } + + "a predicate that references no aggregate" should "still contribute no metric" in { + metricKeys(group + "status = 'A'") shouldBe empty + metricKeys(group + "UPPER(status) = 'A'") shouldBe empty + } + + "a SELECT bucket_script referenced by its alias" should "publish the alias, never its operands" in { + // The control the third arm must not swallow: the selector reads the sibling pipeline + // aggregation `d` by name; publishing `max_x` / `min_x` here would change a shipped query. + val sql = "SELECT status, MAX(x) - MIN(x) AS d FROM t GROUP BY status HAVING d > 3" + having(sql).extractAllMetricsPath shouldBe Map("d" -> "d") + script(sql) shouldBe "(params.d == null ? false : (params.d > 3))" + } + + // ------------------------------------------------------------------------------------------- + // Emission -- what the selector renders once the aggregate is seen + // + // šŸ”“ The emittable population is the one that MEASURABLY RUNS. `GREATEST` / `LEAST` / `COALESCE` + // emit; `ABS` and the rest of the boxing family are REFUSED, because their rendering does not + // compile in the bucket-pipeline script context -- see the disqualifier tests below. + // ------------------------------------------------------------------------------------------- + + "a null-PROPAGATING function of an aggregate" should "render guarded, exactly as a bare aggregate does" in { + // SQL says `GREATEST(NULL, 0) > 1` is UNKNOWN, so the group is excluded -- which is the `false` + // this guard supplies. Without it `Math.max(null, 0)` fails the whole search. + script(group + "GREATEST(COUNT(*), 0) > 1") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) > 1))" + script(group + "LEAST(COUNT(*), 99) > 1") shouldBe + "(params.c == null ? false : (Math.min(params.c, 99) > 1))" + script(group + "1 < GREATEST(COUNT(*), 0)") shouldBe + "(params.c == null ? false : (1 < Math.max(params.c, 0)))" + } + + "a null-ABSORBING function of an aggregate" should "render UNguarded" in { + // šŸ”“ `COALESCE` exists precisely to decide what a null means. Forcing `false` on it would make + // `COALESCE(MAX(x), 99) > 1` answer `false` where SQL says `true`. + script(group + "COALESCE(COUNT(*), 0) > 1") shouldBe "(params.c != null ? params.c : 0) > 1" + } + + "BETWEEN and IN over a function of an aggregate" should "render as one guarded boolean" in { + script(group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5") shouldBe + "(params.c == null ? false : ((Math.max(params.c, 0) >= 1 && Math.max(params.c, 0) <= 5)))" + script(group + "GREATEST(COUNT(*), 0) IN (1, 2)") shouldBe + "(params.c == null ? false : ((Math.max(params.c, 0) == 1 || Math.max(params.c, 0) == 2)))" + } + + "NOT over a function of an aggregate" should "push the negation into the comparison" in { + script(group + "NOT GREATEST(COUNT(*), 0) > 1") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) <= 1))" + } + + "a conjunction of two expressible predicates" should "emit BOTH" in { + // šŸ”“ On `main` the second conjunct was DROPPED and the answer looked filtered while being + // wrong -- a partial filter is a wrong answer, not a lesser fix. + script(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") shouldBe + "(params.c == null ? false : (params.c > 1)) && " + + "(params.c == null ? false : (Math.max(params.c, 0) > 2))" + } + + "a disjunction of two expressible predicates" should "emit BOTH" in { + // The OR form is WORSE than the AND form when a branch is dropped: the surviving filter is + // STRICTER than what was written, so rows silently disappear. + script(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) > 1)) || " + + "(params.c == null ? false : (params.c > 5))" + } + + "an aggregate reached only through a HAVING function" should "be created and published" in { + // Issue #389 AC-5: with no SELECT alias there was no aggregation for the script to read. + val sql = "SELECT status FROM t GROUP BY status HAVING GREATEST(COUNT(*), 0) > 1" + parsed(sql).sqlAggregations.keys.toList shouldBe List("count_all") + having(sql).extractAllMetricsPath shouldBe Map("count_all" -> "count_all") + } + + "an aggregate reached only through the RIGHT operand" should "be created and guarded" in { + // Measured on `main`: the script read `params.max_x`, `buckets_path` declared only `c`, and NO + // `max_x` aggregation existed anywhere. + val sql = + "SELECT status, COUNT(*) AS c FROM t GROUP BY status HAVING COUNT(*) > GREATEST(MAX(x), 0)" + parsed(sql).sqlAggregations.keys.toList shouldBe List("c", "max_x") + having(sql).extractAllMetricsPath shouldBe Map("c" -> "c", "max_x" -> "max_x") + script(sql) shouldBe + "(params.c == null || params.max_x == null ? false : " + + "(params.c > Math.max(params.max_x, 0)))" + } + + // ------------------------------------------------------------------------------------------- + // The representability gate -- one refusal per disqualifier, each naming its own reason + // ------------------------------------------------------------------------------------------- + + "a rendering that can evaluate to NULL" should "be refused, naming the boolean requirement" in { + val msg = rejection(group + "NULLIF(COUNT(*), 0) > 1") + msg should include("HAVING cannot be applied to NULLIF(COUNT(*), 0) > 1") + msg should include("can evaluate to NULL") + } + + it should "be refused through an outer function too" in { + rejection(group + "COALESCE(NULLIF(COUNT(*), 0), 5) > 1") should include("can evaluate to NULL") + } + + "a rendering that needs a local declaration" should "be refused, naming the expression requirement" in { + val msg = rejection( + "SELECT status, MAX(amount) AS m FROM t GROUP BY status HAVING ROUND(MAX(amount), 2) > 1" + ) + msg should include("HAVING cannot be applied to ROUND(MAX(amount), 2) > 1") + msg should include("needs a local declaration") + } + + "a rendering that BOXES a number" should "be refused, naming the script context" in { + // šŸ”“ The disqualifier no amount of reading could have produced, and the reason this issue's + // acceptance bar was EXECUTION. `Double.valueOf(Math.abs(params.c)) > 1` is syntactically a + // perfectly good boolean expression -- and on a real Elasticsearch 8.18.3 it is a COMPILE error + // (`Cannot cast from [java.lang.Double] to [int]`) that fails the whole search. The same call + // compiles in the QUERY script context, so it is the bucket-pipeline context that refuses it. + // Derived, not guessed: eighteen shapes were rendered and executed, and `.valueOf(` separates + // the nine that run from the nine that do not, exactly. + Seq("ABS", "FLOOR", "CEIL", "SQRT", "EXP", "LOG").foreach { fn => + withClue(s"[$fn] ") { + rejection(group + s"$fn(COUNT(*)) > 1") should include("boxes a number") + } + } + rejection(group + "POWER(COUNT(*), 2) > 1") should include("boxes a number") + } + + "a function whose rendering boxes nothing" should "still emit" in { + // The complement that proves the rule above is not simply "refuse every math function": + // `SIGN` renders a bare ternary, and it runs. + script(group + "SIGN(COUNT(*)) > 0") shouldBe + "(params.c == null ? false : ((params.c > 0 ? 1 : (params.c < 0 ? -1 : 0)) > 0))" + } + + "a rendering that needs a document" should "be refused" in { + // A `CASE` renders only against a Painless context; a bucket pipeline has no document. + rejection(group + "CASE WHEN COUNT(*) > 1 THEN 1 ELSE 0 END = 1") should include( + "cannot be rendered without a document" + ) + } + + "an aggregate ALIAS used inside a function argument" should "be refused by name" in { + // `Having.resolveAggregateAliases` substitutes an OPERAND only, so `c` inside `NULLIF(c, 0)` + // stays a bare column and renders `arg0`, a context parameter nothing binds. + val msg = rejection(group + "NULLIF(c, 0) > 1") + msg should include("aggregate alias 'c'") + msg should include("NULLIF(c, 0)") + } + + "a conjunction mixing an expressible and an un-expressible predicate" should "refuse the whole statement" in { + // šŸ”“ The alternative is a PARTIAL filter, which is a wrong answer dressed as a right one. + rejection(group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2") should include( + "can evaluate to NULL" + ) + rejection(group + "NULLIF(COUNT(*), 0) > 2 AND COUNT(*) > 1") should include( + "can evaluate to NULL" + ) + rejection(group + "COUNT(*) > 1 OR NULLIF(COUNT(*), 0) > 2") should include( + "can evaluate to NULL" + ) + } + + "the whole-table HAVING" should "follow the same rule" in { + script("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe + "(params.c == null ? false : (Math.max(params.c, 0) > 1))" + rejection("SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1") should include( + "can evaluate to NULL" + ) + } + + "a nested-relation HAVING" should "follow the same rule" in { + val sql = "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING GREATEST(COUNT(e.address), 0) > 1" + parsed(sql).sqlAggregations.keys.toList shouldBe List("e.filtered_agg.count_e_address") + script(sql) shouldBe + "(params.count_e_address == null ? false : (Math.max(params.count_e_address, 0) > 1))" + } + + it should "be REFUSED inside the relation too, not only at the flat level" in { + // The walk recurses through `ElasticRelation`, so a nested predicate the engine cannot express + // is refused rather than silently dropped -- the nested level is where the ORIGINAL drop was + // most damaging (measured on `main`: the whole aggregation tree vanished and raw documents came + // back). + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING NULLIF(COUNT(e.address), 0) > 1" + ) should include("can evaluate to NULL") + } + + // ------------------------------------------------------------------------------------------- + // Each gate rule, falsified on its OWN input + // + // šŸ”“ The rules OVERLAP on real SQL -- every statement that trips the top-level-conditional rule + // also renders a `? null :`, and every statement carrying a free local also carries one. A + // mutation matrix driven by statements alone therefore reports live rules as unguarded, which is + // the "a guard never seen RED is a hypothesis" trap. `disqualifyRendering` is a pure function of + // a string, so each rule gets an input only IT can classify. + // ------------------------------------------------------------------------------------------- + + private def reason(rendering: String): Option[String] = + MetricSelectorScript.disqualifyRendering(rendering) + + "the statement rule" should "refuse a declaration and accept a call chain" in { + reason("def x = 1; x > 1").value should include("local declaration") + reason("params.c > 1") shouldBe None + } + + "the NULL rule" should "refuse a null-producing ternary anywhere in the rendering" in { + reason("(params.c) == 0 ? null : (params.c) > 1").value should include("evaluate to NULL") + reason("((params.c) == 0 ? null : 5) > 1").value should include("evaluate to NULL") + } + + "the top-level-conditional rule" should "refuse a rendering whose outermost operator is `?`" in { + // No SQL statement reaches this rule today -- every conditional the renderer emits is either + // parenthesised (`SIGN`) or null-producing (`NULLIF`), so the two rules above catch them + // first. It is kept because the gate's contract is to prove a BOOLEAN, and a bare ternary is + // not one; this row is the only thing that can hold it honest. + reason("params.c > 0 ? 1 : 2").value should include("outermost operator is a conditional") + reason("(params.c > 0 ? 1 : 2) > 1") shouldBe None + } + + "the boxing rule" should "refuse a valueOf and accept the same call without it" in { + reason("Double.valueOf(Math.abs(params.c)) > 1").value should include("boxes a number") + reason("Math.abs(params.c) > 1") shouldBe None + } + + "the text-comparison rule" should "refuse a metric compared as text and accept a numeric one" in { + // šŸ”“ MEASURED on Elasticsearch 8.18.3: a date metric arrives from `buckets_path` as epoch + // millis, so `(params.max_d != null ? params.max_d : "2020-01-01").compareTo("2019-01-01") > 0` + // fails with `cannot explicitly cast def [java.lang.String] to java.lang.Double`. + reason( + """(params.max_d != null ? params.max_d : "2020-01-01").compareTo("2019-01-01") > 0""" + ).value should + include("compares a metric as text") + reason("params.c > 1") shouldBe None + } + + "the free-local rule" should "refuse an unbound name and accept methods, classes and literals" in { + reason("arg0 > 1").value should include("reads 'arg0'") + reason("Double.parseDouble(params.c) > 1") shouldBe None + // `null` / `true` / `false` are Painless LITERALS, not free names -- allowing them is not a + // whitelist of the renderer's vocabulary, which is the thing this rule must not become. + reason("(params.c == null ? false : true) && params.c != null") shouldBe None + } + + "every rule" should "read CODE and never the inside of a string literal" in { + // šŸ”“ Without the literal blanking, each of these is refused by a DIFFERENT rule, and each of + // them is a perfectly good script. Measured on a real Elasticsearch 8.18.3: the first form + // runs. + // āš ļø Deliberately NOT a `.compareTo("...")` payload: that is itself a disqualifier now (a + // metric arrives as a number), so such a row could never isolate the blanking. + reason("""params.c > 1 && params.c != "a; b"""") shouldBe None + reason("""params.c > 1 && params.c != "a ? null : b"""") shouldBe None + reason("""params.c > 1 && params.c != "def x"""") shouldBe None + reason("""params.c > 1 && params.c != 'arg0'""") shouldBe None + } + + "an un-expressible criterion" should "make the selector THROW, never return a partial script" in { + // šŸ”“ The second line of defence. `validate()` refuses every such statement inside + // `Parser.apply`, so this is reachable only for a `SingleSearch` assembled in code and emitted + // without validation -- exactly the route on which `""` used to mean "nothing to filter". + val sql = group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2" + val criteria = havingOf(unvalidated(sql), sql) + val thrown = intercept[IllegalStateException] { + MetricSelectorScript.metricSelector(criteria) + } + thrown.getMessage should include("HAVING cannot be applied to") + // ... and it must not have quietly emitted the OTHER half instead. + thrown.getMessage should not include "params.c > 1" + } + + it should "be reported by name from either side of a conjunction" in { + Seq( + group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2", + group + "NULLIF(COUNT(*), 0) > 2 AND COUNT(*) > 1", + group + "COUNT(*) > 1 OR NULLIF(COUNT(*), 0) > 2" + ).foreach { sql => + withClue(s"[$sql] ") { + val u = unvalidated(sql).having.map(_.unrepresentable).getOrElse(Nil) + u should have size 1 + u.head.reason should include("evaluate to NULL") + } + } + } + + "a function of an aggregate over a plain COLUMN" should "be refused by name" in { + // The free-local rule's own SQL witness: the second operand is a document field, which a + // bucket pipeline cannot read at all. + rejection(group + "GREATEST(COUNT(*), status) > 1") should include("reads 'arg1'") + } + + // ------------------------------------------------------------------------------------------- + // Controls -- shapes that must NOT move + // ------------------------------------------------------------------------------------------- + + "a bare aggregate predicate" should "render exactly as it always did" in { + script(group + "COUNT(*) > 1") shouldBe "(params.c == null ? false : (params.c > 1))" + script(group + "c > 1") shouldBe "(params.c == null ? false : (params.c > 1))" + } + + "a predicate over the bucket KEY" should "still contribute nothing to the selector" in { + // It is honoured by the `terms` include / exclude, which is a DIFFERENT mechanism. Refusing it + // here would break every `HAVING status = 'A'` shipped today. + script(group + "status = 'A'") shouldBe "1 == 1" + Parser(group + "status = 'A'").isRight shouldBe true + } + + // ------------------------------------------------------------------------------------------- + // The GROUP BY key population (round 3 -- the document push-down is GONE) + // ------------------------------------------------------------------------------------------- + + "a key predicate the terms filter expresses" should "be classified TermsFilter and left to it" in { + // The ONE classifier both emission paths read. `TermsFilter` means the existing `terms` + // include/exclude call applies it, so the selector sees nothing. + val st = parsed(group + "status = 'a'") + val leaf = st.havingLeaves.head + st.keyPredicateOutcome(leaf) shouldBe st.KeyPredicateOutcome.TermsFilter + script(group + "status = 'a'") shouldBe "1 == 1" + } + + "a key predicate neither bucket mechanism expresses" should "be REFUSED, never applied to documents" in { + // šŸ”“ ROUND 3. This used to be pushed into the QUERY as a document filter, and that is unsound + // twice over -- MEASURED on Elasticsearch 8.18.3: + // `GROUP BY DAY(d) HAVING YEAR(d) = 2025` kept bucket `3` and changed its count 3 -> 1; + // `GROUP BY tags HAVING UPPER(tags) = 'A'` kept bucket `b`, which FAILS the predicate, + // because a multi-valued document is kept or dropped whole. + // The second is strictly less sound than the `include` path it was meant to generalise. + Seq("UPPER(status) = 'A'", "LENGTH(status) = 1", "SUBSTRING(status, 1, 1) = 'a'").foreach { p => + withClue(s"[$p] ") { + rejection(group + p) should include("terms filter, which can only express") + } + } + } + + it should "be refused on a FUNCTION-of-column key too, where the push-down silently miscounted" in { + rejection( + "SELECT DAY(d) AS dd, COUNT(*) AS c FROM t GROUP BY DAY(d) HAVING YEAR(d) = 2025" + ) should include("HAVING cannot filter on") + } + + it should "name the nested case separately" in { + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name HAVING UPPER(e.name) = 'X'" + ) should include("nested object") + } + + "a HAVING that reads neither an aggregate nor a GROUP BY key" should "be refused by name" in { + rejection(group + "UPPER(name) = 'X'") should include("can only filter on a GROUP BY key") + rejection(group + "name = 'x'") should include("can only filter on a GROUP BY key") + } + + it should "be refused with NO GROUP BY as well" in { + // šŸ”“ Review MEDIUM-4: the rule was gated on `groupBy.isDefined`, so S8's original hole stayed + // open for a FUNCTION of a column. MEASURED: `SELECT COUNT(*) AS c FROM t HAVING + // UPPER(status) = 'A'` answered `{"c":{"value":4}}` with the predicate gone, while the bare + // `HAVING status = 'a'` was correctly refused. + rejection("SELECT COUNT(*) AS c FROM t HAVING UPPER(status) = 'A'") should include( + "can only filter on a GROUP BY key" + ) + } + + // ------------------------------------------------------------------------------------------- + // The include/exclude CHANNEL -- it unions, it never intersects (rule b2) + // + // šŸ”“ `Criteria.excludes` IS `Criteria.includes(bucket, !not, ā€¦)`: one method read in two senses. + // A rule about these channels written for one sense is wrong for the other by construction -- + // that is exactly how an abandoned round shipped an inverted rule that turned `<> a OR <> b` + // from a dropped predicate into a WRONG answer. So the matrix below is DERIVED and covers both + // senses; the population, not the assertion, is what was missing. + // ------------------------------------------------------------------------------------------- + + /** Every key operator, with the CHANNEL it feeds and the FORM it puts there. + * + * šŸ”“ The first version of this matrix paired each operator with a fixed partner (`=` with `=`, + * `IN` with `=`, `LIKE` with `LIKE`) and varied only channel Ɨ combiner, so `=` was NEVER + * crossed with `LIKE` -- which is exactly where the defect lived. The operators are CROSSED now. + * Every defect found on this branch was a hole in the population, never in the assertions. + */ + private val keyOperators = Seq( + ("status = 'a'", "include", "values"), + ("status IN ('a','x')", "include", "values"), + ("status LIKE 'a%'", "include", "regex"), + ("status <> 'a'", "exclude", "values"), + ("status NOT IN ('a','x')", "exclude", "values"), + ("status NOT LIKE 'a%'", "exclude", "regex") + ) + + /** The second operand, so a crossed cell never repeats its partner verbatim. */ + private def partnerOf(predicate: String): String = + predicate.replace("'a'", "'b'").replace("'a%'", "'b%'") + + "the terms channel" should "express every CROSSED combination of key predicates, or refuse it" in { + // The verdict is DERIVED from two properties of the channel, never listed by hand: + // 1. the kept list is a union (a DISJUNCTION) and the removed list a union of negations (a + // CONJUNCTION), so two same-channel leaves need OR and AND respectively; + // 2. a channel holds ONE list and ONE pattern, and the pattern REPLACES the list -- so a + // pattern meeting anything else in the same channel loses a side whatever the operator. + val cells = + for { + (left, leftChannel, leftForm) <- keyOperators + (right, rightChannel, rightForm) <- keyOperators + combiner <- Seq("AND", "OR") + } yield { + val rhs = partnerOf(right) + val sameChannel = leftChannel == rightChannel + // āš ļø COARSER THAN PRODUCTION, deliberately. `keyChannelConflict` exempts two IDENTICAL + // patterns (one pattern expresses both); this model cannot express that, and is sound + // here only because `partnerOf` makes every crossed partner differ. Do not read it as the + // production rule -- the exemption has its own test above. + val collides = + sameChannel && (leftForm == "regex" || rightForm == "regex") + val expressible = + !collides && ( + (sameChannel && leftChannel == "include" && combiner == "OR") || + (sameChannel && leftChannel == "exclude" && combiner == "AND") || + (!sameChannel && combiner == "AND") + ) + (s"$left $combiner $rhs", expressible) + } + cells.distinct.size shouldBe 72 + cells.distinct.foreach { case (predicate, expressible) => + withClue(s"[$predicate] expressible=$expressible ") { + Parser(group + predicate).isRight shouldBe expressible + } + } + } + + it should "refuse a pattern that meets a value list in the SAME channel" in { + // šŸ”“ MEASURED on ES 8.18.3 over the buckets `a`, `b1`, `c`: + // `HAVING status = 'a' OR status LIKE 'b%'` emitted `include:"b.*"` and returned ['b1']; + // the SQL means ['a','b1']. The emission keeps the PATTERN and discards the list. + // āš ļø It is an OR of two KEPT-value contributions -- the very row "the kept list is a union" + // blesses -- so the union property alone is NOT sufficient, and an earlier draft of this + // branch's own documentation said it was. + Seq( + "status = 'a' OR status LIKE 'b%'", + "status LIKE 'b%' OR status IN ('a','c')", + "status <> 'a' AND status NOT LIKE 'b%'", + "status NOT LIKE 'b%' AND status NOT IN ('a','c')", + "status LIKE 'a%' OR status LIKE 'b%'" + ).foreach { p => + withClue(s"[$p] ") { + rejection(group + p) should include("a pattern replaces the list") + } + } + } + + "a LIKE pattern on the key" should "go through the SHARED translation, not a third private one" in { + // šŸ”“ The terms channel was a THIRD derivation of LIKE -> regex: `value.replaceAll("%", ".*")`. + // It neither translated `_` nor escaped a metacharacter, while the query-DSL path uses the + // shared `toRegex` -- and `metricSelector`'s own scaladoc asserts the shared one is used. + // MEASURED on ES 8.18.3 over the buckets `a.bZ`, `axbZ`, `ab`, `a1`: + // `status LIKE 'a_'` WHERE -> [a1, ab] HAVING -> NO BUCKETS + // `status LIKE 'a.b%'` WHERE -> [a.bZ] HAVING -> [a.bZ, axbZ] + // Pre-existing and byte-identical to `455433ae`; fixed here because it is a silent wrong + // answer in the very channel this branch reasons about. + includeOf(group + "status LIKE 'a_'").value shouldBe "a." + includeOf(group + "status LIKE 'a.b%'").value shouldBe "a\\.b.*" + includeOf(group + "status LIKE 'a+b%'").value shouldBe "a\\+b.*" + excludeOf(group + "status NOT LIKE 'a_b'").value shouldBe "a.b" + // ... and the shapes with neither stay byte-identical. + includeOf(group + "status LIKE 'a%'").value shouldBe "a.*" + includeOf(group + "status LIKE '%a%'").value shouldBe ".*a.*" + } + + it should "leave RLIKE alone, which is RAW regex by definition" in { + includeOf(group + "status RLIKE 'a.b'").value shouldBe "a.b" + excludeOf(group + "status NOT RLIKE 'a.*'").value shouldBe "a.*" + } + + "two IDENTICAL patterns on one key" should "NOT collide, because one pattern expresses both" in { + // šŸ”“ F4: the `a.regex != b.regex` exemption in `collides` was live and correct but unpinned. + // `LIKE 'a%' OR LIKE 'a%'` is one pattern twice: nothing is dropped, so nothing is refused. + Parser(group + "status LIKE 'a%' OR status LIKE 'a%'").isRight shouldBe true + includeOf(group + "status LIKE 'a%' OR status LIKE 'a%'").value shouldBe "a.*" + // ... while two DIFFERENT patterns in one channel do collide. + rejection(group + "status LIKE 'a%' OR status LIKE 'b%'") should include( + "a pattern replaces the list" + ) + } + + it should "still allow a pattern in EACH channel, which do not collide" in { + Parser(group + "status LIKE 'a%' AND status NOT LIKE 'b%'").isRight shouldBe true + } + + "an OR across TWO GROUP BY keys" should "be refused, because the levels are NESTED" in { + // šŸ”“ MEASURED on ES 8.18.3 over (a,a) (a,b) (x,b) (x,y): + // `GROUP BY status, city HAVING status = 'a' OR city = 'b'` + // -> terms status include:["a"] > terms city include:["b"] -> ONE group (a,b), + // and the SQL means THREE. + // `mechanismOf` labels every grouping level "key", so the round-3 rule saw one mechanism and + // passed. Nesting two `terms` aggregations IS the conjunction that rule exists to catch. + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + val msg = rejection(two + "status = 'a' OR city = 'b'") + msg should include("DIFFERENT GROUP BY keys") + msg should include("nests one grouping level inside the other") + // ... and the message must NOT reuse the mechanism wording, which is about a group filter, + // a key filter and a nested filter -- not about two grouping levels. + msg should not include "different mechanisms" + } + + it should "still allow an AND across two grouping keys, which IS the nesting" in { + val two = "SELECT status, city, COUNT(*) AS c FROM t GROUP BY status, city HAVING " + Parser(two + "status = 'a' AND city = 'b'").isRight shouldBe true + } + + it should "leave a SINGLE key predicate of either sense alone" in { + keyOperators.map(_._1).foreach { p => + withClue(s"[$p] ")(Parser(group + p).isRight shouldBe true) + } + } + + it should "keep the AND of inequalities that works on `455433ae`, byte for byte" in { + // šŸ”“ The row this rule must NOT move: `exclude:["a","b"]` is CORRECT, because NOT-in-A and + // NOT-in-B is NOT-in-(A āˆŖ B). An earlier, inverted rule refused it -- a regression against + // `main` that the docs then contradicted. + val st = parsed(group + "status <> 'a' AND status <> 'b'") + st.buckets + .map(b => havingOf(st, "and").excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("a", "b")) + // ... and the three-leaf chain, which is the same union one level deeper. + val three = parsed(group + "status <> 'a' AND status <> 'b' AND status <> 'c'") + three.buckets + .map(b => havingOf(three, "and3").excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("a", "b", "c")) + } + + it should "refuse the OR of inequalities that is WRONG on `455433ae`" in { + // šŸ”“ MEASURED on `455433ae` and on ES 8.18.3: `<> 'a' OR <> 'b'` emitted the SAME + // `exclude:["a","b"]` as the AND. The disjunction is true for EVERY bucket, and two were + // dropped, HTTP 200. Live on `main` today. + rejection(group + "status <> 'a' OR status <> 'b'") should include("cannot combine") + rejection(group + "status NOT IN ('a','b') OR status <> 'c'") should include("cannot combine") + } + + it should "refuse the AND of equalities, whose include list means OR" in { + // `include:["a","b"]` keeps two buckets; the SQL means none. DECISION: refused, not + // "corrected" to an empty result -- a loud refusal is the contract this issue is about. + rejection(group + "status = 'a' AND status = 'b'") should include("cannot combine") + } + + it should "refuse a MIXED pair under OR, which the emission would AND" in { + // `= 'a' OR <> 'b'` emitted `include:["a"]` AND `exclude:["b"]` on `455433ae` -- a conjunction + // where the SQL says disjunction. + rejection(group + "status = 'a' OR status <> 'b'") should include("cannot combine") + } + + it should "not refuse a conjunction the channels express SEPARATELY" in { + // One include and one exclude under AND is exactly what the two lists mean together. + Parser(group + "status = 'a' AND status <> 'b'").isRight shouldBe true + Parser(group + "status IN ('a','b') AND status <> 'c'").isRight shouldBe true + // ... and a key predicate beside a METRIC is a different mechanism, not a second channel. + Parser(group + "status <> 'a' AND COUNT(*) > 1").isRight shouldBe true + } + + it should "still place a NOT before a LEAF in the right channel" in { + // The polarity threading that IS reachable: `NOT = 'b'` feeds the EXCLUDE list, so the pair is + // one include and one exclude under a conjunction -- expressible. + val st = parsed(group + "status = 'a' AND NOT status = 'b'") + st.buckets + .map(b => havingOf(st, "notleaf").includes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("a")) + st.buckets + .map(b => havingOf(st, "notleaf").excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(Set("b")) + // šŸ”“ And the shape that would need De Morgan does not exist: `NOT ( ā€¦ )` around a group is + // rejected by the grammar, here and on `455433ae`. MEASURED -- which is why the rule carries + // no arm for it. + Parser(group + "NOT (status = 'x' OR status = 'y')").isLeft shouldBe true + } + + // ------------------------------------------------------------------------------------------- + // The OR rule -- homogeneity of MECHANISM (round 3) + // ------------------------------------------------------------------------------------------- + + "an OR whose branches need different mechanisms" should "be refused, because it executes as an AND" in { + // šŸ”“ MEASURED on `main`: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO buckets where the + // disjunction is two of them -- the key became a terms `include` (removing one) and the metric + // a `bucket_selector` (removing the other), so the OR answered the empty AND. + val msg = rejection(group + "COUNT(*) > 1 OR status = 'b'") + msg should include("different mechanisms") + msg should include("key") + msg should include("metric") + } + + it should "name the mechanisms it actually found, not an aggregate that is not there" in { + // šŸ”“ Review MEDIUM-5: the old message said "cannot OR a GROUP BY key with an aggregate" even + // for a statement containing NO aggregate. The message now names what it found. + rejection(group + "COUNT(*) > 1 OR status = 'b'") should include("mixes key and metric") + // ... and a key predicate neither mechanism expresses is refused for its OWN reason first, + // which is the more actionable one. + rejection(group + "status = 'a' OR UPPER(status) = 'B'") should include( + "terms filter, which can only express" + ) + } + + it should "close the NESTED pair too, which the round-2 rule left executing as an AND" in { + // šŸ”“ The nested filter aggregation is a THIRD mechanism. The round-2 rule dropped `_.nested` + // leaves before counting, so this pair was invisible to it and still executed as an AND. + rejection( + "SELECT e.name, COUNT(*) AS c FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING COUNT(*) > 1 OR e.name = 'x'" + ) should include("mixes metric and nested") + } + + it should "accept an OR the terms filter UNIONS into one include list" in { + // šŸ”“ The complement, and the mutation-killer for the rule's second clause: `include: [a, b]` + // IS the disjunction. + Parser(group + "status = 'a' OR status = 'b'").isRight shouldBe true + } + + it should "accept an OR of two metric conditions" in { + Parser(group + "COUNT(*) > 1 OR COUNT(*) > 5").isRight shouldBe true + } + + "Having.script" should "answer None for a clause the gate refuses, never a partial filter" in { + // šŸ”“ NOT dead code: `softclient4es-extensions` reads it to build a materialized view's + // `TransformBucketSelectorConfig` (`graph/Stage.scala:291`). Round 1 recorded it as having no + // production caller and that was WRONG -- it is a FIFTH HAVING mechanism, outside this repo. + // + // The contract core can offer: never hand the extension a script for a clause the engine + // cannot express (a PARTIAL filter inside an MV), and never THROW (a 500 inside the extension). + // `unrepresentable` is public so the extension can refuse for itself. + val refused = unvalidated(group + "NULLIF(COUNT(*), 0) > 1").having.get + refused.unrepresentable should not be empty + refused.script shouldBe None + val ok = unvalidated(group + "COUNT(*) > 1").having.get + ok.unrepresentable shouldBe empty + ok.script shouldBe Some("(params.c == null ? false : (params.c > 1))") + } + + "a metric computed outside the nested grouping" should "be refused rather than vanish" in { + // šŸ”“ The issue's SECOND conflation site. MEASURED on the branch before this rule: the statement + // answered HTTP 200 with `{"aggs":{"max_amount":{"max":{"field":"amount"}}}}` -- no + // `having_filter` AND no grouping at all. (The vanished GROUP BY is a separate pre-existing + // defect: the control answers `{"query":{"match_all":{}},"_source":true}` for the same input.) + rejection( + "SELECT e.name FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name " + + "HAVING COALESCE(MAX(amount), 0) > 1" + ) should include("computed outside the nested grouping") + } + + "an aggregate written in WHERE" should "still be rejected, now through the wider walk" in { + Parser("SELECT status FROM t WHERE COUNT(*) > 1 GROUP BY status").isLeft shouldBe true + // NEW: the function-wrapped form is rejected too. It used to reach the bridge, where an + // aggregate has no document-level query form at all. + rejection("SELECT status FROM t WHERE ABS(COUNT(*)) > 1 GROUP BY status") should include( + "Aggregate functions are not allowed in WHERE" + ) + } +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index 187b02d1..62d81e38 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -29,6 +29,8 @@ import org.scalatest.flatspec.AnyFlatSpecLike import org.scalatest.matchers.should.Matchers import org.slf4j.{Logger, LoggerFactory} +import scala.concurrent.Await +import scala.concurrent.duration._ import scala.language.implicitConversions case class CategoryCount(category: String, cnt: Long) @@ -697,6 +699,390 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + // ------------------------------------------------------------------ + // Issue #389 -- a HAVING over a FUNCTION of an aggregate must FILTER, or FAIL. + // + // MEASURED on `main`: every shape below parsed, ran, and returned EVERY group with HTTP 200 -- + // the `bucket_selector` was never emitted because a function does not look inside its own + // arguments. These rows EXECUTE the emitted script against a real cluster, because the claim is + // that Elasticsearch COMPILES and RUNS it: `cat_i` holds exactly `i` documents, so the filtered + // answer and the unfiltered one differ by construction (7 groups against 37). + // + // šŸ”“ EXECUTION is what set the emittable population, not reading. `ABS(COUNT(*)) > 1` renders a + // perfectly good-looking boolean expression and Elasticsearch REFUSES to compile it + // (`Double.valueOf` in a bucket-pipeline script). It is in the refusal list below, where the + // cluster put it. + // ------------------------------------------------------------------ + + private val over30 = 7 // cat_31 .. cat_37 + + "a HAVING over a function of an aggregate" should "filter the groups it names" in { + // āš ļø Unrolled, not looped: `searchAs` is a macro and needs a compile-time constant SQL string. + def check(label: String, result: ElasticResult[Seq[CategoryCount]], expected: Int): Unit = + result match { + case ElasticSuccess(rows) => + withClue(s"[$label] rows=${rows.map(_.category).sorted}: ") { + rows should have size expected.toLong + rows.map(_.category).toSet shouldBe + (categories - expected + 1 to categories).map(c => f"cat_$c%02d").toSet + } + case ElasticFailure(error) => fail(s"[$label] Query failed: ${error.message}") + } + + check( + "GREATEST over an aggregate", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING GREATEST(COUNT(*), 0) > 30" + ), + over30 + ) + check( + "COALESCE over an aggregate", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COALESCE(COUNT(*), 0) > 30" + ), + over30 + ) + check( + "the aggregate on the RIGHT of the comparison", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING 30 < GREATEST(COUNT(*), 0)" + ), + over30 + ) + check( + "BETWEEN over a function of an aggregate", + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING GREATEST(COUNT(*), 0) BETWEEN 31 AND 37" + ), + over30 + ) + } + + it should "emit BOTH halves of a conjunction" in { + // šŸ”“ On `main` the second conjunct was DROPPED, so this returned 37 groups: a PARTIAL filter, + // which looks filtered and is wrong. The oracle (cat_31..cat_34) differs from BOTH the + // unfiltered answer (37) and from either conjunct alone (7 and 34). + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 30 AND GREATEST(COUNT(*), 0) < 35" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category).toSet shouldBe Set("cat_31", "cat_32", "cat_33", "cat_34") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "create the aggregation when the aggregate appears ONLY in the HAVING function" in { + // No SELECT aggregate at all: on `main` there was no `count_all` aggregation for the script to + // read, so even a corrected script would have had nothing to compare. + client.searchAs[CategoryOnly]( + "SELECT category FROM group_by_completeness GROUP BY category HAVING GREATEST(COUNT(*), 0) > 30" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category).toSet shouldBe + (categories - over30 + 1 to categories).map(c => f"cat_$c%02d").toSet + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "filter the implicit whole-table group too" in { + implicit val wholeCtx: ConversionContext = NativeContext + // 703 documents in total: the TRUE predicate keeps the single row, the FALSE one removes it. + client.search( + SelectStatement( + "SELECT COUNT(*) AS c FROM group_by_completeness HAVING GREATEST(COUNT(*), 0) > 700" + ) + ) match { + case ElasticSuccess(response) => response.results should have size 1 + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + client.search( + SelectStatement( + "SELECT COUNT(*) AS c FROM group_by_completeness HAVING GREATEST(COUNT(*), 0) > 9999" + ) + ) match { + case ElasticSuccess(response) => response.results shouldBe empty + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "REFUSE the shapes it cannot express, rather than return every group" in { + // šŸ”“ The whole point of the issue: a predicate the engine cannot emit used to come back as + // HTTP 200 over UNFILTERED groups. Each of these must now be an error. + // + // āš ļø Asserted through `run`, not `search(SelectStatement(...))`: `SelectStatement` yields + // `statement = None` on a rejection and the client reports its own generic "does not contain a + // valid search request" message, which would hide WHICH rule fired. `run` is the REPL / JDBC / + // Flight route and relays the parser's reason (#262). + Seq( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING ABS(COUNT(*)) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING NULLIF(COUNT(*), 0) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING NULLIF(cnt, 0) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING CASE WHEN COUNT(*) > 30 THEN 1 ELSE 0 END = 1", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING ROUND(SUM(amount), 2) > 30", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 30 AND NULLIF(COUNT(*), 0) > 1", + // šŸ”“ The RIGHT-operand family (F1): the left operand is a bare aggregate, so the gate used to + // be skipped entirely and these reached Elasticsearch as HTTP 400 / an internal-error label. + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > ROUND(SUM(amount), 2)", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > NULLIF(SUM(amount), 0)", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) BETWEEN 1 AND ABS(SUM(amount))", + "SELECT category, SUM(amount) AS s FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > CASE WHEN SUM(amount) > 1 THEN 1 ELSE 0 END" + ).foreach { sql => + Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(result) => fail(s"[$sql] was accepted and answered $result") + case ElasticFailure(error) => + withClue(s"[$sql] ") { error.message should include("HAVING cannot") } + } + } + } + + it should "REFUSE a key condition the terms filter cannot express, rather than answer wrongly" in { + // šŸ”“ ROUND 3. Round 2 pushed these into the QUERY as a document filter. That is unsound twice, + // both MEASURED on real Elasticsearch: with a key that is a FUNCTION of the column a SURVIVING + // bucket's count changed (`GROUP BY DAY(d) HAVING YEAR(d) = 2025` kept bucket 3 and moved its + // count 3 -> 1), and on a MULTI-VALUED field a bucket that FAILS the predicate survived, + // because a document in several buckets is kept or dropped whole. The `terms` `include` path + // answers such a grouping correctly, so the push-down was strictly LESS sound than the + // mechanism it was meant to generalise. + Seq( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING UPPER(category) = 'CAT_37'", + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING SUBSTRING(category, 5, 2) = '37'" + ).foreach { sql => + Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(result) => fail(s"[$sql] was accepted and answered $result") + case ElasticFailure(error) => + withClue(s"[$sql] ") { error.message should include("HAVING cannot filter on") } + } + } + } + + it should "still apply a key condition the terms filter DOES express" in { + // The complement: this is the mechanism the refusal above defers to, and it must keep working. + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING category = 'cat_37'" + ) match { + case ElasticSuccess(rows) => + rows.map(r => r.category -> r.cnt) shouldBe Seq("cat_37" -> categories.toLong) + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "combine a key condition with an aggregate condition as an AND" in { + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 35 AND category <> 'cat_36'" + ) match { + case ElasticSuccess(rows) => rows.map(_.category).toSet shouldBe Set("cat_37") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "REFUSE an OR whose branches need different mechanisms" in { + // šŸ”“ PRE-EXISTING silent wrong answer: the key became a terms `include` and the aggregate a + // `bucket_selector`, so the OR was EXECUTED AS AN AND. Measured on `main` over a 4-document + // fixture: `HAVING COUNT(*) > 1 OR status = 'b'` returned NO groups where the disjunction is + // two of them. + Await.result( + client.run( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 35 OR category = 'cat_01'" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"the OR was accepted and answered $result") + case ElasticFailure(error) => error.message should include("different mechanisms") + } + } + + it should "still accept an OR the terms filter unions into one include list" in { + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING category = 'cat_36' OR category = 'cat_37'" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category).toSet shouldBe Set("cat_36", "cat_37") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + // ----------------------------------------------------------------------------------------- + // The include/exclude CHANNEL, EXECUTED (rule b2) + // + // šŸ”“ `excludes` IS `includes(bucket, !not, ā€¦)` -- one method, two senses -- and every other row + // in this file exercises the INCLUDE sense. These are the exclude-side cells, each against a + // bucket set computed by hand from the fixture: `cat_i` holds exactly `i` documents, 37 groups. + // ----------------------------------------------------------------------------------------- + + private def categoriesOf(having: String): Seq[String] = + client.searchAsUnchecked[CategoryCount]( + SelectStatement( + s"SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category $having" + ) + ) match { + case ElasticSuccess(rows) => rows.map(_.category).sorted + case ElasticFailure(error) => fail(s"[$having] failed: ${error.message}") + } + + private def categoryRefusal(having: String): String = + Await.result( + client.run( + s"SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category $having" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"[$having] was accepted and answered $result") + case ElasticFailure(error) => error.message + } + + private lazy val allCategories: Seq[String] = (1 to categories).map(c => f"cat_$c%02d").sorted + + "an AND of inequalities on the GROUP BY key" should "remove exactly those groups" in { + // NOT-in-A and NOT-in-B is NOT-in-(A āˆŖ B): the exclude list IS that union, so this is correct + // on `455433ae` and must stay byte-identical. + categoriesOf("HAVING category <> 'cat_36' AND category <> 'cat_37'") shouldBe + allCategories.filterNot(Set("cat_36", "cat_37")) + categoriesOf( + "HAVING category NOT IN ('cat_36','cat_37') AND category <> 'cat_35'" + ) shouldBe allCategories.filterNot(Set("cat_35", "cat_36", "cat_37")) + // three leaves, the same union one level deeper + categoriesOf( + "HAVING category <> 'cat_35' AND category <> 'cat_36' AND category <> 'cat_37'" + ) shouldBe allCategories.filterNot(Set("cat_35", "cat_36", "cat_37")) + } + + it should "still combine with an aggregate condition, which is a different mechanism" in { + categoriesOf("HAVING category <> 'cat_37' AND COUNT(*) > 35") shouldBe Seq("cat_36") + } + + "an OR of inequalities" should "be REFUSED, because the exclude list would execute it as an AND" in { + // šŸ”“ MEASURED on `455433ae` and on this cluster: it emitted the SAME `exclude:["cat_36", + // "cat_37"]` as the AND above. The disjunction is true for EVERY one of the 37 groups, and two + // were dropped -- HTTP 200, no error. Live on `main` today. + categoryRefusal( + "HAVING category <> 'cat_36' OR category <> 'cat_37'" + ) should include("cannot combine") + categoryRefusal( + "HAVING category NOT IN ('cat_36','cat_37') OR category <> 'cat_35'" + ) should include("cannot combine") + } + + "an AND of equalities" should "be REFUSED, because the include list means OR" in { + // `include:["cat_36","cat_37"]` keeps two groups; the SQL means none. + categoryRefusal( + "HAVING category = 'cat_36' AND category = 'cat_37'" + ) should include("cannot combine") + } + + "a MIXED pair under OR" should "be REFUSED, because the two lists are ANDed" in { + categoryRefusal( + "HAVING category = 'cat_37' OR category <> 'cat_36'" + ) should include("cannot combine") + } + + it should "still honour one include and one exclude under AND" in { + // āš ļø Both lists carry SEVERAL values on purpose. RESIDUAL, pre-existing and ES-6-ONLY, found + // by this matrix: the es6 bridge renders a SINGLE-element list as a bare string + // (`"exclude":"c"`), which ES 6.8 reads as a REGEX, and mixing a set-based include with a + // regex-based exclude is rejected with HTTP 400 `Cannot mix a set-based include with a + // regex-based method`. The emission is byte-identical to `455433ae`; it is recorded, not fixed + // here. + categoriesOf( + "HAVING category IN ('cat_35','cat_36','cat_37') AND category NOT IN ('cat_35','cat_36')" + ) shouldBe Seq("cat_37") + } + + "a pattern meeting a value list on the same key" should "be REFUSED, not half-applied" in { + // šŸ”“ MEASURED on ES 8.18.3 over a three-bucket fixture: `= 'a' OR LIKE 'b%'` emitted + // `include:"b.*"` and returned only the pattern's bucket -- the value list is DISCARDED by the + // emission when a pattern is present, and a second pattern is lost to `orElse`. + // āš ļø the client caps a relayed parse message at 200 characters (#262), so the assertion + // anchors on the TAIL of the reason, not its middle. + categoryRefusal( + "HAVING category = 'cat_37' OR category LIKE 'cat_3%'" + ) should include("Use a single RLIKE") + // ... and the pattern ALONE still works: cat_30 .. cat_37. + categoriesOf("HAVING category LIKE 'cat_3%'") shouldBe + (30 to 37).map(c => f"cat_$c%02d") + } + + "a LIKE pattern on the key" should "treat _ as a wildcard, like every other LIKE" in { + // šŸ”“ EXECUTED. The terms channel had its own LIKE translation that left `_` literal, so this + // matched NOTHING on `455433ae` -- there is no category named `cat_0_`. With the shared + // translation `_` is one character, so it selects cat_01 .. cat_09. + categoriesOf("HAVING category LIKE 'cat_0_'") shouldBe (1 to 9).map(c => f"cat_0$c%d") + // ... and the plain wildcard form is unchanged. + categoriesOf("HAVING category LIKE 'cat_3%'") shouldBe (30 to 37).map(c => f"cat_$c%02d") + } + + "an OR across TWO grouping levels" should "be REFUSED, because Elasticsearch NESTS them" in { + // šŸ”“ The two `terms` aggregations are nested, which IS a conjunction: an OR across them + // returned the intersection. Measured on a two-key fixture; here the refusal is asserted + // end-to-end, and the AND -- which is exactly what the nesting means -- still answers. + Await.result( + client.run( + "SELECT category, id, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category, id " + + "HAVING category = 'cat_02' OR id = 'cat_02_1'" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"the OR was accepted and answered $result") + case ElasticFailure(error) => error.message should include("DIFFERENT GROUP BY keys") + } + client.searchAsUnchecked[CategoryCount]( + SelectStatement( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category, id " + + "HAVING category = 'cat_02' AND id = 'cat_02_1'" + ) + ) match { + case ElasticSuccess(rows) => rows.map(_.category) shouldBe Seq("cat_02") + case ElasticFailure(error) => fail(s"the AND failed: ${error.message}") + } + } + + it should "REFUSE a HAVING on a column that is neither grouped nor aggregated" in { + Await.result( + client.run( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING UPPER(id) = 'X'" + ), + 60.seconds + ) match { + case ElasticSuccess(result) => fail(s"accepted and answered $result") + case ElasticFailure(error) => + error.message should include("can only filter on a GROUP BY key") + } + } + + it should "still return every group when the un-expressible predicate is NOT there" in { + // šŸ”“ Non-vacuity for the refusals above: the same statements WITHOUT the offending function + // are accepted, so the rejection is about the predicate and not about the fixture. + client.searchAs[CategoryCount]( + "SELECT category, COUNT(*) AS cnt FROM group_by_completeness GROUP BY category " + + "HAVING COUNT(*) > 30" + ) match { + case ElasticSuccess(rows) => rows should have size over30.toLong + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + it should "refuse -- never discard -- a HAVING it cannot evaluate over the implicit group" in { implicit val havingCtx: ConversionContext = NativeContext // `category` is not aggregated and there is no GROUP BY, so there is no group to filter. The