fix(sql): a HAVING over a function of an aggregate must filter, or fail — never vanish (#389)

[fupelaqu]

Closes #389

A HAVING predicate whose aggregate sits inside a function argument emitted no bucket_selector: the clause vanished and every group came back, HTTP 200. The #205 / #253 silent-wrong-answer family.

The cause was not the one the issue named

#389 blamed ElasticAggregation.metricSelectorForBucket's if (fullScript.isEmpty) return "". Measured: that is the second line of defence. One rung up, in sql, MetricSelectorScript.metricSelector falls to its case _ => "1 == 1" catch-all because Expression.referencesBucketMetric is false — a function never looks inside its own arguments, so COUNT(*) within ABS(...) / COALESCE(...) / a CASE branch is invisible to isAggregation and hasAggregation. That single fact explained the dropped clause, the missing auxiliary aggregation, and why the whole-table guard whose comment promises "nothing may fall through silently" did not fire.

Three things the issue does not record, all measured on main: an AND conjunct was silently dropped (a partial filter — the answer looks filtered and is wrong), an OR disjunct likewise, and the whole-table HAVING took the same path.

What ships

A function-wrapped aggregate in HAVING now emits a correct bucket_selector, or the statement is refused by name. Never dropped, never partially applied.

• HAVING COALESCE(COUNT(*), 0) > 1 and GREATEST(...) forms emit and run.
• NULLIF / ROUND / CASE / boxing forms are refused, naming the clause and the reason. The gate is structural, on the rendered Painless — not a function-name list, which would be a fourth derivation of the renderer. ⚠️ The bucket-pipeline Painless context has a different whitelist from the query context: Double.valueOf(x) compiles in one and not the other. Only execution settled that.
• Conditions on the GROUP BY key are honoured at bucket level by the terms include/exclude filter, or refused. The channel unions, it never intersects — an OR across two grouping levels is a conjunction, because Elasticsearch nests the terms aggregations.
• Four shapes that reached Elasticsearch as HTTP 400 are now named rejections — one of them leaked raw SQL into the Painless source, one surfaced Internal parser error to the user.
• Having.script is restored (@deprecated): it has a production caller in softclient4es-extensions (graph/Stage.scala:291), contrary to what an earlier round of this branch asserted.
• Error plumbing: a refusal now reaches the caller on search, searchAsync, searchWithInnerHits and ScrollApi, not just search.

Verification

sql 1635 · core 1141 · bridge 238 · es6bridge 234 · + compile (2.12.20 + 2.13.16) · the exact CI lint line — all green. Integration re-run after the final change: 47/47 on real ES 8.18.3, 47/47 on real ES 6.8.23.

Four independent fresh-context reviews, each over the commits the previous one had not seen, executing against hand-computed oracles on real clusters. The last measured 0 surviving wrong answers over its own independently generated 309-statement population.

Behaviour changes — release-note material

1. Statements that returned every group now return the right groups, or fail. Dashboards built on the old answer will show fewer rows.
2. Previously-accepted statements are now refused by name — and, contrary to what this PR body originally said, they were not working. ⚠️ CORRECTION, measured on 455433ae after the merge (verdict AND emitted query): GROUP BY t.status HAVING t.status = 'a', GROUP BY t.status HAVING status = 'a', GROUP BY age HAVING age > 0, GROUP BY status HAVING status IS NOT NULL and the JOIN form GROUP BY d.category HAVING d.category = 'x' were all ACCEPTED on main and emitted no include at all — the condition was silently dropped and every group was returned. That is HAVING <function>(<aggregate>) silently drops the entire HAVING clause — every group is returned #389 itself, in the group-key population; the refusal makes the same defect loud and nothing regressed. Only the UNQUALIFIED direct comparison (HAVING x = 'v', <>, IN, LIKE) was ever correct, and it is unchanged. The original wording ("correct only coincidentally") relayed a reviewer's classification that had not been measured. Aggregation over JOINs is unaffected — GROUP BY d.category HAVING COUNT(*) > 1, GROUP BY d.category, o.kind HAVING COUNT(*) > 1 and WHERE age > 0 GROUP BY age HAVING COUNT(*) > 1 are all accepted (measured on the merged tree). Residual, now filed as a follow-up: a qualified key should resolve to its bucket and does not — same family as SQL parser: support backtick-qualified catalog names natively #85 / Two tables differing only by qualifier collapse into one alias, silently dropping an alias #292.
3. HAVING <key> LIKE now uses the shared translation. _ becomes . and regex metacharacters are escaped, so LIKE 'a_' — which matched nothing — now matches, and LIKE 'cat_3%' emits cat.3.* rather than cat_3.*. No test pin anywhere used such a literal on this channel, which is how it survived.
4. Binary compatibility: additive. No downstream rebuild required by this PR.

Recorded, not fixed — pre-existing and byte-identical to main

• ES 6.8 only: a single-valued include/exclude renders as a bare string that ES 6.8 reads as a regex, so HAVING status = 'a.b' returns bucket axb. Wider than first recorded: a pattern in one channel beside a multi-value list in the other is HTTP 400 there (three further shapes).
• child(...) / nested(...) puts an include on the parent bucket key.
• HAVING k = 'a' OR child(LENGTH(k) = 1) executes as its first branch.
• The vanished GROUP BY in a JOIN UNNEST + root-metric shape — a separate defect on a different mechanism. That statement is no longer answered 200-with-a-wrong-answer; it is refused.

A capability that was built and then dropped

A case-fold inversion (HAVING UPPER(key) = 'A' → a terms include regex) was built across two rounds and removed by decision. It produced three silent wrong answers and a remotely-triggerable OutOfMemoryError from a ~30-character string literal. The structural reason it was dropped: it inverts a case fold performed by a JVM the client has never seen. Reading the client JVM made answers depend on the client's JDK (96 patterns differ between zulu-11 and zulu-21); freezing a table relocated the divergence to the node (ES 7.17 → Unicode 15, ES 8.18/9.0 → Unicode 16, versus a pinned Unicode 13 — 134 extra cased code points, with a missing bucket proved end-to-end). The work is preserved locally and the reasoning is recorded so it is not rebuilt the same way.

One lesson worth carrying

Every defect found on this branch — across six dev rounds and four reviews, including two rounds where the fix shipped something worse than what it fixed — was a hole in the population under test, never in the assertions. The mutation matrices were sound each time. The failures were all absences — a bucket that is not there — which only an independently computed expected set can see.

🤖 Generated with Claude Code

Cost — measured on this tree (1a6a23a1), reported, not asserted

9 interleaved rounds per side against a 455433ae control in one session, with six must-be-zero baseline shapes (bare projection, WHERE-only, wide WHERE, plain GROUP BY, 2-key GROUP BY, ORDER BY) carrying the noise floor. No timing assertion is added anywhere (#269/#270).

Search — nothing to degrade. Of the 99 well-formed HAVING statements present in the control tree's own sources, 99 emit byte-identical queries. Zero changed, zero newly refused, zero refused→emitted. The only queries that move are ones the control emitted wrong (a bucket_selector reading params.max_x with no max_x aggregation — that query fails at runtime) or did not emit at all (#389). Their extra ES work is one metric sub-aggregation plus a coordinating-node pipeline. RowCostProbe was deliberately not run: every core/src/main hunk here is in a failure branch (invalidSearchRequest and its 5 call sites, ScrollApi's Source.failed), so nothing on the row-extraction, paging or response-parsing path is touched.

Parse + validate (µs, median of 9 rounds × 500 runs; floor over the 6 baseline shapes = −4.6 … +1.7):

shape	control	branch	Δ	verdict
HAVING COALESCE(COUNT(*),0) > 1	318.8	329.3	+10.5	signal, +3.3% (paired +12.5; ranges non-overlapping)
HAVING COUNT(*) > 1	270.0	272.9	+2.9	marginal (~1%)
HAVING COUNT(*) > 1 AND MAX(a) < 9	323.6	325.7	+2.1	marginal (~0.6%)
HAVING n > 1 (alias)	258.7	260.0	+1.3	no signal
HAVING code = 'x' (GROUP BY key)	262.5	256.9	−5.6	no signal
every non-HAVING shape	—	—	inside floor	no signal

Resolution columns, same run (floors: resolve ±0.4, validate ±0.2): a metric-bearing GROUP BY … HAVING resolves in 2.3 → 6.6 µs, and COALESCE(COUNT(*),0) in 2.4 → 8.9 µs. A ratio of ~3×, ~4–6 µs in absolute terms against a ~270 µs parse. A GROUP-BY-key HAVING pays nothing (1.5 → 1.8 µs).

Rendering / emission (fresh parse + ElasticSearchRequest.query; floor ±1.3 µs on min): metric-bearing HAVING costs +3.7 to +15.2 µs on a 329–348 µs parse+emit (≈1–4.5%), scaling with metric-leaf count — 1 leaf +6.0, 2 leaves +15.2. Key-only HAVING (=, IN, LIKE, <>) and every non-HAVING shape sit inside the floor.

Mechanism (reasoning, corroborated by the leaf-count scaling): MetricSelectorScript.selector now calls representable(e) per metric leaf — a painless(None) render plus a StringBuilder scan. That is the gate that converts four HTTP 400s and the silent drop into named refusals. It runs once per metric leaf at parse/emit time, never per document. Key-only leaves return NoFilter before reaching it, which is why they measure flat.

Two caveats stated rather than hidden: the branch ran first in every round (a fixed position, absorbed by the baseline floor — which is why the floor is reported), and the newly-refused statements are not counted as "slower" — a refusal short-circuits emission, and two of them measure faster.