Merge #42087 #42113

42087: workload: minor enhancement of tpch workload r=yuzefovich a=yuzefovich

This commit removes `distsql` option (since we now only have a
distributed engine) and adds the following:
1. `vectorize` option to override the vectorize session variable
2. `verbose` option to specify whether histograms and the queries
themselves should be output (default is set to `false`)
3. checking whether the expected output was returned is improved (if a
difference is encountered, we first try converting both the expected and
the actual values to floats and check whether they differ by more than
0.01 - this is what TPC-H spec requires for DECIMALs).

Release note: None

42113: settings: share the maxSettings constant with the test r=andreimatei a=andreimatei

The tests were re-defining the constant, and the values had to match.

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>

pkg/settings/setting.go

@@ -17,10 +17,12 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
 
-const maxSettings = 256
+// MaxSettings is the maximum number of settings that the system supports.
+// Exported for tests.
+const MaxSettings = 256
 
 // Values is a container that stores values for all registered settings.
-// Each setting is assigned a unique slot (up to maxSettings).
+// Each setting is assigned a unique slot (up to MaxSettings).
 // Note that slot indices are 1-based (this is to trigger panics if an
 // uninitialized slot index is used).
 type Values struct {
@@ -39,16 +41,16 @@ type Values struct {
 		syncutil.Mutex
 		// NB: any in place modification to individual slices must also hold the
 		// lock, e.g. if we ever add RemoveOnChange or something.
-		onChange [maxSettings][]func()
+		onChange [MaxSettings][]func()
 	}
 	// opaque is an arbitrary object that can be set by a higher layer to make it
 	// accessible from certain callbacks (like state machine transformers).
 	opaque interface{}
 }
 
 type valuesContainer struct {
-	intVals     [maxSettings]int64
-	genericVals [maxSettings]atomic.Value
+	intVals     [MaxSettings]int64
+	genericVals [MaxSettings]atomic.Value
 }
 
 func (c *valuesContainer) setGenericVal(slotIdx int, newVal interface{}) {
@@ -212,8 +214,8 @@ func (i *common) setSlotIdx(slotIdx int) {
 	if slotIdx < 1 {
 		panic(fmt.Sprintf("Invalid slot index %d", slotIdx))
 	}
-	if slotIdx > maxSettings {
-		panic(fmt.Sprintf("too many settings; increase maxSettings"))
+	if slotIdx > MaxSettings {
+		panic(fmt.Sprintf("too many settings; increase MaxSettings"))
 	}
 	i.slotIdx = slotIdx
 }

pkg/settings/settings_test.go

@@ -24,8 +24,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-const maxSettings = 256
-
 type dummy struct {
 	msg1       string
 	growsbyone string
@@ -617,8 +615,8 @@ func TestHide(t *testing.T) {
 }
 
 func TestOnChangeWithMaxSettings(t *testing.T) {
-	// Register maxSettings settings to ensure that no errors occur.
-	maxName, err := batchRegisterSettings(t, t.Name(), maxSettings-1-len(settings.Keys()))
+	// Register MaxSettings settings to ensure that no errors occur.
+	maxName, err := batchRegisterSettings(t, t.Name(), settings.MaxSettings-1-len(settings.Keys()))
 	if err != nil {
 		t.Errorf("expected no error to register 128 settings, but get error : %s", err)
 	}
@@ -653,8 +651,8 @@ func TestMaxSettingsPanics(t *testing.T) {
 	}()
 
 	// Register too many settings which will cause a panic which is caught and converted to an error.
-	_, err := batchRegisterSettings(t, t.Name(), maxSettings-len(settings.Keys()))
-	expectedErr := "too many settings; increase maxSettings"
+	_, err := batchRegisterSettings(t, t.Name(), settings.MaxSettings-len(settings.Keys()))
+	expectedErr := "too many settings; increase MaxSettings"
 	if err == nil || err.Error() != expectedErr {
 		t.Errorf("expected error %v, but got %v", expectedErr, err)
 	}

pkg/workload/tpch/tpch.go

@@ -14,6 +14,8 @@ import (
 	"context"
 	gosql "database/sql"
 	"fmt"
+	"math"
+	"strconv"
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -42,8 +44,9 @@ type tpch struct {
 	seed        int64
 	scaleFactor int
 
-	distsql       bool
 	disableChecks bool
+	vectorize     string
+	verbose       bool
 
 	queriesRaw      string
 	selectedQueries []string
@@ -64,16 +67,21 @@ var tpchMeta = workload.Meta{
 			`queries`:        {RuntimeOnly: true},
 			`dist-sql`:       {RuntimeOnly: true},
 			`disable-checks`: {RuntimeOnly: true},
+			`vectorize`:      {RuntimeOnly: true},
 		}
 		g.flags.Int64Var(&g.seed, `seed`, 1, `Random number generator seed`)
 		g.flags.IntVar(&g.scaleFactor, `scale-factor`, 1,
 			`Linear scale of how much data to use (each SF is ~1GB)`)
-		g.flags.StringVar(&g.queriesRaw, `queries`, `1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22`,
+		g.flags.StringVar(&g.queriesRaw, `queries`,
+			`1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22`,
 			`Queries to run. Use a comma separated list of query numbers`)
-		g.flags.BoolVar(&g.distsql, `dist-sql`, true, `Use DistSQL for query execution`)
 		g.flags.BoolVar(&g.disableChecks, `disable-checks`, false,
 			"Disable checking the output against the expected rows (default false). "+
 				"Note that the checks are only supported for scale factor 1")
+		g.flags.StringVar(&g.vectorize, `vectorize`, `auto`,
+			`Set vectorize session variable`)
+		g.flags.BoolVar(&g.verbose, `verbose`, false,
+			`Prints out the queries being run as well as histograms`)
 		g.connFlags = workload.NewConnFlags(&g.flags)
 		return g
 	},
@@ -193,12 +201,7 @@ func (w *worker) run(ctx context.Context) error {
 	queryName := w.config.selectedQueries[w.ops%len(w.config.selectedQueries)]
 	w.ops++
 
-	var query string
-	if w.config.distsql {
-		query = `SET DISTSQL = 'always'; ` + queriesByName[queryName]
-	} else {
-		query = `SET DISTSQL = 'off'; ` + queriesByName[queryName]
-	}
+	query := fmt.Sprintf("SET vectorize = '%s'; %s", w.config.vectorize, queriesByName[queryName])
 
 	vals := make([]interface{}, maxCols)
 	for i := range vals {
@@ -211,22 +214,39 @@ func (w *worker) run(ctx context.Context) error {
 		defer rows.Close()
 	}
 	if err != nil {
-		return err
+		return errors.Errorf("[q%s]: %s", queryName, err)
 	}
 	var numRows int
 	for rows.Next() {
 		if !w.config.disableChecks {
 			if !queriesToCheckOnlyNumRows[queryName] && !queriesToSkip[queryName] {
 				if err = rows.Scan(vals[:numColsByQueryName[queryName]]...); err != nil {
-					return err
+					return errors.Errorf("[q%s]: %s", queryName, err)
 				}
 
 				expectedRow := expectedRowsByQueryName[queryName][numRows]
 				for i, expectedValue := range expectedRow {
 					if val := *vals[i].(*interface{}); val != nil {
-						if strings.Compare(expectedValue, fmt.Sprint(val)) != 0 {
-							return errors.Errorf("wrong result on query %s in row %d in column %d: got %q, expected %q",
-								queryName, numRows, i, fmt.Sprint(val), expectedValue)
+						actualValue := fmt.Sprint(val)
+						if strings.Compare(expectedValue, actualValue) != 0 {
+							expectedFloat, err := strconv.ParseFloat(expectedValue, 64)
+							if err != nil {
+								return errors.Errorf("[q%s] failed parsing expected value as float64 with %s\n"+
+									"wrong result in row %d in column %d: got %q, expected %q",
+									queryName, err, numRows, i, actualValue, expectedValue)
+							}
+							actualFloat, err := strconv.ParseFloat(actualValue, 64)
+							if err != nil {
+								return errors.Errorf("[q%s] failed parsing actual value as float64 with %s\n"+
+									"wrong result in row %d in column %d: got %q, expected %q",
+									queryName, err, numRows, i, actualValue, expectedValue)
+							}
+							if math.Abs(expectedFloat-actualFloat) > 0.01 {
+								// We only fail the check if the difference is more than 0.01 -
+								// this is what TPC-H spec requires for DECIMALs.
+								return errors.Errorf("[q%s] wrong result in row %d in column %d: got %q, expected %q",
+									queryName, numRows, i, actualValue, expectedValue)
+							}
 						}
 					}
 				}
@@ -235,19 +255,27 @@ func (w *worker) run(ctx context.Context) error {
 		numRows++
 	}
 	if err := rows.Err(); err != nil {
-		return err
+		return errors.Errorf("[q%s]: %s", queryName, err)
 	}
 	if !w.config.disableChecks {
 		if !queriesToSkip[queryName] {
 			if numRows != numExpectedRowsByQueryName[queryName] {
-				return errors.Errorf("query %s returned wrong number of rows: got %d, expected %d", queryName, numRows, numExpectedRowsByQueryName[queryName])
+				return errors.Errorf("[q%s] returned wrong number of rows: got %d, expected %d",
+					queryName, numRows, numExpectedRowsByQueryName[queryName],
+				)
 			}
 		}
 	}
 	elapsed := timeutil.Since(start)
-	w.hists.Get(queryName).Record(elapsed)
-	log.Infof(ctx, "[%s] return %d rows after %4.2f seconds:\n  %s",
-		queryName, numRows, elapsed.Seconds(), query)
+	if w.config.verbose {
+		w.hists.Get(queryName).Record(elapsed)
+		log.Infof(ctx, "[q%s] returned %d rows after %4.2f seconds:\n %s",
+			queryName, numRows, elapsed.Seconds(), query)
+
+	} else {
+		log.Infof(ctx, "[q%s] returned %d rows after %4.2f seconds",
+			queryName, numRows, elapsed.Seconds())
+	}
 	return nil
 }