sql/stats: fix histogram type check after metadata-only ALTER COLUMN TYPE#162122
Conversation
|
This change is |
|
(Hold off on reviewing, I need to do a little more testing.) |
|
Ok, this is RFAL. |
|
Hmm, actually, adding one more thing (sorry). |
|
I explored changing ALTER COLUMN TYPE to also update the column type in the histogram(s), but this ended up pretty messy, so never mind. This should be RFAL. |
ZhouXing19
left a comment
There was a problem hiding this comment.
@ZhouXing19 reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich).
|
Oh, Yahor also had a reminder to unskip |
4bb7276 to
ed9c9bb
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Nice!
@yuzefovich reviewed 6 files and all commit messages, and made 3 comments.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/distsql_stats line 3988 at r2 (raw file):
# Verify the histogram is being used before the ALTER. query T
nit: we probably should either add retry option or clear the table stats cache via the builtin to avoid flakes.
pkg/sql/types/types.go line 2447 at r2 (raw file):
// both types use identical encoding (both store time.Time), the histogram // remains valid. func (t *T) HasIdenticalHistogramEncoding(other *T) bool {
nit: it seems a bit off to me to have a method with "histogram" and "[binary] encoding" in its name in types package - personally, I'd move it probably to sql/stats/histogram.go file (it seems unlikely it'll be used anywhere else).
ed9c9bb to
ed8dde1
Compare
michae2
left a comment
There was a problem hiding this comment.
Thanks for the reviews!
/trunk merge
@michae2 made 3 comments and resolved 2 discussions.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/distsql_stats line 3988 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: we probably should either add
retryoption or clear the table stats cache via the builtin to avoid flakes.
Done.
pkg/sql/types/types.go line 2447 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: it seems a bit off to me to have a method with "histogram" and "[binary] encoding" in its name in
typespackage - personally, I'd move it probably tosql/stats/histogram.gofile (it seems unlikely it'll be used anywhere else).
Done.
This comment was marked as resolved.
This comment was marked as resolved.
ed8dde1 to
4284c5a
Compare
|
/trunk merge |
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
4284c5a to
838dbbe
Compare
|
/trunk merge |
…TYPE Previously, after a metadata-only ALTER COLUMN TYPE conversion (e.g., TIMESTAMPTZ to TIMESTAMP), histograms would fail a type check because `Equivalent()` requires types to be in the same family. This caused an internal error in test builds or histograms being silently skipped in production, leading to degraded query planning. This change adds a new `HasIdenticalHistogramEncoding()` method to `types.T` that returns true when two types have identical binary encodings for histogram bucket upper bounds. TIMESTAMP and TIMESTAMPTZ are the only cross-family types that share identical encoding (both store time.Time), so histograms remain valid after conversion between these types. The histogram `TypeCheck()` function now uses this method instead of `Equivalent()`, allowing histograms to be reused after metadata-only type conversions where the encoding doesn't change. Fixes: cockroachdb#125620 Epic: None Release note: None Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
838dbbe to
2bf1688
Compare
|
blathers backport 26.1 25.4 |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #125620: branch-release-25.4, branch-release-26.1. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, after a metadata-only ALTER COLUMN TYPE conversion (e.g., TIMESTAMPTZ to TIMESTAMP), histograms would fail a type check because
Equivalent()requires types to be in the same family. This caused an internal error in test builds or histograms being silently skipped in production, leading to degraded query planning.This change adds a new
HasIdenticalHistogramEncoding()method totypes.Tthat returns true when two types have identical binary encodings for histogram bucket upper bounds. TIMESTAMP and TIMESTAMPTZ are the only cross-family types that share identical encoding (both store time.Time), so histograms remain valid after conversion between these types.The histogram
TypeCheck()function now uses this method instead ofEquivalent(), allowing histograms to be reused after metadata-only type conversions where the encoding doesn't change.Fixes: #125620
Epic: None
Release note: None