workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93814

rytaft · 2022-12-16T21:04:03Z

workload: fix TPC-H generator to generate correct values for p_name

The TPC-H generator creates values for p_name in the part table by selecting 5 elements from a list of words called randPartNames. It is supposed to select 5 random unique elements from the full list, but prior to this commit, it only selected a permutation of the first 5 elements. This commit fixes the logic and adds a unit test.

Epic: None
Release note: None

workload: fix TPC-H generator value of ps_supplycost

Prior to this commit, the TPC-H generator was generating
a value between 0 and 10 for ps_supplycost in the partsupp
table. However, the spec calls for a random value in the range
[1.00 .. 1,000.00]. This commit fixes the oversight.

Epic: None
Release note: None

cockroach-teamcity · 2022-12-16T21:04:12Z

This change is

jordanlewis · 2022-12-16T21:22:46Z

pkg/workload/tpch/generate.go

@@ -213,7 +213,7 @@ func (w *tpch) tpchPartSuppInitialRowBatch(
 		// PS_AVAILQTY random value [1 .. 9,999].
 		availQtyCol[i] = int16(randInt(rng, 1, 9999))
 		// PS_SUPPLYCOST random value [1.00 .. 1,000.00].
-		supplyCostCol[i] = float64(randFloat(rng, 1, 1000, 100))


This seems out of sync with the comment - should the comment be 10,000 or should the value stay 1,000? Or does this function do something odd?

The comment is correct. randFloat does do something a bit odd:

func randFloat(rng *rand.Rand, x, y, shift int) float32 { return float32(randInt(rng, x, y)) / float32(shift) }

Just realized that I needed to change the first value too -- done.

jordanlewis · 2022-12-16T21:23:51Z

pkg/workload/tpch/random.go

-		namePerm[i] = namePerm[j]
-		namePerm[j] = i
+		j := rng.Intn(len(namePerm))
+		namePerm[i], namePerm[j] = namePerm[j], namePerm[i]


🤦
How did you notice this?

I noticed some weird values while working on a stats PR over a year ago (this has been on my todo list since then... haha)

haha, woohoo!

jordanlewis · 2022-12-16T21:35:27Z

pkg/workload/tpch/generate.go

@@ -213,7 +213,7 @@ func (w *tpch) tpchPartSuppInitialRowBatch(
 		// PS_AVAILQTY random value [1 .. 9,999].
 		availQtyCol[i] = int16(randInt(rng, 1, 9999))
 		// PS_SUPPLYCOST random value [1.00 .. 1,000.00].
-		supplyCostCol[i] = float64(randFloat(rng, 1, 1000, 100))


jordanlewis · 2022-12-16T21:35:41Z

pkg/workload/tpch/random.go

-		namePerm[i] = namePerm[j]
-		namePerm[j] = i
+		j := rng.Intn(len(namePerm))
+		namePerm[i], namePerm[j] = namePerm[j], namePerm[i]


haha, woohoo!

The TPC-H generator creates values for p_name in the part table by selecting 5 elements from a list of words called randPartNames. It is supposed to select 5 random unique elements from the full list, but prior to this commit, it only selected a permutation of the first 5 elements. This commit fixes the logic and adds a unit test. Epic: None Release note: None

rytaft

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @smg260)

craig · 2022-12-17T01:18:25Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

srosenberg

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @rytaft, and @smg260)

pkg/workload/tpch/random_test.go line 46 at r4 (raw file):

	// We can't guarantee much about the global distribution of names,
	// but we should make sure that we're not always using the same 5

I think we could safely make a stronger assertion. 92 choose 5 gives 4,9177,128 unique combinations. After 100 shuffles, the probability of seeing the same combination is astronomically low.

pkg/workload/tpch/random_test.go line 49 at r4 (raw file):

	// names. Run up to 100 times before failing.
	for i := 0; i < 100; i++ {
		if len(seen) > 5 {

nPartNames instead of the magic number.

craig · 2022-12-17T04:34:52Z

Build failed:

Bazel Essential CI (Cockroach)

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @renatolabs, @rytaft, and @smg260)

pkg/workload/tpch/random.go line 133 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

haha, woohoo!

I remember talking to Lina about this bug (which she shared with me) at some point this Summer, but we forgot to follow up.

Prior to this commit, the TPC-H generator was generating a value between 0 and 10 for ps_supplycost in the partsupp table. However, the spec calls for a random value in the range [1.00 .. 1,000.00]. This commit fixes the oversight. Epic: None Release note: None

rytaft

TFTRs! Last failure is an unrelated flake: #93460.

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @renatolabs, @smg260, and @srosenberg)

