coldata: make coldata.BatchSize a package function #40866
Conversation
|
This change is |
There was a problem hiding this comment.
I think we should keep sizes of 1, 2, and 3 because our unit test cases often have very few input tuples, so the varying small batch size will test "more cases." I definitely remember seeing some failures on merge joiner on the batch size of 2 or 3 but not on others.
Reviewed 58 of 58 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/sql/colexec/select_in_tmpl.go, line 30 at r1 (raw file):
"github.com/cockroachdb/cockroach/pkg/col/coldata" "github.com/cockroachdb/cockroach/pkg/col/coltypes"
[nit]: seems like extra new line.
There was a problem hiding this comment.
bors r=yuzefovich
Reviewable status:
pkg/sql/colexec/select_in_tmpl.go, line 30 at r1 (raw file):
Previously, yuzefovich wrote…
[nit]: seems like extra new line.
Thanks. No idea why goland keeps doing this.
Build failed |
|
bors r=yuzefovich |
40866: coldata: make coldata.BatchSize a package function r=yuzefovich a=asubiotto This will allow us to vary the batchSize private variable int the future by providing a testing hook for external packages to modify the batch size at will. Release justification: Category 1 non-production code change. Release note: None The only "interesting" changes are in `coldata/batch.go` and `colexec/utils_test.go` where I made a change to use `BatchSize()` rather than hardcode batch sizes to use. In passing, I also noticed we test batch sizes of 1, 2, and 3 elements. Do we need to do that? If not, we could remove one of them to reduce test runtime by `~2s`. Co-authored-by: Alfonso Subiotto Marqués <[email protected]>
Build failed |
|
Trying again. bors r=yuzefovich |
Build failed (retrying...) |
|
Ahh, actually seems to be test compilation errors that I missed. Fixing. |
Build failed (retrying...) |
|
bors r- |
Canceled |
|
OK, I think I got all of them bors r=yuzefovich |
Build failed |
This will allow us to vary the batchSize private variable int the future by providing a testing hook for external packages to modify the batch size at will. Release justification: Category 1 non-production code change. Release note: None
|
bors r=yuzefovich |
40866: coldata: make coldata.BatchSize a package function r=yuzefovich a=asubiotto This will allow us to vary the batchSize private variable int the future by providing a testing hook for external packages to modify the batch size at will. Release justification: Category 1 non-production code change. Release note: None The only "interesting" changes are in `coldata/batch.go` and `colexec/utils_test.go` where I made a change to use `BatchSize()` rather than hardcode batch sizes to use. In passing, I also noticed we test batch sizes of 1, 2, and 3 elements. Do we need to do that? If not, we could remove one of them to reduce test runtime by `~2s`. Co-authored-by: Alfonso Subiotto Marqués <[email protected]>
Build succeeded |
This will allow us to vary the batchSize private variable int the future
by providing a testing hook for external packages to modify the batch
size at will.
Release justification: Category 1 non-production code change.
Release note: None
The only "interesting" changes are in
coldata/batch.goandcolexec/utils_test.gowhere I made a change to useBatchSize()rather than hardcode batch sizes to use. In passing, I also noticed we test batch sizes of 1, 2, and 3 elements. Do we need to do that? If not, we could remove one of them to reduce test runtime by~2s.