fix(core): prevent unbindable preaggregation queries, harden preagg tests - #1177
Draft
domoritz wants to merge 6 commits into
Draft
fix(core): prevent unbindable preaggregation queries, harden preagg tests#1177domoritz wants to merge 6 commits into
domoritz wants to merge 6 commits into
Conversation
21 tasks
domoritz
force-pushed
the
sql-fix/preagg
branch
from
September 3, 2026 02:22
1431d36 to
bc20db0
Compare
domoritz
marked this pull request as ready for review
September 3, 2026 02:30
Fixes #1189. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #1187. Mean-centering builds a scalar subquery over the base table, so an operand that only exists in an intermediate CTE, or that is aggregate- or window-valued, cannot be resolved there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #1188. A window function with no aggregate inputs was classified as a groupby dimension, so the preaggregate CREATE TABLE query placed its alias in GROUP BY and failed to bind (GROUP BY clause cannot contain window functions). Bail out of the optimization instead; the coordinator then serves the client through the standard query path. Related to #259, which covers invalid client queries and a potential CTE-based rewrite; that broader approach is not attempted here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #1190. Column references inside verbatim SQL text (e.g. sql-template selection fields) can not be identified by collectColumns, so they can not be pushed down into client query CTEs/subqueries. Bail out of the preaggregation optimization in that case; the coordinator falls back to the standard client query. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Part of #1170. The window and verbatim bail-outs both walk an expression looking for a single node class, so give them a common `containsNode` helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Part of #1170. `NaN` forced callers into `typeof` checks that read as type accidents rather than as a test for base queries that disagree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
domoritz
force-pushed
the
sql-fix/preagg
branch
from
September 3, 2026 02:46
bc20db0 to
4ad7082
Compare
domoritz
marked this pull request as draft
September 3, 2026 03:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1189, #1187, #1188, #1190. Part of the audit tracked in #1170.
The preaggregator emits an unbindable
CREATE TABLEin four different shapes. Since #1090 (and now #1158) the coordinator recovers by falling back to the client's query, so every one of these fails silently as a lost optimization — charts are correct, the speedup just never happens. Relatedly,preaggregator.test.tsmostly asserted!!info, which is also true when the create fails and the fallback answers; that is why none of these were caught. One commit per issue, plus two droppable refactors:fix: Qualify preagg mean-centering subquery table refs—getBasekeeps the full serialized table path ("s"."data"), restoring the optimization for schema-qualified tables (and fixing cross-schema base-identity comparison as a side effect).fix: Bail out of preaggregation when mean-centering CTE-derived columns— four explicit guards (divergent bases / aggregate-valued / window-valued / unresolvable-through-subqueries operands) throw into the existing bail channel.fix: Skip preaggregation for window functions without aggregate inputs— a plain window function among selection outputs skips the optimization instead of landing inGROUP BY(conservative interim relative to Window functions do not work in query with aggregate #259's CTE-hoisting idea).fix: Skip preaggregation when verbatim active columns meet subqueries—preaggregateInforeturns null when verbatim selection fields can't be pushed into client CTEs.refactor(core): Share one node-presence test across preagg bail-outs— dedupes the walk helpers intocontainsNode(newpreagg/contains-node.ts).refactor(core): Use a symbol for getBase's divergent-base result— replaces the NaN sentinel; callers readexpr === DIVERGENTinstead oftypeof expr === 'number'.Commits 5–6 are separately droppable (reviewer-verified they revert cleanly in order) if you'd rather keep this PR fixes-only.
Relationship to #1158
Complementary, verified empirically per fix: #1158 recovers after a failed view create; these guards prevent the invalid
CREATE TABLEfrom being issued at all (cases 2–4), removing the error/log noise the recovery path would absorb. Case 1 is the only one that makes preaggregation work rather than bail.Test hardening
The schema-qualified test probes the materialized view directly; the bail-out tests assert the non-optimized route plus correct values. Six new tests total (core 72/72); revert checks reproduce exactly the expected per-fix failures.
🤖 Generated with Claude Code