GROUP BY without an aggregate returns one row per group by fupelaqu · Pull Request #299 · SOFTNETWORK-APP/SoftClient4ES

fupelaqu · 2026-09-07T07:58:04Z

Closes #253
Closes #295
Closes #296
Closes #297
Closes #298

Story 21.3 — an explicit GROUP BY is aggregation-shaped, whatever the SELECT list contains.

The defect

SELECT category FROM t GROUP BY category — the SQL-standard spelling of DISTINCT, and 22 of the 99 captured BI statements — returned per-document rows instead of one row per group. Nothing failed; the row count tracked the fixture size, capped at the licensed maxQueryResults. The fifth member of the #205 / #207 / #209 / #224 family.

Root cause was one expression: returnsRows = windowRowQuery || sqlAggregations.isEmpty. With no aggregate anywhere, sqlAggregations is empty, so an explicit GROUP BY was invisible to the shape discriminator. The scaladoc on that val already promised false for GROUP BY — the expression was the bug, not the contract.

21 of those 22 statements carry no LIMIT and were rejected only by the quoting gap that stories 21.1/21.2 closed. Without this story they would have flipped from a loud parse error to a silent wrong answer.

What is in here


#253	`returnsRows` accounts for `groupBy`; aggregate-free `GROUP BY` executes as a `terms` aggregation
#298	ordinal detection moved off a regex over the rendered name onto the AST — `GROUP BY city2` / `logs2025` no longer crash or mis-resolve
#296	`GROUP BY <select alias>` resolves to the aliased field instead of a non-existent one
#297	a constant in a grouped SELECT list is legal, and carries its value on every row
#295	`OFFSET` under `GROUP BY` is rejected loudly instead of silently dropped
—	ordinal `ORDER BY <n>`, which parsed and was silently discarded, now resolves

The emission layer needed no new plumbing for #253 itself: the terms aggregation, the size: 0, the bucket-key projection and the row normalisation all already existed and were exercised by the aggregate-bearing form.

Behaviour changes to release-note

SELECT col FROM t GROUP BY col returns one row per group. The old output was wrong.
Result variant moves QueryStream → QueryStructured for an un-LIMITed aggregate-free GROUP BY on the licensed gateway.
A high-cardinality aggregate-free GROUP BY may now fail loudly with too_many_buckets_exception instead of returning a plausible wrong page. That is the GROUP BY without LIMIT silently returns only the top 10 groups (silent wrong answer) #205 design intent; add a LIMIT.
ORDER BY <n> already parsed and was silently ignored; it now resolves, so a query may come back in a different — correct — order. ORDER BY 0, ORDER BY -1 and out-of-range positions are now rejected.
OFFSET with GROUP BY now errors (was: silently dropped, on the aggregate-bearing form too).
GROUP BY <select alias> now returns real groups where it returned zero rows with HTTP 200.
A GROUP BY over an empty result now returns no rows. It previously returned one all-NULL phantom row — including for the aggregate-bearing form, which has behaved that way since before 0.22.0.
A substituted bucket re-emits the SELECT alias, so ordinals normalise to the alias (GROUP BY 1 renders GROUP BY cat), not away from it. Anything pinning generated SQL is affected.
SELECT x AS y ... ORDER BY <n> sorts on y, a field Elasticsearch has no mapping for unless y is real. The ORDER BY y spelling already behaved this way.
Binary-incompatible: Bucket arity 2→5, FieldSort 4→7, a defaulted parameter on eight methods, plus both overriding registries. Downstream repos must rebuild; no sibling source change is needed (verified by named-token grep across jdbc / arrow / extensions).
Known divergence, deliberately not 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 was not wrapped to match the defect.

Verification

