Merge cockroachdb#109861 cockroachdb#110016 cockroachdb#110029

109861: sql: disable order by index in aggregate decoration clauses r=rharding6373 a=rharding6373 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 cockroachdb#109847 has been opened to track usage. Epic: None Fixes: cockroachdb#109069 Informs: cockroachdb#109847 Release note: None. 110016: github: split up spanconfig(ccl) owners r=arulajmani,rafiss a=kvoli 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 110029: roachtest: fix calls to schemachange workload in acceptance/version-upgrade r=fqazi a=renatolabs 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 Co-authored-by: rharding6373 <[email protected]> Co-authored-by: Austen McClernon <[email protected]> Co-authored-by: Renato Costa <[email protected]>
jbowens · Sep 5, 2023 · 71eaeec · 71eaeec
4 parents 9b91e98 + 0deff4c + 9b8bbdd + 2f719e4
commit 71eaeec
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 33 deletions.
diff --git 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

diff --git 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
@@ -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/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())

diff --git 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
@@ -1543,7 +1543,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
@@ -12734,36 +12734,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}
@@ -12779,6 +12784,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
   {