-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
workload/bank: batch initial row generation #39719
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)
pkg/workload/bank/bank.go, line 129 at r1 (raw file):
// Tables implements the Generator interface. func (b *bank) Tables() []workload.Table { numBatches := int(math.Ceil(float64(b.rows) / float64(b.batchSize)))
Float arithmetic seems like it could cause issues. I'd favor integer arithmetic for something like this:
numBatches := (b.rows + b.batchSize - 1) / b.batchSize // ceil(b.rows/b.batchSize)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
bors r=nvanbenschoten
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)
pkg/workload/bank/bank.go, line 129 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Float arithmetic seems like it could cause issues. I'd favor integer arithmetic for something like this:
numBatches := (b.rows + b.batchSize - 1) / b.batchSize // ceil(b.rows/b.batchSize)
Whoops, totally agree. I meant to go back and do that myself before I PR'd this and forgot. Done
Build failed |
Release note: None
Fixed my broken tests bors r=nvanbenschoten |
Build failed |
Flake is #39653 bors r=nvanbenschoten |
Build succeeded |
Release note: None