sql: fix expected batch count for edge case in copy test #109292
Conversation
In TestLargeDynamicRows we test that 4 rows of data can fit in a batch size of at least 4 rows given default memory sizes. However, when we set the batch row size to the minimum value of 4, the test hook that counts batches counts an extra empty batch. This PR changes adjusts the minimum row size to 5 for the purposes of this test. Epic: None Fixes: cockroachdb#109134 Release note: None
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cucaroach)
pkg/sql/copy/copy_test.go line 605 at r1 (raw file):
// This won't work if the batch size gets set to less than 5. When the batch // size is 4, the test hook will count an extra empty batch.
An alternative approach I tried is to move the call to the test hook TestingKnobs.BeforeCopyFromInsert below where we check the number of rows to insert in insertRowsInternal so that we don't call it if there are 0 rows (as is the case in the final batch), but counting the empty batch may be more accurate. Let me know if you think that approach would be better. Either way, this is a test only change.
There was a problem hiding this comment.
Thanks for fixing this!
Reviewable status:
pkg/sql/copy/copy_test.go line 605 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
An alternative approach I tried is to move the call to the test hook
TestingKnobs.BeforeCopyFromInsertbelow where we check the number of rows to insert ininsertRowsInternalso that we don't call it if there are 0 rows (as is the case in the final batch), but counting the empty batch may be more accurate. Let me know if you think that approach would be better. Either way, this is a test only change.
This is fine I think.
|
TFTR! bors r+ |
|
👎 Rejected by too few approved reviews |
|
bors r+ |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build succeeded: |
|
blathers backport 23.1 |
In TestLargeDynamicRows we test that 4 rows of data can fit in a batch size of at least 4 rows given default memory sizes. However, when we set the batch row size to the minimum value of 4, the test hook that counts batches counts an extra empty batch. This PR changes adjusts the minimum row size to 5 for the purposes of this test.
Epic: None
Fixes: #109134
Release note: None