sql/stats: include partial stats in results of statsCache.GetTableStats#95145
sql/stats: include partial stats in results of statsCache.GetTableStats#95145craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
stevendanna
left a comment
There was a problem hiding this comment.
Code looks pretty reasonable to me. I like the various predicate methods you added.
Should we perhaps filter partial stats from backupRestore with a TODO until we fix the insertion of partial stats? Unless I'm mistaken (very possible), I think we probably do the wrong thing with the FullHistogramID on the partial stat.
Ah, thanks for reminding me, I forgot to file an issue! I think we should go ahead and backup and restore them anyway. It won't break anything serious if we do the wrong thing with the FullHistogramID, it just means we won't be able to create those merged stats after restore. |
Updated #94101 about this. |
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 10 of 14 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @michae2)
pkg/sql/distsql_plan_stats.go line 230 at r2 (raw file):
// someone could create a statistic named __forecast__ or __merged__. // Update system.table_statistics to add an enum to indicate which // type of statistic it is.
nit: update comment now that you've removed the name references
pkg/sql/stats/forecast.go line 67 at r2 (raw file):
// return the same forecasts. func ForecastTableStatistics(ctx context.Context, observed []*TableStatistic) []*TableStatistic { // Early sanity check. We'll check this again in forecastColumnStatistics.
Is this not needed anymore?
michae2
left a comment
There was a problem hiding this comment.
Thank you!
Reviewable status:
pkg/sql/distsql_plan_stats.go line 230 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
nit: update comment now that you've removed the name references
I ended up removing it altogether in favor of a comment on #50655.
pkg/sql/stats/forecast.go line 67 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Is this not needed anymore?
Yeah, it was only really needed to protect the latest := observed[0].CreatedAt which has become part of the first loop over observed. Now if len(observed) is zero then latest.IsZero() will be true after the first loop, and we will return nil. If len(observed) is less than minObservationsForForecast we will catch it at the beginning of forecastColumnStatistics.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status:
We were not including partial stats in the list of table statistics returned by `statsCache.GetTableStats`. This was fine for the optimizer, which currently cannot use partial stats directly, but it was a problem for backup. We'd like to use partial stats directly in the optimizer eventually, so this commit goes ahead and adds them to the results of `GetTableStats`. The optimizer then must filter them out. To streamline this we add some helper functions. Finally, in an act of overzealous refactoring, this commit also changes `MergedStatistics` and `ForecastTableStatistics` to accept partial statistics and full statistics mixed together in the same input list. This simplifies the code that calls these functions. Fixes: cockroachdb#95056 Part of: cockroachdb#93983 Epic: CRDB-19449 Release note: None
|
bors r=rytaft |
|
Build failed (retrying...): |
|
Build succeeded: |
We were not including partial stats in the list of table statistics returned by
statsCache.GetTableStats. This was fine for the optimizer, which currently cannot use partial stats directly, but it was a problem for backup.We'd like to use partial stats directly in the optimizer eventually, so this commit goes ahead and adds them to the results of
GetTableStats. The optimizer then must filter them out. To streamline this we add some helper functions.Finally, in an act of overzealous refactoring, this commit also changes
MergedStatisticsandForecastTableStatisticsto accept partial statistics and full statistics mixed together in the same input list. This simplifies the code that calls these functions.Fixes: #95056
Part of: #93983
Epic: CRDB-19449
Release note: None