sql: minor improvements around estimated row count #118679
Conversation
This commit makes it so that we use the ceiling of the estimated row count in scans and aggregation. It also fixes recently-introduced bug that could cause integer overflow in the agg alloc code. Release note: None
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @yuzefovich)
pkg/sql/colexec/hash_aggregator.go line 205 at r1 (raw file):
if args.EstimatedRowCount != 0 { // Use uint64s for comparison to prevent overflow in case // args.EstimatedRowCount is larger than MaxInt64.
So the estimated row count from the optimizer is fine, just overflowing here? Good catch!
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/hash_aggregator.go line 205 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
So the estimated row count from the optimizer is fine, just overflowing here? Good catch!
I think so - the problem occurred because we took the estimated row count (which is uint64 and could be very large), then down-casted it to int64 possibly producing negative numbers. This change prevents that from happening.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:
|
TFTRs! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
This commit makes it so that we use the ceiling of the estimated row count in scans and aggregation. It also fixes recently-introduced bug that could cause integer overflow in the agg alloc code.
Fixes: #118724.
Epic: None
Release note: None