Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 89 additions & 3 deletions src/Core/Resolvers/BaseSqlQueryBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,92 @@ protected virtual string Build(AggregationColumn column, bool useAlias = false)
return $"{column.Type.ToString()}({columnName}) {appendAlias}";
Comment thread
ArjunNarendra marked this conversation as resolved.
}

/// <summary>
/// Build the Group By Clause needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with group-by clause</returns>
protected virtual string BuildGroupBy(SqlQueryStructure structure)
{
// Add GROUP BY clause if there are any group by columns
if (structure.GroupByMetadata.Fields.Any())
{
return $" GROUP BY {string.Join(", ", structure.GroupByMetadata.Fields.Values.Select(c => Build(c)))}";
}

return string.Empty;
}

/// <summary>
/// Build the Having clause needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with having clause</returns>
protected virtual string BuildHaving(SqlQueryStructure structure)
{
if (structure.GroupByMetadata.Aggregations.Count > 0)
{
List<Predicate>? havingPredicates = structure.GroupByMetadata.Aggregations
.SelectMany(aggregation => aggregation.HavingPredicates ?? new List<Predicate>())
.ToList();

if (havingPredicates.Any())
{
return $" HAVING {Build(havingPredicates)}";
}
}

return string.Empty;
}

/// <summary>
/// Build the aggregation columns needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with aggregation columns</returns>
protected virtual string BuildAggregationColumns(SqlQueryStructure structure)
{
string aggregations = string.Empty;
if (structure.GroupByMetadata.Aggregations.Count > 0)
{
if (structure.Columns.Any())
{
aggregations = $",{BuildAggregationColumns(structure.GroupByMetadata)}";
}
else
{
aggregations = $"{BuildAggregationColumns(structure.GroupByMetadata)}";
}
}

return aggregations;
}

/// <summary>
/// Build the aggregation columns needed to append to the main query
/// </summary>
/// <param name="metadata">GroupByMetadata</param>
/// <returns>SQL query with aggregation columns</returns>
protected virtual string BuildAggregationColumns(GroupByMetadata metadata)
{
return string.Join(", ", metadata.Aggregations.Select(aggregation => Build(aggregation.Column, useAlias: true)));
}

/// <summary>
/// Build the Order By clause needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with order-by clause</returns>
protected virtual string BuildOrderBy(SqlQueryStructure structure)
{
if (structure.OrderByColumns.Any())
{
return $" ORDER BY {Build(structure.OrderByColumns)}";
}

return string.Empty;
}

/// <summary>
/// Build orderby column as
/// {SourceAlias}.{ColumnName} {direction}
Expand Down Expand Up @@ -447,7 +533,7 @@ public virtual string BuildForeignKeyInfoQuery(int numberOfParameters)
// constraint columns - one inner join for the columns from the 'Referencing table'
// and the other join for the columns from the 'Referenced Table'.
string foreignKeyQuery = $@"
SELECT
SELECT
ReferentialConstraints.CONSTRAINT_NAME {QuoteIdentifier(nameof(ForeignKeyDefinition))},
ReferencingColumnUsage.TABLE_SCHEMA
{QuoteIdentifier($"Referencing{nameof(DatabaseObject.SchemaName)}")},
Expand All @@ -457,9 +543,9 @@ public virtual string BuildForeignKeyInfoQuery(int numberOfParameters)
{QuoteIdentifier($"Referenced{nameof(DatabaseObject.SchemaName)}")},
ReferencedColumnUsage.TABLE_NAME {QuoteIdentifier($"Referenced{nameof(SourceDefinition)}")},
ReferencedColumnUsage.COLUMN_NAME {QuoteIdentifier(nameof(ForeignKeyDefinition.ReferencedColumns))}
FROM
FROM
INFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS ReferentialConstraints
INNER JOIN
INNER JOIN
INFORMATION_SCHEMA.KEY_COLUMN_USAGE ReferencingColumnUsage
ON ReferentialConstraints.CONSTRAINT_CATALOG = ReferencingColumnUsage.CONSTRAINT_CATALOG
AND ReferentialConstraints.CONSTRAINT_SCHEMA = ReferencingColumnUsage.CONSTRAINT_SCHEMA
Expand Down
86 changes: 0 additions & 86 deletions src/Core/Resolvers/BaseTSqlQueryBuilder.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Azure.DataApiBuilder.Config.ObjectModel;
using Azure.DataApiBuilder.Core.Models;

namespace Azure.DataApiBuilder.Core.Resolvers
{
Expand Down Expand Up @@ -43,90 +42,5 @@ protected virtual string BuildPredicates(SqlQueryStructure structure)
Build(structure.PaginationMetadata.PaginationPredicate));
}

