From c9f93f8d6d97b4c8008ef75c2164c32d93d5e0ae Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 7 Nov 2019 16:34:25 -0800 Subject: [PATCH] workload/tpch: fix the check of the output on TPCH workload Previously, when doing the check that TPCH queries return the expected output, we would round down both the expected and the actual float values to a hundredth, and then we would confirm that the rounded values differ by no more than 0.01 (this is what the spec requires). However, this can be problematic in some scenarios. It is possible, for example, for two values 0.601 and 0.609 be rounded in such a way that they are represented by 0.599998 and 0.610001, respectively. Then, when the check that the difference is no greater than 0.01 would fail although the numbers are correct (according to the spec). Now this is fixed by rounding to a thousandth. It turns out that this was the problem that I occasionally observed when running TPCHVec roachtest - it would show up as "exit status 1" because of the failed check. Release note: None --- pkg/cmd/roachtest/tpchvec.go | 7 ++----- pkg/workload/tpch/tpch.go | 12 +++++++++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/roachtest/tpchvec.go b/pkg/cmd/roachtest/tpchvec.go index 059bd7510fa2..65e883837155 100644 --- a/pkg/cmd/roachtest/tpchvec.go +++ b/pkg/cmd/roachtest/tpchvec.go @@ -78,11 +78,6 @@ RESTORE tpch.* FROM 'gs://cockroach-fixtures/workload/tpch/scalefactor=1/backup' t.Status("waiting for full replication") waitForFullReplication(t, conn) timeByQueryNum := make([]map[int][]float64, 2) - // Note that the order in which we run the configuration is important: - // there are some issues with dropping a view (created in query 15), so if - // we run vec off first, the vec on config run might error out. - // TODO(yuzefovich): figure out what is the root problem or create an issue - // about it. for configIdx, vectorize := range []bool{true, false} { // To reduce the variance on the first query we're interested in, we'll // do an aggregation over all tables. This will make comparison on two @@ -122,6 +117,8 @@ RESTORE tpch.* FROM 'gs://cockroach-fixtures/workload/tpch/scalefactor=1/backup' queriesToRun, vectorizeSetting, nodeCount) workloadOutput, err := repeatRunWithBuffer(ctx, c, t.l, firstNode, operation, cmd) 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.Fatal(err) } t.l.Printf(string(workloadOutput)) diff --git a/pkg/workload/tpch/tpch.go b/pkg/workload/tpch/tpch.go index 8192857db89e..969af6e744f1 100644 --- a/pkg/workload/tpch/tpch.go +++ b/pkg/workload/tpch/tpch.go @@ -242,14 +242,20 @@ func (w *worker) run(ctx context.Context) error { queryName, err, numRows, i, actualValue, expectedValue) } // TPC-H spec requires 0.01 precision for DECIMALs, so we will - // first round the values to use in the comparison. - expectedFloatRounded, err := strconv.ParseFloat(fmt.Sprintf("%.2f", expectedFloat), 64) + // first round the values to use in the comparison. Note that we + // round to a thousandth so that values like 0.601 and 0.609 were + // always considered to differ by less than 0.01 (due to the + // nature of representation of floats, it is possible that those + // two values when rounded to a hundredth would be represented as + // something like 0.59999 and 0.610001 which differ by more than + // 0.01). + expectedFloatRounded, err := strconv.ParseFloat(fmt.Sprintf("%.3f", expectedFloat), 64) if err != nil { return errors.Errorf("[q%s] failed parsing rounded 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) } - actualFloatRounded, err := strconv.ParseFloat(fmt.Sprintf("%.2f", actualFloat), 64) + actualFloatRounded, err := strconv.ParseFloat(fmt.Sprintf("%.3f", actualFloat), 64) if err != nil { return errors.Errorf("[q%s] failed parsing rounded actual value as float64 with %s\n"+ "wrong result in row %d in column %d: got %q, expected %q",