sql/stats: miscellaneous minor cleanup #158226
Conversation
|
This change is |
yuzefovich
force-pushed
the
stats-nits
branch
2 times, most recently
from
d7f22ef to
3202082
Compare
yuzefovich
requested review from
kev-cao
and removed request for
a team and
kev-cao
mgartner
left a comment
mgartner
left a comment
There was a problem hiding this comment.
@mgartner reviewed 3 of 3 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2)
-- commits line 8 at r1:
nit: Is there an easy way to add a regression test for this?
pkg/sql/opt/cat/utils.go line 345 at r1 (raw file):
} // FindLatestFullStat finds the most recent full statistic that can be used for
nit: mention that it returns the index to be used with tab.Statistic().
michae2
left a comment
michae2
left a comment
There was a problem hiding this comment.
@michae2 reviewed 3 of 3 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status:
This commit extracts the logic for iterating over all statistics on a table to find the most recent full stat that can be utilized (i.e. while paying attention to session variables that might disallow usage of forecasts and merged partial). In particular, this allows us to fix the bug where `OptimizerUseMergedPartialStatistics` was being ignored when building the scan - the impact of that is pretty minor though, so there is no release note (the variable is `true` by default, so only non-default configs would be affected, and only that `statsCreatedTime` would be reported newer than it should - only used in logs - as well as in the telemetry we'd have the wrong row count. Release note: None
This commit contains a bunch of minor cleanups I noticed while reading the stats related code: - remove no longer used `ImportStatsName` - remove redundant disabling of auto stats in logic tests (the framework itself does it already) - fix the pre-allocation slice for out types - update TODOs with Faizaan's name to the corresponding issue numbers - wrap some comments, remove a couple stale / redundant ones, collapse some lines. Release note: None
yuzefovich
force-pushed
the
stats-nits
branch
from
3202082 to
0ca5102
Compare
yuzefovich
left a comment
yuzefovich
left a comment
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
Previously, mgartner (Marcus Gartner) wrote…
nit: Is there an easy way to add a regression test for this?
I don't think so. IIUC the bug could be observed in two ways:
- incorrect
statsCreatedTime- currently only used in the log message added by Uzair if we have stats misestimate, - incorrect
TotalScanRowsWithoutForecastsEstimatefield ofeventpb.SampledQueryin the telemetry which I think is also only observable in the logs.
So the test would need to be an integration test and would need to examine the log sink (or something along those lines), which doesn't seem worth it.
4 participants
sql/opt: unify search for latest full stat
This commit extracts the logic for iterating over all statistics on a table to find the most recent full stat that can be utilized (i.e. while paying attention to session variables that might disallow usage of forecasts and merged partial). In particular, this allows us to fix the bug where
OptimizerUseMergedPartialStatisticswas being ignored when building the scan - the impact of that is pretty minor though, so there is no release note (the variable istrueby default, so only non-default configs would be affected, and only thatstatsCreatedTimewould be reported newer than it should - only used in logs - as well as in the telemetry we'd have the wrong row count.sql/stats: miscellaneous minor cleanup
This commit contains a bunch of minor cleanups I noticed while reading the stats related code:
ImportStatsNameEpic: None
Release note: None