diff --git a/pkg/sql/logictest/testdata/logic_test/timestamp b/pkg/sql/logictest/testdata/logic_test/timestamp index 1f6bd96fcfd4..42fbae58faeb 100644 --- a/pkg/sql/logictest/testdata/logic_test/timestamp +++ b/pkg/sql/logictest/testdata/logic_test/timestamp @@ -829,3 +829,36 @@ SELECT '-infinity'::timestamp + '1 second'::interval, '-infinity'::timestamp - ' -infinity -infinity subtest end + +# Regression test for #169202. The NormalizeCmpTimeZoneFunction rule must not +# rewrite timezone(zone, ts) op const into ts op timezone(zone, const) when +# the rewrite could hide a runtime "exceeds supported timestamp bounds" error +# that the original expression would have produced for boundary input rows. +subtest regression_normalize_cmp_timezone_bounds + +statement ok +CREATE TABLE timezone_bounds_t (ts TIMESTAMP) + +statement ok +INSERT INTO timezone_bounds_t VALUES ('294276-12-31 23:59:59.999999') + +# Sanity check: with the rule disabled, per-row evaluation errors as +# expected on the boundary row. +statement ok +SET disable_optimizer_rules = 'NormalizeCmpTimeZoneFunction' + +query error exceeds supported timestamp bounds +SELECT timezone('America/Denver', ts) >= '2020-06-01 12:35:55-07' FROM timezone_bounds_t + +statement ok +SET disable_optimizer_rules = '' + +# With the rule enabled (default), the rewrite must not silently mask +# the bounds error. +query error exceeds supported timestamp bounds +SELECT timezone('America/Denver', ts) >= '2020-06-01 12:35:55-07' FROM timezone_bounds_t + +statement ok +DROP TABLE timezone_bounds_t + +subtest end diff --git a/pkg/sql/opt/norm/comp_funcs.go b/pkg/sql/opt/norm/comp_funcs.go index 1ae4c7325653..30a863b3e5f3 100644 --- a/pkg/sql/opt/norm/comp_funcs.go +++ b/pkg/sql/opt/norm/comp_funcs.go @@ -6,12 +6,15 @@ package norm import ( + "time" + "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinsregistry" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" ) @@ -77,6 +80,61 @@ func (c *CustomFuncs) NormalizeTupleEquality(left, right memo.ScalarListExpr) op return result } +// CanTimeZoneFunctionOverflow returns true if the NormalizeCmpTimeZoneFunction +// or NormalizeCmpTimeZoneFunctionTZ rewrite could mask a runtime +// "exceeds supported timestamp bounds" error. The rewrite hoists a per-row +// timezone(zone, ts) call out of a comparison; if any value in the column +// domain would push the timezone-shifted result past the supported timestamp +// bounds, the original expression would have errored at evaluation while the +// rewritten predicate would not. The rewrite is only safe when applying the +// timezone shift to both MinSupportedTime and MaxSupportedTime succeeds, which +// is a sufficient condition that no value in the column can trigger the +// bounds error. +// +// Returns true (rewrite is unsafe) if zone is not a constant string, or if +// applying the timezone shift to either supported-time bound errors. +func (c *CustomFuncs) CanTimeZoneFunctionOverflow(zone, ts opt.ScalarExpr) bool { + zoneConst, ok := zone.(*memo.ConstExpr) + if !ok { + return true + } + zoneStr, ok := zoneConst.Value.(*tree.DString) + if !ok { + return true + } + loc, err := timeutil.TimeZoneStringToLocation( + string(*zoneStr), timeutil.TimeZoneStringToLocationPOSIXStandard, + ) + if err != nil { + return true + } + // The rule normalizes timezone(zone, ts) where ts is of type TIMESTAMP or + // TIMESTAMPTZ. The timezone() overload chosen by MakeTimeZoneFunction + // determines whether the shift is AddTimeZone (TIMESTAMP -> TIMESTAMPTZ) + // or EvalAtAndRemoveTimeZone (TIMESTAMPTZ -> TIMESTAMP). Either direction + // is an additive shift that can push a boundary value out of bounds. + minTS := &tree.DTimestamp{Time: tree.MinSupportedTime} + maxTS := &tree.DTimestamp{Time: tree.MaxSupportedTime} + if ts.DataType().Family() == types.TimestampFamily { + if _, err := minTS.AddTimeZone(loc, time.Microsecond); err != nil { + return true + } + if _, err := maxTS.AddTimeZone(loc, time.Microsecond); err != nil { + return true + } + return false + } + minTZ := &tree.DTimestampTZ{Time: tree.MinSupportedTime} + maxTZ := &tree.DTimestampTZ{Time: tree.MaxSupportedTime} + if _, err := minTZ.EvalAtAndRemoveTimeZone(loc, time.Microsecond); err != nil { + return true + } + if _, err := maxTZ.EvalAtAndRemoveTimeZone(loc, time.Microsecond); err != nil { + return true + } + return false +} + // MakeTimeZoneFunction constructs a new timezone() function with the given zone // and timestamp as arguments. The type of the function result is TIMESTAMPTZ if // ts is of type TIMESTAMP, or TIMESTAMP if is of type TIMESTAMPTZ. diff --git a/pkg/sql/opt/norm/rules/comp.opt b/pkg/sql/opt/norm/rules/comp.opt index aa4b4fe0fe22..10f1d7f2e9ed 100644 --- a/pkg/sql/opt/norm/rules/comp.opt +++ b/pkg/sql/opt/norm/rules/comp.opt @@ -206,6 +206,10 @@ # 1. The left side of the comparison is a timezone() function. # 2. The second argument to timezone() is a variable of type TIMESTAMP. # 3. The right side of the comparison is a constant value TIMESTAMPTZ. +# 4. Applying the zone shift to the supported timestamp bounds does not +# overflow. This ensures the rewrite cannot mask a per-row +# "exceeds supported timestamp bounds" error that the original +# expression would have produced (see #169202). # # Here's an example: # @@ -221,7 +225,8 @@ $right:(ConstValue) & (IsTimestampTZ $right) & (Let ($zone $ts $ok):(ScalarPair $args) $ok) & - (IsTimestamp $ts) + (IsTimestamp $ts) & + ^(CanTimeZoneFunctionOverflow $zone $ts) ) => ((OpName) $ts (MakeTimeZoneFunction $zone $right)) @@ -232,6 +237,10 @@ # 1. The left side of the comparison is a timezone() function. # 2. The second argument to timezone() is a variable of type TIMESTAMPTZ. # 3. The right side of the comparison is a constant value TIMESTAMP. +# 4. Applying the zone shift to the supported timestamp bounds does not +# overflow. This ensures the rewrite cannot mask a per-row +# "exceeds supported timestamp bounds" error that the original +# expression would have produced (see #169202). # # Here's an example: # @@ -248,7 +257,8 @@ (IsTimestamp $right) & (Let ($zone $tz $ok):(ScalarPair $args) $ok) & (IsTimestampTZ $tz) & - ^(IsConstValueOrGroupOfConstValues $tz) + ^(IsConstValueOrGroupOfConstValues $tz) & + ^(CanTimeZoneFunctionOverflow $zone $tz) ) => ((OpName) $tz (MakeTimeZoneFunction $zone $right))