Skip to content

Commit

Permalink
Merge pull request #66478 from RaduBerinde/backport21.1-66454
Browse files Browse the repository at this point in the history
release-21.1: sql: add a hint for timestamptz -> string error
  • Loading branch information
RaduBerinde authored Jun 16, 2021
2 parents 72635e9 + be0d695 commit af2c90b
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 30 deletions.
14 changes: 14 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/computed
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,20 @@ CREATE TABLE y (
b TIMESTAMP AS (a::TIMESTAMP) STORED
)

# Make sure the error has a hint that mentions AT TIME ZONE.
statement error context-dependent operators are not allowed in computed column.*\nHINT:.*AT TIME ZONE
CREATE TABLE y (
a TIMESTAMPTZ,
b STRING AS (a::STRING) STORED
)

# Make sure the error has a hint that mentions AT TIME ZONE.
statement error context-dependent operators are not allowed in computed column.*\nHINT:.*AT TIME ZONE
CREATE TABLE y (
a TIMESTAMPTZ,
b TIMESTAMP AS (a::TIMESTAMP) STORED
)

statement error context-dependent operators are not allowed in computed column
CREATE TABLE y (
a TIMESTAMPTZ,
Expand Down
20 changes: 17 additions & 3 deletions pkg/sql/sem/tree/casts.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ type castInfo struct {
to types.Family
volatility Volatility

// volatilityHint is an optional string for VolatilityStable casts. When set,
// it is used as an error hint suggesting a possible workaround when stable
// casts are not allowed.
volatilityHint string

// Telemetry counter; set by init().
counter telemetry.Counter

Expand Down Expand Up @@ -170,7 +175,10 @@ var validCasts = []castInfo{
{from: types.GeographyFamily, to: types.StringFamily, volatility: VolatilityImmutable},
{from: types.BytesFamily, to: types.StringFamily, volatility: VolatilityStable},
{from: types.TimestampFamily, to: types.StringFamily, volatility: VolatilityImmutable},
{from: types.TimestampTZFamily, to: types.StringFamily, volatility: VolatilityStable},
{
from: types.TimestampTZFamily, to: types.StringFamily, volatility: VolatilityStable,
volatilityHint: "TIMESTAMPTZ to STRING casts depend on the current timezone; consider using (t AT TIME ZONE 'UTC')::STRING instead.",
},
{from: types.IntervalFamily, to: types.StringFamily, volatility: VolatilityImmutable},
{from: types.UuidFamily, to: types.StringFamily, volatility: VolatilityImmutable},
{from: types.DateFamily, to: types.StringFamily, volatility: VolatilityImmutable},
Expand Down Expand Up @@ -246,11 +254,17 @@ var validCasts = []castInfo{

// Casts to TimestampFamily.
{from: types.UnknownFamily, to: types.TimestampFamily, volatility: VolatilityImmutable},
{from: types.StringFamily, to: types.TimestampFamily, volatility: VolatilityStable},
{
from: types.StringFamily, to: types.TimestampFamily, volatility: VolatilityStable,
volatilityHint: "STRING to TIMESTAMP casts are context-dependent because of relative timestamp strings like 'now'; use parse_timestamp() instead.",
},
{from: types.CollatedStringFamily, to: types.TimestampFamily, volatility: VolatilityStable},
{from: types.DateFamily, to: types.TimestampFamily, volatility: VolatilityImmutable},
{from: types.TimestampFamily, to: types.TimestampFamily, volatility: VolatilityImmutable},
{from: types.TimestampTZFamily, to: types.TimestampFamily, volatility: VolatilityStable},
{
from: types.TimestampTZFamily, to: types.TimestampFamily, volatility: VolatilityStable,
volatilityHint: "TIMESTAMPTZ to TIMESTAMP casts depend on the current timezone; consider using AT TIME ZONE 'UTC' instead",
},
{from: types.IntFamily, to: types.TimestampFamily, volatility: VolatilityImmutable},

// Casts to TimestampTZFamily.
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/sem/tree/datum_invariants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func TestAllTypesCastableToString(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
for _, typ := range types.Scalar {
if ok, _, _ := isCastDeepValid(typ, types.String); !ok {
if err := resolveCast("", typ, types.String, true /* allowStable */); err != nil {
t.Errorf("%s is not castable to STRING, all types should be", typ)
}
}
Expand All @@ -32,7 +32,7 @@ func TestAllTypesCastableFromString(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
for _, typ := range types.Scalar {
if ok, _, _ := isCastDeepValid(types.String, typ); !ok {
if err := resolveCast("", types.String, typ, true /* allowStable */); err != nil {
t.Errorf("%s is not castable from STRING, all types should be", typ)
}
}
Expand Down
65 changes: 40 additions & 25 deletions pkg/sql/sem/tree/type_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,26 +416,47 @@ func (expr *CaseExpr) TypeCheck(
return expr, nil
}

func isCastDeepValid(castFrom, castTo *types.T) (bool, telemetry.Counter, Volatility) {
// resolveCast checks that the cast from the two types is valid. If allowStable
// is false, it also checks that the cast has VolatilityImmutable.
//
// On success, any relevant telemetry counters are incremented.
func resolveCast(context string, castFrom, castTo *types.T, allowStable bool) error {
toFamily := castTo.Family()
fromFamily := castFrom.Family()
switch {
case toFamily == types.ArrayFamily && fromFamily == types.ArrayFamily:
ok, c, v := isCastDeepValid(castFrom.ArrayContents(), castTo.ArrayContents())
if ok {
telemetry.Inc(sqltelemetry.ArrayCastCounter)
err := resolveCast(context, castFrom.ArrayContents(), castTo.ArrayContents(), allowStable)
if err != nil {
return err
}
return ok, c, v
telemetry.Inc(sqltelemetry.ArrayCastCounter)
return nil

case toFamily == types.EnumFamily && fromFamily == types.EnumFamily:
// Casts from ENUM to ENUM type can only succeed if the two enums
return castFrom.Equivalent(castTo), sqltelemetry.EnumCastCounter, VolatilityImmutable
}
// Casts from ENUM to ENUM type can only succeed if the two types are the
// same.
if !castFrom.Equivalent(castTo) {
return pgerror.Newf(pgcode.CannotCoerce, "invalid cast: %s -> %s", castFrom, castTo)
}
telemetry.Inc(sqltelemetry.EnumCastCounter)
return nil

cast := lookupCast(fromFamily, toFamily)
if cast == nil {
return false, nil, 0
default:
cast := lookupCast(fromFamily, toFamily)
if cast == nil {
return pgerror.Newf(pgcode.CannotCoerce, "invalid cast: %s -> %s", castFrom, castTo)
}
if !allowStable && cast.volatility >= VolatilityStable {
err := NewContextDependentOpsNotAllowedError(context)
err = pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s::%s", castFrom, castTo)
if cast.volatilityHint != "" {
err = errors.WithHint(err, cast.volatilityHint)
}
return err
}
telemetry.Inc(cast.counter)
return nil
}
return true, cast.counter, cast.volatility
}

func isEmptyArray(expr Expr) bool {
Expand Down Expand Up @@ -494,22 +515,16 @@ func (expr *CastExpr) TypeCheck(
}

castFrom := typedSubExpr.ResolvedType()

ok, c, volatility := isCastDeepValid(castFrom, exprType)
if !ok {
return nil, pgerror.Newf(pgcode.CannotCoerce, "invalid cast: %s -> %s", castFrom, exprType)
allowStable := true
context := ""
if semaCtx != nil && semaCtx.Properties.required.rejectFlags&RejectStableOperators != 0 {
allowStable = false
context = semaCtx.Properties.required.context
}
if err := semaCtx.checkVolatility(volatility); err != nil {
err = pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s::%s", castFrom, exprType)
// Special cases where we can provide useful hints.
if castFrom.Family() == types.StringFamily && exprType.Family() == types.TimestampFamily {
err = errors.WithHint(err, "string to timestamp casts are context-dependent because "+
"of relative timestamp strings like 'now'; use parse_timestamp() instead.")
}
err = resolveCast(context, castFrom, exprType, allowStable)
if err != nil {
return nil, err
}

telemetry.Inc(c)
expr.Expr = typedSubExpr
expr.Type = exprType
expr.typ = exprType
Expand Down

0 comments on commit af2c90b

Please sign in to comment.