Skip to content

fix(core): UNION ALL matches branch columns by POSITION, and the licensed cap reports truncation from a fact (#354, #355) - #357

Merged
fupelaqu merged 1 commit into
mainfrom
fix/354-355-union-all-positional
Sep 17, 2026
Merged

fupelaqu merged 1 commit into
mainfrom
fix/354-355-union-all-positional

Conversation

@fupelaqu

Copy link
Copy Markdown
Contributor

Closes #354
Closes #355

#354UNION ALL matched branch columns BY NAME, not by position

SQL-92 §7.10 matches set-operation branches by ordinal position and takes the first branch's column names; names play no part in the matching. CORRESPONDING — the optional clause that asks for name-based matching — is the proof, because nobody adds an opt-in for the behaviour they already have. (Almost nobody implements it; DuckDB went the other way and added a non-standard UNION BY NAME.)

Every route looked the first branch's names up in the leg's row instead — the path of least resistance from a wire format that is a name → value map — so a branch that named its columns differently lost its values, HTTP 200 throughout. Same family as #205 / #209 / #253: the query runs, returns rows, and the rows are wrong.

Three shapes, all measured on real Elasticsearch 8.18:

statement before now
SELECT a, b FROM x UNION ALL SELECT a, a FROM y branch 2 column 2 = NULL both columns hold branch 2's a
SELECT id AS x FROM l UNION ALL SELECT id AS y FROM r every row {x -> null, y -> …}, branch 1's own included one column x, both branches' ids
SELECT category, tag … UNION ALL SELECT tag, category … column 1 held branch 2's category column 1 holds branch 2's column 1

The second is the one an analyst hits: MultiSearch.fieldAliases merges the branches' maps keyed by the source field, so id AS x and id AS y collapse to one entry and the survivor renames both legs.

The fix

rowNormalizer becomes rowProjector(sourceFields, targetFields) — the names looked up in the row are decoupled from the output names, in the same single pass. rowNormalizer(f) is rowProjector(f, f), so every non-union path is unchanged by construction.

Each _msearch leg now builds its rows from its own aliases, projection and nested-hits mapping (new LegProjection) and is then renamed positionally onto branch 0's names — which is what search(leg) already gave the per-leg route, so the three routes agree because they do the same thing, not because one was taught to imitate the other. unionAllRowNormalizer becomes unionAllRowMappers, one mapper per leg, shared by the per-leg route and the licensed cap fold.

🔴 The trap this had to avoid, recorded because a story-22.6 draft fell into it: "position" means the index into the branch's declared projection, never an index into whatever order the row map enumerates. A re-key derived from row.keys put values under the wrong column names — HTTP 200, and strictly harder to detect than the raggedness it replaced, because the column names looked right. That is why rowProjector takes a name list and walks the row looking those names up.

Two shapes keep by-name matching on purpose, not by omission:

  • a first branch of SELECT * yields no names, so there is nothing to match against — each leg then keeps its own projection (its own names, its own aliases, never the merged map), which is what that leg would answer alone;
  • an opaque SELECT * leg past the first declares no projection at all (MultiSearch.declared is None and the arity/type guards exempt it), so a positional zip over _source order would put the id under the first branch's first column and drop everything past its width.

Two things a row MAP cannot express

Both decided once per stream, so the row loop pays nothing:

  • a result column name repeated at two positions keeps the first. Emitting both let the last win, so column 1 displayed column 2's value — and it diverged across cross-builds, because 2.13's ListMap builder replaces a duplicate key in place while 2.12's removes and re-appends it, so the same release answered with a different column order on the two artifacts;
  • an unrequested row entry carrying a result column's name is dropped rather than appended over it. Before the two lists could differ such a key was always found by the name index and could never become an "extra"; decoupling them opened the hole, and it is reachable via the parent of a dotted path (addr beside addr.city -> city), _id when the document-id column is on, and the internal aggregation keys a metric leaves behind.

#355 — the cap reported truncation from a guess, and the per-leg route resolved twice

The over-report. cappedUnionAllRows detected truncation inside a leg (exact) and at a leg boundary (a guess: dropping a leg was treated as truncation without asking whether that leg had rows). With every remaining leg empty the statement reported truncated = true, a non-empty warning advising a LIMIT the analyst had often already written, and a cap-hit — byte-identical in outcome to a genuine cut, so no consumer could tell them apart. The trigger is not exotic: a first branch whose LIMIT equals the quota, or legs summing to exactly the quota, followed by a branch that matches nothing.

The budget arithmetic now keeps working when it is spent: remaining == 0 asks the leg for 0 + 1 rows and keeps take(0) — an existence probe — and stops at the first leg that answers, because from there truncation is an established fact. Both directions are guarded, because a fix that merely stopped reporting at a boundary would reinstate the under-report this replaced (a genuinely truncated statement answering truncated = false with zero cap-hits — the flag and the meter agreeing and both wrong).

🔴 The probe is gated on SearchExecutor's own predicate (limit.isDefined || fields.isEmpty), not a proxy for it, because its whole job is to keep the probe off the scroll path. Measured: a returnsRows gate left SELECT amount, MAX(amount) AS m FROM y scrolled — it is not row-shaped, so SearchExecutor answers it as QueryStream(api.scroll(…)). A leg with its own LIMIT is executed as itself, because rewriting it would discard that bound and make the probe lie: LIMIT 0 contributes nothing by construction, yet a LIMIT 1 probe finds a row in it and reports a truncation nothing truncated — the very over-report this closes, needing no empty index at all.

The double resolution. unionAllByLeg re-entered search(leg) on legs the MultiSearch seam had already resolved, and resolveWithSchema is not a pure check — a leg carrying a WHERE subquery has its inner statement executed against Elasticsearch, uncached. The body of search's SingleSearch arm is lifted into searchResolved / searchResolvedAsync and the fold calls that. (Counting inner searches would be vacuous: phase one rewrites the subquery into literals, so the second pass finds nothing left to execute and the count stays at 1 either way. The test asserts seam entries — 3, where reinstating search(leg) makes it 5.)

Performance

Row shaping is the per-row hot path for every query, so it was probed against a control: the pre-change implementation, verbatim, interleaved A/B in one JVM (RowCostProbe — non-asserting, not collected by sbt test, following the ParseCostProbe convention).

The first implementation cost a consistent +3.6% on the commonest shape of all (a five-column projection Elasticsearch returns in SELECT order) because of one added per-entry guard. The loop now leaves early exactly as it did before and drains trailing entries afterwards, so the per-entry condition is byte-identical and parity holds by construction:

shape (ns/row) baseline current
5 cols, already shaped (fast path) 26.7–31.4 27.1–30.0
20 cols, already shaped 114–124 115–122
5 cols, reordered (rebuild) 66.5–71.0 67.5–70.7
5 cols + 2 extras 32.0–38.9 32.8–37.6
5 cols, one missing (rebuild) 56.8–62.3 60.0–59.9

Across three runs the delta is −4.5%..+5.6% with the sign flipping — no signal.

What the feature itself costs: a renaming leg is ~+27 ns/row (27 → 55 ns) on an already-shaped row, because renaming necessarily forgoes the "return the same instance" fast path; only +2–4% when the row needed rebuilding anyway. ~27 ms per 1M rows, about 0.2% of the published 11.91 s/1M extraction headline — and paid only by legs whose names differ from the first branch's. Homogeneous branches keep renames == false and the original fast path.

Per statement/leg: +29 ns (5 cols) / +53 ns (20 cols) to build a normalizer, plus one small LegProjection per leg. The parse path is untouched (no grammar change). #355's second half is a small win: N resolutions instead of 2N on the per-leg route.

Verification

  • core 1129 · sql 1325 · macrosTests 30 green; 2.12 cross-compile clean (it caught a classOf[X.type] in the probe — 2.13 only); scalafmtCheck + headerCheck clean.
  • New real-Elasticsearch rows in UnionAllCompletenessSpec (alias per branch one-shot, the same statement paged per leg, reordered projection) green on ES 6.8 rest + jest, 7.17, 8.18, 9.0; full es8java suite 470 green.
  • Every fix mutation-checked in both directions: restoring by-name matching reddens 5 rows and reproduces UNION ALL matches branch columns BY NAME, not by position: column 2 comes back NULL when the branches name their columns differently #354 case 2 byte-for-byte; restoring the boundary guess reddens 3; the opposite mutation (never report at a boundary) reddens 4, so the controls have teeth; reverting the probe gate leaves the aggregation-bearing leg scrolled; reverting the projection emission renames the SELECT * branch's own id column.

An independent adversarial review of the first implementation found 9 defects — three silent wrong answers (the target-name collision, the duplicate-target last-wins, and #354 case 2 surviving behind an opaque first branch) plus the LIMIT 0 false positive and the scrolled aggregation leg. All are fixed above and each has a test that reddens without it.

⚠️ Release notes

  1. Behaviour change, user-visible. A UNION ALL whose branches name their columns differently now returns different — correct — values. Where the branches agree on their names, which is every captured BI statement, nothing changes.
  2. A dropped leg that fails now fails the statement, where before it was never executed and the statement answered HTTP 200 with truncated = true.
  3. Binary-incompatible. multiSearch, multiSearchAsync, parseResponseTree, parseMultiSearchResponse and jsonToRows each gained a parameter, and unionAllRowNormalizer was removed. Downstream repos must rebuild on the next core bump.

Not in scope, for the lead to rule on

🤖 Generated with Claude Code

…reports truncation from a fact

Closes #354
Closes #355

SQL-92 §7.10 matches set-operation branches by ORDINAL POSITION and takes the
FIRST branch's column names; names play no part in it (`CORRESPONDING` is the
opt-in for name matching, and its existence is the proof). Every route looked the
first branch's names up IN THE LEG'S ROW instead — the path of least resistance
from a wire format that is a name → value map — so a branch that named its
columns differently lost its values, with HTTP 200 throughout. Three shapes,
all measured on real Elasticsearch 8.18:

  * `SELECT a, b FROM x UNION ALL SELECT a, a FROM y` — both parse-time guards
    admit it (same degree, same types) and column 2 came back NULL;
  * `SELECT id AS x FROM l UNION ALL SELECT id AS y FROM r` — `fieldAliases`
    merges the branches' maps keyed by the SOURCE field, so one alias survived
    and EVERY row, branch 1's own included, answered `{x -> null, y -> …}`;
  * a duplicate output name sent the row through a legacy per-row path with a
    requested-name SET, leaking internal aggregate keys and making the three
    routes disagree.

`rowNormalizer` becomes `rowProjector(sourceFields, targetFields)`: the names
LOOKED UP in the row are decoupled from the OUTPUT names, in the same single
pass. `rowNormalizer(f)` is `rowProjector(f, f)`, so every non-union path is
unchanged by construction. Each `_msearch` leg now builds its rows from its OWN
aliases, projection and nested-hits mapping (new `LegProjection`) and is then
renamed positionally — which is what `search(leg)` already gave the per-leg
route, so the routes agree because they do the same thing rather than because
one imitates the other. `unionAllRowNormalizer` becomes `unionAllRowMappers`,
one mapper per leg, shared by the per-leg route and the licensed cap fold.

Two shapes keep BY-NAME matching on purpose: a FIRST branch of `SELECT *` (no
names to match against — each leg then keeps its own projection, never the
merged alias map) and an opaque `SELECT *` leg, which declares no projection at
all, so a positional zip over `_source` order would put the id under the first
branch's first column and drop everything past its width.

A row MAP cannot express two things the standard allows, and both are decided
once per stream rather than per row: a result column name repeated at two
positions keeps the FIRST (emitting both let the LAST win, so column 1 showed
column 2's value — and it diverged across cross-builds, 2.13's ListMap builder
replacing a duplicate key in place where 2.12's removes and re-appends it), and
an unrequested row entry carrying a result column's name is dropped rather than
appended over it (before the lists could differ such a key was always found by
the name index and could never be an "extra"; reachable via the parent of a
dotted path, `_id`, and internal aggregate keys).

#355, first half — the licensed cap treated "a leg is being dropped" as
truncation without asking whether that leg had rows, so a statement whose
remaining legs were all EMPTY reported `truncated = true`, a warning advising a
LIMIT the analyst had often already written, and a cap-hit, byte-identical in
outcome to a genuine cut. The budget arithmetic now keeps working when it is
spent: `remaining == 0` asks the leg for `0 + 1` rows and keeps `take(0)` — an
existence probe — and stops at the first leg that answers. The probe is gated on
the same predicate `SearchExecutor` routes on (`limit.isDefined ||
fields.isEmpty`), not a proxy for it, so the shape that would otherwise be
scrolled is the shape that gets `LIMIT 1`: a `returnsRows` gate left the
aggregation-BEARING-but-not-grouped shape SCROLLED, measured. A leg with its own
LIMIT is executed as itself, because rewriting it would discard that bound and
make the probe lie (`LIMIT 0` contributes nothing yet a `LIMIT 1` probe finds a
row in it).

#355, second half — `unionAllByLeg` re-entered `search(leg)` on legs the
`MultiSearch` seam had already resolved, and `resolveWithSchema` is not a pure
check: a leg carrying a WHERE subquery has its inner statement EXECUTED,
uncached. The body of `search`'s `SingleSearch` arm is lifted into
`searchResolved` / `searchResolvedAsync` and the fold calls that instead.

Row shaping is the per-row hot path for every query, so it was probed against a
control — the pre-change implementation, verbatim, interleaved A/B in one JVM
(`RowCostProbe`, non-asserting, not collected by `sbt test`). The first
implementation cost a consistent +3.6% on the commonest shape of all because of
one added per-ENTRY guard; the loop now leaves early exactly as it did before
and drains trailing entries afterwards, so the per-entry condition is identical
and the measured delta is −4.5%..+5.6% across runs with the sign flipping. A
renaming leg costs ~+27 ns/row (renaming forgoes the same-instance fast path) —
only for legs whose names differ from the first branch's.

Coverage: positional matching, the alias-per-branch shape, the reordered branch
and the duplicate projection on BOTH routes, the opaque-first-branch alias map,
the truncation matrix in both directions with three controls, the probe's shape
and its stopping rule, and the resolution count. Green on real Elasticsearch
6.8 (rest + jest), 7.17, 8.18 and 9.0.

BREAKING: `multiSearch`, `multiSearchAsync`, `parseResponseTree`,
`parseMultiSearchResponse` and `jsonToRows` each gained a parameter, and
`unionAllRowNormalizer` was removed — binary-incompatible, downstream rebuild.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fupelaqu
fupelaqu force-pushed the fix/354-355-union-all-positional branch from 2b5cdb8 to 7250177 Compare September 17, 2026 09:38
@fupelaqu
fupelaqu marked this pull request as ready for review September 17, 2026 10:25
@fupelaqu
fupelaqu merged commit 7187c7d into main Sep 17, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant