Merge #45986 #46009 #46032 #46036

45986: roachtest: adjust tpchvec r=yuzefovich a=yuzefovich Release justification: non-production code change. This commit unskips query 9 and removes some code that was put in place to handle missing disk spilling in the vectorized engine. Release note: None 46009: sql: add telemetry for window functions r=yuzefovich a=yuzefovich **sql: remove some unused code** Release justification: non-production code changes. This code is no longer needed since the optimizer took over planning of the window functions. Release note: None **sql: add telemetry for window functions** Release justification: telemetry change. Fixes: #45598. Release note: None 46032: vendor, rocksdb: Bump rocksdb and pebble refs r=itsbilal a=itsbilal Bumps rocksdb ref to pick up cockroachdb/rocksdb#75 Bumps pebble ref to pick up SeqNum fix and standardization of initial and last SeqNum handling with RocksDB. New pebble revision is d2ecbc248decea8d177e2d2777d8ef458267eddc Release note: None 46036: sqltelemetry: adjust IAM telemetry methods to have Inc/Counter r=RichardJCai a=otan Trying to standardise the sqltelemetry package such that every name ends with `Counter`, so this is a `sed` replace to do just that. Also noticed that `IAMAlter` has a different name, so fixing that form too. Release justification: Refactoring of names only. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Bilal Akhtar <[email protected]> Co-authored-by: Oliver Tan <[email protected]>
cockroachdb · Mar 12, 2020 · 0373c81 · 0373c81
5 parents a30aedd + 10b797f + bbb3afb + 73c02a0 + 0495ab2
commit 0373c81
Show file tree

Hide file tree

Showing 16 changed files with 87 additions and 233 deletions.
diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/c-deps/rocksdb b/c-deps/rocksdb
diff --git a/pkg/ccl/roleccl/role.go b/pkg/ccl/roleccl/role.go
@@ -96,7 +96,7 @@ func grantRolePlanHook(
 		return nil, nil, nil, false, nil
 	}
 
