From 0deff4cd62676fba7f4351b6654a2ba5faa13ecb Mon Sep 17 00:00:00 2001 From: rharding6373 Date: Thu, 31 Aug 2023 16:44:22 -0700 Subject: [PATCH 1/3] sql: disable order by index in aggregate decoration clauses 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. --- docs/generated/sql/bnf/sort_clause.bnf | 25 ++++++-------- docs/generated/sql/bnf/stmt_block.bnf | 7 ++-- pkg/sql/parser/parse_test.go | 3 ++ pkg/sql/parser/sql.y | 47 ++++++++++++++++++-------- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/docs/generated/sql/bnf/sort_clause.bnf b/docs/generated/sql/bnf/sort_clause.bnf index e69efe87e0ed..4c0884e909bb 100644 --- a/docs/generated/sql/bnf/sort_clause.bnf +++ b/docs/generated/sql/bnf/sort_clause.bnf @@ -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 ) )* diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 1c82c7319ad5..14cb11e6f821 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -2576,7 +2576,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' @@ -3181,7 +3181,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 ::= @@ -4470,6 +4472,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 ')' diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index ee8e4402d0ae..022781f7cab2 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -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) { diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 1e8217ef9c08..b3fad0830c82 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -1536,7 +1536,7 @@ func (u *sqlSymUnion) showCreateFormatOption() tree.ShowCreateFormatOption { %type numeric_only %type alias_clause opt_alias_clause func_alias_clause opt_func_alias_clause %type opt_ordinality opt_compact -%type <*tree.Order> sortby +%type <*tree.Order> sortby sortby_index %type index_elem index_elem_options create_as_param %type table_ref numeric_table_ref func_table %type rowsfrom_list @@ -12732,36 +12732,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} @@ -12777,6 +12782,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 { From 2f719e4c342fc6e07635b2f494f70611d357b735 Mon Sep 17 00:00:00 2001 From: Renato Costa Date: Tue, 5 Sep 2023 10:47:35 -0400 Subject: [PATCH 2/3] roachtest: fix calls to schemachange workload in acceptance/version-upgrade The `schemachange` workload would always be called in that test; however, we are currently only staging the `workload` binary build on the SHA being tested; this is insufficient now that we test multiple upgrades in `mixedversion` tests. We fix the immediate issue by not invoking the workload when we are not performing an upgrade or downgrade involving the current cockroach binary. In the future, it would be nice to stage multiple workload binaries and use the appropriate one in the mixed-version hook. Epic: none Release note: None --- pkg/cmd/roachtest/tests/versionupgrade.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/cmd/roachtest/tests/versionupgrade.go b/pkg/cmd/roachtest/tests/versionupgrade.go index 2336653daa75..61cb9e4fedf9 100644 --- a/pkg/cmd/roachtest/tests/versionupgrade.go +++ b/pkg/cmd/roachtest/tests/versionupgrade.go @@ -136,6 +136,19 @@ func runVersionUpgrade(ctx context.Context, t test.Test, c cluster.Cluster) { mvt.InMixedVersion( "test schema change step", func(ctx context.Context, l *logger.Logger, rng *rand.Rand, h *mixedversion.Helper) error { + tc := h.Context() + // We currently only stage the `workload` binary built off the + // SHA being tested; therefore, we skip testing the schemachange + // workload if this is not an upgrade or downgrade involving the + // current cockroach binary. + // TODO(renato): stage different workload binaries for the + // releases being used in the test and use the appropriate + // binary in this step. + if tc.FromVersion != clusterupgrade.MainVersion && tc.ToVersion != clusterupgrade.MainVersion { + l.Printf("skipping this step -- only supported when current version is involved") + return nil + } + l.Printf("running schema workload step") runCmd := roachtestutil.NewCommand("./workload run schemachange").Flag("verbose", 1).Flag("max-ops", 10).Flag("concurrency", 2).Arg("{pgurl:1-%d}", len(c.All())) randomNode := h.RandomNode(rng, c.All()) From 9b8bbdd377c72371f97e3dfa3f6bd4c7f8e34bb2 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Tue, 5 Sep 2023 13:24:09 +0000 Subject: [PATCH 3/3] github: split up spanconfig(ccl) owners Prior to this commit, `kv-prs` was the owner of `pkg/spanconfig` and `pkg/ccl/spanconfigccl`. Package ownership is updated to reflect the ownership split between `kv-prs` and `sql-foundations`. See CODEOWNERS for the split. Epic: none Release note: None --- .github/CODEOWNERS | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 4c3cb15985eb..81d4bc052333 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -329,7 +329,13 @@ /pkg/kv/kvserver/txnwait/ @cockroachdb/kv-prs /pkg/kv/kvserver/uncertainty/ @cockroachdb/kv-prs -/pkg/ccl/spanconfigccl/ @cockroachdb/kv-prs +/pkg/ccl/spanconfigccl/ @cockroachdb/kv-prs @cockroachdb/sql-foundations +/pkg/ccl/spanconfigccl/spanconfigkvaccessorccl/ @cockroachdb/kv-prs +/pkg/ccl/spanconfigccl/spanconfiglimiterccl/ @cockroachdb/sql-foundations +/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/ @cockroachdb/sql-foundations +/pkg/ccl/spanconfigccl/spanconfigsplitterccl/ @cockroachdb/sql-foundations +/pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/ @cockroachdb/sql-foundations +/pkg/ccl/spanconfigccl/spanconfigsqlwatcherccl/ @cockroachdb/sql-foundations /pkg/ccl/storageccl/engineccl @cockroachdb/storage /pkg/storage/ @cockroachdb/storage @@ -509,7 +515,21 @@ /pkg/security/ @cockroachdb/prodsec @cockroachdb/server-prs /pkg/security/clientsecopts/ @cockroachdb/sql-foundations @cockroachdb/prodsec #!/pkg/settings/ @cockroachdb/unowned -/pkg/spanconfig/ @cockroachdb/kv-prs +/pkg/spanconfig/ @cockroachdb/kv-prs @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigbounds/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigjob/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigkvaccessor/ @cockroachdb/kv-prs +/pkg/spanconfig/spanconfigkvsubscriber/ @cockroachdb/kv-prs +/pkg/spanconfig/spanconfiglimiter/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigmanager/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigptsreader/ @cockroachdb/kv-prs +/pkg/spanconfig/spanconfigreconciler/ @cockroachdb/kv-prs +/pkg/spanconfig/spanconfigreporter/ @cockroachdb/kv-prs +/pkg/spanconfig/spanconfigsplitter/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigsqltranslator/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigsqlwatcher/ @cockroachdb/sql-foundations +/pkg/spanconfig/spanconfigstore/ @cockroachdb/kv-prs +/pkg/spanconfig/spanconfigtestutils/ @cockroachdb/kv-prs @cockroachdb/sql-foundations /pkg/repstream/ @cockroachdb/disaster-recovery #!/pkg/testutils/ @cockroachdb/test-eng-noreview /pkg/testutils/reduce/ @cockroachdb/sql-queries-prs