From 4368d9dcf13d07cdfa8a7b1df3affe7b3c1b40be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 03:44:17 +0200 Subject: [PATCH 1/6] fix(sql,bridge): name un-aliased aggregates after the whole expression; bucket pipelines read params. null-guarded An un-aliased aggregate's metricName was the bare field name, so two aggregates over one field in HAVING/ORDER BY collapsed onto one aggregation (HAVING COUNT(x) > 5 AND MAX(x) > 3 compared the count twice, ORDER BY MIN(x) beside HAVING MAX(x) lost its order key), a dotted field leaked into buckets_path keys (a nested-level HAVING silently emitted no having_filter), and an aggregate over a self-contained function (MAX(ABS(salary))) was named "". Identifier.metricName now derives the name from the whole expression (count_x, max_abs_salary, count_distinct_x; COUNT(*) keeps count_all). The bucket_selector script read doc[...] and re-applied the transform the metric aggregation had already applied (HAVING MAX(YEAR(x)) > 2020). A context-free rendering of an aggregate is now the bucket-pipeline form: params. only, the transform once in the metric script, and every compared metric null-guarded as one parenthesised expression (lead directive, AC 4b) so it composes under AND/OR; the temporal literal is converted to epoch millis inside the guard. Both bridge copies resolve buckets_path through one resolver keyed on the aggregation's carried local name. Found by the live run and fixed here too: HAVING of a SELECT aggregate returned every group (Having.resolveAggregateAliases); arithmetic over aggregates (MAX(x) - MIN(x) AS d) never created its operands and core rejected the bucket_script before Elasticsearch (auxiliaryAggs operands, AggregationType.BucketScript); auxiliaryAggs dedup is now order-preserving. Ten pinned having_filter fixtures per bridge copy move to the guarded form; AggregationNamingSpec pins the emitted JSON and adds two structural guards (no empty aggregation/order key; no unguarded possibly-null dereference or comparison in any emitted Painless). Seven live tests with exact row oracles green on ES 6.8 (rest + jest), 7.17, 8.18, 9.0. Story BIDC-2 Closes #54 Closes #223 Co-Authored-By: Claude Fable 5.1 --- .../sql/bridge/ElasticAggregation.scala | 146 +++--- .../elastic/sql/AggregationNamingSpec.scala | 432 ++++++++++++++++++ .../elastic/sql/SQLQuerySpec.scala | 31 +- .../softnetwork/elastic/client/package.scala | 8 +- documentation/sql/dql_statements.md | 9 + .../sql/bridge/ElasticAggregation.scala | 146 +++--- .../elastic/sql/AggregationNamingSpec.scala | 432 ++++++++++++++++++ .../elastic/sql/SQLQuerySpec.scala | 31 +- .../sql/function/aggregate/package.scala | 14 +- .../app/softnetwork/elastic/sql/package.scala | 54 ++- .../elastic/sql/query/GroupBy.scala | 24 +- .../elastic/sql/query/Having.scala | 60 ++- .../softnetwork/elastic/sql/query/Where.scala | 68 ++- .../elastic/sql/query/package.scala | 22 +- .../sql/parser/ScriptFunctionParenSpec.scala | 16 +- .../client/GatewayApiIntegrationSpec.scala | 150 ++++++ 16 files changed, 1378 insertions(+), 265 deletions(-) create mode 100644 bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala create mode 100644 es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala diff --git a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index 47dc67002..d0880e16f 100644 --- a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala +++ b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala @@ -640,44 +640,16 @@ object ElasticAggregation { val conditions = parseConditions(fullScript) // println(s"[DEBUG] conditions = $conditions") - // Filter based on availability in buckets_path + // Keep a condition only when every metric it reads is one extractMetricsPathForBucket publishes + // for this very bucket -- both go through resolveBucketMetric, so the emitted script can never + // read a `params.` that buckets_path does not declare. A name no aggregation carries is an + // opaque script parameter (`params.__now__`) at root level and unresolvable inside a nested one. val relevantConditions = conditions.filter { condition => - val metricNames = extractMetricNames(condition) - // println(s"[DEBUG] condition = $condition, metricNames = $metricNames") - - metricNames.forall { metricName => - allElasticAggregations.find(agg => - agg.aggName == metricName || agg.field == metricName - ) match { - case Some(elasticAgg) => - val metricBucketPath = elasticAgg.nestedElement - .map(_.nestedPath) - .getOrElse("") - - // println( - // s"[DEBUG] metricName = $metricName, metricBucketPath = $metricBucketPath, aggType = ${elasticAgg.agg.getClass.getSimpleName}" - // ) - - val belongsToLevel = metricBucketPath == currentNestedPath - - val isDirectChildAndAccessible = - if (isDirectChild(metricBucketPath, currentNestedPath)) { - // Check if it's a "global" metric (cardinality, etc.) - elasticAgg.isGlobalMetric - } else { - false - } - - val result = belongsToLevel || isDirectChildAndAccessible - - // println( - // s"[DEBUG] belongsToLevel = $belongsToLevel, isDirectChildAndAccessible = $isDirectChildAndAccessible, result = $result" - // ) - result - - case None => - // println(s"[DEBUG] metricName = $metricName NOT FOUND") - currentNestedPath.isEmpty + extractMetricNames(condition).forall { metricName => + resolveBucketMetric(metricName, currentNestedPath, allElasticAggregations) match { + case Resolved(_) => true + case Unknown => currentNestedPath.isEmpty + case OutOfScope => false } } } @@ -718,6 +690,46 @@ object ElasticAggregation { } } + /** How a metric a bucket pipeline reads (`params.`) resolves against the aggregations + * of the bucket being built. ONE resolver feeds both the `bucket_selector` script (which + * conditions survive) and its `buckets_path` (which names are published), so the two cannot + * drift. + */ + private sealed trait MetricResolution + + /** Addressable from this bucket. `path` is the `buckets_path` value: the aggregation's LOCAL name + * -- `agg.name`, the name the elastic4s aggregation was built with, never the `.`-joined + * `aggName` a nested aggregation carries (issue #54) -- or `>` for + * a global metric of a direct nested child. + */ + private case class Resolved(path: String) extends MetricResolution + + /** Exists, but at a level this bucket cannot address (a bucket-level metric of a child, or + * another branch entirely). + */ + private case object OutOfScope extends MetricResolution + + /** No aggregation carries this name. */ + private case object Unknown extends MetricResolution + + private def resolveBucketMetric( + metricName: String, + currentNestedPath: String, + allElasticAggregations: Seq[ElasticAggregation] + ): MetricResolution = + allElasticAggregations.find(agg => agg.aggName == metricName || agg.field == metricName) match { + case Some(elasticAgg) => + val metricBucketPath = elasticAgg.nestedElement.map(_.nestedPath).getOrElse("") + if (metricBucketPath == currentNestedPath) + Resolved(elasticAgg.agg.name) + else if (isDirectChild(metricBucketPath, currentNestedPath) && elasticAgg.isGlobalMetric) { + val childNestedName = elasticAgg.nestedElement.map(_.innerHitsName).getOrElse("") + Resolved(s"$childNestedName>${elasticAgg.agg.name}") + } else + OutOfScope + case None => Unknown + } + def extractMetricsPathForBucketScript( bucketScriptAggregation: BucketScriptAggregation, allAggregations: Seq[SQLAggregation] @@ -782,62 +794,14 @@ object ElasticAggregation { // println(s"[DEBUG extractMetricsPath] currentBucketPath = $currentBucketPath") // println(s"[DEBUG extractMetricsPath] allMetricsPaths = $allMetricsPaths") - // Filter and adapt the paths for this bucket - val result = allMetricsPaths.flatMap { case (metricName, _) => - allElasticAggregations.find(agg => - agg.aggName == metricName || agg.field == metricName - ) match { - case Some(elasticAgg) => - val metricBucketPath = elasticAgg.nestedElement - .map(_.nestedPath) - .getOrElse("") - - // println( - // s"[DEBUG extractMetricsPath] metricName = $metricName, metricBucketPath = $metricBucketPath, aggType = ${elasticAgg.agg.getClass.getSimpleName}" - // ) - - if (metricBucketPath == currentBucketPath) { - // Metric of the same level - // println(s"[DEBUG extractMetricsPath] Same level: $metricName -> $metricName") - Some(metricName -> metricName) - - } else if (isDirectChild(metricBucketPath, currentBucketPath)) { - // Metric of a direct child - - // CHECK if it is a "global" metric (cardinality, etc.) or a bucket metric (avg, sum, etc.) - val isGlobalMetric = elasticAgg.isGlobalMetric - - if (isGlobalMetric) { - // Global metric: can be referenced from the parent - val childNestedName = elasticAgg.nestedElement - .map(_.innerHitsName) - .getOrElse("") - // println( - // s"[DEBUG extractMetricsPath] Direct child (global metric): $metricName -> $childNestedName>$metricName" - // ) - Some(metricName -> s"$childNestedName>$metricName") - } else { - // Bucket metric: cannot be referenced from the parent - // println( - // s"[DEBUG extractMetricsPath] Direct child (bucket metric): $metricName -> SKIP (bucket-level metric)" - // ) - None - } - - } else { - // A different level of metric - // println(s"[DEBUG extractMetricsPath] Other level: $metricName -> SKIP") - None - } - - case None => - // println(s"[DEBUG extractMetricsPath] Not found: $metricName -> SKIP") - None + // Publish, under the name the script reads, the path of the aggregation that carries it -- + // resolved by the very rule metricSelectorForBucket filtered the script with. + allMetricsPaths.flatMap { case (metricName, _) => + resolveBucketMetric(metricName, currentBucketPath, allElasticAggregations) match { + case Resolved(path) => Some(metricName -> path) + case _ => None } } - - // println(s"[DEBUG extractMetricsPath] result = $result") - result } } diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala new file mode 100644 index 000000000..398cfcbe6 --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -0,0 +1,432 @@ +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +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._ + +/** Issues #54 / #223 (story BIDC-2): `buckets_path` and aggregation naming, and the Painless a + * bucket pipeline emits. + * + * Every assertion here is on the GENERATED Elasticsearch query, never on the `.sql` render: the + * defects this pins were invisible to a parse (the statements parsed, ran, and returned wrong + * buckets). Two structural guards close the file: no emitted aggregation or `order` key is `""` + * (AC 5), and no emitted Painless compares or dereferences a possibly-null value unguarded (AC 4b, + * lead directive 2026-09-06). + */ +class AggregationNamingSpec 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 def queryOf(sql: String): String = { + val select: ElasticSearchRequest = SelectStatement(sql) + select.query + } + + private val terms = """"terms":{"field":"id","size":65536,"min_doc_count":1}""" + + // --------------------------------------------------------------------------------------------- + // #54 -- an un-aliased aggregate is named after the WHOLE expression + // --------------------------------------------------------------------------------------------- + + "a HAVING-only COUNT(field)" should "be named after the aggregate and resolve its buckets_path (issue #54)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5))"}}}}}}}""" + ).mkString + } + + "a HAVING that references a SELECT aggregate by its alias" should "filter on that aggregate (issue #54)" in { + // `cnt` has no aggregate function of its own: the selector saw no metric and the whole HAVING + // was dropped -- every group came back. The alias now resolves to the SELECT item's aggregate. + queryOf("SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"cnt":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"cnt":"cnt"},""", + """"script":{"source":"(params.cnt == null ? false : (params.cnt > 5))"}}}}}}}""" + ).mkString + } + + it should "resolve the alias of a transformed aggregate too (issue #54)" in { + queryOf( + "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020 AND COUNT(x) > 1" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"y":{"max":{"field":"createdAt","script":{"lang":"painless",""", + """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """, + """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).get(ChronoField.YEAR)); param1"}}},""", + """"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"y":"y","count_x":"count_x"},""", + """"script":{"source":"(params.y == null ? false : (params.y > 2020)) && """, + """(params.count_x == null ? false : (params.count_x > 1))"}}}}}}}""" + ).mkString + } + + "two aggregates over one field" should "stay two aggregations, both in buckets_path (issue #54)" in { + // Both used to be named `x`; the second was dropped and every term compared the count. + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND MAX(x) > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5)) && """, + """(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + ).mkString + } + + it should "keep an ORDER BY aggregate beside a HAVING aggregate over the same field (issue #54)" in { + // `ORDER BY MIN(x)` used to vanish entirely -- no `order` key at all. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(x) > 3 ORDER BY MIN(x) DESC" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + """"terms":{"field":"id","size":65536,"min_doc_count":1,"order":{"min_x":"desc"}},""", + """"aggs":{"c":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", + """"min_x":{"min":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + ).mkString + } + + it should "tell COUNT from COUNT(DISTINCT) (issue #54)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND COUNT(DISTINCT x) > 2") shouldBe + Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"count_distinct_x":{"cardinality":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x",""", + """"count_distinct_x":"count_distinct_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5)) && """, + """(params.count_distinct_x == null ? false : (params.count_distinct_x > 2))"}}}}}}}""" + ).mkString + } + + "a dotted field" should "never leak into a buckets_path key (issue #54)" in { + // `params.profile.age` is a nested-property read Painless rejects. + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(profile.age) > 30") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_profile_age":{"max":{"field":"profile.age"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_profile_age":"max_profile_age"},""", + """"script":{"source":"(params.max_profile_age == null ? false : (params.max_profile_age > 30))"}}}}}}}""" + ).mkString + } + + it should "let a HAVING inside a nested level keep its selector (issue #54)" in { + // The selector's name scanner saw `params.emails` (no such aggregation, not root level) and + // dropped the condition: the HAVING was silently ignored. + queryOf( + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING COUNT(e.address) > 1""".stripMargin + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.domain":{"terms":{"field":"emails.domain","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 : (params.count_e_address > 1))"}}}}}}}}}""" + ).mkString + } + + "a global metric of a direct nested child" should "resolve through its local name (AC 3)" in { + // The `>`-joined child path: the only place `buckets_path` value and key legitimately differ. + queryOf( + """SELECT c.id, COUNT(DISTINCT e.address) AS nb + |FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY c.id HAVING COUNT(DISTINCT e.address) > 1""".stripMargin + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"having_filter":{"bucket_selector":{"buckets_path":{"nb":"e>nb"},""", + """"script":{"source":"(params.nb == null ? false : (params.nb > 1))"}}},""", + """"e":{"nested":{"path":"emails"},"aggs":{"nb":{"cardinality":{"field":"emails.address"}}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // #223 -- HAVING / ORDER BY over a transformed aggregate + // --------------------------------------------------------------------------------------------- + + private val maxYearCreatedAt = + """"max_year_createdat":{"max":{"field":"createdAt","script":{"lang":"painless",""" + + """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """ + + """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).get(ChronoField.YEAR)); param1"}}}""" + + "HAVING over a transformed aggregate" should "read the metric, guarded, with the transform applied once (issue #223)" in { + // Was: `def left = (doc[..] ? null : ..get(YEAR)).get(YEAR); left == null ? false : (left > 2020)` + // -- a `doc[]` read where there is no document, the transform twice, a method on a nullable. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"x"}},""", + maxYearCreatedAt, + ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat"},""", + """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020))"}}}}}}}""" + ).mkString + } + + it should "compose under OR as one guarded expression per term (issue #223)" in { + // The statement form `def left = ...; left == null ? false : (left > 2020) || params.c > 5` + // parsed as `left == null ? false : ((left > 2020) || params.c > 5)`: a null left swallowed B. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020 OR COUNT(x) > 5" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"x"}},""", + maxYearCreatedAt, + ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat","c":"c"},""", + """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020)) || """, + """(params.c == null ? false : (params.c > 5))"}}}}}}}""" + ).mkString + } + + it should "guard an aggregate on the right-hand side too (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(a) > MIN(b)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_a":{"max":{"field":"a"}},"min_b":{"min":{"field":"b"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_a":"max_a","min_b":"min_b"},""", + """"script":{"source":"(params.max_a == null || params.min_b == null ? false : (params.max_a > params.min_b))"}}}}}}}""" + ).mkString + } + + it should "convert a temporal literal to epoch millis inside the guard (issue #223)" in { + // The conversion used to be appended to the WHOLE predicate; guarded, it would land on the boolean. + queryOf( + "SELECT id FROM t GROUP BY id HAVING MAX(DATE_TRUNC(createdAt, DAY)) > now - interval 7 day" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_date_trunc_createdat_day":{"max":{"field":"createdAt","script":{"lang":"painless",""", + """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """, + """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).truncatedTo(ChronoUnit.DAYS)); param1"}}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_date_trunc_createdat_day":"max_date_trunc_createdat_day"},""", + """"script":{"source":"(params.max_date_trunc_createdat_day == null ? false : """, + """(params.max_date_trunc_createdat_day > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z'))""", + """.minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}""" + ).mkString + } + + "ORDER BY over a transformed aggregate" should "name its aggregation (issue #223)" in { + // The sub-aggregation key and the `order` key were both `""`. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id ORDER BY MAX(ABS(salary)) DESC" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + """"terms":{"field":"id","size":65536,"min_doc_count":1,"order":{"max_abs_salary":"desc"}},""", + """"aggs":{"c":{"value_count":{"field":"x"}},"max_abs_salary":{"max":{"script":{"lang":"painless",""", + """"source":"def param1 = (doc['salary'].size() == 0 ? null : doc['salary'].value); """, + """(param1 == null) ? null : Double.valueOf(Math.abs(param1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // #54 -- arithmetic over aggregates (bucket_script) + // --------------------------------------------------------------------------------------------- + + "arithmetic over aggregates" should "create its operands and read them as params (issue #54)" in { + // Was: no `max`/`min` at all, `buckets_path {"d":"d"}` (itself) and `params.x - params.x`. + queryOf("SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"d":{"bucket_script":{"buckets_path":{"max_x":"max_x","min_x":"min_x"},""", + """"script":"params.max_x - params.min_x"}},""", + """"max_x":{"max":{"field":"x"}},"min_x":{"min":{"field":"x"}}}}}}""" + ).mkString + } + + it should "reuse an operand that is also a SELECT item (issue #54)" in { + queryOf("SELECT id, MAX(x) AS m, MAX(x) - MIN(x) AS d FROM t GROUP BY id") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"d":{"bucket_script":{"buckets_path":{"m":"m","min_x":"min_x"},""", + """"script":"params.m - params.min_x"}},"m":{"max":{"field":"x"}},""", + """"min_x":{"min":{"field":"x"}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // Structural guards over every shape above (and the ones the pins do not spell out) + // --------------------------------------------------------------------------------------------- + + private val shapes: Seq[String] = Seq( + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5", + "SELECT id, COUNT(x) FROM t GROUP BY id HAVING COUNT(x) > 5", + "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 5", + "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020 AND COUNT(x) > 1", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND MAX(x) > 3", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(x) > 3 ORDER BY MIN(x) DESC", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND COUNT(DISTINCT x) > 2", + "SELECT id FROM t GROUP BY id HAVING COUNT(*) > 2", + "SELECT id FROM t GROUP BY id HAVING MAX(a) > MIN(b)", + "SELECT id FROM t GROUP BY id HAVING MAX(profile.age) > 30", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(DATE_TRUNC(createdAt, MINUTE)) > 2020", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(ABS(salary)) > 10", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020 OR COUNT(x) > 5", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id ORDER BY MAX(ABS(salary)) DESC", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id ORDER BY MAX(YEAR(createdAt)) DESC", + "SELECT id, MAX(ABS(salary)) FROM t GROUP BY id ORDER BY MAX(ABS(salary)) DESC", + "SELECT id, MAX(ABS(salary)) AS m FROM t GROUP BY id HAVING MAX(ABS(salary)) > 10", + "SELECT id FROM t GROUP BY id HAVING MAX(DATE_TRUNC(createdAt, DAY)) > now - interval 7 day", + "SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id", + "SELECT id, MAX(ABS(x)) - MIN(x) AS d FROM t GROUP BY id", + """SELECT c.id, COUNT(DISTINCT e.address) AS nb FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY c.id HAVING COUNT(DISTINCT e.address) > 1""".stripMargin, + """SELECT c.id FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY c.id HAVING COUNT(DISTINCT e.address) > 1""".stripMargin, + """SELECT e.domain, COUNT(e.address) AS nb FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING COUNT(e.address) > 1""".stripMargin, + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING COUNT(e.address) > 1""".stripMargin + ) + + private val mapper = new ObjectMapper() + + /** Every value of a field named `name`, anywhere in the tree. */ + private def valuesOf(node: JsonNode, name: String): Seq[JsonNode] = { + val here = Option(node.get(name)).toSeq + val below = node.elements().asScala.toSeq.flatMap(child => valuesOf(child, name)) + here ++ below + } + + /** Every Painless script the query carries: `"script":{"source":...}` objects and the bare + * `"script":"..."` form a bucket_script uses. + */ + private def scriptsOf(root: JsonNode): Seq[String] = + valuesOf(root, "script").flatMap { s => + if (s.isTextual) Some(s.asText()) + else Option(s.get("source")).filter(_.isTextual).map(_.asText()) + } + + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + "no emitted aggregation or order key" should "be the empty string (AC 5)" in { + shapes.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "aggs").foreach { aggs => + aggs.fieldNames().asScala.toSeq.foreach(_ should not be empty) + } + valuesOf(root, "order").foreach { order => + order.fieldNames().asScala.toSeq.foreach(_ should not be empty) + } + } + } + } + + "a bucket pipeline script" should "read only declared, null-guarded params and never a document (AC 4, AC 4b)" in { + shapes.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") + pipelines.foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + scriptsOf(pipeline).foreach { script => + script should not include "doc[" + val read = paramRef.findAllMatchIn(script).map(_.group(1)).toSet - "__now__" + read shouldBe declared + // The selector null-guards every metric it compares; a bucket_script computes a value + // (a null operand yields a null result, which Elasticsearch treats as a gap). + if (pipeline.has("script") && Option(pipeline.get("script")).exists(_.isObject)) + declared.foreach(key => script should include(s"params.$key == null")) + } + } + } + } + } + + "no emitted Painless" should "dereference or compare a possibly-null value unguarded (AC 4b)" in { + shapes.foreach { sql => + withClue(s"[$sql] ") { + scriptsOf(mapper.readTree(queryOf(sql))).foreach { script => + withClue(s"script [$script] ") { + // (a) A `( ... ? null : ... )` group must not be dereferenced: `(cond ? null : v).m()` + // invokes `m` on null whenever `cond` holds -- the doubled `.get(ChronoField.YEAR)`. + nullTernaryGroupEnds(script).foreach { end => + if (end + 1 < script.length) script.charAt(end + 1) should not be '.' + } + // (b) A `def` bound to such a group is nullable: any later `name.` dereference must be + // preceded by a `name == null` / `name != null` test. + nullableDefs(script).foreach { name => + val afterDef = script.substring(script.indexOf(s"def $name") + 4 + name.length) + if (afterDef.contains(s"$name.")) + assert( + afterDef.contains(s"$name == null") || afterDef.contains(s"$name != null"), + s"'$name.' is dereferenced without a null test" + ) + } + } + } + } + } + } + + /** Index of the `)` closing each parenthesised group whose body carries a `? null :`. */ + private def nullTernaryGroupEnds(script: String): Seq[Int] = { + val marker = "? null :" + Iterator + .iterate(script.indexOf(marker))(from => script.indexOf(marker, from + 1)) + .takeWhile(_ >= 0) + .toList // strict on both Scala legs (2.12's Iterator.toSeq is a lazy Stream) + .flatMap { at => + // Walk back to the unmatched `(` that opens the group holding this ternary... + var depth = 0 + var open = at - 1 + while (open >= 0 && !(script(open) == '(' && depth == 0)) { + script(open) match { + case ')' => depth += 1 + case '(' => depth -= 1 + case _ => + } + open -= 1 + } + // ...then forward to its matching `)`. + if (open < 0) None + else { + depth = 0 + var close = open + var found = -1 + while (found < 0 && close < script.length) { + script(close) match { + case '(' => depth += 1 + case ')' => + depth -= 1 + if (depth == 0) found = close + case _ => + } + close += 1 + } + if (found >= 0) Some(found) else None + } + } + } + + /** Names bound by `def = ;` whose initializer carries a `? null :`. */ + private def nullableDefs(script: String): Seq[String] = + "def ([A-Za-z_][A-Za-z0-9_]*) = ([^;]*);".r + .findAllMatchIn(script) + .filter(_.group(2).contains("? null :")) + .map(_.group(1)) + .toSeq +} diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index c869647a8..d7ffc164c 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -563,7 +563,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "cnt": "cnt" | }, | "script": { - | "source": "params.cnt > 1" + | "source": "(params.cnt == null ? false : (params.cnt > 1))" | } | } | } @@ -577,6 +577,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "perform complex query" in { @@ -821,7 +822,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max_price": "max_price" | }, | "script": { - | "source": "params.min_price > 5.0 && params.max_price < 50.0" + | "source": "(params.min_price == null ? false : (params.min_price > 5.0)) && (params.max_price == null ? false : (params.max_price < 50.0))" | } | } | } @@ -838,6 +839,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll("<(\\d)", " < $1") .replaceAll(">(\\d)", " > $1") + .replaceAll("\\?false:", " ? false : ") } @@ -1033,7 +1035,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()", + | "source": "(params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1045,11 +1047,13 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | } |}""".stripMargin .replaceAll("\\s", "") + .replaceAll("==", " == ") .replaceAll("ChronoUnit", " ChronoUnit") .replaceAll("!=", " != ") .replaceAll("&&", " && ") .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") + .replaceAll("\\?false:", " ? false : ") } it should "handle group by with having and date time functions" in { @@ -1101,7 +1105,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "params.cnt > 1 && params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1121,6 +1125,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") + .replaceAll("\\?false:", " ? false : ") } it should "handle group by index" in { @@ -1174,7 +1179,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "params.cnt > 1 && params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1194,6 +1199,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") + .replaceAll("\\?false:", " ? false : ") } it should "handle date_parse function" in { @@ -3974,7 +3980,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "__c3": "__c3" | }, | "script": { - | "source": "params.__c3 > 1" + | "source": "(params.__c3 == null ? false : (params.__c3 > 1))" | } | } | } @@ -3988,6 +3994,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "handle HAVING COUNT(*) without alias combined with aliased aggregation" in { @@ -4031,7 +4038,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "avg_age": "avg_age" | }, | "script": { - | "source": "params.__c2 >= 1 && params.avg_age > 25" + | "source": "(params.__c2 == null ? false : (params.__c2 >= 1)) && (params.avg_age == null ? false : (params.avg_age > 25))" | } | } | } @@ -4044,6 +4051,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll(">=", " >= ") .replaceAll("(?)>(?!=)", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "handle HAVING COUNT(*) only in HAVING clause not in SELECT" in { @@ -4086,7 +4094,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "count_all": "count_all" | }, | "script": { - | "source": "params.count_all > 1" + | "source": "(params.count_all == null ? false : (params.count_all > 1))" | } | } | } @@ -4098,6 +4106,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } // === Issue #52: ORDER BY on aggregation alias === @@ -4224,7 +4233,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "__c2": "__c2" | }, | "script": { - | "source": "params.__c2 > 1" + | "source": "(params.__c2 == null ? false : (params.__c2 > 1))" | } | } | } @@ -4236,6 +4245,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "handle HAVING COUNT(DISTINCT *) only in HAVING clause not in SELECT" in { @@ -4278,7 +4288,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "count_distinct_all": "count_distinct_all" | }, | "script": { - | "source": "params.count_distinct_all > 1" + | "source": "(params.count_distinct_all == null ? false : (params.count_distinct_all > 1))" | } | } | } @@ -4290,6 +4300,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } // === Story 14.1: NULLS FIRST / NULLS LAST on ORDER BY === diff --git a/core/src/main/scala/app/softnetwork/elastic/client/package.scala b/core/src/main/scala/app/softnetwork/elastic/client/package.scala index 94d9aab70..17884fe9a 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/package.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/package.scala @@ -388,7 +388,10 @@ package object client extends SerializationApi { // PERCENTILE_CONT / PERCENTILE_DISC — both back the ES `percentiles` // aggregation; the requested percentile key (e.g. "99.0") is carried on // ClientAggregation.aggResultField and projected from the response `values`. - PercentileCont, PercentileDisc = Value + PercentileCont, PercentileDisc, + // Arithmetic over aggregates (`MAX(x) - MIN(x) AS d`) — an ES `bucket_script` pipeline + // aggregation whose result is a plain `value` node (issue #54, BIDC-2). + BucketScript = Value } /** Client Aggregation @@ -478,6 +481,9 @@ package object client extends SerializationApi { } case p: PercentileAgg => if (p.cont) AggregationType.PercentileCont else AggregationType.PercentileDisc + // The bridge has always emitted the bucket_script; this arm was the missing piece that made + // every arithmetic-over-aggregates statement fail here, before reaching Elasticsearch. + case _: BucketScriptAggregation => AggregationType.BucketScript case _ => throw new IllegalArgumentException(s"Unsupported aggregation type: ${agg.aggType}") } // `extended_stats` is multi-key — pick which one to project. Plain diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 754bdf14f..65f929927 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -513,6 +513,15 @@ ORDER BY COUNT(*) DESC; - `GROUP BY` supports nested fields (`profile.city`). - `HAVING` filters groups based on aggregate conditions. - Translated to Elasticsearch aggregations. +- An aggregate referenced only in `HAVING` or `ORDER BY` needs no alias and no `SELECT` item: it is + computed for the filter or the sort and kept out of the result columns. Distinct aggregates over + the same column stay distinct (`HAVING COUNT(age) >= 1 AND MAX(age) > 45`), and the aggregate may + wrap a transform (`HAVING MAX(YEAR(birthdate)) > 1990`, `ORDER BY MAX(ABS(age)) DESC`). +- Arithmetic over aggregates is computed per group (`MAX(price) - MIN(price) AS price_range`); the + operands are computed as hidden aggregations of the group. +- A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents + all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter + script null-checks every metric before comparing it. --- diff --git a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index f838d8f06..cd1d398d8 100644 --- a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala +++ b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala @@ -636,44 +636,16 @@ object ElasticAggregation { val conditions = parseConditions(fullScript) // println(s"[DEBUG] conditions = $conditions") - // Filter based on availability in buckets_path + // Keep a condition only when every metric it reads is one extractMetricsPathForBucket publishes + // for this very bucket -- both go through resolveBucketMetric, so the emitted script can never + // read a `params.` that buckets_path does not declare. A name no aggregation carries is an + // opaque script parameter (`params.__now__`) at root level and unresolvable inside a nested one. val relevantConditions = conditions.filter { condition => - val metricNames = extractMetricNames(condition) - // println(s"[DEBUG] condition = $condition, metricNames = $metricNames") - - metricNames.forall { metricName => - allElasticAggregations.find(agg => - agg.aggName == metricName || agg.field == metricName - ) match { - case Some(elasticAgg) => - val metricBucketPath = elasticAgg.nestedElement - .map(_.nestedPath) - .getOrElse("") - - // println( - // s"[DEBUG] metricName = $metricName, metricBucketPath = $metricBucketPath, aggType = ${elasticAgg.agg.getClass.getSimpleName}" - // ) - - val belongsToLevel = metricBucketPath == currentNestedPath - - val isDirectChildAndAccessible = - if (isDirectChild(metricBucketPath, currentNestedPath)) { - // Check if it's a "global" metric (cardinality, etc.) - elasticAgg.isGlobalMetric - } else { - false - } - - val result = belongsToLevel || isDirectChildAndAccessible - - // println( - // s"[DEBUG] belongsToLevel = $belongsToLevel, isDirectChildAndAccessible = $isDirectChildAndAccessible, result = $result" - // ) - result - - case None => - // println(s"[DEBUG] metricName = $metricName NOT FOUND") - currentNestedPath.isEmpty + extractMetricNames(condition).forall { metricName => + resolveBucketMetric(metricName, currentNestedPath, allElasticAggregations) match { + case Resolved(_) => true + case Unknown => currentNestedPath.isEmpty + case OutOfScope => false } } } @@ -714,6 +686,46 @@ object ElasticAggregation { } } + /** How a metric a bucket pipeline reads (`params.`) resolves against the aggregations + * of the bucket being built. ONE resolver feeds both the `bucket_selector` script (which + * conditions survive) and its `buckets_path` (which names are published), so the two cannot + * drift. + */ + private sealed trait MetricResolution + + /** Addressable from this bucket. `path` is the `buckets_path` value: the aggregation's LOCAL name + * -- `agg.name`, the name the elastic4s aggregation was built with, never the `.`-joined + * `aggName` a nested aggregation carries (issue #54) -- or `>` for + * a global metric of a direct nested child. + */ + private case class Resolved(path: String) extends MetricResolution + + /** Exists, but at a level this bucket cannot address (a bucket-level metric of a child, or + * another branch entirely). + */ + private case object OutOfScope extends MetricResolution + + /** No aggregation carries this name. */ + private case object Unknown extends MetricResolution + + private def resolveBucketMetric( + metricName: String, + currentNestedPath: String, + allElasticAggregations: Seq[ElasticAggregation] + ): MetricResolution = + allElasticAggregations.find(agg => agg.aggName == metricName || agg.field == metricName) match { + case Some(elasticAgg) => + val metricBucketPath = elasticAgg.nestedElement.map(_.nestedPath).getOrElse("") + if (metricBucketPath == currentNestedPath) + Resolved(elasticAgg.agg.name) + else if (isDirectChild(metricBucketPath, currentNestedPath) && elasticAgg.isGlobalMetric) { + val childNestedName = elasticAgg.nestedElement.map(_.innerHitsName).getOrElse("") + Resolved(s"$childNestedName>${elasticAgg.agg.name}") + } else + OutOfScope + case None => Unknown + } + def extractMetricsPathForBucketScript( bucketScriptAggregation: BucketScriptAggregation, allAggregations: Seq[SQLAggregation] @@ -778,62 +790,14 @@ object ElasticAggregation { // println(s"[DEBUG extractMetricsPath] currentBucketPath = $currentBucketPath") // println(s"[DEBUG extractMetricsPath] allMetricsPaths = $allMetricsPaths") - // Filter and adapt the paths for this bucket - val result = allMetricsPaths.flatMap { case (metricName, _) => - allElasticAggregations.find(agg => - agg.aggName == metricName || agg.field == metricName - ) match { - case Some(elasticAgg) => - val metricBucketPath = elasticAgg.nestedElement - .map(_.nestedPath) - .getOrElse("") - - // println( - // s"[DEBUG extractMetricsPath] metricName = $metricName, metricBucketPath = $metricBucketPath, aggType = ${elasticAgg.agg.getClass.getSimpleName}" - // ) - - if (metricBucketPath == currentBucketPath) { - // Metric of the same level - // println(s"[DEBUG extractMetricsPath] Same level: $metricName -> $metricName") - Some(metricName -> metricName) - - } else if (isDirectChild(metricBucketPath, currentBucketPath)) { - // Metric of a direct child - - // CHECK if it is a "global" metric (cardinality, etc.) or a bucket metric (avg, sum, etc.) - val isGlobalMetric = elasticAgg.isGlobalMetric - - if (isGlobalMetric) { - // Global metric: can be referenced from the parent - val childNestedName = elasticAgg.nestedElement - .map(_.innerHitsName) - .getOrElse("") - // println( - // s"[DEBUG extractMetricsPath] Direct child (global metric): $metricName -> $childNestedName>$metricName" - // ) - Some(metricName -> s"$childNestedName>$metricName") - } else { - // Bucket metric: cannot be referenced from the parent - // println( - // s"[DEBUG extractMetricsPath] Direct child (bucket metric): $metricName -> SKIP (bucket-level metric)" - // ) - None - } - - } else { - // A different level of metric - // println(s"[DEBUG extractMetricsPath] Other level: $metricName -> SKIP") - None - } - - case None => - // println(s"[DEBUG extractMetricsPath] Not found: $metricName -> SKIP") - None + // Publish, under the name the script reads, the path of the aggregation that carries it -- + // resolved by the very rule metricSelectorForBucket filtered the script with. + allMetricsPaths.flatMap { case (metricName, _) => + resolveBucketMetric(metricName, currentBucketPath, allElasticAggregations) match { + case Resolved(path) => Some(metricName -> path) + case _ => None } } - - // println(s"[DEBUG extractMetricsPath] result = $result") - result } } diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala new file mode 100644 index 000000000..398cfcbe6 --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -0,0 +1,432 @@ +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +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._ + +/** Issues #54 / #223 (story BIDC-2): `buckets_path` and aggregation naming, and the Painless a + * bucket pipeline emits. + * + * Every assertion here is on the GENERATED Elasticsearch query, never on the `.sql` render: the + * defects this pins were invisible to a parse (the statements parsed, ran, and returned wrong + * buckets). Two structural guards close the file: no emitted aggregation or `order` key is `""` + * (AC 5), and no emitted Painless compares or dereferences a possibly-null value unguarded (AC 4b, + * lead directive 2026-09-06). + */ +class AggregationNamingSpec 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 def queryOf(sql: String): String = { + val select: ElasticSearchRequest = SelectStatement(sql) + select.query + } + + private val terms = """"terms":{"field":"id","size":65536,"min_doc_count":1}""" + + // --------------------------------------------------------------------------------------------- + // #54 -- an un-aliased aggregate is named after the WHOLE expression + // --------------------------------------------------------------------------------------------- + + "a HAVING-only COUNT(field)" should "be named after the aggregate and resolve its buckets_path (issue #54)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5))"}}}}}}}""" + ).mkString + } + + "a HAVING that references a SELECT aggregate by its alias" should "filter on that aggregate (issue #54)" in { + // `cnt` has no aggregate function of its own: the selector saw no metric and the whole HAVING + // was dropped -- every group came back. The alias now resolves to the SELECT item's aggregate. + queryOf("SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"cnt":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"cnt":"cnt"},""", + """"script":{"source":"(params.cnt == null ? false : (params.cnt > 5))"}}}}}}}""" + ).mkString + } + + it should "resolve the alias of a transformed aggregate too (issue #54)" in { + queryOf( + "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020 AND COUNT(x) > 1" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"y":{"max":{"field":"createdAt","script":{"lang":"painless",""", + """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """, + """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).get(ChronoField.YEAR)); param1"}}},""", + """"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"y":"y","count_x":"count_x"},""", + """"script":{"source":"(params.y == null ? false : (params.y > 2020)) && """, + """(params.count_x == null ? false : (params.count_x > 1))"}}}}}}}""" + ).mkString + } + + "two aggregates over one field" should "stay two aggregations, both in buckets_path (issue #54)" in { + // Both used to be named `x`; the second was dropped and every term compared the count. + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND MAX(x) > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5)) && """, + """(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + ).mkString + } + + it should "keep an ORDER BY aggregate beside a HAVING aggregate over the same field (issue #54)" in { + // `ORDER BY MIN(x)` used to vanish entirely -- no `order` key at all. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(x) > 3 ORDER BY MIN(x) DESC" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + """"terms":{"field":"id","size":65536,"min_doc_count":1,"order":{"min_x":"desc"}},""", + """"aggs":{"c":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", + """"min_x":{"min":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + ).mkString + } + + it should "tell COUNT from COUNT(DISTINCT) (issue #54)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND COUNT(DISTINCT x) > 2") shouldBe + Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"count_distinct_x":{"cardinality":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x",""", + """"count_distinct_x":"count_distinct_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5)) && """, + """(params.count_distinct_x == null ? false : (params.count_distinct_x > 2))"}}}}}}}""" + ).mkString + } + + "a dotted field" should "never leak into a buckets_path key (issue #54)" in { + // `params.profile.age` is a nested-property read Painless rejects. + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(profile.age) > 30") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_profile_age":{"max":{"field":"profile.age"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_profile_age":"max_profile_age"},""", + """"script":{"source":"(params.max_profile_age == null ? false : (params.max_profile_age > 30))"}}}}}}}""" + ).mkString + } + + it should "let a HAVING inside a nested level keep its selector (issue #54)" in { + // The selector's name scanner saw `params.emails` (no such aggregation, not root level) and + // dropped the condition: the HAVING was silently ignored. + queryOf( + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING COUNT(e.address) > 1""".stripMargin + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.domain":{"terms":{"field":"emails.domain","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 : (params.count_e_address > 1))"}}}}}}}}}""" + ).mkString + } + + "a global metric of a direct nested child" should "resolve through its local name (AC 3)" in { + // The `>`-joined child path: the only place `buckets_path` value and key legitimately differ. + queryOf( + """SELECT c.id, COUNT(DISTINCT e.address) AS nb + |FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY c.id HAVING COUNT(DISTINCT e.address) > 1""".stripMargin + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"having_filter":{"bucket_selector":{"buckets_path":{"nb":"e>nb"},""", + """"script":{"source":"(params.nb == null ? false : (params.nb > 1))"}}},""", + """"e":{"nested":{"path":"emails"},"aggs":{"nb":{"cardinality":{"field":"emails.address"}}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // #223 -- HAVING / ORDER BY over a transformed aggregate + // --------------------------------------------------------------------------------------------- + + private val maxYearCreatedAt = + """"max_year_createdat":{"max":{"field":"createdAt","script":{"lang":"painless",""" + + """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """ + + """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).get(ChronoField.YEAR)); param1"}}}""" + + "HAVING over a transformed aggregate" should "read the metric, guarded, with the transform applied once (issue #223)" in { + // Was: `def left = (doc[..] ? null : ..get(YEAR)).get(YEAR); left == null ? false : (left > 2020)` + // -- a `doc[]` read where there is no document, the transform twice, a method on a nullable. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"x"}},""", + maxYearCreatedAt, + ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat"},""", + """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020))"}}}}}}}""" + ).mkString + } + + it should "compose under OR as one guarded expression per term (issue #223)" in { + // The statement form `def left = ...; left == null ? false : (left > 2020) || params.c > 5` + // parsed as `left == null ? false : ((left > 2020) || params.c > 5)`: a null left swallowed B. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020 OR COUNT(x) > 5" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"c":{"value_count":{"field":"x"}},""", + maxYearCreatedAt, + ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat","c":"c"},""", + """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020)) || """, + """(params.c == null ? false : (params.c > 5))"}}}}}}}""" + ).mkString + } + + it should "guard an aggregate on the right-hand side too (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(a) > MIN(b)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_a":{"max":{"field":"a"}},"min_b":{"min":{"field":"b"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_a":"max_a","min_b":"min_b"},""", + """"script":{"source":"(params.max_a == null || params.min_b == null ? false : (params.max_a > params.min_b))"}}}}}}}""" + ).mkString + } + + it should "convert a temporal literal to epoch millis inside the guard (issue #223)" in { + // The conversion used to be appended to the WHOLE predicate; guarded, it would land on the boolean. + queryOf( + "SELECT id FROM t GROUP BY id HAVING MAX(DATE_TRUNC(createdAt, DAY)) > now - interval 7 day" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_date_trunc_createdat_day":{"max":{"field":"createdAt","script":{"lang":"painless",""", + """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """, + """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).truncatedTo(ChronoUnit.DAYS)); param1"}}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_date_trunc_createdat_day":"max_date_trunc_createdat_day"},""", + """"script":{"source":"(params.max_date_trunc_createdat_day == null ? false : """, + """(params.max_date_trunc_createdat_day > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z'))""", + """.minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}""" + ).mkString + } + + "ORDER BY over a transformed aggregate" should "name its aggregation (issue #223)" in { + // The sub-aggregation key and the `order` key were both `""`. + queryOf( + "SELECT id, COUNT(x) AS c FROM t GROUP BY id ORDER BY MAX(ABS(salary)) DESC" + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + """"terms":{"field":"id","size":65536,"min_doc_count":1,"order":{"max_abs_salary":"desc"}},""", + """"aggs":{"c":{"value_count":{"field":"x"}},"max_abs_salary":{"max":{"script":{"lang":"painless",""", + """"source":"def param1 = (doc['salary'].size() == 0 ? null : doc['salary'].value); """, + """(param1 == null) ? null : Double.valueOf(Math.abs(param1))"}}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // #54 -- arithmetic over aggregates (bucket_script) + // --------------------------------------------------------------------------------------------- + + "arithmetic over aggregates" should "create its operands and read them as params (issue #54)" in { + // Was: no `max`/`min` at all, `buckets_path {"d":"d"}` (itself) and `params.x - params.x`. + queryOf("SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"d":{"bucket_script":{"buckets_path":{"max_x":"max_x","min_x":"min_x"},""", + """"script":"params.max_x - params.min_x"}},""", + """"max_x":{"max":{"field":"x"}},"min_x":{"min":{"field":"x"}}}}}}""" + ).mkString + } + + it should "reuse an operand that is also a SELECT item (issue #54)" in { + queryOf("SELECT id, MAX(x) AS m, MAX(x) - MIN(x) AS d FROM t GROUP BY id") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"d":{"bucket_script":{"buckets_path":{"m":"m","min_x":"min_x"},""", + """"script":"params.m - params.min_x"}},"m":{"max":{"field":"x"}},""", + """"min_x":{"min":{"field":"x"}}}}}}""" + ).mkString + } + + // --------------------------------------------------------------------------------------------- + // Structural guards over every shape above (and the ones the pins do not spell out) + // --------------------------------------------------------------------------------------------- + + private val shapes: Seq[String] = Seq( + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5", + "SELECT id, COUNT(x) FROM t GROUP BY id HAVING COUNT(x) > 5", + "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 5", + "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020 AND COUNT(x) > 1", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND MAX(x) > 3", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(x) > 3 ORDER BY MIN(x) DESC", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND COUNT(DISTINCT x) > 2", + "SELECT id FROM t GROUP BY id HAVING COUNT(*) > 2", + "SELECT id FROM t GROUP BY id HAVING MAX(a) > MIN(b)", + "SELECT id FROM t GROUP BY id HAVING MAX(profile.age) > 30", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(DATE_TRUNC(createdAt, MINUTE)) > 2020", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(ABS(salary)) > 10", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(YEAR(createdAt)) > 2020 OR COUNT(x) > 5", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id ORDER BY MAX(ABS(salary)) DESC", + "SELECT id, COUNT(x) AS c FROM t GROUP BY id ORDER BY MAX(YEAR(createdAt)) DESC", + "SELECT id, MAX(ABS(salary)) FROM t GROUP BY id ORDER BY MAX(ABS(salary)) DESC", + "SELECT id, MAX(ABS(salary)) AS m FROM t GROUP BY id HAVING MAX(ABS(salary)) > 10", + "SELECT id FROM t GROUP BY id HAVING MAX(DATE_TRUNC(createdAt, DAY)) > now - interval 7 day", + "SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id", + "SELECT id, MAX(ABS(x)) - MIN(x) AS d FROM t GROUP BY id", + """SELECT c.id, COUNT(DISTINCT e.address) AS nb FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY c.id HAVING COUNT(DISTINCT e.address) > 1""".stripMargin, + """SELECT c.id FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY c.id HAVING COUNT(DISTINCT e.address) > 1""".stripMargin, + """SELECT e.domain, COUNT(e.address) AS nb FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING COUNT(e.address) > 1""".stripMargin, + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING COUNT(e.address) > 1""".stripMargin + ) + + private val mapper = new ObjectMapper() + + /** Every value of a field named `name`, anywhere in the tree. */ + private def valuesOf(node: JsonNode, name: String): Seq[JsonNode] = { + val here = Option(node.get(name)).toSeq + val below = node.elements().asScala.toSeq.flatMap(child => valuesOf(child, name)) + here ++ below + } + + /** Every Painless script the query carries: `"script":{"source":...}` objects and the bare + * `"script":"..."` form a bucket_script uses. + */ + private def scriptsOf(root: JsonNode): Seq[String] = + valuesOf(root, "script").flatMap { s => + if (s.isTextual) Some(s.asText()) + else Option(s.get("source")).filter(_.isTextual).map(_.asText()) + } + + private val paramRef = "params\\.([A-Za-z_][A-Za-z0-9_]*)".r + + "no emitted aggregation or order key" should "be the empty string (AC 5)" in { + shapes.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + valuesOf(root, "aggs").foreach { aggs => + aggs.fieldNames().asScala.toSeq.foreach(_ should not be empty) + } + valuesOf(root, "order").foreach { order => + order.fieldNames().asScala.toSeq.foreach(_ should not be empty) + } + } + } + } + + "a bucket pipeline script" should "read only declared, null-guarded params and never a document (AC 4, AC 4b)" in { + shapes.foreach { sql => + withClue(s"[$sql] ") { + val root = mapper.readTree(queryOf(sql)) + val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") + pipelines.foreach { pipeline => + val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet + scriptsOf(pipeline).foreach { script => + script should not include "doc[" + val read = paramRef.findAllMatchIn(script).map(_.group(1)).toSet - "__now__" + read shouldBe declared + // The selector null-guards every metric it compares; a bucket_script computes a value + // (a null operand yields a null result, which Elasticsearch treats as a gap). + if (pipeline.has("script") && Option(pipeline.get("script")).exists(_.isObject)) + declared.foreach(key => script should include(s"params.$key == null")) + } + } + } + } + } + + "no emitted Painless" should "dereference or compare a possibly-null value unguarded (AC 4b)" in { + shapes.foreach { sql => + withClue(s"[$sql] ") { + scriptsOf(mapper.readTree(queryOf(sql))).foreach { script => + withClue(s"script [$script] ") { + // (a) A `( ... ? null : ... )` group must not be dereferenced: `(cond ? null : v).m()` + // invokes `m` on null whenever `cond` holds -- the doubled `.get(ChronoField.YEAR)`. + nullTernaryGroupEnds(script).foreach { end => + if (end + 1 < script.length) script.charAt(end + 1) should not be '.' + } + // (b) A `def` bound to such a group is nullable: any later `name.` dereference must be + // preceded by a `name == null` / `name != null` test. + nullableDefs(script).foreach { name => + val afterDef = script.substring(script.indexOf(s"def $name") + 4 + name.length) + if (afterDef.contains(s"$name.")) + assert( + afterDef.contains(s"$name == null") || afterDef.contains(s"$name != null"), + s"'$name.' is dereferenced without a null test" + ) + } + } + } + } + } + } + + /** Index of the `)` closing each parenthesised group whose body carries a `? null :`. */ + private def nullTernaryGroupEnds(script: String): Seq[Int] = { + val marker = "? null :" + Iterator + .iterate(script.indexOf(marker))(from => script.indexOf(marker, from + 1)) + .takeWhile(_ >= 0) + .toList // strict on both Scala legs (2.12's Iterator.toSeq is a lazy Stream) + .flatMap { at => + // Walk back to the unmatched `(` that opens the group holding this ternary... + var depth = 0 + var open = at - 1 + while (open >= 0 && !(script(open) == '(' && depth == 0)) { + script(open) match { + case ')' => depth += 1 + case '(' => depth -= 1 + case _ => + } + open -= 1 + } + // ...then forward to its matching `)`. + if (open < 0) None + else { + depth = 0 + var close = open + var found = -1 + while (found < 0 && close < script.length) { + script(close) match { + case '(' => depth += 1 + case ')' => + depth -= 1 + if (depth == 0) found = close + case _ => + } + close += 1 + } + if (found >= 0) Some(found) else None + } + } + } + + /** Names bound by `def = ;` whose initializer carries a `? null :`. */ + private def nullableDefs(script: String): Seq[String] = + "def ([A-Za-z_][A-Za-z0-9_]*) = ([^;]*);".r + .findAllMatchIn(script) + .filter(_.group(2).contains("? null :")) + .map(_.group(1)) + .toSeq +} diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 869f76061..e89a0b91f 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -563,7 +563,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "cnt": "cnt" | }, | "script": { - | "source": "params.cnt > 1" + | "source": "(params.cnt == null ? false : (params.cnt > 1))" | } | } | } @@ -577,6 +577,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "perform complex query" in { @@ -821,7 +822,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max_price": "max_price" | }, | "script": { - | "source": "params.min_price > 5.0 && params.max_price < 50.0" + | "source": "(params.min_price == null ? false : (params.min_price > 5.0)) && (params.max_price == null ? false : (params.max_price < 50.0))" | } | } | } @@ -838,6 +839,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll("<(\\d)", " < $1") .replaceAll(">(\\d)", " > $1") + .replaceAll("\\?false:", " ? false : ") } @@ -1033,7 +1035,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()", + | "source": "(params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1045,11 +1047,13 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | } |}""".stripMargin .replaceAll("\\s", "") + .replaceAll("==", " == ") .replaceAll("ChronoUnit", " ChronoUnit") .replaceAll("!=", " != ") .replaceAll("&&", " && ") .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") + .replaceAll("\\?false:", " ? false : ") } it should "handle group by with having and date time functions" in { @@ -1101,7 +1105,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "params.cnt > 1 && params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1121,6 +1125,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") + .replaceAll("\\?false:", " ? false : ") } it should "handle group by index" in { @@ -1174,7 +1179,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "params.cnt > 1 && params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1194,6 +1199,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") + .replaceAll("\\?false:", " ? false : ") } it should "handle date_parse function" in { @@ -3965,7 +3971,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "__c3": "__c3" | }, | "script": { - | "source": "params.__c3 > 1" + | "source": "(params.__c3 == null ? false : (params.__c3 > 1))" | } | } | } @@ -3979,6 +3985,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "handle HAVING COUNT(*) without alias combined with aliased aggregation" in { @@ -4022,7 +4029,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "avg_age": "avg_age" | }, | "script": { - | "source": "params.__c2 >= 1 && params.avg_age > 25" + | "source": "(params.__c2 == null ? false : (params.__c2 >= 1)) && (params.avg_age == null ? false : (params.avg_age > 25))" | } | } | } @@ -4035,6 +4042,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("&&", " && ") .replaceAll(">=", " >= ") .replaceAll("(?)>(?!=)", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "handle HAVING COUNT(*) only in HAVING clause not in SELECT" in { @@ -4077,7 +4085,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "count_all": "count_all" | }, | "script": { - | "source": "params.count_all > 1" + | "source": "(params.count_all == null ? false : (params.count_all > 1))" | } | } | } @@ -4089,6 +4097,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } // === Issue #52: ORDER BY on aggregation alias === @@ -4215,7 +4224,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "__c2": "__c2" | }, | "script": { - | "source": "params.__c2 > 1" + | "source": "(params.__c2 == null ? false : (params.__c2 > 1))" | } | } | } @@ -4227,6 +4236,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "handle HAVING COUNT(DISTINCT *) only in HAVING clause not in SELECT" in { @@ -4269,7 +4279,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "count_distinct_all": "count_distinct_all" | }, | "script": { - | "source": "params.count_distinct_all > 1" + | "source": "(params.count_distinct_all == null ? false : (params.count_distinct_all > 1))" | } | } | } @@ -4281,6 +4291,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("==", " == ") .replaceAll("&&", " && ") .replaceAll(">", " > ") + .replaceAll("\\?false:", " ? false : ") } it should "test" in { diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/function/aggregate/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/function/aggregate/package.scala index a018af0ae..db45c7923 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/function/aggregate/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/function/aggregate/package.scala @@ -109,13 +109,13 @@ package object aggregate { override def update(request: SingleSearch): BucketScriptAggregation = { val identifiers = FunctionUtils.funIdentifiers(identifier) - val params = identifiers.flatMap { - case identifier: Identifier => - val name = identifier.metricName.getOrElse(identifier.aliasOrName) - Some( - name -> request.fieldAliases.getOrElse(identifier.identifierName, name) - ) // TODO may be be a path - case _ => None + // Only the AGGREGATE operands are metrics. `funIdentifiers` also yields the script's own + // identifier (the arithmetic wrapper, no metricName), which used to register itself through + // its alias (`d -> d`) -- a self-referencing buckets_path entry (issue #54). + val params = identifiers.flatMap { operand => + operand.metricName.map { name => + name -> request.fieldAliases.getOrElse(operand.identifierName, name) + } // TODO may be be a path }.toMap this.copy(params = params) } 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 03b62f230..b3bfd47f8 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -977,30 +977,47 @@ package object sql { } lazy val paramName: String = - if (isAggregation && functions.size == 1) s"params.${metricName.getOrElse(aliasOrName)}" + if (isAggregation && functions.size == 1) metricParam else if (path.nonEmpty) s"doc['$path'].value" else "" + /** The name an AGGREGATE identifier is addressed by in a bucket pipeline: the `buckets_path` + * key, the `params.` a `bucket_selector` / `bucket_script` reads, and -- when the + * aggregate is not itself a SELECT item -- the name of the auxiliary aggregation created for + * it (`Criteria.extractAggregationFields` / `FieldSort.extractAggregationFields` alias the + * synthesised Field with it, and both `SQLAggregation.fromField` and the bridge name the + * aggregation after that alias). One rule, one name: the path resolves by construction. + * + * Aliased: the alias. Un-aliased: a name derived from the WHOLE expression, never the bare + * field name. The bare name collapsed distinct aggregates over one field (`COUNT(x)` and + * `MAX(x)` were both `x`; `auxiliaryAggs` dedups by name, so the second silently vanished and + * every HAVING term compared the first -- issue #54), let a dotted field leak into a `params.` + * key (`params.emails.address` is a nested-property read Painless rejects, and the selector's + * name scanner then dropped the condition altogether), and named an aggregate over a + * self-contained function (`MAX(ABS(salary))`, whose identifier has no field-derived `name`) + * `""` (issue #223). `COUNT(*)` keeps its long-standing `count_all` / `count_distinct_all`. + */ lazy val metricName: Option[String] = - aggregateFunction match { - case Some(af) => + aggregateFunction.map { af => + fieldAlias.getOrElse { af match { - case COUNT | _: CountAgg => - aliasOrName match { - case "*" => - if (distinct) { - Some(s"count_distinct_all") - } else { - Some(s"count_all") - } - case _ => Some(aliasOrName) - } - case _ => Some(aliasOrName) + case COUNT | _: CountAgg if name == "*" => + if (distinct) "count_distinct_all" else "count_all" + case _ => + val operand = if (distinct) s"$Distinct $name" else name + AliasUtils.normalize( + functions.reverse.foldLeft(operand)((expr, fun) => fun.toSQL(expr)) + ) } - case _ => None + } } + /** 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 script: Option[String] = if (isTemporal) { var orderedFunctions = FunctionUtils.transformFunctions(this).reverse @@ -1091,6 +1108,13 @@ package object sql { else this.baseType override def painless(context: Option[PainlessContext]): String = { + // A context-free rendering of an AGGREGATE is a bucket-pipeline rendering (`bucket_selector` + // for HAVING, `bucket_script` for arithmetic over aggregates): there is no document to read, + // only the metric Elasticsearch already computed, published under `buckets_path`. The + // transform chain (`MAX(YEAR(x))`) belongs to the METRIC aggregation's own script, rendered by + // the context-bearing call below; re-applying it here produced a `doc[...]` read -- which a + // bucket script cannot do -- and a doubled transform in the selector (issue #223). + if (context.isEmpty && isAggregation) return metricParam val orderedFunctions = FunctionUtils.transformFunctions(this).reverse var currType = this.originalType currType match { 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 8f0d55823..4d388e392 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 @@ -16,7 +16,7 @@ package app.softnetwork.elastic.sql.query -import app.softnetwork.elastic.sql.`type`.{SQLType, SQLTypes} +import app.softnetwork.elastic.sql.`type`.SQLType import app.softnetwork.elastic.sql.operator._ import app.softnetwork.elastic.sql.{ Expr, @@ -177,21 +177,13 @@ object MetricSelectorScript { case _: MultiMatchCriteria => "1 == 1" case e: Expression if e.isAggregation => - // NO FILTERING: the script is generated for all metrics - val painless = e.painless(None) - e.maybeValue match { - case Some(value) if e.operator.isInstanceOf[ComparisonOperator] => - value.out match { - case SQLTypes.Date => - s"$painless.truncatedTo(ChronoUnit.DAYS).toInstant().toEpochMilli()" - case SQLTypes.Time if e.operator.isInstanceOf[ComparisonOperator] => - s"$painless.truncatedTo(ChronoUnit.SECONDS).toInstant().toEpochMilli()" - case SQLTypes.DateTime if e.operator.isInstanceOf[ComparisonOperator] => - s"$painless.toInstant().toEpochMilli()" - case _ => painless - } - case _ => painless - } + // 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" } } 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 9ccd1b426..b02fa7703 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 @@ -16,9 +16,61 @@ package app.softnetwork.elastic.sql.query -import app.softnetwork.elastic.sql.{Expr, TokenRegex, Updateable} +import app.softnetwork.elastic.sql.{Expr, Identifier, TokenRegex, Updateable} -case object Having extends Expr("HAVING") with TokenRegex +case object Having extends Expr("HAVING") with TokenRegex { + + /** `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 + * "workaround" issue #54 itself prescribed, never tested (BIDC-2). Substituting the aliased + * SELECT item's identifier BEFORE the criteria are updated makes the reference resolve like any + * other aggregate: the alias comes back through `fieldAliases`, the selector reads `params.cnt`, + * and no extra aggregation is created since the item is already a SELECT aggregate. Scoped to + * HAVING on purpose: ORDER BY resolves an alias by name already, and WHERE must keep reading a + * bare name as a field. Relation predicates (`NESTED(...)`) are left untouched. + */ + private[query] def resolveAggregateAliases( + criteria: Criteria, + request: SingleSearch + ): Criteria = { + val aliased: Map[String, Identifier] = request.select.fields.collect { + case f if f.isAggregation && f.fieldAlias.isDefined => f.fieldAlias.get.alias -> f.identifier + }.toMap + if (aliased.isEmpty) return criteria + + def substitute(id: Identifier): Identifier = + if (id.functions.isEmpty && !id.nested) aliased.getOrElse(id.name, id) else id + + def rewrite(c: Criteria): Criteria = c match { + case p: Predicate => + p.copy(leftCriteria = rewrite(p.leftCriteria), rightCriteria = rewrite(p.rightCriteria)) + case e: GenericExpression => + e.copy( + identifier = substitute(e.identifier), + value = e.value match { + case id: Identifier => substitute(id) + case v => v + } + ) + case e: Comparison => + e.copy( + identifier = substitute(e.identifier), + value = e.value match { + case id: Identifier => substitute(id) + case v => v + } + ) + case e: BetweenExpr => e.copy(identifier = substitute(e.identifier)) + case e: InExpr[_, _] => e.copy(identifier = substitute(e.identifier)) + case e: IsNullCriteria => e.copy(identifier = substitute(e.identifier)) + case e: IsNotNullCriteria => e.copy(identifier = substitute(e.identifier)) + case other => other + } + + rewrite(criteria) + } +} case class Having(criteria: Option[Criteria]) extends Updateable { override def sql: String = criteria match { @@ -26,7 +78,9 @@ case class Having(criteria: Option[Criteria]) extends Updateable { case _ => "" } def update(request: SingleSearch): Having = - this.copy(criteria = criteria.map(_.update(request))) + this.copy(criteria = + criteria.map(c => Having.resolveAggregateAliases(c, request).update(request)) + ) override def validate(): Either[String, Unit] = criteria.map(_.validate()).getOrElse(Right(())) 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 3b5f47dda..d7918c664 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 @@ -460,66 +460,104 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { SQLTypeUtils.coerce(identifier, targetedType, context) } - protected def check(context: Option[PainlessContext], param: String): String = { + protected def check(context: Option[PainlessContext], param: String): String = + check(context, param, painlessValue(context)) + + /** `param value` in the operator's Painless spelling. `value` is the already-rendered + * right-hand side, so a caller may adapt it first (the bucket-pipeline rendering converts a + * temporal literal to epoch millis, the unit a date metric arrives in). + */ + protected def check(context: Option[PainlessContext], param: String, value: String): String = { operator match { case comparison: ComparisonOperator => comparison match { case LT => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => - return s"$param.compareTo(${painlessValue(context)}) < 0" + return s"$param.compareTo($value) < 0" case _: SQLTemporal if !isAggregation && !hasBucket => - return s"$param.isBefore(${painlessValue(context)})" + return s"$param.isBefore($value)" case _ => } case GT => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => - return s"$param.compareTo(${painlessValue(context)}) > 0" + return s"$param.compareTo($value) > 0" case _: SQLTemporal if !isAggregation && !hasBucket => - return s"$param.isAfter(${painlessValue(context)})" + return s"$param.isAfter($value)" case _ => } case EQ => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => - return s"$param.compareTo(${painlessValue(context)}) == 0" + return s"$param.compareTo($value) == 0" case _: SQLTemporal if !isAggregation && !hasBucket => - return s"$param.isEqual(${painlessValue(context)})" + return s"$param.isEqual($value)" case _ => } case NE | DIFF => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => - return s"$param.compareTo(${painlessValue(context)}) != 0" + return s"$param.compareTo($value) != 0" case _: SQLTemporal if !isAggregation && !hasBucket => - return s"$param.isEqual(${painlessValue(context)}) == false" + return s"$param.isEqual($value) == false" case _ => } case GE => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => - return s"$param.compareTo(${painlessValue(context)}) >= 0" + return s"$param.compareTo($value) >= 0" case _: SQLTemporal if !isAggregation && !hasBucket => - return s"$param.isBefore(${painlessValue(context)}) == false" + return s"$param.isBefore($value) == false" case _ => } case LE => maybeValue.map(v => v.out).getOrElse(SQLTypes.Any) match { case SQLTypes.Varchar => - return s"$param.compareTo(${painlessValue(context)}) <= 0" + return s"$param.compareTo($value) <= 0" case _: SQLTemporal if !isAggregation && !hasBucket => - return s"$param.isAfter(${painlessValue(context)}) == false" + return s"$param.isAfter($value) == false" case _ => } case _ => } - s"$param $painlessOp ${painlessValue(context)}" - case _ => s"$param$painlessOp(${painlessValue(context)})" + s"$param $painlessOp $value" + case _ => s"$param$painlessOp($value)" + } + } + + /** The rendering of an aggregate predicate for a bucket pipeline (`bucket_selector` for HAVING). + * There is no document here, only the metrics Elasticsearch already computed, read as + * `params.` (issue #223: the old rendering read `doc[...]` and re-applied the + * transform the metric aggregation had already applied). The whole predicate is ONE + * parenthesised expression whose first act is to null-guard every metric it compares -- lead + * directive, BIDC-2 AC 4b: no emitted Painless compares a possibly-null value unguarded. An + * expression (not a `def left = ...;` statement) is what composes under the `&&` / `||` the + * HAVING tree is joined with: `? :` binds looser than `||`, so an unparenthesised guard would + * swallow the right-hand side of an OR. A temporal literal on the right is converted to epoch + * millis, which is what a date metric arrives in. + */ + private def bucketPipelinePainless: String = { + val metrics: Seq[Identifier] = + identifier +: maybeValue.collect { case id: Identifier if id.isAggregation => id }.toSeq + val rhs = painlessValue(None) + val value = maybeValue match { + case Some(v) if operator.isInstanceOf[ComparisonOperator] && !v.isAggregation => + v.out match { + case SQLTypes.Date => s"$rhs.truncatedTo(ChronoUnit.DAYS).toInstant().toEpochMilli()" + case SQLTypes.Time => s"$rhs.truncatedTo(ChronoUnit.SECONDS).toInstant().toEpochMilli()" + case SQLTypes.DateTime => s"$rhs.toInstant().toEpochMilli()" + case _ => rhs + } + case _ => rhs } + val guard = metrics.map(id => s"${id.metricParam} == null").mkString(" || ") + s"($guard ? false : $painlessNot(${check(None, identifier.metricParam, value)}))" } override def painless(context: Option[PainlessContext]): String = { + // A context-free rendering of an aggregate predicate is a bucket-pipeline rendering. + if (context.isEmpty && identifier.isAggregation) return bucketPipelinePainless val innerLeft = left(context) context match { case Some(ctx) => 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 ee1801029..60e14d14a 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 @@ -32,6 +32,7 @@ import app.softnetwork.elastic.sql.schema.{ Table => Schema, TableType } +import app.softnetwork.elastic.sql.function.FunctionUtils import app.softnetwork.elastic.sql.function.aggregate.WindowFunction import app.softnetwork.elastic.sql.policy.{EnrichPolicy, EnrichPolicyType} import app.softnetwork.elastic.sql.serialization._ @@ -279,14 +280,27 @@ package object query { val orderByAggs = orderBy .map(_.sorts.flatMap(_.extractAggregationFields)) .getOrElse(Seq.empty) - (havingAggs ++ whereAggs ++ orderByAggs) + // Aggregates nested inside a SELECT bucket script (`MAX(x) - MIN(x) AS d`): the script reads + // each as `params.`, so each must exist as its own aggregation. None was ever + // created -- the bucket_script shipped with a self-referencing buckets_path and its operands + // collapsed onto one name (issue #54). The wrapper identifier itself (its head is the + // arithmetic expression, not an aggregate) is excluded by `isAggregation`; an operand that is + // also a SELECT item is excluded below like any other auxiliary candidate. + val bucketScriptAggs = selectAggs + .filter(_.isBucketScript) + .flatMap(f => FunctionUtils.funIdentifiers(f.identifier)) + .filter(_.isAggregation) + .flatMap(id => id.metricName.map(name => Field(id, Some(Alias(name))))) + // Dedup by name, keeping the first occurrence IN ORDER -- a `groupBy` here hashed the order, + // so the emitted `aggs` shuffled between runs and could not be pinned. + (havingAggs ++ whereAggs ++ orderByAggs ++ bucketScriptAggs) .filterNot(f => f.fieldAlias.exists(a => selectAggNames.contains(a.alias)) || selectAggNames.contains(f.identifier.identifierName) ) - .groupBy(_.fieldAlias.map(_.alias)) - .map(_._2.head) - .toSeq + .foldLeft(Seq.empty[Field]) { (acc, f) => + if (acc.exists(_.fieldAlias.map(_.alias) == f.fieldAlias.map(_.alias))) acc else acc :+ f + } } lazy val aggregates: Seq[Field] = selectAggs ++ auxiliaryAggs diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala index 2dd1365d1..80e68ac3d 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala @@ -1,6 +1,6 @@ package app.softnetwork.elastic.sql.parser -import app.softnetwork.elastic.sql.Identifier +import app.softnetwork.elastic.sql.{Identifier, PainlessContext} import app.softnetwork.elastic.sql.query.{AlterTable, CreateTable, SingleSearch} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -168,7 +168,19 @@ class ScriptFunctionParenSpec extends AnyFlatSpec with Matchers { // window function — so `YEAR` vanished and the aggregation was scripted without it. val id = identifierOf("SELECT MAX(YEAR(DATE_TRUNC(createdAt, MINUTE))) AS m FROM t GROUP BY id") id.functions.map(_.getClass.getSimpleName) should contain allOf ("Year", "DateTrunc") - id.painless(None) should (include("ChronoField.YEAR") and include("truncatedTo")) + // The METRIC script is the context-bearing rendering — the bridge emits `s"$ctx$expr"`, and the + // transforms land in the context's `def param1 = ...` preamble. The context-free rendering of an + // aggregate is the bucket-pipeline form (`params.`, BIDC-2 / issue #223) and carries + // no transform by design. + val ctx = PainlessContext() + val expr = id.painless(Some(ctx)) // renders first: it is what fills the context's preamble + val metricScript = s"$ctx$expr" + metricScript should (include("ChronoField.YEAR") and include("truncatedTo")) + // The bucket form reads the metric and nothing else (which exact name it reads is the naming + // rule's business, pinned in AggregationNamingSpec). + val bucketForm = id.painless(None) + bucketForm should startWith("params.") + bucketForm should (not include "doc[" and not include "ChronoField" and not include "truncatedTo") // And it is now the same aggregate the plain form produces — the two used to differ only // because one argument happened to be parenthesis-balanced and the other was not. id.functions.head.getClass shouldBe diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index 880f2adcb..43a6612d3 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -1054,6 +1054,156 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { assertSelectResult(System.nanoTime(), res) } + // --------------------------------------------------------------------------- + // Issues #54 / #223 (BIDC-2) — buckets_path and aggregation naming, bucket-pipeline scripts. + // A dedicated table: `Nice` has a single user with NO age, so every MAX/MIN over `age` is + // missing for that bucket — the null-metric contract (AC 4b) needs such a bucket, and adding it + // to `dql_users` would move every 4-row oracle above. + // --------------------------------------------------------------------------- + + it should "prepare the aggregation-naming test data (issues #54 / #223)" in { + val create = + """CREATE TABLE IF NOT EXISTS having_naming ( + | id INT NOT NULL, + | city KEYWORD, + | name KEYWORD, + | age INT, + | birthdate DATE + |);""".stripMargin + assertDdl(System.nanoTime(), client.run(create).futureValue) + + val insert = + """INSERT INTO having_naming (id, city, name, age, birthdate) VALUES + | (1, 'Paris', 'Alice', 30, '1994-01-01'), + | (2, 'Lyon', 'Bob', 40, '1984-05-10'), + | (3, 'Paris', 'Chloe', 25, '1999-07-20'), + | (4, 'Marseille', 'David', 50, '1974-03-15');""".stripMargin + assertDml(System.nanoTime(), client.run(insert).futureValue, Some(DmlResult(inserted = 4))) + + val insertNoAge = + """INSERT INTO having_naming (id, city, name, birthdate) VALUES + | (5, 'Nice', 'Eve', '2000-01-01');""".stripMargin + assertDml(System.nanoTime(), client.run(insertNoAge).futureValue, Some(DmlResult(inserted = 1))) + } + + it should "filter on an un-aliased HAVING COUNT(field) exactly like the aliased form (issue #54)" in { + val expected = Seq(Map("city" -> "Paris", "cnt" -> 2)) + val aliased = + """SELECT city, COUNT(name) AS cnt FROM having_naming + |GROUP BY city HAVING cnt > 1;""".stripMargin + assertSelectResult(System.nanoTime(), client.run(aliased).futureValue, expected) + + val unaliased = + """SELECT city, COUNT(name) AS cnt FROM having_naming + |GROUP BY city HAVING COUNT(name) > 1;""".stripMargin + assertSelectResult(System.nanoTime(), client.run(unaliased).futureValue, expected) + + // The aggregate exists only in HAVING: it is created for the filter and hidden from the rows. + val havingOnly = + """SELECT city FROM having_naming + |GROUP BY city HAVING COUNT(name) > 1;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(havingOnly).futureValue, + Seq(Map("city" -> "Paris")) + ) + } + + it should "keep two aggregates over the same field apart in HAVING (issue #54)" in { + // COUNT(age) >= 1 holds for every city but Nice; only Marseille has MAX(age) > 45. Before the + // fix both aggregates were named `age`, the second was dropped, and the count stood in for the + // max — `1 >= 1 && 1 > 45` — so no city qualified. + val sql = + """SELECT city FROM having_naming + |GROUP BY city HAVING COUNT(age) >= 1 AND MAX(age) > 45;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(sql).futureValue, + Seq(Map("city" -> "Marseille")) + ) + } + + it should "filter on HAVING over a transformed aggregate (issue #223)" in { + // MAX(YEAR(birthdate)): Paris 1999, Lyon 1984, Marseille 1974, Nice 2000. + val year = + """SELECT city, COUNT(*) AS cnt FROM having_naming + |GROUP BY city HAVING MAX(YEAR(birthdate)) > 1990;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(year).futureValue, + Seq(Map("city" -> "Paris", "cnt" -> 2), Map("city" -> "Nice", "cnt" -> 1)) + ) + + // OR across a transformed aggregate and a count: Paris by count, Marseille by MAX(ABS(age)); + // Nice has no age, so its MAX(ABS(age)) is missing and its count is 1 — it must NOT pass. + val or = + """SELECT city FROM having_naming + |GROUP BY city HAVING MAX(ABS(age)) > 45 OR COUNT(*) > 1;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(or).futureValue, + Seq(Map("city" -> "Paris"), Map("city" -> "Marseille")) + ) + } + + it should "sort buckets by an aggregate over a transform (issue #223)" in { + // MAX(ABS(age)): Marseille 50, Lyon 40, Paris 30 — distinct values, so the order is exact; + // Nice's is missing and sorts last. + val sql = + """SELECT city FROM having_naming + |GROUP BY city ORDER BY MAX(ABS(age)) DESC;""".stripMargin + val rows = collectRows(System.nanoTime(), client.run(sql).futureValue) + rows.map(_("city")) shouldBe Seq("Marseille", "Lyon", "Paris", "Nice") + } + + it should "never let a bucket whose compared metric is missing pass a HAVING comparison (AC 4b)" in { + // Nice's MAX(age) has no value. Whatever the runtime hands the bucket_selector for it, the + // guarded script yields false: the bucket is dropped by BOTH directions of the comparison, and + // the request does not fail. + val expected = Seq(Map("city" -> "Paris"), Map("city" -> "Lyon"), Map("city" -> "Marseille")) + val above = + """SELECT city FROM having_naming + |GROUP BY city HAVING MAX(age) > 0;""".stripMargin + assertSelectResult(System.nanoTime(), client.run(above).futureValue, expected) + val below = + """SELECT city FROM having_naming + |GROUP BY city HAVING MAX(age) < 1000;""".stripMargin + assertSelectResult(System.nanoTime(), client.run(below).futureValue, expected) + } + + it should "compute arithmetic over aggregates as a bucket_script (issue #54)" in { + // The operands are created as their own (hidden) aggregations and read as params; the value is + // a double, as every bucket_script result is. + val range = + """SELECT city, MAX(age) - MIN(age) AS age_range FROM having_naming + |WHERE age IS NOT NULL + |GROUP BY city ORDER BY city ASC;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(range).futureValue, + Seq( + Map("city" -> "Lyon", "age_range" -> 0.0), + Map("city" -> "Marseille", "age_range" -> 0.0), + Map("city" -> "Paris", "age_range" -> 5.0) + ) + ) + + // An operand that is also a SELECT item resolves to that item's alias — one aggregation, two uses. + val shared = + """SELECT city, MAX(age) AS oldest, MAX(age) - MIN(age) AS age_range FROM having_naming + |WHERE age IS NOT NULL + |GROUP BY city ORDER BY city ASC;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(shared).futureValue, + Seq( + Map("city" -> "Lyon", "oldest" -> 40.0, "age_range" -> 0.0), + Map("city" -> "Marseille", "oldest" -> 50.0, "age_range" -> 0.0), + Map("city" -> "Paris", "oldest" -> 30.0, "age_range" -> 5.0) + ) + ) + } + // --------------------------------------------------------------------------- // Arithmetic, IN, BETWEEN, IS NULL, LIKE, RLIKE // --------------------------------------------------------------------------- From a5f376f25f77f84767c61747ac5bac8d7e72c58b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 06:40:21 +0200 Subject: [PATCH 2/6] =?UTF-8?q?fix(sql,bridge):=20review=20follow-up=20?= =?UTF-8?q?=E2=80=94=20HAVING=20alias=20of=20a=20bucket=5Fscript,=20NOT=20?= =?UTF-8?q?on=20the=20right=20operand,=20guarded=20BETWEEN/IN,=20loud=20re?= =?UTF-8?q?jects=20(R2-1..R2-9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R2-1: HAVING of a bucket_script (MAX(x) - MIN(x) AS d ... HAVING d > 3) was silently ignored and the first commit made the statement executable; the alias now resolves to the SELECT item, the selector reads params.d from the sibling pipeline aggregation (buckets_path d -> d), and inline arithmetic over aggregates in HAVING is a validation error naming the remedy. R2-2: MetricSelectorScript negated the LEFT operand of A AND NOT B; the NOT is pushed into the right-hand expression so a missing metric still fails the negated test. R2-3: BETWEEN and IN over an aggregate bypassed the guarded bucket rendering (BETWEEN even emitted a chained comparison Painless rejects); both render through bucketPipelineCheck with their own negation. R2-4: the AC 4b guard asserts a bucket_selector is present for every HAVING shape. R2-5: IS [NOT] NULL renders the null test alone. R2-6: extractMetricNames drops __now__ (both bridge copies) so a nested-level HAVING with now - interval keeps its condition. R2-9: auxiliary aggregates dedup against SELECT by expression; an alias naming a SELECT aggregate and a different HAVING/ORDER BY aggregate is a validation error. Lead to confirm: an aggregate function in WHERE is now rejected (it used to be silently dropped while its aggregation was still created) — the house-style default, reversible. Docs: HAVING alias references and the rejected shapes. Story BIDC-2 Closes #54 Closes #223 Co-Authored-By: Claude Fable 5.1 --- .../sql/bridge/ElasticAggregation.scala | 5 +- .../elastic/sql/AggregationNamingSpec.scala | 129 ++++++++++++++++++ documentation/sql/dql_statements.md | 7 + .../sql/bridge/ElasticAggregation.scala | 5 +- .../elastic/sql/AggregationNamingSpec.scala | 129 ++++++++++++++++++ .../app/softnetwork/elastic/sql/package.scala | 5 +- .../elastic/sql/query/GroupBy.scala | 35 ++++- .../elastic/sql/query/Having.scala | 5 +- .../softnetwork/elastic/sql/query/Where.scala | 58 +++++++- .../elastic/sql/query/package.scala | 57 +++++++- .../query/HavingAggregateResolutionSpec.scala | 56 ++++++++ .../client/GatewayApiIntegrationSpec.scala | 69 ++++++++++ 12 files changed, 537 insertions(+), 23 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala diff --git a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index d0880e16f..f01cfa5e1 100644 --- a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala +++ b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala @@ -677,7 +677,10 @@ object ElasticAggregation { private def extractMetricNames(condition: String): Seq[String] = { // Pattern to extract "params.XXX" val pattern = "params\\.([a-zA-Z_][a-zA-Z0-9_]*)".r - pattern.findAllMatchIn(condition).map(_.group(1)).toSeq + // `params.__now__` is the request clock the bridge binds itself, not a metric: left in, it + // resolved Unknown and made every nested-level HAVING with `now - interval ...` drop its + // condition silently (Unknown is only tolerated at root level). + pattern.findAllMatchIn(condition).map(_.group(1)).filterNot(_ == "__now__").toSeq } // HELPER: Check if a path is a direct child diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index 398cfcbe6..a74dc0807 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -227,6 +227,120 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ).mkString } + "BETWEEN over an aggregate" should "read the guarded metric as two comparisons (AC 4b)" in { + // The document form rendered the chained `1 <= p <= 5`, which Painless rejects. + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) BETWEEN 1 AND 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x >= 1 && params.count_x <= 5))"}}}}}}}""" + ).mkString + } + + it should "keep its NOT (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT BETWEEN 1 AND 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (!(params.count_x >= 1 && params.count_x <= 5)))"}}}}}}}""" + ).mkString + } + + "IN over an aggregate" should "read the guarded metric through a list membership (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(x) IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : ([1,2].contains(params.max_x)))"}}}}}}}""" + ).mkString + } + + it should "keep its NOT (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(x) NOT IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : (!([1,2].contains(params.max_x))))"}}}}}}}""" + ).mkString + } + + "A AND NOT B" should "negate the RIGHT operand, inside its guard (R2-2)" in { + // The selector used to prefix `!` to the LEFT operand -- the exact complement of what was asked. + // The NOT is pushed into the right-hand comparison (`> 3` becomes `<= 3`) so a bucket whose + // metric is missing still fails it, as SQL's three-valued NOT would. + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", + """"script":{"source":"((params.count_x == null ? false : (params.count_x > 5))) && """, + """(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + ).mkString + } + + it should "negate a leading NOT the same way" in { + queryOf("SELECT id FROM t GROUP BY id HAVING NOT MAX(x) > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + ).mkString + } + + "a HAVING that references a bucket_script by its alias" should "read the sibling pipeline aggregation (R2-1)" in { + // `d` used to stay a bare identifier: no metric, `1 == 1`, no having_filter -- every group back. + queryOf("SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id HAVING d > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"d":{"bucket_script":{"buckets_path":{"max_x":"max_x","min_x":"min_x"},""", + """"script":"params.max_x - params.min_x"}},""", + """"max_x":{"max":{"field":"x"}},"min_x":{"min":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"d":"d"},""", + """"script":{"source":"(params.d == null ? false : (params.d > 3))"}}}}}}}""" + ).mkString + } + + "a nested-level HAVING with now - interval" should "keep its condition (R2-6)" in { + // `params.__now__` is the request clock, not a metric; resolved as Unknown it dropped the whole + // condition inside a nested bucket (Unknown is only tolerated at root level). + queryOf( + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING MAX(e.sent) > now - interval 7 day""".stripMargin + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.domain":{"terms":{"field":"emails.domain","size":65536,"min_doc_count":1},""", + """"aggs":{"max_e_sent":{"max":{"field":"emails.sent"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_e_sent":"max_e_sent"},""", + """"script":{"source":"(params.max_e_sent == null ? false : (params.max_e_sent > """, + """ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS)""", + """.toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}}}""" + ).mkString + } + + "the validator" should "reject the shapes the pipeline cannot express, loudly" in { + Seq( + "SELECT id FROM t GROUP BY id HAVING MAX(x) - MIN(x) > 3" -> + "HAVING cannot combine aggregates arithmetically inline", + "SELECT id FROM t WHERE COUNT(x) > 5 GROUP BY id" -> + "Aggregate functions are not allowed in WHERE", + "SELECT id, MIN(x) AS max_x FROM t GROUP BY id HAVING MAX(x) > 3" -> + "Alias 'max_x' names a SELECT aggregate and a different aggregate" + ).foreach { case (sql, reason) => + withClue(s"[$sql] ") { + val rejected = app.softnetwork.elastic.sql.parser.Parser(sql).swap.toOption.map(_.msg) + rejected shouldBe defined + rejected.get should include(reason) + // A boundary catch would ALSO yield a Left carrying the reason -- assert it is the grammar's. + rejected.get should not startWith app.softnetwork.elastic.sql.parser.Parser.InternalParseFailure + } + } + } + "ORDER BY over a transformed aggregate" should "name its aggregation (issue #223)" in { // The sub-aggregation key and the `order` key were both `""`. queryOf( @@ -274,6 +388,17 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { "SELECT id, COUNT(x) FROM t GROUP BY id HAVING COUNT(x) > 5", "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 5", "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020 AND COUNT(x) > 1", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) BETWEEN 1 AND 5", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT BETWEEN 1 AND 5", + "SELECT id FROM t GROUP BY id HAVING MAX(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING MAX(x) NOT IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) > 3", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING NOT MAX(x) > 3", + "SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id HAVING d > 3", + "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING COUNT(x) > 1 ORDER BY COUNT(x) DESC", + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING MAX(e.sent) > now - interval 7 day""".stripMargin, "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND MAX(x) > 3", "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(x) > 3 ORDER BY MIN(x) DESC", "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND COUNT(DISTINCT x) > 2", @@ -339,6 +464,10 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { shapes.foreach { sql => withClue(s"[$sql] ") { val root = mapper.readTree(queryOf(sql)) + // A HAVING whose selector was dropped (the nested-level class this story fixed) would leave + // nothing below to check -- the guard must not pass vacuously. + if (sql.toUpperCase.contains("HAVING")) + valuesOf(root, "bucket_selector") should not be empty val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") pipelines.foreach { pipeline => val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 65f929927..3dface591 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -519,6 +519,13 @@ ORDER BY COUNT(*) DESC; wrap a transform (`HAVING MAX(YEAR(birthdate)) > 1990`, `ORDER BY MAX(ABS(age)) DESC`). - Arithmetic over aggregates is computed per group (`MAX(price) - MIN(price) AS price_range`); the operands are computed as hidden aggregations of the group. +- `HAVING` may reference a `SELECT` aggregate by its alias (`COUNT(*) AS cnt ... HAVING cnt > 1`), + including the alias of an arithmetic expression over aggregates (`... AS price_range ... HAVING + 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`. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter script null-checks every metric before comparing it. diff --git a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index cd1d398d8..41ccf34e1 100644 --- a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala +++ b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala @@ -673,7 +673,10 @@ object ElasticAggregation { private def extractMetricNames(condition: String): Seq[String] = { // Pattern to extract "params.XXX" val pattern = "params\\.([a-zA-Z_][a-zA-Z0-9_]*)".r - pattern.findAllMatchIn(condition).map(_.group(1)).toSeq + // `params.__now__` is the request clock the bridge binds itself, not a metric: left in, it + // resolved Unknown and made every nested-level HAVING with `now - interval ...` drop its + // condition silently (Unknown is only tolerated at root level). + pattern.findAllMatchIn(condition).map(_.group(1)).filterNot(_ == "__now__").toSeq } // HELPER: Check if a path is a direct child diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index 398cfcbe6..a74dc0807 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -227,6 +227,120 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ).mkString } + "BETWEEN over an aggregate" should "read the guarded metric as two comparisons (AC 4b)" in { + // The document form rendered the chained `1 <= p <= 5`, which Painless rejects. + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) BETWEEN 1 AND 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x >= 1 && params.count_x <= 5))"}}}}}}}""" + ).mkString + } + + it should "keep its NOT (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT BETWEEN 1 AND 5") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (!(params.count_x >= 1 && params.count_x <= 5)))"}}}}}}}""" + ).mkString + } + + "IN over an aggregate" should "read the guarded metric through a list membership (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(x) IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : ([1,2].contains(params.max_x)))"}}}}}}}""" + ).mkString + } + + it should "keep its NOT (AC 4b)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING MAX(x) NOT IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : (!([1,2].contains(params.max_x))))"}}}}}}}""" + ).mkString + } + + "A AND NOT B" should "negate the RIGHT operand, inside its guard (R2-2)" in { + // The selector used to prefix `!` to the LEFT operand -- the exact complement of what was asked. + // The NOT is pushed into the right-hand comparison (`> 3` becomes `<= 3`) so a bucket whose + // metric is missing still fails it, as SQL's three-valued NOT would. + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", + """"script":{"source":"((params.count_x == null ? false : (params.count_x > 5))) && """, + """(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + ).mkString + } + + it should "negate a leading NOT the same way" in { + queryOf("SELECT id FROM t GROUP BY id HAVING NOT MAX(x) > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"max_x":{"max":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", + """"script":{"source":"(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + ).mkString + } + + "a HAVING that references a bucket_script by its alias" should "read the sibling pipeline aggregation (R2-1)" in { + // `d` used to stay a bare identifier: no metric, `1 == 1`, no having_filter -- every group back. + queryOf("SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id HAVING d > 3") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"d":{"bucket_script":{"buckets_path":{"max_x":"max_x","min_x":"min_x"},""", + """"script":"params.max_x - params.min_x"}},""", + """"max_x":{"max":{"field":"x"}},"min_x":{"min":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"d":"d"},""", + """"script":{"source":"(params.d == null ? false : (params.d > 3))"}}}}}}}""" + ).mkString + } + + "a nested-level HAVING with now - interval" should "keep its condition (R2-6)" in { + // `params.__now__` is the request clock, not a metric; resolved as Unknown it dropped the whole + // condition inside a nested bucket (Unknown is only tolerated at root level). + queryOf( + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING MAX(e.sent) > now - interval 7 day""".stripMargin + ) shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"e":{"nested":{"path":"emails"},""", + """"aggs":{"e.domain":{"terms":{"field":"emails.domain","size":65536,"min_doc_count":1},""", + """"aggs":{"max_e_sent":{"max":{"field":"emails.sent"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"max_e_sent":"max_e_sent"},""", + """"script":{"source":"(params.max_e_sent == null ? false : (params.max_e_sent > """, + """ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS)""", + """.toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}}}""" + ).mkString + } + + "the validator" should "reject the shapes the pipeline cannot express, loudly" in { + Seq( + "SELECT id FROM t GROUP BY id HAVING MAX(x) - MIN(x) > 3" -> + "HAVING cannot combine aggregates arithmetically inline", + "SELECT id FROM t WHERE COUNT(x) > 5 GROUP BY id" -> + "Aggregate functions are not allowed in WHERE", + "SELECT id, MIN(x) AS max_x FROM t GROUP BY id HAVING MAX(x) > 3" -> + "Alias 'max_x' names a SELECT aggregate and a different aggregate" + ).foreach { case (sql, reason) => + withClue(s"[$sql] ") { + val rejected = app.softnetwork.elastic.sql.parser.Parser(sql).swap.toOption.map(_.msg) + rejected shouldBe defined + rejected.get should include(reason) + // A boundary catch would ALSO yield a Left carrying the reason -- assert it is the grammar's. + rejected.get should not startWith app.softnetwork.elastic.sql.parser.Parser.InternalParseFailure + } + } + } + "ORDER BY over a transformed aggregate" should "name its aggregation (issue #223)" in { // The sub-aggregation key and the `order` key were both `""`. queryOf( @@ -274,6 +388,17 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { "SELECT id, COUNT(x) FROM t GROUP BY id HAVING COUNT(x) > 5", "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 5", "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020 AND COUNT(x) > 1", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) BETWEEN 1 AND 5", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT BETWEEN 1 AND 5", + "SELECT id FROM t GROUP BY id HAVING MAX(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING MAX(x) NOT IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) > 3", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING NOT MAX(x) > 3", + "SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id HAVING d > 3", + "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING COUNT(x) > 1 ORDER BY COUNT(x) DESC", + """SELECT e.domain FROM customers c JOIN UNNEST(c.emails) AS e + |GROUP BY e.domain HAVING MAX(e.sent) > now - interval 7 day""".stripMargin, "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND MAX(x) > 3", "SELECT id, COUNT(x) AS c FROM t GROUP BY id HAVING MAX(x) > 3 ORDER BY MIN(x) DESC", "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND COUNT(DISTINCT x) > 2", @@ -339,6 +464,10 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { shapes.foreach { sql => withClue(s"[$sql] ") { val root = mapper.readTree(queryOf(sql)) + // A HAVING whose selector was dropped (the nested-level class this story fixed) would leave + // nothing below to check -- the guard must not pass vacuously. + if (sql.toUpperCase.contains("HAVING")) + valuesOf(root, "bucket_selector") should not be empty val pipelines = valuesOf(root, "bucket_selector") ++ valuesOf(root, "bucket_script") pipelines.foreach { pipeline => val declared = pipeline.get("buckets_path").fieldNames().asScala.toSet 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 b3bfd47f8..9d8e6b3e8 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -917,7 +917,10 @@ package object sql { lazy val allMetricsPath: Map[String, String] = { metricName match { case Some(name) => Map(name -> name) - case _ => Map.empty + // 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 } } 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 4d388e392..7ae6ad4b9 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 @@ -165,18 +165,29 @@ object MetricSelectorScript { case AND | OR => op.painless(None) case _ => throw new IllegalArgumentException(s"Unsupported logical operator: $op") } - val not = maybeNot.nonEmpty - if (group || not) - s"${maybeNot.map(_ => "!").getOrElse("")}($leftStr) $opStr ($rightStr)" - else - s"$leftStr $opStr $rightStr" + // `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(_) => + negated(right) match { + case Some(n) => s"($leftStr) $opStr ${metricSelector(n)}" + case None => 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 _: MultiMatchCriteria => "1 == 1" - case e: Expression if e.isAggregation => + 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 @@ -186,6 +197,18 @@ object MetricSelectorScript { e.painless(None) case _ => "1 == 1" } + + /** The single expression `c` with its own NOT toggled, when `c` is one that carries a NOT. */ + private def negated(c: Criteria): Option[Criteria] = { + def toggle(not: Option[NOT.type]): Option[NOT.type] = if (not.isDefined) None else Some(NOT) + c match { + case e: GenericExpression => Some(e.copy(maybeNot = toggle(e.maybeNot))) + case e: Comparison => Some(e.copy(maybeNot = toggle(e.maybeNot))) + case e: BetweenExpr => Some(e.copy(maybeNot = toggle(e.maybeNot))) + case e: InExpr[_, _] => Some(e.copy(maybeNot = toggle(e.maybeNot))) + case _ => None + } + } } case class BucketIncludesExcludes(values: Set[String] = Set.empty, regex: Option[String] = None) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala index b02fa7703..5e7dd1e21 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 @@ -34,8 +34,11 @@ 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.fieldAlias.isDefined => f.fieldAlias.get.alias -> f.identifier + case f if (f.isAggregation || f.isBucketScript) && f.fieldAlias.isDefined => + f.fieldAlias.get.alias -> f.identifier }.toMap if (aliased.isEmpty) return criteria diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala index d7918c664..a5f17f36e 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 @@ -537,9 +537,26 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * swallow the right-hand side of an OR. A temporal literal on the right is converted to epoch * millis, which is what a date metric arrives in. */ - private def bucketPipelinePainless: String = { - val metrics: Seq[Identifier] = - identifier +: maybeValue.collect { case id: Identifier if id.isAggregation => id }.toSeq + protected def bucketPipelinePainless: String = { + val param = identifier.metricParam + operator match { + // A null test IS the guard -- guarding it again would render the contradiction + // `(p == null ? false : (p == null))`. Unreachable from SQL today (`IS NULL` takes a bare + // name), kept total so a grammar widening cannot ship it. + 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(" || ") + s"($guard ? false : $painlessNot(${bucketPipelineCheck(param)}))" + } + } + + /** 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. + */ + protected def bucketPipelineCheck(param: String): String = { val rhs = painlessValue(None) val value = maybeValue match { case Some(v) if operator.isInstanceOf[ComparisonOperator] && !v.isAggregation => @@ -551,13 +568,21 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } case _ => rhs } - val guard = metrics.map(id => s"${id.metricParam} == null").mkString(" || ") - s"($guard ? false : $painlessNot(${check(None, identifier.metricParam, value)}))" + check(None, param, value) } + /** True when this predicate reads a bucket-pipeline metric: an aggregate, or the alias of a + * SELECT `bucket_script` item (`MAX(x) - MIN(x) AS d ... HAVING d > 3` -- the alias is resolved + * by `Having.resolveAggregateAliases`, so the identifier is the arithmetic wrapper carrying `d` + * as its alias; Elasticsearch lets a `bucket_selector` read a sibling pipeline aggregation by + * name). + */ + def referencesBucketMetric: Boolean = + identifier.isAggregation || (identifier.hasAggregation && identifier.fieldAlias.isDefined) + override def painless(context: Option[PainlessContext]): String = { // A context-free rendering of an aggregate predicate is a bucket-pipeline rendering. - if (context.isEmpty && identifier.isAggregation) return bucketPipelinePainless + if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless val innerLeft = left(context) context match { case Some(ctx) => @@ -793,8 +818,18 @@ case class InExpr[R, +T <: Value[R]]( override def asFilter(currentQuery: Option[ElasticBoolQuery]): ElasticFilter = this - override def painless(context: Option[PainlessContext]): String = + override def painless(context: Option[PainlessContext]): String = { + if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless s"$painlessNot${identifier.painless(context)}$painlessOp(${painlessValue(context)})" + } + + // `[v1,v2].contains(params.)` -- the guarded bucket form of ` IN (v1, v2)`. + // IN is a ComparisonOperator, so `painlessNot` is "" (a comparison folds its NOT into the + // operator, which this form never uses): the negation is rendered here. + override protected def bucketPipelineCheck(param: String): String = { + val membership = s"${values.painless(None)}.contains($param)" + if (maybeNot.isDefined) s"!($membership)" else membership + } } @@ -826,6 +861,7 @@ case class BetweenExpr( } override def painless(context: Option[PainlessContext]): String = { + if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless context match { case Some(ctx) => ctx.addParam(identifier) match { @@ -844,6 +880,14 @@ case class BetweenExpr( s"$painlessNot(${fromTo.from} <= ${left(context)} <= ${fromTo.to})" } + // The guarded bucket form of ` BETWEEN a AND b` -- two comparisons, not the chained + // `a <= p <= b` the document form used (Painless rejects `boolean <= int`). BETWEEN is a + // ComparisonOperator, so `painlessNot` is "": the negation is rendered here. + override protected def bucketPipelineCheck(param: String): String = { + val range = s"$param >= ${fromTo.from} && $param <= ${fromTo.to}" + if (maybeNot.isDefined) s"!($range)" else range + } + } case class DistanceCriteria( 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 60e14d14a..136e2a68f 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 @@ -267,8 +267,10 @@ package object query { // Aggregations referenced only in HAVING, WHERE, or ORDER BY clauses (not in SELECT) private lazy val auxiliaryAggs: Seq[Field] = { - val selectAggNames = selectAggs.flatMap(_.fieldAlias.map(_.alias)).toSet ++ - selectAggs.map(_.identifier.identifierName).toSet + // Dedup against SELECT by EXPRESSION only. Matching by alias too let a user alias hijack a + // derived metric name -- `SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3` read MIN under + // `params.max_x`; such a collision is now rejected by `validate()` instead. + val selectAggNames = selectAggs.map(_.identifier.identifierName).toSet val havingAggs = having .flatMap(_.criteria) .map(_.extractAggregationFields) @@ -294,10 +296,7 @@ package object query { // Dedup by name, keeping the first occurrence IN ORDER -- a `groupBy` here hashed the order, // so the emitted `aggs` shuffled between runs and could not be pinned. (havingAggs ++ whereAggs ++ orderByAggs ++ bucketScriptAggs) - .filterNot(f => - f.fieldAlias.exists(a => selectAggNames.contains(a.alias)) || - selectAggNames.contains(f.identifier.identifierName) - ) + .filterNot(f => selectAggNames.contains(f.identifier.identifierName)) .foldLeft(Seq.empty[Field]) { (acc, f) => if (acc.exists(_.fieldAlias.map(_.alias) == f.fieldAlias.map(_.alias))) acc else acc :+ f } @@ -338,6 +337,52 @@ package object query { _ <- having.map(_.validate()).getOrElse(Right(())) _ <- orderBy.map(_.validate()).getOrElse(Right(())) _ <- limit.map(_.validate()).getOrElse(Right(())) + _ <- { + // An aggregate in WHERE has no document-level query form: the bridge rendered it as + // `match_all` and the predicate was silently DROPPED while its aggregation was still + // created (BIDC-2 T2b). Loud over silent (the #205 / #280 rule). Lead to confirm the + // product choice (reject, as here, vs rewrite as HAVING). + where + .flatMap(_.criteria) + .map(_.extractAggregationFields) + .getOrElse(Nil) + .headOption match { + case Some(f) => + Left( + s"Aggregate functions are not allowed in WHERE (found ${f.identifier.sql}); use HAVING" + ) + case None => Right(()) + } + } + _ <- { + // Arithmetic over aggregates written INLINE in HAVING (`HAVING MAX(x) - MIN(x) > 3`) has no + // aggregation to read from and was silently dropped. Alias it in SELECT and reference the + // alias (`... AS d ... HAVING d > 3`), which IS supported (Having.resolveAggregateAliases). + having + .flatMap(_.criteria) + .map(_.referencedIdentifiers) + .getOrElse(Nil) + .find(id => !id.isAggregation && id.hasAggregation && id.fieldAlias.isEmpty) match { + case Some(id) => + Left( + s"HAVING cannot combine aggregates arithmetically inline (${id.sql}); alias the expression in SELECT and reference the alias" + ) + case None => Right(()) + } + } + _ <- { + // A HAVING / ORDER BY aggregate whose derived name equals a SELECT alias of a DIFFERENT + // aggregate (`SELECT MIN(x) AS max_x ... HAVING MAX(x) > 3`) would read the wrong metric + // under that name -- reject instead of colliding. + val selectAliases = selectAggs.flatMap(_.fieldAlias.map(_.alias)).toSet + auxiliaryAggs.flatMap(_.fieldAlias.map(_.alias)).find(selectAliases.contains) match { + case Some(alias) => + Left( + s"Alias '$alias' names a SELECT aggregate and a different aggregate referenced in HAVING or ORDER BY; rename one of them" + ) + case None => Right(()) + } + } /*_ <- { // validate that having clauses are only applied when group by is present if (having.isDefined && groupBy.isEmpty) { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala new file mode 100644 index 000000000..0a1cd2efc --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala @@ -0,0 +1,56 @@ +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.parser.Parser +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** BIDC-2 (issues #54 / #223): what `validate()` accepts and rejects around aggregates in HAVING + * and WHERE. Rejections are asserted as the GRAMMAR's (a boundary catch would also yield a `Left` + * carrying the reason, so `not startWith Parser.InternalParseFailure` is what makes each case + * falsifiable). + */ +class HavingAggregateResolutionSpec extends AnyFlatSpec with Matchers { + + private def rejection(sql: String): String = + Parser(sql).swap.toOption.map(_.msg).getOrElse(fail(s"expected a rejection for [$sql]")) + + "a HAVING referencing a SELECT aggregate by its alias" should "be accepted" in { + Seq( + "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING cnt > 1", + "SELECT id, MAX(YEAR(createdAt)) AS y FROM t GROUP BY id HAVING y > 2020", + // the alias of an arithmetic expression over aggregates (a bucket_script) as well + "SELECT id, MAX(x) - MIN(x) AS d FROM t GROUP BY id HAVING d > 3", + // the same expression repeated, aliased or not, is one aggregation + "SELECT id, COUNT(x) AS cnt FROM t GROUP BY id HAVING COUNT(x) > 1 ORDER BY COUNT(x) DESC" + ).foreach { sql => + withClue(s"[$sql] ") { + Parser(sql).isRight shouldBe true + } + } + } + + "an aggregate in WHERE" should "be rejected, naming HAVING (lead to confirm the product choice)" in { + // It used to parse, create the aggregation and silently DROP the predicate (`match_all`). + val msg = rejection("SELECT id FROM t WHERE COUNT(x) > 5 GROUP BY id") + msg should include("Aggregate functions are not allowed in WHERE") + msg should include("COUNT(x)") + msg should include("use HAVING") + msg should not startWith Parser.InternalParseFailure + } + + "arithmetic over aggregates written inline in HAVING" should "be rejected, naming the remedy" in { + // No aggregation to read from: it was silently dropped. Aliased in SELECT it is supported. + val msg = rejection("SELECT id FROM t GROUP BY id HAVING MAX(x) - MIN(x) > 3") + msg should include("HAVING cannot combine aggregates arithmetically inline") + msg should include("alias the expression in SELECT") + msg should not startWith Parser.InternalParseFailure + } + + "a SELECT alias equal to the derived name of a different HAVING aggregate" should "be rejected" in { + // Before: the HAVING read MIN(x) under `params.max_x` -- a silent wrong answer. + val msg = rejection("SELECT id, MIN(x) AS max_x FROM t GROUP BY id HAVING MAX(x) > 3") + msg should include("Alias 'max_x'") + msg should include("rename one of them") + msg should not startWith Parser.InternalParseFailure + } +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index 43a6612d3..b3096d426 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -1204,6 +1204,75 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { ) } + it should "filter on a bucket_script through its alias (R2-1)" in { + // The bucket_selector reads the sibling pipeline aggregation by name; only Paris (30 - 25 = 5) + // clears 3. Before the fix the alias was a bare name, the selector was dropped, all came back. + val sql = + """SELECT city, MAX(age) - MIN(age) AS age_range FROM having_naming + |WHERE age IS NOT NULL + |GROUP BY city HAVING age_range > 3;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(sql).futureValue, + Seq(Map("city" -> "Paris", "age_range" -> 5.0)) + ) + } + + it should "apply BETWEEN, IN and NOT to aggregates in HAVING (AC 4b, R2-2, R2-3)" in { + // COUNT(name): Paris 2, others 1 (Nice included -- its NAME is set, only its age is missing). + val between = + """SELECT city FROM having_naming + |GROUP BY city HAVING COUNT(name) BETWEEN 2 AND 3;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(between).futureValue, + Seq(Map("city" -> "Paris")) + ) + + val notBetween = + """SELECT city FROM having_naming + |GROUP BY city HAVING COUNT(name) NOT BETWEEN 2 AND 3;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(notBetween).futureValue, + Seq(Map("city" -> "Lyon"), Map("city" -> "Marseille"), Map("city" -> "Nice")) + ) + + // MAX(age): Paris 30, Lyon 40, Marseille 50, Nice missing. + val in = + """SELECT city FROM having_naming + |GROUP BY city HAVING MAX(age) IN (40, 50);""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(in).futureValue, + Seq(Map("city" -> "Lyon"), Map("city" -> "Marseille")) + ) + + // `A AND NOT B`: the NOT belongs to the RIGHT operand (it used to negate the left one), and a + // bucket whose metric is missing (Nice) must still fail `NOT MAX(age) > 45`, as in SQL. + val andNot = + """SELECT city FROM having_naming + |GROUP BY city HAVING COUNT(*) >= 1 AND NOT MAX(age) > 45;""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(andNot).futureValue, + Seq(Map("city" -> "Paris"), Map("city" -> "Lyon")) + ) + } + + it should "reject an aggregate in WHERE instead of silently dropping it (lead to confirm)" in { + val sql = + """SELECT city FROM having_naming + |WHERE COUNT(name) > 1 + |GROUP BY city;""".stripMargin + val res = client.run(sql).futureValue + renderResults(System.nanoTime(), res) + res.isSuccess shouldBe false + res.error.map(_.message).getOrElse("") should include( + "Aggregate functions are not allowed in WHERE" + ) + } + // --------------------------------------------------------------------------- // Arithmetic, IN, BETWEEN, IS NULL, LIKE, RLIKE // --------------------------------------------------------------------------- From b09aaf27f21d5c67572d6a84f704518ff8e02959 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 11:18:21 +0200 Subject: [PATCH 3/6] fix(sql): IN over an aggregate compares the buckets_path metric with Painless == (R2-3, live) The ES 8.18 leg found that the bucket form [v1,v2].contains(params.) never matched: a buckets_path value arrives as a boxed Double while the literals are Integers, and List.contains uses Java equals. The membership is now rendered as a disjunction of Painless == comparisons, which promote numerics and use equals for strings; the two IN pins in both bridge copies follow. Story BIDC-2 Closes #54 Closes #223 Co-Authored-By: Claude Fable 5.1 --- .../elastic/sql/AggregationNamingSpec.scala | 4 ++-- .../elastic/sql/AggregationNamingSpec.scala | 4 ++-- .../app/softnetwork/elastic/sql/query/Where.scala | 11 +++++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index a74dc0807..47390e32f 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -254,7 +254,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : ([1,2].contains(params.max_x)))"}}}}}}}""" + """"script":{"source":"(params.max_x == null ? false : (params.max_x == 1 || params.max_x == 2))"}}}}}}}""" ).mkString } @@ -264,7 +264,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (!([1,2].contains(params.max_x))))"}}}}}}}""" + """"script":{"source":"(params.max_x == null ? false : (!(params.max_x == 1 || params.max_x == 2)))"}}}}}}}""" ).mkString } diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index a74dc0807..47390e32f 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -254,7 +254,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : ([1,2].contains(params.max_x)))"}}}}}}}""" + """"script":{"source":"(params.max_x == null ? false : (params.max_x == 1 || params.max_x == 2))"}}}}}}}""" ).mkString } @@ -264,7 +264,7 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (!([1,2].contains(params.max_x))))"}}}}}}}""" + """"script":{"source":"(params.max_x == null ? false : (!(params.max_x == 1 || params.max_x == 2)))"}}}}}}}""" ).mkString } 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 a5f17f36e..7c59c94f2 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 @@ -823,11 +823,14 @@ case class InExpr[R, +T <: Value[R]]( s"$painlessNot${identifier.painless(context)}$painlessOp(${painlessValue(context)})" } - // `[v1,v2].contains(params.)` -- the guarded bucket form of ` IN (v1, v2)`. - // IN is a ComparisonOperator, so `painlessNot` is "" (a comparison folds its NOT into the - // operator, which this form never uses): the negation is rendered here. + // `params. == v1 || params. == v2` -- the guarded bucket form of + // ` IN (v1, v2)`. NOT `[v1,v2].contains(p)`: a buckets_path value arrives as a boxed + // Double and the literals are Integers, so `List.contains` (Java `equals`) never matched -- + // measured live, `MAX(age) IN (40, 50)` returned no bucket. Painless `==` promotes numerics and + // uses `equals` for strings. IN is a ComparisonOperator, so `painlessNot` is "" (a comparison + // folds its NOT into the operator, which this form never uses): the negation is rendered here. override protected def bucketPipelineCheck(param: String): String = { - val membership = s"${values.painless(None)}.contains($param)" + val membership = values.values.map(v => s"$param == ${v.painless(None)}").mkString(" || ") if (maybeNot.isDefined) s"!($membership)" else membership } From b5a86cf9b16367a08dc253bd9f7c835a7169efaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 11:23:07 +0200 Subject: [PATCH 4/6] fix(sql): reject an aggregate in the WHERE of every statement kind, DELETE and UPDATE included (S2-2) The reject lived only in SingleSearch.validate(), so DELETE FROM t WHERE COUNT(x) > 5 still became match_all and wiped the index, and the same UPDATE touched every document (#280 data-loss family). The arm now lives in Where.validate(), and Update / Delete gain a validate() that validates their WHERE; Parser.apply already validates every parsed statement, so the parse path covers DML. Pinned in HavingAggregateResolutionSpec (DELETE, UPDATE, AND-combined; plain forms stay accepted) and live (both rejected, count and ages unchanged). Also: negated()'s compound fallback documented as grammar-unreachable. Story BIDC-2 Closes #54 Closes #223 Co-Authored-By: Claude Fable 5.1 --- .../elastic/sql/query/GroupBy.scala | 4 ++- .../softnetwork/elastic/sql/query/Where.scala | 16 +++++++++-- .../elastic/sql/query/package.scala | 27 +++++++------------ .../query/HavingAggregateResolutionSpec.scala | 19 +++++++++++++ .../client/GatewayApiIntegrationSpec.scala | 27 +++++++++++++++++++ 5 files changed, 73 insertions(+), 20 deletions(-) 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 7ae6ad4b9..d449058d0 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 @@ -176,7 +176,9 @@ object MetricSelectorScript { case Some(_) => negated(right) match { case Some(n) => s"($leftStr) $opStr ${metricSelector(n)}" - case None => 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 None => s"($leftStr) $opStr !($rightStr)" } case None if group => s"($leftStr) $opStr ($rightStr)" case None => s"$leftStr $opStr $rightStr" 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 7c59c94f2..7cbcb0a17 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 @@ -1072,8 +1072,20 @@ case class Where(criteria: Option[Criteria]) extends Updateable { this.copy(criteria = criteria.map(_.update(request))) override def validate(): Either[String, Unit] = criteria match { - case Some(c) => c.validate() - case _ => Right(()) + case Some(c) => + // An aggregate has no document-level query form: the bridge rendered it as `match_all` and + // the predicate was silently DROPPED -- a wrong group set for a SELECT, EVERY document for a + // DELETE or UPDATE (#280 family). Checked here, not in SingleSearch.validate(), so every + // statement kind that carries a WHERE is covered. Lead to confirm the product choice (reject, + // as here, vs rewrite as HAVING under a GROUP BY). + c.extractAggregationFields.headOption match { + case Some(f) => + Left( + s"Aggregate functions are not allowed in WHERE (found ${f.identifier.sql}); use HAVING" + ) + case None => c.validate() + } + case _ => Right(()) } def nestedElements: Seq[NestedElement] = criteria match { 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 136e2a68f..f329d2d5a 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 @@ -337,23 +337,8 @@ package object query { _ <- having.map(_.validate()).getOrElse(Right(())) _ <- orderBy.map(_.validate()).getOrElse(Right(())) _ <- limit.map(_.validate()).getOrElse(Right(())) - _ <- { - // An aggregate in WHERE has no document-level query form: the bridge rendered it as - // `match_all` and the predicate was silently DROPPED while its aggregation was still - // created (BIDC-2 T2b). Loud over silent (the #205 / #280 rule). Lead to confirm the - // product choice (reject, as here, vs rewrite as HAVING). - where - .flatMap(_.criteria) - .map(_.extractAggregationFields) - .getOrElse(Nil) - .headOption match { - case Some(f) => - Left( - s"Aggregate functions are not allowed in WHERE (found ${f.identifier.sql}); use HAVING" - ) - case None => Right(()) - } - } + // (An aggregate in WHERE is rejected by Where.validate() itself -- run above through + // `where.map(_.validate())` -- so DELETE / UPDATE are covered too; see there.) _ <- { // Arithmetic over aggregates written INLINE in HAVING (`HAVING MAX(x) - MIN(x) > 3`) has no // aggregation to read from and was silently dropped. Alias it in SELECT and reference the @@ -766,6 +751,11 @@ package object query { } .mkString(", ")}${where.map(w => s"${w.sql}").getOrElse("")}" + // The parse path validates every statement kind; without this, an aggregate in a DML WHERE + // (`UPDATE t SET ... WHERE COUNT(x) > 5`) slipped through as `match_all` and touched EVERY + // document (BIDC-2 review, S2-2). + override def validate(): Either[String, Unit] = where.map(_.validate()).getOrElse(Right(())) + lazy val customPipeline: IngestPipeline = IngestPipeline( s"update-$table-${Instant.now.toEpochMilli}", IngestPipelineType.Custom, @@ -788,6 +778,9 @@ package object query { case class Delete(table: Table, where: Option[Where]) extends DmlStatement { override def sql: String = s"DELETE FROM ${table.name}${asString(where)}" + + // `DELETE FROM t WHERE COUNT(x) > 5` used to become `match_all` and WIPE the index (S2-2). + override def validate(): Either[String, Unit] = where.map(_.validate()).getOrElse(Right(())) } sealed trait FileFormat extends Token { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala index 0a1cd2efc..afe6023dd 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala @@ -38,6 +38,25 @@ class HavingAggregateResolutionSpec extends AnyFlatSpec with Matchers { msg should not startWith Parser.InternalParseFailure } + it should "be rejected in DELETE and UPDATE too, where the drop touched every document (S2-2)" in { + // `DELETE FROM t WHERE COUNT(x) > 5` became `match_all` and wiped the index; the same UPDATE + // updated every row. The check lives in Where.validate() so every statement kind gets it. + Seq( + "DELETE FROM t WHERE COUNT(x) > 5", + "UPDATE t SET a = 1 WHERE COUNT(x) > 5", + "DELETE FROM t WHERE id = 1 AND MAX(x) > 5" + ).foreach { sql => + withClue(s"[$sql] ") { + val msg = rejection(sql) + msg should include("Aggregate functions are not allowed in WHERE") + msg should not startWith Parser.InternalParseFailure + } + } + // The plain forms are untouched. + Parser("DELETE FROM t WHERE id = 1").isRight shouldBe true + Parser("UPDATE t SET a = 1 WHERE id = 1").isRight shouldBe true + } + "arithmetic over aggregates written inline in HAVING" should "be rejected, naming the remedy" in { // No aggregation to read from: it was silently dropped. Aliased in SELECT it is supported. val msg = rejection("SELECT id FROM t GROUP BY id HAVING MAX(x) - MIN(x) > 3") diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index b3096d426..2c299ef26 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -1273,6 +1273,33 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { ) } + it should "reject an aggregate in a DELETE or UPDATE WHERE and touch nothing (S2-2, lead to confirm)" in { + // Before: the predicate became `match_all` -- the DELETE wiped the index, the UPDATE hit every + // document. The count and the ages must be exactly what they were. + val snapshot = + "SELECT COUNT(*) AS n, MAX(age) AS oldest, MIN(age) AS youngest FROM having_naming;" + val before = collectRows(System.nanoTime(), client.run(snapshot).futureValue) + before shouldBe Seq(Map("n" -> 5, "oldest" -> 50.0, "youngest" -> 25.0)) + + val delete = client.run("DELETE FROM having_naming WHERE COUNT(name) > 1;").futureValue + renderResults(System.nanoTime(), delete) + delete.isSuccess shouldBe false + delete.error.map(_.message).getOrElse("") should include( + "Aggregate functions are not allowed in WHERE" + ) + + val update = + client.run("UPDATE having_naming SET age = 0 WHERE COUNT(name) > 1;").futureValue + renderResults(System.nanoTime(), update) + update.isSuccess shouldBe false + update.error.map(_.message).getOrElse("") should include( + "Aggregate functions are not allowed in WHERE" + ) + + val after = collectRows(System.nanoTime(), client.run(snapshot).futureValue) + after shouldBe before + } + // --------------------------------------------------------------------------- // Arithmetic, IN, BETWEEN, IS NULL, LIKE, RLIKE // --------------------------------------------------------------------------- From a363f57952bb362aab5879330f996aa0596c83f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 11:50:58 +0200 Subject: [PATCH 5/6] fix(sql): validate the source WHERE of CREATE ENRICH POLICY; IN type-checks the element, not the array (third look) Third-look tidy-up on top of the BIDC-2 review follow-ups (verdict: approve, no HIGH/MEDIUM residual). - T3-1 `CreateEnrichPolicy.validate()` never validated its source WHERE, so an aggregate there was dropped and the policy enriched from EVERY source document (`match_all`). It now uses the same `where.map(_.validate())` idiom as `Update`/`Delete`, which brings it under the S2-2 aggregate reject. - T3-3 `HAVING COUNT(x) IN (1, 2)` was rejected by the shared `Expression.validate` comparing the element with the list's ARRAY type (`BIGINT` vs `ARRAY`), while the untyped `MAX(x) IN (...)` passed through `Any`. `InExpr` overrides `validate()` and compares against the list's ELEMENT type; a genuine mismatch (`COUNT(x) IN ('a','b')`) stays loud and now names the element types. - Lead ruling 2026-09-06: the loud reject of an aggregate in WHERE (SELECT, DELETE, UPDATE, and now CREATE ENRICH POLICY's source WHERE) is CONFIRMED as shipped; the "lead to confirm" markers become "lead-confirmed 2026-09-06" and the behaviour change is a 0.23.0 release note. Tests: sql `*ParserTotalitySpec *HavingAggregateResolutionSpec` 42/42 (two new pins, each asserting the rejection is the grammar's and not the boundary catch); `softclient4es-sql-bridge` and `es6bridge` `*AggregationNamingSpec` 28/28 each; `++ 2.12.20 sql/Test/compile` and `scalafmtAll` clean. Story BIDC-2 Closes #54 Closes #223 Co-Authored-By: Claude Opus 5 (1M context) --- .../softnetwork/elastic/sql/query/Where.scala | 36 +++++++++++++++-- .../elastic/sql/query/package.scala | 4 +- .../query/HavingAggregateResolutionSpec.scala | 39 ++++++++++++++++++- .../client/GatewayApiIntegrationSpec.scala | 4 +- 4 files changed, 76 insertions(+), 7 deletions(-) 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 7cbcb0a17..d53d092c5 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 @@ -16,7 +16,14 @@ package app.softnetwork.elastic.sql.query -import app.softnetwork.elastic.sql.`type`.{SQLAny, SQLTemporal, SQLType, SQLTypeUtils, SQLTypes} +import app.softnetwork.elastic.sql.`type`.{ + SQLAny, + SQLArray, + SQLTemporal, + SQLType, + SQLTypeUtils, + SQLTypes +} import app.softnetwork.elastic.sql.function._ import app.softnetwork.elastic.sql.function.cond.{ConditionalFunction, IsNotNull, IsNull} import app.softnetwork.elastic.sql.function.geo.Distance @@ -818,6 +825,28 @@ case class InExpr[R, +T <: Value[R]]( override def asFilter(currentQuery: Option[ElasticBoolQuery]): ElasticFilter = this + // ` IN (v1, v2)` compares the element with the list's ELEMENT type. The shared + // Expression.validate compared `identifier.out` with the list's ARRAY type, which rejected + // `COUNT(x) IN (1, 2)` (`BIGINT` vs `ARRAY`) while the untyped `MAX(x) IN (…)` passed + // through `Any` -- loud, but misleading. + override def validate(): Either[String, Unit] = + for { + _ <- identifier.validate() + _ <- values.validate() + _ <- { + val elementType = values.out match { + case a: SQLArray => a.elementType + case other => other + } + Validator + .validateTypesMatching(identifier.out, elementType) + .left + .map(_ => + s"Type mismatch: '${identifier.out.typeId}' is not compatible with '${elementType.typeId}' in expression: $this" + ) + } + } yield () + override def painless(context: Option[PainlessContext]): String = { if (context.isEmpty && referencesBucketMetric) return bucketPipelinePainless s"$painlessNot${identifier.painless(context)}$painlessOp(${painlessValue(context)})" @@ -1076,8 +1105,9 @@ case class Where(criteria: Option[Criteria]) extends Updateable { // An aggregate has no document-level query form: the bridge rendered it as `match_all` and // the predicate was silently DROPPED -- a wrong group set for a SELECT, EVERY document for a // DELETE or UPDATE (#280 family). Checked here, not in SingleSearch.validate(), so every - // statement kind that carries a WHERE is covered. Lead to confirm the product choice (reject, - // as here, vs rewrite as HAVING under a GROUP BY). + // statement kind that carries a WHERE is covered: SELECT, DELETE, UPDATE and CREATE ENRICH + // POLICY's source WHERE. Lead-confirmed 2026-09-06 (reject, rather than rewriting it as a + // HAVING under a GROUP BY). c.extractAggregationFields.headOption match { case Some(f) => Left( 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 f329d2d5a..857d701d1 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 @@ -1517,7 +1517,9 @@ package object query { } else if (enrichFields.isEmpty) { Left("Enrich fields cannot be empty") } else { - Right(()) + // The source WHERE is a document-level filter: an aggregate in it is dropped by + // ElasticCriteria and the policy would enrich from EVERY source document (match_all). + where.map(_.validate()).getOrElse(Right(())) } } } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala index afe6023dd..c29949b95 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAggregateResolutionSpec.scala @@ -29,7 +29,7 @@ class HavingAggregateResolutionSpec extends AnyFlatSpec with Matchers { } } - "an aggregate in WHERE" should "be rejected, naming HAVING (lead to confirm the product choice)" in { + "an aggregate in WHERE" should "be rejected, naming HAVING (lead-confirmed 2026-09-06)" in { // It used to parse, create the aggregation and silently DROP the predicate (`match_all`). val msg = rejection("SELECT id FROM t WHERE COUNT(x) > 5 GROUP BY id") msg should include("Aggregate functions are not allowed in WHERE") @@ -72,4 +72,41 @@ class HavingAggregateResolutionSpec extends AnyFlatSpec with Matchers { msg should include("rename one of them") msg should not startWith Parser.InternalParseFailure } + + "an aggregate in the source WHERE of CREATE ENRICH POLICY" should "be rejected (third look)" in { + // CreateEnrichPolicy.validate() never validated its WHERE: the aggregate was dropped and the + // policy enriched from EVERY source document (match_all). + val msg = rejection( + "CREATE ENRICH POLICY p FROM users ON user_id ENRICH name, email WHERE COUNT(orders) > 5" + ) + msg should include("Aggregate functions are not allowed in WHERE") + msg should include("COUNT(orders)") + msg should not startWith Parser.InternalParseFailure + // The document-level WHERE is untouched. + Parser( + "CREATE ENRICH POLICY p FROM users ON user_id ENRICH name, email WHERE status = 'active'" + ).isRight shouldBe true + } + + "an aggregate IN (…) in HAVING" should "type-check against the list's element type (third look)" in { + // `COUNT(x) IN (1, 2)` used to be rejected as `BIGINT` vs `ARRAY` while the untyped + // `MAX(x) IN (…)` passed -- the element is compared with the element type now. + Seq( + "SELECT id, COUNT(x) FROM t GROUP BY id HAVING COUNT(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT IN (1, 2)", + "SELECT id, MAX(x) FROM t GROUP BY id HAVING MAX(x) IN (40, 50)", + "SELECT id, MAX(name) FROM t GROUP BY id HAVING MAX(name) IN ('a', 'b')", + // a typed document-level element gets the same comparison + "SELECT id FROM t WHERE YEAR(createdAt) IN (2020, 2021)" + ).foreach { sql => + withClue(s"[$sql] ") { + Parser(sql).isRight shouldBe true + } + } + // A genuine mismatch stays loud, and names the ELEMENT types. + val msg = rejection("SELECT id FROM t GROUP BY id HAVING COUNT(x) IN ('a', 'b')") + msg should include("Type mismatch") + msg should include("'BIGINT' is not compatible with 'VARCHAR'") + msg should not startWith Parser.InternalParseFailure + } } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index 2c299ef26..dd786845a 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -1260,7 +1260,7 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { ) } - it should "reject an aggregate in WHERE instead of silently dropping it (lead to confirm)" in { + it should "reject an aggregate in WHERE instead of silently dropping it (lead-confirmed 2026-09-06)" in { val sql = """SELECT city FROM having_naming |WHERE COUNT(name) > 1 @@ -1273,7 +1273,7 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { ) } - it should "reject an aggregate in a DELETE or UPDATE WHERE and touch nothing (S2-2, lead to confirm)" in { + it should "reject an aggregate in a DELETE or UPDATE WHERE and touch nothing (S2-2, lead-confirmed 2026-09-06)" in { // Before: the predicate became `match_all` -- the DELETE wiped the index, the UPDATE hit every // document. The count and the ages must be exactly what they were. val snapshot = From 10f3f8fcaa3c78547c539cbbf18c75ce91feffbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 11:54:27 +0200 Subject: [PATCH 6/6] test(bridge,testkit): pin the emitted script for HAVING COUNT(x) IN, newly accepted by T3-3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T3-3 made `COUNT(x) IN (…)` pass validation, so a shape that used to be rejected now reaches the bucket-pipeline emission -- and every existing IN pin was a `MAX(x)` one, with `COUNT(x) IN` covered only at parse level, which cannot see a wrong script. - `AggregationNamingSpec` (template and the hand-maintained es6 copy, kept byte-identical): two emitted-JSON pins for `HAVING COUNT(x) IN (1, 2)` and its NOT twin -- the guarded `==` chain over `params.count_x` and the declared `buckets_path` key `count_x -> count_x`, the same assertions the MAX pins make. Both shapes added to the structural guards' list (35 -> 37 shapes). - `GatewayApiIntegrationSpec`: one live case beside `MAX(age) IN (40, 50)` -- `HAVING COUNT(name) IN (2, 3)` over the `having_naming` table (Paris 2, Lyon 1, Marseille 1, Nice 1) expects exactly Paris, so an empty result (a `value_count` long compared with Integer literals -- the S2-1 boxing class of defect) and an all-pass guard both fail loudly. Tests: sql `*HavingAggregateResolutionSpec` 7/7, `softclient4es-sql-bridge` and `es6bridge` `*AggregationNamingSpec` 30/30 each, `softclient4es-core-testkit` compile clean, `scalafmtAll` clean. Story BIDC-2 Closes #54 Closes #223 Co-Authored-By: Claude Opus 5 (1M context) --- .../elastic/sql/AggregationNamingSpec.scala | 27 +++++++++++++++++++ .../elastic/sql/AggregationNamingSpec.scala | 27 +++++++++++++++++++ .../client/GatewayApiIntegrationSpec.scala | 14 ++++++++++ 3 files changed, 68 insertions(+) diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index 47390e32f..543fec334 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -268,6 +268,31 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ).mkString } + // `COUNT(x) IN (…)` was REJECTED by the shared Expression.validate (BIGINT vs ARRAY) until + // the third look, so it never reached the emission: the MAX pins above could not cover it. A + // `value_count` metric arrives on the buckets_path as a number too, so the guarded `==` chain is + // the same shape -- the S2-1 defect (Integer literals vs a boxed Double) was exactly a typing + // mismatch that a parse-level pin cannot see. + it should "emit the same guarded chain for a COUNT metric, newly accepted (T3-3)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x == 1 || params.count_x == 2))"}}}}}}}""" + ).mkString + } + + it should "keep the NOT of a COUNT metric (T3-3)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (!(params.count_x == 1 || params.count_x == 2)))"}}}}}}}""" + ).mkString + } + "A AND NOT B" should "negate the RIGHT operand, inside its guard (R2-2)" in { // The selector used to prefix `!` to the LEFT operand -- the exact complement of what was asked. // The NOT is pushed into the right-hand comparison (`> 3` becomes `<= 3`) so a bucket whose @@ -392,6 +417,8 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT BETWEEN 1 AND 5", "SELECT id FROM t GROUP BY id HAVING MAX(x) IN (1, 2)", "SELECT id FROM t GROUP BY id HAVING MAX(x) NOT IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT IN (1, 2)", "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) > 3", "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) IN (1, 2)", "SELECT id FROM t GROUP BY id HAVING NOT MAX(x) > 3", diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index 47390e32f..543fec334 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -268,6 +268,31 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ).mkString } + // `COUNT(x) IN (…)` was REJECTED by the shared Expression.validate (BIGINT vs ARRAY) until + // the third look, so it never reached the emission: the MAX pins above could not cover it. A + // `value_count` metric arrives on the buckets_path as a number too, so the guarded `==` chain is + // the same shape -- the S2-1 defect (Integer literals vs a boxed Double) was exactly a typing + // mismatch that a parse-level pin cannot see. + it should "emit the same guarded chain for a COUNT metric, newly accepted (T3-3)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (params.count_x == 1 || params.count_x == 2))"}}}}}}}""" + ).mkString + } + + it should "keep the NOT of a COUNT metric (T3-3)" in { + queryOf("SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT IN (1, 2)") shouldBe Seq( + """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"id":{""", + terms, + ""","aggs":{"count_x":{"value_count":{"field":"x"}},""", + """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x"},""", + """"script":{"source":"(params.count_x == null ? false : (!(params.count_x == 1 || params.count_x == 2)))"}}}}}}}""" + ).mkString + } + "A AND NOT B" should "negate the RIGHT operand, inside its guard (R2-2)" in { // The selector used to prefix `!` to the LEFT operand -- the exact complement of what was asked. // The NOT is pushed into the right-hand comparison (`> 3` becomes `<= 3`) so a bucket whose @@ -392,6 +417,8 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT BETWEEN 1 AND 5", "SELECT id FROM t GROUP BY id HAVING MAX(x) IN (1, 2)", "SELECT id FROM t GROUP BY id HAVING MAX(x) NOT IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) IN (1, 2)", + "SELECT id FROM t GROUP BY id HAVING COUNT(x) NOT IN (1, 2)", "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) > 3", "SELECT id FROM t GROUP BY id HAVING COUNT(x) > 5 AND NOT MAX(x) IN (1, 2)", "SELECT id FROM t GROUP BY id HAVING NOT MAX(x) > 3", diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala index dd786845a..ba89faf75 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GatewayApiIntegrationSpec.scala @@ -1248,6 +1248,20 @@ trait GatewayApiIntegrationSpec extends GatewayIntegrationTestKit { Seq(Map("city" -> "Lyon"), Map("city" -> "Marseille")) ) + // The same over a COUNT metric, which the type validator rejected until the third look (T3-3) + // so it had never reached a cluster: `value_count` comes back as a long on the buckets_path + // while the literals are Integers -- the S2-1 boxing class of defect. COUNT(name): Paris 2, + // Lyon 1, Marseille 1, Nice 1, so exactly one bucket may pass (an all-pass or an empty result + // both fail this assertion). + val countIn = + """SELECT city FROM having_naming + |GROUP BY city HAVING COUNT(name) IN (2, 3);""".stripMargin + assertSelectResult( + System.nanoTime(), + client.run(countIn).futureValue, + Seq(Map("city" -> "Paris")) + ) + // `A AND NOT B`: the NOT belongs to the RIGHT operand (it used to negate the left one), and a // bucket whose metric is missing (Nice) must still fail `NOT MAX(age) > 45`, as in SQL. val andNot =