-	sqltelemetry.IncIAMGrant(grant.AdminOption)
+	sqltelemetry.IncIAMGrantCounter(grant.AdminOption)
 
 	fn = func(ctx context.Context, _ []sql.PlanNode, _ chan<- tree.Datums) error {
 		// TODO(dan): Move this span into sql.
@@ -250,7 +250,7 @@ func revokeRolePlanHook(
 		return nil, nil, nil, false, nil
 	}
 
-	sqltelemetry.IncIAMRevoke(revoke.AdminOption)
+	sqltelemetry.IncIAMRevokeCounter(revoke.AdminOption)
 
 	fn = func(ctx context.Context, _ []sql.PlanNode, _ chan<- tree.Datums) error {
 		// TODO(dan): Move this span into sql.

diff --git a/pkg/cmd/roachtest/tpchvec.go b/pkg/cmd/roachtest/tpchvec.go
@@ -21,7 +21,6 @@ import (
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
-	tpchworkload "github.com/cockroachdb/cockroach/pkg/workload/tpch"
 )
 
 func registerTPCHVec(r *testRegistry) {
@@ -49,10 +48,6 @@ func registerTPCHVec(r *testRegistry) {
 		9:  "can cause OOM",
 		19: "can cause OOM",
 	}
-	queriesToSkipByVersionPrefix["v20.1"] = map[int]string{
-		// TODO(yuzefovich): remove this once disk spilling is in place.
-		9: "needs disk spilling",
-	}
 
 	runTPCHVec := func(ctx context.Context, t *test, c *cluster) {
 		TPCHTables := []string{
@@ -522,14 +517,7 @@ RESTORE tpch.* FROM 'gs://cockroach-fixtures/workload/tpch/scalefactor=1/backup'
 				if err != nil {
 					// Note: if you see an error like "exit status 1", it is likely caused
 					// by the erroneous output of the query.
-					t.Status(fmt.Sprintf("\n%s", err))
-					// We expect that with low workmem limit some queries can hit OOM
-					// error, and we don't want to fail the test in such scenarios.
-					// TODO(yuzefovich): remove the condition once disk spilling is in
-					// place.
-					if !strings.Contains(string(workloadOutput), "memory budget exceeded") {
-						t.Fatal(err)
-					}
+					t.Fatal(err)
 				}
 				parseOutput := func(output []byte, timeByQueryNum map[int][]float64) {
 					runtimeRegex := regexp.MustCompile(`.*\[q([\d]+)\] returned \d+ rows after ([\d]+\.[\d]+) seconds.*`)
@@ -551,17 +539,9 @@ RESTORE tpch.* FROM 'gs://cockroach-fixtures/workload/tpch/scalefactor=1/backup'
 					}
 				}
 				parseOutput(workloadOutput, timeByQueryNum[configIdx])
-				// We want to fail the test only if wrong results were returned (we
-				// ignore errors like OOM and unsupported features in order to not
-				// short-circuit the run of this test).
-				if strings.Contains(string(workloadOutput), tpchworkload.TPCHWrongOutputErrorPrefix) {
-					t.Fatal("tpch workload found wrong results")
-				}
 			}
 		}
-		// TODO(yuzefovich): remove the note when disk spilling is in place.
-		t.Status("comparing the runtimes (only median values for each query are compared).\n" +
-			"NOTE: the comparison might not be fair because vec ON doesn't spill to disk")
+		t.Status("comparing the runtimes (only median values for each query are compared)")
 		for queryNum := 1; queryNum <= numTPCHQueries; queryNum++ {
 			if _, skipped := queriesToSkip[queryNum]; skipped {
 				continue
@@ -572,32 +552,36 @@ RESTORE tpch.* FROM 'gs://cockroach-fixtures/workload/tpch/scalefactor=1/backup'
 			}
 			vecOnTimes := timeByQueryNum[vecOnConfig][queryNum]
 			vecOffTimes := timeByQueryNum[vecOffConfig][queryNum]
+			if len(vecOnTimes) != numRunsPerQuery {
+				t.Fatal(fmt.Sprintf("[q%d] unexpectedly wrong number of run times "+
+					"recorded with vec ON config: %v", queryNum, vecOnTimes))
+			}
 			if len(vecOffTimes) != numRunsPerQuery {
 				t.Fatal(fmt.Sprintf("[q%d] unexpectedly wrong number of run times "+
 					"recorded with vec OFF config: %v", queryNum, vecOffTimes))
 			}
-			// It is possible that the query errored out on vec ON config. We want to
-			// compare the run times only if that's not the case.
-			if len(vecOnTimes) > 0 {
-				vecOnTime := findMedian(vecOnTimes)
-				vecOffTime := findMedian(vecOffTimes)
-				if vecOffTime < vecOnTime {
-					t.l.Printf(
-						fmt.Sprintf("[q%d] vec OFF was faster by %.2f%%: "+
-							"%.2fs ON vs %.2fs OFF --- WARNING",
-							queryNum, 100*(vecOnTime-vecOffTime)/vecOffTime, vecOnTime, vecOffTime))
-				} else {
-					t.l.Printf(
-						fmt.Sprintf("[q%d] vec ON was faster by %.2f%%: "+
-							"%.2fs ON vs %.2fs OFF",
-							queryNum, 100*(vecOffTime-vecOnTime)/vecOnTime, vecOnTime, vecOffTime))
-				}
-				if vecOnTime >= vecOnSlowerFailFactor*vecOffTime {
-					t.Fatal(fmt.Sprintf(
-						"[q%d] vec ON is slower by %.2f%% than vec OFF\n"+
-							"vec ON times: %v\nvec OFF times: %v",
-						queryNum, 100*(vecOnTime-vecOffTime)/vecOffTime, vecOnTimes, vecOffTimes))
-				}
+			vecOnTime := findMedian(vecOnTimes)
+			vecOffTime := findMedian(vecOffTimes)
+			if vecOffTime < vecOnTime {
+				t.l.Printf(
+					fmt.Sprintf("[q%d] vec OFF was faster by %.2f%%: "+
+						"%.2fs ON vs %.2fs OFF --- WARNING\n"+
+						"vec ON times: %v\t vec OFF times: %v",
+						queryNum, 100*(vecOnTime-vecOffTime)/vecOffTime,
+						vecOnTime, vecOffTime, vecOnTimes, vecOffTimes))
+			} else {
+				t.l.Printf(
+					fmt.Sprintf("[q%d] vec ON was faster by %.2f%%: "+
+						"%.2fs ON vs %.2fs OFF\n"+
+						"vec ON times: %v\t vec OFF times: %v",
+						queryNum, 100*(vecOffTime-vecOnTime)/vecOnTime,
+						vecOnTime, vecOffTime, vecOnTimes, vecOffTimes))
+			}
+			if vecOnTime >= vecOnSlowerFailFactor*vecOffTime {
+				t.Fatal(fmt.Sprintf(
+					"[q%d] vec ON is slower by %.2f%% than vec OFF\n"+
+						"vec ON times: %v\nvec OFF times: %v",
+					queryNum, 100*(vecOnTime-vecOffTime)/vecOffTime, vecOnTimes, vecOffTimes))
 			}
 		}
 	}

diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go
@@ -86,10 +86,10 @@ type alterRoleRun struct {
 func (n *alterRoleNode) startExec(params runParams) error {
 	var opName string
 	if n.isRole {
-		sqltelemetry.IAMAlter(sqltelemetry.Role)
+		sqltelemetry.IncIAMAlterCounter(sqltelemetry.Role)
 		opName = "alter-role"
 	} else {
-		sqltelemetry.IAMAlter(sqltelemetry.User)
+		sqltelemetry.IncIAMAlterCounter(sqltelemetry.User)
 		opName = "alter-user"
 	}
 	name, err := n.name()

diff --git a/pkg/sql/create_role.go b/pkg/sql/create_role.go
@@ -97,10 +97,10 @@ func (p *planner) CreateRoleNode(
 func (n *CreateRoleNode) startExec(params runParams) error {
 	var opName string
 	if n.isRole {
-		sqltelemetry.IncIAMCreate(sqltelemetry.Role)
+		sqltelemetry.IncIAMCreateCounter(sqltelemetry.Role)
 		opName = "create-role"
 	} else {
-		sqltelemetry.IncIAMCreate(sqltelemetry.User)
+		sqltelemetry.IncIAMCreateCounter(sqltelemetry.User)
 		opName = "create-user"
 	}
 

diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
@@ -70,10 +70,10 @@ type dropRoleRun struct {
 func (n *DropRoleNode) startExec(params runParams) error {
 	var opName string
 	if n.isRole {
-		sqltelemetry.IncIAMDrop(sqltelemetry.Role)
+		sqltelemetry.IncIAMDropCounter(sqltelemetry.Role)
 		opName = "drop-role"
 	} else {
-		sqltelemetry.IncIAMDrop(sqltelemetry.User)
+		sqltelemetry.IncIAMDropCounter(sqltelemetry.User)
 		opName = "drop-user"
 	}
 

diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go
@@ -31,9 +31,9 @@ import (
 //          mysql requires the "grant option" and the same privileges, and sometimes superuser.
 func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) {
 	if n.Targets.Databases != nil {
-		sqltelemetry.IncIAMGrantPrivileges(sqltelemetry.OnDatabase)
+		sqltelemetry.IncIAMGrantPrivilegesCounter(sqltelemetry.OnDatabase)
 	} else {
-		sqltelemetry.IncIAMGrantPrivileges(sqltelemetry.OnTable)
+		sqltelemetry.IncIAMGrantPrivilegesCounter(sqltelemetry.OnTable)
 	}
 
 	return &changePrivilegesNode{
@@ -57,9 +57,9 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) {
 //          mysql requires the "grant option" and the same privileges, and sometimes superuser.
 func (p *planner) Revoke(ctx context.Context, n *tree.Revoke) (planNode, error) {
 	if n.Targets.Databases != nil {
-		sqltelemetry.IncIAMRevokePrivileges(sqltelemetry.OnDatabase)
+		sqltelemetry.IncIAMRevokePrivilegesCounter(sqltelemetry.OnDatabase)
 	} else {
-		sqltelemetry.IncIAMRevokePrivileges(sqltelemetry.OnTable)
+		sqltelemetry.IncIAMRevokePrivilegesCounter(sqltelemetry.OnTable)
 	}
 
 	return &changePrivilegesNode{

diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go
@@ -1796,6 +1796,9 @@ func (b *Builder) buildWindow(w *memo.WindowExpr) (execPlan, error) {
 		item := &w.Windows[i]
 		fn := b.extractWindowFunction(item.Function)
 		name, overload := memo.FindWindowOverload(fn)
+		if !b.disableTelemetry {
+			telemetry.Inc(sqltelemetry.WindowFunctionCounter(name))
+		}
 		props, _ := builtins.GetBuiltinProperties(name)
 
 		args := make([]tree.TypedExpr, fn.ChildCount())

diff --git a/pkg/sql/roleoption/role_option.go b/pkg/sql/roleoption/role_option.go
@@ -97,7 +97,7 @@ func (rol List) GetSQLStmts(op string) (map[string]func() (bool, string, error),
 	}
 
 	for _, ro := range rol {
-		sqltelemetry.IncIAMOption(
+		sqltelemetry.IncIAMOptionCounter(
 			op,
 			strings.ToLower(ro.Option.String()),
 		)

diff --git a/pkg/sql/sem/transform/window.go b/pkg/sql/sem/transform/window.go
diff --git a/pkg/sql/sem/tree/expr.go b/pkg/sql/sem/tree/expr.go
@@ -1266,26 +1266,6 @@ func (node *FuncExpr) ResolvedOverload() *Overload {
 	return node.fn
 }
 
-// GetAggregateConstructor exposes the AggregateFunc field for use by
-// the group node in package sql.
-func (node *FuncExpr) GetAggregateConstructor() func(*EvalContext, Datums) AggregateFunc {
-	if node.fn == nil || node.fn.AggregateFunc == nil {
-		return nil
-	}
-	return func(evalCtx *EvalContext, arguments Datums) AggregateFunc {
-		types := typesOfExprs(node.Exprs)
-		return node.fn.AggregateFunc(types, evalCtx, arguments)
-	}
-}
-
-func typesOfExprs(exprs Exprs) []*types.T {
-	types := make([]*types.T, len(exprs))
-	for i, expr := range exprs {
-		types[i] = expr.(TypedExpr).ResolvedType()
-	}
-	return types
-}
-
 // IsGeneratorApplication returns true iff the function applied is a generator (SRF).
 func (node *FuncExpr) IsGeneratorApplication() bool {
 	return node.fn != nil && node.fn.Generator != nil