colexec: adjust the distribution of randomized batch size#50167
colexec: adjust the distribution of randomized batch size#50167craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
This commit adjusts the distribution of randomized batch size to favor the small sizes yet keeps the possibility of larger sizes as well. Such distribution is chosen due to the fact that most of our unit tests don't have a lot of data, so in order to exercise the multi-batch behavior we favor really small batch sizes. On the other hand, we also want to occasionally exercise that we handle batch sizes larger than default one correctly. Release note: None
|
This change is |
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/main_test.go, line 100 at r1 (raw file):
// exercise that we handle batch sizes larger than default one // correctly. var sizesToChooseFrom = []int{
It might make sense to put this into coldata and reuse here since I think logic tests also attempt to do something similar re distribution and might benefit from just calling this when it's moved into coldata.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/main_test.go, line 100 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
It might make sense to put this into
coldataand reuse here since I think logic tests also attempt to do something similar re distribution and might benefit from just calling this when it's moved intocoldata.
I disagree with this for a couple of reasons:
- currently the method is very easy to use, and the caller can decide on the distribution of random values that suits the caller's needs. If we force the distribution into the method itself, it won't be as flexible.
- the distributions in
colexec/main_test.goandlogic.goare different - note that incolexecwe have aminBatchSizeprohibiting values of1and2be used, but inlogic.gothose are actually quite desirable. If we make an investment in auditing the unit tests incolexecto remove this limitation, then maybe we could unify the two calls.
asubiotto
left a comment
There was a problem hiding this comment.
Reviewable status:
|
TFTR! bors r+ |
Build succeeded |
This commit adjusts the distribution of randomized batch size to favor
the small sizes yet keeps the possibility of larger sizes as well. Such
distribution is chosen due to the fact that most of our unit tests
don't have a lot of data, so in order to exercise the multi-batch
behavior we favor really small batch sizes. On the other hand, we also
want to occasionally exercise that we handle batch sizes larger than
default one correctly.
Release note: None