-
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
opt: fix overflow causing zero row-count #38036
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.
Hey @mjibson - check this out! Could we lint for this?
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 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained
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.
Thanks for the quick diagnose and fix!
Reviewed 1 of 2 files at r1.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @justinj)
pkg/sql/opt/memo/statistics_builder.go, line 2530 at r1 (raw file):
// by constraint.PreferInclusive). if c.Columns.Get(col).Ascending() { distinctCount += float64(end) - float64(start)
Definitely worth a comment explaining that we're avoiding overflow.
We were subtracting two ints that could overflow and then casting the result to a float64. There's an easy solution to avoid the overflow: just cast each integer to a float64 before performing the subtraction. I will backport this to 19.1 and 2.1. Release note (bug fix): Previously, due to a bug when estimating result set sizes in the optimizer, queries involving int ranges that were very large could result in poor plans being generated.
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.
TFTRs!
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @RaduBerinde and @rytaft)
pkg/sql/opt/memo/statistics_builder.go, line 2530 at r1 (raw file):
Previously, RaduBerinde wrote…
Definitely worth a comment explaining that we're avoiding overflow.
Done.
38036: opt: fix overflow causing zero row-count r=justinj a=justinj We were subtracting two ints that could overflow and then casting the result to a float64. There's an easy solution to avoid the overflow: just cast each integer to a float64 *before* performing the subtraction. I will backport this to 19.1 and 2.1. Release note (bug fix): Previously, due to a bug when estimating result set sizes in the optimizer, queries involving int ranges that were very large could result in poor plans being generated. Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
I made a linter at one point that looked for unsafe casts and int arithmetic that didn't check for overflows at one point that would have caught this. It's hard because it has a lot of false positives, but this is definitely a thing we could pursue. |
Build failed |
bors r+ |
38036: opt: fix overflow causing zero row-count r=justinj a=justinj We were subtracting two ints that could overflow and then casting the result to a float64. There's an easy solution to avoid the overflow: just cast each integer to a float64 *before* performing the subtraction. I will backport this to 19.1 and 2.1. Release note (bug fix): Previously, due to a bug when estimating result set sizes in the optimizer, queries involving int ranges that were very large could result in poor plans being generated. Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
Filed an issue for the test flake: #38037 |
Build succeeded |
Thanks! |
We were subtracting two ints that could overflow and then casting the
result to a float64. There's an easy solution to avoid the overflow:
just cast each integer to a float64 before performing the subtraction.
I will backport this to 19.1 and 2.1.
Release note (bug fix): Previously, due to a bug when estimating result
set sizes in the optimizer, queries involving int ranges that were very
large could result in poor plans being generated.