Add orderBy, first and last parameters to groupedAggregates #80
Conversation
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
benjie
requested changes
Nov 5, 2025
Member
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
This is really thorough, I like it! I wonder how we avoid type explosion with this - perhaps we should have an opt-in to the feature? I don't think index behaviours can save us here. This plugin already adds a lot of types to the schema 😅
For now let's drop the last argument.
Author
I think definitely a good idea and safe default. I'm making it happen through behaviours. |
Enhances the groupedAggregates:orderBy behavior system to support per-aggregate scoping, providing more granular control over which aggregates can be used for ordering grouped results. Changes: - Add per-aggregate behavior declarations (sum:, min:, max:, average:, distinctCount:, stddevSample:, stddevPopulation:, varianceSample:, variancePopulation:) for groupedAggregates:orderBy - Simplify entity behavior registration by removing redundant array wrapper - Remove manual hierarchical behavior checking logic in favor of PostGraphile's built-in behavior precedence system - Inline resource validation check for better encapsulation Documentation: - Document groupedAggregates:orderBy behavior with explanation of why it's disabled by default (18 enum values per attribute = schema bloat) - Add examples showing three levels of granularity: table-level, column-level, and per-aggregate scoping - Follow existing documentation style for consistency This allows users to selectively enable only the specific aggregates they need (e.g., +sum:attribute:aggregate:groupedAggregates:orderBy) rather than all 9 aggregates, reducing schema bloat while maintaining flexibility.
The 'last' argument is the opposite of 'first' and adds unnecessary complexity for grouped aggregates. Users can achieve the same result by reversing the orderBy direction and using 'first'. Changes: - Remove 'last' argument from groupedAggregates field definition - Update README to only document 'first' argument - Update test query to remove usage of 'last' argument - Regenerate snapshots with updated schema This simplifies the API while maintaining all necessary functionality.
When no explicit orderBy is specified, groupedAggregates now returns results ordered by the GROUP BY columns in ascending order. This ensures deterministic, predictable results instead of database-dependent ordering. Changes: - Modify groupBy enum value apply functions to add default ORDER BY - Apply to both direct attributes and derivative group-by specs - Update README to document default ordering behavior - Regenerate test snapshots with new ORDER BY clauses in SQL This provides a better developer experience with consistent, sorted results by default while still allowing custom ordering via orderBy argument when needed.
Adds comprehensive tests validating that per-aggregate behavior scoping works correctly for groupedAggregates orderBy functionality. Tests verify: - Enabling only SUM aggregate for orderBy (excludes MIN, MAX, AVERAGE, etc.) - Enabling multiple specific aggregates (SUM + AVERAGE) - Proper behavior precedence (more specific beats less specific) Example usage shown in tests: -attribute:aggregate:groupedAggregates:orderBy +sum:attribute:aggregate:groupedAggregates:orderBy This allows users to selectively enable only the aggregates they need, reducing schema bloat from 18 enum values per attribute down to 2 per enabled aggregate type.
Corrects the per-aggregate behavior example to show that the generic behavior must be disabled first before enabling specific aggregates. Before (incorrect): +sum:... +average:... After (correct): -attribute:aggregate:groupedAggregates:orderBy +sum:... +average:... Without the initial `-attribute:...`, all aggregates would be enabled due to behavior precedence, defeating the purpose of per-aggregate scoping. Adds explanatory note to prevent confusion.
Author
|
Implemented all suggestions, with also some tests for the per-aggregate behaviours, let me know if they make sense and it's not too much testing. |
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.
More context on #75. Closes #75
Basically adds
orderBy,firstandlastparameters togroupedAggregates.orderByaccepts any possible field aggregations. Note that I still don't have a very good understanding of the project, this PR is party AI generated and partly from copy pasting various concepts around, would appreciate deep review from someone who understands the code better to see if there can better ways.Performance impact
Slight performance impact in startup time due to added(18 entries per numerical fields in AggregatesOrderBy enum)
Security impact
Shouldn't be.
Checklist
yarn lint:fixpasses. - I'm working on estlint to run properlyyarn testpasses.RELEASE_NOTES.mdfile (if one exists). - File doesn't exist