fix(sql): fix UNION ALL column mismatch under aggregates

[nwoolmer]

Problem

An aggregate (count(), sum(), ...) over a UNION ALL of aliased sub-queries crashed query compilation with an AssertionError, which surfaced as a 500 in the web console and a "failure when getting column type" error. Minimal reproducer:

select count() from (
    select a, close price from u1
    union all
    select a, close price from u2
);

It was originally reported through a view built as a UNION ALL of LATEST ON sub-queries, but LATEST ON is incidental — it only forces the per-branch CHOOSE-wrapper structure that keeps the projection aliases distinct.

Root cause

SqlOptimiser.propagateTopDownColumns0() emitted a model's columns to its UNION sibling by name. The optimizer's column-propagation machinery resolves literals against the target's alias map, so across a UNION boundary it only matches columns whose source name happens to equal the sibling's alias. The emit also fires only for the union head at non-top-level, so it reaches just the head's immediate sibling.

When the outer query selects nothing from the union (an aggregate), this pruned that one branch down to the few matching-by-name columns while leaving the other branches intact. The branches then disagreed on column count, and SqlCodeGenerator.checkIfSetCastIsRequired() tripped its columnCount == metadataB.getColumnCount() assertion.

Fix

UNION columns are matched by position, not name. The indexed, by-position propagation added in #6744 already handles cross-branch column propagation correctly, so this removes the superseded by-name emit.

• For same-alias unions (the common case) the two mechanisms are equivalent — no plan change.
• For differently-aliased unions the by-name emit was silently wrong; by-position propagation is correct.

Rollback flag

cairo.sql.legacy.union.column.propagation (default false) restores the old by-name emit, as an operational rollback if the optimizer change has an unforeseen effect on a workload. EntPropServerConfiguration and EntCairoConfigurationWrapper extend the OSS classes, so enterprise servers honor the flag with no extra wiring; the interface getter is a default method, so no other config implementer needs changes.

Tradeoffs and risk

• The change removes a code path rather than adding one, so the main risk is mis-pruning union branches elsewhere. This was checked across union shapes (subset projection, nested unions, union in join, UNION vs UNION ALL, GROUP BY and LATEST ON branches): the by-position loop runs earlier and over the full sibling chain, so it subsumes the removed emit.
• The ~1079 SqlParserTest model assertions, which print each branch's pruned column list, are unchanged — query plans are stable.
• Net effect on the compile path is one fewer tree-walk; there is no data-path impact.

Tests

• UnionTest.testAggregateOverUnionAllOfLatestOn — count() and sum() over a 4-branch UNION ALL of LATEST ON sub-queries.
• UnionTest.testAggregateOverUnionAllWithAliasMismatch — minimal 2-branch case with no LATEST ON, isolating alias divergence as the trigger.
• UnionTest.testAggregateOverUnionAllLegacyFlagReintroducesCrash — with the rollback flag on, the otherwise-fixed query fails again, guarding the switch against becoming a no-op.
• ViewQueryTest.testAggregateOverUnionAllOfLatestOnView — the original report, through a CREATE VIEW (one table plus CREATE TABLE ... (LIKE ...) clones).
• The first three (flag off) fail without the fix (AssertionError) and pass with it.
• Regression-checked green: UnionTest, UnionAllCastTest, UnionStringyCastTest, FilterPushdownIntoUnionTest, LatestByTest, ParallelLatestByTest, SqlParserTest, SqlOptimiserTest, SqlCodeGeneratorTest, PropertyKeyTest, PropServerConfigurationTest, the view test suite, GroupByTest, DistinctTest, ViewFuzzTest, MatViewFuzzTest.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 53611c89-89d2-4112-8d52-3ab274a4893d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-union-aggregate-column-mismatch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nwoolmer]

Code review (automated, level 2)

Multi-agent review across correctness + caller-impact, performance + adversarial-performance, test coverage, and code-quality/conventions. Each finding was verified against source before reporting. Concurrency / resource / Rust / cross-context passes had no surface on a single-threaded, no-Rust, no-resource compile-path removal.

Critical

None.

Moderate

