-
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 floating point precision error in statisticsBuilder #38624
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)
pkg/sql/opt/memo/statistics_builder.go, line 2664 at r1 (raw file):
if nullsRemoved == rowCount { // Note: this approach protects against floating point precision errors. nullsRemoved *= 0.9999999
It would be more straightforward if we just did selectivity *= 1e-7
(or whatever) instead in this case.
This commit fixes a floating point precision error in the statisticsBuilder code for estimating the selectivity due to a null-rejecting filter. Prior to this commit, the code was subtracting one from the nullsRemoved estimate if needed to avoid estimating selectivity=0. But the problem is, if nullsRemoved is extremely large (e.g., 2e+20), subtracting 1 does nothing since it's below the precision threshold. This commit changes the logic so now we directly multiply the selectivity by a small number (1e-7) if necessary. This has the same effect as subtracting 1 from nullsRemoved, but without the risk of a floating point error. Fixes cockroachdb#38344 Release note: None
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!
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj and @RaduBerinde)
pkg/sql/opt/memo/statistics_builder.go, line 2664 at r1 (raw file):
Previously, RaduBerinde wrote…
It would be more straightforward if we just did
selectivity *= 1e-7
(or whatever) instead in this case.
Good point - fixed.
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 r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj and @RaduBerinde)
bors r+ |
38624: opt: fix floating point precision error in statisticsBuilder r=rytaft a=rytaft This commit fixes a floating point precision error in the `statisticsBuilder` code for estimating the selectivity due to a null-rejecting filter. Prior to this commit, the code was subtracting one from the `nullsRemoved` estimate if needed to avoid estimating selectivity=0. But the problem is, if `nullsRemoved` is extremely large (e.g., 2e+20), subtracting 1 does nothing since it's below the precision threshold. This commit changes the logic so now we multiply `nullsRemoved` by 0.9999999 if necessary. This has the same effect as subtracting 1, but without the risk of a floating point error. Fixes #38344 Release note: None Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Build succeeded |
This commit fixes a floating point precision error in the
statisticsBuilder
code for estimating the selectivity due toa null-rejecting filter.
Prior to this commit, the code was subtracting one from the
nullsRemoved
estimate if needed to avoid estimating selectivity=0.But the problem is, if
nullsRemoved
is extremely large (e.g., 2e+20),subtracting 1 does nothing since it's below the precision threshold.
This commit changes the logic so now we multiply
nullsRemoved
by0.9999999 if necessary. This has the same effect as subtracting 1,
but without the risk of a floating point error.
Fixes #38344
Release note: None