/// <summary>
/// Build the Group By Clause needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with group-by clause</returns>
protected virtual string BuildGroupBy(SqlQueryStructure structure)
{
// Add GROUP BY clause if there are any group by columns
if (structure.GroupByMetadata.Fields.Any())
{
return $" GROUP BY {string.Join(", ", structure.GroupByMetadata.Fields.Values.Select(c => Build(c)))}";
}

return string.Empty;
}

/// <summary>
/// Build the Having clause needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with having clause</returns>
protected virtual string BuildHaving(SqlQueryStructure structure)
{
if (structure.GroupByMetadata.Aggregations.Count > 0)
{
List<Predicate>? havingPredicates = structure.GroupByMetadata.Aggregations
.SelectMany(aggregation => aggregation.HavingPredicates ?? new List<Predicate>())
.ToList();

if (havingPredicates.Any())
{
return $" HAVING {Build(havingPredicates)}";
}
}

return string.Empty;
}

/// <summary>
/// Build the Order By clause needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with order-by clause</returns>
protected virtual string BuildOrderBy(SqlQueryStructure structure)
{
if (structure.OrderByColumns.Any())
{
return $" ORDER BY {Build(structure.OrderByColumns)}";
}

return string.Empty;
}

/// <summary>
/// Build the aggregation columns needed to append to the main query
/// </summary>
/// <param name="structure">Sql query structure to build query on</param>
/// <returns>SQL query with aggregation columns</returns>
protected virtual string BuildAggregationColumns(SqlQueryStructure structure)
{
string aggregations = string.Empty;
if (structure.GroupByMetadata.Aggregations.Count > 0)
{
if (structure.Columns.Any())
{
aggregations = $",{BuildAggregationColumns(structure.GroupByMetadata)}";
}
else
{
aggregations = $"{BuildAggregationColumns(structure.GroupByMetadata)}";
}
}

return aggregations;
}

/// <summary>
/// Build the aggregation columns needed to append to the main query
/// </summary>
/// <param name="metadata">GroupByMetadata</param>
/// <returns>SQL query with aggregation columns</returns>
protected virtual string BuildAggregationColumns(GroupByMetadata metadata)
{
return string.Join(", ", metadata.Aggregations.Select(aggregation => Build(aggregation.Column, useAlias: true)));
}
}
}
8 changes: 6 additions & 2 deletions src/Core/Resolvers/PostgresQueryBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this kind of resonates with one of the test case comment summaryimage.png

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

@ArjunNarendra ArjunNarendra Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix I am planning is that every column in the GROUP BY clause will also be in the ORDER BY clause when building the query in DAB.

+ $" LIMIT {structure.Limit()}";

string subqueryName = QuoteIdentifier($"subq{structure.Counter.Next()}");
Expand Down
25 changes: 23 additions & 2 deletions src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,26 @@ private List<OrderByColumn> PrimaryKeyAsOrderByColumns()
return _primaryKeyAsOrderByColumns;
}

/// <summary>
/// Exposes the groupBy fields of this structure as a list of OrderByColumn,
/// giving groupBy queries a deterministic row order (in the absence of an explicit
/// client-provided orderBy) since Postgres/MSSQL do not guarantee GROUP BY row order otherwise.
/// </summary>
private List<OrderByColumn> GroupByColumnsAsOrderByColumns()
{
List<OrderByColumn> groupByColumnsAsOrderByColumns = new();

foreach (Column column in GroupByMetadata.Fields.Values)
{
groupByColumnsAsOrderByColumns.Add(new OrderByColumn(tableSchema: column.TableSchema,
tableName: column.TableName,
columnName: column.ColumnName,
tableAlias: column.TableAlias));
}

return groupByColumnsAsOrderByColumns;
}

/// <summary>
/// Private constructor that is used for recursive query generation,
/// for each subquery that's necessary to resolve a nested GraphQL
Expand Down Expand Up @@ -511,8 +531,9 @@ private SqlQueryStructure(
}
}

