Skip to content

Commit

Permalink
sql: disable order by index in aggregate decoration clauses
Browse files Browse the repository at this point in the history 
Before this change, queries like `SELECT percentile_disc ( 0.50 ) WITHIN
GROUP ( ORDER BY PRIMARY KEY tbl ) FROM tbl;` would fail with an
internal error. This is due to optbuilder expecting an order expression,
which order by index does not provide, in the aggregate case in order to
resolve the function.

Since this functionality has never worked and appears to be a rare or
never used feature, this PR disables queries with order by index in this
position at the parsing stage. Issue #109847 has been opened to track
usage.

Epic: None
Fixes: #109069
Informs: #109847

Release note: None.
  • Loading branch information
@rharding6373
rharding6373 committed Aug 31, 2023
1 parent 126d7bb commit bd21e67
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 31 deletions.
25 changes: 10 additions & 15 deletions docs/generated/sql/bnf/sort_clause.bnf
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,11 @@
sort_clause ::=
'ORDER' 'BY' a_expr 'ASC' 'NULLS' 'FIRST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'ASC' 'NULLS' 'LAST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'ASC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'DESC' 'NULLS' 'FIRST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'DESC' 'NULLS' 'LAST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'DESC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'NULLS' 'FIRST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr 'NULLS' 'LAST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' a_expr ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' 'PRIMARY' 'KEY' table_name 'ASC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' 'PRIMARY' 'KEY' table_name 'DESC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' 'PRIMARY' 'KEY' table_name ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' 'INDEX' table_name '@' index_name 'ASC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' 'INDEX' table_name '@' index_name 'DESC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
| 'ORDER' 'BY' 'INDEX' table_name '@' index_name ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) | 'PRIMARY' 'KEY' table_name ( 'ASC' | 'DESC' | ) | 'INDEX' table_name '@' index_name ( 'ASC' | 'DESC' | ) ) ) )*
'ORDER' 'BY' a_expr 'ASC' 'NULLS' 'FIRST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'ASC' 'NULLS' 'LAST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'ASC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'DESC' 'NULLS' 'FIRST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'DESC' 'NULLS' 'LAST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'DESC' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'NULLS' 'FIRST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr 'NULLS' 'LAST' ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' a_expr ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
| 'ORDER' 'BY' sortby_index ( ( ',' ( a_expr ( 'ASC' | 'DESC' | ) ( 'NULLS' 'FIRST' | 'NULLS' 'LAST' | ) ) | ',' sortby_index ) )*
7 changes: 5 additions & 2 deletions docs/generated/sql/bnf/stmt_block.bnf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2553,7 +2553,7 @@ table_ref ::=
| '[' row_source_extension_stmt ']' opt_ordinality opt_alias_clause

sortby_list ::=
( sortby ) ( ( ',' sortby ) )*
( sortby | sortby_index ) ( ( ',' sortby | ',' sortby_index ) )*

first_or_next ::=
'FIRST'
Expand Down Expand Up @@ -3149,7 +3149,9 @@ row_source_extension_stmt ::=

sortby ::=
a_expr opt_asc_desc opt_nulls_order
| 'PRIMARY' 'KEY' table_name opt_asc_desc

sortby_index ::=
'PRIMARY' 'KEY' table_name opt_asc_desc
| 'INDEX' table_name '@' index_name opt_asc_desc

only_signed_fconst ::=
Expand Down Expand Up @@ -4435,6 +4437,7 @@ func_application_name ::=
single_sort_clause ::=
'ORDER' 'BY' sortby
| 'ORDER' 'BY' sortby ',' sortby_list
| 'ORDER' 'BY' sortby_index ',' sortby_list

window_specification ::=
'(' opt_existing_window_name opt_partition_clause opt_sort_clause opt_frame_clause ')'
Expand Down
3 changes: 3 additions & 0 deletions pkg/sql/parser/parse_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,9 @@ func TestUnimplementedSyntax(t *testing.T) {
{`UPSERT INTO foo(a, a.b) VALUES (1,2)`, 27792, ``, ``},

{`SELECT 1 OPERATOR(public.+) 2`, 65017, ``, ``},

{`SELECT percentile_disc ( 0.50 ) WITHIN GROUP ( ORDER BY PRIMARY KEY tbl ) FROM tbl;`, 109847, `order by index`, ``},
{`SELECT percentile_disc ( 0.50 ) WITHIN GROUP ( ORDER BY INDEX_AFTER_ORDER_BY_BEFORE_AT INT . LIKE @ FAMILY );`, 109847, `order by index`, ``},
}
for _, d := range testData {
t.Run(d.sql, func(t *testing.T) {
Expand Down
47 changes: 33 additions & 14 deletions pkg/sql/parser/sql.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ func (u *sqlSymUnion) beginTransaction() *tree.BeginTransaction {
%type <tree.Expr> numeric_only
%type <tree.AliasClause> alias_clause opt_alias_clause func_alias_clause opt_func_alias_clause
%type <bool> opt_ordinality opt_compact
%type <*tree.Order> sortby
%type <*tree.Order> sortby sortby_index
%type <tree.IndexElem> index_elem index_elem_options create_as_param
%type <tree.TableExpr> table_ref numeric_table_ref func_table
%type <tree.Exprs> rowsfrom_list
Expand Down Expand Up @@ -12560,36 +12560,41 @@ single_sort_clause:
{
$$.val = tree.OrderBy([]*tree.Order{$3.order()})
}
| ORDER BY sortby_index
{
return unimplementedWithIssueDetail(sqllex, 109847, "order by index")
}
| ORDER BY sortby ',' sortby_list
{
sqllex.Error("multiple ORDER BY clauses are not supported in this function")
return 1
}
| ORDER BY sortby_index ',' sortby_list
{
sqllex.Error("multiple ORDER BY clauses are not supported in this function")
return 1
}

sortby_list:
sortby
{
$$.val = []*tree.Order{$1.order()}
}
| sortby_index
{
$$.val = []*tree.Order{$1.order()}
}
| sortby_list ',' sortby
{
$$.val = append($1.orders(), $3.order())
}

sortby:
a_expr opt_asc_desc opt_nulls_order
| sortby_list ',' sortby_index
{
/* FORCE DOC */
dir := $2.dir()
nullsOrder := $3.nullsOrder()
$$.val = &tree.Order{
OrderType: tree.OrderByColumn,
Expr: $1.expr(),
Direction: dir,
NullsOrder: nullsOrder,
}
$$.val = append($1.orders(), $3.order())
}
| PRIMARY KEY table_name opt_asc_desc

sortby_index:
PRIMARY KEY table_name opt_asc_desc
{
name := $3.unresolvedObjectName().ToTableName()
$$.val = &tree.Order{OrderType: tree.OrderByIndex, Direction: $4.dir(), Table: name}
Expand All @@ -12605,6 +12610,20 @@ sortby:
}
}

sortby:
a_expr opt_asc_desc opt_nulls_order
{
/* FORCE DOC */
dir := $2.dir()
nullsOrder := $3.nullsOrder()
$$.val = &tree.Order{
OrderType: tree.OrderByColumn,
Expr: $1.expr(),
Direction: dir,
NullsOrder: nullsOrder,
}
}

opt_nulls_order:
NULLS_LA FIRST
{
Expand Down

0 comments on commit bd21e67

Please sign in to comment.