sql 823 · core 905 · softclient4es-sql-bridge 197 · es6bridge 197, on 2.13.16 and 2.12.20, bridge suites run alone with real test counts. GroupByCompletenessSpec 19/19 on real ES 8.18; 18/18 across ES 6.8 (rest + jest) / 7.17 / 8.18 / 9.0 as of the previous commit, with 31/31 sibling guards (#197 / #205 / #207 / #209 / #224 / #238) on ES 8. Corpus diff: 1,408 harvested statements through parsers built from both trees — 33 verdict changes, every one a GROUP BY / ORDER BY shape; inputs reaching #250's parser boundary catch: 0.

build.sbt:23 verified 0.23.0-SNAPSHOT and untouched — the line is owned by story 21.4's bump.

Review record

Three independent review rounds by two fresh-context reviewers, in held-PR mode: the branch was pushed with no PR so every finding could be fixed in-branch rather than shipped.

Round 1 — 11 findings (4 HIGH). Three were regressions this branch introduced: the alias resolution fixed the terms field and silently desynchronised the terms order, the include/exclude, and the render's fixed point from it. Every bridge JSON test had passed because none exercised an aliased ORDER BY or HAVING — the coverage gap and the defect had the same shape.
Round 2 — the ORDER BY fix was falsification-proved redundant: disabling it left all eight ordering shapes correct and both suites green, while it was the only new code that rewrote a window's OVER (...) clause. Deleted.
Round 3 — the constant projection was a second pass over every row (two ListMap rebuilds each, O(rows × cols × constants) over up to 65,536 buckets per level). Re-seated onto the aggregation-row recursion's existing parentContext: zero extra traversals, zero extra allocations. That also removed a boolean guard by making it structural, closed the UNION ALL gap by construction, and made Field.outputName the single derivation two paths had been deriving twice.
Final — an empty grouping returned a phantom all-NULL row (newly reachable for this shape); Field.scriptName was still a byte-identical twin of outputName; and the claim that a computed value always beats a constant was measurably false for a metric Elasticsearch computes as NULL, which emits no entry to arbitrate with.

Every rejection assertion also asserts not startWith Parser.InternalParseFailure — after #250's boundary catch, noException + isLeft passes with a throw restored, so without it the guards would have been unfalsifiable.

🤖 Generated with Claude Code

…t (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 <n>` 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 <n>` 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 <select alias>` emitted `terms { field: "<alias>" }` on a field that does not exist and Elasticsearch answered ZERO buckets with HTTP 200 -- a silent empty result, on the aggregate-bearing form too. It now resolves to the aliased field through the same AST-shape discipline. `OFFSET` under a `GROUP BY` was silently dropped by the aggregation emission branch and is now rejected, following the in-file NULLS FIRST / NULLS LAST precedent. Three defects found while implementing and fixed here rather than filed: - ordinal resolution was NOT idempotent. A resolved position naming a bare SELECT literal is itself ordinal-SHAPED, and `SingleSearch.update()` really does run twice on one statement (`Table.mergeWithSearch` re-updates an already-parsed search), so a second pass re-read the literal as a position -- measured re-pointing `GROUP BY COL2` onto `SUM(amount)`. Fixed with a `resolved` latch on `Bucket` and `FieldSort`. - a bucket over a constant emitted a `terms` with neither `field` nor `script`, which Elasticsearch rejects outright. Pre-existing and reachable on main through `GROUP BY 'x'`; now scripted. - the render of a constant bucket was not a fixed point: `GROUP BY COL2` re-emitted `GROUP BY 2`, which re-parses as POSITION 2. `MaterializedViewExtension` persists that render and re-runs it, so it was a live corruption. A constant bucket now re-emits its alias, and one with no alias to name it is rejected. Verified: 1,408 SQL statements harvested from this repo's own test sources were run through parsers built from both trees -- 35 verdict changes, every one intended, and inputs reaching `Parser.apply`'s NonFatal boundary went from 12 to 0. Unit suites green on 2.13.16 and 2.12.20 (sql 818, core 898, bridge 191 x2); `GroupByCompletenessSpec` green on real ES 6.8 rest + jest, 7.17, 8.18 and 9.0, with the #197/#207/#209/#224/#238 sibling guards green on ES 8.

…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.

… 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.

…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.

fupelaqu added 4 commits September 7, 2026 06:55

fupelaqu marked this pull request as ready for review September 7, 2026 09:00

fupelaqu merged commit 29a2556 into main Sep 7, 2026
4 checks passed

Navigation Menu

GROUP BY without an aggregate returns one row per group - #299

GROUP BY without an aggregate returns one row per group#299
fupelaqu merged 4 commits into
mainfrom
feature/21.3

fupelaqu commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

fupelaqu commented Sep 7, 2026

The defect

What is in here

Behaviour changes to release-note

Verification

Review record

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant