Add aggregate strategies support (lateral & grouped) - #1
Draft
wtsnz wants to merge 12 commits into
Draft
Conversation
wtsnz
marked this pull request as draft
May 21, 2026 21:42
7 tasks
wtsnz
force-pushed
the
feat/aggregate-strategies-split
branch
2 times, most recently
from
July 22, 2026 01:17
b19bc87 to
5eedd0a
Compare
Problem: The new grouped query path implemented `first` as a raw ordered `LIMIT 1` query. It could return nil when `include_nil?` was false, skip the aggregate default for empty results, and pass Ash null-ordering atoms directly to Ecto. Change: Filter nil values unless they are requested, translate every supported Ash sort order to the matching Ecto order, and apply the aggregate default when the selected result is nil. Provenance: This defect was introduced by the new grouped query aggregate implementation. The fix makes grouped `first` behavior match Ash aggregate semantics.
Problem: Fieldless grouped counts with `uniq?: true` hard-coded an `:id` field. Ash resources can use a differently named key, a composite key, or no primary key, so the generated query could reference a field that does not exist. Change: Read the resource primary-key metadata and count the single key when one exists. Return a clear unsupported error for resources with no primary key or a composite primary key. Provenance: The `:id` assumption was introduced by the new grouped query aggregate implementation and was not copied from the previous AshSQL path.
Problem: Three moved aggregate paths produced avoidable compiler feedback: the grouped support predicate tested `related?` indirectly, the lateral module required `Ash.Query` without using its macros, and a list-default clause required a nil default even though Ash normally constructs list aggregates with a default of `[]`. Change: Match related aggregates directly, remove the unused requirement, and remove the nil-only list-default path. These changes make the accepted shapes explicit and remove code that normal Ash aggregate construction cannot reach. Provenance: These are non-functional hygiene changes in moved code. The predicate and list-default path came from the earlier SQLite implementation, while the unused requirement came from upstream lateral code.
Problem: Grouped list and first aggregates built parameterized Ecto types with an empty constraint list. Parameterized Ash types can depend on aggregate constraints, so dropping them can select or decode values with the wrong type configuration. Change: Pass both `aggregate.type` and `aggregate.constraints` through `AshSql.Expr.parameterized_type/4` for window and loaded aggregate values. Provenance: The empty constraint list originated in the earlier SQLite implementation. The extraction moved and generalized that code but did not introduce the defect.
Problem: The grouped query path wrapped aggregate inputs only for `distinct` or `limit`. An ordered query with only an offset could therefore aggregate the wrong input set, and SQLite cannot express a standalone `OFFSET` without a limit. Change: Wrap offset-only grouped inputs, preserve ordering when a limit or offset depends on it, and add SQLite's `LIMIT -1` sentinel when no explicit limit exists. Provenance: The missing offset condition was inherited from existing AshSQL wrapping logic and copied into the new grouped path. Fixing the grouped copy is required for the grouped aggregate feature; the separate lateral bug is tracked in a follow-up branch.
wtsnz
force-pushed
the
feat/aggregate-strategies-split
branch
from
July 22, 2026 02:58
5eedd0a to
f96581e
Compare
Problem: The grouped strategy discarded relationship limits and offsets, treated aggregate fields as database-column atoms, and let root first and exists bypass the limited aggregate input. The extraction also added a redundant context wrapper, duplicate binding setup, manual relationship traversal, and SQLite-specific list SQL in the shared strategy. Change: Rank related rows per parent before aggregation, resolve fields and sorts through AshSQL expression helpers, and route all root aggregates through the common input wrapper. Reuse AshSQL relationship traversal, remove the unused context and duplicate initialization, and delegate grouped list expressions to the adapter. Why: Ash values can be calculations or aggregates, and relationship bounds must apply before rows are folded. Reusing the existing AshSQL paths keeps grouped behavior aligned with lateral behavior and avoids silent wrong results. Provenance: These correctness defects and cleanup issues were introduced by the grouped aggregate extraction. Validation: The four focused SQLite regressions pass. Full suites pass in AshSQL (1 test), AshSQLite (210 tests), and AshPostgres (851 tests, 21 excluded).
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
1 participant
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.
Contributor checklist
Leave anything that you believe does not apply unchecked.
Ultimately this came out of my last pr to ash_sqlite that adds aggregate support. After talking to Zach he brought up the idea that the different aggregate strategies should probably live in ash_sql instead of specifically inside of ash_sqlite. SQLite doesn't support aggregates based on lateral queries so instead we must implement a different strategy based on windows.
Two possible approaches going forward 1 OR 2:
a. Add support for Aggregates ash-project/ash_sqlite#213
a. This PR that adds the multiple aggregate strategies to ash_sql
b. A PR that adds the consumption of the above into ash_sqlite Add Aggregate Support using ash_sql implementation ash_sqlite#5
c. A tiny pr to ash_postgres that explicitly states the aggregate_strategy (wtsnz/ash_postgres@c02e86f)
The direction depends on what ash community want - I'll defer to them after asking