sql/opt: prevent NormalizeCmpTimeZoneFunction from masking bounds errors#169733
Open
SAY-5 wants to merge 1 commit intocockroachdb:masterfrom
Open
sql/opt: prevent NormalizeCmpTimeZoneFunction from masking bounds errors#169733SAY-5 wants to merge 1 commit intocockroachdb:masterfrom
SAY-5 wants to merge 1 commit intocockroachdb:masterfrom
Conversation
NormalizeCmpTimeZoneFunction and NormalizeCmpTimeZoneFunctionTZ rewrite predicates of the form `timezone(zone, ts) op const` into `ts op timezone(zone, const)`, hoisting the per-row timezone() call out of the comparison. When a column value is at or near the supported timestamp bounds (294276-12-31 23:59:59.999999 / 4714-11-24 BC), the per-row timezone() call would return an "exceeds supported timestamp bounds" error, but the rewritten predicate evaluates the constant comparison directly and silently returns a result the original expression could not have produced. Add a CanTimeZoneFunctionOverflow guard that requires both MinSupportedTime and MaxSupportedTime to round-trip through the chosen timezone shift without erroring. This is a sufficient condition that no value in the column domain can trigger the bounds error, preserving the original expression's error semantics. The guard also declines to fire when the zone is not a constant string, since we cannot prove safety statically in that case. Includes a logictest regression that fails on master and passes with this change. Same class of bug as cockroachdb#125751 / cockroachdb#88199 (predicate rewrites that ignore runtime evaluation errors), for a different rule and a different error class. Fixes cockroachdb#169202 Release note (bug fix): Fixed a bug where comparisons of the form `timezone(zone, ts) op const` could silently return results that the original expression could not have produced when the per-row timezone() evaluation would have errored with "exceeds supported timestamp bounds". Signed-off-by: SAY-5 <[email protected]>
Contributor
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
SAY-5 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Member
|
This change is |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #169202.
NormalizeCmpTimeZoneFunctionandNormalizeCmpTimeZoneFunctionTZrewrite predicates of the formtimezone(zone, ts) op constintots op timezone(zone, const), hoisting the per-rowtimezone()call out of the comparison. When a column value is at or near the supported timestamp bounds, the per-row call would return anexceeds supported timestamp boundserror, but the rewritten predicate evaluates the constant comparison directly and silently returns a result the original expression could not have produced.This change adds a
CanTimeZoneFunctionOverflowguard that requires bothMinSupportedTimeandMaxSupportedTimeto round-trip through the chosen timezone shift without erroring, which is a sufficient condition that no value in the column domain can trigger the bounds error. The guard also declines when the zone is non-constant.Same class of bug as #125751 / #88199.
Release note (bug fix): Fixed a bug where comparisons of the form
timezone(zone, ts) op constcould silently return results that the original expression could not have produced when the per-rowtimezone()evaluation would have errored with "exceeds supported timestamp bounds".