sql: add partial statistics predicate column to system.table_statistics#91248
Conversation
|
This change is |
997a253 to
bcace43
Compare
system.table_statisticssystem.table_statistics
af73bf8 to
117fa59
Compare
|
RFC link, for the curious: https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20220126_partial_statistics_collection.md |
117fa59 to
56e62c2
Compare
rytaft
left a comment
There was a problem hiding this comment.
Nice work! Mostly just some nits.
Reviewed 11 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @faizaanmadhani and @michae2)
-- commits line 14 at r2:
this is technically a user-visible change, so I'd add a Release note (sql change)
pkg/clusterversion/cockroach_versions.go line 317 at r1 (raw file):
V23_1Start // TenantNames adds a name column to system.tenants.
(Not your change, but this should be V23_1TenantNames)
pkg/clusterversion/cockroach_versions.go line 325 at r1 (raw file):
V23_1DescIDSequenceForSystemTenant // AddPartialPredicateCol adds a column to store the predicate
nit: V23_1AddPartialPredicateCol
Also, I'd call this V23_1AddPartialStatisticsPredicateCol (or V23_1AddTableStatisticsPartialPredicateCol) to make it really clear what this is.
pkg/clusterversion/cockroach_versions.go line 326 at r1 (raw file):
// AddPartialPredicateCol adds a column to store the predicate // for a partial statistics collection
nit: end comments with a period.
pkg/upgrade/upgrades/alter_system_table_partial_predicate.go line 1 at r1 (raw file):
// Copyright 2022 The Cockroach Authors.
I'd name this file alter_table_statistics_partial_predicate.go for consistency with some of the other files (doesn't seem like there is one consistent naming scheme, but I think referencing table_statistics is still better)
pkg/upgrade/upgrades/alter_system_table_partial_predicate.go line 33 at r1 (raw file):
op := operation{ name: "add-table-statistics-partialPredicate-col", schemaList: []string{"total_consumption"},
total_consumption -> partialPredicate
pkg/upgrade/upgrades/alter_system_table_partial_predicate_test.go line 64 at r1 (raw file):
) // Inject the old copy of the descriptor
nit: end comment with period (here and below).
pkg/upgrade/upgrades/alter_system_table_partial_predicate_test.go line 146 at r1 (raw file):
NextFamilyID: 1, PrimaryIndex: descpb.IndexDescriptor{ Name: "primary",
nit: can use tabledesc.LegacyPrimaryKeyIndexName
|
|
dc9860b to
6678f35
Compare
faizaanmadhani
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, rytaft (Rebecca Taft) wrote…
this is technically a user-visible change, so I'd add a
Release note (sql change)
Done.
pkg/clusterversion/cockroach_versions.go line 317 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
(Not your change, but this should be V23_1TenantNames)
Done.
pkg/clusterversion/cockroach_versions.go line 325 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
nit: V23_1AddPartialPredicateCol
Also, I'd call this V23_1AddPartialStatisticsPredicateCol (or V23_1AddTableStatisticsPartialPredicateCol) to make it really clear what this is.
Done.
pkg/clusterversion/cockroach_versions.go line 326 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
nit: end comments with a period.
Done.
pkg/upgrade/upgrades/alter_system_table_partial_predicate.go line 1 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I'd name this file
alter_table_statistics_partial_predicate.gofor consistency with some of the other files (doesn't seem like there is one consistent naming scheme, but I think referencing table_statistics is still better)
Done.
pkg/upgrade/upgrades/alter_system_table_partial_predicate.go line 33 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
total_consumption -> partialPredicate
Done.
pkg/upgrade/upgrades/alter_system_table_partial_predicate_test.go line 146 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
nit: can use tabledesc.LegacyPrimaryKeyIndexName
Done.
6678f35 to
ccebb5a
Compare
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 8 of 11 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:
6fa0404 to
eaaf2a9
Compare
michae2
left a comment
There was a problem hiding this comment.
Reviewed 5 of 11 files at r1, 4 of 11 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, 4 of 4 files at r7, all commit messages.
Reviewable status:
pkg/ccl/logictestccl/tests/fakedist/BUILD.bazel line 13 at r7 (raw file):
"//pkg/ccl/logictestccl:testdata", # keep ], shard_count = 7,
Did these come from ./dev gen bazel --short? I'm not sure why they changed.
Yes, I'm not sure what caused them to change too, but I'm gonna try rebasing with master and rerunning the build. |
2ef042a to
f8f9b12
Compare
This commit adds a column to system.table_statistics to store a string representing the predicate for a partial statistics collection. Previously, only full table statistics could be stored in `system.table_statistics`, and this commit adds support to store partial table statistics to this table. It assists Epic CRDB-19449 and is a prerequisite for adding support for collecting partial statistics. Epic: CRDB-19449 Release note (sql change): system.table_statistics now contains a column called partialPredicate to store a predicate for a partial statistic collection.
f8f9b12 to
2e39af4
Compare
|
TFTRs! 🎉 bors r=rytaft,michae2 |
|
Build succeeded: |
Previously, the partial statistics rfc indicated that we would need to add a column to `system.table_statistics` that would store a boolean indicating that the statistic is partial. However, the implementation differs from this (cockroachdb#91248) and instead adds a column to store a string containing a predicate. This PR updates the RFC to be consistent with this change. Release note: None Epic: CRDB-19449
91759: rfcs: update the partial stats collection rfc to reflect implementation r=rytaft a=faizaanmadhani Previously, the partial statistics rfc indicated that we would need to add a column to `system.table_statistics` that would store a boolean indicating that the statistic is partial. However, the implementation differs from this (#91248) and instead adds a column to store a string containing a predicate. This PR updates the RFC to be consistent with this change. Release note: None Epic: CRDB-19449 Co-authored-by: Faizaan Madhani <fzmadhani@gmail.com>
This commit adds a column to system.table_statistics to store
a string representing the predicate for a partial statistics
collection. Previously, only full table statistics could be
stored in
system.table_statistics, and this commit addssupport to store partial table statistics to this table. It
assists Epic CRDB-19449 and is a prerequisite for adding
support for collecting partial statistics.
Epic: CRDB-19449
Release note (sql change): system.table_statistics now
contains a column called partialPredicate to store a
predicate for a partial statistic collection.