fix(db): support aggregates nested inside expressions (#720)#1274
Merged
fix(db): support aggregates nested inside expressions (#720)#1274
Conversation
Add failing tests that reproduce the issue where aggregates wrapped inside other expressions (e.g. coalesce(count(...), 0)) throw QueryCompilationError: Unknown expression type: agg. Tests cover coalesce wrapping count/sum, add combining two aggregates, mixed plain and wrapped aggregates, and subquery join sources — in both @tanstack/db and useLiveQuery. Ref: #720 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: fd16336 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: +464 B (+0.5%) Total Size: 92.5 kB
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 3.7 kB ℹ️ View Unchanged
|
Extract nested aggregates (e.g. coalesce(count(...), 0)) into synthetic aliases, register them with the groupBy operator, and evaluate the wrapper expression post-aggregation. This follows the same pattern used for HAVING clauses via replaceAggregatesByRefs. Changes: - select.ts: defer expressions containing nested aggregates to groupBy - group-by.ts: extract, register, and evaluate wrapped aggregates in both single-group and multi-group paths - index.ts: detect nested aggregates for implicit single-group aggregation Fixes: #720 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kevin-dp
changed the title
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
samwillis
reviewed
Feb 23, 2026
Collaborator
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
I think __agg_{n} is exposed on the projected results rows? unless I am missing something.
None of the new tests check for the exact data on the projected rows, and so don't validate this, and as this is new functionality the __agg_ would not appear on any rows on old tests.
Three options - not sure whats best:
- do nothing, and accept that the aggregates are there (but not on the types)
- have a cleanup pass to remove them
- use a weakMap to store them with the result row as the key
Comment on lines
274
to
283
| // Add synthetic aggregate values so wrapped expressions can reference them | ||
| for (const key of Object.keys(aggregatedRow)) { | ||
| if (key.startsWith(`__agg_`)) { | ||
| finalResults[key] = aggregatedRow[key] | ||
| } | ||
| } | ||
| // Second pass: evaluate wrapped-aggregate expressions | ||
| for (const [alias, evaluator] of Object.entries(wrappedAggExprs)) { | ||
| finalResults[alias] = evaluator({ $selected: finalResults }) | ||
| } |
Collaborator
There was a problem hiding this comment.
Nit: This is exactly the same code as on 134
Contributor
Author
There was a problem hiding this comment.
Fixed — extracted into a shared evaluateWrappedAggregates helper.
Remove temporary __agg_N keys from finalResults after wrapped-aggregate expressions have been evaluated. Without this cleanup, internal synthetic aliases leak onto user-visible projected result rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deduplicate the synthetic-value copy, evaluate, and cleanup logic that was repeated in both the single-group and multi-group paths of processGroupBy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use toEqual to verify that projected rows contain only the expected keys and no leaked internal state like __agg_N synthetic aliases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@samwillis Good catch on all three points. Fixed in the latest commits:
|
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
2 participants
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.
Summary
Fixes #720.
Aggregates wrapped inside other expressions (e.g.
coalesce(count(...), 0)) threwQueryCompilationError: Unknown expression type: aggbecause the compiler only recognized top-level aggregates in SELECT, not aggregates nested inside function expressions.Approach
HAVING clauses already solve a similar problem —
replaceAggregatesByRefswalks the HAVING expression tree, matches aggregates against SELECT entries, and replaces them with refs to computed values. But for SELECT, the aggregates are buried inside other expressions rather than existing as top-level entries, so there's nothing to match against.The fix adds an extract-register-replace pass: before groupBy processing, nested aggregates are extracted from wrapper expressions and registered under synthetic aliases (e.g.
__agg_0). The aggregates in the original expression are replaced withPropRefreferences to these aliases. After groupBy computes the aggregate values, a second pass evaluates the wrapper expressions against the populated results.Changes
select.ts: Defer expressions containing nested aggregates toprocessGroupBy(same as plain aggregates)group-by.ts: AddcontainsAggregate/extractAndReplaceAggregateshelpers; update both single-group and multi-group paths to extract, register, and evaluate wrapped aggregatesindex.ts: Detect nested aggregates for implicit single-group aggregationReproduction tests
@tanstack/db(packages/db/tests/query/group-by.test.ts):coalesce(count(...)),coalesce(sum(...)),add(count(...), count(...)), mixed plain + wrapped, subquery join source@tanstack/react-db(packages/react-db/tests/useLiveQuery.test.tsx):coalesce(count(...)),coalesce(sum(...)), subquery join source — confirming the bug also affectsuseLiveQueryTest plan
Unknown expression type: agg)tsc --noEmit)🤖 Generated with Claude Code