// primary key should only be added to order by for non groupby queries.
OrderByColumns = isGroupByQuery ? [] : PrimaryKeyAsOrderByColumns();
// groupBy queries default to ordering by the grouped columns (since Postgres/MSSQL
// don't guarantee GROUP BY row order otherwise); non-groupBy queries default to the primary key.
OrderByColumns = isGroupByQuery ? GroupByColumnsAsOrderByColumns() : PrimaryKeyAsOrderByColumns();
if (IsListQuery && queryParams.ContainsKey(QueryBuilder.ORDER_BY_FIELD_NAME))
{
object? orderByObject = queryParams[QueryBuilder.ORDER_BY_FIELD_NAME];
Expand Down
1 change: 1 addition & 0 deletions src/Service.GraphQLBuilder/Queries/QueryBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public static class QueryBuilder
{
DatabaseType.MSSQL,
DatabaseType.DWSQL,
DatabaseType.PostgreSQL,
};

/// <summary>
Expand Down
18 changes: 15 additions & 3 deletions src/Service.Tests/DatabaseSchema-PostgreSql.sql
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ DROP TABLE IF EXISTS default_with_function_table;
DROP TABLE IF EXISTS users;
DROP TABLE IF EXISTS user_profiles;
DROP TABLE IF EXISTS dimaccount;
DROP TABLE IF EXISTS date_only_table;
DROP FUNCTION IF EXISTS insertCompositeView;

DROP SCHEMA IF EXISTS foo;
Expand Down Expand Up @@ -361,14 +362,14 @@ INSERT INTO bookmarks (id, bkname)
SELECT
value,
CONCAT('Test Item #' , value)
FROM
FROM
GENERATE_SERIES(1, 10000, 1) as value;

INSERT INTO mappedbookmarks (id, bkname)
SELECT
value,
CONCAT('Test Item #' , value)
FROM
FROM
GENERATE_SERIES(1, 10000, 1) as value;

INSERT INTO GQLmappings(__column1, __column2, column3) VALUES (1, 'Incompatible GraphQL Name', 'Compatible GraphQL Name');
Expand All @@ -378,7 +379,7 @@ INSERT INTO GQLmappings(__column1, __column2, column3) VALUES (5, 'Filtered Reco
INSERT INTO publishers(id, name) VALUES (1234, 'Big Company'), (2345, 'Small Town Publisher'), (2323, 'TBD Publishing One'), (2324, 'TBD Publishing Two Ltd'), (1940, 'Policy Publisher 01'), (1941, 'Policy Publisher 02'), (1156, 'The First Publisher');
INSERT INTO clubs(id, name) VALUES (1111, 'Manchester United'), (1112, 'FC Barcelona'), (1113, 'Real Madrid');
INSERT INTO players(id, name, current_club_id, new_club_id)
VALUES
VALUES
(1, 'Cristiano Ronaldo', 1113, 1111),
(2, 'Leonel Messi', 1112, 1113);
INSERT INTO authors(id, name, birthdate) VALUES (123, 'Jelte', '2001-01-01'), (124, 'Aniruddh', '2002-02-02'), (125, 'Aniruddh', '2001-01-01'), (126, 'Aaron', '2001-01-01');
Expand Down Expand Up @@ -487,3 +488,14 @@ $$ LANGUAGE plpgsql;

CREATE TRIGGER insertCompositeViewTrigger INSTEAD OF INSERT ON books_publishers_view_composite_insertable
FOR EACH ROW EXECUTE PROCEDURE insertCompositeView();

CREATE TABLE date_only_table (
event_date date NOT NULL,
event_time time NOT NULL,
event_timestamp timestamp NOT NULL
);

INSERT INTO date_only_table(event_date, event_time, event_timestamp)
VALUES ('2023-01-01', '08:30:00', '2023-01-01 08:30:00'),
('2023-02-15', '12:45:00', '2023-02-15 12:45:00'),
('2023-03-30', '17:15:00', '2023-03-30 17:15:00');
4 changes: 2 additions & 2 deletions src/Service.Tests/GraphQLBuilder/QueryBuilderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -578,12 +578,12 @@ public void GenerateReturnType_ExcludesGroupByField_WhenAggregationDisabled()
/// Tests that QueryBuilder.Build correctly adds or omits the groupBy field on the
/// connection type based on whether the database type is in
/// <see cref="QueryBuilder.AggregationEnabledDatabaseTypes"/>.
/// MSSQL and DWSQL are supported; other types (e.g. PostgreSQL) are not.
/// MSSQL, DWSQL, and PostgreSQL are supported; other types (e.g. MySQL) are not.
/// </summary>
[DataTestMethod]
[DataRow(DatabaseType.MSSQL, true, DisplayName = "MSSQL: groupBy field present when aggregation enabled")]
[DataRow(DatabaseType.DWSQL, true, DisplayName = "DWSQL: groupBy field present when aggregation enabled")]
[DataRow(DatabaseType.PostgreSQL, false, DisplayName = "PostgreSQL: groupBy field absent (not in AggregationEnabledDatabaseTypes)")]
[DataRow(DatabaseType.PostgreSQL, true, DisplayName = "PostgreSQL: groupBy field present when aggregation enabled")]
[DataRow(DatabaseType.MySQL, false, DisplayName = "MySQL: groupBy field absent (not in AggregationEnabledDatabaseTypes)")]
[TestCategory("Query Builder - Aggregation")]
public void Build_WithAggregationEnabled_GroupByPresenceMatchesDatabaseSupport(
Expand Down
Loading
Loading