Fix column mapping in GroupBy and aggregation queries to ensure corre… - #3750
Conversation
…ct backing column references
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes SQL generation for GraphQL groupBy + aggregation queries when entity fields are mapped/aliased in runtime config, ensuring the generated SQL references backing (database) column names while projecting results under exposed (mapped) names. It also adds a regression test to prevent reintroducing the mapped-column failure scenario.
Changes:
- Update
SqlQueryStructuregroup-by column handling to use backing column names in SQL while keeping exposed names as result labels. - Add a new MsSql GraphQL regression test validating group-by + aggregation behavior with mapped columns.
- Add a missing test import needed for accessing GraphQL query builder constants.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Service.Tests/SqlTests/GraphQLQueryTests/MsSqlGraphQLQueryTests.cs | Adds a regression test for group-by aggregations on mapped columns (and required import). |
| src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs | Fixes column/label handling for group-by fields and selections to generate correct SQL for mapped columns. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Service.Tests/SqlTests/GraphQLQueryTests/MsSqlGraphQLQueryTests.cs:1097
- This test compares the full JSON response against a hard-coded array ordering, but the query does not specify orderBy. Without an ORDER BY, SQL Server can return GROUP BY rows in an arbitrary order, making this assertion potentially flaky. Add an explicit orderBy (ASC) to make the group ordering deterministic.
string graphQLQuery = @"
{
gQLmappings {
src/Service.Tests/SqlTests/GraphQLQueryTests/MsSqlGraphQLQueryTests.cs:1062
- These tests assert a specific order of grouped results (1,3,4,5) but the GraphQL query does not specify any ordering. Since the generated SQL for groupBy does not add an ORDER BY by default, result order is not guaranteed and this can make the test flaky across environments. Add an explicit orderBy so the response order is deterministic.
This issue also appears on line 1095 of the same file.
{
gQLmappings {
groupBy(fields: [column1]) {
|
/azp run |
|
Azure Pipelines: Successfully started running 6 pipeline(s). |
…in groupBy queries
|
/azp run |
|
Azure Pipelines: Successfully started running 6 pipeline(s). |
|
Looks like we are missing a regression test for DWSQL. |
aaronburtle
left a comment
There was a problem hiding this comment.
Just a couple comments about testing, but look good!
#3750) ## Why make this change? There is a bug when running aggregation queries, wherein if a mapping backed column is present, the aggregation fails since it references the surfaced column and not the backed column. This pull request improves how group-by queries with mapped (aliased) columns are handled in SQL generation and adds a regression test to ensure correct behavior. The main focus is to ensure that SQL queries reference the backing (database) column names in the SELECT and GROUP BY clauses, while projecting the correct exposed (mapped) names in the results. ## What is this change? * Updated `ProcessGroupByField` and `ProcessGroupByFieldSelections` in `SqlQueryStructure.cs` to ensure that the backing (database) column name is used in the SQL query, while the exposed (mapped) field name is used as the label in the results. This resolves previous issues where the SELECT clause referenced the exposed name instead of the actual database column. [[1]](diffhunk://#diff-4b3df270873038f36ec2f627091120b7aafa271c0e2a2d257380f90602f5a298L907-R908) [[2]](diffhunk://#diff-4b3df270873038f36ec2f627091120b7aafa271c0e2a2d257380f90602f5a298L956-R956) ## How was this tested? - [x] Integration Tests - [X] Unit Tests * Added a new test method `TestSupportForGroupByAggregationWithMappedColumns` in `MsSqlGraphQLQueryTests.cs` to verify that group-by fields and aggregations both resolve to the correct backing columns, and that results are projected under the mapped names. This prevents regressions on mapped column handling in group-by queries. * Added missing import for `Azure.DataApiBuilder.Service.GraphQLBuilder.Queries` in `MsSqlGraphQLQueryTests.cs` to support the new test. --------- Co-authored-by: ARPIT GUPTA <guptaar@microsoft.com> (cherry picked from commit 032eff9)
…ease/2.0 (#3767) Ports the PostgreSQL GraphQL groupby/aggregation and related feature commits from `main` (released in v2.1.0-rc) to `release/2.0` via cherry-pick. ## Commits ported (chronological) | PR | Title | |----|-------| | #3450 | Fix GraphQL aggregation features disabled when runtime.graphql config section is absent | | #3694 | Database policy support for PUT/PATCH operations - PostgreSQL | | #3728 | Add support for DateTime filters in PostgreSQL | | #3750 | Fix column mapping in GroupBy and aggregation queries | | #3741 | Add groupby/aggregation support for PostgreSQL in GraphQL | | #3753 | Enhance test coverage for GraphQL queries by adding orderBy clause | ## Notes - All six cherry-picks applied cleanly. - One manual adjustment in `SqlMutationEngine.cs` (part of #3694 port): the original referenced `effectiveOperationType` (a local introduced by the unrelated refactor #3287, which is not in `release/2.0`). Substituted `context.OperationType`, which is functionally equivalent in that non-upsert branch and matches the `release/2.0` convention. Folded into the #3694 commit. - Solution builds clean (0 warnings, 0 errors). - Integration tests (PostgreSql/MsSql) require live databases and were not run locally. --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Aniruddh25 <3513779+Aniruddh25@users.noreply.github.com> Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: Souvik Ghosh <souvikofficial04@gmail.com> Co-authored-by: Arjun Narendra <arjunnarendra1@gmail.com> Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com> Co-authored-by: Arpit Gupta <106474712+ar-guptaar@users.noreply.github.com> Co-authored-by: ARPIT GUPTA <guptaar@microsoft.com> Co-authored-by: Anusha Kolan <anushakolan10@gmail.com>
Why make this change?
There is a bug when running aggregation queries, wherein if a mapping backed column is present, the aggregation fails since it references the surfaced column and not the backed column.
This pull request improves how group-by queries with mapped (aliased) columns are handled in SQL generation and adds a regression test to ensure correct behavior. The main focus is to ensure that SQL queries reference the backing (database) column names in the SELECT and GROUP BY clauses, while projecting the correct exposed (mapped) names in the results.
What is this change?
ProcessGroupByFieldandProcessGroupByFieldSelectionsinSqlQueryStructure.csto ensure that the backing (database) column name is used in the SQL query, while the exposed (mapped) field name is used as the label in the results. This resolves previous issues where the SELECT clause referenced the exposed name instead of the actual database column. [1] [2]How was this tested?
TestSupportForGroupByAggregationWithMappedColumnsinMsSqlGraphQLQueryTests.csto verify that group-by fields and aggregations both resolve to the correct backing columns, and that results are projected under the mapped names. This prevents regressions on mapped column handling in group-by queries.Azure.DataApiBuilder.Service.GraphQLBuilder.QueriesinMsSqlGraphQLQueryTests.csto support the new test.