Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-20.2: sql: add a hint for timestamptz -> string error #66479

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -307,6 +307,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 @@ -39,6 +39,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 @@ -164,7 +169,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 @@ -240,11 +248,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 @@ -27,7 +27,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 @@ -37,7 +37,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 @@ -409,26 +409,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 @@ -487,22 +508,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