None outstanding. One coverage gap was found and fixed within this commit (see Minor #2).

Minor — both found independently by two agents, both fixed in this commit

1. Member ordering — testAggregateOverUnionAllOfLatestOnView was appended at the end of ViewQueryTest; QuestDB sorts members alphabetically, so it belongs before testCreateConstantView. Relocated.
2. Root-cause coverage gap — the original tests all used LATEST ON, but the real trigger is alias divergence between branches (LATEST ON only forces the CHOOSE-wrapper structure). Verified by temporarily restoring the by-name emit and confirming select count() from (select a, close price from u1 union all select a, close price from u2) crashes with the same AssertionError pre-fix. Added minimal testAggregateOverUnionAllWithAliasMismatch (2 branches, no LATEST ON).

Downgraded (false positives)

None. Both deep agents reached NO ISSUE / NO REGRESSION on the fix, with structural and empirical backing:

• Correctness — the removed by-name emit fired only for the union head at !topLevel, reaching only the immediate sibling (topLevel is consumed in exactly two places; siblings are visited at topLevel=true). The retained indexed by-position loop is a strict superset. Checked head-empty-topDown (count/sum), subset projection, nested unions, union-in-join, UNION-distinct (allowsColumnsChange()==false ⇒ keep-all), and odd-branch-not-2nd. No case where removal drops or mis-positions a column.
• Performance — no branch retains more columns than before; the ~1079 SqlParserTest bracketed-column model assertions are unchanged, confirming plan stability; the hunk is a net compile-path reduction.

Summary

• Verdict: approve. Clean removal of a 2020-era by-name cross-union emit that fix(core): fix assertion errors in UNION ALL with column count mismatch #6744 already superseded with by-position propagation.
• Findings: 2 unique (1 minor ordering, 1 test-coverage), both resolved here. 0 correctness / concurrency / resource / performance findings. 0 false positives.
• In-diff vs out-of-diff: all findings in-diff; the only out-of-diff surface (code-generation consuming pruned models) was confirmed safe via the unchanged parser/codegen suites.

🤖 Generated with Claude Code

[nwoolmer]

Code review (automated, level 3 — full mission-critical pass)

10 agents (correctness, concurrency, performance, resource-management, tests, code-quality, PR-metadata, cross-context/enterprise, fresh-context adversarial, adversarial-performance; Rust agent skipped — no .rs). Every finding verified against source. The fix itself (the optimizer change + config wiring) drew no production findings — agents independently confirmed the gated block is byte-identical to the removed code, configuration is non-null, the getter is a pure final-boolean read, and enterprise honors the flag through the delegation chain (EntPropServerConfiguration/EntCairoConfigurationWrapper both reach the OSS property read; the default method means no enterprise change and no compile break).

All findings below were fixed in 892d580.

Critical

1. ServerMainTest.testShowParameters would fail CI (in-diff consequence, out-of-diff test) — SHOW PARAMETERS enumerates every PropertyKey and the test asserts an exhaustive expected list; the new cairo.sql.legacy.union.column.propagation row was missing → "Extra properties". Registered the row (mirroring the cairo.sql.legacy.operator.precedence sibling); test now passes. Enterprise ShowAclTest.testShowParameters is unaffected — it filters to EntPropertyKey.values() only (verified).

Moderate

2. Test isolation — rollback-flag leak — testAggregateOverUnionAllLegacyFlagReintroducesCrash set the flag via setProperty, but overrides are cleared only at @AfterClass and there is no @FixMethodOrder (JUnit's order is hash-based, not alphabetical), so the enabled flag could leak into later UnionTest methods and crash them. It passed only by luck of order. Now reset in a finally.
3. Over-broad catch — the same test caught AssertionError | Exception, so it could have gone green on an unrelated failure (e.g. a SQL typo). Narrowed to catch (AssertionError e) — the specific code-gen assertion; re-ran and confirmed it still passes (the thrown type really is AssertionError).

Minor

4. Unrolled the CREATE TABLE ... (LIKE ...) loop in ViewQueryTest into three explicit statements.

Downgraded (false positives)

None — the agents were disciplined. Notable non-issues they verified rather than assumed: concurrency (final field, safe publication, correctly excluded from the dynamic-reload set); performance (net compile-path win — skipNoneTypeModels is now computed only when the flag is on); resource-management (the throwing select() path frees factories in generateSetFactory's catch, and assertMemoryLeak's check runs after the test's catch so it can't be suppressed); CREATE TABLE LIKE preserves WAL + dedup keys + designated timestamp, so the view test isn't weakened.

Summary

• Verdict: approve after the four fixes (all landed in 892d580).
• The production change (optimizer fix + rollback flag) is clean: 0 correctness/concurrency/perf/resource/cross-context findings. All four findings were test-side.
• Re-verified green: UnionTest (50), ViewQueryTest (38), ServerMainTest.testShowParameters, plus the earlier SqlParserTest/SqlOptimiserTest/SqlCodeGeneratorTest/PropertyKeyTest/PropServerConfigurationTest sweep.

🤖 Generated with Claude Code

[bluestreak01]

/azp run macwin

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mtopolnik]

[PR Coverage check]

😍 pass : 15 / 15 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/PropertyKey.java	1	1	100.00%
🔵	io/questdb/cairo/CairoConfigurationWrapper.java	1	1	100.00%
🔵	io/questdb/griffin/SqlOptimiser.java	10	10	100.00%
🔵	io/questdb/PropServerConfiguration.java	2	2	100.00%
🔵	io/questdb/cairo/CairoConfiguration.java	1	1	100.00%