From b560194f3260166848605cd397334eead5ebf30d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Mon, 7 Sep 2026 06:55:54 +0200 Subject: [PATCH 1/4] fix(sql): an explicit GROUP BY is aggregation-shaped, aggregate or not (story 21.3) `SELECT category FROM t GROUP BY category` -- the SQL-standard spelling of DISTINCT, and 22 of the 99 captured BI statements -- executed as a DOCUMENT query and returned per-document rows, capped at the licensed `maxQueryResults`. Nothing failed; the row count tracked the fixture size instead of the group count. Fourth member of the #205/#207/#209/#224 silent-wrong-answer family. Root cause was one routing expression: `returnsRows = windowRowQuery || sqlAggregations.isEmpty`. With no aggregate anywhere `sqlAggregations` is empty, so an explicit GROUP BY was invisible to the shape discriminator. Its own scaladoc already promised `false` for GROUP BY -- the expression, not the contract, was the bug -- so this is a one-expression fix, not a second discriminator. The emission layer already builds the terms aggregation, sizes it and projects the bucket keys; the wide test surface exists to prove that. Ordinal positions, hardened at the same time (epic 21 OQ-1). `GROUP BY ` resolved to the n-th SELECT item with three sharp edges, all measured: an out-of-range or negative position CRASHED inside `Parser.apply` (`SingleSearch.bucketNames` indexed `select.fields(n - 1)` unguarded, and it runs before `Bucket.update`'s own bounds check); the substituted identifier was never `update(request)`-ed, so `SELECT tbl.category ... GROUP BY 1` was rejected with an unrelated message; and ordinal-ness was sniffed with `"\d+".r.findFirstIn` over the RENDERED name, so any column carrying a digit was a candidate -- `GROUP BY city2` crashed, `GROUP BY status2` silently re-pointed. All three now go through ONE `Bucket.ordinalOf` on the AST, and an unresolved position is recorded and rejected by `validate()` instead of thrown. `ORDER BY ` needed no parser change: it always PARSED and was silently discarded (a Painless sort over a constant on the row path, an unmatched bucket-order key on the GROUP BY path), so it is repaired, not implemented. `GROUP BY ` resolves to the aliased field. It used to emit `terms { + * field: "" }` on a NON-EXISTENT field, which Elasticsearch answers with ZERO buckets + * and HTTP 200 -- a silent wrong answer in #253's own family. + * - `OFFSET` under a `GROUP BY` is rejected loudly, where it used to be silently dropped by the + * aggregation emission branch. + * + * Every "today" verdict in the comments was MEASURED against a parser compiled from `origin/main` + * `8c426253` on 2026-09-07. + */ +class GroupByFoldInSpec extends AnyFlatSpec with Matchers { + + private def parsed(sql: String): SingleSearch = + Parser(sql) match { + case Right(select: SelectStatement) => + select.statement match { + case Some(s: SingleSearch) => s + case other => fail(s"Not a SingleSearch: $other") + } + case Right(s: SingleSearch) => s + case other => fail(s"Failed to parse '$sql': $other") + } + + private def reasonOf(sql: String): String = + Parser(sql).swap.getOrElse(fail(s"expected Left for [$sql]")).msg + + private def rejects(sql: String, reasons: String*): Unit = { + withClue(s"[$sql] ") { noException should be thrownBy Parser(sql) } + withClue(s"[$sql] ") { Parser(sql).isLeft shouldBe true } + val msg = reasonOf(sql) + withClue(s"[$sql] msg=[$msg] ") { msg should not startWith Parser.InternalParseFailure } + reasons.foreach(r => withClue(s"[$sql] msg=[$msg] ") { msg should include(r) }) + () + } + + private def accepts(sql: String): Unit = { + withClue(s"[$sql] ") { noException should be thrownBy Parser(sql) } + withClue(s"[$sql] reason=[${Parser(sql).swap.map(_.msg).getOrElse("")}] ") { + Parser(sql).isRight shouldBe true + } + () + } + + private def bucketTriples(sql: String): Seq[(String, String, String)] = + parsed(sql).buckets.map(b => (b.name, b.path, b.sourceBucket)) + + // ---- FOLD-IN 1: a row-invariant literal that IS the group key ---- + // + // 🔴 SCOPE, decided by MEASUREMENT during implementation (2026-09-07) and escalated to the lead. + // FOLD-IN 1 has two sub-cases and only one of them works end to end: + // + // (A) the constant IS the grouped item (`GROUP BY 2` naming `2 AS COL2`, or `GROUP BY COL2`). + // The bucket key carries the value, and the four Tableau corpus probes PD-7 names -- the + // ones "holding the published ceiling at 65/99" -- are all this shape. It is delivered + // here, end to end, and verified against real Elasticsearch. + // + // (B) the constant is an EXTRA ungrouped column (`SELECT category, 2 AS flag ... GROUP BY + // category`). MEASURED on real ES 8.18: the statement parses, the constant is emitted as a + // `script_fields` entry, the aggregation path answers with `"size": 0`, and the column + // comes back **null** for every row. Accepting it would trade one loud rejection for a + // silent wrong answer, in the exact family this story exists to close, so it is NOT + // accepted here; the rows below pin the rejection so the decision is visible. + // + // Case (B) needs the constant PROJECTED into each aggregation row, which lives in `core` + // (`ElasticConversion.jsonToRows` takes `fields: Seq[String]`, with no statement in scope) and + // reaches ~10 call sites across `SearchApi` / `ScrollApi`. That is a design question for the + // lead, not a drive-by: it is recorded in `docs/issues/local-21.3-grouped-select-literal.md`. + + "a constant that IS the group key" should "be accepted (FOLD-IN 1, case A)" in { + // The 4-row Tableau corpus shape, and its alias spelling. Both were Left before this story + // (the ordinal resolved onto the RAW literal identifier, whose bucket name was "" rather than + // the alias, so the non-aggregated-field check rejected the SELECT item). + accepts("SELECT SUM(1) AS COL, 2 AS COL2 FROM t GROUP BY 2") + accepts("SELECT SUM(1) AS COL, 2 AS COL2 FROM t GROUP BY 2 HAVING COUNT(1) > 0") + accepts("SELECT 2 AS COL2 FROM t GROUP BY COL2") + accepts("SELECT 3 AS c FROM t GROUP BY 1") + } + + it should "keep accepting a constant written directly as the group key" in { + // Pre-existing on main and unchanged -- included so the constant-bucket work is pinned for the + // non-integer spellings too (a string constant renders and re-parses as itself). + accepts("SELECT 'x' AS lbl FROM t GROUP BY 'x'") + } + + "an ungrouped constant beside a GROUP BY" should "stay rejected (FOLD-IN 1, case B)" in { + // See the scope note above: accepted, this returns the column as NULL on every row (MEASURED + // on real ES 8.18). The rejection is the SAME message these statements got before this story, + // so nothing regresses -- what changes is that the reason is now recorded rather than assumed. + rejects("SELECT category, 2 AS COL2 FROM t GROUP BY category", "Non-aggregated fields") + rejects("SELECT category, 'x' AS lbl FROM t GROUP BY category", "Non-aggregated fields") + rejects("SELECT SUM(amount) AS s, 2 AS COL2 FROM t GROUP BY country", "Non-aggregated fields") + } + + it should "resolve an ordinal naming a literal EXACTLY ONCE" in { + // The substituted identifier is itself ordinal-SHAPED (`name = ""`, `functions = [LongValue]`), + // so a second derivation of `ordinalOf` on an already-substituted bucket would re-read the + // literal as a position. `SELECT 3 AS c FROM t GROUP BY 1` is the probe: position 1, literal 3. + // If resolution ran twice the bucket would re-point to position 3, which does not exist, and + // the statement would be rejected as out of range. + accepts("SELECT 3 AS c FROM t GROUP BY 1") + val s = parsed("SELECT 3 AS c FROM t GROUP BY 1") + s.buckets.map(_.name) shouldBe Seq("c") + } + + it should "keep accepting the alias-of-a-literal spelling (the don't-regress pin)" in { + // Right TODAY (measured) -- this pin proves the fix does not disturb it. + accepts("SELECT 2 AS COL2, SUM(amount) AS s FROM t GROUP BY COL2") + } + + it should "re-emit a constant bucket as its ALIAS, so the render re-parses to the same bucket" in { + // 🔴 Found while implementing. A bucket resolved onto an integer constant renders through + // `Identifier.sql` as the bare `2`, which the grammar reads back as POSITION 2. MEASURED + // before the fix: `SELECT 2 AS COL2, SUM(amount) AS s FROM t GROUP BY COL2` rendered + // `... GROUP BY 2` and re-parsed as `GROUP BY SUM(amount)` -- a DIFFERENT aggregation, and one + // `MaterializedViewExtension` would persist and re-run. Story 21.1's AD-13 lesson, one clause + // over: re-parse the RENDER, because every parse-only assertion is green either way. + Seq( + "SELECT 2 AS COL2, SUM(amount) AS s FROM t GROUP BY COL2", + "SELECT 2 AS COL2 FROM t GROUP BY COL2", + "SELECT 3 AS c FROM t GROUP BY 1", + "SELECT SUM(1) AS COL, 2 AS COL2 FROM t GROUP BY 2" + ).foreach { sql => + val rendered = Parser(sql).map(_.sql).getOrElse(fail(s"did not parse: $sql")) + withClue(s"[$sql] rendered=[$rendered] ") { + Parser(rendered).map(_.sql).getOrElse("") shouldBe rendered + parsed(rendered).buckets.map(b => (b.name, b.path)) shouldBe + parsed(sql).buckets.map(b => (b.name, b.path)) + } + } + // A NON-integer constant is unambiguous and keeps rendering as itself (unchanged behaviour). + Parser("SELECT 'x' AS lbl FROM t GROUP BY 'x'").map(_.sql).getOrElse("") should + include("GROUP BY 'x'") + } + + it should "reject a constant bucket that has no alias to name it" in { + // The residual of the rule above: with no explicit SELECT alias there is NO spelling that + // re-emits the bucket (`SELECT 3 FROM t GROUP BY 1` would render `GROUP BY 3` and re-parse as + // position 3, which is out of range). Rejected rather than shipped with a corrupt render -- + // and it was rejected before the constant classification landed too, so nothing regresses. + rejects("SELECT 3 FROM t GROUP BY 1", "requires a SELECT alias") + } + + it should "still reject an ungrouped COLUMN" in { + rejects("SELECT category, country FROM t GROUP BY category", "Non-aggregated fields country") + } + + it should "never treat a ROW-VARIANT pseudo-value as a constant" in { + // `Bucket.shouldBeScripted`'s constant classification is an ALLOW-list, so a row-VARIANT value + // can never be scripted as a constant bucket: `RANDOM` re-evaluates per document and `?` is an + // unbound parameter. + // ⚠️ The spelling is the bare `RANDOM`, not `RANDOM()`: MEASURED, `RANDOM()` does not parse at + // all ("end of input expected"), so a `RANDOM()` row would have passed for the wrong reason. + rejects("SELECT category, RANDOM AS r FROM t GROUP BY category", "Non-aggregated fields") + rejects("SELECT category, ? AS p FROM t GROUP BY category", "Non-aggregated fields") + SingleSearch.isRowInvariantLiteral( + parsed("SELECT RANDOM AS r FROM t").select.fields.head.identifier + ) shouldBe false + SingleSearch.isRowInvariantLiteral( + parsed("SELECT ? AS p FROM t").select.fields.head.identifier + ) shouldBe false + SingleSearch.isRowInvariantLiteral( + parsed("SELECT 2 AS n FROM t").select.fields.head.identifier + ) shouldBe true + } + + // ---- FOLD-IN 2: GROUP BY " should "produce the SAME bucket as the explicit spelling" in { + // Equivalence oracle (measured pre-fix: (pays, pays, pays) vs (pays, country, country)). + bucketTriples("SELECT country AS pays FROM t GROUP BY pays") shouldBe + bucketTriples("SELECT country AS pays FROM t GROUP BY country") + bucketTriples("SELECT country AS pays FROM t GROUP BY pays") shouldBe + Seq(("pays", "country", "country")) + } + + it should "resolve an EXPRESSION alias to the aliased expression's bucket" in { + bucketTriples("SELECT UPPER(country) AS c FROM t GROUP BY c") shouldBe + bucketTriples("SELECT UPPER(country) AS c FROM t GROUP BY UPPER(country)") + } + + it should "fix the aggregate-BEARING twin too (pre-existing, even without #253)" in { + parsed("SELECT country AS pays, COUNT(*) AS cnt FROM t GROUP BY pays").buckets + .map(_.sourceBucket) shouldBe Seq("country") + } + + it should "let a PROJECTED column win over an alias spelled the same" in { + // `SELECT country AS pays FROM t GROUP BY country` must never re-route through an alias. + bucketTriples("SELECT country AS pays FROM t GROUP BY country") shouldBe + Seq(("pays", "country", "country")) + // And a column aliased to its own name is not an alias reference either. + bucketTriples("SELECT country AS country FROM t GROUP BY country") shouldBe + Seq(("country", "country", "country")) + } + + it should "index the bucket under BOTH spellings so the SELECT field finds it" in { + val s = parsed("SELECT country AS pays FROM t GROUP BY pays") + s.bucketNames.keys.toSeq should contain("country") + } + + it should "normalise the alias to the resolved column on render, with the render fixed point" in { + val rendered = Parser("SELECT country AS pays FROM t GROUP BY pays") + .map(_.sql) + .getOrElse(fail("did not parse")) + rendered should include("GROUP BY country") + Parser(rendered).map(_.sql).getOrElse("") shouldBe rendered + } + + it should "leave a bucket that matches no alias alone" in { + bucketTriples("SELECT category FROM t GROUP BY category") shouldBe + Seq(("category", "category", "category")) + } + + // ---- OFFSET under GROUP BY: loud rejection ---- + + "OFFSET under a GROUP BY" should "be rejected loudly, aggregate-free or not" in { + // BOTH succeed today with the offset silently dropped (behaviour change -- release note). + rejects("SELECT category FROM t GROUP BY category LIMIT 10 OFFSET 5", "OFFSET") + rejects( + "SELECT category, COUNT(*) AS c FROM t GROUP BY category LIMIT 10 OFFSET 5", + "OFFSET" + ) + } + + it should "leave LIMIT without OFFSET, and OFFSET without GROUP BY, untouched" in { + accepts("SELECT category FROM t GROUP BY category LIMIT 10") + accepts("SELECT category FROM t LIMIT 10 OFFSET 5") + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala new file mode 100644 index 000000000..a48c541ad --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala @@ -0,0 +1,262 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.parser.Parser +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Issue #253 / epic 21 OQ-1 -- ordinal `GROUP BY ` and `ORDER BY `. + * + * Before this story (every verdict below MEASURED against a parser compiled from `origin/main` + * `8c426253` on 2026-09-07, not inferred): + * - `GROUP BY ` resolved to the n-th SELECT item, but an out-of-range POSITIVE position + * raised `IndexOutOfBoundsException` from `SingleSearch.bucketNames`, which runs BEFORE + * `Bucket.update`'s own bounds check; + * - only a NEGATIVE position reached `Bucket.update` and raised `IllegalArgumentException`; + * - `bucketNames` sniffed the ordinal with `"\d+".r.findFirstIn` on the RENDERED name, so `GROUP + * BY city2` raised `IndexOutOfBoundsException: 1` and `GROUP BY logs2025` raised + * `IndexOutOfBoundsException: 2024`; + * - `ORDER BY ` PARSED (via `identifierWithArithmeticExpression`) and was silently DROPPED -- + * a constant script sort on the row path, an unmatched bucket-order key on the GROUP BY path. + * + * 🔴 Since story 21.4 (#250) none of those crashes ESCAPES: `Parser.apply` has a `NonFatal` + * boundary catch that turns them into a `Left` labelled `Parser.InternalParseFailure`. That makes + * `noException` + `isLeft` UNFALSIFIABLE on their own -- restoring the crash still yields a + * `Left`. Every rejection below therefore also asserts the message does NOT start with that label, + * which is what distinguishes "the grammar/validator rejected it" from "something blew up". + */ +class GroupByOrdinalSpec extends AnyFlatSpec with Matchers { + + private def parsed(sql: String): SingleSearch = + Parser(sql) match { + case Right(select: SelectStatement) => + select.statement match { + case Some(s: SingleSearch) => s + case other => fail(s"Not a SingleSearch: $other") + } + case Right(s: SingleSearch) => s + case other => fail(s"Failed to parse '$sql': $other") + } + + private def bucketPaths(sql: String): Seq[String] = parsed(sql).buckets.map(_.path) + + private def reasonOf(sql: String): String = + Parser(sql).swap.getOrElse(fail(s"expected Left for [$sql]")).msg + + /** A rejection the VALIDATOR owns: not a throw, not the boundary catch, and naming the problem. + */ + private def rejects(sql: String, reasons: String*): Unit = { + withClue(s"[$sql] ") { noException should be thrownBy Parser(sql) } + withClue(s"[$sql] ") { Parser(sql).isLeft shouldBe true } + val msg = reasonOf(sql) + withClue(s"[$sql] msg=[$msg] ") { msg should not startWith Parser.InternalParseFailure } + reasons.foreach(r => withClue(s"[$sql] msg=[$msg] ") { msg should include(r) }) + () + } + + // ---- resolution ---- + + // ⚠️ EVERY statement below was run through the real parser and behaves as the comment says. Do + // NOT "simplify" one into a multi-column shape: an ordinal that groups only SOME of the + // projected columns is rejected by the non-aggregated-field check, before AND after this fix, + // because that is correct SQL (`SELECT country, category FROM t GROUP BY 1` is a Left both + // ways). + + "GROUP BY " should "resolve to the n-th SELECT item" in { + bucketPaths("SELECT country FROM t GROUP BY 1") shouldBe Seq("country") + bucketPaths("SELECT country, category FROM t GROUP BY 1, 2") shouldBe + Seq("country", "category") + } + + it should "resolve several positions, in the order WRITTEN, not in SELECT order" in { + bucketPaths("SELECT country, category FROM t GROUP BY 2, 1") shouldBe Seq("category", "country") + } + + it should "resolve a position that names an aggregate-free item beside an aggregate" in { + bucketPaths("SELECT country, COUNT(*) AS c FROM t GROUP BY 1") shouldBe Seq("country") + } + + it should "resolve a QUALIFIED select item to its column, not to the qualified spelling" in { + // Defect (b): the substituted identifier was not `update(request)`-ed, so the bucket named + // "tbl.category" while the SELECT field resolved to "category", and the statement was rejected + // with the unrelated "Non-aggregated fields tbl.category ..." (MEASURED pre-fix -- this test is + // RED on `origin/main` as a Left, not as a wrong value). + bucketPaths("SELECT tbl.category FROM idx tbl GROUP BY 1") shouldBe Seq("category") + bucketPaths("SELECT tbl.category FROM idx tbl GROUP BY 1") shouldBe + bucketPaths("SELECT tbl.category FROM idx tbl GROUP BY tbl.category") + } + + it should "agree with the explicit column spelling" in { + bucketPaths("SELECT country FROM t GROUP BY 1") shouldBe + bucketPaths("SELECT country FROM t GROUP BY country") + } + + // ---- rejection, never a throw (issue #250's family) ---- + // + // MEASURED pre-fix, on `origin/main` `8c426253`: each of these is already a `Left`, but produced + // by 21.4's boundary catch, so the message reads `Internal parser error: : `: + // GROUP BY 99 -> Internal parser error: IndexOutOfBoundsException: 98 + // GROUP BY 0 -> Internal parser error: IndexOutOfBoundsException: -1 + // GROUP BY -1 -> Internal parser error: IllegalArgumentException: Bucket index must be ... + // The `not startWith InternalParseFailure` assertion is what makes these tests fail pre-fix. + + "an out-of-range GROUP BY position" should "be rejected by validate(), not by the boundary catch" in { + rejects("SELECT a FROM t GROUP BY 99", "GROUP BY position 99", "1 item(s)") + rejects("SELECT a, b FROM t GROUP BY 9", "GROUP BY position 9", "2 item(s)") + } + + "a non-positive GROUP BY position" should "be rejected by validate(), not by the boundary catch" in { + rejects("SELECT a FROM t GROUP BY 0", "GROUP BY position 0") + rejects("SELECT a, b FROM t GROUP BY 0", "GROUP BY position 0") + rejects("SELECT a FROM t GROUP BY -1", "GROUP BY position -1") + } + + // ---- defect (c): a digit inside a COLUMN NAME is not an ordinal ---- + + "a bucket whose column name contains a digit" should "not be treated as an ordinal" in { + // Pre-fix this crashed inside `Parser.apply` (one SELECT item, + // findFirstIn("city2") == Some("2") -> select.fields(1)) and came back as the boundary catch's + // `Internal parser error: IndexOutOfBoundsException: 1`. + noException should be thrownBy Parser("SELECT city2 FROM t GROUP BY city2") + bucketPaths("SELECT city2 FROM t GROUP BY city2") shouldBe Seq("city2") + } + + it should "not silently re-point the bucket when the SELECT is long enough to index" in { + // Pre-fix this mapped "2" -> the COUNT(*) identifier and LOST the "status2" key. + val s = parsed("SELECT status2, COUNT(*) AS c FROM t GROUP BY status2") + s.buckets.map(_.path) shouldBe Seq("status2") + s.bucketNames.keys.toSeq should contain("status2") + s.bucketNames.keys.toSeq should not contain "2" + } + + it should "not be confused by a four-digit column name" in { + noException should be thrownBy Parser("SELECT logs2025 FROM t GROUP BY logs2025") + bucketPaths("SELECT logs2025 FROM t GROUP BY logs2025") shouldBe Seq("logs2025") + } + + it should "not be confused by a digit inside a SELECT ALIAS" in { + // Pre-fix `findFirstIn("pays2")` matched the `2` and indexed `select.fields(1)` on a one-column + // SELECT, crashing inside `Parser.apply`. Post-fix the name is not an ordinal at all and the + // alias resolves to its column, exactly as an alias without a digit does. + noException should be thrownBy Parser("SELECT country AS pays2 FROM t GROUP BY pays2") + bucketPaths("SELECT country AS pays2 FROM t GROUP BY pays2") shouldBe Seq("country") + bucketPaths("SELECT country AS pays2 FROM t GROUP BY pays2") shouldBe + bucketPaths("SELECT country AS pays FROM t GROUP BY pays") + } + + it should "resolve an ordinal EXACTLY ONCE, even when update() runs twice" in { + // 🔴 Idempotence. A bare literal in the SELECT list has exactly the ordinal's AST shape, so a + // resolved bucket is itself ordinal-SHAPED. `SingleSearch.update()` really does run twice on + // one statement (`Table.mergeWithSearch` re-updates an already-parsed search with a schema), + // and without the `resolved` latch the second pass re-read the substituted literal as a + // position: MEASURED, `GROUP BY COL2` re-pointed from the literal onto `SUM(amount)`. + val once = parsed("SELECT 2 AS COL2, SUM(amount) AS s FROM t GROUP BY COL2") + val twice = once.update() + twice.buckets.map(b => (b.name, b.path)) shouldBe once.buckets.map(b => (b.name, b.path)) + + val ordinalOnce = parsed("SELECT country, category FROM t GROUP BY 2, 1") + ordinalOnce.update().buckets.map(_.path) shouldBe ordinalOnce.buckets.map(_.path) + } + + it should "read a QUOTED digit as the column named with it, never as a position" in { + // The 21.1 escape hatch: `GROUP BY `1`` is `name = "1"` with NO functions, so + // `Bucket.ordinalOf` declines it. Under the old rendered-name regex it was sniffed as + // position 1. + val s = parsed("SELECT `1` FROM t GROUP BY `1`") + s.buckets.map(_.path) shouldBe Seq("1") + s.bucketNames.keys.toSeq shouldBe Seq("1") + } + + // ---- ORDER BY ---- + + "ORDER BY " should "resolve to the n-th SELECT item" in { + parsed("SELECT country, category FROM t GROUP BY country, category ORDER BY 1 ASC").orderBy + .map(_.sorts.map(_.name)) shouldBe Some(Seq("country")) + } + + it should "carry the sort direction" in { + val sorts = parsed("SELECT a, b FROM t ORDER BY 2 DESC").orderBy.map(_.sorts).getOrElse(Nil) + sorts.map(_.name) shouldBe Seq("b") + sorts.map(_.direction) shouldBe Seq(Desc) + } + + it should "stop being a script sort over a constant once it resolves" in { + // The row-path half of the silent drop: pre-fix `isScriptSort` was true and the Painless was + // the constant `1`, so every document got the same sort key. + val sorts = parsed("SELECT a, b FROM t ORDER BY 1").orderBy.map(_.sorts).getOrElse(Nil) + sorts.map(_.isScriptSort) shouldBe Seq(false) + sorts.map(_.field.name) shouldBe Seq("a") + } + + it should "resolve a position that names an AGGREGATE, ordering the buckets by the metric" in { + // The Tableau/Superset shape `GROUP BY 1 ORDER BY 2 DESC`. Pre-fix `sorts` was + // ListMap("2" -> DESC), matched no bucket, and was silently dropped (MEASURED). + val byOrdinal = + parsed("SELECT category, COUNT(*) AS c FROM t GROUP BY 1 ORDER BY 2 DESC") + val byExplicit = + parsed("SELECT category, COUNT(*) AS c FROM t GROUP BY category ORDER BY COUNT(*) DESC") + + byOrdinal.orderBy.map(_.sorts.map(_.name)) should not be Some(Seq("2")) + byOrdinal.orderBy.map(_.sorts.map(_.name)) shouldBe + byExplicit.orderBy.map(_.sorts.map(_.name)) + // MEASURED post-fix: `COUNT(*) AS c` folds to the function chain, not to the alias. + byOrdinal.orderBy.map(_.sorts.map(_.name)) shouldBe Some(Seq("COUNT(*)")) + byOrdinal.orderBy.map(_.sorts.map(_.direction)) shouldBe Some(Seq(Desc)) + } + + it should "be rejected for an out-of-range position, not silently ignored" in { + // Pre-fix this PARSED and silently sorted by a constant -- so the RED here is a Right, not a + // throw and not the boundary catch. + rejects("SELECT a FROM t ORDER BY 5", "ORDER BY position 5", "1 item(s)") + } + + it should "reject `ORDER BY 0` and `ORDER BY -1`, which name no SELECT item" in { + // MEASURED pre-fix: BOTH parse and render back as `ORDER BY 0 ASC` / `ORDER BY -1 ASC`, sorting + // by a constant. `-1` is NOT "a column named -1" -- it is LongValue(-1) with an empty + // field.name; `identifierName` merely renders it back. This is a deliberate behaviour change on + // statements that "succeed" today (release-note item). + rejects("SELECT a FROM t ORDER BY 0", "ORDER BY position 0") + rejects("SELECT a FROM t ORDER BY -1", "ORDER BY position -1") + } + + it should "read a QUOTED digit as the column named with it, never as a position" in { + val sorts = parsed("SELECT a FROM t ORDER BY `1`").orderBy.map(_.sorts).getOrElse(Nil) + sorts.map(_.name) shouldBe Seq("1") + sorts.map(_.field.name) shouldBe Seq("1") + } + + it should "not read an ARITHMETIC or FUNCTION sort as an ordinal" in { + // The discriminator guard (AD-8). MEASURED chains: `a + 1` / `1 + 1` / `2 * 1` -> + // List(ArithmeticExpression); `ABS(a)` -> List(MathematicalFunctionWithOp). None is an ordinal, + // so none may be resolved against the SELECT list -- and `1 + 1` in particular must NOT become + // "the 2nd SELECT item". + parsed("SELECT a, b FROM t ORDER BY 1 + 1").orderBy.map(_.sorts.map(_.name)) shouldBe + Some(Seq("1 + 1")) + parsed("SELECT a FROM t ORDER BY a + 1").orderBy.map(_.sorts.map(_.name)) shouldBe + Some(Seq("a + 1")) + parsed("SELECT a FROM t ORDER BY 2 * 1").orderBy.map(_.sorts.map(_.name)) shouldBe + Some(Seq("2 * 1")) + parsed("SELECT a FROM t ORDER BY ABS(a)").orderBy.map(_.sorts.map(_.name)) shouldBe + Some(Seq("ABS(a)")) + } + + it should "not read a GROUP BY function or quoted expression as an ordinal" in { + bucketPaths("SELECT SUBSTRING(a, 1, 3) AS s FROM t GROUP BY SUBSTRING(a, 1, 3)") shouldBe + Seq("") + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/ReturnsRowsSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/ReturnsRowsSpec.scala index 3800fa44e..b40afc820 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/ReturnsRowsSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/ReturnsRowsSpec.scala @@ -80,4 +80,29 @@ class ReturnsRowsSpec extends AnyFlatSpec with Matchers { single("SELECT id FROM t LIMIT 5").returnsRows shouldBe true single("SELECT COUNT(*) AS cnt FROM t LIMIT 5").returnsRows shouldBe false } + + // ---- issue #253: an explicit GROUP BY is aggregation-shaped, aggregate or not ---- + + it should "be false for a GROUP BY with NO aggregate anywhere (issue #253)" in { + val s = single("SELECT category FROM t GROUP BY category") + s.windowRowQuery shouldBe false + s.returnsRows shouldBe false + } + + it should "be false for a multi-column aggregate-free GROUP BY" in { + single("SELECT country, category FROM t GROUP BY country, category").returnsRows shouldBe false + } + + it should "be false for an aggregate-free GROUP BY with an explicit LIMIT" in { + single("SELECT category FROM t GROUP BY category LIMIT 100").returnsRows shouldBe false + } + + it should "be false for an aggregate-free GROUP BY over a scripted bucket" in { + single("SELECT UPPER(country) AS c FROM t GROUP BY UPPER(country)").returnsRows shouldBe false + } + + it should "stay true for a projection with no GROUP BY and no aggregate" in { + // The guard must not leak: only an explicit GROUP BY flips the shape. + single("SELECT category FROM t").returnsRows shouldBe true + } } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index 0eda28a8c..70b594ad3 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -23,6 +23,7 @@ import app.softnetwork.elastic.client.bulk._ import app.softnetwork.elastic.client.result.{ElasticFailure, ElasticSuccess} import app.softnetwork.elastic.client.spi.ElasticClientFactory import app.softnetwork.elastic.scalatest.ElasticDockerTestKit +import app.softnetwork.elastic.sql.query.SelectStatement import app.softnetwork.persistence.generateUUID import org.scalatest.flatspec.AnyFlatSpecLike import org.scalatest.matchers.should.Matchers @@ -32,6 +33,11 @@ import scala.language.implicitConversions case class CategoryCount(category: String, cnt: Long) +// Issue #253 result shapes: an aggregate-free GROUP BY projects the bucket keys only. +case class CategoryOnly(category: String) +case class CategoryAmount(category: String, amount: Int) +case class CategoryAliased(cat: String) + /** Regression test for issue #205: `GROUP BY` with no `LIMIT` must return EVERY group. * * The failure mode this guards against is SILENT: without an explicit `size` on the `terms` @@ -155,4 +161,127 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit fail(s"Query failed: ${error.message}") } } + + // ------------------------------------------------------------------ + // Issue #253 -- GROUP BY with NO aggregate in the SELECT list returns + // one row per GROUP, not one row per DOCUMENT. + // + // Before the fix the statement was row-shaped (`returnsRows` was true because `sqlAggregations` + // was empty), so it was routed to the scroll / capped-scroll path and came back with + // per-document rows -- 703 here, and 10,000 on the reporter's 200k fixture. The oracle below is + // the GROUP count (37), deliberately different from BOTH the document count (703) and the ES + // terms default (10), so neither failure mode can pass. + // ------------------------------------------------------------------ + + "GROUP BY with no aggregate" should "return exactly one row per group" in { + client.searchAs[CategoryOnly]( + "SELECT category FROM group_by_completeness GROUP BY category" + ) match { + case ElasticSuccess(rows) => + rows should have size categories.toLong // 37 groups, not 703 documents + rows.map(_.category).distinct should have size categories.toLong + rows.map(_.category).toSet shouldBe (1 to categories).map(c => f"cat_$c%02d").toSet + log.info(s"OK ${rows.size} groups from $index (documents indexed: $totalDocs)") + + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "still bound the group count with an explicit LIMIT" in { + // LIMIT on a GROUP BY is the terms SIZE (a bucket count), aggregate or not -- unchanged + // semantics, pinned so the #253 fix cannot drift it into a row count. + client.searchAs[CategoryOnly]( + "SELECT category FROM group_by_completeness GROUP BY category LIMIT 5" + ) match { + case ElasticSuccess(rows) => rows should have size 5 + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "return one row per COMBINATION for a multi-column aggregate-free GROUP BY" in { + // `cat_i` holds exactly i docs with amounts 1..i, so every document IS a distinct + // (category, amount) pair: the exact oracle is `totalDocs` = sum(1..37) = 703. + client.searchAs[CategoryAmount]( + "SELECT category, amount FROM group_by_completeness GROUP BY category, amount" + ) match { + case ElasticSuccess(rows) => + rows should have size totalDocs.toLong + rows.map(r => (r.category, r.amount)).distinct should have size totalDocs.toLong + + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "order an aggregate-free GROUP BY by the bucket key" in { + client.searchAs[CategoryOnly]( + "SELECT category FROM group_by_completeness GROUP BY category ORDER BY category DESC LIMIT 3" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category) shouldBe (0 until 3).map(i => f"cat_${categories - i}%02d") + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "resolve an ordinal GROUP BY / ORDER BY to the n-th SELECT item (OQ-1)" in { + // Tableau's own shape: `... GROUP BY 1 ORDER BY 1 ASC`. + // ⚠️ RED pre-fix for a NON-obvious reason, which is what makes it a strong gate: the + // `GROUP BY 1` already resolved, but the `ORDER BY 1` was silently dropped, so the terms + // aggregation fell back to its default doc_count-descending order and `LIMIT 3` returned the + // THREE BIGGEST categories (cat_37, cat_36, cat_35 -- `cat_i` holds `i` docs) instead of the + // three smallest by key. The assertion is on the KEY order, which is exact on a multi-shard + // index; a doc_count-ordered top-N would be shard-approximate. + client.searchAs[CategoryOnly]( + "SELECT category FROM group_by_completeness GROUP BY 1 ORDER BY 1 ASC LIMIT 3" + ) match { + case ElasticSuccess(rows) => + rows.map(_.category) shouldBe Seq("cat_01", "cat_02", "cat_03") + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "resolve a GROUP BY select-alias to the aliased field (FOLD-IN 2)" in { + // Pre-fix failure mode: terms on the non-existent field "cat" => ZERO rows with HTTP 200 -- + // the strongest possible RED (an empty success, not an error). The 37-group oracle is the same + // as the plain #253 test's, so the two must agree exactly. The result binds the ALIAS (the + // bucket is named "cat" -- name = identifier.fieldAlias), hence CategoryAliased. + client.searchAs[CategoryAliased]( + "SELECT category AS cat FROM group_by_completeness GROUP BY cat" + ) match { + case ElasticSuccess(rows) => + rows should have size categories.toLong + rows.map(_.cat).toSet shouldBe (1 to categories).map(c => f"cat_$c%02d").toSet + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "group by a row-invariant constant, which is exactly ONE group (FOLD-IN 1)" in { + // The Tableau capability-probe shape (`SELECT SUM(1) AS COL, 2 AS COL2 ... GROUP BY 2`) reduced + // to what makes it work: a bucket whose identifier is a CONSTANT has no field to name, so it + // must be emitted as a SCRIPTED `terms`. Before that, the emitted aggregation carried neither + // `field` nor `script` and Elasticsearch rejects such a request outright -- which is why this + // assertion has to run against a real cluster and not against the generated JSON alone. + // A constant is the same for every document, so the oracle is ONE group over all 703 of them. + // ⚠️ Asserted on the RAW row, not through `searchAs`: the macro types a bare integer literal + // as BIGINT and therefore demands a `Long` field, while the value arrives as Jackson's + // smallest type (an Integer), so the generated decoder rejects it. That binding mismatch is + // pre-existing for any literal column and is recorded separately -- it must not be allowed to + // hide whether the AGGREGATION is right, which is what this test is for. + implicit val ctx: ConversionContext = NativeContext + client.search( + SelectStatement("SELECT 2 AS flag FROM group_by_completeness GROUP BY flag") + ) match { + case ElasticSuccess(response) => + response.results should have size 1L + response.results.head.keys.toSeq shouldBe Seq("flag") + response.results.head("flag").toString shouldBe "2" + + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } } From 567446515078f6af08e678efde3a0f1c3f63658e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Mon, 7 Sep 2026 08:17:46 +0200 Subject: [PATCH 2/4] fix(sql,core,bridge): resolve GROUP BY aliases without desyncing the bucket key, and project grouped constants (story 21.3) Follow-up to the independent review of `feature/21.3`, plus the lead's OQ-G ruling. FOUR REGRESSIONS against main, all one mechanism: the alias resolution fixed the terms FIELD and desynchronised everything keyed off the bucket's DISPLAY name. Every bridge JSON test passed, because none exercised an aliased ORDER BY or HAVING -- the coverage gap and the defect had the same shape. Measured before -> after on the emitted query: - `GROUP BY cat ORDER BY cat ASC LIMIT 3` lost its terms `order` entirely -- with a LIMIT that is a DIFFERENT SET of groups, returned with HTTP 200. `sorts` is keyed by the sort's name while `buildBuckets` looks the direction up under the bucket's resolved column. `ElasticAggregation` (the METRIC path) has had a three-way fallback for ever; `buildBuckets` had only the first lookup, which is exactly why metric sorts worked and bucket sorts did not. `FieldSort.update` now resolves a sort that names a bucket, and `BucketOrder` mirrors the fallback into both bridges. - `GROUP BY cat HAVING cat <> 'x'` lost its terms `exclude`: `Expression.includes` matches the attached bucket by DISPLAY name, and `bucketNames`' copy was named `category` while the real bucket was named `cat`. The copy now carries the SELECT alias, from the same `fieldAliases` map `Identifier.update` reads, and the bucket is indexed under its output name too. - `SELECT 2 AS "COL 2" ... GROUP BY "COL 2"` rendered `GROUP BY COL 2`, which does not re-parse -- the exact defect class the render rule was introduced to fix, one alias away. A substituted bucket now re-emits its alias through the alias's own quoting. - `GROUP BY p` over `? AS p` (a JDBC PreparedStatement parameter) and `RANDOM AS r` emitted a `terms` with neither `field` nor `script` -- an ES 400. The rule is now stated once with no "except": a bucket with no field name must be scripted. Scoping matters: resolving EVERY ORDER BY alias re-pointed a window `ORDER BY hire_date` inside `OVER (...)` onto `CAST(hire_date AS DATE)` and broke Superset's `ORDER BY "Revenue"`. The resolution is scoped to a name that IS a bucket's output name; metric sorts keep their own fallback. FOLD-IN 1 case (B), per the lead's ruling: `SELECT category, 2 AS flag FROM t GROUP BY category` now returns `flag = 2` on every row. `SingleSearch.rowInvariantProjection` carries the value from the AST and `SearchApi` merges it into aggregation rows at the two dispatch points that have both the statement and the assembled rows -- the client entry points are shared with the scroll pages and carry no statement. Guarded on `!returnsRows`, so the row path is untouched. The first wiring was a silent no-op that only the integration leg caught: `rowNormalizer` null-fills every requested column first, so the constant's key already existed carrying `null` and a "never overwrite an existing key" guard projected nothing. It now fills a null placeholder and still never overwrites a value Elasticsearch computed. Measured divergence, recorded rather than papered over: the same constant is `2` on the aggregation path and `List(2)` on the row path -- the pre-existing `script_fields` array wrap. The correct side is deliberately NOT wrapped to match; the divergence is pinned as a defect-pin. Also: the AC-2 integration oracle could not fail (703 rows over 703 documents, green before and after) and is replaced by a grouping whose key repeats; elastic4s 6 renders a single-value terms `exclude` as a bare string, so the hand-maintained es6 spec carries its own spelling; and `es6/testkit/.../EmbeddedElasticTestKit.scala` is tracked despite the directory being gitignored. Verified: 1,408 harvested statements through parsers from both trees -- 33 verdict changes, every one a GROUP BY / ORDER BY shape, inputs reaching the parser's NonFatal boundary still 0. sql 821, core 903, bridge 197, es6bridge 197 on 2.13.16 and 2.12.20; `GroupByCompletenessSpec` 15/15 on real ES 6.8 (rest + jest), 7.17, 8.18 and 9.0, with 31/31 sibling guards on ES 8. --- .../sql/bridge/ElasticAggregation.scala | 34 ++++- .../elastic/sql/SQLQuerySpec.scala | 89 +++++++++++++ .../elastic/client/ElasticConversion.scala | 33 +++++ .../elastic/client/SearchApi.scala | 23 +++- .../client/ElasticConversionSpec.scala | 46 +++++++ documentation/sql/dql_statements.md | 10 +- .../sql/bridge/ElasticAggregation.scala | 34 ++++- .../elastic/sql/SQLQuerySpec.scala | 91 +++++++++++++ .../elastic/sql/query/GroupBy.scala | 103 +++++++++------ .../elastic/sql/query/OrderBy.scala | 53 ++++++-- .../elastic/sql/query/package.scala | 94 +++++++++---- .../sql/parser/ParserTotalitySpec.scala | 4 +- .../elastic/sql/query/GroupByFoldInSpec.scala | 115 +++++++++++----- .../sql/query/GroupByOrdinalSpec.scala | 9 +- .../client/GroupByCompletenessSpec.scala | 123 +++++++++++++++++- 15 files changed, 727 insertions(+), 134 deletions(-) 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 f71e92e83..be8b17d44 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 @@ -108,7 +108,33 @@ case class ElasticAggregation( def hasTransformExtendedStats: Boolean = ScriptedExtendedStatsAggregation.existsIn(Seq(agg)) } +/** The terms `order` a bucket asks for, looked up under EVERY spelling the sort could have been + * written with -- the resolved column name, the bucket's output name, and its SELECT alias. + * + * The metric path (`ElasticAggregation.apply`) has had this three-way fallback all along; the + * BUCKET path had only the first, so a sort naming the SELECT alias of a grouped column silently + * produced NO `order` at all: `SELECT country AS pays FROM t GROUP BY country ORDER BY pays` came + * back in an arbitrary doc_count order with HTTP 200, and with a LIMIT that is a different SET of + * groups. `FieldSort.update` now resolves the alias so the first lookup already matches; this + * mirrors the metric path so the two cannot disagree, and it closes the pre-existing spelling too. + */ +private[bridge] object BucketOrder { + def apply( + bucketsDirection: Map[String, SortOrder], + bucket: app.softnetwork.elastic.sql.query.Bucket + ): Option[SortOrder] = + bucketsDirection + .get(bucket.identifier.identifierName) + .orElse(bucketsDirection.get(bucket.name)) + .orElse(bucket.identifier.fieldAlias.flatMap(bucketsDirection.get)) +} + object ElasticAggregation { + private def bucketDirection( + bucketsDirection: Map[String, SortOrder], + bucket: app.softnetwork.elastic.sql.query.Bucket + ): Option[SortOrder] = BucketOrder(bucketsDirection, bucket) + def apply( sqlAgg: Field, having: Option[Criteria], @@ -470,7 +496,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted date histogram - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, calendarInterval = interval) .script(script) @@ -486,7 +512,7 @@ object ElasticAggregation { } case _ => // Standard date histogram - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, calendarInterval = interval) .field(currentBucketNestedPath) @@ -506,7 +532,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted terms aggregation - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => TermsAggregation(bucket.name) .script(script) @@ -522,7 +548,7 @@ object ElasticAggregation { } case _ => // Standard terms aggregation - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => termsAgg(bucket.name, currentBucketNestedPath) .minDocCount(1) 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 1d0766dc4..9934b7891 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -4746,4 +4746,93 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { ) } + // ---- issue #253 review round: the alias/bucket key desync, pinned on the EMITTED query ---- + // + // Every one of these was a REGRESSION against main introduced by the alias resolution, and none + // of them is visible to a parse-level assertion: the statement parses either way, and only the + // generated JSON shows the `order` / `exclude` / `script` that went missing. + + it should "keep the terms order when ORDER BY names the grouped column by its SELECT ALIAS" in { + // Regression: `sorts` was keyed by the alias (`cat`) while `buildBuckets` looked the direction + // up under the resolved column (`category`), so the `order` was silently DROPPED -- and with a + // LIMIT that is a DIFFERENT SET of groups, returned with HTTP 200. + val select: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY cat ORDER BY cat ASC LIMIT 3") + val query = select.query + println(query) + query should include(""""field":"category"""") + query should include(""""order":{"_key":"asc"}""") + } + + it should "keep the terms order for the aggregate-bearing twin" in { + val select: ElasticSearchRequest = + SelectStatement( + "SELECT category AS cat, COUNT(*) AS n FROM Table GROUP BY cat ORDER BY cat ASC LIMIT 3" + ) + select.query should include(""""order":{"_key":"asc"}""") + } + + it should "keep the terms exclude when HAVING names the grouped column by its SELECT ALIAS" in { + // Regression: the bucket `Identifier.update` attaches is matched against the real one by + // DISPLAY NAME, and the copy in `bucketNames` was named `category` while the real bucket was + // named `cat`, so `Expression.includes` stopped matching and the user's filter vanished. + val select: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY cat HAVING cat <> 'x'") + val query = select.query + println(query) + query should include(""""field":"category"""") + query should include(""""exclude":["x"]""") + } + + it should "resolve an ordinal GROUP BY that an aliased ORDER BY or HAVING then names" in { + // Both were a loud `Left` on main and must not become accepted-and-silently-unordered / + // accepted-and-silently-unfiltered. + val ordered: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY 1 ORDER BY cat ASC") + ordered.query should include(""""order":{"_key":"asc"}""") + + val filtered: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY 1 HAVING cat <> 'x'") + filtered.query should include(""""exclude":["x"]""") + } + + it should "script EVERY bucket that has no field name, not only the constant ones" in { + // An Elasticsearch `terms` aggregation must specify a `field` or a `script`. A bucket resolved + // onto a nameless identifier has no field to give, and `Value` inherits + // `shouldBeScripted = false`, so scoping the rule to row-INVARIANT literals left a fieldless, + // scriptless `terms` for every other nameless Value -- an ES 400. A JDBC `PreparedStatement` + // parameter, aliased and grouped, is the realistic route. + val param: ElasticSearchRequest = SelectStatement("SELECT ? AS p FROM Table GROUP BY p") + println(param.query) + param.query should include(""""script":{"lang":"painless","source":"params.paramValue"}""") + + val random: ElasticSearchRequest = SelectStatement("SELECT RANDOM AS r FROM Table GROUP BY r") + random.query should include(""""script":{"lang":"painless","source":"Math.random()"}""") + + val decimal: ElasticSearchRequest = SelectStatement("SELECT 2.5 AS d FROM Table GROUP BY d") + decimal.query should include(""""script":{"lang":"painless","source":"2.5"}""") + + val arith: ElasticSearchRequest = SelectStatement("SELECT 2 + 0 AS c FROM Table GROUP BY c") + arith.query should include(""""script":{"lang":"painless","source":"2 + 0"}""") + } + + it should "not emit a terms aggregation that carries neither field nor script" in { + // The invariant behind the test above, stated once over every shape that reaches a bucket. + Seq( + "SELECT ? AS p FROM Table GROUP BY p", + "SELECT RANDOM AS r FROM Table GROUP BY r", + "SELECT 2.5 AS d FROM Table GROUP BY d", + "SELECT 2 + 0 AS c FROM Table GROUP BY c", + "SELECT 2 AS n FROM Table GROUP BY n", + "SELECT 'x' AS lbl FROM Table GROUP BY lbl", + "SELECT SUM(1) AS COL, 2 AS COL2 FROM Table GROUP BY 2", + "SELECT UPPER(country) AS u FROM Table GROUP BY u" + ).foreach { sql => + val q: ElasticSearchRequest = SelectStatement(sql) + withClue(s"[$sql] ${q.query}: ") { + q.query.contains("\"field\"") || q.query.contains("\"script\"") shouldBe true + } + } + } + } diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala b/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala index 6342ccebb..c75f47a3c 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala @@ -223,6 +223,39 @@ trait ElasticConversion { } } + /** Merge a statement's ROW-INVARIANT SELECT items into aggregation rows (issue #253, FOLD-IN 1). + * + * `SELECT category, 2 AS flag FROM t GROUP BY category` is standard SQL -- a constant does not + * vary within a group -- but an aggregation response carries NO hits, so the `script_fields` + * entry the constant is emitted as is never fetched (`"size": 0`) and `rowNormalizer` null-fills + * the requested column. MEASURED on real ES 8.18 before this: `flag` came back `null` on every + * row. The value is taken from the AST instead and added here, on the RESULT side. + * + * Deliberately NOT applied to the row path: there a constant already arrives through + * `script_fields` and works, and double-handling it would be the divergence this exists to + * remove. + * + * 🔴 A key that is already PRESENT but `null` is filled, not skipped. `rowNormalizer` runs first + * and null-fills every requested output column, so by the time the rows get here the constant's + * column already exists carrying `null` -- a "never overwrite an existing key" rule reads that + * placeholder as a real value and projects nothing at all (measured: the column stayed `null` on + * real ES 8.18 with the projection wired in). A NON-null value is still never overwritten, so a + * column Elasticsearch actually computed always wins over a constant of the same name. Existing + * column ORDER is preserved -- `rowNormalizer` has already put them in SELECT order. + */ + protected def projectRowInvariants( + rows: Seq[ListMap[String, Any]], + constants: ListMap[String, Any] + ): Seq[ListMap[String, Any]] = + if (constants.isEmpty) rows + else + rows.map { row => + val filled = row.map { case (k, v) => + k -> (if (v == null) constants.getOrElse(k, v) else v) + } + filled ++ constants.filterNot { case (k, _) => filled.contains(k) } + } + /** convert JsonNode to Rows */ def jsonToRows( diff --git a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala index 72ebdbb18..d5abaea1b 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala @@ -269,7 +269,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { single.sqlAggregations, extractOutputFieldNames(single), single.nestedHitsMappings - ) + ).map(withRowInvariants(single, _)) } case parsed: MultiSearch => @@ -343,6 +343,25 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { * @return * the Elasticsearch response */ + /** Add a statement's ROW-INVARIANT SELECT items to an AGGREGATION response's rows (#253, FOLD-IN + * 1) -- `SELECT category, 2 AS flag ... GROUP BY category` must return `flag = 2`, not the + * `null` an aggregation response leaves behind (measured on real Elasticsearch: the + * `script_fields` entry a constant is emitted as is never fetched under `"size": 0`). + * + * Guarded on `!returnsRows`, so it applies to the aggregation path ONLY: on the row path the + * constant already arrives through `script_fields` and must not be handled twice. This is the + * one seam that has BOTH the statement and the assembled rows -- the client entry points + * (`parseSingleSearchResponse` and friends) are shared with the scroll pages and carry no + * statement, which is why the projection is applied here rather than threaded through them. + */ + private def withRowInvariants( + single: SingleSearch, + response: ElasticResponse + ): ElasticResponse = + if (single.returnsRows || single.rowInvariantProjection.isEmpty) response + else + response.copy(results = projectRowInvariants(response.results, single.rowInvariantProjection)) + def singleSearch( elasticQuery: ElasticQuery, fieldAliases: ListMap[String, String], @@ -616,7 +635,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { single.sqlAggregations, extractOutputFieldNames(single), single.nestedHitsMappings - ) + ).map(_.map(withRowInvariants(single, _)))(ExecutionContext.global) } case parsed: MultiSearch => diff --git a/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala index 9d873acbf..9c0c0164c 100644 --- a/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala +++ b/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala @@ -1624,6 +1624,52 @@ class ElasticConversionSpec extends AnyFlatSpec with Matchers with ElasticConver // egress unless `elastic.include-document-id` is enabled or `_id` is selected. // ------------------------------------------------------------------------- + // ---- issue #253 FOLD-IN 1: row-invariant constants are projected into AGGREGATION rows ---- + // + // An aggregation response carries no hits, so the `script_fields` entry a constant is emitted as + // is never fetched (`"size": 0`) and `rowNormalizer` null-fills the requested column -- measured + // on real ES 8.18, `SELECT category, 2 AS flag ... GROUP BY category` returned `flag = null` on + // every row. `SearchApi` merges the AST value in on the result side instead. + + "projectRowInvariants" should "add every constant to every aggregation row" in { + val rows = Seq( + ListMap[String, Any]("category" -> "a"), + ListMap[String, Any]("category" -> "b") + ) + projectRowInvariants(rows, ListMap[String, Any]("flag" -> 2L, "lbl" -> "x")) shouldBe Seq( + ListMap[String, Any]("category" -> "a", "flag" -> 2L, "lbl" -> "x"), + ListMap[String, Any]("category" -> "b", "flag" -> 2L, "lbl" -> "x") + ) + } + + it should "return the rows UNTOUCHED when there is no constant" in { + val rows = Seq(ListMap[String, Any]("category" -> "a")) + projectRowInvariants(rows, ListMap.empty) should be theSameInstanceAs rows + } + + it should "FILL a column that rowNormalizer already null-filled, keeping its position" in { + // 🔴 This is the case that made the first wiring a no-op: `rowNormalizer` runs first and + // null-fills every requested output column, so the constant's key is already present carrying + // `null` by the time the projection sees the row. Skipping present keys therefore projected + // NOTHING -- measured on real ES 8.18, `flag` stayed null. + val rows = Seq(ListMap[String, Any]("category" -> "a", "flag" -> null)) + val out = projectRowInvariants(rows, ListMap[String, Any]("flag" -> 2L)) + out shouldBe Seq(ListMap[String, Any]("category" -> "a", "flag" -> 2L)) + out.head.keys.toSeq shouldBe Seq("category", "flag") // SELECT order preserved + } + + it should "never overwrite a real column with a constant of the same name" in { + // A projected constant is presentation; a value Elasticsearch actually computed always wins. + val rows = Seq(ListMap[String, Any]("flag" -> "from-elasticsearch")) + projectRowInvariants(rows, ListMap[String, Any]("flag" -> 2L)) shouldBe + Seq(ListMap[String, Any]("flag" -> "from-elasticsearch")) + } + + it should "produce no row where there was none" in { + // A grouping that matched nothing stays empty -- the constant must not invent a row. + projectRowInvariants(Seq.empty, ListMap[String, Any]("flag" -> 2L)) shouldBe empty + } + private object EnabledDocumentIdConversion extends ElasticConversion { override protected def includeDocumentId: Boolean = true } diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index b63101473..b8fc1a016 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -661,12 +661,14 @@ GROUP BY profile.city; that names nothing (`0`, a negative, or one past the end of the `SELECT` list) is a parse error naming the position. A column genuinely named with digits is unaffected, and a column named `1` is addressed as `` GROUP BY `1` ``. -- `SELECT` aliases are supported: `SELECT country AS pays ... GROUP BY pays` groups by `country`. -- Grouping **by** a constant is legal and means exactly one group +- `SELECT` aliases are supported: `SELECT country AS pays ... GROUP BY pays` groups by `country`, + and `ORDER BY pays` / `HAVING pays <> 'x'` address that group by the same name. +- A constant is legal beside a `GROUP BY` and carries its value on every row + (`SELECT category, 2 AS flag FROM t GROUP BY category`) — it does not vary within a group, so it + needs no grouping. +- Grouping **by** a constant is also legal and means exactly one group (`SELECT 2 AS flag ... GROUP BY flag`, or the equivalent position `... GROUP BY 1`). It needs a `SELECT` alias, because the alias is the only name that group can be given. -- A constant that is *not* grouped still has to be grouped like any other column: - `SELECT category, 2 AS flag ... GROUP BY category` is rejected. - `LIMIT` on a `GROUP BY` bounds the number of **groups**, not the number of rows — it is pushed down as the Elasticsearch `terms` size. On a multi-column `GROUP BY` it bounds **each level**, so the row count can exceed it. diff --git a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index 99812237f..1ad042b58 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 @@ -108,7 +108,33 @@ case class ElasticAggregation( def hasTransformExtendedStats: Boolean = ScriptedExtendedStatsAggregation.existsIn(Seq(agg)) } +/** The terms `order` a bucket asks for, looked up under EVERY spelling the sort could have been + * written with -- the resolved column name, the bucket's output name, and its SELECT alias. + * + * The metric path (`ElasticAggregation.apply`) has had this three-way fallback all along; the + * BUCKET path had only the first, so a sort naming the SELECT alias of a grouped column silently + * produced NO `order` at all: `SELECT country AS pays FROM t GROUP BY country ORDER BY pays` came + * back in an arbitrary doc_count order with HTTP 200, and with a LIMIT that is a different SET of + * groups. `FieldSort.update` now resolves the alias so the first lookup already matches; this + * mirrors the metric path so the two cannot disagree, and it closes the pre-existing spelling too. + */ +private[bridge] object BucketOrder { + def apply( + bucketsDirection: Map[String, SortOrder], + bucket: app.softnetwork.elastic.sql.query.Bucket + ): Option[SortOrder] = + bucketsDirection + .get(bucket.identifier.identifierName) + .orElse(bucketsDirection.get(bucket.name)) + .orElse(bucket.identifier.fieldAlias.flatMap(bucketsDirection.get)) +} + object ElasticAggregation { + private def bucketDirection( + bucketsDirection: Map[String, SortOrder], + bucket: app.softnetwork.elastic.sql.query.Bucket + ): Option[SortOrder] = BucketOrder(bucketsDirection, bucket) + def apply( sqlAgg: Field, having: Option[Criteria], @@ -466,7 +492,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted date histogram - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, interval = interval) .script(script) @@ -482,7 +508,7 @@ object ElasticAggregation { } case _ => // Standard date histogram - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, interval = interval) .field(currentBucketNestedPath) @@ -502,7 +528,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted terms aggregation - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => TermsAggregation(bucket.name) .script(script) @@ -518,7 +544,7 @@ object ElasticAggregation { } case _ => // Standard terms aggregation - bucketsDirection.get(bucket.identifier.identifierName) match { + bucketDirection(bucketsDirection, bucket) match { case Some(direction) => termsAgg(bucket.name, currentBucketNestedPath) .minDocCount(1) 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 a21789d34..d4e0cf06e 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 @@ -4805,4 +4805,95 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { ) } + // ---- issue #253 review round: the alias/bucket key desync, pinned on the EMITTED query ---- + // + // Every one of these was a REGRESSION against main introduced by the alias resolution, and none + // of them is visible to a parse-level assertion: the statement parses either way, and only the + // generated JSON shows the `order` / `exclude` / `script` that went missing. + + it should "keep the terms order when ORDER BY names the grouped column by its SELECT ALIAS" in { + // Regression: `sorts` was keyed by the alias (`cat`) while `buildBuckets` looked the direction + // up under the resolved column (`category`), so the `order` was silently DROPPED -- and with a + // LIMIT that is a DIFFERENT SET of groups, returned with HTTP 200. + val select: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY cat ORDER BY cat ASC LIMIT 3") + val query = select.query + println(query) + query should include(""""field":"category"""") + query should include(""""order":{"_key":"asc"}""") + } + + it should "keep the terms order for the aggregate-bearing twin" in { + val select: ElasticSearchRequest = + SelectStatement( + "SELECT category AS cat, COUNT(*) AS n FROM Table GROUP BY cat ORDER BY cat ASC LIMIT 3" + ) + select.query should include(""""order":{"_key":"asc"}""") + } + + it should "keep the terms exclude when HAVING names the grouped column by its SELECT ALIAS" in { + // Regression: the bucket `Identifier.update` attaches is matched against the real one by + // DISPLAY NAME, and the copy in `bucketNames` was named `category` while the real bucket was + // named `cat`, so `Expression.includes` stopped matching and the user's filter vanished. + val select: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY cat HAVING cat <> 'x'") + val query = select.query + println(query) + query should include(""""field":"category"""") + // elastic4s 6 renders a single-value terms exclude as a bare string; 7+ renders an array. + query should include(""""exclude":"x""") + } + + it should "resolve an ordinal GROUP BY that an aliased ORDER BY or HAVING then names" in { + // Both were a loud `Left` on main and must not become accepted-and-silently-unordered / + // accepted-and-silently-unfiltered. + val ordered: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY 1 ORDER BY cat ASC") + ordered.query should include(""""order":{"_key":"asc"}""") + + val filtered: ElasticSearchRequest = + SelectStatement("SELECT category AS cat FROM Table GROUP BY 1 HAVING cat <> 'x'") + // elastic4s 6 renders a single-value terms exclude as a bare string; 7+ renders an array. + filtered.query should include(""""exclude":"x""") + } + + it should "script EVERY bucket that has no field name, not only the constant ones" in { + // An Elasticsearch `terms` aggregation must specify a `field` or a `script`. A bucket resolved + // onto a nameless identifier has no field to give, and `Value` inherits + // `shouldBeScripted = false`, so scoping the rule to row-INVARIANT literals left a fieldless, + // scriptless `terms` for every other nameless Value -- an ES 400. A JDBC `PreparedStatement` + // parameter, aliased and grouped, is the realistic route. + val param: ElasticSearchRequest = SelectStatement("SELECT ? AS p FROM Table GROUP BY p") + println(param.query) + param.query should include(""""script":{"lang":"painless","source":"params.paramValue"}""") + + val random: ElasticSearchRequest = SelectStatement("SELECT RANDOM AS r FROM Table GROUP BY r") + random.query should include(""""script":{"lang":"painless","source":"Math.random()"}""") + + val decimal: ElasticSearchRequest = SelectStatement("SELECT 2.5 AS d FROM Table GROUP BY d") + decimal.query should include(""""script":{"lang":"painless","source":"2.5"}""") + + val arith: ElasticSearchRequest = SelectStatement("SELECT 2 + 0 AS c FROM Table GROUP BY c") + arith.query should include(""""script":{"lang":"painless","source":"2 + 0"}""") + } + + it should "not emit a terms aggregation that carries neither field nor script" in { + // The invariant behind the test above, stated once over every shape that reaches a bucket. + Seq( + "SELECT ? AS p FROM Table GROUP BY p", + "SELECT RANDOM AS r FROM Table GROUP BY r", + "SELECT 2.5 AS d FROM Table GROUP BY d", + "SELECT 2 + 0 AS c FROM Table GROUP BY c", + "SELECT 2 AS n FROM Table GROUP BY n", + "SELECT 'x' AS lbl FROM Table GROUP BY lbl", + "SELECT SUM(1) AS COL, 2 AS COL2 FROM Table GROUP BY 2", + "SELECT UPPER(country) AS u FROM Table GROUP BY u" + ).foreach { sql => + val q: ElasticSearchRequest = SelectStatement(sql) + withClue(s"[$sql] ${q.query}: ") { + q.query.contains("\"field\"") || q.query.contains("\"script\"") shouldBe true + } + } + } + } 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 0d09bae3f..5d04ad4e2 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 @@ -19,7 +19,9 @@ package app.softnetwork.elastic.sql.query import app.softnetwork.elastic.sql.`type`.SQLType import app.softnetwork.elastic.sql.operator._ import app.softnetwork.elastic.sql.{ + quoteIdentifier, Expr, + GenericIdentifier, Identifier, LongValue, PainlessContext, @@ -102,16 +104,34 @@ object Bucket { else None - /** The spelling a resolved bucket must RE-EMIT, when its own render would not re-parse to it. + /** How a bucket that was SUBSTITUTED -- an ordinal `GROUP BY ` or a `GROUP BY resolves to the aliased field ---- @@ -223,12 +269,17 @@ class GroupByFoldInSpec extends AnyFlatSpec with Matchers { s.bucketNames.keys.toSeq should contain("country") } - it should "normalise the alias to the resolved column on render, with the render fixed point" in { + it should "re-emit the alias the bucket was resolved through, and re-parse to the same bucket" in { + // The render keeps the spelling the statement used (`GROUP BY pays`), NOT the resolved column: + // the alias is what `Bucket.aliasItem` reads back to the same SELECT item, it is what the user + // wrote, and it matches what `origin/main` rendered -- so an existing MATERIALIZED VIEW whose + // definition groups by an alias does not see its stored render change and rebuild. val rendered = Parser("SELECT country AS pays FROM t GROUP BY pays") .map(_.sql) .getOrElse(fail("did not parse")) - rendered should include("GROUP BY country") + rendered should include("GROUP BY pays") Parser(rendered).map(_.sql).getOrElse("") shouldBe rendered + bucketTriples(rendered) shouldBe bucketTriples("SELECT country AS pays FROM t GROUP BY country") } it should "leave a bucket that matches no alias alone" in { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala index a48c541ad..8f4d2fbca 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByOrdinalSpec.scala @@ -255,8 +255,11 @@ class GroupByOrdinalSpec extends AnyFlatSpec with Matchers { Some(Seq("ABS(a)")) } - it should "not read a GROUP BY function or quoted expression as an ordinal" in { - bucketPaths("SELECT SUBSTRING(a, 1, 3) AS s FROM t GROUP BY SUBSTRING(a, 1, 3)") shouldBe - Seq("") + it should "not read a GROUP BY function as an ordinal" in { + // Assert the PREDICATE, not the empty `path` an expression bucket happens to have -- that + // string is an artifact of `sourceBucket` on a nameless identifier, not the intent. + val s = parsed("SELECT SUBSTRING(a, 1, 3) AS s FROM t GROUP BY SUBSTRING(a, 1, 3)") + Bucket.ordinalOf(s.buckets.head.identifier) shouldBe None + s.buckets.head.ordinal shouldBe None } } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index 70b594ad3..d2b92ed8a 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -37,6 +37,7 @@ case class CategoryCount(category: String, cnt: Long) case class CategoryOnly(category: String) case class CategoryAmount(category: String, amount: Int) case class CategoryAliased(cat: String) +case class AmountOnly(amount: Int) /** Regression test for issue #205: `GROUP BY` with no `LIMIT` must return EVERY group. * @@ -200,14 +201,36 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } it should "return one row per COMBINATION for a multi-column aggregate-free GROUP BY" in { - // `cat_i` holds exactly i docs with amounts 1..i, so every document IS a distinct - // (category, amount) pair: the exact oracle is `totalDocs` = sum(1..37) = 703. + // 🔴 The oracle has to be strictly LESS than the document count, or it cannot fail: grouping by + // (category, amount) yields exactly one pair per document here (`cat_i` holds `i` docs with + // amounts 1..i), so a row-shaped -- i.e. UNFIXED -- execution returns the same 703 rows and the + // test is green either way. Slicing to `amount <= 3` makes the amount REPEAT across categories: + // cat_01 contributes 1 pair, cat_02 two, every cat_i (i >= 3) three, so the combination count + // is 1 + 2 + 3*35 = 108 while the DOCUMENT count in the slice is the same 108... so the slice + // alone is not enough either. Grouping by `amount` ONLY is what separates them: 3 groups + // against 108 documents. client.searchAs[CategoryAmount]( - "SELECT category, amount FROM group_by_completeness GROUP BY category, amount" + "SELECT category, amount FROM group_by_completeness WHERE amount <= 3 GROUP BY category, amount" ) match { case ElasticSuccess(rows) => - rows should have size totalDocs.toLong - rows.map(r => (r.category, r.amount)).distinct should have size totalDocs.toLong + val expected = 1 + 2 + 3 * (categories - 2) // 108 distinct (category, amount) pairs + rows should have size expected.toLong + rows.map(r => (r.category, r.amount)).distinct should have size expected.toLong + + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "return one row per GROUP, not per document, when the group key repeats" in { + // The gate the test above cannot be: `amount <= 3` selects 108 documents but only THREE + // distinct amounts, so a row-shaped execution returns 108 rows and an aggregation-shaped one + // returns 3. Nothing about the fixture can make those numbers coincide. + client.searchAs[AmountOnly]( + "SELECT amount FROM group_by_completeness WHERE amount <= 3 GROUP BY amount" + ) match { + case ElasticSuccess(rows) => + rows.map(_.amount).sorted shouldBe Seq(1, 2, 3) case ElasticFailure(error) => fail(s"Query failed: ${error.message}") @@ -259,7 +282,95 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } - it should "group by a row-invariant constant, which is exactly ONE group (FOLD-IN 1)" in { + it should "project a row-invariant constant beside the group key (FOLD-IN 1, case B)" in { + // 🔴 The pre-fix failure mode is a NULL column, not an error: an aggregation response has no + // hits, so the `script_fields` entry the constant is emitted as is never fetched under + // `"size": 0`. MEASURED on real ES 8.18 before the projection landed: + // `List((cat_37,None), (cat_36,None), (cat_35,None))`. + // + // ⚠️ Asserted on the RAW row, not through `searchAs`: the macro types a bare integer literal + // as BIGINT (so an `Int` field is a compile error) while the value arrives as Jackson's + // smallest type at run time (so a `Long` field fails to decode). That binding mismatch is + // pre-existing for ANY literal column and is recorded separately -- it must not be allowed to + // hide whether the projection works, which is what this test is for. + implicit val projCtx: ConversionContext = NativeContext + client.search( + SelectStatement( + "SELECT category, 2 AS flag FROM group_by_completeness GROUP BY category" + ) + ) match { + case ElasticSuccess(response) => + response.results should have size categories.toLong + response.results.foreach { row => + row.keys.toSeq should contain("flag") + row("flag").toString shouldBe "2" + } + response.results.map(_("category").toString).toSet shouldBe + (1 to categories).map(c => f"cat_$c%02d").toSet + + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + // 🔴 PINS A KNOWN DEFECT, not a contract -- delete this test when the defect is fixed. + // + // The same constant reaches the caller DIFFERENTLY on the two paths, MEASURED on real ES 8.18: + // aggregation path (GROUP BY): `flag -> 2` -- a clean scalar, projected from the AST + // row path (no GROUP BY): `flag -> List(2)` -- array-wrapped + // + // The wrap is the pre-existing `script_fields` defect (`jsonNodeToAny` keeps an ES per-field + // array as a List and nothing unwraps it), recorded in + // docs/issues/local-21.3-script-fields-values-are-array-wrapped.md. The aggregation side is the + // CORRECT one and is deliberately NOT wrapped to match: matching would spread a defect to a path + // that does not have it. Pinned so the divergence is visible rather than discovered. + it should "expose the known row-path array wrap, which the aggregation path does not share" in { + implicit val wrapCtx: ConversionContext = NativeContext + client.search( + SelectStatement("SELECT id, 2 AS flag FROM group_by_completeness LIMIT 2") + ) match { + case ElasticSuccess(response) => + response.results.foreach { row => + withClue(s"row path, row=$row: ") { row("flag") shouldBe List(2) } + } + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "order an aggregate-free GROUP BY by a SELECT ALIAS of the grouped column" in { + // 🔴 Regression guard for the alias/bucket key desync: `SingleSearch.sorts` is keyed by the + // sort's name while `buildBuckets` looks the direction up under the bucket's resolved column, + // so an ORDER BY naming the alias silently produced NO terms `order` at all -- with a LIMIT + // that is a DIFFERENT SET of groups (the top 3 by doc_count instead of the 3 smallest keys), + // returned with HTTP 200. No integration test covered the alias spelling, which is why it + // shipped. `cat_i` holds `i` docs, so doc_count order and key order disagree by construction. + client.searchAs[CategoryAliased]( + "SELECT category AS cat FROM group_by_completeness GROUP BY cat ORDER BY cat ASC LIMIT 3" + ) match { + case ElasticSuccess(rows) => + rows.map(_.cat) shouldBe Seq("cat_01", "cat_02", "cat_03") + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "apply a HAVING over an alias-resolved bucket" in { + // 🔴 Regression guard for the same desync on the HAVING path: the bucket `Identifier.update` + // attaches is matched against the real one by DISPLAY NAME, so an aliased grouped column made + // `Expression.includes` stop matching and the terms `exclude` vanished -- the user's filter + // silently dropped and MORE rows came back. + client.searchAs[CategoryAliased]( + "SELECT category AS cat FROM group_by_completeness GROUP BY cat HAVING cat <> 'cat_01'" + ) match { + case ElasticSuccess(rows) => + rows should have size (categories - 1).toLong + rows.map(_.cat) should not contain "cat_01" + case ElasticFailure(error) => + fail(s"Query failed: ${error.message}") + } + } + + it should "group by a row-invariant constant, which is exactly ONE group (FOLD-IN 1, case A)" in { // The Tableau capability-probe shape (`SELECT SUM(1) AS COL, 2 AS COL2 ... GROUP BY 2`) reduced // to what makes it work: a bucket whose identifier is a CONSTANT has no field to name, so it // must be emitted as a SCRIPTED `terms`. Before that, the emitted aggregation carried neither From bed9ba0613b23c0a6fb19ce5007856bd210ef025 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Mon, 7 Sep 2026 09:23:24 +0200 Subject: [PATCH 3/4] fix(sql,core,bridge): place grouped constants as rows are built; drop the ORDER BY alias rewrite (story 21.3) Round 3 review findings plus the lead's ruling on where the constant projection belongs. LEAD RULING -- the projection must happen per row materialization, not as a pass over the rows. `projectRowInvariants` rebuilt every row twice, ran a `getOrElse` for every cell whether or not a constant applied, and used linear `contains`/`++` on a `ListMap` -- roughly O(rows x cols x constants) on top of rows already materialized once, over the largest collection the engine produces (`Bucket.DefaultSize` is 65,536 per level, multiplied on a multi-level grouping). The constants are now handed to the parse layer as ordinary statement-derived data, like `fieldAliases` and the output `fields`, and SEED `parseAggregations`' `parentContext`: each is placed once at the top of the recursion and carried into every leaf row by the `parentContext ++ ...` already being performed. Per-row cost is zero extra traversals and zero extra allocations. `projectRowInvariants` is deleted, so it never reaches the `ElasticConversion` trait's surface. Three consequences, measured rather than assumed: the `!returnsRows` guard became structural and disappeared (`parseAggregations` IS the aggregation path); precedence flipped so a bucket key or metric of the same name now WINS over a constant, including a computed null, which the old "never overwrite a non-null value" rule could not express; and `rowNormalizer` restores SELECT order after the seed. This subsumed two open findings -- UNION ALL legs now project because each leg seeds its own rows inside the existing per-response loop, and the un-aliased constant is keyed correctly because `Field.outputName` is now the ONE definition that both `SearchApi.extractOutputFieldNames` and `rowInvariantProjection` read. REGRESSIONS AND DEFECTS FIXED, measured before -> after: - `SELECT category, 2 FROM t GROUP BY category` (no alias) returned the requested `__c2` column as NULL and invented a bogus `2` column beside it. The two sides derived the same key differently; they now share `Field.outputName`. - `SELECT 2 AS flag, SUM(amount) AS s FROM t GROUP BY flag ORDER BY flag ASC` rendered `ORDER BY 2 ASC` and re-parsed as `ORDER BY SUM(amount) ASC` -- a bucket sort silently becoming a metric sort -- and `... GROUP BY 1 ORDER BY 1 ASC` rendered a statement that no longer parsed. `FieldSort` now carries the same substitution record `Bucket` does, with the same quoting. - The `FieldSort` bucket-alias rewrite is DELETED. Falsification showed it redundant (the bridge `BucketOrder` fallback alone keeps all eight ORDER BY shapes correct) and harmful: it was the only new code that rewrote a window's `OVER (...)` clause, and its guard keyed on a property of the STATEMENT rather than of the sort being updated. - A HAVING naming an aliased grouped column was dropped whenever the GROUP BY spelled the COLUMN, so whether a filter was honoured depended on the spelling. The repo's own "complex query" bridge fixture gained the `exclude` it always asked for. - `GROUP BY ?` over an unbound parameter scripted `params.paramValue`, which nothing binds, so Elasticsearch answered zero groups with HTTP 200. Now rejected. - A grouped query no longer emits a `script_fields` block: under `"size": 0` no hit is returned, so Elasticsearch was parsing and compiling Painless that could never be fetched. - `BucketOrder`'s third fallback was unreachable (`Bucket.name` IS `fieldAlias.getOrElse(path)`) and is dropped, in both bridge copies. Also: the two overriding registries (`ElasticClientDelegator`, `MetricsElasticClient`) carry the new defaulted parameter; the AC-2 integration oracle was unfalsifiable (703 rows over 703 documents) and is replaced by a grouping whose key repeats; and the `isRowInvariantLiteral` scaladoc, which instructed a maintainer to delete live code, describes what it does. Verified: 1,408 harvested statements through parsers from both trees -- 33 verdict changes, every one a GROUP BY / ORDER BY shape, internal faults 0. sql 823, core 903, bridge 197, es6bridge 197 on 2.13.16 and 2.12.20; `GroupByCompletenessSpec` 18/18 on real ES 6.8 (rest + jest), 7.17, 8.18 and 9.0, with 31/31 sibling guards on ES 8. --- .../sql/bridge/ElasticAggregation.scala | 31 +++--- .../elastic/sql/SQLQuerySpec.scala | 19 ++-- .../client/ElasticClientDelegator.scala | 48 ++++++++-- .../elastic/client/ElasticConversion.scala | 88 ++++++++--------- .../elastic/client/SearchApi.scala | 92 +++++++++++------- .../client/metrics/MetricsElasticClient.scala | 48 ++++++++-- .../client/ElasticConversionSpec.scala | 94 +++++++++++++------ .../sql/bridge/ElasticAggregation.scala | 31 +++--- .../elastic/sql/SQLQuerySpec.scala | 19 ++-- .../elastic/sql/query/GroupBy.scala | 11 +++ .../elastic/sql/query/OrderBy.scala | 87 +++++++++-------- .../elastic/sql/query/Select.scala | 15 +++ .../elastic/sql/query/package.scala | 79 +++++++++++----- .../elastic/sql/query/GroupByFoldInSpec.scala | 36 ++++++- .../client/GroupByCompletenessSpec.scala | 73 +++++++++++++- 15 files changed, 534 insertions(+), 237 deletions(-) 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 be8b17d44..4f76a2f10 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 @@ -108,15 +108,20 @@ case class ElasticAggregation( def hasTransformExtendedStats: Boolean = ScriptedExtendedStatsAggregation.existsIn(Seq(agg)) } -/** The terms `order` a bucket asks for, looked up under EVERY spelling the sort could have been - * written with -- the resolved column name, the bucket's output name, and its SELECT alias. +/** The terms `order` a bucket asks for, looked up under BOTH spellings a sort can be keyed by: the + * bucket's resolved column (`identifier.identifierName`) and its OUTPUT name (`Bucket.name`, i.e. + * the SELECT alias when there is one, else the path). * - * The metric path (`ElasticAggregation.apply`) has had this three-way fallback all along; the - * BUCKET path had only the first, so a sort naming the SELECT alias of a grouped column silently + * The metric path (`ElasticAggregation.apply`) has had this fallback all along; the BUCKET path + * had only the first lookup, so a sort naming the SELECT alias of a grouped column silently * produced NO `order` at all: `SELECT country AS pays FROM t GROUP BY country ORDER BY pays` came * back in an arbitrary doc_count order with HTTP 200, and with a LIMIT that is a different SET of - * groups. `FieldSort.update` now resolves the alias so the first lookup already matches; this - * mirrors the metric path so the two cannot disagree, and it closes the pre-existing spelling too. + * groups. + * + * ⚠️ TWO spellings, not three. A third `.orElse` on `identifier.fieldAlias` would be unreachable: + * `Bucket.name` IS `identifier.fieldAlias.getOrElse(path)`, so whenever an alias exists the second + * lookup already used that key. (The metric path's three arms are NOT redundant -- they read two + * different members -- so the shape must not be copied blindly from it.) */ private[bridge] object BucketOrder { def apply( @@ -126,15 +131,9 @@ private[bridge] object BucketOrder { bucketsDirection .get(bucket.identifier.identifierName) .orElse(bucketsDirection.get(bucket.name)) - .orElse(bucket.identifier.fieldAlias.flatMap(bucketsDirection.get)) } object ElasticAggregation { - private def bucketDirection( - bucketsDirection: Map[String, SortOrder], - bucket: app.softnetwork.elastic.sql.query.Bucket - ): Option[SortOrder] = BucketOrder(bucketsDirection, bucket) - def apply( sqlAgg: Field, having: Option[Criteria], @@ -496,7 +495,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted date histogram - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, calendarInterval = interval) .script(script) @@ -512,7 +511,7 @@ object ElasticAggregation { } case _ => // Standard date histogram - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, calendarInterval = interval) .field(currentBucketNestedPath) @@ -532,7 +531,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted terms aggregation - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => TermsAggregation(bucket.name) .script(script) @@ -548,7 +547,7 @@ object ElasticAggregation { } case _ => // Standard terms aggregation - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => termsAgg(bucket.name, currentBucketNestedPath) .minDocCount(1) 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 9934b7891..c59a67b7b 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -802,6 +802,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "terms": { | "field": "products.category", | "size": 10, + | "exclude": ["coffee"], | "min_doc_count": 1 | }, | "aggs": { @@ -4802,10 +4803,9 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { // `shouldBeScripted = false`, so scoping the rule to row-INVARIANT literals left a fieldless, // scriptless `terms` for every other nameless Value -- an ES 400. A JDBC `PreparedStatement` // parameter, aliased and grouped, is the realistic route. - val param: ElasticSearchRequest = SelectStatement("SELECT ? AS p FROM Table GROUP BY p") - println(param.query) - param.query should include(""""script":{"lang":"painless","source":"params.paramValue"}""") - + // ⚠️ NOT `? AS p`: a bucket over an unbound parameter is REJECTED (it would script + // `params.paramValue`, which nothing binds, so Elasticsearch would answer zero groups with + // HTTP 200). The scripting rule below is what makes every OTHER nameless bucket work. val random: ElasticSearchRequest = SelectStatement("SELECT RANDOM AS r FROM Table GROUP BY r") random.query should include(""""script":{"lang":"painless","source":"Math.random()"}""") @@ -4819,7 +4819,6 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { it should "not emit a terms aggregation that carries neither field nor script" in { // The invariant behind the test above, stated once over every shape that reaches a bucket. Seq( - "SELECT ? AS p FROM Table GROUP BY p", "SELECT RANDOM AS r FROM Table GROUP BY r", "SELECT 2.5 AS d FROM Table GROUP BY d", "SELECT 2 + 0 AS c FROM Table GROUP BY c", @@ -4829,8 +4828,16 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { "SELECT UPPER(country) AS u FROM Table GROUP BY u" ).foreach { sql => val q: ElasticSearchRequest = SelectStatement(sql) + // Scoped INSIDE the `terms` object: asserting over the whole query lets an unrelated + // `script_fields` block satisfy it while the aggregation itself carries neither. + val terms = q.query.split("\"terms\":\\{").drop(1).map(_.takeWhile(_ != '}')) withClue(s"[$sql] ${q.query}: ") { - q.query.contains("\"field\"") || q.query.contains("\"script\"") shouldBe true + terms should not be empty + terms.foreach(t => + withClue(s"terms{$t}: ") { + t.contains("\"field\"") || t.contains("\"script\"") shouldBe true + } + ) } } } diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ElasticClientDelegator.scala b/core/src/main/scala/app/softnetwork/elastic/client/ElasticClientDelegator.scala index 48070d41f..7216d5420 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ElasticClientDelegator.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ElasticClientDelegator.scala @@ -1218,9 +1218,17 @@ trait ElasticClientDelegator extends ElasticClientApi with BulkTypes { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = - delegate.singleSearch(elasticQuery, fieldAliases, aggregations, fields, nestedHits) + delegate.singleSearch( + elasticQuery, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) /** Multi-search with Elasticsearch queries. * @@ -1238,9 +1246,17 @@ trait ElasticClientDelegator extends ElasticClientApi with BulkTypes { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = - delegate.multiSearch(elasticQueries, fieldAliases, aggregations, fields, nestedHits) + delegate.multiSearch( + elasticQueries, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) /** Asynchronous search for documents / aggregations matching the SQL query. * @@ -1270,12 +1286,20 @@ trait ElasticClientDelegator extends ElasticClientApi with BulkTypes { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit ec: ExecutionContext, context: ConversionContext ): Future[ElasticResult[ElasticResponse]] = - delegate.singleSearchAsync(elasticQuery, fieldAliases, aggregations, fields, nestedHits) + delegate.singleSearchAsync( + elasticQuery, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) /** Asynchronous multi-search with Elasticsearch queries. * @@ -1293,12 +1317,20 @@ trait ElasticClientDelegator extends ElasticClientApi with BulkTypes { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit ec: ExecutionContext, context: ConversionContext ): Future[ElasticResult[ElasticResponse]] = - delegate.multiSearchAsync(elasticQueries, fieldAliases, aggregations, fields, nestedHits) + delegate.multiSearchAsync( + elasticQueries, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) /** Searches and converts results into typed entities from an SQL query. * diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala b/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala index c75f47a3c..216206a70 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala @@ -108,6 +108,12 @@ trait ElasticConversion { * Elasticsearch response is parsed exactly once — never serialized back to a String for core to * re-parse. */ + /** `rowInvariants` carries one constant map PER RESPONSE, aligned with the queries that produced + * them (#253 FOLD-IN 1). A UNION ALL leg has its OWN constants -- `SELECT category, 2 AS flag + * ... UNION ALL SELECT category, 3 AS flag ...` must project 2 into the first leg's rows and 3 + * into the second's -- so they cannot be applied to the concatenated result after the fact. + * Empty (the default) means project nothing, which is what every scroll-page caller wants. + */ def parseResponseTree( results: JsonNode, fieldAliases: ListMap[String, String], @@ -115,7 +121,8 @@ trait ElasticConversion { fields: Seq[String] = Seq.empty, nestedHits: Map[String, Seq[(String, String)]] = Map.empty, explodeNested: Boolean = true, - retainDocumentId: Boolean = false + retainDocumentId: Boolean = false, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit context: ConversionContext): Try[Seq[ListMap[String, Any]]] = { var json = results if (json.has("responses")) { @@ -130,7 +137,8 @@ trait ElasticConversion { fields, nestedHits, explodeNested, - retainDocumentId + retainDocumentId, + rowInvariants ) } else { // Single search response @@ -141,7 +149,8 @@ trait ElasticConversion { fields, nestedHits, explodeNested, - retainDocumentId + retainDocumentId, + rowInvariants.headOption.getOrElse(ListMap.empty) ) } } @@ -155,7 +164,8 @@ trait ElasticConversion { fields: Seq[String] = Seq.empty, nestedHits: Map[String, Seq[(String, String)]] = Map.empty, explodeNested: Boolean = true, - retainDocumentId: Boolean = false + retainDocumentId: Boolean = false, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit context: ConversionContext): Try[Seq[ListMap[String, Any]]] = Try { val responses = jsonArray.elements().asScala.toList @@ -173,7 +183,11 @@ trait ElasticConversion { throw new Exception(s"Elasticsearch errors in multi-search:\n${errors.mkString("\n")}") } else { // Parse each response and combine all rows - val allRows = responses.flatMap { response => + // Each leg carries its OWN row-invariant constants (#253 FOLD-IN 1): a UNION of + // `SELECT category, 2 AS flag ...` and `SELECT category, 3 AS flag ...` seeds 2 into the + // first leg's rows and 3 into the second's. They are placed as each leg's rows are BUILT, + // which is also why the legs being concatenated afterwards costs nothing. + val allRows = responses.zipWithIndex.flatMap { case (response, leg) => if (!response.has("error")) { jsonToRows( response, @@ -182,7 +196,8 @@ trait ElasticConversion { fields, nestedHits, explodeNested, - retainDocumentId + retainDocumentId, + rowInvariants.lift(leg).getOrElse(ListMap.empty) ) } else { Seq.empty @@ -201,7 +216,8 @@ trait ElasticConversion { fields: Seq[String] = Seq.empty, nestedHits: Map[String, Seq[(String, String)]] = Map.empty, explodeNested: Boolean = true, - retainDocumentId: Boolean = false + retainDocumentId: Boolean = false, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit context: ConversionContext): Try[Seq[ListMap[String, Any]]] = Try { // check if it is an error response @@ -218,45 +234,32 @@ trait ElasticConversion { fields, nestedHits, explodeNested, - retainDocumentId + retainDocumentId, + rowInvariants ) } } - /** Merge a statement's ROW-INVARIANT SELECT items into aggregation rows (issue #253, FOLD-IN 1). + /** convert JsonNode to Rows + */ + /** `rowInvariants` are the statement's ROW-INVARIANT SELECT items -- constants, which cannot vary + * within a group (issue #253, FOLD-IN 1). An aggregation response carries no hits, so the + * `script_fields` entry a constant is emitted as is never fetched under `"size": 0` and the + * column would come back NULL; the value is taken from the AST instead. * - * `SELECT category, 2 AS flag FROM t GROUP BY category` is standard SQL -- a constant does not - * vary within a group -- but an aggregation response carries NO hits, so the `script_fields` - * entry the constant is emitted as is never fetched (`"size": 0`) and `rowNormalizer` null-fills - * the requested column. MEASURED on real ES 8.18 before this: `flag` came back `null` on every - * row. The value is taken from the AST instead and added here, on the RESULT side. + * 🔴 They SEED `parseAggregations`' `parentContext`, so each constant is placed ONCE at the top + * of the recursion and carried into every leaf row by the `parentContext ++ ...` the recursion + * already performs -- no extra traversal and no extra allocation. A second pass over the + * assembled rows would be `O(rows x cols)` on the largest collection the engine produces + * (`Bucket.DefaultSize` is 65,536 per level, multiplied on a multi-level grouping). * - * Deliberately NOT applied to the row path: there a constant already arrives through - * `script_fields` and works, and double-handling it would be the divergence this exists to - * remove. + * Seeding also makes precedence structural: a bucket key or metric of the same name is merged on + * the RIGHT of `++` and therefore WINS, so a value Elasticsearch computed always beats a + * constant -- including a computed `null`. * - * 🔴 A key that is already PRESENT but `null` is filled, not skipped. `rowNormalizer` runs first - * and null-fills every requested output column, so by the time the rows get here the constant's - * column already exists carrying `null` -- a "never overwrite an existing key" rule reads that - * placeholder as a real value and projects nothing at all (measured: the column stayed `null` on - * real ES 8.18 with the projection wired in). A NON-null value is still never overwritten, so a - * column Elasticsearch actually computed always wins over a constant of the same name. Existing - * column ORDER is preserved -- `rowNormalizer` has already put them in SELECT order. - */ - protected def projectRowInvariants( - rows: Seq[ListMap[String, Any]], - constants: ListMap[String, Any] - ): Seq[ListMap[String, Any]] = - if (constants.isEmpty) rows - else - rows.map { row => - val filled = row.map { case (k, v) => - k -> (if (v == null) constants.getOrElse(k, v) else v) - } - filled ++ constants.filterNot { case (k, _) => filled.contains(k) } - } - - /** convert JsonNode to Rows + * Only the AGGREGATION arms are seeded. On the row path (Case 1) and the mixed hits+aggs path + * (Case 4) a constant already arrives through `script_fields`, and double-handling it is exactly + * the divergence this exists to avoid. */ def jsonToRows( json: JsonNode, @@ -265,7 +268,8 @@ trait ElasticConversion { fields: Seq[String] = Seq.empty, nestedHits: Map[String, Seq[(String, String)]] = Map.empty, explodeNested: Boolean = true, - retainDocumentId: Boolean = false + retainDocumentId: Boolean = false, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit context: ConversionContext): Seq[ListMap[String, Any]] = { val hitsNode = Option(json.path("hits").path("hits")) .filter(_.isArray) @@ -283,7 +287,7 @@ trait ElasticConversion { case (None, Some(aggs)) => // Case 2 : only aggregations - val ret = parseAggregations(aggs, ListMap.empty, fieldAliases, aggregations) + val ret = parseAggregations(aggs, rowInvariants, fieldAliases, aggregations) val groupedRows: Map[String, Seq[ListMap[String, Any]]] = ret.groupBy(_.getOrElse("bucket_root", "").toString) groupedRows.values @@ -297,7 +301,7 @@ trait ElasticConversion { case (Some(hits), Some(aggs)) if hits.isEmpty => // Case 3 : aggregations with no hits - val ret = parseAggregations(aggs, ListMap.empty, fieldAliases, aggregations) + val ret = parseAggregations(aggs, rowInvariants, fieldAliases, aggregations) val groupedRows: Map[String, Seq[ListMap[String, Any]]] = ret.groupBy(_.getOrElse("bucket_root", "").toString) groupedRows.values diff --git a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala index d5abaea1b..6bb63fe3d 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala @@ -70,8 +70,11 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { */ protected def extractOutputFieldNames(single: SingleSearch): Seq[String] = { val fields = single.select.fieldsWithComputedAliases + // `Field.outputName` is the SHARED definition: `SingleSearch.rowInvariantProjection` keys a + // projected constant off the very same expression, so the column this asks for and the column + // that supplies its value are the same string by construction (#253 FOLD-IN 1). if (fields.size == 1 && fields.head.identifier.identifierName == "*") Seq.empty - else fields.map(f => f.fieldAlias.map(_.alias).getOrElse(f.sourceField)) + else fields.map(_.outputName) } /** Issue #276 -- resolve the string literals a WHERE clause compares against `date`-mapped @@ -268,8 +271,9 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { single.fieldAliases, single.sqlAggregations, extractOutputFieldNames(single), - single.nestedHitsMappings - ).map(withRowInvariants(single, _)) + single.nestedHitsMappings, + rowInvariantsOf(single) + ) } case parsed: MultiSearch => @@ -292,8 +296,12 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { elasticQueries, multiple.fieldAliases, multiple.sqlAggregations, + // ⚠️ Pre-existing simplification, untouched: the output NAMES come from the FIRST leg + // only. The row-invariant CONSTANTS below are per-leg, because each leg may declare its + // own (#253 FOLD-IN 1). multiple.requests.headOption.map(extractOutputFieldNames).getOrElse(Seq.empty), - multiple.requests.headOption.map(_.nestedHitsMappings).getOrElse(Map.empty) + multiple.requests.headOption.map(_.nestedHitsMappings).getOrElse(Map.empty), + multiple.requests.map(rowInvariantsOf) ) case _ => @@ -332,6 +340,19 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { } } + /** The ROW-INVARIANT constants a statement contributes to its own rows (#253 FOLD-IN 1). + * + * Handed to the parse layer as ordinary statement-derived data, exactly like `fieldAliases` and + * the output `fields`, and SEEDED into `parseAggregations`' `parentContext` so each constant is + * placed once as the rows are built rather than by a second pass over them. + * + * No `returnsRows` guard is needed: `parseAggregations` IS the aggregation path by construction, + * so a row-shaped statement never reaches the seed. The empty default is what every scroll-page + * caller gets. + */ + private def rowInvariantsOf(single: SingleSearch): ListMap[String, Any] = + single.rowInvariantProjection + /** Search for documents / aggregations matching the Elasticsearch query. * * @param elasticQuery @@ -343,33 +364,22 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { * @return * the Elasticsearch response */ - /** Add a statement's ROW-INVARIANT SELECT items to an AGGREGATION response's rows (#253, FOLD-IN - * 1) -- `SELECT category, 2 AS flag ... GROUP BY category` must return `flag = 2`, not the - * `null` an aggregation response leaves behind (measured on real Elasticsearch: the - * `script_fields` entry a constant is emitted as is never fetched under `"size": 0`). - * - * Guarded on `!returnsRows`, so it applies to the aggregation path ONLY: on the row path the - * constant already arrives through `script_fields` and must not be handled twice. This is the - * one seam that has BOTH the statement and the assembled rows -- the client entry points - * (`parseSingleSearchResponse` and friends) are shared with the scroll pages and carry no - * statement, which is why the projection is applied here rather than threaded through them. - */ - private def withRowInvariants( - single: SingleSearch, - response: ElasticResponse - ): ElasticResponse = - if (single.returnsRows || single.rowInvariantProjection.isEmpty) response - else - response.copy(results = projectRowInvariants(response.results, single.rowInvariantProjection)) - def singleSearch( elasticQuery: ElasticQuery, fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = - singleSearchInternal(elasticQuery, fieldAliases, aggregations, fields, nestedHits) + singleSearchInternal( + elasticQuery, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) /** [[singleSearch]] with an explicit document-id retention decision. `retainDocumentId = true` is * reserved for the window-enrichment base query, which matches rows to their ranking ordinals by @@ -381,6 +391,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty, retainDocumentId: Boolean = false )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = { validateJson("search", elasticQuery.query) match { @@ -418,7 +429,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { fields, nestedHits, elasticQuery.explodeNested, - retainDocumentId + retainDocumentId, + Seq(rowInvariants) ) ) match { case success @ ElasticSuccess(_) => @@ -486,7 +498,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = { elasticQueries.queries.flatMap { elasticQuery => validateJson("search", elasticQuery.query).map(error => @@ -527,7 +540,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { aggs, fields, nestedHits, - elasticQueries.explodeNested + elasticQueries.explodeNested, + rowInvariants = rowInvariants ) ) match { case success @ ElasticSuccess(_) => @@ -634,8 +648,9 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { single.fieldAliases, single.sqlAggregations, extractOutputFieldNames(single), - single.nestedHitsMappings - ).map(_.map(withRowInvariants(single, _)))(ExecutionContext.global) + single.nestedHitsMappings, + rowInvariantsOf(single) + ) } case parsed: MultiSearch => @@ -655,8 +670,11 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { elasticQueries, multiple.fieldAliases, multiple.sqlAggregations, + // ⚠️ Pre-existing simplification, untouched: the output NAMES come from the FIRST leg + // only. The row-invariant CONSTANTS below are per-leg (#253 FOLD-IN 1). multiple.requests.headOption.map(extractOutputFieldNames).getOrElse(Seq.empty), - multiple.requests.headOption.map(_.nestedHitsMappings).getOrElse(Map.empty) + multiple.requests.headOption.map(_.nestedHitsMappings).getOrElse(Map.empty), + multiple.requests.map(rowInvariantsOf) ) case _ => @@ -691,7 +709,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit ec: ExecutionContext, context: ConversionContext @@ -713,7 +732,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { aggs, fields, nestedHits, - elasticQuery.explodeNested + elasticQuery.explodeNested, + rowInvariants = Seq(rowInvariants) ) ) match { case success @ ElasticSuccess(_) => @@ -808,7 +828,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit ec: ExecutionContext, context: ConversionContext @@ -832,7 +853,8 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { aggs, fields, nestedHits, - elasticQueries.explodeNested + elasticQueries.explodeNested, + rowInvariants = rowInvariants ) ) match { case success @ ElasticSuccess(_) => diff --git a/core/src/main/scala/app/softnetwork/elastic/client/metrics/MetricsElasticClient.scala b/core/src/main/scala/app/softnetwork/elastic/client/metrics/MetricsElasticClient.scala index 28bb50b38..9b636a3d2 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/metrics/MetricsElasticClient.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/metrics/MetricsElasticClient.scala @@ -924,10 +924,18 @@ class MetricsElasticClient( fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = { measureResult("search", Some(elasticQuery.indices.mkString(","))) { - delegate.singleSearch(elasticQuery, fieldAliases, aggregations, fields, nestedHits) + delegate.singleSearch( + elasticQuery, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) } } @@ -936,10 +944,18 @@ class MetricsElasticClient( fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit context: ConversionContext): ElasticResult[ElasticResponse] = { measureResult("multisearch") { - delegate.multiSearch(elasticQueries, fieldAliases, aggregations, fields, nestedHits) + delegate.multiSearch( + elasticQueries, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) } } @@ -959,7 +975,8 @@ class MetricsElasticClient( fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: ListMap[String, Any] = ListMap.empty )(implicit ec: ExecutionContext, context: ConversionContext @@ -969,7 +986,14 @@ class MetricsElasticClient( // [[ElasticClientDelegator]]. Project to this trait's `ElasticResponse` so the cast is a // no-op at runtime (both sides erase to Object) instead of `Future → Nothing$`. delegate - .singleSearchAsync(elasticQuery, fieldAliases, aggregations, fields, nestedHits) + .singleSearchAsync( + elasticQuery, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) .asInstanceOf[Future[ElasticResult[ElasticResponse]]] } @@ -989,7 +1013,8 @@ class MetricsElasticClient( fieldAliases: ListMap[String, String], aggregations: ListMap[String, SQLAggregation], fields: Seq[String] = Seq.empty, - nestedHits: Map[String, Seq[(String, String)]] = Map.empty + nestedHits: Map[String, Seq[(String, String)]] = Map.empty, + rowInvariants: Seq[ListMap[String, Any]] = Seq.empty )(implicit ec: ExecutionContext, context: ConversionContext @@ -997,7 +1022,14 @@ class MetricsElasticClient( measureAsync("multisearchAsync") { // Same latent-`Nothing`-cast fix as `singleSearchAsync` above. delegate - .multiSearchAsync(elasticQueries, fieldAliases, aggregations, fields, nestedHits) + .multiSearchAsync( + elasticQueries, + fieldAliases, + aggregations, + fields, + nestedHits, + rowInvariants + ) .asInstanceOf[Future[ElasticResult[ElasticResponse]]] } diff --git a/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala index 9c0c0164c..1c97fe547 100644 --- a/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala +++ b/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala @@ -1624,50 +1624,82 @@ class ElasticConversionSpec extends AnyFlatSpec with Matchers with ElasticConver // egress unless `elastic.include-document-id` is enabled or `_id` is selected. // ------------------------------------------------------------------------- - // ---- issue #253 FOLD-IN 1: row-invariant constants are projected into AGGREGATION rows ---- + // ---- issue #253 FOLD-IN 1: row-invariant constants are placed AS aggregation rows are built ---- // // An aggregation response carries no hits, so the `script_fields` entry a constant is emitted as // is never fetched (`"size": 0`) and `rowNormalizer` null-fills the requested column -- measured // on real ES 8.18, `SELECT category, 2 AS flag ... GROUP BY category` returned `flag = null` on - // every row. `SearchApi` merges the AST value in on the result side instead. + // every row. The value comes from the AST and SEEDS `parseAggregations`' `parentContext`, so it + // is placed once at the top of the recursion rather than by a second pass over the rows. - "projectRowInvariants" should "add every constant to every aggregation row" in { - val rows = Seq( - ListMap[String, Any]("category" -> "a"), - ListMap[String, Any]("category" -> "b") - ) - projectRowInvariants(rows, ListMap[String, Any]("flag" -> 2L, "lbl" -> "x")) shouldBe Seq( - ListMap[String, Any]("category" -> "a", "flag" -> 2L, "lbl" -> "x"), - ListMap[String, Any]("category" -> "b", "flag" -> 2L, "lbl" -> "x") - ) + private val groupedResponse = """{ + | "took": 5, + | "hits": { "total": { "value": 3 }, "hits": [] }, + | "aggregations": { + | "category": { + | "buckets": [ + | { "key": "a", "doc_count": 2, "n": { "value": 2 } }, + | { "key": "b", "doc_count": 1, "n": { "value": 1 } } + | ] + | } + | } + |}""".stripMargin + + private def groupedRows( + constants: ListMap[String, Any], + fields: Seq[String] = Seq.empty + ): Seq[ListMap[String, Any]] = + parseSingleSearchResponse( + mapper.readTree(groupedResponse), + ListMap.empty, + ListMap.empty, + fields, + rowInvariants = constants + ).get + + "a seeded row-invariant constant" should "reach every aggregation row" in { + val rows = groupedRows(ListMap[String, Any]("flag" -> 2L)) + rows should have size 2 + rows.map(_("category")) shouldBe Seq("a", "b") + rows.foreach(r => withClue(s"row=$r: ") { r("flag") shouldBe 2L }) } - it should "return the rows UNTOUCHED when there is no constant" in { - val rows = Seq(ListMap[String, Any]("category" -> "a")) - projectRowInvariants(rows, ListMap.empty) should be theSameInstanceAs rows + it should "leave the rows untouched when the statement declares none" in { + groupedRows(ListMap.empty).foreach(r => r.keys.toSeq shouldBe Seq("category", "n")) } - it should "FILL a column that rowNormalizer already null-filled, keeping its position" in { - // 🔴 This is the case that made the first wiring a no-op: `rowNormalizer` runs first and - // null-fills every requested output column, so the constant's key is already present carrying - // `null` by the time the projection sees the row. Skipping present keys therefore projected - // NOTHING -- measured on real ES 8.18, `flag` stayed null. - val rows = Seq(ListMap[String, Any]("category" -> "a", "flag" -> null)) - val out = projectRowInvariants(rows, ListMap[String, Any]("flag" -> 2L)) - out shouldBe Seq(ListMap[String, Any]("category" -> "a", "flag" -> 2L)) - out.head.keys.toSeq shouldBe Seq("category", "flag") // SELECT order preserved + it should "LOSE to a value Elasticsearch computed under the same name" in { + // Precedence is structural, not a rule to remember: the seed goes in FIRST and the recursion + // merges bucket keys and metrics on the RIGHT of `++`, so anything Elasticsearch actually + // produced overwrites the constant -- including a computed null, which is the case a + // "never overwrite a non-null value" guard could not express. + val rows = groupedRows(ListMap[String, Any]("n" -> 999L, "category" -> "seeded")) + rows.map(_("n")) shouldBe Seq(2, 1) + rows.map(_("category")) shouldBe Seq("a", "b") } - it should "never overwrite a real column with a constant of the same name" in { - // A projected constant is presentation; a value Elasticsearch actually computed always wins. - val rows = Seq(ListMap[String, Any]("flag" -> "from-elasticsearch")) - projectRowInvariants(rows, ListMap[String, Any]("flag" -> 2L)) shouldBe - Seq(ListMap[String, Any]("flag" -> "from-elasticsearch")) + it should "be ordered by the requested output fields, not by insertion" in { + // Seeded constants enter the map before the bucket keys; `rowNormalizer` puts the row back into + // SELECT order at the end of `jsonToRows`. + // `rowNormalizer` puts the REQUESTED fields first, in order, and appends anything extra the + // response carried (here the metric `n`), which is its established behaviour. + val rows = groupedRows(ListMap[String, Any]("flag" -> 2L), Seq("category", "flag")) + rows.foreach(r => + withClue(s"row=$r: ") { r.keys.toSeq.take(2) shouldBe Seq("category", "flag") } + ) } - it should "produce no row where there was none" in { - // A grouping that matched nothing stays empty -- the constant must not invent a row. - projectRowInvariants(Seq.empty, ListMap[String, Any]("flag" -> 2L)) shouldBe empty + it should "fill the COMPUTED-alias column an un-aliased constant is requested under" in { + // 🔴 The shape every other constant test misses: with no alias the requested column is the + // computed `__c2`, not the rendered `2`. Keying the projection by the rendered name left the + // requested column NULL and invented a bogus `2` beside it. + val rows = groupedRows(ListMap[String, Any]("__c2" -> 2L), Seq("category", "__c2")) + rows.foreach { r => + withClue(s"row=$r: ") { + r("__c2") shouldBe 2L + r.keys.toSeq should not contain "2" + } + } } private object EnabledDocumentIdConversion extends ElasticConversion { 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 1ad042b58..b42e97c6f 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 @@ -108,15 +108,20 @@ case class ElasticAggregation( def hasTransformExtendedStats: Boolean = ScriptedExtendedStatsAggregation.existsIn(Seq(agg)) } -/** The terms `order` a bucket asks for, looked up under EVERY spelling the sort could have been - * written with -- the resolved column name, the bucket's output name, and its SELECT alias. +/** The terms `order` a bucket asks for, looked up under BOTH spellings a sort can be keyed by: the + * bucket's resolved column (`identifier.identifierName`) and its OUTPUT name (`Bucket.name`, i.e. + * the SELECT alias when there is one, else the path). * - * The metric path (`ElasticAggregation.apply`) has had this three-way fallback all along; the - * BUCKET path had only the first, so a sort naming the SELECT alias of a grouped column silently + * The metric path (`ElasticAggregation.apply`) has had this fallback all along; the BUCKET path + * had only the first lookup, so a sort naming the SELECT alias of a grouped column silently * produced NO `order` at all: `SELECT country AS pays FROM t GROUP BY country ORDER BY pays` came * back in an arbitrary doc_count order with HTTP 200, and with a LIMIT that is a different SET of - * groups. `FieldSort.update` now resolves the alias so the first lookup already matches; this - * mirrors the metric path so the two cannot disagree, and it closes the pre-existing spelling too. + * groups. + * + * ⚠️ TWO spellings, not three. A third `.orElse` on `identifier.fieldAlias` would be unreachable: + * `Bucket.name` IS `identifier.fieldAlias.getOrElse(path)`, so whenever an alias exists the second + * lookup already used that key. (The metric path's three arms are NOT redundant -- they read two + * different members -- so the shape must not be copied blindly from it.) */ private[bridge] object BucketOrder { def apply( @@ -126,15 +131,9 @@ private[bridge] object BucketOrder { bucketsDirection .get(bucket.identifier.identifierName) .orElse(bucketsDirection.get(bucket.name)) - .orElse(bucket.identifier.fieldAlias.flatMap(bucketsDirection.get)) } object ElasticAggregation { - private def bucketDirection( - bucketsDirection: Map[String, SortOrder], - bucket: app.softnetwork.elastic.sql.query.Bucket - ): Option[SortOrder] = BucketOrder(bucketsDirection, bucket) - def apply( sqlAgg: Field, having: Option[Criteria], @@ -492,7 +491,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted date histogram - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, interval = interval) .script(script) @@ -508,7 +507,7 @@ object ElasticAggregation { } case _ => // Standard date histogram - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => DateHistogramAggregation(bucket.name, interval = interval) .field(currentBucketNestedPath) @@ -528,7 +527,7 @@ object ElasticAggregation { aggScript match { case Some(script) => // Scripted terms aggregation - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => TermsAggregation(bucket.name) .script(script) @@ -544,7 +543,7 @@ object ElasticAggregation { } case _ => // Standard terms aggregation - bucketDirection(bucketsDirection, bucket) match { + BucketOrder(bucketsDirection, bucket) match { case Some(direction) => termsAgg(bucket.name, currentBucketNestedPath) .minDocCount(1) 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 d4e0cf06e..f41fca5c9 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 @@ -802,6 +802,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "terms": { | "field": "products.category", | "size": 10, + | "exclude": "coffee", | "min_doc_count": 1 | }, | "aggs": { @@ -4863,10 +4864,9 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { // `shouldBeScripted = false`, so scoping the rule to row-INVARIANT literals left a fieldless, // scriptless `terms` for every other nameless Value -- an ES 400. A JDBC `PreparedStatement` // parameter, aliased and grouped, is the realistic route. - val param: ElasticSearchRequest = SelectStatement("SELECT ? AS p FROM Table GROUP BY p") - println(param.query) - param.query should include(""""script":{"lang":"painless","source":"params.paramValue"}""") - + // ⚠️ NOT `? AS p`: a bucket over an unbound parameter is REJECTED (it would script + // `params.paramValue`, which nothing binds, so Elasticsearch would answer zero groups with + // HTTP 200). The scripting rule below is what makes every OTHER nameless bucket work. val random: ElasticSearchRequest = SelectStatement("SELECT RANDOM AS r FROM Table GROUP BY r") random.query should include(""""script":{"lang":"painless","source":"Math.random()"}""") @@ -4880,7 +4880,6 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { it should "not emit a terms aggregation that carries neither field nor script" in { // The invariant behind the test above, stated once over every shape that reaches a bucket. Seq( - "SELECT ? AS p FROM Table GROUP BY p", "SELECT RANDOM AS r FROM Table GROUP BY r", "SELECT 2.5 AS d FROM Table GROUP BY d", "SELECT 2 + 0 AS c FROM Table GROUP BY c", @@ -4890,8 +4889,16 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { "SELECT UPPER(country) AS u FROM Table GROUP BY u" ).foreach { sql => val q: ElasticSearchRequest = SelectStatement(sql) + // Scoped INSIDE the `terms` object: asserting over the whole query lets an unrelated + // `script_fields` block satisfy it while the aggregation itself carries neither. + val terms = q.query.split("\"terms\":\\{").drop(1).map(_.takeWhile(_ != '}')) withClue(s"[$sql] ${q.query}: ") { - q.query.contains("\"field\"") || q.query.contains("\"script\"") shouldBe true + terms should not be empty + terms.foreach(t => + withClue(s"terms{$t}: ") { + t.contains("\"field\"") || t.contains("\"script\"") shouldBe true + } + ) } } } 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 5d04ad4e2..c2edb7c19 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 @@ -26,6 +26,7 @@ import app.softnetwork.elastic.sql.{ LongValue, PainlessContext, PainlessScript, + ParamValue, TokenRegex, Updateable } @@ -240,6 +241,16 @@ case class Bucket( s"GROUP BY position ${u.position} is out of range: the SELECT list has ${u.selectSize} " + s"item(s), so positions 1 to ${u.selectSize} are valid" ) + case None if identifier.functions == List(ParamValue) => + // A bucket over an unbound `?` is scripted as `params.paramValue`, and nothing binds that + // key -- Painless reads a missing param as null, so Elasticsearch answers ZERO groups with + // HTTP 200. Scripting every nameless bucket is the right rule (it is what a terms + // aggregation needs), but this one shape has no value to script, so reject it rather than + // return a silent empty result. Consistent with story 20.9's ruling on a nested `?`. + Left( + "GROUP BY on a query parameter (?) is not supported: the parameter is never bound, so " + + "the grouping would silently match nothing" + ) case None if substitution.exists(_.alias.isEmpty) && identifier.name.isEmpty => // A SUBSTITUTED bucket whose resolved identifier has no field name of its own, and no // alias either, cannot be re-emitted: `SELECT 3 FROM t GROUP BY 1` would render diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala index 5be10016c..df459152a 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/OrderBy.scala @@ -39,7 +39,8 @@ case class FieldSort( nullOrdering: Option[NullOrdering] = None, bareTableAlias: Option[String] = None, unresolvedOrdinal: Option[UnresolvedOrdinal] = None, - resolved: Boolean = false + resolved: Boolean = false, + substitution: Option[Bucket.Substitution] = None ) extends FunctionChain with Updateable { lazy val functions: List[Function] = field.functions @@ -65,8 +66,19 @@ case class FieldSort( // Render via field.sql (not identifierName) so the table-alias qualifier // survives the AST → SQL round-trip — consumers such as the join planner // re-parse this output and reject unqualified columns as ambiguous (#158). + // + // 🔴 A SUBSTITUTED sort re-emits the SELECT alias, for exactly the reason `Bucket.sql` does: an + // `ORDER BY ` resolved onto a bare numeric literal renders as that literal, which the grammar + // reads back as a POSITION. MEASURED before this: + // `SELECT 2 AS flag, SUM(amount) AS s FROM t GROUP BY flag ORDER BY flag ASC` rendered + // `ORDER BY 2 ASC` and re-parsed as `ORDER BY SUM(amount) ASC` -- a bucket sort silently becoming + // a METRIC sort -- and `SELECT 2 AS flag FROM t GROUP BY 1 ORDER BY 1 ASC` rendered a statement + // that no longer parses at all. The render IS re-parsed by consumers (#158's join planner, and + // `MaterializedViewExtension` re-runs a persisted one), so this is a live corruption, not a + // cosmetic round-trip failure. `Bucket` was given this protection in the same commit that taught + // `FieldSort` to substitute; the rule now covers both clauses, with no "except". override def sql: String = - s"${field.sql} $direction${nullOrdering.map(n => s" ${n.sql}").getOrElse("")}" + s"${substitution.flatMap(_.alias).getOrElse(field.sql)} $direction${nullOrdering.map(n => s" ${n.sql}").getOrElse("")}" override def update(request: SingleSearch): FieldSort = ordinal match { @@ -81,7 +93,12 @@ case class FieldSort( // obviously so for a nested or unnested field. Take the resolved identifier as it stands. Bucket.selectItem(position, request) match { case Some(selected) => - this.copy(field = selected.identifier, unresolvedOrdinal = None, resolved = true) + this.copy( + field = selected.identifier, + unresolvedOrdinal = None, + resolved = true, + substitution = Some(Bucket.Substitution.of(selected)) + ) case None => this.copy( unresolvedOrdinal = Some(UnresolvedOrdinal(position, request.select.fields.size)), @@ -89,45 +106,35 @@ case class FieldSort( ) } case None => - // 🔴 An ORDER BY that names a BUCKET by its output name must resolve to the same identifier - // the bucket did, through the SAME function -- otherwise `SingleSearch.sorts` is keyed by - // the alias (`"cat"`) while `buildBuckets` looks the direction up under the resolved column - // (`"category"`), and the terms `order` is silently DROPPED. Measured: - // `SELECT category AS cat FROM t GROUP BY cat ORDER BY cat ASC LIMIT 3` came back as an - // arbitrary top-3 by doc_count instead of the three smallest keys, HTTP 200. Same - // obligation as `SingleSearch.bucketNames`' key language, one clause over. + // 🔴 NO alias resolution here, and that is a DELIBERATE deletion, not an omission. // - // ⚠️ SCOPED to a name that IS a bucket's output name, and nothing else. Resolving every - // ORDER BY alias broke three measured shapes: a window `ORDER BY hire_date` inside - // `OVER (...)` re-pointed onto a SELECT item aliased `hire_date` (`CAST(hire_date AS DATE)`), - // and Superset's `ORDER BY "Revenue"` over `sum(total_price) AS "Revenue"` lost its alias -- - // METRIC sorts already resolve through `ElasticAggregation`'s own three-way fallback and - // must not be rewritten here. Only the BUCKET path lacked that fallback, so only the bucket - // path is resolved. + // An earlier round resolved an ORDER BY that named a bucket by its output name, to keep + // `SingleSearch.sorts`' key in step with what `buildBuckets` looks up. It was proved + // REDUNDANT by falsification -- disabling it left all eight ORDER BY shapes emitting the + // correct terms `order`, because `BucketOrder` in the bridge already falls back to the + // bucket's own name -- and it was actively HARMFUL: `FieldSort.update` is shared verbatim + // between the statement's ORDER BY and every window's `OVER (...)` sort (six call sites in + // `function/aggregate`), with nothing to tell them apart, so it rewrote + // `ROW_NUMBER() OVER (PARTITION BY country ORDER BY pays)` into `ORDER BY country` whenever + // the statement happened to carry a GROUP BY. The guard that hid this keyed on a property + // of the STATEMENT (`groupBy.isDefined`), never of the sort being updated. // - // No `.update(request)` on what this resolves, for the same reason as the ordinal arm - // above: `orderBy` is updated LAST, so `request.select.fields` is already updated. - val namesABucket = - request.groupBy.isDefined && request.buckets.exists(_.name == field.name) - (if (namesABucket) Bucket.aliasItem(field, request) else None) match { - case Some(aliased) => - this.copy(field = aliased.identifier, resolved = true, bareTableAlias = None) - case None => - this.copy( - field = field.update(request), - resolved = true, - // `ORDER BY e` where `e` aliases a FROM table sorts on nothing (#159). It used to be - // caught downstream, because `Identifier.update` rewrote any bare name matching an alias - // to the empty string; that rewrite also broke a column legitimately sharing its table's - // name, so it is gone and the collision is recorded here, where the FROM aliases are - // still in scope. - bareTableAlias = - if (!field.name.contains('.') && request.tableAliases.exists(_._2 == field.name)) - Some(field.name) - else - None - ) - } + // Keep the user's spelling. The bridge resolves the direction; the render stays a fixed + // point; and a window's own ORDER BY is left alone. + this.copy( + field = field.update(request), + resolved = true, + // `ORDER BY e` where `e` aliases a FROM table sorts on nothing (#159). It used to be + // caught downstream, because `Identifier.update` rewrote any bare name matching an alias + // to the empty string; that rewrite also broke a column legitimately sharing its table's + // name, so it is gone and the collision is recorded here, where the FROM aliases are + // still in scope. + bareTableAlias = + if (!field.name.contains('.') && request.tableAliases.exists(_._2 == field.name)) + Some(field.name) + else + None + ) } override def validate(): Either[String, Unit] = diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala index 4308ebba6..d2033e66b 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala @@ -47,6 +47,21 @@ case class Field( with FunctionChain with PainlessScript with DateMathScript { + + /** The name this SELECT item appears under in a result row -- its alias when it has one, else its + * source field. + * + * 🔴 ONE definition, because two places have to agree about it and did not: `SearchApi`'s + * requested-output names (which drive `rowNormalizer`, and therefore which columns exist and in + * what order) and `SingleSearch.rowInvariantProjection` (which supplies the VALUE of a constant + * column). Keying the projection off `identifierName` instead put `SELECT category, 2 FROM t + * GROUP BY category` under `2` while the row wanted `__c2`, so the requested column came back + * NULL and a bogus `2` column was invented beside it. Both callers now read this, over + * `Select.fieldsWithComputedAliases`, so a `__cN` computed for an un-aliased expression is the + * same on both sides by construction. + */ + lazy val outputName: String = fieldAlias.map(_.alias).getOrElse(sourceField) + def tableAlias: Option[String] = identifier.tableAlias def table: Option[String] = identifier.table def isScriptField: Boolean = identifier.painlessScriptRequired 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 6b32a75ca..41a12a663 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 @@ -173,7 +173,21 @@ package object query { resolvedName -> updated // the resolved column name ) ++ display.map(_ -> updated)): _* // the OUTPUT name a HAVING or WHERE would use ) - case None => ListMap(name -> b) + case None => + // The bucket was NOT substituted -- but it still has to be named the way the real bucket + // is, or `Expression.includes` (which compares by DISPLAY name) stops matching and a + // HAVING is silently dropped. `SELECT category AS cat ... GROUP BY category HAVING cat + // <> 'x'` lost its `exclude` for exactly this reason: the copy was named `category` while + // the bucket that reaches Elasticsearch is named `cat`. Broken on main too, but leaving + // it made whether a HAVING is honoured depend on whether the GROUP BY spelled the alias + // or the column, which is worse than uniformly broken. + val display = fieldAliases.get(name) + val named = b.identifier match { + case g: GenericIdentifier => g.copy(fieldAlias = display.orElse(g.fieldAlias)) + case other => other + } + val updated = b.copy(identifier = named) + ListMap((Seq(name -> updated) ++ display.map(_ -> updated)): _*) } }: _*) @@ -266,7 +280,12 @@ package object query { } lazy val scriptFields: Seq[Field] = { - if (aggregates.nonEmpty) + // A GROUP BY emits `"size": 0`, so NO hit is ever returned and a `script_fields` block can + // never be fetched -- Elasticsearch would still parse and compile every Painless script in it + // on each query. It used to be emitted whenever the statement carried no metric aggregate, + // which is exactly the aggregate-free GROUP BY shape issue #253 makes common. The VALUES that + // block used to be asked for now come from `rowInvariantProjection` on the result side. + if (aggregates.nonEmpty || groupBy.isDefined) Seq.empty else select.fieldsWithComputedAliases.filter(_.isScriptField) @@ -295,24 +314,29 @@ package object query { * the GROUP BY as an extra scripted `terms` level -- it works, but it costs a Painless bucket * level per constant on the aggregation hot path.) * - * The key follows the bucket-key convention: the alias when there is one, else - * `identifierName`. Row-VARIANT and unbound shapes are excluded by + * The key is [[Field.outputName]] over `fieldsWithComputedAliases` -- the SAME expression + * `SearchApi.extractOutputFieldNames` uses, so the projected key and the requested column are + * the same string by construction. Row-VARIANT and unbound shapes are excluded by * [[SingleSearch.isRowInvariantLiteral]] -- a projected `?` would be a NULL column by another * route, which is the very thing this fixes. */ - lazy val rowInvariantProjection: ListMap[String, Any] = + lazy val rowInvariantProjection: ListMap[String, Any] = { + // No de-duplication against other SELECT items: the constants SEED `parseAggregations`' + // `parentContext`, so anything Elasticsearch computes is merged on the RIGHT of `++` and + // therefore WINS -- including a computed null, which a "never overwrite a non-null value" + // guard could not express. Precedence is structural, not a rule to remember. ListMap( - select.fields + select.fieldsWithComputedAliases .filter(f => SingleSearch.isRowInvariantLiteral(f.identifier)) .map { f => - val name = f.fieldAlias.map(_.alias).getOrElse(f.identifier.identifierName) val value = f.identifier.functions.headOption match { case Some(v: Value[_]) => v.value case _ => null } - name -> value + f.outputName -> value }: _* ) + } lazy val windowFields: Seq[Field] = select.fieldsWithComputedAliases.filter(_.identifier.hasWindow) @@ -536,29 +560,34 @@ package object query { object SingleSearch { /** A bare ROW-INVARIANT literal: an empty name carrying exactly one scalar constant `Value` -- - * the shape the parser builds for `2 AS COL2` (`GenericIdentifier("", List(LongValue(2)))`, - * measured 2026-09-07). Such an item is the same for every document in a group. + * the shape the parser builds for `2 AS COL2` (`GenericIdentifier("", List(LongValue(2)))`). + * Such an item is the same for every document in a group. * - * 🔴 It has NO production consumer today, deliberately, and it is kept only because the - * decision it belongs to is with the lead (story 21.3, OQ-G). It was written for FOLD-IN 1's - * validator branch -- "a constant is legal beside a GROUP BY" -- but the branch was NOT - * shipped: measured on real Elasticsearch, accepting `SELECT category, 2 AS flag ... GROUP BY - * category` returns the constant column as NULL on every row, because the aggregation path - * answers `"size": 0` and the `script_fields` entry is never fetched. It also briefly scoped - * `Bucket.shouldBeScripted`, which was wrong for a different reason: every NAMELESS `Value` - * needs a script, not just the row-invariant ones, so that rule now keys on - * `identifier.name.isEmpty`. + * TWO production consumers, and they are two halves of one feature (issue #253, FOLD-IN 1 -- + * "a constant is legal beside a GROUP BY"): + * - `SingleSearch.validate()` excuses such a field from the non-aggregated-field check, so + * `SELECT category, 2 AS flag FROM t GROUP BY category` parses; + * - `SingleSearch.rowInvariantProjection` carries its VALUE, which `SearchApi` merges into + * each aggregation row -- without that the column parses and then comes back NULL, because + * an aggregation response has no hits and the `script_fields` entry a constant is emitted + * as is never fetched under `"size": 0`. * - * When OQ-G is answered this must not survive unchanged: either it becomes the classifier - * FOLD-IN 1 needs, or it is deleted. + * It does NOT decide whether a BUCKET must be scripted: that rule is `identifier.name.isEmpty` + * (a bucket with no field name has nothing to name), which covers every nameless `Value` and + * not merely the row-invariant ones. * * 🔴 It is an ALLOW-list on purpose, not a deny-list. A deny-list ("everything but * `RandomValue`/`ParamValue`/...") defaults to ACCEPT, so it silently widens the moment a * `Value` subclass is added -- and it would already admit `Values` (array literals, which - * extend `Value[Seq[T]]`) unmeasured. `Null` IS included: it is a constant, and excluding it - * would be an exception with no principle behind it. `CharValue` / `EValue` are not reachable - * from today's grammar (`value = literal|pi|random|double|long|boolean|null|param|array`); - * they are classified with their peers rather than left to the reject arm by accident. + * extend `Value[Seq[T]]`) unmeasured. Keeping `ParamValue` / `RandomValue` / `IdValue` / + * `IngestTimestampValue` / `Values` OUT is load-bearing: they are row-variant or unbound, and + * a projected `?` would be a NULL column by another route -- the very thing this fixes. `Null` + * IS included: it is a constant, and excluding it would be an exception with no principle + * behind it (a projected SQL `NULL` column is correct, though indistinguishable downstream + * from one that failed to project, since `rowNormalizer` null-fills identically). `CharValue` + * / `EValue` are not reachable from today's grammar (`value = + * literal|pi|random|double|long|boolean|null|param|array`); they are classified with their + * peers rather than left to the reject arm by accident. */ def isRowInvariantLiteral(identifier: Identifier): Boolean = identifier.name.isEmpty && (identifier.functions match { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala index 79f06e582..75ce803ef 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala @@ -131,9 +131,16 @@ class GroupByFoldInSpec extends AnyFlatSpec with Matchers { parsed("SELECT category, true AS b FROM t GROUP BY category").rowInvariantProjection shouldBe ListMap("b" -> true) - // An un-aliased constant is keyed by its rendered name, the same convention bucket keys use. + // 🔴 An UN-ALIASED constant is keyed by the COMPUTED alias, because that is the column the + // result side asks for. Keying it by the rendered name (`"2"`) left the requested `__c2` column + // NULL and invented a bogus `2` column beside it -- measured end to end through the real + // `rowNormalizer`. `Field.outputName` is the single definition both sides read. parsed("SELECT category, 2 FROM t GROUP BY category").rowInvariantProjection.keys.toSeq shouldBe - Seq("2") + Seq("__c2") + parsed( + "SELECT category, 'x' FROM t GROUP BY category" + ).rowInvariantProjection.keys.toSeq shouldBe + Seq("__c2") // A statement with no constant projects nothing, so the merge is a no-op by construction. parsed("SELECT category FROM t GROUP BY category").rowInvariantProjection shouldBe empty @@ -157,6 +164,22 @@ class GroupByFoldInSpec extends AnyFlatSpec with Matchers { parsed("SELECT category, ? AS p FROM t").rowInvariantProjection shouldBe empty } + it should "still declare a constant whose name an aggregation also claims" in { + // Precedence is settled downstream, not here: the constants SEED the aggregation recursion, so + // anything Elasticsearch computes overwrites them (asserted in `ElasticConversionSpec`). The + // AST layer therefore just reports what the statement declares. + parsed( + "SELECT category, 2 AS m, MAX(amount) AS m FROM t GROUP BY category" + ).rowInvariantProjection shouldBe ListMap("m" -> 2L) + } + + it should "reject a GROUP BY over an unbound query parameter" in { + // Scripting every nameless bucket is right, but `params.paramValue` is never bound: Painless + // reads the missing key as null and Elasticsearch answers ZERO groups with HTTP 200. A silent + // empty result is worse than the loud rejection. + rejects("SELECT ? AS p FROM t GROUP BY p", "query parameter") + } + it should "still reject a ROW-VARIANT pseudo-value beside a GROUP BY" in { // The classification must not leak: `RANDOM` re-evaluates per document and `?` is unbound, so // neither is a constant a group can be said to carry. @@ -191,7 +214,14 @@ class GroupByFoldInSpec extends AnyFlatSpec with Matchers { "SELECT 2 AS COL2, SUM(amount) AS s FROM t GROUP BY COL2", "SELECT 2 AS COL2 FROM t GROUP BY COL2", "SELECT 3 AS c FROM t GROUP BY 1", - "SELECT SUM(1) AS COL, 2 AS COL2 FROM t GROUP BY 2" + "SELECT SUM(1) AS COL, 2 AS COL2 FROM t GROUP BY 2", + // 🔴 The ORDER BY half: a sort resolved onto a bare numeric literal rendered as that literal + // and re-parsed as a POSITION. `... GROUP BY flag ORDER BY flag` became + // `ORDER BY 2` -> `ORDER BY SUM(amount)` (a bucket sort silently becoming a metric sort), and + // `... GROUP BY 1 ORDER BY 1` rendered a statement that no longer parses at all. + "SELECT 2 AS flag, SUM(amount) AS s FROM t GROUP BY flag ORDER BY flag ASC", + "SELECT 2 AS flag FROM t GROUP BY 1 ORDER BY 1 ASC", + "SELECT 2 AS flag FROM t GROUP BY flag ORDER BY flag DESC" ).foreach { sql => val rendered = Parser(sql).map(_.sql).getOrElse(fail(s"did not parse: $sql")) withClue(s"[$sql] rendered=[$rendered] ") { diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index d2b92ed8a..dc73d0e74 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -20,7 +20,7 @@ import akka.NotUsed import akka.actor.ActorSystem import akka.stream.scaladsl.Source import app.softnetwork.elastic.client.bulk._ -import app.softnetwork.elastic.client.result.{ElasticFailure, ElasticSuccess} +import app.softnetwork.elastic.client.result.{ElasticFailure, ElasticResult, ElasticSuccess} import app.softnetwork.elastic.client.spi.ElasticClientFactory import app.softnetwork.elastic.scalatest.ElasticDockerTestKit import app.softnetwork.elastic.sql.query.SelectStatement @@ -337,6 +337,48 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + it should "project an UN-ALIASED constant under the column the row actually asks for" in { + // 🔴 Every other constant test uses an alias, which is why this shipped broken: with no alias + // the requested column is the COMPUTED `__c2`, and keying the projection by the rendered name + // (`2`) left that column NULL while inventing a bogus `2` beside it. + implicit val bareCtx: ConversionContext = NativeContext + client.search( + SelectStatement("SELECT category, 2 FROM group_by_completeness GROUP BY category") + ) match { + case ElasticSuccess(response) => + response.results should have size categories.toLong + response.results.foreach { row => + withClue(s"row=$row: ") { + row.keys.toSeq should contain("__c2") + row("__c2").toString shouldBe "2" + row.keys.toSeq should not contain "2" + } + } + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + + it should "project each UNION ALL leg's OWN constant" in { + // 🔴 The multi-search arm never projected at all: both legs are aggregation-shaped and each + // carries a different constant, so the rows came back with `flag = null`. The legs are + // concatenated by the time a caller sees them, so the projection has to happen per response. + implicit val unionCtx: ConversionContext = NativeContext + client.search( + SelectStatement( + "SELECT category, 2 AS flag FROM group_by_completeness WHERE amount <= 1 GROUP BY category" + + " UNION ALL " + + "SELECT category, 3 AS flag FROM group_by_completeness WHERE amount <= 2 GROUP BY category" + ) + ) match { + case ElasticSuccess(response) => + val flags = response.results.map(_("flag").toString).toSet + withClue(s"rows=${response.results.take(4)}: ") { + flags shouldBe Set("2", "3") + } + case ElasticFailure(error) => fail(s"Query failed: ${error.message}") + } + } + it should "order an aggregate-free GROUP BY by a SELECT ALIAS of the grouped column" in { // 🔴 Regression guard for the alias/bucket key desync: `SingleSearch.sorts` is keyed by the // sort's name while `buildBuckets` looks the direction up under the bucket's resolved column, @@ -354,6 +396,35 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + it should "apply a HAVING however the GROUP BY spelled the column" in { + // 🔴 M-2: the repair originally covered only a SUBSTITUTED bucket, so whether a HAVING was + // honoured depended on whether the GROUP BY named the alias or the column -- worse than + // uniformly broken. Both spellings must filter. + // ⚠️ Unrolled, not looped: `searchAs` is a macro and needs a compile-time constant SQL string. + def check(label: String, result: ElasticResult[Seq[CategoryAliased]]): Unit = + result match { + case ElasticSuccess(rows) => + withClue(s"[$label] ") { + rows should have size (categories - 1).toLong + rows.map(_.cat) should not contain "cat_01" + } + case ElasticFailure(error) => fail(s"[$label] Query failed: ${error.message}") + } + + check( + "HAVING names the alias", + client.searchAs[CategoryAliased]( + "SELECT category AS cat FROM group_by_completeness GROUP BY category HAVING cat <> 'cat_01'" + ) + ) + check( + "HAVING names the column", + client.searchAs[CategoryAliased]( + "SELECT category AS cat FROM group_by_completeness GROUP BY category HAVING category <> 'cat_01'" + ) + ) + } + it should "apply a HAVING over an alias-resolved bucket" in { // 🔴 Regression guard for the same desync on the HAVING path: the bucket `Identifier.update` // attaches is matched against the real one by DISPLAY NAME, so an aliased grouped column made From 0d389cd64af326f082e7db739cb07b1c5131ba50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Mon, 7 Sep 2026 09:54:23 +0200 Subject: [PATCH 4/4] fix(sql,core): no phantom row on an empty grouping; a constant never shadows a computed column (story 21.3) Final review round. F-1 -- "Elasticsearch always wins, including a computed null" was measurably FALSE and it was the justification for deleting the explicit guard. `extractMetrics` writes NO entry for a null-valued metric, so nothing lands on the right of the merge and the seeded constant survives: measured, `{"m":{"value":null}}` with seed `m -> 2` returned `m = 2` where SQL says NULL. Rather than document the hole, the claim is made true: `rowInvariantProjection` again refuses to declare a constant for a name another SELECT item owns, so the merge never has to arbitrate the collision. Merge precedence is still real and still pinned; it is now described as what it is -- true wherever Elasticsearch EMITS. The precedence test gained the null-metric row that the original could not see. F-4 -- an empty grouping returned ONE phantom row with every column null, and with a seeded constant that phantom carried REAL values. The row-assembly fold seeds itself with one empty row and an empty bucket list fell straight through it. Fixed for BOTH the aggregate-free and aggregate-bearing spellings: no rows in, no rows out. The aggregate-bearing phantom predates 0.22.0, so this is a behaviour change with a release-note line. Pinned Docker-free on the SEEDED path and on real Elasticsearch for all three spellings. F-2 -- `Field.scriptName` was a byte-identical twin of `Field.outputName` 61 lines away, and it is the bridge's `script_fields` key: the other half of the very coupling `outputName` exists to single-source. It now delegates. F-3 -- four more live copies of the same expression routed through `outputName` (the non-aggregated-field validator and both ranking-window alias maps). The scaladoc no longer claims to be the single definition while others remain; the rest are named as a follow-up, and `FromlessSelect.columnNames` is left alone as a deliberately different convention. F-5 -- the UNION ALL oracle could not catch a leg swap. The legs now select different numbers of groups (`amount >= 2` gives 36, `amount >= 3` gives 35) and the assertion pairs each constant with its row count. F-6 -- a short `rowInvariants` would have given the unmatched legs a silent NULL constant column. `parseMultiSearchResponse` now requires one seed per response, or none at all. Also: the ordering test that stayed green under both mutations is retitled, since it pins `rowNormalizer`'s ordering rather than the seeding. Verified: sql 823, core 905, bridge 197, es6bridge 197 on 2.13.16 and 2.12.20, bridges run alone; `GroupByCompletenessSpec` 19/19 on real ES 8.18. --- .../elastic/client/ElasticConversion.scala | 62 +++++++++++------ .../elastic/client/SearchApi.scala | 4 +- .../client/ElasticConversionSpec.scala | 66 +++++++++++++++++-- .../elastic/sql/query/Select.scala | 26 +++++--- .../elastic/sql/query/package.scala | 22 +++++-- .../elastic/sql/query/GroupByFoldInSpec.scala | 11 ++-- .../client/GroupByCompletenessSpec.scala | 31 +++++++-- 7 files changed, 169 insertions(+), 53 deletions(-) diff --git a/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala b/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala index 216206a70..39843a234 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/ElasticConversion.scala @@ -170,6 +170,16 @@ trait ElasticConversion { Try { val responses = jsonArray.elements().asScala.toList + // 🔴 One seed per response, or none at all. A short `rowInvariants` would silently give the + // unmatched legs a NULL constant column -- the defect class issue #253 exists to close -- + // so a misalignment fails here instead of reaching the caller. `Seq.empty` is the only + // legitimate short form and means "this caller declares no constants" (every scroll page). + require( + rowInvariants.isEmpty || rowInvariants.size == responses.size, + s"rowInvariants must carry one entry per response (got ${rowInvariants.size} " + + s"for ${responses.size} responses)" + ) + // Collect all errors val errors = responses.zipWithIndex.collect { case (response, idx) if response.has("error") => @@ -240,6 +250,36 @@ trait ElasticConversion { } } + /** Cross-join the per-root aggregation rows into result rows. + * + * 🔴 NO ROWS IN, NO ROWS OUT. The fold's initial value is one EMPTY row, so an aggregation that + * matched nothing -- `"buckets": []` over an empty index or a `WHERE` that excludes everything + * -- used to fall straight through it and yield that seed, which `rowNormalizer` then + * null-filled into a phantom all-NULL row. MEASURED: `SELECT category FROM t GROUP BY category` + * over an empty index returned 1 row of `ListMap(category -> null)`, and with issue #253's + * seeded constants the phantom row carried REAL values, which is worse. + * + * The phantom predates 0.22.0 for the aggregate-BEARING spelling; issue #253 moved the + * aggregate-free spelling off the scroll path (which returned no rows) onto this one. The guard + * is applied to BOTH, because a rule justified by correctness takes no "except" -- one all-null + * row is wrong for either spelling. + */ + private def combineAggregationRows( + rows: Seq[ListMap[String, Any]] + ): Seq[ListMap[String, Any]] = + if (rows.isEmpty) Seq.empty + else + rows + .groupBy(_.getOrElse("bucket_root", "").toString) + .values + .foldLeft(Seq(ListMap.empty[String, Any])) { (acc, group) => + for { + accMap <- acc + groupMap <- group + } yield accMap ++ groupMap + } + .map(_ - "bucket_root") + /** convert JsonNode to Rows */ /** `rowInvariants` are the statement's ROW-INVARIANT SELECT items -- constants, which cannot vary @@ -288,30 +328,12 @@ trait ElasticConversion { case (None, Some(aggs)) => // Case 2 : only aggregations val ret = parseAggregations(aggs, rowInvariants, fieldAliases, aggregations) - val groupedRows: Map[String, Seq[ListMap[String, Any]]] = - ret.groupBy(_.getOrElse("bucket_root", "").toString) - groupedRows.values - .foldLeft(Seq(ListMap.empty[String, Any])) { (acc, group) => - for { - accMap <- acc - groupMap <- group - } yield accMap ++ groupMap - } - .map(_ - "bucket_root") + combineAggregationRows(ret) case (Some(hits), Some(aggs)) if hits.isEmpty => // Case 3 : aggregations with no hits val ret = parseAggregations(aggs, rowInvariants, fieldAliases, aggregations) - val groupedRows: Map[String, Seq[ListMap[String, Any]]] = - ret.groupBy(_.getOrElse("bucket_root", "").toString) - groupedRows.values - .foldLeft(Seq(ListMap.empty[String, Any])) { (acc, group) => - for { - accMap <- acc - groupMap <- group - } yield accMap ++ groupMap - } - .map(_ - "bucket_root") + combineAggregationRows(ret) case (Some(hits), Some(aggs)) if hits.nonEmpty => // Case 4 : Hits + global aggregations + top_hits aggregations diff --git a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala index 6bb63fe3d..2d1abfbf1 100644 --- a/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala +++ b/core/src/main/scala/app/softnetwork/elastic/client/SearchApi.scala @@ -1674,7 +1674,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { val rankingWindows: Seq[(String, RankingWindow)] = request.windowFields.flatMap { f => f.identifier.windows.collect { case r: RankingWindow => - f.fieldAlias.map(_.alias).getOrElse(f.sourceField) -> r + f.outputName -> r } } @@ -1882,7 +1882,7 @@ trait SearchApi extends ElasticConversion with ElasticClientHelpers { val rankingAliases: Seq[String] = request.windowFields.flatMap { f => f.identifier.windows.collect { case _: RankingWindow => - f.fieldAlias.map(_.alias).getOrElse(f.sourceField) + f.outputName } } diff --git a/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala index 1c97fe547..5ed45b3de 100644 --- a/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala +++ b/core/src/test/scala/app/softnetwork/elastic/client/ElasticConversionSpec.scala @@ -1668,17 +1668,71 @@ class ElasticConversionSpec extends AnyFlatSpec with Matchers with ElasticConver groupedRows(ListMap.empty).foreach(r => r.keys.toSeq shouldBe Seq("category", "n")) } - it should "LOSE to a value Elasticsearch computed under the same name" in { - // Precedence is structural, not a rule to remember: the seed goes in FIRST and the recursion - // merges bucket keys and metrics on the RIGHT of `++`, so anything Elasticsearch actually - // produced overwrites the constant -- including a computed null, which is the case a - // "never overwrite a non-null value" guard could not express. + it should "LOSE to a value Elasticsearch actually EMITTED under the same name" in { + // The seed goes in FIRST and the recursion merges bucket keys and metrics on the RIGHT of `++`, + // so anything Elasticsearch produced overwrites the constant. val rows = groupedRows(ListMap[String, Any]("n" -> 999L, "category" -> "seeded")) rows.map(_("n")) shouldBe Seq(2, 1) rows.map(_("category")) shouldBe Seq("a", "b") } - it should "be ordered by the requested output fields, not by insertion" in { + it should "SURVIVE a metric Elasticsearch computed as NULL — which is why the AST guard exists" in { + // 🔴 The limit of merge precedence, and the reason `SingleSearch.rowInvariantProjection` still + // refuses to declare a constant for a name another SELECT item owns. `extractMetrics` writes NO + // entry for a null-valued metric, so nothing lands on the right of `++` and the constant + // survives: "Elasticsearch always wins" is true only where Elasticsearch EMITS. Without the AST + // guard, `SELECT category, 2 AS m, MAX(amount) AS m ... GROUP BY category` would return `m = 2` + // where SQL says NULL. + val withNullMetric = """{ + | "took": 5, + | "hits": { "total": { "value": 2 }, "hits": [] }, + | "aggregations": { + | "category": { + | "buckets": [ + | { "key": "a", "doc_count": 1, "m": { "value": null } }, + | { "key": "b", "doc_count": 1, "m": { "value": 3 } } + | ] + | } + | } + |}""".stripMargin + val rows = parseSingleSearchResponse( + mapper.readTree(withNullMetric), + ListMap.empty, + ListMap.empty, + rowInvariants = ListMap[String, Any]("m" -> 2L) + ).get + rows.map(_("m")) shouldBe Seq(2L, 3) + } + + it should "produce NO row when the aggregation matched nothing" in { + // 🔴 The fold's initial value is one EMPTY row, so an empty `buckets` array used to fall + // through it and yield a phantom all-NULL row -- and with a seeded constant that phantom + // carried REAL values. Pinned on the SEEDED path so it cannot be lost again. + val empty = """{ + | "took": 1, + | "hits": { "total": { "value": 0 }, "hits": [] }, + | "aggregations": { "category": { "buckets": [] } } + |}""".stripMargin + parseSingleSearchResponse( + mapper.readTree(empty), + ListMap.empty, + ListMap.empty, + Seq("category", "flag"), + rowInvariants = ListMap[String, Any]("flag" -> 2L) + ).get shouldBe Seq.empty + + // and with no constants at all — the aggregate-BEARING spelling, phantom since before 0.22.0 + parseSingleSearchResponse( + mapper.readTree(empty), + ListMap.empty, + ListMap.empty, + Seq("category") + ).get shouldBe Seq.empty + } + + // NOTE: this pins `rowNormalizer`'s ordering, NOT the seeding — it stays green with seeding + // disabled. It lives here because the seeded constants are what made the question worth asking. + it should "leave rowNormalizer's output ordering intact when constants are seeded" in { // Seeded constants enter the map before the bucket keys; `rowNormalizer` puts the row back into // SELECT order at the end of `jsonToRows`. // `rowNormalizer` puts the REQUESTED fields first, in order, and appends anything extra the diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala index d2033e66b..051b6ef80 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala @@ -51,14 +51,19 @@ case class Field( /** The name this SELECT item appears under in a result row -- its alias when it has one, else its * source field. * - * 🔴 ONE definition, because two places have to agree about it and did not: `SearchApi`'s - * requested-output names (which drive `rowNormalizer`, and therefore which columns exist and in - * what order) and `SingleSearch.rowInvariantProjection` (which supplies the VALUE of a constant - * column). Keying the projection off `identifierName` instead put `SELECT category, 2 FROM t - * GROUP BY category` under `2` while the row wanted `__c2`, so the requested column came back - * NULL and a bogus `2` column was invented beside it. Both callers now read this, over - * `Select.fieldsWithComputedAliases`, so a `__cN` computed for an un-aliased expression is the - * same on both sides by construction. + * 🔴 Introduced because places that MUST agree about it did not: `SearchApi`'s requested-output + * names (which drive `rowNormalizer`, and therefore which columns exist and in what order), + * `SingleSearch.rowInvariantProjection` (which supplies the VALUE of a constant column), the + * non-aggregated-field validator, the ranking-window alias maps and `scriptName` (the bridge's + * `script_fields` key). Keying the projection off `identifierName` instead put `SELECT category, + * 2 FROM t GROUP BY category` under `2` while the row wanted `__c2`, so the requested column + * came back NULL and a bogus `2` column was invented beside it. + * + * ⚠️ NOT yet the only definition in the codebase: the same expression is still written out in + * `schema/package.scala`, `IndicesApi`, `HandshakeEvaluator` and extensions' `RequiredField` -- + * a follow-up, not a claim. `FromlessSelect.columnNames` is deliberately different and must stay + * so. Route any NEW consumer through this rather than re-deriving it: every key-desync defect in + * issue #253's story was one expression with two spellings. */ lazy val outputName: String = fieldAlias.map(_.alias).getOrElse(sourceField) @@ -121,7 +126,10 @@ case class Field( def script: Option[String] = identifier.script - lazy val scriptName: String = fieldAlias.map(_.alias).getOrElse(sourceField) + // The bridge's `script_fields` key -- the OTHER half of the coupling `outputName` exists to + // single-source. It kept a byte-identical second definition of the same expression 61 lines away; + // they agreed only by coincidence. + lazy val scriptName: String = outputName override def validate(): Either[String, Unit] = identifier.validate() 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 41a12a663..627b36ae9 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 @@ -321,13 +321,23 @@ package object query { * route, which is the very thing this fixes. */ lazy val rowInvariantProjection: ListMap[String, Any] = { - // No de-duplication against other SELECT items: the constants SEED `parseAggregations`' - // `parentContext`, so anything Elasticsearch computes is merged on the RIGHT of `++` and - // therefore WINS -- including a computed null, which a "never overwrite a non-null value" - // guard could not express. Precedence is structural, not a rule to remember. + // 🔴 A constant is NOT declared for a name some other SELECT item owns, and this guard is + // load-bearing rather than belt-and-braces. Seeding makes Elasticsearch win only when it + // actually EMITS a value: `extractMetrics` writes no entry for a null-valued metric, so + // nothing lands on the right of `++` and the constant would survive. MEASURED against a + // response whose `m` is `{"value": null}`, `SELECT category, 2 AS m, MAX(amount) AS m ... + // GROUP BY category` returned `m = 2` where SQL says NULL. Precedence at the merge is + // therefore necessary but not sufficient; the collision is excluded HERE, where the statement + // is known, so the merge never has to arbitrate it. + val claimedElsewhere = + select.fieldsWithComputedAliases + .filterNot(f => SingleSearch.isRowInvariantLiteral(f.identifier)) + .map(_.outputName) + .toSet ListMap( select.fieldsWithComputedAliases .filter(f => SingleSearch.isRowInvariantLiteral(f.identifier)) + .filterNot(f => claimedElsewhere.contains(f.outputName)) .map { f => val value = f.identifier.functions.headOption match { case Some(v: Value[_]) => v.value @@ -493,9 +503,7 @@ package object query { val nonAggregatedFields = select.fields.filterNot(f => f.hasAggregation) val invalidFields = nonAggregatedFields - .filterNot(f => - buckets.exists(b => b.name == f.fieldAlias.map(_.alias).getOrElse(f.sourceField)) - ) + .filterNot(f => buckets.exists(_.name == f.outputName)) // FOLD-IN 1: a ROW-INVARIANT literal is legal beside a GROUP BY in standard SQL -- // it cannot vary within a group, so it needs no bucket. It is not merely accepted: // `rowInvariantProjection` carries its VALUE into every aggregation row, because a diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala index 75ce803ef..50e2c5358 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/GroupByFoldInSpec.scala @@ -164,13 +164,14 @@ class GroupByFoldInSpec extends AnyFlatSpec with Matchers { parsed("SELECT category, ? AS p FROM t").rowInvariantProjection shouldBe empty } - it should "still declare a constant whose name an aggregation also claims" in { - // Precedence is settled downstream, not here: the constants SEED the aggregation recursion, so - // anything Elasticsearch computes overwrites them (asserted in `ElasticConversionSpec`). The - // AST layer therefore just reports what the statement declares. + it should "never declare a constant whose name another SELECT item owns" in { + // 🔴 Merge precedence alone is NOT enough: `extractMetrics` emits no entry for a null-valued + // metric, so nothing would land on the right of `++` and the constant would survive -- + // MEASURED, `SELECT category, 2 AS m, MAX(amount) AS m ... GROUP BY category` returned `m = 2` + // where SQL says NULL. The collision is excluded here, where the statement is known. parsed( "SELECT category, 2 AS m, MAX(amount) AS m FROM t GROUP BY category" - ).rowInvariantProjection shouldBe ListMap("m" -> 2L) + ).rowInvariantProjection shouldBe empty } it should "reject a GROUP BY over an unbound query parameter" in { diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index dc73d0e74..dfd81b8ae 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -365,20 +365,43 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit implicit val unionCtx: ConversionContext = NativeContext client.search( SelectStatement( - "SELECT category, 2 AS flag FROM group_by_completeness WHERE amount <= 1 GROUP BY category" + + "SELECT category, 2 AS flag FROM group_by_completeness WHERE amount >= 2 GROUP BY category" + " UNION ALL " + - "SELECT category, 3 AS flag FROM group_by_completeness WHERE amount <= 2 GROUP BY category" + "SELECT category, 3 AS flag FROM group_by_completeness WHERE amount >= 3 GROUP BY category" ) ) match { case ElasticSuccess(response) => - val flags = response.results.map(_("flag").toString).toSet + // 🔴 Pair the constant with a leg-distinguishing ROW COUNT: `Set("2","3")` alone would pass + // if the legs' seeds were swapped (an off-by-one in the per-leg lookup). `cat_i` holds + // amounts 1..i, so `amount >= 2` matches cat_02..cat_37 (36 groups) and `amount >= 3` + // matches cat_03..cat_37 (35) -- counts that differ, and differ from each other. + val byFlag = response.results.groupBy(_("flag").toString).map { case (k, v) => k -> v.size } withClue(s"rows=${response.results.take(4)}: ") { - flags shouldBe Set("2", "3") + byFlag shouldBe Map("2" -> (categories - 1), "3" -> (categories - 2)) } case ElasticFailure(error) => fail(s"Query failed: ${error.message}") } } + it should "return NO row when the grouping matches nothing" in { + // 🔴 The aggregation fold seeded itself with one EMPTY row, so a `WHERE` that excludes + // everything produced a phantom all-NULL row -- and with a seeded constant that phantom carried + // REAL values. Asserted for the aggregate-free, constant-bearing and aggregate-bearing + // spellings alike: the guard takes no "except". + implicit val emptyCtx: ConversionContext = NativeContext + Seq( + "SELECT category FROM group_by_completeness WHERE amount > 9999 GROUP BY category", + "SELECT category, 2 AS flag FROM group_by_completeness WHERE amount > 9999 GROUP BY category", + "SELECT category, COUNT(*) AS c FROM group_by_completeness WHERE amount > 9999 GROUP BY category" + ).foreach { sql => + client.search(SelectStatement(sql)) match { + case ElasticSuccess(response) => + withClue(s"[$sql] rows=${response.results}: ") { response.results shouldBe empty } + case ElasticFailure(error) => fail(s"[$sql] Query failed: ${error.message}") + } + } + } + it should "order an aggregate-free GROUP BY by a SELECT ALIAS of the grouped column" in { // 🔴 Regression guard for the alias/bucket key desync: `SingleSearch.sorts` is keyed by the // sort's name while `buildBuckets` looks the direction up under the bucket's resolved column,