Skip to content

Commit

Permalink
sql: disallow aggregate functions in ORDER BY in DELETE and UPDATE
Browse files Browse the repository at this point in the history
This commit disallows aggregate functions in the context of an
`ORDER BY` clause in a `DELETE` or `UPDATE` statement. An aggregate
function in an `ORDER BY` would require a `GROUP BY` clause to group
non-aggregate columns. A `GROUP BY` is not allowed in `DELETE` or
`UPDATE` statements as it's not obvious how grouping in these statements
would behave. So we simply disallow aggregates in `ORDER BY` instead.

Fixes #107634

Release note (bug fix): A bug has been fixed that caused internal errors
when using an aggregate function in an `ORDER BY` clause of a `DELETE`
or `UPDATE` statement. Aggregate functions are no longer allowed in
these contexts. The bug has been present since at least version 20.2.
  • Loading branch information
mgartner committed Jul 26, 2023
1 parent 9db3a0e commit b3a90bd
Show file tree
Hide file tree
Showing 8 changed files with 52 additions and 7 deletions.
11 changes: 11 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/delete
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,14 @@ false
query I
SELECT a FROM t99630a@idx WHERE a > 0
----

# Regression test for #107634. Do not allow aggregate functions in ORDER BY.
subtest regression_107634

statement ok
CREATE TABLE t107634 (a INT)

statement error pgcode 42803 sum\(\): aggregate functions are not allowed in ORDER BY in DELETE
DELETE FROM t107634 ORDER BY sum(a) LIMIT 1;

subtest end
11 changes: 11 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/update
Original file line number Diff line number Diff line change
Expand Up @@ -661,3 +661,14 @@ SELECT * FROM generated_as_id_t ORDER BY a;
7 4 4
8 5 5
9 6 6

# Regression test for #107634. Do not allow aggregate functions in ORDER BY.
subtest regression_107634

statement ok
CREATE TABLE t107634 (a INT)

statement error pgcode 42803 sum\(\): aggregate functions are not allowed in ORDER BY in UPDATE
UPDATE t107634 SET a = 1 ORDER BY sum(a) LIMIT 1;