pkg/workload/tpch/random.go line 133 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I remember talking to Lina about this bug (which she shared with me) at some point this Summer, but we forgot to follow up.

👍

pkg/workload/tpch/random_test.go line 46 at r4 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

I think we could safely make a stronger assertion. 92 choose 5 gives 4,9177,128 unique combinations. After 100 shuffles, the probability of seeing the same combination is astronomically low.

Done.

pkg/workload/tpch/random_test.go line 49 at r4 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

nPartNames instead of the magic number.

Done.

craig · 2022-12-19T16:28:59Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-12-19T18:12:37Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-12-19T19:51:37Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2022-12-19T19:51:53Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 7c9dd1b to blathers/backport-release-22.1-93814: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

rytaft requested review from jordanlewis and a team December 16, 2022 21:04

rytaft requested a review from a team as a code owner December 16, 2022 21:04

rytaft requested review from smg260 and renatolabs and removed request for a team December 16, 2022 21:04

rytaft changed the title ~~workload: fix TPC-H generator to generate correct values for p_name~~ workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost Dec 16, 2022

jordanlewis approved these changes Dec 16, 2022

View reviewed changes

rytaft force-pushed the tpch-fix branch from 9c166be to 641ab9c Compare December 16, 2022 21:33

jordanlewis approved these changes Dec 16, 2022

View reviewed changes

rytaft added backport-22.1.x labels Dec 16, 2022

rytaft force-pushed the tpch-fix branch from 641ab9c to 1f136e6 Compare December 16, 2022 21:49

rytaft force-pushed the tpch-fix branch from 1f136e6 to 7da6336 Compare December 16, 2022 22:53

rytaft commented Dec 17, 2022

View reviewed changes

srosenberg approved these changes Dec 17, 2022

View reviewed changes

yuzefovich reviewed Dec 19, 2022

View reviewed changes

rytaft force-pushed the tpch-fix branch from 7da6336 to d711549 Compare December 19, 2022 15:45

rytaft commented Dec 19, 2022

View reviewed changes

craig bot merged commit 91fbbad into cockroachdb:master Dec 19, 2022

blathers-crl bot mentioned this pull request Dec 19, 2022

release-22.2: workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93932

Merged

rytaft mentioned this pull request Dec 19, 2022

release-22.1: workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93814

workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93814

rytaft commented Dec 16, 2022 •

edited

Loading

cockroach-teamcity commented Dec 16, 2022

jordanlewis Dec 16, 2022

rytaft Dec 16, 2022

rytaft Dec 16, 2022

jordanlewis Dec 16, 2022

jordanlewis Dec 16, 2022

rytaft Dec 16, 2022

jordanlewis Dec 16, 2022

jordanlewis Dec 16, 2022

jordanlewis Dec 16, 2022

rytaft left a comment

craig bot commented Dec 17, 2022

srosenberg left a comment

craig bot commented Dec 17, 2022

yuzefovich left a comment

rytaft left a comment

craig bot commented Dec 19, 2022

craig bot commented Dec 19, 2022

craig bot commented Dec 19, 2022

blathers-crl bot commented Dec 19, 2022

workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93814

workload: fix TPC-H generator to generate correct values for p_name and ps_supplycost #93814

Conversation

rytaft commented Dec 16, 2022 • edited Loading

cockroach-teamcity commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

craig bot commented Dec 17, 2022

srosenberg left a comment

Choose a reason for hiding this comment

craig bot commented Dec 17, 2022

yuzefovich left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

craig bot commented Dec 19, 2022

craig bot commented Dec 19, 2022

craig bot commented Dec 19, 2022

blathers-crl bot commented Dec 19, 2022

rytaft commented Dec 16, 2022 •

edited

Loading