From c2b111823592857e302446cd7e010d72d9639328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Wed, 30 Sep 2026 16:44:33 +0200 Subject: [PATCH] fix(sql): AND binds tighter than OR in every clause (ANDOR) Story ANDOR: a condition that mixes AND and OR is evaluated with SQL precedence. - The condition reducer applies SQL precedence (AND before OR); a written group, NESTED/CHILD/PARENT bodies included, stays one operand. - WHERE (and DELETE / UPDATE, which share it): a child bool shares its parent only on the same operator, so an OR branch is no longer made optional beside a filter. - HAVING: the bucket_selector script parenthesises an OR under an AND; the selector no longer strips "1 == 1" from the whole script (it ate params.max_c1 == 1); a one-key HAVING mix is accepted exactly when the include/exclude lists give SQL's answer. - NOT at the head of an AND run folds into its condition (ISNULL <-> ISNOTNULL included); on MATCH it is refused by name with a rewrite. A MATCH-bearing OR group under an AND stays a required clause. - The rendered SQL keeps the parentheses precedence needs. - Docs: precedence sections, NOT examples and 31 examples corrected. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../sql/bridge/ElasticAggregation.scala | 10 +- .../elastic/sql/BoolQueryModel.scala | 101 +++++ .../sql/ConditionBoolEmissionSpec.scala | 128 ++++++ .../sql/HavingFunctionEmissionSpec.scala | 20 + .../elastic/sql/SQLQuerySpec.scala | 4 +- documentation/sql/known_limitations.md | 21 + documentation/sql/operator_precedence.md | 86 ++-- documentation/sql/operators.md | 212 +++++----- .../sql/bridge/ElasticAggregation.scala | 10 +- .../elastic/sql/BoolQueryModel.scala | 101 +++++ .../sql/ConditionBoolEmissionSpec.scala | 128 ++++++ .../sql/HavingFunctionEmissionSpec.scala | 20 + .../elastic/sql/SQLQuerySpec.scala | 4 +- .../elastic/sql/parser/WhereParser.scala | 253 ++++++------ .../elastic/sql/query/GroupBy.scala | 26 +- .../elastic/sql/query/Having.scala | 10 +- .../softnetwork/elastic/sql/query/Where.scala | 108 ++++- .../elastic/sql/query/package.scala | 73 ++++ .../sql/parser/ConditionPopulation.scala | 374 ++++++++++++++++++ .../sql/parser/ConditionPrecedenceSpec.scala | 244 ++++++++++++ .../sql/parser/ParserTotalitySpec.scala | 20 +- .../HavingOverAggregateFunctionSpec.scala | 148 +++++++ .../query/MetricSelectorPrecedenceSpec.scala | 106 +++++ .../elastic/client/ConditionTruthTable.scala | 169 ++++++++ .../client/PredicateFunctionResultSpec.scala | 227 +++++++++++ 25 files changed, 2258 insertions(+), 345 deletions(-) create mode 100644 bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala create mode 100644 bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala create mode 100644 es6/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala create mode 100644 es6/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPopulation.scala create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPrecedenceSpec.scala create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala create mode 100644 testkit/src/main/scala/app/softnetwork/elastic/client/ConditionTruthTable.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 642db6e2..29baa821 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 @@ -757,13 +757,9 @@ object ElasticAggregation { val currentNestedPath = nested.map(_.nestedPath).getOrElse("") - // No filtering - val fullScript = MetricSelectorScript - .metricSelector(criteria) - .replaceAll("1 == 1 &&", "") - .replaceAll("&& 1 == 1", "") - .replaceAll("1 == 1", "") - .trim + // No filtering at this level is `None`, never a placeholder to strip out of a script: a + // rendered comparison can hold the placeholder's text (`params.max_c1 == 1`). + val fullScript = MetricSelectorScript.selectorScript(criteria).map(_.trim).getOrElse("") // println(s"[DEBUG] currentNestedPath = $currentNestedPath") // println(s"[DEBUG] fullScript (complete) = $fullScript") diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala new file mode 100644 index 00000000..28a127fb --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala @@ -0,0 +1,101 @@ +package app.softnetwork.elastic.sql + +import com.fasterxml.jackson.databind.JsonNode + +import scala.jdk.CollectionConverters._ + +/** How Elasticsearch evaluates the `bool` queries a WHERE emits, over documents whose fields `c1 … + * cN` are 0 or 1 (`bits`: field `ci` is bit `i - 1`). + * + * The one rule that matters here, from the Elasticsearch reference: a `bool` with `should` and no + * explicit `minimum_should_match` requires ONE `should` clause only when it holds no `filter` and + * no `must` clause -- next to either, the `should` clauses are optional. Elasticsearch 6 adds one + * exception: a `bool` evaluated in a FILTER context requires one `should` clause anyway. `es6 = + * true` applies that exception. `nested` is read with a ONE-child document (its query is evaluated + * on the same bits); the evaluation context carries through it. + */ +object BoolQueryModel { + + final case class Unmodelled(msg: String) extends Exception(msg) + + def matches(q: JsonNode, bits: Int, es6: Boolean, filterContext: Boolean = false): Boolean = { + val kinds = q.fieldNames().asScala.toList + if (kinds.size != 1) throw Unmodelled(s"query node with keys $kinds") + val body = q.get(kinds.head) + kinds.head match { + case "bool" => + val known = Set( + "filter", + "must", + "must_not", + "should", + "minimum_should_match", + "boost", + "adjust_pure_negative" + ) + body + .fieldNames() + .asScala + .find(k => !known.contains(k)) + .foreach(k => throw Unmodelled(s"bool key $k")) + def clauses(name: String): List[JsonNode] = Option(body.get(name)) match { + case Some(n) if n.isArray => n.elements().asScala.toList + case Some(n) => List(n) + case None => Nil + } + val (filters, musts, nots, shoulds) = + (clauses("filter"), clauses("must"), clauses("must_not"), clauses("should")) + val required = Option(body.get("minimum_should_match")).map(_.asText.toInt).getOrElse { + if (shoulds.isEmpty) 0 + else if (es6 && filterContext) 1 + else if (filters.isEmpty && musts.isEmpty) 1 + else 0 + } + filters.forall(matches(_, bits, es6, filterContext = true)) && + musts.forall(matches(_, bits, es6, filterContext)) && + nots.forall(n => !matches(n, bits, es6, filterContext = true)) && + shoulds.count(matches(_, bits, es6, filterContext)) >= required + case "nested" => matches(body.get("query"), bits, es6, filterContext) + case "term" => + val field = body.fieldNames().asScala.toList.head + val v = Option(body.get(field)).map(n => if (n.isObject) n.get("value") else n).get + val i = """c(\d+)""".r + .findFirstMatchIn(field) + .map(_.group(1).toInt) + .getOrElse(throw Unmodelled(s"term on $field")) + val bit = ((bits >> (i - 1)) & 1) == 1 + (if (bit) 1.0 else 0.0) == v.asDouble + case "match" => + // `MATCH (ci) AGAINST ('1')`: read like `ci = 1` -- one term, the field's value + val field = body.fieldNames().asScala.toList.head + val v = Option(body.get(field)).map(n => if (n.isObject) n.get("query") else n).get + val i = """c(\d+)""".r + .findFirstMatchIn(field) + .map(_.group(1).toInt) + .getOrElse(throw Unmodelled(s"match on $field")) + val bit = ((bits >> (i - 1)) & 1) == 1 + bit == (v.asText == "1") + case "match_all" => true + case other => throw Unmodelled(s"query kind $other") + } + } + + /** Every `bool` that holds `should` clauses next to `filter` or `must` clauses without an + * explicit `minimum_should_match` -- where its `should` clauses are optional. + */ + def optionalShoulds(q: JsonNode): Int = { + var count = 0 + def walk(n: JsonNode): Unit = + if (n.isObject) { + Option(n.get("bool")).filter(_.isObject).foreach { b => + val should = Option(b.get("should")).exists(_.size > 0) + val required = + Option(b.get("filter")).exists(_.size > 0) || Option(b.get("must")).exists(_.size > 0) + if (should && required && b.get("minimum_should_match") == null) count += 1 + } + n.elements().asScala.foreach(walk) + } else if (n.isArray) n.elements().asScala.foreach(walk) + walk(q) + count + } +} diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala new file mode 100644 index 00000000..4916a1ac --- /dev/null +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala @@ -0,0 +1,128 @@ +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +import app.softnetwork.elastic.sql.parser.ConditionPopulation._ +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query.SingleSearch +import com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** The Elasticsearch query a WHERE emits evaluates SQL's reading of the condition. + * + * 🔴 A correct condition TREE is not enough: an unparenthesised sub-condition used to be written + * into its parent's `bool`, where an OR under an AND put its `should` clauses next to `filter` + * clauses -- optional for Elasticsearch. The query is evaluated here with Elasticsearch's own + * `bool` rules (`BoolQueryModel`), for Elasticsearch 7+ and for the Elasticsearch 6 filter-context + * rule, against the oracle of the generated structure. + */ +class ConditionBoolEmissionSpec extends AnyFlatSpec with Matchers { + + implicit val timestamp: Long = 0L + + private val mapper = new ObjectMapper() + + /** P2: the WHERE statements of P1 whose leaves are all `ci = 1` (every class but the kinds). */ + private lazy val wherePopulation: List[Gen] = + population().filter(g => g.clause == WhereC && g.cls != "K") + + private def queryOf(search: SingleSearch): JsonNode = { + val request: ElasticSearchRequest = search + mapper.readTree(request.query).get("query") + } + + private def searchOf(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] $other") + } + + "a WHERE that combines AND and OR" should "emit a query that evaluates SQL's reading" in { + val wrong = wherePopulation.flatMap { g => + val q = queryOf(searchOf(g.sql)) + val bits = 0 until (1 << g.n) + val es7 = bits.map(BoolQueryModel.matches(q, _, es6 = false)).toVector + val es6 = bits.map(BoolQueryModel.matches(q, _, es6 = true)).toVector + if (es7 == g.ansiTable && es6 == g.ansiTable) None + else Some(s"[${g.sql}] es7=${es7 == g.ansiTable} es6=${es6 == g.ansiTable}: $q") + } + withClue( + s"${wrong.size} of ${wherePopulation.size} wrong, first 10:\n${wrong.take(10).mkString("\n")}\n" + ) { + wrong shouldBe empty + } + } + + it should "never leave should clauses beside filter or must clauses" in { + val offending = wherePopulation + .map(g => g.sql -> BoolQueryModel.optionalShoulds(queryOf(searchOf(g.sql)))) + .filter(_._2 > 0) + withClue(s"first 10: ${offending.take(10).mkString("\n")}\n") { offending shouldBe empty } + } + + "a WHERE with ONE operator" should "stay one flat bool" in { + queryOf( + searchOf("SELECT id FROM t WHERE c1 = 1 AND c2 = 1 AND c3 = 1 AND c4 = 1") + ).toString shouldBe + """{"bool":{"filter":[{"term":{"c1":{"value":1}}},{"term":{"c2":{"value":1}}},{"term":{"c3":{"value":1}}},{"term":{"c4":{"value":1}}}]}}""" + queryOf(searchOf("SELECT id FROM t WHERE c1 = 1 OR c2 = 1 OR c3 = 1")).toString shouldBe + """{"bool":{"should":[{"term":{"c1":{"value":1}}},{"term":{"c2":{"value":1}}},{"term":{"c3":{"value":1}}}]}}""" + } + + "DELETE and UPDATE" should "send the query the same WHERE sends in a SELECT" in { + val wrong = wherePopulation.filter(_.mixedLevel).flatMap { g => + val select = searchOf(g.sql) + val expected = queryOf(select) + Seq( + "DELETE" -> select.copy(deleteByQuery = true), + "UPDATE" -> select.copy(updateByQuery = true) + ) + .collect { case (kind, s) if queryOf(s) != expected => s"$kind [${g.sql}]: ${queryOf(s)}" } + } + wrong shouldBe empty + } + + "an OR group holding a MATCH, under an AND" should "stay ONE condition of the AND" in { + // Spread into the root bool beside its `filter` clauses, the group's `should` clauses turned + // optional: `c1 = 1 AND (MATCH ... OR c3 = 1)` selected every document with c1 = 1. A MATCH + // over several columns is such a group too. The group that IS the whole condition is still + // spread, which is exact (the flat `should` of a lone MATCH). + Seq( + "c1 = 1 AND (MATCH (c2) AGAINST ('1') OR c3 = 1)" -> (3, A(L(1), O(L(2), L(3)))), + "(MATCH (c1) AGAINST ('1') OR c2 = 1) AND c3 = 1" -> (3, A(O(L(1), L(2)), L(3))), + "c1 = 1 AND MATCH (c2, c3) AGAINST ('1')" -> (3, A(L(1), O(L(2), L(3)))), + "(c1 = 1 OR MATCH (c2) AGAINST ('1')) AND (c3 = 1 OR MATCH (c4) AGAINST ('1'))" -> + (4, A(O(L(1), L(2)), O(L(3), L(4)))), + "MATCH (c1, c2) AGAINST ('1')" -> (2, O(L(1), L(2))), + "MATCH (c1) AGAINST ('1') OR c2 = 1 AND c3 = 1" -> (3, O(L(1), A(L(2), L(3)))) + ).foreach { case (cond, (n, expected)) => + val q = queryOf(searchOf(s"SELECT id FROM t WHERE $cond")) + val bits = 0 until (1 << n) + withClue(s"[$cond] $q ") { + bits.map(BoolQueryModel.matches(q, _, es6 = false)).toVector shouldBe table(expected, n) + bits.map(BoolQueryModel.matches(q, _, es6 = true)).toVector shouldBe table(expected, n) + BoolQueryModel.optionalShoulds(q) shouldBe 0 + } + } + } + + "a WHERE over an UNNEST column" should "emit the nested query SQL's reading needs" in { + // one child per document: a nested query is its own query + Seq( + "inner_items.c1 = 1 OR inner_items.c2 = 1 AND inner_items.c3 = 1" -> O(L(1), A(L(2), L(3))), + "c1 = 1 OR inner_items.c2 = 1 AND inner_items.c3 = 1" -> O(L(1), A(L(2), L(3))), + "inner_items.c1 = 1 AND inner_items.c2 = 1 OR c3 = 1" -> O(A(L(1), L(2)), L(3)) + ).foreach { case (cond, expected) => + val q = queryOf(searchOf(s"SELECT id FROM t JOIN UNNEST(t.items) AS inner_items WHERE $cond")) + withClue(s"[$cond] $q ") { + (0 until 8).map(BoolQueryModel.matches(q, _, es6 = false)).toVector shouldBe table( + expected, + 3 + ) + (0 until 8).map(BoolQueryModel.matches(q, _, es6 = true)).toVector shouldBe table( + expected, + 3 + ) + } + } + } +} diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 6fa427d3..9d46e30c 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -351,6 +351,26 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { ) } + "a comparison of an aggregate with 1 or 10" should "read that aggregate's own parameter" in { + // 🔴 The emission stripped every `1 == 1` out of the script -- the text the selector answers + // when there is nothing to filter -- and `params.max_c1 == 1` holds that text. MEASURED on the + // base: `= 1` read `params.max_c`, `= 10` read `params.max_c0`, and `IN (1, 2)` lost its first + // member; on Elasticsearch 8.18.3 all three searches failed (`Cannot invoke + // "Object.getClass()" because "value" is null`). + Seq( + "MAX(c1) = 1" -> "(params.max_c1 == null ? false : (params.max_c1 == 1))", + "MAX(c1) = 10" -> "(params.max_c1 == null ? false : (params.max_c1 == 10))", + "MAX(c1) IN (1, 2)" -> "(params.max_c1 == null ? false : (params.max_c1 == 1 || params.max_c1 == 2))" + ).foreach { case (condition, script) => + withClue(s"[$condition] ") { + queryOf(s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING $condition") should include( + """"having_filter":{"bucket_selector":{"buckets_path":{"max_c1":"max_c1"},""" + + s""""script":{"source":"$script"}}}""" + ) + } + } + } + "a wrapped aggregate beside a bucket-key predicate" should "honour BOTH mechanisms" in { // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may // cost the other. 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 4b99fe4c..88786ec5 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -5002,8 +5002,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { } it should "emit no bucket_selector for a HAVING with no aggregate over an aggregate-free GROUP BY" in { - // `metricSelectorForBucket` strips "1 == 1" to the empty string, so the HAVING becomes a terms - // exclude and no bucket_selector is produced. + // `metricSelectorForBucket` finds nothing to filter at this level (`selectorScript` is `None`), + // so the HAVING becomes a terms exclude and no bucket_selector is produced. val select: ElasticSearchRequest = SelectStatement("SELECT category FROM Table GROUP BY category HAVING category <> 'x'") val query = select.query diff --git a/documentation/sql/known_limitations.md b/documentation/sql/known_limitations.md index fe712fc0..38a1b1bc 100644 --- a/documentation/sql/known_limitations.md +++ b/documentation/sql/known_limitations.md @@ -295,6 +295,27 @@ within the elements an UNNEST projection returns per parent. Two inner columns w coincide (`o.id` and `items.id`) are refused: alias one of them (measured on Elasticsearch 8.18 and 6.8). See [JOIN UNNEST](dql_statements.md#join-unnest). +## `NOT` applies to one condition + +`NOT` negates the one condition written after it, and only some spellings of it are accepted: + +| Written | Verdict | +|---------|---------| +| `NOT price > 100`, `NOT status = 'x'` (before a comparison) | Accepted | +| `status NOT IN (...)`, `name NOT LIKE 'x%'`, `price NOT BETWEEN 1 AND 2`, `manager IS NOT NULL` | Accepted | +| `NOT (a = 1 OR b = 2)`, `NOT (price > 100)` (before a parenthesised group) | Parse error | +| `NOT is_active` (before a bare boolean column) | Parse error | +| `NOT status IN (...)`, `NOT name LIKE 'x%'`, `NOT manager IS NULL`, `NOT MATCH (...) AGAINST (...)`, `NOT ISNULL(x)` at the start of a condition | Parse error | +| `a = 1 AND NOT name LIKE 'x%'` (the same spellings right after `AND` / `OR`) | Accepted in some positions only: `a = 1 AND b = 2 AND NOT name LIKE 'x%'` is a parse error | +| `a = 1 OR NOT ISNULL(x) AND b = 2` (also with `ISNOTNULL(x)`) | Accepted, read as `a = 1 OR (ISNOTNULL(x) AND b = 2)` | +| `a = 1 OR NOT MATCH (t) AGAINST ('x') AND b = 2` | Refused: *"NOT ... cannot start conditions joined by AND after an OR"* | + +Write the condition De Morgan's laws give instead of a `NOT` over a group (`NOT (a = 1 OR b = 2)` is +`NOT a = 1 AND NOT b = 2`), compare a boolean column (`is_active = false`), put the `NOT` of `IN`, +`LIKE`, `RLIKE`, `BETWEEN` and `IS NULL` after the column, write `ISNOTNULL(x)` for `NOT ISNULL(x)` +(and `ISNULL(x)` for `NOT ISNOTNULL(x)`), and move a negated `MATCH` to the end of its `AND` group +(`a = 1 OR (b = 2 AND NOT MATCH (t) AGAINST ('x'))`). + ## Coming in the upcoming release (Quarter 1 2027) - **Heterogeneous federation**: JOIN or correlate Elasticsearch with PostgreSQL, MySQL, ClickHouse, Snowflake, and more — plus cross-cluster subqueries (e.g. correlate one cluster's data against another's). diff --git a/documentation/sql/operator_precedence.md b/documentation/sql/operator_precedence.md index 6792f9da..74b30f9a 100644 --- a/documentation/sql/operator_precedence.md +++ b/documentation/sql/operator_precedence.md @@ -20,6 +20,8 @@ This page lists operator precedence used by the parser and evaluator. Operators | **8** | `AND` | Logical AND | Logical conjunction | | **9** (Lowest) | `OR` | Logical OR | Logical disjunction | +> Since 0.24.0 a condition that combines `AND` and `OR` without parentheses is evaluated with this precedence in every clause (`WHERE`, `HAVING`, `CASE WHEN`, `DELETE`, `UPDATE`); earlier versions could evaluate it in another order and return other rows without an error. + --- ### 1. Parentheses `(...)` @@ -88,16 +90,16 @@ SELECT 10 + -5 AS result; SELECT 10 + (-5) AS result; -- Result: 5 --- Double negation -SELECT -(-10) AS result; +-- Double negation: subtract the negative value (a unary minus before a parenthesis does not +-- parse) +SELECT 0 - (-10) AS result; -- Result: 10 ``` **Unary Plus:** ```sql --- Explicit positive (rarely used) +-- ✗ parse error: a unary plus is not supported -- write the value itself SELECT +5 AS pos_value; --- Result: 5 SELECT +price AS positive_price FROM products; @@ -105,27 +107,23 @@ FROM products; **Logical NOT:** ```sql --- Negate boolean expression -SELECT * FROM users -WHERE NOT is_active; --- Same as: WHERE is_active = false - --- NOT with comparison +-- NOT negates the condition right after it SELECT * FROM products -WHERE NOT (price > 100); +WHERE NOT price > 100; -- Same as: WHERE price <= 100 --- NOT with IN -SELECT * FROM orders -WHERE NOT status IN ('cancelled', 'refunded'); --- Same as: WHERE status NOT IN ('cancelled', 'refunded') - --- Multiple NOT +-- NOT binds tighter than AND and OR SELECT * FROM users -WHERE NOT (NOT is_verified); --- Same as: WHERE is_verified +WHERE NOT is_active = true AND is_verified = true; +-- Evaluated as: (NOT is_active = true) AND (is_verified = true) + +-- IN, LIKE, RLIKE, BETWEEN and IS NULL take their NOT after the column +SELECT * FROM orders +WHERE status NOT IN ('cancelled', 'refunded'); ``` +`NOT` applies to one condition. It is not accepted before a parenthesised group (`NOT (a = 1 OR b = 2)`) or a bare boolean column (`NOT is_active`): write the condition De Morgan's laws give (`NOT a = 1 AND NOT b = 2`), or compare the column (`is_active = false`). See [Known Limitations](known_limitations.md#not-applies-to-one-condition). + --- ### 3. Multiplicative: `*`, `/`, `%` @@ -270,12 +268,12 @@ SELECT (10 + 5) * (2 - 3) AS result; **Less Than / Greater Than:** ```sql --- Basic comparisons -SELECT 5 < 10 AS result; --- Result: true +-- Basic comparisons (a comparison is a condition: in the SELECT list, write it in a CASE) +SELECT CASE WHEN 5 < 10 THEN 1 ELSE 0 END AS result; +-- Result: 1 -SELECT 5 > 10 AS result; --- Result: false +SELECT CASE WHEN 5 > 10 THEN 1 ELSE 0 END AS result; +-- Result: 0 -- In WHERE clause SELECT * FROM products @@ -321,9 +319,9 @@ WHERE price BETWEEN 50 AND 100; **Equality:** ```sql --- Basic equality -SELECT 5 = 5 AS result; --- Result: true +-- Basic equality (a comparison is a condition: in the SELECT list, write it in a CASE) +SELECT CASE WHEN 5 = 5 THEN 1 ELSE 0 END AS result; +-- Result: 1 -- In WHERE clause SELECT * FROM users @@ -338,11 +336,11 @@ WHERE email = NULL; -- Always false! **Inequality:** ```sql -- Not equal (two forms) -SELECT 5 != 3 AS result; --- Result: true +SELECT CASE WHEN 5 != 3 THEN 1 ELSE 0 END AS result; +-- Result: 1 -SELECT 5 <> 3 AS result; --- Result: true +SELECT CASE WHEN 5 <> 3 THEN 1 ELSE 0 END AS result; +-- Result: 1 -- In WHERE clause SELECT * FROM orders @@ -354,16 +352,14 @@ WHERE category <> 'discontinued'; **With Comparisons:** ```sql --- Comparison before equality +-- ✗ parse error: a comparison is not compared with a boolean, with or without parentheses SELECT * FROM products WHERE price > 50 = true; --- Evaluated as: (price > 50) = true --- More readable: SELECT * FROM products WHERE (price > 50) = true; --- Or simply: +-- Write the comparison itself: SELECT * FROM products WHERE price > 50; ``` @@ -596,13 +592,13 @@ WHERE category = 'electronics' AND (price < 100 OR on_sale = true); ```sql -- NOT with high precedence SELECT * FROM users -WHERE NOT is_active AND is_verified; --- Evaluated as: (NOT is_active) AND (is_verified) +WHERE NOT is_active = true AND is_verified = true; +-- Evaluated as: (NOT is_active = true) AND (is_verified = true) --- Use parentheses for different logic +-- NOT over both conditions: NOT (A AND B) is written (NOT A) OR (NOT B) SELECT * FROM users -WHERE NOT (is_active AND is_verified); --- Evaluated as: NOT ((is_active) AND (is_verified)) +WHERE NOT is_active = true OR NOT is_verified = true; +-- Evaluated as: (NOT is_active = true) OR (NOT is_verified = true) ``` **Example 4: Complex Business Logic** @@ -755,12 +751,14 @@ SELECT (price + tax) * quantity FROM orders; ```sql -- Wrong interpretation SELECT * FROM users -WHERE NOT is_active AND is_verified; --- Evaluated as: (NOT is_active) AND (is_verified) +WHERE NOT is_active = true AND is_verified = true; +-- Might think: NOT (is_active = true AND is_verified = true) +-- Actually means: (NOT is_active = true) AND (is_verified = true) --- If you want: NOT (is_active AND is_verified) +-- If you want NOT (is_active = true AND is_verified = true), apply De Morgan's law +-- (NOT before a parenthesised group is not supported) SELECT * FROM users -WHERE NOT (is_active AND is_verified); +WHERE NOT is_active = true OR NOT is_verified = true; ``` **Mistake 4: Multiple comparisons** diff --git a/documentation/sql/operators.md b/documentation/sql/operators.md index 0a0fda31..f286ae89 100644 --- a/documentation/sql/operators.md +++ b/documentation/sql/operators.md @@ -55,15 +55,15 @@ FROM orders; **With Different Types:** ```sql -- Integer addition -SELECT 10 + 20 AS sum; +SELECT 10 + 20 AS total; -- Result: 30 -- Float addition -SELECT 10.5 + 20.3 AS sum; +SELECT 10.5 + 20.3 AS total; -- Result: 30.8 -- Mixed types (INT + DOUBLE) -SELECT 10 + 20.5 AS sum; +SELECT 10 + 20.5 AS total; -- Result: 30.5 (promoted to DOUBLE) ``` @@ -169,8 +169,8 @@ SELECT 5 * 3 AS result; -- Calculate revenue SELECT quantity * price AS revenue FROM sales; --- Multiple multiplications -SELECT length * width * height AS volume +-- Multiple multiplications (`length` is also a function name: quote it) +SELECT `length` * width * height AS volume FROM boxes; ``` @@ -434,10 +434,10 @@ SELECT day_number % 7 AS day_of_week FROM calendar; --- Alternate row colors (even/odd) +-- Alternate row colors (even/odd; `row_number` is also a function name: quote it) SELECT - row_number, - CASE WHEN row_number % 2 = 0 THEN 'even-row' ELSE 'odd-row' END AS css_class + `row_number`, + CASE WHEN `row_number` % 2 = 0 THEN 'even-row' ELSE 'odd-row' END AS css_class FROM data_table; ``` @@ -475,9 +475,9 @@ expr1 = expr2 **Basic Equality:** ```sql --- Compare values -SELECT 5 = 5 AS result; --- Result: true +-- Compare values (a comparison is a condition: in the SELECT list, write it in a CASE) +SELECT CASE WHEN 5 = 5 THEN 1 ELSE 0 END AS result; +-- Result: 1 -- Filter by department SELECT * FROM emp WHERE department = 'IT'; @@ -548,12 +548,12 @@ expr1 != expr2 **Basic Inequality:** ```sql --- Not equal -SELECT 5 <> 3 AS result; --- Result: true +-- Not equal (a comparison is a condition: in the SELECT list, write it in a CASE) +SELECT CASE WHEN 5 <> 3 THEN 1 ELSE 0 END AS result; +-- Result: 1 -SELECT 5 != 3 AS result; --- Result: true +SELECT CASE WHEN 5 != 3 THEN 1 ELSE 0 END AS result; +-- Result: 1 -- Filter by status SELECT * FROM emp WHERE status <> 'terminated'; @@ -730,16 +730,17 @@ WHERE category_id IN ( **Empty List:** ```sql --- Empty IN list returns false +-- ✗ parse error: an IN list holds at least one value SELECT * FROM products WHERE id IN (); --- Returns no rows ``` **NULL Handling:** ```sql --- NULL in list +-- ✗ parse error: NULL is not accepted in an IN list SELECT * FROM users WHERE status IN ('active', NULL); --- NULL is ignored in the list + +-- Keep the rows whose status is NULL as well +SELECT * FROM users WHERE status = 'active' OR status IS NULL; -- Column with NULL SELECT * FROM users WHERE email IN ('test@example.com'); @@ -798,9 +799,8 @@ WHERE category_id NOT IN ( **NULL Handling (Important!):** ```sql --- NOT IN with NULL in list returns NULL (not true!) +-- ✗ parse error: NULL is not accepted in a NOT IN list SELECT * FROM users WHERE id NOT IN (1, 2, NULL); --- Returns no rows because comparison with NULL is NULL -- Safe alternative: filter NULLs in subquery SELECT * FROM customers @@ -887,9 +887,9 @@ WHERE ST_DISTANCE(POINT(-70.0, 40.0), toLocation) BETWEEN 4000 AND 5000; SELECT id FROM locations WHERE ST_DISTANCE(POINT(-70.0, 40.0), toLocation) BETWEEN 4000 km AND 5000 km; --- Distance with miles +-- Distance with miles (a distance with a unit is a whole number: `2.5 mi` does not parse) SELECT id FROM locations -WHERE ST_DISTANCE(POINT(-70.0, 40.0), toLocation) BETWEEN 2.5 mi AND 3.1 mi; +WHERE ST_DISTANCE(POINT(-70.0, 40.0), toLocation) BETWEEN 2 mi AND 4 mi; ``` **Elasticsearch Optimization:** @@ -1135,8 +1135,8 @@ SELECT * FROM phone_numbers WHERE number LIKE '555-____'; SELECT * FROM users WHERE name LIKE 'john%'; -- May or may not match 'JOHN', 'John', 'john' --- Force case-insensitive with LOWER -SELECT * FROM users WHERE LOWER(name) LIKE LOWER('john%'); +-- Force case-insensitive with LOWER (the pattern is a literal: write it in lower case) +SELECT * FROM users WHERE LOWER(name) LIKE 'john%'; -- Matches all case variations ``` @@ -1153,12 +1153,14 @@ WHERE title NOT LIKE '%draft%' **Escaping Special Characters:** ```sql --- Literal % or _ (if supported) +-- ✗ parse error: the ESCAPE clause is not supported SELECT * FROM products WHERE name LIKE '100\% cotton' ESCAPE '\'; + +-- A literal % or _: use RLIKE, where both are ordinary characters +SELECT * FROM products WHERE name RLIKE '100% cotton'; -- Matches: '100% cotton' --- Literal underscore -SELECT * FROM codes WHERE code LIKE 'CODE\_123' ESCAPE '\'; +SELECT * FROM codes WHERE code RLIKE 'CODE_123'; -- Matches: 'CODE_123' ``` @@ -1503,6 +1505,8 @@ WHERE category = 'Electronics' AND (price < 100 OR on_sale = true); -- Evaluated as: (category = 'Electronics') AND ((price < 100) OR (on_sale = true)) ``` +> Since 0.24.0 a condition that combines `AND` and `OR` without parentheses is evaluated with this precedence in every clause (`WHERE`, `HAVING`, `CASE WHEN`, `DELETE`, `UPDATE`); earlier versions could evaluate it in another order and return other rows without an error. + **Multiple OR Conditions:** ```sql -- Status check @@ -1580,98 +1584,86 @@ NOT condition **Examples:** +`NOT` negates the ONE condition after it. It is written before a comparison (`NOT price > 100`), and +after the column for `IN`, `LIKE`, `RLIKE`, `BETWEEN` and `IS NULL` (`status NOT IN (...)`, +`manager IS NOT NULL`). A `NOT` before a parenthesised group or before a bare boolean column is not +accepted: write the condition De Morgan's laws give, or compare the column. See +[Known Limitations](known_limitations.md#not-applies-to-one-condition). + **Basic NOT:** ```sql --- Negate boolean column -SELECT * FROM emp WHERE NOT active; --- Same as: WHERE active = false +-- Negate a boolean column: compare it +SELECT * FROM emp WHERE active = false; -- Negate comparison -SELECT * FROM products WHERE NOT (price > 100); +SELECT * FROM products WHERE NOT price > 100; -- Same as: WHERE price <= 100 ``` **NOT with IN:** ```sql -- Exclude values -SELECT * FROM orders WHERE NOT status IN ('cancelled', 'refunded'); --- Same as: WHERE status NOT IN ('cancelled', 'refunded') - --- Explicit NOT -SELECT * FROM products WHERE NOT (category IN ('Discontinued', 'Obsolete')); +SELECT * FROM orders WHERE status NOT IN ('cancelled', 'refunded'); ``` **NOT with BETWEEN:** ```sql -- Outside range -SELECT * FROM products WHERE NOT (price BETWEEN 50 AND 100); --- Same as: WHERE price NOT BETWEEN 50 AND 100 --- Same as: WHERE price < 50 OR price > 100 +SELECT * FROM products WHERE price NOT BETWEEN 50 AND 100; ``` **NOT with LIKE:** ```sql -- Exclude pattern -SELECT * FROM users WHERE NOT (email LIKE '%@spam.com'); --- Same as: WHERE email NOT LIKE '%@spam.com' +SELECT * FROM users WHERE email NOT LIKE '%@spam.com'; -- Multiple NOT LIKE SELECT * FROM products -WHERE NOT (name LIKE '%discontinued%') - AND NOT (name LIKE '%obsolete%'); +WHERE name NOT LIKE '%discontinued%' + AND name NOT LIKE '%obsolete%'; ``` **NOT with IS NULL:** ```sql -- Has value -SELECT * FROM emp WHERE NOT (manager IS NULL); --- Same as: WHERE manager IS NOT NULL +SELECT * FROM emp WHERE manager IS NOT NULL; --- Both fields have values +-- Both fields have values: NOT (email IS NULL OR phone IS NULL) SELECT * FROM contacts -WHERE NOT (email IS NULL OR phone IS NULL); --- Same as: WHERE email IS NOT NULL AND phone IS NOT NULL +WHERE email IS NOT NULL AND phone IS NOT NULL; ``` **NOT with AND/OR:** ```sql -- De Morgan's Law: NOT (A AND B) = (NOT A) OR (NOT B) SELECT * FROM users -WHERE NOT (is_active = true AND is_verified = true); --- Same as: WHERE is_active = false OR is_verified = false +WHERE NOT is_active = true OR NOT is_verified = true; -- De Morgan's Law: NOT (A OR B) = (NOT A) AND (NOT B) SELECT * FROM products -WHERE NOT (category = 'Discontinued' OR in_stock = false); --- Same as: WHERE category != 'Discontinued' AND in_stock = true +WHERE NOT category = 'Discontinued' AND NOT in_stock = false; ``` **Double Negation:** ```sql --- NOT NOT = identity -SELECT * FROM users WHERE NOT (NOT is_active); --- Same as: WHERE is_active - --- Can be confusing, avoid in practice -SELECT * FROM products WHERE NOT (NOT (price > 100)); --- Same as: WHERE price > 100 +-- NOT NOT is the identity: write the condition itself +SELECT * FROM users WHERE is_active = true; +SELECT * FROM products WHERE price > 100; ``` **NOT with Complex Expressions:** ```sql --- Negate entire condition +-- Orders that don't meet ALL three conditions: +-- NOT (status = 'completed' AND payment_status = 'paid' AND total_amount > 1000) SELECT * FROM orders -WHERE NOT ( - status = 'completed' - AND payment_status = 'paid' - AND total_amount > 1000 -); --- Returns orders that don't meet ALL three conditions +WHERE NOT status = 'completed' + OR NOT payment_status = 'paid' + OR NOT total_amount > 1000; --- Negate with parentheses +-- Non-Sales employees OR Sales employees earning >= 50000: +-- NOT (department = 'Sales' AND salary < 50000) SELECT * FROM employees -WHERE NOT (department = 'Sales' AND salary < 50000); --- Returns non-Sales employees OR Sales employees earning >= 50000 +WHERE NOT department = 'Sales' OR NOT salary < 50000; ``` **NOT with EXISTS:** @@ -1685,38 +1677,31 @@ WHERE NOT EXISTS ( **Practical Examples:** ```sql --- Exclude inactive and unverified users +-- Active and verified users: NOT (is_active = false OR is_verified = false) SELECT * FROM users -WHERE NOT (is_active = false OR is_verified = false); --- Same as: WHERE is_active = true AND is_verified = true +WHERE NOT is_active = false AND NOT is_verified = false; -- Products not in specific categories SELECT * FROM products -WHERE NOT (category IN ('Discontinued', 'Clearance', 'Obsolete')); +WHERE category NOT IN ('Discontinued', 'Clearance', 'Obsolete'); -- Orders not in terminal states SELECT * FROM orders -WHERE NOT (status IN ('completed', 'cancelled', 'refunded')); +WHERE status NOT IN ('completed', 'cancelled', 'refunded'); --- Users without complete profile +-- Users without complete profile: +-- NOT (email IS NOT NULL AND phone IS NOT NULL AND address IS NOT NULL) SELECT * FROM users -WHERE NOT ( - email IS NOT NULL - AND phone IS NOT NULL - AND address IS NOT NULL -); +WHERE email IS NULL + OR phone IS NULL + OR address IS NULL; ``` **NULL Handling:** ```sql --- NOT NULL = NULL (not false!) -SELECT * FROM products WHERE NOT (discount IS NULL); --- Same as: WHERE discount IS NOT NULL - --- NOT with NULL comparison -SELECT * FROM users WHERE NOT (status = NULL); --- Always returns no rows (NULL comparison is always NULL) --- Use: WHERE status IS NOT NULL +-- Test NULL with IS NULL / IS NOT NULL, never with = NULL +SELECT * FROM products WHERE discount IS NOT NULL; +SELECT * FROM users WHERE status IS NOT NULL; ``` **Best Practices:** @@ -1725,13 +1710,10 @@ SELECT * FROM users WHERE NOT (status = NULL); SELECT * FROM users WHERE is_active = true; -- Avoid: Double negatives -SELECT * FROM users WHERE NOT (is_active = false); +SELECT * FROM users WHERE NOT is_active = false; -- Good: Use specific operators SELECT * FROM products WHERE category NOT IN ('A', 'B'); - --- Avoid: NOT with IN -SELECT * FROM products WHERE NOT (category IN ('A', 'B')); ``` --- @@ -1746,7 +1728,7 @@ WHERE ( (status = 'pending' AND created_date < DATE_SUB(CURRENT_DATE, INTERVAL 7 DAY)) OR (status = 'processing' AND priority = 'high') ) - AND NOT (customer_type = 'blocked') + AND NOT customer_type = 'blocked' AND total_amount > 0; ``` @@ -1758,29 +1740,28 @@ WHERE ( ```sql -- Without parentheses (follows precedence) SELECT * FROM products -WHERE NOT in_stock AND price < 100 OR on_sale = true; --- Evaluated as: ((NOT in_stock) AND (price < 100)) OR (on_sale = true) +WHERE NOT in_stock = true AND price < 100 OR on_sale = true; +-- Evaluated as: ((NOT in_stock = true) AND (price < 100)) OR (on_sale = true) + +-- With parentheses (explicit, same meaning) +SELECT * FROM products +WHERE (NOT in_stock = true AND price < 100) OR on_sale = true; --- With parentheses (explicit) +-- With parentheses (different logic) SELECT * FROM products -WHERE NOT (in_stock AND price < 100) OR on_sale = true; --- Evaluated as: (NOT (in_stock AND price < 100)) OR (on_sale = true) +WHERE NOT in_stock = true AND (price < 100 OR on_sale = true); +-- Evaluated as: (NOT in_stock = true) AND ((price < 100) OR (on_sale = true)) ``` **De Morgan's Laws:** ```sql -- NOT (A AND B) = (NOT A) OR (NOT B) +-- NOT before a parenthesised group is not supported: write the right-hand side SELECT * FROM users -WHERE NOT (is_active = true AND is_verified = true); --- Equivalent to: -SELECT * FROM users -WHERE is_active = false OR is_verified = false; +WHERE NOT is_active = true OR NOT is_verified = true; -- NOT (A OR B) = (NOT A) AND (NOT B) SELECT * FROM products -WHERE NOT (category = 'A' OR category = 'B'); --- Equivalent to: -SELECT * FROM products WHERE category != 'A' AND category != 'B'; -- Or better: SELECT * FROM products @@ -1838,8 +1819,8 @@ SELECT '2025-01-10'::DATE AS d; SELECT '2025-01-10 14:30:00'::TIMESTAMP AS ts; -- Result: 2025-01-10 14:30:00 --- TIMESTAMP to DATE -SELECT CURRENT_TIMESTAMP::DATE AS today; +-- TIMESTAMP to DATE (`today` is a function name: another alias) +SELECT CURRENT_TIMESTAMP::DATE AS current_day; -- Result: 2025-10-27 -- Date string with explicit cast @@ -1880,9 +1861,9 @@ SELECT 0::BOOLEAN AS b; **In WHERE Clause:** ```sql --- Cast for comparison +-- Cast for comparison (the literal on the right is cast with CAST) SELECT * FROM orders -WHERE order_date::DATE >= '2025-01-01'::DATE; +WHERE order_date::DATE >= CAST('2025-01-01' AS DATE); -- Cast string to number SELECT * FROM products @@ -1890,7 +1871,7 @@ WHERE price_str::DOUBLE > 100; -- Cast to timestamp SELECT * FROM events -WHERE event_time::TIMESTAMP >= '2025-01-10 00:00:00'::TIMESTAMP; +WHERE event_time::TIMESTAMP >= CAST('2025-01-10 00:00:00' AS TIMESTAMP); ``` **In Calculations:** @@ -1911,7 +1892,7 @@ SELECT hire_date::VARCHAR::DATE FROM emp; -- First to VARCHAR, then to DATE -- Cast then manipulate -SELECT (salary::VARCHAR || ' USD') AS formatted_salary +SELECT CONCAT(salary::VARCHAR, ' USD') AS formatted_salary FROM employees; ``` @@ -1930,7 +1911,8 @@ SELECT CAST(hire_date AS DATE) FROM emp; **Complex Examples:** ```sql --- Cast in JOIN condition +-- ✗ refused: a JOIN condition compares the columns themselves +-- ("... cannot use functions in equality expressions") SELECT o.*, p.* FROM orders o JOIN products p ON o.product_id::VARCHAR = p.product_code; @@ -1988,7 +1970,7 @@ SELECT CAST(created_at AS DATE) FROM users; -- Good: Cast both sides of comparison SELECT * FROM orders -WHERE order_date::DATE = '2025-01-10'::DATE; +WHERE order_date::DATE = CAST('2025-01-10' AS DATE); -- Avoid: Implicit type conversion (may cause issues) SELECT * FROM orders WHERE order_date = '2025-01-10'; 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 003cf669..232aaf99 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 @@ -753,13 +753,9 @@ object ElasticAggregation { val currentNestedPath = nested.map(_.nestedPath).getOrElse("") - // No filtering - val fullScript = MetricSelectorScript - .metricSelector(criteria) - .replaceAll("1 == 1 &&", "") - .replaceAll("&& 1 == 1", "") - .replaceAll("1 == 1", "") - .trim + // No filtering at this level is `None`, never a placeholder to strip out of a script: a + // rendered comparison can hold the placeholder's text (`params.max_c1 == 1`). + val fullScript = MetricSelectorScript.selectorScript(criteria).map(_.trim).getOrElse("") // println(s"[DEBUG] currentNestedPath = $currentNestedPath") // println(s"[DEBUG] fullScript (complete) = $fullScript") diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala new file mode 100644 index 00000000..28a127fb --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/BoolQueryModel.scala @@ -0,0 +1,101 @@ +package app.softnetwork.elastic.sql + +import com.fasterxml.jackson.databind.JsonNode + +import scala.jdk.CollectionConverters._ + +/** How Elasticsearch evaluates the `bool` queries a WHERE emits, over documents whose fields `c1 … + * cN` are 0 or 1 (`bits`: field `ci` is bit `i - 1`). + * + * The one rule that matters here, from the Elasticsearch reference: a `bool` with `should` and no + * explicit `minimum_should_match` requires ONE `should` clause only when it holds no `filter` and + * no `must` clause -- next to either, the `should` clauses are optional. Elasticsearch 6 adds one + * exception: a `bool` evaluated in a FILTER context requires one `should` clause anyway. `es6 = + * true` applies that exception. `nested` is read with a ONE-child document (its query is evaluated + * on the same bits); the evaluation context carries through it. + */ +object BoolQueryModel { + + final case class Unmodelled(msg: String) extends Exception(msg) + + def matches(q: JsonNode, bits: Int, es6: Boolean, filterContext: Boolean = false): Boolean = { + val kinds = q.fieldNames().asScala.toList + if (kinds.size != 1) throw Unmodelled(s"query node with keys $kinds") + val body = q.get(kinds.head) + kinds.head match { + case "bool" => + val known = Set( + "filter", + "must", + "must_not", + "should", + "minimum_should_match", + "boost", + "adjust_pure_negative" + ) + body + .fieldNames() + .asScala + .find(k => !known.contains(k)) + .foreach(k => throw Unmodelled(s"bool key $k")) + def clauses(name: String): List[JsonNode] = Option(body.get(name)) match { + case Some(n) if n.isArray => n.elements().asScala.toList + case Some(n) => List(n) + case None => Nil + } + val (filters, musts, nots, shoulds) = + (clauses("filter"), clauses("must"), clauses("must_not"), clauses("should")) + val required = Option(body.get("minimum_should_match")).map(_.asText.toInt).getOrElse { + if (shoulds.isEmpty) 0 + else if (es6 && filterContext) 1 + else if (filters.isEmpty && musts.isEmpty) 1 + else 0 + } + filters.forall(matches(_, bits, es6, filterContext = true)) && + musts.forall(matches(_, bits, es6, filterContext)) && + nots.forall(n => !matches(n, bits, es6, filterContext = true)) && + shoulds.count(matches(_, bits, es6, filterContext)) >= required + case "nested" => matches(body.get("query"), bits, es6, filterContext) + case "term" => + val field = body.fieldNames().asScala.toList.head + val v = Option(body.get(field)).map(n => if (n.isObject) n.get("value") else n).get + val i = """c(\d+)""".r + .findFirstMatchIn(field) + .map(_.group(1).toInt) + .getOrElse(throw Unmodelled(s"term on $field")) + val bit = ((bits >> (i - 1)) & 1) == 1 + (if (bit) 1.0 else 0.0) == v.asDouble + case "match" => + // `MATCH (ci) AGAINST ('1')`: read like `ci = 1` -- one term, the field's value + val field = body.fieldNames().asScala.toList.head + val v = Option(body.get(field)).map(n => if (n.isObject) n.get("query") else n).get + val i = """c(\d+)""".r + .findFirstMatchIn(field) + .map(_.group(1).toInt) + .getOrElse(throw Unmodelled(s"match on $field")) + val bit = ((bits >> (i - 1)) & 1) == 1 + bit == (v.asText == "1") + case "match_all" => true + case other => throw Unmodelled(s"query kind $other") + } + } + + /** Every `bool` that holds `should` clauses next to `filter` or `must` clauses without an + * explicit `minimum_should_match` -- where its `should` clauses are optional. + */ + def optionalShoulds(q: JsonNode): Int = { + var count = 0 + def walk(n: JsonNode): Unit = + if (n.isObject) { + Option(n.get("bool")).filter(_.isObject).foreach { b => + val should = Option(b.get("should")).exists(_.size > 0) + val required = + Option(b.get("filter")).exists(_.size > 0) || Option(b.get("must")).exists(_.size > 0) + if (should && required && b.get("minimum_should_match") == null) count += 1 + } + n.elements().asScala.foreach(walk) + } else if (n.isArray) n.elements().asScala.foreach(walk) + walk(q) + count + } +} diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala new file mode 100644 index 00000000..4916a1ac --- /dev/null +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/ConditionBoolEmissionSpec.scala @@ -0,0 +1,128 @@ +package app.softnetwork.elastic.sql + +import app.softnetwork.elastic.sql.bridge._ +import app.softnetwork.elastic.sql.parser.ConditionPopulation._ +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query.SingleSearch +import com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** The Elasticsearch query a WHERE emits evaluates SQL's reading of the condition. + * + * 🔴 A correct condition TREE is not enough: an unparenthesised sub-condition used to be written + * into its parent's `bool`, where an OR under an AND put its `should` clauses next to `filter` + * clauses -- optional for Elasticsearch. The query is evaluated here with Elasticsearch's own + * `bool` rules (`BoolQueryModel`), for Elasticsearch 7+ and for the Elasticsearch 6 filter-context + * rule, against the oracle of the generated structure. + */ +class ConditionBoolEmissionSpec extends AnyFlatSpec with Matchers { + + implicit val timestamp: Long = 0L + + private val mapper = new ObjectMapper() + + /** P2: the WHERE statements of P1 whose leaves are all `ci = 1` (every class but the kinds). */ + private lazy val wherePopulation: List[Gen] = + population().filter(g => g.clause == WhereC && g.cls != "K") + + private def queryOf(search: SingleSearch): JsonNode = { + val request: ElasticSearchRequest = search + mapper.readTree(request.query).get("query") + } + + private def searchOf(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] $other") + } + + "a WHERE that combines AND and OR" should "emit a query that evaluates SQL's reading" in { + val wrong = wherePopulation.flatMap { g => + val q = queryOf(searchOf(g.sql)) + val bits = 0 until (1 << g.n) + val es7 = bits.map(BoolQueryModel.matches(q, _, es6 = false)).toVector + val es6 = bits.map(BoolQueryModel.matches(q, _, es6 = true)).toVector + if (es7 == g.ansiTable && es6 == g.ansiTable) None + else Some(s"[${g.sql}] es7=${es7 == g.ansiTable} es6=${es6 == g.ansiTable}: $q") + } + withClue( + s"${wrong.size} of ${wherePopulation.size} wrong, first 10:\n${wrong.take(10).mkString("\n")}\n" + ) { + wrong shouldBe empty + } + } + + it should "never leave should clauses beside filter or must clauses" in { + val offending = wherePopulation + .map(g => g.sql -> BoolQueryModel.optionalShoulds(queryOf(searchOf(g.sql)))) + .filter(_._2 > 0) + withClue(s"first 10: ${offending.take(10).mkString("\n")}\n") { offending shouldBe empty } + } + + "a WHERE with ONE operator" should "stay one flat bool" in { + queryOf( + searchOf("SELECT id FROM t WHERE c1 = 1 AND c2 = 1 AND c3 = 1 AND c4 = 1") + ).toString shouldBe + """{"bool":{"filter":[{"term":{"c1":{"value":1}}},{"term":{"c2":{"value":1}}},{"term":{"c3":{"value":1}}},{"term":{"c4":{"value":1}}}]}}""" + queryOf(searchOf("SELECT id FROM t WHERE c1 = 1 OR c2 = 1 OR c3 = 1")).toString shouldBe + """{"bool":{"should":[{"term":{"c1":{"value":1}}},{"term":{"c2":{"value":1}}},{"term":{"c3":{"value":1}}}]}}""" + } + + "DELETE and UPDATE" should "send the query the same WHERE sends in a SELECT" in { + val wrong = wherePopulation.filter(_.mixedLevel).flatMap { g => + val select = searchOf(g.sql) + val expected = queryOf(select) + Seq( + "DELETE" -> select.copy(deleteByQuery = true), + "UPDATE" -> select.copy(updateByQuery = true) + ) + .collect { case (kind, s) if queryOf(s) != expected => s"$kind [${g.sql}]: ${queryOf(s)}" } + } + wrong shouldBe empty + } + + "an OR group holding a MATCH, under an AND" should "stay ONE condition of the AND" in { + // Spread into the root bool beside its `filter` clauses, the group's `should` clauses turned + // optional: `c1 = 1 AND (MATCH ... OR c3 = 1)` selected every document with c1 = 1. A MATCH + // over several columns is such a group too. The group that IS the whole condition is still + // spread, which is exact (the flat `should` of a lone MATCH). + Seq( + "c1 = 1 AND (MATCH (c2) AGAINST ('1') OR c3 = 1)" -> (3, A(L(1), O(L(2), L(3)))), + "(MATCH (c1) AGAINST ('1') OR c2 = 1) AND c3 = 1" -> (3, A(O(L(1), L(2)), L(3))), + "c1 = 1 AND MATCH (c2, c3) AGAINST ('1')" -> (3, A(L(1), O(L(2), L(3)))), + "(c1 = 1 OR MATCH (c2) AGAINST ('1')) AND (c3 = 1 OR MATCH (c4) AGAINST ('1'))" -> + (4, A(O(L(1), L(2)), O(L(3), L(4)))), + "MATCH (c1, c2) AGAINST ('1')" -> (2, O(L(1), L(2))), + "MATCH (c1) AGAINST ('1') OR c2 = 1 AND c3 = 1" -> (3, O(L(1), A(L(2), L(3)))) + ).foreach { case (cond, (n, expected)) => + val q = queryOf(searchOf(s"SELECT id FROM t WHERE $cond")) + val bits = 0 until (1 << n) + withClue(s"[$cond] $q ") { + bits.map(BoolQueryModel.matches(q, _, es6 = false)).toVector shouldBe table(expected, n) + bits.map(BoolQueryModel.matches(q, _, es6 = true)).toVector shouldBe table(expected, n) + BoolQueryModel.optionalShoulds(q) shouldBe 0 + } + } + } + + "a WHERE over an UNNEST column" should "emit the nested query SQL's reading needs" in { + // one child per document: a nested query is its own query + Seq( + "inner_items.c1 = 1 OR inner_items.c2 = 1 AND inner_items.c3 = 1" -> O(L(1), A(L(2), L(3))), + "c1 = 1 OR inner_items.c2 = 1 AND inner_items.c3 = 1" -> O(L(1), A(L(2), L(3))), + "inner_items.c1 = 1 AND inner_items.c2 = 1 OR c3 = 1" -> O(A(L(1), L(2)), L(3)) + ).foreach { case (cond, expected) => + val q = queryOf(searchOf(s"SELECT id FROM t JOIN UNNEST(t.items) AS inner_items WHERE $cond")) + withClue(s"[$cond] $q ") { + (0 until 8).map(BoolQueryModel.matches(q, _, es6 = false)).toVector shouldBe table( + expected, + 3 + ) + (0 until 8).map(BoolQueryModel.matches(q, _, es6 = true)).toVector shouldBe table( + expected, + 3 + ) + } + } + } +} diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 1c10886f..77805bfe 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -365,6 +365,26 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { ) } + "a comparison of an aggregate with 1 or 10" should "read that aggregate's own parameter" in { + // 🔴 The emission stripped every `1 == 1` out of the script -- the text the selector answers + // when there is nothing to filter -- and `params.max_c1 == 1` holds that text. MEASURED on the + // base: `= 1` read `params.max_c`, `= 10` read `params.max_c0`, and `IN (1, 2)` lost its first + // member; on Elasticsearch 8.18.3 all three searches failed (`Cannot invoke + // "Object.getClass()" because "value" is null`). + Seq( + "MAX(c1) = 1" -> "(params.max_c1 == null ? false : (params.max_c1 == 1))", + "MAX(c1) = 10" -> "(params.max_c1 == null ? false : (params.max_c1 == 10))", + "MAX(c1) IN (1, 2)" -> "(params.max_c1 == null ? false : (params.max_c1 == 1 || params.max_c1 == 2))" + ).foreach { case (condition, script) => + withClue(s"[$condition] ") { + queryOf(s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING $condition") should include( + """"having_filter":{"bucket_selector":{"buckets_path":{"max_c1":"max_c1"},""" + + s""""script":{"source":"$script"}}}""" + ) + } + } + } + "a wrapped aggregate beside a bucket-key predicate" should "honour BOTH mechanisms" in { // The key predicate is a `terms` exclude, the aggregate one a `bucket_selector`. Neither may // cost the other. 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 0ff8600d..345c6d96 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 @@ -5054,8 +5054,8 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { } it should "emit no bucket_selector for a HAVING with no aggregate over an aggregate-free GROUP BY" in { - // `metricSelectorForBucket` strips "1 == 1" to the empty string, so the HAVING becomes a terms - // exclude and no bucket_selector is produced. + // `metricSelectorForBucket` finds nothing to filter at this level (`selectorScript` is `None`), + // so the HAVING becomes a terms exclude and no bucket_selector is produced. val select: ElasticSearchRequest = SelectStatement("SELECT category FROM Table GROUP BY category HAVING category <> 'x'") val query = select.query diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala index 84c9cb00..8c7e9a83 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala @@ -85,6 +85,16 @@ import app.softnetwork.elastic.sql.query.{ Where } +/** One element of a condition once the scanner's tokens are taken apart (`WhereParser.flatten`): an + * operand, or the operator joining two operands together with the `NOT` the scanner attached to + * the operand AFTER it. Top-level, not inside the trait: an inner case class of a trait cannot be + * type-tested without an unchecked outer reference. + */ +private[parser] sealed trait ChainItem +private[parser] final case class ChainOperand(criteria: Criteria) extends ChainItem +private[parser] final case class ChainJunction(operator: PredicateOperator, negatesNext: Boolean) + extends ChainItem + trait WhereParser { self: Parser with GroupByParser with OrderByParser => @@ -488,7 +498,7 @@ trait WhereParser { * 🔴 Why it had to change. `rep1` offers a bare `end` and never backtracks, so for every * PARENTHESISED body whose last clause is a WHERE or a HAVING — story 22.1's `FROM (SELECT a * FROM t WHERE x = 1) d`, story 22.2's `IN (SELECT id FROM c WHERE r = 'EU')` — the inner clause - * swallowed the subquery's own `)`, `processTokensHelper`'s top-level `EndDelimiter` arm + * swallowed the subquery's own `)`, the reducer's (`processTokens`) top-level `EndDelimiter` arm * answered `Left("Unbalanced parentheses")` and `where` raised it as a NON-backtracking `err` * that killed the whole statement. It is the mechanism story 21.4 met inside relation predicates * and fixed by giving them `relationTokens`, which omits `end` (`:264-293`) — the same defect, @@ -497,7 +507,7 @@ trait WhereParser { * Depth is counted on `StartPredicate` / `EndPredicate` ONLY, because those are the only * delimiters this alternation can emit: `start` produces `StartPredicate`, `end` produces * `EndPredicate`, and `then_case` produces `ThenCase` — which IS an `EndDelimiter` but must keep - * being consumed, since `processTokensHelper` reads it as end-of-tokens for a CASE-WHEN + * being consumed, since the reducer (`processTokens`) reads it as end-of-tokens for a CASE-WHEN * condition. A parenthesis that an ITEM consumes (a function call, a relation predicate's own * group, `IN (1, 2)`) never reaches this counter: items are consumed atomically. * @@ -557,67 +567,31 @@ trait WhereParser { import scala.annotation.tailrec - /** This method is used to recursively process a list of SQL tokens and construct SQL criteria and - * predicates from these tokens. Here are the key points: - * - * Base case (Nil): If the list of tokens is empty (Nil), we check the contents of the stack to - * determine the final result. - * - * If the stack contains an operator, a left criterion and a right criterion, we create a - * SQLPredicate predicate. Otherwise, we return the first criterion (SQLCriteria) of the stack if - * it exists. Case of criteria (SQLCriteria): If the first token is a criterion, we treat it - * according to the content of the stack: + /** The tokens of ONE parenthesis level as an alternating operand / junction list. * - * If the stack contains a predicate operator, we create a predicate with the left and right - * criteria and update the stack. Otherwise, we simply add the criterion to the stack. Case of - * operators (SQLPredicateOperator): If the first token is a predicate operator, we treat it - * according to the contents of the stack: + * 🔴 `whereCriteria` scans `allPredicate` first, and `allPredicate` holds the binary + * [[predicate]] production (`criteria (AND|OR) [NOT] criteria`). So the scanner PRE-PAIRS two + * adjacent conditions into one `Predicate` token, left to right and whatever their operators: `a + * OR b AND c` arrives as `[a OR b], AND, c`. A precedence reduction must take the pairs apart + * before it can apply AND before OR. A pair is taken apart WITHOUT moving its `NOT`: the scanner + * put it on the pair's right operand, and [[reduceChain]] puts it back there whenever that + * operand stays a right operand. `predicate` itself must stay in the scanner: it is the only + * home of a `NOT` in front of a LIKE, IN, BETWEEN or IS condition after an operator (`a AND NOT + * b LIKE 'x%'`) -- those carry their own NOT only after the column. * - * If the stack contains at least two elements, we create a predicate with the left and right - * criterion and update the stack. If the stack contains only one element (a single operator), we - * simply add the operator to the stack. Otherwise, it is an invalid stack state. Case of - * delimiters (StartDelimiter and EndDelimiter): If the first token is a start delimiter - * (StartDelimiter), we extract the tokens up to the corresponding end delimiter (EndDelimiter), - * we recursively process the extracted sub-tokens, then we continue with the rest of the tokens. - * A closing delimiter that reaches this scan is unmatched, because a balanced group is consumed - * whole by extractSubTokens. + * A parenthesised group is reduced on its own ([[processSubTokens]]) and enters the chain as ONE + * operand, marked `group = true`: the mark `Predicate.sql` renders the parentheses from. `THEN` + * ends the scan: `case_condition` hands this reducer its tokens up to and including it. * - * Rejections: every failure is returned as a `Left(reason)` and NEVER thrown (#250). - * `Parser.apply` is typed `Either[ParserError, Statement]` and five production call sites match - * on that Either with no `try` of their own; the combinator callers of this helper turn a `Left` - * into `err(reason)`. - * - * @param tokens - * - list of SQL tokens - * @param stack - * - stack of tokens - * @return - * the criteria built from the tokens, or a Left carrying the reason the tokens are invalid + * Rejections are returned, never thrown (#250). */ @tailrec - private def processTokensHelper( + private def flatten( tokens: List[Token], - stack: List[Token] - ): Either[String, Option[Criteria]] = { + acc: List[ChainItem] + ): Either[String, List[ChainItem]] = tokens match { - case Nil => - stack match { - case (right: Criteria) :: (op: PredicateOperator) :: (left: Criteria) :: Nil => - Right(Option(Predicate(left, op, right))) - // #250 - a Criteria head with anything still UNDER it means the tokens folded into a - // stack this function cannot reduce, and returning just the head would SILENTLY DROP the - // rest: the same defect class as the EndDelimiter arm below, and the #213 family. It used - // to return `stack.headOption`. - // MEASURED 2026-09-05 by instrumenting this arm and running every suite that parses SQL - // (sql 592, core 856, bridge 120, macros-tests 19): NO input reaches it with a Criteria - // head and a non-empty tail. The shapes that do reach the fallback below all have a - // PredicateOperator head - a dangling AND/OR - and must keep yielding `Right(None)` so - // the caller's "WHERE/HAVING clause requires criteria" message wins over this one. - case (_: Criteria) :: rest if rest.nonEmpty => - Left("Invalid stack state for predicate creation") - case _ => - Right(stack.headOption.collect { case c: Criteria => c }) - } + case Nil | (ThenCase :: _) => Right(acc.reverse) case (_: StartDelimiter) :: rest => extractSubTokens(rest, 1) match { case Left(reason) => Left(reason) @@ -625,89 +599,97 @@ trait WhereParser { processSubTokens(subTokens) match { case Left(reason) => Left(reason) case Right(p: Predicate) => - processTokensHelper(remainingTokens, p.copy(group = true) :: stack) - case Right(c) => - processTokensHelper(remainingTokens, c :: stack) + flatten(remainingTokens, ChainOperand(p.copy(group = true)) :: acc) + case Right(c) => flatten(remainingTokens, ChainOperand(c) :: acc) } } - case (c: Criteria) :: rest => - stack match { - case (op: PredicateOperator) :: (left: Criteria) :: tail => - val predicate = Predicate(left, op, c) - processTokensHelper(rest, predicate :: tail) - case _ => - processTokensHelper(rest, c :: stack) - } + case (p: Predicate) :: rest => + flatten( + rest, + ChainOperand(p.rightCriteria) :: ChainJunction(p.operator, p.not.isDefined) :: + ChainOperand(p.leftCriteria) :: acc + ) + case (c: Criteria) :: rest => flatten(rest, ChainOperand(c) :: acc) case (op: PredicateOperator) :: rest => - stack match { - case (right: Criteria) :: (left: Criteria) :: tail => - val predicate = Predicate(left, op, right) - processTokensHelper(rest, predicate :: tail) - case (right: Criteria) :: (o: PredicateOperator) :: tail => - tail match { - case (left: Criteria) :: tt => - val predicate = Predicate(left, op, right) - processTokensHelper(rest, o :: predicate :: tt) - case _ => - processTokensHelper(rest, op :: stack) - } - case _ :: Nil => - processTokensHelper(rest, op :: stack) - case _ => - // #250 - was `throw ValidationError(...)`. `Parser.apply` is typed - // `Either[ParserError, Statement]`; five production call sites match on that Either - // with no `try` of their own (SQLImplicits.queryToStatement, IndicesApi x3, the - // searchAs macro). The caller turns this into `err(...)`, which short-circuits - // `where.?` instead of silently yielding a None. - Left("Invalid stack state for predicate creation") - } - case ThenCase :: _ => - processTokensHelper(Nil, stack) // exit processing on THEN + flatten(rest, ChainJunction(op, negatesNext = false) :: acc) case (_: EndDelimiter) :: _ => - // A closing delimiter reaching the TOP-LEVEL scan means no `StartDelimiter` arm above ever - // took ownership of it. This used to "ignore and move on", which silently discarded it. - // TWO different inputs land here, and BOTH used to be corrupted rather than reported - // (measured 2026-09-05 by reverting just this arm): - // - // 1. A genuinely stray `)`. `SELECT a FROM t WHERE a = 1)` parsed as `... WHERE a = 1`. - // - // 2. 🔴 A BALANCED relation predicate with THREE OR MORE criteria - so the reason text - // "Unbalanced parentheses" is accurate about the TOKEN STREAM, not about what the - // user typed. `nestedPredicate`/`childPredicate`/`parentPredicate` take a `predicate`, - // which is strictly BINARY (`criteria ~ (and|or) ~ not.? ~ criteria`), so with a third - // criterion they fail and the parser falls back to - // `nestedCriteria`/`childCriteria`/`parentCriteria` = `X.regex ~ start.? ~ criteria ~ - // end.?`. That takes ONE criterion, its `start.?` swallows the `(`, its `end.?` finds - // `AND` instead of `)` - and the real `)` arrives here with nothing to close. - // Measured before this change: - // `WHERE id = 1 AND child(a = 2 AND b = 3 AND c = 4)` - // parsed as `WHERE id = 1 AND CHILD(a = 2) AND b = 3 AND c = 4` - // i.e. the CHILD scope silently collapsed to the first criterion and the other two - // escaped onto the parent document - a wrong answer that executes and returns rows. - // `child(x = 1 OR y = 2 OR z = 3)` likewise became `CHILD(x = 1) OR y = 2 OR z = 3`. - // Rejecting is strictly better, and is the #213 family this story is closing; the - // `start.?`/`end.?` asymmetry that causes it belongs to a later story (local record - // docs/issues/local-21.4-relation-predicate-paren-asymmetry.md). Its twin hole - an - // unmatched OPENING paren, `child(a = 1 AND b = 2`, still silently accepted - is NOT - // reachable from here and is recorded there too. + // A closing delimiter reaching the scan of a level means no `StartDelimiter` arm ever took + // ownership of it: a stray `)` (`WHERE a = 1)`), or -- before story 21.4 -- the `)` of a + // relation predicate whose `(` the paren-less form swallowed. Pinned in ParserTotalitySpec. Left("Unbalanced parentheses") case unexpected :: _ => - // #250 - this arm used to be `processTokensHelper(Nil, stack)`, which ABANDONED every - // remaining token and returned whatever the stack happened to hold: a silent truncation of - // the clause the user wrote. It is believed unreachable - `whereCriteria` scans - // `allPredicate | allCriteria | start | or | and | end | then_case` (story 22.1 added a - // depth rule, not a token kind) and every one of those token kinds is matched by an arm - // above - and it was NEVER reached while - // instrumented across the sql, core, bridge and macros-tests suites (2026-09-05). That is - // exactly why it must not silently truncate: an unreachable arm that loses data is one - // grammar change away from being reachable. Same reasoning as the defensive arm in - // `parser/operator/math`. + // Believed unreachable (`whereCriteria` emits no other token kind) -- and must not + // silently truncate the clause if a grammar change makes it reachable (#250). Left(s"Unexpected token in predicate: ${unexpected.getClass.getSimpleName}") } + + /** Reduces one level with SQL precedence -- `NOT` > `AND` > `OR`, left-associative, parentheses + * respected (a group is one operand). + * + * The operands are split at every `OR` into runs joined by `AND`; each run is folded left, and + * the runs are folded left with `OR`. So `a OR b AND c` is `a OR (b AND c)`, `a AND b OR c AND + * d` is `(a AND b) OR (c AND d)`, and a one-operator chain is the left fold. + * + * The scanner's `NOT` qualifies the operand after its operator, and that operand is the RIGHT + * operand of the predicate built for it -- `Predicate.not`, as the scanner built it -- except in + * ONE position: the first operand of a run that follows an `OR` becomes the LEFT operand of the + * run's first `AND`, and `Predicate` has no left-hand `NOT`. There the negation is folded into + * the operand through `Criteria.negated`, which is the criterion the grammar itself builds for + * that text at the start of a clause (`NOT a = 1 AND b = 2`, `a NOT LIKE 'x%' AND b = 2`, + * `ISNOTNULL(a) AND b = 2`). A criterion that cannot carry its own `NOT` (`MATCH ... AGAINST`) + * is refused there by name -- it is refused at the start of a clause too. + * + * The shapes of the old reduction are kept where they were right: a dangling operator is still + * `Right(None)` (the caller names the clause); a leading operator and two consecutive operands + * or operators are still `Left("Invalid stack state for predicate creation")`. + */ + private def reduceChain(items: List[ChainItem]): Either[String, Option[Criteria]] = { + val invalid = "Invalid stack state for predicate creation" + def not(negated: Boolean): Option[NOT.type] = if (negated) Some(NOT) else None + // The LEFT operand of a run's first AND. + def runHead(c: Criteria, negated: Boolean): Either[String, Criteria] = + if (!negated) Right(c) + else + c.negated.toRight( + s"NOT ${c.sql} cannot start conditions joined by AND after an OR: write it after the " + + s"AND, for example A OR (B AND NOT ${c.sql})" + ) + // `runs`: the finished OR operands, newest first, each with the scanner's NOT of a run that + // stayed ONE operand long; `current` / `currentNot`: the run being folded. + @tailrec + def loop( + rest: List[ChainItem], + runs: List[(Criteria, Boolean)], + current: Criteria, + currentNot: Boolean + ): Either[String, Option[Criteria]] = + rest match { + case Nil => + val all = ((current, currentNot) :: runs).reverse + Right(Some(all.tail.foldLeft(all.head._1) { case (acc, (run, negated)) => + Predicate(acc, OR, run, not(negated)) + })) + case ChainJunction(_, _) :: Nil => Right(None) // a dangling operator + case ChainJunction(AND, negated) :: ChainOperand(c) :: tail => + runHead(current, currentNot) match { + case Left(reason) => Left(reason) + case Right(head) => + loop(tail, runs, Predicate(head, AND, c, not(negated)), currentNot = false) + } + case ChainJunction(OR, negated) :: ChainOperand(c) :: tail => + loop(tail, (current, currentNot) :: runs, c, negated) + case _ => Left(invalid) + } + items match { + case Nil => Right(None) + case ChainOperand(first) :: rest => loop(rest, Nil, first, currentNot = false) + case _ => Left(invalid) + } } - /** This method calls processTokensHelper with an empty stack (Nil) to begin processing primary - * tokens. + /** Reduces the tokens of a WHERE / HAVING / JOIN ON / CASE WHEN condition, or of a relation body, + * to one criteria tree: [[flatten]] takes the scanner's pairs apart, then [[reduceChain]] + * applies SQL precedence. * * Narrowed to `private[parser]` with #250: its four callers (`where` here, * `HavingParser.having`, `FromParser.on` and `parser.function.cond.case_condition`) are all @@ -721,13 +703,12 @@ trait WhereParser { */ private[parser] def processTokens( tokens: List[Token] - ): Either[String, Option[Criteria]] = { - processTokensHelper(tokens, Nil) - } + ): Either[String, Option[Criteria]] = + flatten(tokens, Nil).flatMap(reduceChain) - /** This method is used to process subtokens extracted between delimiters. It calls - * processTokensHelper and returns the result as a SQLCriteria, or a `Left` carrying the reason - * no criteria could be built (#250 - it used to throw). + /** This method is used to process subtokens extracted between delimiters. It reduces them with + * [[processTokens]] and returns the result as a SQLCriteria, or a `Left` carrying the reason no + * criteria could be built (#250 - it used to throw). * * @param tokens * - list of SQL tokens @@ -735,7 +716,7 @@ trait WhereParser { * the criteria built from the sub-tokens, or a Left carrying the reason they are invalid */ private def processSubTokens(tokens: List[Token]): Either[String, Criteria] = - processTokensHelper(tokens, Nil) match { + processTokens(tokens) match { case Right(Some(criteria)) => Right(criteria) case Right(None) => Left("Empty sub-expression") case Left(reason) => Left(reason) 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 8238ada0..b9497f92 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 @@ -399,9 +399,19 @@ object MetricSelectorScript { * it is the invariant's second line of defence, for a `SingleSearch` assembled in code and * emitted without validation. */ - def metricSelector(expr: Criteria): String = selector(expr) match { - case NoFilter => "1 == 1" - case Filter(script) => script + def metricSelector(expr: Criteria): String = selectorScript(expr).getOrElse("1 == 1") + + /** [[metricSelector]] without its `"1 == 1"` placeholder: the script, or `None` when there is + * nothing to filter at this level. Throws exactly where [[metricSelector]] does. + * + * 🔴 The emission must never look for the placeholder INSIDE a script. It used to strip it with + * `replaceAll("1 == 1", "")`, and a rendered comparison can hold that very text: `MAX(c1) = 1` + * renders `params.max_c1 == 1`, which the strip turned into `params.max_c` -- a parameter no + * aggregation publishes, and the search failed -- and `MAX(c1) = 10` into `params.max_c0`. + */ + def selectorScript(expr: Criteria): Option[String] = selector(expr) match { + case NoFilter => None + case Filter(script) => Some(script) case u: Unrepresentable => throw new IllegalStateException(u.message) } @@ -449,13 +459,21 @@ object MetricSelectorScript { // 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 `!( ... )`. + // + // The script must evaluate the TREE. Painless, like SQL, binds `&&` tighter than `||`, so + // an operand joined by OR under an AND is parenthesised whatever its `group` flag -- a + // written group renders `(a) || (b)` for itself and nothing around it, and its parent used + // to read `(a) || (b) && c` as `a || (b && c)`. + val l = if (PredicatePrecedence.orUnderAnd(left, op)) s"($leftStr)" else leftStr + val r = + if (PredicatePrecedence.orUnderAnd(effectiveRight, op)) s"($rightStr)" else rightStr Filter(maybeNot match { case Some(_) if right.negated.isDefined => s"($leftStr) $opStr $rightStr" // Grammar-unreachable today (`NOT (A AND B)` in HAVING is a parse rejection); kept // as the total fallback for a compound right side. case Some(_) => s"($leftStr) $opStr !($rightStr)" case None if group => s"($leftStr) $opStr ($rightStr)" - case None => s"$leftStr $opStr $rightStr" + case None => s"$l $opStr $r" }) } 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 fd129495..3de6287a 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 @@ -145,13 +145,9 @@ case class Having(criteria: Option[Criteria]) extends Updateable { def script: Option[String] = criteria.flatMap { criteria => if (unrepresentable.nonEmpty) None else { - val fullScript = MetricSelectorScript - .metricSelector(criteria) - .replaceAll("1 == 1 &&", "") - .replaceAll("&& 1 == 1", "") - .replaceAll("1 == 1", "") - .trim - if (fullScript.nonEmpty) Some(fullScript) else None + // `None` when there is nothing to filter -- never a placeholder stripped out of the script, + // which also ate `params.max_c1 == 1` (see `MetricSelectorScript.selectorScript`). + MetricSelectorScript.selectorScript(criteria).map(_.trim).filter(_.nonEmpty) } } } 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 02f4c950..29986131 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 @@ -363,10 +363,19 @@ case class Predicate( not: Option[NOT.type] = None, group: Boolean = false ) extends Criteria { - override def sql = s"${if (group) s"($leftCriteria" - else leftCriteria} $operator${not - .map(_ => " NOT") - .getOrElse("")} ${if (group) s"$rightCriteria)" else rightCriteria}" + + /** Parenthesised where the user wrote parentheses (`group`), and wherever an operand binds LOOSER + * than this predicate -- an `OR` under an `AND` -- whatever its flag, so the text always + * re-parses to this tree under SQL precedence: a tree built by the parser never needs the second + * rule, a tree rebuilt later (a relation wrapper stripped by `update`, a rewrite) may. + */ + override def sql: String = { + def operand(c: Criteria): String = + if (PredicatePrecedence.bindsLooserThan(c, operator)) s"(${c.sql})" else c.sql + val body = + s"${operand(leftCriteria)} $operator${not.map(_ => " NOT").getOrElse("")} ${operand(rightCriteria)}" + if (group) s"($body)" else body + } /** The polarity the RIGHT criterion inherits when a `terms` include/exclude traversal walks this * predicate. The predicate's own `NOT` binds the RIGHT operand, and this IS its fold: the @@ -435,6 +444,13 @@ case class Predicate( * So the first three fold and the fourth does not, each for a stated reason. * `PainlessNullSurvivalSpec`'s source scan fails if a consumer appears that states NEITHER — * silence is the failure mode both round-10 defects had in common. + * + * What the four consumers can meet is set by the reducer (`WhereParser.reduceChain`): it builds + * `Predicate.not` only where the scanner put it, on a right operand that is ONE condition. At + * the head of an AND run after an OR (`a OR NOT b AND c`) that condition becomes a LEFT operand, + * so the reducer folds the `NOT` into it through `negated` -- the criterion the grammar builds + * for the same text at a condition's start -- and refuses by name a condition that cannot carry + * its own `NOT`. The four consumers are unchanged. */ private[query] lazy val (emittedRight: Criteria, notConsumed: Boolean) = not match { @@ -448,15 +464,23 @@ case class Predicate( // `must_not` over a script MATCHES documents the script rejects, including those that lack the // field, which is the opposite of the three-valued reading the criterion itself emits (B-2). val negate = not.isDefined && !notConsumed + // A child predicate writes into THIS bool only when it joins its operands with the same + // operator (AND in `filter`, OR in `should`: associative, so flattening is exact). An operator + // change opens a bool of its own, as a written group always did: shared, an OR under an AND put + // its `should` clauses next to `filter` clauses, where Elasticsearch treats them as optional + // (`minimum_should_match` defaults to 0 once a `filter` or `must` clause is present). + def filterOf(c: Criteria): ElasticFilter = c match { + case p: Predicate if !p.group && p.operator != operator => + p.copy(group = true).asFilter(Option(query)) + case other => other.asFilter(Option(query)) + } operator match { case AND => - (if (negate) query.not(emittedRight.asFilter(Option(query))) - else query.filter(emittedRight.asFilter(Option(query)))) - .filter(leftCriteria.asFilter(Option(query))) + (if (negate) query.not(filterOf(emittedRight)) else query.filter(filterOf(emittedRight))) + .filter(filterOf(leftCriteria)) case OR => - (if (negate) query.not(emittedRight.asFilter(Option(query))) - else query.should(emittedRight.asFilter(Option(query)))) - .should(leftCriteria.asFilter(Option(query))) + (if (negate) query.not(filterOf(emittedRight)) else query.should(filterOf(emittedRight))) + .should(filterOf(leftCriteria)) } } @@ -481,6 +505,37 @@ case class Predicate( leftCriteria.nestedCriteria(innerHitsName) ++ rightCriteria.nestedCriteria(innerHitsName) } +/** SQL precedence for the renderings of a criteria tree (`Predicate.sql`, `MetricSelectorScript`). + * NOT a companion of `Predicate`: an explicit companion would drop the synthetic one's `Function5` + * parent -- a binary change for no reason. + */ +private[query] object PredicatePrecedence { + + /** Does `operand`, as an operand of a predicate joined by `parent`, bind LOOSER than it -- an + * `OR` under an `AND`, not written in parentheses? Then its text needs parentheses to keep its + * meaning (SQL and Painless both bind AND tighter than OR). The transparent wrapper `update` + * puts around a nested operand renders bare, so it is looked through; a written relation renders + * its own. + */ + private[query] def bindsLooserThan(operand: Criteria, parent: PredicateOperator): Boolean = + operand match { + case p: Predicate => !p.group && orUnderAnd(p, parent) + case n: ElasticNested if n.fromCriteria => bindsLooserThan(n.criteria, parent) + case _ => false + } + + /** Is `operand` -- a predicate, seen through any relation -- joined by OR while its parent joins + * by AND? Asked by `MetricSelectorScript`, whose Painless for a written group parenthesises the + * group's OPERANDS and not the group itself, so the parent must parenthesise it, flag or not. + */ + private[query] def orUnderAnd(operand: Criteria, parent: PredicateOperator): Boolean = + operand match { + case p: Predicate => p.operator == OR && parent == AND + case r: ElasticRelation => orUnderAnd(r.criteria, parent) + case _ => false + } +} + sealed trait ElasticFilter case class ElasticBoolQuery( @@ -524,7 +579,17 @@ case class ElasticBoolQuery( notFilters = this.notFilters, shouldFilters = this.shouldFilters ) + // A MATCH-bearing bool that holds `should` clauses is ONE condition: an OR group + // (`c = 1 AND (MATCH (t) AGAINST ('x') OR d = 1)`), or a MATCH over several columns. Spread into + // this bool beside any other clause, its `should` clauses would turn optional (Elasticsearch + // requires one only while the bool holds no `filter` / `must` clause) or merge with another + // group's. So it is spread only when it is the whole condition, and kept whole otherwise, as + // one scoring `must` clause. + val whole = + innerFilters.size == 1 && mustFilters.isEmpty && notFilters.isEmpty && shouldFilters.isEmpty innerFilters.reverse.map { + case b: ElasticBoolQuery if b.matchCriteria && b.shouldFilters.nonEmpty && !whole => + query.must(b) case b: ElasticBoolQuery if b.matchCriteria => b.innerFilters.reverse.foreach(query.must) b.mustFilters.reverse.foreach(query.must) @@ -549,7 +614,17 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { def notAsString: String = maybeNot.map(v => s"$v ").getOrElse("") def valueAsString: String = maybeValue.map(v => s" $v").getOrElse("") - override def sql = s"$identifier $notAsString$operator$valueAsString" + + /** A `NOT` on `=`, `<>`, `!=`, `<`, `<=`, `>`, `>=` is written BEFORE the column (`NOT c = 1`), + * the only place the grammar reads it (`WhereParser.equality` / `comparison`): `c NOT = 1` does + * not parse. LIKE and RLIKE -- `ComparisonOperator`s too -- read theirs AFTER the column only + * (`c NOT LIKE 'x%'`), so the operator is matched by NAME, never by its trait. + */ + override def sql: String = operator match { + case EQ | NE | DIFF | GE | GT | LE | LT if maybeNot.isDefined => + s"$NOT $identifier $operator$valueAsString" + case _ => s"$identifier $notAsString$operator$valueAsString" + } override lazy val dependencies: Seq[Identifier] = maybeValue match { @@ -1718,6 +1793,13 @@ object ConditionalFunctionAsCriteria { case class IsNullCriteria(identifier: Identifier) extends CriteriaWithConditionalFunction[SQLAny] { override val conditionalFunction: ConditionalFunction[SQLAny] = IsNull(identifier) override val operator: Operator = IS_NULL + + /** `NOT ISNULL(x)` is `ISNOTNULL(x)`, exactly as `NOT x IS NULL` is `x IS NOT NULL`: the function + * forms emit the same `exists` query and the same `== null` / `!= null` test as the operator + * forms, which carry their negation the same way. + */ + override def negated: Option[Criteria] = Some(IsNotNullCriteria(identifier)) + override def update(request: SingleSearch): Criteria = { val updated = this.copy(identifier = identifier.update(request)) if (updated.nested) { @@ -1757,6 +1839,10 @@ case class IsNotNullCriteria(identifier: Identifier) identifier ) override val operator: Operator = IS_NOT_NULL + + /** `NOT ISNOTNULL(x)` is `ISNULL(x)` -- see [[IsNullCriteria.negated]]. */ + override def negated: Option[Criteria] = Some(IsNullCriteria(identifier)) + override def update(request: SingleSearch): Criteria = { val updated = this.copy(identifier = identifier.update(request)) if (updated.nested) { 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 106da279..bd19f889 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 @@ -1425,12 +1425,81 @@ package object query { } having.flatMap(_.criteria).flatMap { criteria => buckets.view + .filterNot(bucket => keyEqualitiesAnsweredExactly(criteria, bucket)) .flatMap(bucket => conflict(criteria, bucket, not = false)) .headOption .map { case (_, reason) => reason } } } + /** Rule (b2)'s one exception: a `HAVING` whose every condition compares ONE key with values + * (`=`, `<>`, `!=`, `IN`, each possibly negated), joined by AND and OR together, and whose two + * lists keep exactly the buckets SQL's reading keeps. The per-node walk of + * [[keyChannelConflict]] refuses such a combination as soon as a removed value sits under an + * OR, yet `k = 'v1' OR k = 'v2' AND NOT k = 'v3'` -- `k = 'v1' OR (k = 'v2' AND k <> 'v3')` -- + * keeps `v1` and `v2` through `include:["v1","v2"]` and `exclude:["v3"]`, which is SQL's + * answer. + * + * Decided EXACTLY, never by shape. A bucket holds ONE key value, so the condition's truth + * depends only on which of the named values the key is, if any: each named value, plus one + * value none of the conditions names, is evaluated both ways -- the tree as SQL reads it, and + * the two lists as Elasticsearch applies them (kept if the include list is empty or holds it, + * and the exclude list does not). The values and the sense of each condition are those + * `Criteria.includes` / `excludes` give -- the functions the emission calls -- so the lists + * judged are the lists emitted. A rule on the shape could not do: `k = 'v3' OR k = 'v2' AND + * NOT k = 'v3'` has the very shape of the example above, and its lists (`include:["v3","v2"]`, + * `exclude:["v3"]`) drop `v3`, which SQL keeps -- so it stays refused. + */ + private[query] def keyEqualitiesAnsweredExactly(criteria: Criteria, bucket: Bucket): Boolean = { + val empty = BucketIncludesExcludes() + // A condition's truth for a key value, `None` standing for a value no condition names; `None` + // for the whole tree when one condition is not such an equality of this key. + def truth(c: Criteria): Option[Option[String] => Boolean] = c match { + case p @ Predicate(left, op, right, _, _) => + // the predicate's NOT, on its RIGHT operand, as the lists themselves read it + val negatedRight = p.includePolarityOfRight(not = false) + for { l <- truth(left); r <- truth(right) } yield { (key: Option[String]) => + val rightHolds = r(key) != negatedRight + if (op == AND) l(key) && rightHolds else l(key) || rightHolds + } + case e: Expression if e.identifier.functions.isEmpty => + (e.includes(bucket, not = false, empty), e.excludes(bucket, not = false, empty)) match { + case (kept, BucketIncludesExcludes(removed, None)) + if kept == empty && removed.nonEmpty => + Some((key: Option[String]) => !key.exists(removed.contains)) + case (BucketIncludesExcludes(values, None), removed) + if removed == empty && values.nonEmpty => + Some((key: Option[String]) => key.exists(values.contains)) + case _ => None + } + case _ => None + } + // the operators joining the conditions, AND as `true` + def joins(c: Criteria): List[Boolean] = c match { + case Predicate(left, op, right, _, _) => (op == AND) :: joins(left) ::: joins(right) + case _ => Nil + } + def named(c: Criteria): Set[String] = c match { + case Predicate(left, _, right, _, _) => named(left) ++ named(right) + case e: Expression => + e.includes(bucket, not = false, empty).values ++ e + .excludes(bucket, not = false, empty) + .values + case _ => Set.empty + } + val ops = joins(criteria) + ops.contains(true) && ops.contains(false) && truth(criteria).exists { holds => + val kept = criteria.includes(bucket, not = false, empty) + val removed = criteria.excludes(bucket, not = false, empty) + def emitted(key: Option[String]): Boolean = key match { + case Some(v) => (kept.values.isEmpty || kept.values.contains(v)) && !removed.values(v) + case None => kept.values.isEmpty + } + kept.regex.isEmpty && removed.regex.isEmpty && + (named(criteria).map(Option(_)) + None).forall(key => emitted(key) == holds(key)) + } + } + /** Does this key predicate read a NESTED bucket? Asked of the bucket the leaf RESOLVES to. */ private[query] def readsNestedKey(e: Expression): Boolean = e.nested || namedLeavesOf(e).exists(l => keyBucketOf(l).exists(_.nestedElement.isDefined)) @@ -1861,6 +1930,10 @@ package object query { // answer: the include list means `a OR b`, the SQL means no bucket at all. // `HAVING city = 'a' OR city = 'b'` -> `include:["a","b"]`, CORRECT. // + // One exception, decided exactly and never by shape: equalities of one key joined by + // AND and OR together, when the two lists keep the buckets SQL keeps + // (`keyEqualitiesAnsweredExactly`). + // // 🔴 This rule is ONE derivation that asks BOTH methods about BOTH sides. The defect it // replaces (a round-5 regression, reverted with the rest of that work) was a rule // written for the include sense only and applied by a method that is also the exclude diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPopulation.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPopulation.scala new file mode 100644 index 00000000..d1ed9c8f --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPopulation.scala @@ -0,0 +1,374 @@ +package app.softnetwork.elastic.sql.parser + +import app.softnetwork.elastic.sql.function.cond.Case +import app.softnetwork.elastic.sql.operator.{AND, OR} +import app.softnetwork.elastic.sql.query._ + +import scala.collection.mutable + +/** The derived population of conditions that combine AND and OR, and its oracles. + * + * Every statement is GENERATED from a structure -- an operator sequence, a laminar family of + * parenthesised intervals, the negated leaves, the single-leaf parentheses and the leaf kinds -- + * and its meaning is computed from that structure ONLY, never from the engine: [[ansi]] is SQL's + * reading (`NOT` > `AND` > `OR`, left-associative, parentheses respected) and [[leftFold]] is kept + * to report what moved. A tree built by the engine is compared by its FULL truth table: a leaf is + * one boolean variable, true iff its bit is set. + */ +object ConditionPopulation { + + // ------------------------------------------------------------ formulas over leaves 1..n + + sealed trait F + final case class L(i: Int) extends F + final case class N(f: F) extends F + final case class A(l: F, r: F) extends F + final case class O(l: F, r: F) extends F + + def eval(f: F, bits: Int): Boolean = f match { + case L(i) => ((bits >> (i - 1)) & 1) == 1 + case N(g) => !eval(g, bits) + case A(l, r) => eval(l, bits) && eval(r, bits) + case O(l, r) => eval(l, bits) || eval(r, bits) + } + + def table(f: F, n: Int): Vector[Boolean] = (0 until (1 << n)).map(eval(f, _)).toVector + + // ------------------------------------------------------------ the WRITTEN form + + sealed trait W + final case class WLeaf(i: Int, neg: Boolean) extends W + final case class WGroup(items: List[W], ands: List[Boolean], paren: Boolean) extends W + + /** SQL's reading: split at every OR into runs joined by AND, fold each run, fold the runs. */ + def ansi(w: W): F = w match { + case WLeaf(i, neg) => if (neg) N(L(i)) else L(i) + case WGroup(items, ands, _) => + val fs = items.map(ansi) + val runs = mutable.ListBuffer[List[F]]() + var current = List(fs.head) + fs.tail.zip(ands).foreach { case (f, isAnd) => + if (isAnd) current = current :+ f + else { runs += current; current = List(f) } + } + runs += current + runs.toList.map(_.reduceLeft((x, y) => A(x, y): F)).reduceLeft((x, y) => O(x, y): F) + } + + def leftFold(w: W): F = w match { + case WLeaf(i, neg) => if (neg) N(L(i)) else L(i) + case WGroup(items, ands, _) => + val fs = items.map(leftFold) + fs.tail.zip(ands).foldLeft(fs.head) { case (acc, (f, isAnd)) => + if (isAnd) A(acc, f) else O(acc, f) + } + } + + def render(w: W, leaf: (Int, Boolean) => String): String = w match { + case WLeaf(i, neg) => leaf(i, neg) + case WGroup(items, ands, paren) => + val sb = new StringBuilder(render(items.head, leaf)) + items.tail.zip(ands).foreach { case (item, isAnd) => + sb.append(if (isAnd) " AND " else " OR ").append(render(item, leaf)) + } + if (paren) s"(${sb.toString})" else sb.toString + } + + /** Leaves `lo..hi`; `ands(k - 1)` joins leaf k and leaf k + 1; `fam` holds the parenthesised + * intervals (laminar), `singles` the leaves written in their own parentheses. + */ + def build( + lo: Int, + hi: Int, + ands: Vector[Boolean], + fam: Set[(Int, Int)], + singles: Set[Int], + negs: Set[Int], + paren: Boolean + ): WGroup = { + val inside = fam.filter { case (a, b) => a >= lo && b <= hi && (a, b) != ((lo, hi)) } + val maximal = inside.filter { case (a, b) => + !inside.exists { case (c, d) => (c, d) != ((a, b)) && c <= a && b <= d } + } + val items = mutable.ListBuffer[W]() + val itemAnds = mutable.ListBuffer[Boolean]() + var p = lo + while (p <= hi) { + val (item, end) = maximal.find(_._1 == p) match { + case Some((a, b)) => (build(a, b, ands, fam, singles, negs, paren = true), b) + case None => + val leaf = WLeaf(p, negs.contains(p)) + (if (singles.contains(p)) WGroup(List(leaf), Nil, paren = true) else leaf, p) + } + if (items.nonEmpty) itemAnds += ands(p - 2) + items += item + p = end + 1 + } + WGroup(items.toList, itemAnds.toList, paren) + } + + def intervals(n: Int): List[(Int, Int)] = + for { a <- (1 to n).toList; b <- (a + 1 to n).toList } yield (a, b) + + def laminar(s: List[(Int, Int)]): Boolean = s.forall { case (a, b) => + s.forall { case (c, d) => + (a, b) == ((c, d)) || b < c || d < a || (a <= c && d <= b) || (c <= a && b <= d) + } + } + + /** Every laminar family of parenthesised intervals of 2+ leaves, the whole clause included. */ + def families(n: Int): List[Set[(Int, Int)]] = { + val iv = intervals(n) + (0 until (1 << iv.size)).toList + .map(m => iv.zipWithIndex.collect { case (x, k) if ((m >> k) & 1) == 1 => x }) + .filter(laminar) + .map(_.toSet) + } + + def opSeqs(n: Int): List[Vector[Boolean]] = + (0 until (1 << (n - 1))).toList.map(m => (0 until n - 1).map(k => ((m >> k) & 1) == 1).toVector) + + def subsets(n: Int): List[Set[Int]] = + (0 until (1 << n)).toList.map(m => (1 to n).filter(i => ((m >> (i - 1)) & 1) == 1).toSet) + + // ------------------------------------------------------------ clauses + + sealed abstract class Clause(val name: String) + case object WhereC extends Clause("WHERE") + case object HavingKeysC extends Clause("HAVING on several keys") + case object HavingKeyC extends Clause("HAVING on one key") + case object HavingAggC extends Clause("HAVING on aggregates") + case object OnC extends Clause("JOIN ON") + case object CaseC extends Clause("CASE WHEN") + + val Clauses: List[Clause] = List(WhereC, HavingKeysC, HavingKeyC, HavingAggC, OnC, CaseC) + + /** Leaf kinds of WHERE / CASE WHEN: 0 `=`, 1 LIKE, 2 IN, 3 BETWEEN, 4 IS NULL, 5 `>`; each with + * its own spelling of NOT. + */ + def leafText(clause: Clause, kindOf: Int => Int)(i: Int, neg: Boolean): String = clause match { + case WhereC | CaseC => + kindOf(i) match { + case 0 => if (neg) s"NOT c$i = 1" else s"c$i = 1" + case 1 => if (neg) s"c$i NOT LIKE 'x%'" else s"c$i LIKE 'x%'" + case 2 => if (neg) s"c$i NOT IN (1, 2)" else s"c$i IN (1, 2)" + case 3 => if (neg) s"c$i NOT BETWEEN 1 AND 2" else s"c$i BETWEEN 1 AND 2" + case 4 => if (neg) s"c$i IS NOT NULL" else s"c$i IS NULL" + case _ => if (neg) s"NOT c$i > 1" else s"c$i > 1" + } + case HavingKeysC => if (neg) s"NOT c$i = 'v$i'" else s"c$i = 'v$i'" + case HavingKeyC => if (neg) s"NOT k = 'v$i'" else s"k = 'v$i'" + case HavingAggC => if (neg) s"NOT MAX(c$i) > 1" else s"MAX(c$i) > 1" + case OnC => if (neg) s"NOT t.c$i = u.c$i" else s"t.c$i = u.c$i" + } + + def statement(clause: Clause, n: Int, cond: String): String = clause match { + case WhereC => s"SELECT id FROM t WHERE $cond" + case HavingKeysC => + val keys = (1 to n).map(i => s"c$i").mkString(", ") + s"SELECT $keys, COUNT(*) AS cnt FROM t GROUP BY $keys HAVING $cond" + case HavingKeyC => s"SELECT k, COUNT(*) AS cnt FROM t GROUP BY k HAVING $cond" + case HavingAggC => s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING $cond" + case OnC => s"SELECT t.id FROM t JOIN u ON $cond" + case CaseC => s"SELECT CASE WHEN $cond THEN 1 ELSE 0 END AS x FROM t" + } + + def criteriaOf(clause: Clause, st: Statement): Option[Criteria] = st match { + case s: SingleSearch => + clause match { + case WhereC => s.where.flatMap(_.criteria) + case HavingKeysC | HavingKeyC | HavingAggC => s.having.flatMap(_.criteria) + case OnC => + s.from.joins.collectFirst { case j: StandardJoin => j.on }.flatten.map(_.criteria) + case CaseC => + s.select.fields.headOption + .flatMap(_.identifier.functions.collectFirst { case c: Case => c }) + .flatMap(_.conditions.headOption) + .map(_._1) + .collect { case c: Criteria => c } + } + case _ => None + } + + // ------------------------------------------------------------ one generated statement + + final case class Gen(clause: Clause, cls: String, n: Int, w: W, kindOf: Int => Int) { + lazy val cond: String = render(w, leafText(clause, kindOf)) + lazy val sql: String = statement(clause, n, cond) + lazy val ansiTable: Vector[Boolean] = table(ansi(w), n) + lazy val leftTable: Vector[Boolean] = table(leftFold(w), n) + + /** Some parenthesis level holds both AND and OR, unparenthesised. */ + lazy val mixedLevel: Boolean = { + def any(x: W): Boolean = x match { + case WGroup(items, ands, _) => + (ands.contains(true) && ands.contains(false)) || items.exists(any) + case _ => false + } + any(w) + } + + /** One operator in the whole condition. */ + lazy val oneOperator: Boolean = { + def ops(x: W): List[Boolean] = x match { + case WGroup(items, ands, _) => ands ++ items.flatMap(ops) + case _ => Nil + } + ops(w).distinct.size <= 1 + } + } + + /** P1. Counts are asserted from this generator by the specs, never typed by hand. */ + def population(): List[Gen] = { + val out = mutable.ListBuffer[Gen]() + val k0: Int => Int = _ => 0 + // P: every parenthesisation x every operator sequence, NOT-free, `=` leaves + for { + clause <- Clauses + n <- if (clause == WhereC) List(3, 4, 5) else List(3, 4) + fam <- families(n) + ands <- opSeqs(n) + } out += Gen( + clause, + "P", + n, + build(1, n, ands, fam, Set.empty, Set.empty, fam.contains((1, n))), + k0 + ) + // N: NOT on any non-empty leaf subset; 3 leaves every parenthesisation, 4 leaves flat + for { + clause <- Clauses + n <- List(3, 4) + fam <- if (n == 3) families(3) else List(Set.empty[(Int, Int)]) + ands <- opSeqs(n) + negs <- subsets(n) if negs.nonEmpty + } out += Gen(clause, "N", n, build(1, n, ands, fam, Set.empty, negs, fam.contains((1, n))), k0) + // S: single-leaf parentheses, flat otherwise + for { + clause <- List(WhereC, HavingAggC, CaseC) + n <- List(3, 4) + singles <- subsets(n) if singles.nonEmpty + ands <- opSeqs(n) + } out += Gen( + clause, + "S", + n, + build(1, n, ands, Set.empty, singles, Set.empty, paren = false), + k0 + ) + // K: the six leaf kinds, rotated, every parenthesisation, with and without the kind's NOT + for { + n <- List(3, 4) + fam <- families(n) + ands <- opSeqs(n) + rot <- 0 until 6 + negAll <- List(false, true) + } { + val negs = if (negAll) Set(2) else Set.empty[Int] + out += Gen( + WhereC, + "K", + n, + build(1, n, ands, fam, Set.empty, negs, fam.contains((1, n))), + i => (i + rot) % 6 + ) + } + out.toList + } + + // ------------------------------------------------------------ the engine's tree as a formula + + private val ColumnIndex = """\bc(\d+)\b""".r + private val ValueIndex = """'v(\d+)'""".r + + /** A leaf's index: its column (`c3`), or -- HAVING on one key, where every leaf names `k` -- its + * value (`'v3'`). + */ + def leafIndex(clause: Clause, e: Expression): Option[Int] = + if (clause == HavingKeyC) ValueIndex.findFirstMatchIn(e.sql).map(_.group(1).toInt) + else ColumnIndex.findFirstMatchIn(e.identifier.sql).map(_.group(1).toInt) + + /** The engine's tree as a formula. `Predicate.not` negates the RIGHT operand. */ + def formulaOf(c: Criteria, clause: Clause): Either[String, F] = c match { + case p: Predicate => + for { + l <- formulaOf(p.leftCriteria, clause) + r <- formulaOf(p.rightCriteria, clause) + } yield { + val right = if (p.not.isDefined) N(r) else r + p.operator match { + case AND => A(l, right) + case OR => O(l, right) + } + } + case r: ElasticRelation => formulaOf(r.criteria, clause) + case e: Expression => + leafIndex(clause, e) match { + case Some(i) => + val negated = e match { + case _: IsNotNullExpr | _: IsNotNullCriteria => true + case _: IsNullExpr | _: IsNullCriteria => false + case other => other.maybeNot.isDefined + } + Right(if (negated) N(L(i)) else L(i)) + case None => Left(s"leaf not indexable: ${e.sql}") + } + case other => Left(s"unhandled ${other.getClass.getSimpleName}: ${other.sql}") + } + + // ------------------------------------------------------------ Painless precedence + + /** Evaluates a `bucket_selector` script of `MAX(ci) > 0` leaves (a negated leaf renders `<= 0`) + * with Painless's own precedence: `!` > `&&` > `||`, parentheses respected. + */ + def scriptTruth(script: String, bits: Int): Boolean = { + val leaf = """\((?:[^()]|\([^()]*\))*?params\.[A-Za-z_]*?c(\d+)(?:[^()]|\([^()]*\))*?\)""".r + var s = script + var more = true + while (more) { + leaf.findFirstMatchIn(s) match { + case Some(m) => + val bit = ((bits >> (m.group(1).toInt - 1)) & 1) == 1 + val truth = if (m.matched.contains("<= 0")) !bit else bit + s = s.substring(0, m.start) + (if (truth) "T" else "F") + s.substring(m.end) + case None => more = false + } + } + val tokens = mutable.Queue[String]() + var k = 0 + while (k < s.length) { + s.charAt(k) match { + case ' ' => k += 1 + case 'T' | 'F' | '(' | ')' | '!' => tokens += s.charAt(k).toString; k += 1 + case '&' if s.startsWith("&&", k) => tokens += "&&"; k += 2 + case '|' if s.startsWith("||", k) => tokens += "||"; k += 2 + case other => throw new IllegalArgumentException(s"'$other' in reduced script $s ($script)") + } + } + def or(): Boolean = { + var v = and() + while (tokens.headOption.contains("||")) { tokens.dequeue(); val r = and(); v = v || r } + v + } + def and(): Boolean = { + var v = not() + while (tokens.headOption.contains("&&")) { tokens.dequeue(); val r = not(); v = v && r } + v + } + def not(): Boolean = + if (tokens.headOption.contains("!")) { tokens.dequeue(); !not() } + else atom() + def atom(): Boolean = tokens.dequeue() match { + case "T" => true + case "F" => false + case "(" => + val v = or() + require(tokens.dequeue() == ")", s"unbalanced: $s") + v + case t => throw new IllegalArgumentException(s"token $t in $s") + } + val result = or() + require(tokens.isEmpty, s"trailing tokens in $s") + result + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPrecedenceSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPrecedenceSpec.scala new file mode 100644 index 00000000..081f7b24 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ConditionPrecedenceSpec.scala @@ -0,0 +1,244 @@ +package app.softnetwork.elastic.sql.parser + +import app.softnetwork.elastic.sql.operator.{AND, OR} +import app.softnetwork.elastic.sql.parser.ConditionPopulation._ +import app.softnetwork.elastic.sql.query._ +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** A condition that combines AND and OR is reduced with SQL precedence -- `NOT` > `AND` > `OR`, + * left-associative, parentheses respected -- in every clause that takes one, and the SQL the + * engine renders back re-parses to the very same tree. + * + * 🔴 The evidence is the DERIVED population of `ConditionPopulation` against an oracle computed + * from the generated structure only. Every test COLLECTS its wrong statements and asserts the list + * once: a failing `foreach` would stop at the first one and hide how many there are. + */ +class ConditionPrecedenceSpec extends AnyFlatSpec with Matchers { + + private lazy val population: List[Gen] = ConditionPopulation.population() + + private def treeOf(clause: Clause, sql: String): Criteria = + Parser.parseUnvalidated(sql) match { + case Right(st) => criteriaOf(clause, st).getOrElse(fail(s"[$sql] carries no condition")) + case Left(e) => fail(s"[$sql] does not parse: ${e.msg}") + } + + private def whereOf(sql: String): Criteria = treeOf(WhereC, sql) + + private def formula(clause: Clause, c: Criteria): F = + formulaOf(c, clause).fold(why => fail(why), identity) + + private def assertNoneWrong(wrong: Seq[String], of: Int): Unit = + withClue(s"${wrong.size} of $of wrong, first 20:\n${wrong.take(20).mkString("\n")}\n") { + wrong shouldBe empty + } + + "the oracle" should "read a OR b AND c as a OR (b AND c), a written group as written" in { + val w = build(1, 3, Vector(false, true), Set.empty, Set.empty, Set.empty, paren = false) + render(w, leafText(WhereC, _ => 0)) shouldBe "c1 = 1 OR c2 = 1 AND c3 = 1" + ansi(w) shouldBe O(L(1), A(L(2), L(3))) + leftFold(w) shouldBe A(O(L(1), L(2)), L(3)) + val g = build(1, 3, Vector(false, true), Set((1, 2)), Set.empty, Set.empty, paren = false) + render(g, leafText(WhereC, _ => 0)) shouldBe "(c1 = 1 OR c2 = 1) AND c3 = 1" + ansi(g) shouldBe A(O(L(1), L(2)), L(3)) + } + + "the population" should "cover every clause with mixes at one level and parenthesised mixes" in { + val empty = for { + clause <- Clauses + cell <- List("mixed level", "parenthesised mix", "one operator") + if !population.exists { g => + g.clause == clause && (cell match { + case "mixed level" => g.mixedLevel + case "parenthesised mix" => !g.mixedLevel && !g.oneOperator + case _ => g.oneOperator + }) + } + } yield s"${clause.name} / $cell" + empty shouldBe empty + } + + "every condition, in every clause" should "reduce to the tree SQL precedence means" in { + val wrong = population.flatMap { g => + Parser.parseUnvalidated(g.sql) match { + case Left(e) => Some(s"[${g.sql}] grammar rejected: ${e.msg}") + case Right(st) => + criteriaOf(g.clause, st) match { + case None => Some(s"[${g.sql}] carries no condition") + case Some(c) => + formulaOf(c, g.clause) match { + case Left(why) => Some(s"[${g.sql}] $why") + case Right(f) => + val built = table(f, g.n) + if (built == g.ansiTable) None + else if (built == g.leftTable) Some(s"[${g.sql}] built the LEFT FOLD") + else Some(s"[${g.sql}] built neither SQL's reading nor the left fold") + } + } + } + } + assertNoneWrong(wrong, population.size) + } + + "the SQL rendered back" should "re-parse to the same statement and render identically" in { + val wrong = population.flatMap { g => + Parser.parseUnvalidated(g.sql).toOption.flatMap { st => + val rendered = st.sql + Parser.parseUnvalidated(rendered) match { + case Left(e) => Some(s"[$rendered] does not re-parse: ${e.msg}") + case Right(again) if again != st => Some(s"[$rendered] re-parses to another AST") + case Right(again) if again.sql != rendered => Some(s"[$rendered] renders ${again.sql}") + case _ => None + } + } + } + assertNoneWrong(wrong, population.size) + } + + "a level after a parenthesised group" should "keep its operators where they were written" in { + // the reduction used to push the group without folding the pending `c1 OR`, then built the next + // predicate with the NEW operator and re-pushed the OLD one: (c1 AND (g)) OR c4 + formula( + WhereC, + whereOf("SELECT id FROM t WHERE c1 = 1 OR (c2 = 1 OR c3 = 1) AND c4 = 1") + ) shouldBe + O(L(1), A(O(L(2), L(3)), L(4))) + } + + "four conditions" should "reduce with precedence although the scanner pairs them left to right" in { + // the scanner hands `[c1 AND c2], AND, [c3 OR c4]` for the first one + Seq( + "c1 = 1 AND c2 = 1 AND c3 = 1 OR c4 = 1" -> O(A(A(L(1), L(2)), L(3)), L(4)), + "c1 = 1 OR c2 = 1 AND c3 = 1 OR c4 = 1" -> O(O(L(1), A(L(2), L(3))), L(4)), + "c1 = 1 AND c2 = 1 OR c3 = 1 AND c4 = 1" -> O(A(L(1), L(2)), A(L(3), L(4))), + "c1 = 1 OR c2 = 1 OR c3 = 1 AND c4 = 1" -> O(O(L(1), L(2)), A(L(3), L(4))) + ).foreach { case (cond, expected) => + withClue(s"[$cond] ") { + formula(WhereC, whereOf(s"SELECT id FROM t WHERE $cond")) shouldBe expected + } + } + } + + "a NOT heading conditions joined by AND after an OR" should + "be folded into its condition -- the criterion the grammar builds for the same text" in { + // (the NOT as written after OR, the spelling the grammar reads at the start of a condition) + val kinds = Seq( + "NOT c2 = 1" -> "NOT c2 = 1", + "NOT c2 > 1" -> "NOT c2 > 1", + "NOT c2 LIKE 'x%'" -> "c2 NOT LIKE 'x%'", + "NOT c2 RLIKE 'x.*'" -> "c2 NOT RLIKE 'x.*'", + "NOT c2 IN (1, 2)" -> "c2 NOT IN (1, 2)", + "NOT c2 BETWEEN 1 AND 2" -> "c2 NOT BETWEEN 1 AND 2", + "NOT c2 IS NULL" -> "c2 IS NOT NULL", + "NOT c2 IS NOT NULL" -> "c2 IS NULL", + "NOT EXISTS (SELECT 1 FROM u)" -> "NOT EXISTS (SELECT 1 FROM u)", + "NOT c2 IN (SELECT a FROM u)" -> "c2 NOT IN (SELECT a FROM u)", + "NOT ISNULL(c2)" -> "ISNOTNULL(c2)", + "NOT ISNOTNULL(c2)" -> "ISNULL(c2)" + ) + val wrong = kinds.flatMap { case (written, spelled) => + val sql = s"SELECT id FROM t WHERE c1 = 1 OR $written AND c3 = 1" + val expectedRun = whereOf(s"SELECT id FROM t WHERE $spelled AND c3 = 1") + Parser(sql) match { + case Left(e) => Some(s"[$sql] refused: ${e.msg}") + case Right(st) => + whereOf(sql) match { + case Predicate(_, OR, run, None, false) if run == expectedRun => + if (Parser(st.sql) == Right(st)) None + else Some(s"[$sql] renders ${st.sql}, which does not re-parse to the same statement") + case other => Some(s"[$sql] built $other, expected c1 OR ($expectedRun)") + } + } + } + assertNoneWrong(wrong, kinds.size) + } + + it should "be refused by name where the condition cannot carry its own NOT" in { + // (written as the message renders them: a MATCH over several columns renders `(c2,c4)`) + val refused = Seq("MATCH (c2) AGAINST ('q')", "MATCH (c2,c4) AGAINST ('q')") + val wrong = refused.flatMap { c => + val sql = s"SELECT id FROM t WHERE c1 = 1 OR NOT $c AND c3 = 1" + // the remedy the message gives, written as it says + val remedy = s"SELECT id FROM t WHERE c1 = 1 OR (c3 = 1 AND NOT $c)" + (Parser(sql), Parser(remedy)) match { + case (Left(e), Right(_)) + if e.msg.startsWith(s"NOT $c cannot start conditions joined by AND after an OR") && + e.msg.contains(s"for example A OR (B AND NOT $c)") => + None + case (verdict, remedyVerdict) => + Some(s"[$sql] got $verdict; its remedy [$remedy] got $remedyVerdict") + } + } + assertNoneWrong(wrong, refused.size) + } + + "a condition the reducer cannot use" should "keep the message it had" in { + Seq( + "SELECT id FROM t WHERE c1 = 1 AND" -> "WHERE clause requires criteria", + "SELECT id FROM t WHERE c1 = 1 OR c2 = 1 AND" -> "WHERE clause requires criteria", + "SELECT id FROM t WHERE AND c1 = 1" -> "Invalid stack state for predicate creation", + "SELECT id FROM t WHERE (c1 = 1" -> "Unbalanced parentheses", + "SELECT id FROM t WHERE c1 = 1 AND ()" -> "Empty sub-expression", + "SELECT g, COUNT(*) AS n FROM t GROUP BY g HAVING MAX(c1) > 1 OR" -> "HAVING clause requires criteria", + "SELECT t.id FROM t JOIN u ON t.a = u.a AND" -> "ON clause requires criteria", + "SELECT id FROM t WHERE child(child.a = 1 AND)" -> "CHILD clause requires criteria" + ).foreach { case (sql, message) => + withClue(s"[$sql] ") { + Parser(sql) match { + case Left(e) => e.msg should include(message) + case Right(s) => fail(s"accepted as ${s.sql}") + } + } + } + } + + "a relation body that mixes AND and OR" should "reduce to the tree the same condition has at top level" in { + Seq( + "child.a = 1 OR child.b = 2 AND child.c = 3", + "child.a = 1 AND child.b = 2 OR child.c = 3 AND child.d = 4", + "child.a = 1 OR NOT child.b = 2 AND child.c = 3", + "child.a = 1 OR (child.b = 2 OR child.c = 3) AND child.d = 4" + ).foreach { shape => + val top = whereOf(s"SELECT * FROM t WHERE $shape") + whereOf(s"SELECT * FROM t WHERE child($shape)") match { + case r: ElasticRelation => withClue(s"[$shape] ")(r.criteria shouldBe top) + case other => fail(s"[$shape] built $other") + } + } + } + + "a tree rebuilt in code" should "render parentheses wherever an OR sits under an AND" in { + val a = whereOf("SELECT id FROM t WHERE a = 1") + val b = whereOf("SELECT id FROM t WHERE b = 1") + val c = whereOf("SELECT id FROM t WHERE c = 1") + val rows = Seq[(Criteria, String)]( + Predicate(a, AND, Predicate(b, OR, c)) -> "a = 1 AND (b = 1 OR c = 1)", + Predicate(Predicate(a, OR, b), AND, c) -> "(a = 1 OR b = 1) AND c = 1", + Predicate(a, OR, Predicate(b, AND, c)) -> "a = 1 OR b = 1 AND c = 1", + // the transparent wrapper `update` puts around an all-nested operand renders bare + Predicate(a, AND, ElasticNested(Predicate(b, OR, c), None)) -> "a = 1 AND (b = 1 OR c = 1)", + // a written relation renders its own parentheses + Predicate(a, AND, ElasticNested(Predicate(b, OR, c), None, fromCriteria = false)) -> + "a = 1 AND NESTED(b = 1 OR c = 1)" + ) + val wrong = rows.flatMap { case (tree, expected) => + if (tree.sql != expected) Some(s"rendered [${tree.sql}], expected [$expected]") + else { + // same meaning once parsed again (the leaves a, b, c are 3 variables) + def f(x: Criteria): F = x match { + case p: Predicate => + val r = if (p.not.isDefined) N(f(p.rightCriteria)) else f(p.rightCriteria) + if (p.operator == AND) A(f(p.leftCriteria), r) else O(f(p.leftCriteria), r) + case r: ElasticRelation => f(r.criteria) + case e: Expression => L(Seq("a", "b", "c").indexOf(e.identifier.name) + 1) + case other => fail(s"unexpected $other") + } + val again = whereOf(s"SELECT id FROM t WHERE ${tree.sql}") + if (table(f(again), 3) == table(f(tree), 3)) None + else Some(s"[${tree.sql}] re-parses to another meaning") + } + } + assertNoneWrong(wrong, rows.size) + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserTotalitySpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserTotalitySpec.scala index d55f09d2..1be6aa90 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserTotalitySpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ParserTotalitySpec.scala @@ -106,7 +106,7 @@ class ParserTotalitySpec extends AnyFlatSpec with Matchers { rejects("SELECT a FROM t JOIN u ON ()", "Empty sub-expression") } - // --- site 1: processTokensHelper's invalid stack ----------------------------------------- + // --- site 1: the reducer's (`processTokens`) invalid stack -------------------------------- it should "reject a leading predicate operator instead of throwing" in { rejects("SELECT a FROM t WHERE AND a = 1", "Invalid stack state for predicate creation") @@ -154,7 +154,7 @@ class ParserTotalitySpec extends AnyFlatSpec with Matchers { // --- OQ-6: the stray closing parenthesis, which used to be swallowed ---------------------- // MEASURED on the unmodified tree: `SELECT a FROM t WHERE a = 1)` parsed as `... WHERE a = 1`, - // the `)` consumed by `whereCriteria` and then ignored by processTokensHelper's EndDelimiter + // the `)` consumed by `whereCriteria` and then ignored by the reducer's EndDelimiter // arm. A closing delimiter reaching that scan is unmatched by construction - a balanced group is // consumed whole by `extractSubTokens` - so rejecting it cannot lose a valid statement. // @@ -300,20 +300,20 @@ class ParserTotalitySpec extends AnyFlatSpec with Matchers { } } - // Every AST `.sql` must re-parse to an EQUAL AST (project_ast_render_roundtrip_family). - // ⚠️ NOT is deliberately absent here: `NOT c = 3` renders as `c NOT = 3`, which does NOT re-parse. - // That render defect is PRE-EXISTING and identical at top level (`WHERE NOT a = 1` renders - // `a NOT = 1` on `main` too), but 3-criteria relations were rejected before, so this commit makes - // it newly REACHABLE inside a relation. Recorded in - // docs/issues/local-21.4-not-render-asymmetry.md; deliberately not pinned, because pinning a - // broken round-trip reads as a contract. + // Every AST `.sql` must re-parse to an EQUAL AST (project_ast_render_roundtrip_family) -- a NOT + // included: `NOT c = 3` renders `NOT c = 3`, the only place the grammar reads that NOT. it should "round-trip an N-ary relation predicate through its own render" in { Seq( "SELECT * FROM Table WHERE child(child.a = 2 AND child.b = 3 AND child.c = 4)", "SELECT * FROM Table WHERE child(child.x = 1 OR child.y = 2 OR child.z = 3)", "SELECT * FROM Table WHERE child(child.a = 1 AND (child.b = 2 OR child.c = 3))", "SELECT * FROM Table WHERE parent(parent.a = 1 AND parent.b = 2 AND parent.c = 3)", - "SELECT * FROM Table WHERE nested nested.a = 1" + "SELECT * FROM Table WHERE nested nested.a = 1", + "SELECT * FROM Table WHERE NOT a = 1", + "SELECT * FROM Table WHERE a = 1 AND NOT b = 2", + "SELECT * FROM Table WHERE a = 1 OR NOT b = 2 AND c = 3", + "SELECT * FROM Table WHERE child(NOT child.a = 1 AND child.b = 2)", + "SELECT * FROM Table WHERE child(child.a = 1 OR NOT child.b = 2 AND child.c = 3)" ).foreach { sql => val parsed = Parser(sql) withClue(s"[$sql] ") { parsed.isRight shouldBe true } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index b66720a6..c268b269 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -744,6 +744,135 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt Parser(group + "NOT (status = 'x' OR status = 'y')").isLeft shouldBe true } + /** A condition comparing the key with values: its text, the values it names, and its sense -- + * `true` when it holds for a key that IS one of them, `false` when it holds for a key that is + * NONE of them. + */ + private val keyEqualities: Seq[(String, Set[String], Boolean)] = Seq( + ("status = 'a'", Set("a"), true), + ("status = 'b'", Set("b"), true), + ("NOT status = 'a'", Set("a"), false), + ("status <> 'b'", Set("b"), false), + ("status IN ('a','c')", Set("a", "c"), true), + ("status NOT IN ('b','c')", Set("b", "c"), false) + ) + + "equalities of one key joined by AND and OR together" should + "be accepted exactly when the two lists keep the buckets SQL's reading keeps" in { + // The verdict is DERIVED from two readings of each condition, compared on every key value that + // matters -- each value a condition names, and one no condition names (`None`): + // - SQL's reading: AND before OR, left to right, a group as written; + // - the terms channel's: a bucket is kept when the kept list is empty or holds its key, and + // the removed list does not. Every condition that holds for its values feeds the kept list, + // every one that holds for any other value feeds the removed list, whatever joins them. + // The two agree -> accepted; they disagree on one value -> refused by (b2). + type Holds = Option[String] => Boolean + def leaf(values: Set[String], sense: Boolean): Holds = key => key.exists(values) == sense + def chain(items: Seq[Holds], ands: Seq[Boolean]): Holds = key => { + val runs = ands.zip(items.tail).foldLeft(List(List(items.head))) { + case (run :: done, (true, item)) => (item :: run) :: done + case (runs, (_, item)) => List(item) :: runs + } + runs.exists(_.forall(_(key))) + } + def op(and: Boolean): String = if (and) " AND " else " OR " + val mixed2 = Seq(Seq(true, false), Seq(false, true)) + val mixed3 = for { + a <- Seq(true, false); b <- Seq(true, false); c <- Seq(true, false) + if Set(a, b, c).size == 2 + } yield Seq(a, b, c) + val statements: Seq[(String, Holds, Seq[(Set[String], Boolean)])] = + (for { + a <- keyEqualities; b <- keyEqualities; c <- keyEqualities + ands <- mixed2 + form <- Seq("flat", "(ab)c", "a(bc)") + } yield { + val h = Seq(a, b, c).map { case (_, v, s) => leaf(v, s) } + val (o1, o2) = (ands.head, ands(1)) + val (text, holds) = form match { + case "flat" => (a._1 + op(o1) + b._1 + op(o2) + c._1, chain(h, ands)) + case "(ab)c" => + ( + s"(${a._1}${op(o1)}${b._1})${op(o2)}${c._1}", + chain(Seq(chain(h.take(2), Seq(o1)), h(2)), Seq(o2)) + ) + case _ => + ( + s"${a._1}${op(o1)}(${b._1}${op(o2)}${c._1})", + chain(Seq(h.head, chain(h.drop(1), Seq(o2))), Seq(o1)) + ) + } + (text, holds, Seq(a, b, c).map(e => (e._2, e._3))) + }) ++ (for { + a <- keyEqualities; b <- keyEqualities; c <- keyEqualities; d <- keyEqualities + ands <- mixed3 + } yield { + val items = Seq(a, b, c, d) + val text = items.head._1 + ands.zip(items.tail).map { case (o, e) => op(o) + e._1 }.mkString + ( + text, + chain(items.map { case (_, v, s) => leaf(v, s) }, ands), + items.map(e => (e._2, e._3)) + ) + }) + def answeredExactly(holds: Holds, conditions: Seq[(Set[String], Boolean)]): Boolean = { + val kept = conditions.filter(_._2).flatMap(_._1).toSet + val removed = conditions.filterNot(_._2).flatMap(_._1).toSet + def emitted(key: Option[String]): Boolean = key match { + case Some(v) => (kept.isEmpty || kept(v)) && !removed(v) + case None => kept.isEmpty + } + (conditions.flatMap(_._1).map(Option(_)).toSet + None).forall(k => emitted(k) == holds(k)) + } + val wrong = statements.flatMap { case (condition, holds, conditions) => + val expected = answeredExactly(holds, conditions) + Parser(group + condition) match { + case Right(_) if expected => None + case Left(e) if !expected && e.msg.contains("HAVING cannot combine the conditions") => None + case verdict => Some(s"[$condition] expected accepted=$expected, got $verdict") + } + } + val accepted = statements.count { case (_, holds, conditions) => + answeredExactly(holds, conditions) + } + withClue(s"${wrong.size} of ${statements.size} wrong:\n${wrong.take(20).mkString("\n")}\n") { + wrong shouldBe empty + } + // non-vacuous both ways + accepted should be > 0 + accepted should be < statements.size + } + + it should "accept the one-key mixes the lists answer, with the lists they always had" in { + // SQL reads each as `v1 OR (v2 AND NOT v3)` and so on; the kept and removed lists below are + // what these statements emitted before AND bound tighter than OR, and they give SQL's answer. + val one = "SELECT k, COUNT(*) AS cnt FROM t GROUP BY k HAVING " + Seq( + ("k = 'v1' OR k = 'v2' AND NOT k = 'v3'", Set("v1", "v2"), Set("v3")), + ("(k = 'v1' OR k = 'v2' AND NOT k = 'v3')", Set("v1", "v2"), Set("v3")), + ("NOT k = 'v1' AND NOT k = 'v2' AND k = 'v3' OR k = 'v4'", Set("v3", "v4"), Set("v1", "v2")), + ("k = 'v1' OR k = 'v2' AND NOT k = 'v3' AND NOT k = 'v4'", Set("v1", "v2"), Set("v3", "v4")) + ).foreach { case (condition, kept, removed) => + withClue(s"[$condition] ") { + val st = parsed(one + condition) + val criteria = havingOf(st, condition) + st.buckets + .map(b => criteria.includes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(kept) + st.buckets + .map(b => criteria.excludes(b, not = false, BucketIncludesExcludes()).values) + .toSet shouldBe Set(removed) + } + } + } + + it should "refuse the same shape when its lists would drop a value SQL keeps" in { + // `v3 OR (v2 AND NOT v3)` keeps v2 and v3; `include:["v3","v2"]` with `exclude:["v3"]` keeps v2 + rejection( + "SELECT k, COUNT(*) AS cnt FROM t GROUP BY k HAVING k = 'v3' OR k = 'v2' AND NOT k = 'v3'" + ) should include("HAVING cannot combine the conditions") + } + // ------------------------------------------------------------------------------------------- // The OR rule -- homogeneity of MECHANISM (round 3) // ------------------------------------------------------------------------------------------- @@ -804,6 +933,25 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt ok.script shouldBe Some("(params.c == null ? false : (params.c > 1))") } + it should "keep a comparison whose text holds the no-filter placeholder" in { + // 🔴 `MAX(c1) = 1` renders `params.max_c1 == 1`, which holds `1 == 1` -- the text + // `metricSelector` answers when there is nothing to filter. Stripped out of the whole script, + // it left `params.max_c`; `= 10` left `params.max_c0`, and `IN (1, 2)` lost its first member. + Seq( + "MAX(c1) = 1" -> "(params.max_c1 == null ? false : (params.max_c1 == 1))", + "MAX(c1) = 10" -> "(params.max_c1 == null ? false : (params.max_c1 == 10))", + "MAX(c1) IN (1, 2)" -> "(params.max_c1 == null ? false : (params.max_c1 == 1 || params.max_c1 == 2))" + ).foreach { case (condition, script) => + withClue(s"[$condition] ") { + unvalidated( + s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING $condition" + ).having.get.script shouldBe Some(script) + } + } + // ... and nothing to filter is still no script at all + unvalidated(group + "status = 'a'").having.get.script shouldBe None + } + "a metric computed outside the nested grouping" should "be refused rather than vanish" in { // 🔴 The issue's SECOND conflation site. MEASURED on the branch before this rule: the statement // answered HTTP 200 with `{"aggs":{"max_amount":{"max":{"field":"amount"}}}}` -- no diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala new file mode 100644 index 00000000..bffc81a1 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala @@ -0,0 +1,106 @@ +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.parser.ConditionPopulation._ +import app.softnetwork.elastic.sql.parser.Parser +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import scala.collection.mutable + +/** The `bucket_selector` script of a HAVING over aggregates evaluates the condition's TREE. + * + * The script is evaluated here with Painless's own precedence (`!` > `&&` > `||`), which is + * exactly how Elasticsearch runs it, and compared with SQL's reading of the written condition. + * + * 🔴 Why both halves must hold together: a written group used to render its operands in + * parentheses but not itself, so Painless re-associated it -- and the unparenthesised mixes that + * came out right were right only because that flattening met a wrong tree. Correcting either half + * alone breaks the statements the other half was hiding. + */ +class MetricSelectorPrecedenceSpec extends AnyFlatSpec with Matchers { + + private def leaf(i: Int, neg: Boolean): String = if (neg) s"NOT MAX(c$i) > 0" else s"MAX(c$i) > 0" + + private final case class Row(label: String, n: Int, w: W) { + lazy val sql: String = s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING ${render(w, leaf)}" + lazy val expected: Vector[Boolean] = table(ansi(w), n) + lazy val mixedLevel: Boolean = { + def any(x: W): Boolean = x match { + case WGroup(items, ands, _) => + (ands.contains(true) && ands.contains(false)) || items.exists(any) + case _ => false + } + any(w) + } + lazy val flat: Boolean = w match { + case WGroup(items, _, false) => items.forall(_.isInstanceOf[WLeaf]) + case _ => false + } + } + + /** P3: derived. */ + private lazy val rows: List[Row] = { + val out = mutable.ListBuffer[Row]() + for { fam <- families(3); ands <- opSeqs(3) } out += Row( + "3, parenthesised", + 3, + build(1, 3, ands, fam, Set.empty, Set.empty, fam.contains((1, 3))) + ) + for { fam <- families(4); ands <- opSeqs(4) } out += Row( + "4, parenthesised", + 4, + build(1, 4, ands, fam, Set.empty, Set.empty, fam.contains((1, 4))) + ) + for { singles <- subsets(3) if singles.nonEmpty; ands <- opSeqs(3) } out += Row( + "3, single-leaf parentheses", + 3, + build(1, 3, ands, Set.empty, singles, Set.empty, paren = false) + ) + for { ands <- opSeqs(3); negs <- subsets(3) if negs.nonEmpty } out += Row( + "3, NOT", + 3, + build(1, 3, ands, Set.empty, Set.empty, negs, paren = false) + ) + out.toList + } + + private def scriptOf(sql: String): String = Parser(sql) match { + case Right(s: SingleSearch) => + MetricSelectorScript.metricSelector( + s.having.flatMap(_.criteria).getOrElse(fail(s"[$sql] no HAVING")) + ) + case other => fail(s"[$sql] $other") + } + + private def wrongAmong(subset: Seq[Row]): Seq[String] = subset.flatMap { r => + val script = scriptOf(r.sql) + val evaluated = (0 until (1 << r.n)).map(scriptTruth(script, _)).toVector + if (evaluated == r.expected) None else Some(s"[${r.sql}] -> $script") + } + + "a HAVING over aggregates" should "emit a script that evaluates SQL's reading, for every shape" in { + val wrong = wrongAmong(rows) + withClue( + s"${wrong.size} of ${rows.size} wrong, first 20:\n${wrong.take(20).mkString("\n")}\n" + ) { + wrong shouldBe empty + } + } + + it should "answer every unparenthesised mix -- those that used to be right by accident included" in { + val mixes = rows.filter(r => r.mixedLevel && r.flat) + mixes should not be empty + wrongAmong(mixes) shouldBe empty + } + + it should "parenthesise a written OR group under an AND" in { + // its own arm renders `(a) || (b)` -- the operands, not the group + val row = Row( + "written group", + 3, + build(1, 3, Vector(false, true), Set((1, 2)), Set.empty, Set.empty, paren = false) + ) + row.sql should endWith("HAVING (MAX(c1) > 0 OR MAX(c2) > 0) AND MAX(c3) > 0") + wrongAmong(Seq(row)) shouldBe empty + } +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/ConditionTruthTable.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/ConditionTruthTable.scala new file mode 100644 index 00000000..9c42f60c --- /dev/null +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/ConditionTruthTable.scala @@ -0,0 +1,169 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.client + +import scala.collection.mutable + +/** A fixture on which every boolean reading of a condition selects a DIFFERENT set of documents, + * and the conditions to run on it. + * + * 32 documents `d0` … `d31`: document `dN` carries `ci` = bit `i - 1` of `N` (`c1` … `c5`, 0 or + * 1), its own group key `g` = its id, its id again as full text in `t` (for MATCH), and `m` = 0. + * So `ci = 1` holds exactly on the documents whose bit `i - 1` is set, and the documents a + * condition selects spell its truth table. The expected answer is computed HERE, from the + * structure of the condition -- never from the engine: SQL's reading, `NOT` before `AND` before + * `OR`, left-associative, parentheses respected. + */ +object ConditionTruthTable { + + val Documents: Int = 32 + + def bit(doc: Int, i: Int): Boolean = ((doc >> (i - 1)) & 1) == 1 + + def id(doc: Int): String = s"d$doc" + + /** The bulk documents, `id` as the key. */ + def documents: List[String] = (0 until Documents).toList.map { n => + val cs = (1 to 5).map(i => s""""c$i":${if (bit(n, i)) 1 else 0}""").mkString(",") + s"""{"id":"${id(n)}","g":"${id(n)}","t":"${id(n)}",$cs,"m":0}""" + } + + val Mapping: String = + """{ + | "properties": { + | "id": { "type": "keyword" }, + | "g": { "type": "keyword" }, + | "t": { "type": "text" }, + | "c1": { "type": "integer" }, "c2": { "type": "integer" }, "c3": { "type": "integer" }, + | "c4": { "type": "integer" }, "c5": { "type": "integer" }, + | "m": { "type": "integer" } + | } + |}""".stripMargin + + /** A written condition: a leaf `i` (negated or not), or a group of items joined by AND (`true`) + * or OR (`false`), written in parentheses or not. + */ + sealed trait Cond { + def render(leaf: (Int, Boolean) => String): String + def holds(doc: Int): Boolean + } + + final case class Leaf(i: Int, negated: Boolean = false) extends Cond { + def render(leaf: (Int, Boolean) => String): String = leaf(i, negated) + def holds(doc: Int): Boolean = bit(doc, i) != negated + } + + final case class Group(items: List[Cond], ands: List[Boolean], paren: Boolean) extends Cond { + def render(leaf: (Int, Boolean) => String): String = { + val body = items.head.render(leaf) + items.tail + .zip(ands) + .map { case (item, isAnd) => + (if (isAnd) " AND " else " OR ") + item.render(leaf) + } + .mkString + if (paren) s"($body)" else body + } + + /** SQL's reading: runs joined by AND, the runs joined by OR. */ + def holds(doc: Int): Boolean = { + val runs = mutable.ListBuffer[List[Cond]]() + var current = List(items.head) + items.tail.zip(ands).foreach { case (item, isAnd) => + if (isAnd) current = current :+ item + else { runs += current; current = List(item) } + } + runs += current + runs.exists(_.forall(_.holds(doc))) + } + } + + /** The documents a condition selects, by id. */ + def expected(c: Cond): Set[String] = (0 until Documents).filter(c.holds).map(id).toSet + + def whereLeaf(i: Int, negated: Boolean): String = if (negated) s"NOT c$i = 1" else s"c$i = 1" + + def havingLeaf(i: Int, negated: Boolean): String = + if (negated) s"NOT MAX(c$i) > 0" else s"MAX(c$i) > 0" + + private def build( + lo: Int, + hi: Int, + ands: Vector[Boolean], + fam: Set[(Int, Int)], + negs: Set[Int], + paren: Boolean + ): Group = { + val inside = fam.filter { case (a, b) => a >= lo && b <= hi && (a, b) != ((lo, hi)) } + val maximal = inside.filter { case (a, b) => + !inside.exists { case (c, d) => (c, d) != ((a, b)) && c <= a && b <= d } + } + val items = mutable.ListBuffer[Cond]() + val itemAnds = mutable.ListBuffer[Boolean]() + var p = lo + while (p <= hi) { + val (item, end) = maximal.find(_._1 == p) match { + case Some((a, b)) => (build(a, b, ands, fam, negs, paren = true), b) + case None => (Leaf(p, negs.contains(p)), p) + } + if (items.nonEmpty) itemAnds += ands(p - 2) + items += item + p = end + 1 + } + Group(items.toList, itemAnds.toList, paren) + } + + /** Every laminar family of parenthesised intervals of 2+ conditions, whole clause excluded. */ + private def families(n: Int): List[Set[(Int, Int)]] = { + val intervals = for { + a <- (1 to n).toList; b <- (a + 1 to n).toList if (a, b) != ((1, n)) + } yield (a, b) + def laminar(s: List[(Int, Int)]): Boolean = s.forall { case (a, b) => + s.forall { case (c, d) => + (a, b) == ((c, d)) || b < c || d < a || (a <= c && d <= b) || (c <= a && b <= d) + } + } + (0 until (1 << intervals.size)).toList + .map(m => intervals.zipWithIndex.collect { case (x, k) if ((m >> k) & 1) == 1 => x }) + .filter(laminar) + .map(_.toSet) + } + + private def operators(n: Int): List[Vector[Boolean]] = + (0 until (1 << (n - 1))).toList.map(m => (0 until n - 1).map(k => ((m >> k) & 1) == 1).toVector) + + /** 3 and 4 conditions: every parenthesisation x every operator sequence (12 + 88). */ + def parenthesised: List[Cond] = + for { n <- List(3, 4); fam <- families(n); ands <- operators(n) } yield build( + 1, + n, + ands, + fam, + Set.empty, + paren = false + ) + + /** 3 conditions, one of them negated, flat or with one group (36). */ + def negated: List[Cond] = + for { fam <- families(3); ands <- operators(3); neg <- List(1, 2, 3) } yield build( + 1, + 3, + ands, + fam, + Set(neg), + paren = false + ) +} diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala index 386b57c0..4e6c4c4a 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/PredicateFunctionResultSpec.scala @@ -134,6 +134,8 @@ trait PredicateFunctionResultSpec extends AnyFlatSpecLike with ElasticDockerTest override def afterAll(): Unit = { client.deleteIndex(index) + Seq(precedenceIndex, s"${precedenceIndex}_delete", s"${precedenceIndex}_update") + .foreach(client.deleteIndex(_)) system.terminate() super.afterAll() } @@ -465,4 +467,229 @@ trait PredicateFunctionResultSpec extends AnyFlatSpecLike with ElasticDockerTest if (esMajor >= 7) selected("WEEKDAY(d) = 0 AND DATE_FORMAT(d, 'yyyy') = '2025'") shouldBe Set("d6") } + + // -- AND / OR precedence, on a truth-table fixture --------------------------------------------- + + /** `ConditionTruthTable`: 32 documents on which every boolean reading of a condition selects a + * different set, so the documents returned show which reading ran. The expected set is computed + * from the condition's structure -- SQL's reading, AND before OR -- never from the engine. + * + * 🔴 Why RUN rows and not query pins: an OR under an AND used to be emitted as `should` clauses + * beside `filter` clauses, which Elasticsearch treats as OPTIONAL -- a well-formed query, HTTP + * 200, the wrong documents. Only the rows show it, and Elasticsearch 6 and 7+ disagree on them. + */ + private val precedenceIndex = "condition_precedence" + + private def loadTruthTable(name: String): Unit = { + client + .createIndex(name, settings = """{"number_of_shards": 1, "number_of_replicas": 0}""") + .get shouldBe true + client.setMapping(name, ConditionTruthTable.Mapping).get shouldBe true + implicit val bulkOptions: BulkOptions = BulkOptions(defaultIndex = name, logEvery = 10) + implicit def listToSource[T](list: List[T]): Source[T, NotUsed] = + Source.fromIterator(() => list.iterator) + client.bulk[String](ConditionTruthTable.documents, identity, idKey = Some(Set("id"))) match { + case ElasticSuccess(_) => client.refresh(name) + case ElasticFailure(error) => fail(s"Bulk indexing into $name failed: ${error.message}") + } + } + + private lazy val truthTableLoaded: Unit = loadTruthTable(precedenceIndex) + + private def idsOf(rows: Seq[ListMap[String, Any]], column: String): Set[String] = + rows.map(r => r.getOrElse(column, fail(s"no $column column in $r")).toString).toSet + + private def whereIds(where: String): Set[String] = + idsOf(rowsOf(s"SELECT id FROM $precedenceIndex WHERE $where"), "id") + + /** The same, through `GatewayApi.run`. */ + private def gatewayWhereIds(where: String): Set[String] = { + val sql = s"SELECT id FROM $precedenceIndex WHERE $where" + val rows = Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(QueryRows(rows, _)) => rows + case ElasticSuccess(QueryStructured(response, _)) => response.results + case ElasticSuccess(QueryStream(stream, _)) => + Await.result(stream.map(_._1).runWith(Sink.seq), 60.seconds) + case ElasticSuccess(other) => fail(s"Unexpected result: $other\n$sql") + case ElasticFailure(error) => fail(s"Query failed: ${error.message}\n$sql") + } + idsOf(rows, "id") + } + + private def collectWrong( + conditions: Seq[(String, Set[String])] + )(run: String => Set[String]): Unit = { + val wrong = conditions.flatMap { case (condition, expected) => + val actual = run(condition) + if (actual == expected) None + else + Some( + s"[$condition] returned ${actual.size}, expected ${expected.size}: missing ${expected -- actual}, extra ${actual -- expected}" + ) + } + withClue(s"${wrong.size} of ${conditions.size} wrong:\n${wrong.take(20).mkString("\n")}\n") { + wrong shouldBe empty + } + } + + "a WHERE that combines AND and OR" should "select the documents SQL's precedence selects" in { + truthTableLoaded + val conditions = (ConditionTruthTable.parenthesised ++ ConditionTruthTable.negated).map { c => + c.render(ConditionTruthTable.whereLeaf) -> ConditionTruthTable.expected(c) + } + collectWrong(conditions)(whereIds) + } + + it should "answer the same through the gateway venue" in { + truthTableLoaded + import ConditionTruthTable.{expected, Group, Leaf} + def g(items: List[ConditionTruthTable.Cond], ands: List[Boolean], paren: Boolean = false) = + Group(items, ands, paren) + val named = Seq( + // the documentation's example: category = 'Electronics' AND price < 100 OR on_sale = true + g(List(Leaf(1), Leaf(2), Leaf(3)), List(true, false)), + // a OR b AND c + g(List(Leaf(1), Leaf(2), Leaf(3)), List(false, true)), + // a AND b OR c AND d + g(List(Leaf(1), Leaf(2), Leaf(3), Leaf(4)), List(true, false, true)), + // a OR (b OR c) AND d: the operators after a group used to trade places + g( + List(Leaf(1), g(List(Leaf(2), Leaf(3)), List(false), paren = true), Leaf(4)), + List(false, true) + ), + // a AND (b OR c AND d): Elasticsearch 6 and 8 used to return different wrong rows + g( + List(Leaf(1), g(List(Leaf(2), Leaf(3), Leaf(4)), List(false, true), paren = true)), + List(true) + ), + // a OR NOT b AND c: the NOT moves into the condition it heads + g(List(Leaf(1), Leaf(2, negated = true), Leaf(3)), List(false, true)) + ).map(c => c.render(ConditionTruthTable.whereLeaf) -> expected(c)) + collectWrong(named)(whereIds) + collectWrong(named)(gatewayWhereIds) + } + + "an OR group holding a MATCH, under an AND" should "stay one condition of the AND" in { + truthTableLoaded + // Spread beside the root's `filter` clauses, the group's `should` clauses turned optional, and + // this returned every document with c1 = 1, whatever the MATCH and c3. `d1` is the one + // document the MATCH alone admits (c1 = 1, c3 = 0). + val where = "c1 = 1 AND (MATCH (t) AGAINST ('d1') OR c3 = 1)" + val expected = (0 until ConditionTruthTable.Documents) + .filter(d => ConditionTruthTable.bit(d, 1) && (d == 1 || ConditionTruthTable.bit(d, 3))) + .map(ConditionTruthTable.id) + .toSet + expected should contain("d1") + collectWrong(Seq(where -> expected))(whereIds) + collectWrong(Seq(where -> expected))(gatewayWhereIds) + } + + "a CASE WHEN that combines AND and OR" should "evaluate SQL's reading on every document" in { + truthTableLoaded + val conditions = ConditionTruthTable.parenthesised.take(12).map { c => + c.render(ConditionTruthTable.whereLeaf) -> ConditionTruthTable.expected(c) + } + collectWrong(conditions) { condition => + rowsOf(s"SELECT id, CASE WHEN $condition THEN 1 ELSE 0 END AS x FROM $precedenceIndex") + .filter { r => + (r.getOrElse("x", "") match { + case s: Seq[_] => s.headOption.map(_.toString).getOrElse("") + case a: java.util.Collection[_] => a.toArray.headOption.map(_.toString).getOrElse("") + case other => String.valueOf(other) + }) == "1" + } + .map(r => r.getOrElse("id", fail(s"no id column in $r")).toString) + .toSet + } + } + + "a HAVING over aggregates that combines AND and OR" should "keep the groups SQL's reading keeps" in { + truthTableLoaded + val conditions = + (ConditionTruthTable.parenthesised.take(12) ++ ConditionTruthTable.negated.take(12)).map { + c => + c.render(ConditionTruthTable.havingLeaf) -> ConditionTruthTable.expected(c) + } + collectWrong(conditions) { having => + idsOf( + rowsOf(s"SELECT g, COUNT(*) AS cnt FROM $precedenceIndex GROUP BY g HAVING $having"), + "g" + ) + } + } + + "a HAVING that compares an aggregate with 1" should "read that aggregate's own parameter" in { + truthTableLoaded + // `MAX(c1) = 1` renders `params.max_c1 == 1`, and the emission used to strip the text `1 == 1` + // -- its placeholder for "nothing to filter" -- out of the script, leaving `params.max_c`, + // which no aggregation publishes: the search failed. + val expected = (0 until ConditionTruthTable.Documents) + .filter(ConditionTruthTable.bit(_, 1)) + .map(ConditionTruthTable.id) + .toSet + idsOf( + rowsOf(s"SELECT g, COUNT(*) AS cnt FROM $precedenceIndex GROUP BY g HAVING MAX(c1) = 1"), + "g" + ) shouldBe expected + } + + "a HAVING on one GROUP BY key that combines AND and OR" should + "keep the groups SQL's reading keeps" in { + truthTableLoaded + // One group per document (`g` is its id), so `g = 'dN'` holds for the group dN alone. + val rows = Seq( + // (g <> d1 AND g <> d2 AND g = d3) OR g = d4 + "NOT g = 'd1' AND NOT g = 'd2' AND g = 'd3' OR g = 'd4'" -> Set("d3", "d4"), + // g = d1 OR (g = d2 AND g <> d3 AND g <> d4) + "g = 'd1' OR g = 'd2' AND NOT g = 'd3' AND NOT g = 'd4'" -> Set("d1", "d2") + ) ++ ( + // g = d1 OR (g = d2 AND g <> d3). ⚠️ Not on Elasticsearch 6: the es6 bridge renders a ONE-value + // exclude list as a bare string, which 6.8 reads as a pattern and refuses beside an include + // list -- pre-existing, and the same for these lists before AND bound tighter than OR. + if (esMajor >= 7) + Seq( + "g = 'd1' OR g = 'd2' AND NOT g = 'd3'" -> Set("d1", "d2"), + "(g = 'd1' OR g = 'd2' AND NOT g = 'd3')" -> Set("d1", "d2") + ) + else Nil + ) + collectWrong(rows) { having => + idsOf( + rowsOf(s"SELECT g, COUNT(*) AS cnt FROM $precedenceIndex GROUP BY g HAVING $having"), + "g" + ) + } + } + + "DELETE and UPDATE with a condition that combines AND and OR" should + "change exactly the documents SQL's reading selects" in { + import ConditionTruthTable.{expected, Group, Leaf} + val conditions = Seq( + Group(List(Leaf(1), Leaf(2), Leaf(3)), List(false, true), paren = false), + Group(List(Leaf(1), Leaf(2), Leaf(3), Leaf(4)), List(true, false, true), paren = false) + ) + val all = (0 until ConditionTruthTable.Documents).map(ConditionTruthTable.id).toSet + conditions.foreach { c => + val where = c.render(ConditionTruthTable.whereLeaf) + def dml(sql: String, on: String): Unit = + Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(_) => client.refresh(on) + case ElasticFailure(error) => fail(s"DML failed: ${error.message}\n$sql") + } + val deleted = s"${precedenceIndex}_delete" + loadTruthTable(deleted) + dml(s"DELETE FROM $deleted WHERE $where", deleted) + withClue(s"[DELETE ... WHERE $where] ") { + idsOf(rowsOf(s"SELECT id FROM $deleted"), "id") shouldBe (all -- expected(c)) + } + client.deleteIndex(deleted) + val updated = s"${precedenceIndex}_update" + loadTruthTable(updated) + dml(s"UPDATE $updated SET m = 1 WHERE $where", updated) + withClue(s"[UPDATE ... WHERE $where] ") { + idsOf(rowsOf(s"SELECT id FROM $updated WHERE m = 1"), "id") shouldBe expected(c) + } + client.deleteIndex(updated) + } + } }