subtest end
6 changes: 4 additions & 2 deletions pkg/sql/opt/optbuilder/mutation_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,8 @@ func (mb *mutationBuilder) buildInputForUpdate(
// SELECT + ORDER BY (which may add projected expressions)
projectionsScope := mb.outScope.replace()
projectionsScope.appendColumnsFromScope(mb.outScope)
orderByScope := mb.b.analyzeOrderBy(orderBy, mb.outScope, projectionsScope, tree.RejectGenerators)
orderByScope := mb.b.analyzeOrderBy(orderBy, mb.outScope, projectionsScope,
exprKindOrderByUpdate, tree.RejectGenerators|tree.RejectAggregates)
mb.b.buildOrderBy(mb.outScope, projectionsScope, orderByScope)
mb.b.constructProjectForScope(mb.outScope, projectionsScope)

Expand Down Expand Up @@ -448,7 +449,8 @@ func (mb *mutationBuilder) buildInputForDelete(
// SELECT + ORDER BY (which may add projected expressions)
projectionsScope := mb.outScope.replace()
projectionsScope.appendColumnsFromScope(mb.outScope)
orderByScope := mb.b.analyzeOrderBy(orderBy, mb.outScope, projectionsScope, tree.RejectGenerators)
orderByScope := mb.b.analyzeOrderBy(orderBy, mb.outScope, projectionsScope,
exprKindOrderByDelete, tree.RejectGenerators|tree.RejectAggregates)
mb.b.buildOrderBy(mb.outScope, projectionsScope, orderByScope)
mb.b.constructProjectForScope(mb.outScope, projectionsScope)

Expand Down
9 changes: 6 additions & 3 deletions pkg/sql/opt/optbuilder/orderby.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ import (
// analyzeOrderBy analyzes an Ordering physical property from the ORDER BY
// clause and adds the resulting typed expressions to orderByScope.
func (b *Builder) analyzeOrderBy(
orderBy tree.OrderBy, inScope, projectionsScope *scope, rejectFlags tree.SemaRejectFlags,
orderBy tree.OrderBy,
inScope, projectionsScope *scope,
kind exprKind,
rejectFlags tree.SemaRejectFlags,
) (orderByScope *scope) {
if orderBy == nil {
return nil
Expand All @@ -41,8 +44,8 @@ func (b *Builder) analyzeOrderBy(
// semaCtx in case we are recursively called within a subquery
// context.
defer b.semaCtx.Properties.Restore(b.semaCtx.Properties)
b.semaCtx.Properties.Require(exprKindOrderBy.String(), rejectFlags)
inScope.context = exprKindOrderBy
b.semaCtx.Properties.Require(kind.String(), rejectFlags)
inScope.context = kind

for i := range orderBy {
b.analyzeOrderByArg(orderBy[i], inScope, projectionsScope, orderByScope)
Expand Down
4 changes: 4 additions & 0 deletions pkg/sql/opt/optbuilder/scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ const (
exprKindOffset
exprKindOn
exprKindOrderBy
exprKindOrderByDelete
exprKindOrderByUpdate
exprKindReturning
exprKindSelect
exprKindStoreID
Expand All @@ -153,6 +155,8 @@ var exprKindName = [...]string{
exprKindOffset: "OFFSET",
exprKindOn: "ON",
exprKindOrderBy: "ORDER BY",
exprKindOrderByDelete: "ORDER BY in DELETE",
exprKindOrderByUpdate: "ORDER BY in UPDATE",
exprKindReturning: "RETURNING",
exprKindSelect: "SELECT",
exprKindStoreID: "RELOCATE STORE ID",
Expand Down
6 changes: 4 additions & 2 deletions pkg/sql/opt/optbuilder/select.go
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,8 @@ func (b *Builder) buildSelectStmtWithoutParens(
col := projectionsScope.addColumn(scopeColName(""), expr)
b.buildScalar(expr, outScope, projectionsScope, col, nil)
}
orderByScope := b.analyzeOrderBy(orderBy, outScope, projectionsScope, tree.RejectGenerators|tree.RejectAggregates|tree.RejectWindowApplications)
orderByScope := b.analyzeOrderBy(orderBy, outScope, projectionsScope, exprKindOrderBy,
tree.RejectGenerators|tree.RejectAggregates|tree.RejectWindowApplications)
b.buildOrderBy(outScope, projectionsScope, orderByScope)
b.constructProjectForScope(outScope, projectionsScope)
outScope = projectionsScope
Expand Down Expand Up @@ -1151,7 +1152,8 @@ func (b *Builder) buildSelectClause(
// Any aggregates in the HAVING, ORDER BY and DISTINCT ON clauses (if they
// exist) will be added here.
havingExpr := b.analyzeHaving(sel.Having, fromScope)
orderByScope := b.analyzeOrderBy(orderBy, fromScope, projectionsScope, tree.RejectGenerators)
orderByScope := b.analyzeOrderBy(orderBy, fromScope, projectionsScope,
exprKindOrderBy, tree.RejectGenerators)
distinctOnScope := b.analyzeDistinctOnArgs(sel.DistinctOn, fromScope, projectionsScope)

var having opt.ScalarExpr
Expand Down
6 changes: 6 additions & 0 deletions pkg/sql/opt/optbuilder/testdata/delete
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,12 @@ DELETE FROM abcde WHERE b=1 ORDER BY c
----
error (42601): DELETE statement requires LIMIT when ORDER BY is used

# Aggregate functions are not allowed in ORDER BY.
build
DELETE FROM abcde WHERE b=1 ORDER BY sum(c) LIMIT 1
----
error (42803): sum(): aggregate functions are not allowed in ORDER BY in DELETE

# ------------------------------------------------------------------------------
# Test RETURNING.
# ------------------------------------------------------------------------------
Expand Down
6 changes: 6 additions & 0 deletions pkg/sql/opt/optbuilder/testdata/update
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,12 @@ UPDATE abcde SET b=1 ORDER BY c
----
error (42601): UPDATE statement requires LIMIT when ORDER BY is used

# Aggregate functions are not allowed in ORDER BY.
build
UPDATE abcde SET b=1 ORDER BY sum(c) LIMIT 1
----
error (42803): sum(): aggregate functions are not allowed in ORDER BY in UPDATE

# ------------------------------------------------------------------------------
# Test RETURNING.
# ------------------------------------------------------------------------------
Expand Down

0 comments on commit b3a90bd

Please sign in to comment.