-
Notifications
You must be signed in to change notification settings - Fork 356
Add groupby/aggregation support for PostgreSQL in GraphQL #3741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f1f1367
6a4e137
79d34fc
a322bc1
e72d3d3
ae989b6
707b5c6
176d795
1e2ac35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,10 +41,14 @@ public string Build(SqlQueryStructure structure) | |
| Build(structure.Predicates), | ||
| Build(structure.PaginationMetadata.PaginationPredicate)); | ||
|
|
||
| string query = $"SELECT {MakeSelectColumns(structure)}" | ||
| string aggregations = BuildAggregationColumns(structure); | ||
|
|
||
| string query = $"SELECT {MakeSelectColumns(structure)}{aggregations}" | ||
| + $" FROM {fromSql}" | ||
| + $" WHERE {predicates}" | ||
| + $" ORDER BY {Build(structure.OrderByColumns)}" | ||
| + BuildGroupBy(structure) | ||
| + BuildHaving(structure) | ||
| + BuildOrderBy(structure) | ||
|
Comment on lines
41
to
+51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the Build(SqlQueryStructure) method now appends BuildOrderBy(structure), which only emits ORDER BY when structure.OrderByColumns.Any(). For a groupBy query the grouping keys are not guaranteed to be in OrderByColumns, so DAB emits GROUP BY with no ORDER BY. Postgres does not guarantee row order for GROUP BY without ORDER BY, so the client-visible ordering of aggregation results becomes nondeterministic (and pagination over grouped results would be unstable)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are right. This is an issue with the implementation (not just the tests), and the clearest negative effect it may have is that pagination over grouped results becomes unstable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix I am planning is that every column in the |
||
| + $" LIMIT {structure.Limit()}"; | ||
|
|
||
| string subqueryName = QuoteIdentifier($"subq{structure.Counter.Next()}"); | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.