sql: Add DateStyle/IntervalStyle visitor

[e-mbrown]

The DateStyle visitor allows for cast expressions with string
to interval and date/interval types to string cast to be rewritten.
These stable cast cause issues with DateStyle/IntervalStyle formatting so they
need to be wrapped in builtins containing their immutable version.

Release note: None
Release justification: Low risk update to new functionality

[cockroach-teamcity]

This change is 

[otan]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @otan)

---

pkg/sql/datestyle_visitor.go, line 13 at r1 (raw file):

)

type DateStyleVisitor struct {

can you add a comment about what this is?
also, does it have to be public? can we make this dateStyleVisitor / makeDateStyleVisitor if not.

edit: i see this also handles IntervalStyle below, maybe fixCastForStyleVisitor? something that makes it clear this handles the IntervalStyle as well.

---

pkg/sql/datestyle_visitor.go, line 35 at r1 (raw file):

	if expr, ok := expr.(*tree.CastExpr); ok {
		ctx := context.TODO()
		var semaCtx tree.SemaContext

can we pass semaCtx up from DateStyleVisitor? same with context

---

pkg/sql/datestyle_visitor.go, line 38 at r1 (raw file):

		desc := tabledesc.NewBuilder(v.desc)
		tDesc := desc.BuildImmutableTable()
		_, typ, _, err := schemaexpr.DequalifyAndValidateExpr(ctx, tDesc, expr, v.typ, "DateStyle visitor", &semaCtx, tree.VolatilityStable, tree.NewUnqualifiedTableName(tree.Name(v.desc.GetName())))

nit: line is a bit long. do you mind putting a newline between each arg, e.g.

, typ, _, err := schemaexpr.DequalifyAndValidateExpr(
   ctx, 
tDesc, 
expr,
 v.typ, 
"DateStyle visitor", 
&semaCtx, 
tree.VolatilityStable, 
tree.NewUnqualifiedTableName(tree.Name(v.desc.GetName())),
)

---

pkg/sql/datestyle_visitor.go, line 48 at r1 (raw file):

			newExpr = &tree.FuncExpr{Func: tree.WrapFunction("to_char"), Exprs: tree.Exprs{expr.Expr}}
			return newExpr
		case types.Interval:

i'm confused why Interval is involved in DateStyle visitor?

---

pkg/sql/datestyle_visitor_test.go, line 64 at r1 (raw file):

		},
		{
			expr:   "it::TEXT",

can you add a test for ::DATE, ::TIMETZ and ::TIMESTAMPTZ

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @otan)

---

pkg/sql/datestyle_visitor_test.go, line 91 at r1 (raw file):

	for _, test := range tests {
		visitor = sql.MakeDateStyleVisitor(nil, tDesc, test.typ)

let's wrap this around to make this a subtest:

for _, test := range tests {
   t.Run(test.expr, func(t *testing.T) {

      ...
   })
}

this makes tests appear as TestDateStyleVisitor/lower(it::STRING) so it's clearer what is broken.

---

pkg/sql/datestyle_visitor_test.go, line 93 at r1 (raw file):

		visitor = sql.MakeDateStyleVisitor(nil, tDesc, test.typ)
		expr, err := parser.ParseExpr(test.expr)
		if err != nil {

let's use require.NoError(t, err)

---

pkg/sql/datestyle_visitor_test.go, line 97 at r1 (raw file):

		}
		newExpr, _ := tree.WalkExpr(&visitor, expr)
		if newExpr.String() != test.expect {

let's use require.Equal(t, test.expect, newExpr.String())

[e-mbrown]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @otan)

---

pkg/sql/datestyle_visitor.go, line 13 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

can you add a comment about what this is?
also, does it have to be public? can we make this dateStyleVisitor / makeDateStyleVisitor if not.

edit: i see this also handles IntervalStyle below, maybe fixCastForStyleVisitor? something that makes it clear this handles the IntervalStyle as well.

Im not sure how to write up that comment, the struct is meant to store information passed from the Datestyle/IntervalStyle migration. Would commenting that be sufficient?

I think they do have to be public. The way the visitor/makeVisitor is used in the test file is similar to how it will be used in the migration.

---

pkg/sql/datestyle_visitor.go, line 35 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

can we pass semaCtx up from DateStyleVisitor? same with context

Yeah we can, i'll fix that.

---

pkg/sql/datestyle_visitor_test.go, line 64 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

can you add a test for ::DATE, ::TIMETZ and ::TIMESTAMPTZ

I actually have a question about that. I noticed that the string::date and other cast from strings are blocked in partial indexes and computed columns because they are context-dependent operators. Should we still test for them even though they may not be cases in the migration?

---

pkg/sql/datestyle_visitor_test.go, line 91 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

let's wrap this around to make this a subtest:

for _, test := range tests {
   t.Run(test.expr, func(t *testing.T) {

      ...
   })
}

this makes tests appear as TestDateStyleVisitor/lower(it::STRING) so it's clearer what is broken.

Done.

---

pkg/sql/datestyle_visitor_test.go, line 93 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

let's use require.NoError(t, err)

Done.

---

pkg/sql/datestyle_visitor_test.go, line 97 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

let's use require.Equal(t, test.expect, newExpr.String())

Done.

[otan]

lgtm after some minor fixes

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @otan)

---

pkg/sql/datestyle_visitor.go, line 13 at r1 (raw file):

Previously, e-mbrown wrote…

Im not sure how to write up that comment, the struct is meant to store information passed from the Datestyle/IntervalStyle migration. Would commenting that be sufficient?

I think they do have to be public. The way the visitor/makeVisitor is used in the test file is similar to how it will be used in the migration.

it'd be useful to explain what the visitor does in the comment. when i read it by name i wasn't aware it would actually change stuff.

they only have to be public if used outside the sql package. if it is then ok!

---

pkg/sql/datestyle_visitor_test.go, line 64 at r1 (raw file):

Previously, e-mbrown wrote…

I actually have a question about that. I noticed that the string::date and other cast from strings are blocked in partial indexes and computed columns because they are context-dependent operators. Should we still test for them even though they may not be cases in the migration?

if it already worked then we're good.

---

pkg/sql/datestyle_visitor_test.go, line 93 at r1 (raw file):

Previously, e-mbrown wrote…

Done.

you don't need the if err != nil anymore!

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

---

pkg/sql/fix_cast_for_style_visitor.go, line 32 at r2 (raw file):

func (v *FixCastForStyleVisitor) VisitPost(expr tree.Expr) tree.Expr {
	if v.err != nil {

it seems like v.err is never set anywhere - so i think we should start doing that

---

pkg/sql/fix_cast_for_style_visitor.go, line 46 at r2 (raw file):

			"DateStyle visitor",
			v.semaCtx,
			tree.VolatilityStable,

I thought we'd want to use VolatilityImmutable for this

but also, we need to be a bit careful since the volatility for the various casts changes if intervalstyle_enabled or datestyle_enabled is set to true

---

pkg/sql/fix_cast_for_style_visitor.go, line 49 at r2 (raw file):

			tree.NewUnqualifiedTableName(tree.Name(v.desc.GetName())),
		)
		if err != nil {

I believe the way this works is an error is returned if the expression is exceeds the given volatility

in that case, wouldn't we want to only do the rewrite if err != nil? (in other words, this should be changed to return early if err == nil)

---

pkg/sql/fix_cast_for_style_visitor.go, line 55 at r2 (raw file):

		var newExpr *tree.FuncExpr
		switch v := typ; v {
		case types.String:

we need to check the input type as well.

to_char should only be used if the input type is interval, timestamp, timestamptz, or date
parse_interval should only be used if the input type is a string

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 42 at r2 (raw file):

	if _, err := sqlDB.Exec(`
CREATE DATABASE t;
CREATE TABLE t.ds (it INTERVAL, s STRING, t TIMESTAMP, c1 INTERVAL AS ((s)::INTERVAL) STORED);

can you also add a TIMESTAMPTZ column and a DATE column? and make sure to test timestamptz::string and date::string

also, i think the c1 column is not needed by this test

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 60 at r2 (raw file):

			typ:    types.Interval,
			expect: "parse_interval(s)",
		},

can you add a test for s::TIMESTAMPTZ, s::TIMESTAMP, and s::DATE

i believe all of those should be rewritten (but again, let's check to see if postgres allows those first)

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

---

pkg/sql/datestyle_visitor_test.go, line 64 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

if it already worked then we're good.

oh just noticed this thread. some of my comments from earlier may not be needed then

but are we also already good with timestamptz->string and date->string casts?

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

---

pkg/sql/fix_cast_for_style_visitor.go, line 49 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

I believe the way this works is an error is returned if the expression is exceeds the given volatility

in that case, wouldn't we want to only do the rewrite if err != nil? (in other words, this should be changed to return early if err == nil)

wow good catch with this and above, my bad for above....

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

---

pkg/sql/datestyle_visitor_test.go, line 64 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

oh just noticed this thread. some of my comments from earlier may not be needed then

but are we also already good with timestamptz->string and date->string casts?

ah, also, this would be good information to add as a comment on the FixCastForStyleVisitor struct -- it should say that some of the casts were already blocked (and say which ones)

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

---

pkg/sql/fix_cast_for_style_visitor.go, line 32 at r9 (raw file):

// already blocked in computed columns and partial indexes. These casts are as
// follows: string::date, string::timestamp, string::timestamptz, string::time,
// timestamptz::string, string::timetz

is timestamptz::string affected? remember, we only are going to be checking the casts where datestyleAffected or intervalStyleAffected is true in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sem/tree/cast.go

---

pkg/sql/fix_cast_for_style_visitor.go, line 67 at r9 (raw file):

			expr,
			v.typ,
			"DateStyle visitor",

nit: change to "FixCastForStyleVisitor"

---

pkg/sql/fix_cast_for_style_visitor.go, line 74 at r9 (raw file):

		if err != nil {
			replacedExpr, err := schemaexpr.MakeDummyColForTypeCheck(v.ctx, tDesc, expr.Expr, tree.NewUnqualifiedTableName(tree.Name(v.desc.GetName())))
			if err != nil {

here an error is not expected, so this should set v.err = err

---

pkg/sql/fix_cast_for_style_visitor.go, line 78 at r9 (raw file):

			}
			typedExpr, err := tree.TypeCheck(v.ctx, replacedExpr, v.semaCtx, v.typ)
			if err != nil {

this should set v.err = err

---

pkg/sql/fix_cast_for_style_visitor.go, line 81 at r9 (raw file):

				return expr
			}
			innerTyp := typedExpr.ResolvedType()

to get the innerTyp, can you just do

innerTyp, err := schemaexpr.DequalifyAndValidateExpr(ctx, tDesc, expr.Expr, types.Any, "FixCastForStyleVisitor", v.semaCtx, tree.VolatilityStable, tree.NewUnqualifiedTableName(tree.Name(v.desc.GetName()))`
if err != nil {
  v.err = err
  return expr
}

(note that here we are passing in VolatilityStable. the visitor will still recurse and then fix any stable casts when it recurses into the inner expr later)

if this way works, then it means you don't have to add the new MakeDummyColForTypeCheck function

---

pkg/sql/fix_cast_for_style_visitor.go, line 83 at r9 (raw file):

			innerTyp := typedExpr.ResolvedType()

			var newExpr *tree.FuncExpr

nit: this should be: var newExpr tree.Expr

---

pkg/sql/fix_cast_for_style_visitor.go, line 89 at r9 (raw file):

					newExpr = &tree.FuncExpr{Func: tree.WrapFunction("parse_interval"), Exprs: tree.Exprs{expr.Expr}}
					return newExpr
				}

you need more cases for:

if v.typ.Family() == types.DateFamily {}
if v.typ.Family() == types.TimeFamily {}
if v.typ.Family() == types.TimestampFamily {}
if v.typ.Family() == types.TimeTZFamily {}

---

pkg/sql/fix_cast_for_style_visitor.go, line 90 at r9 (raw file):

					return newExpr
				}
			case types.IntervalFamily, types.DateFamily, types.TimestampFamily:

this should also include:

case types.IntervalFamily, types.DateFamily, types.TimestampFamily, types.TimeFamily, types.TimeTZFamily:

---

pkg/sql/fix_cast_for_style_visitor.go, line 92 at r9 (raw file):

			case types.IntervalFamily, types.DateFamily, types.TimestampFamily:
				if v.typ.Family() == types.StringFamily {
					newExpr = &tree.FuncExpr{Func: tree.WrapFunction("to_char"), Exprs: tree.Exprs{expr.Expr}}

this is a bit too broad, since the StringFamily includes a lot of different types like CHAR, VARCHAR, and more. i think this should be:

newExpr = &tree.CastExpr{
	Expr:       &tree.FuncExpr{Func: tree.WrapFunction("to_char"), Exprs: tree.Exprs{expr.Expr}},
	Type:       expr.Type,
	SyntaxMode: tree.CastShort,
}

that way, we still get the output type we need

---

pkg/sql/fix_cast_for_style_visitor.go, line 107 at r9 (raw file):

	desc *descpb.TableDescriptor,
	typ *types.T,
	err error,

you don't need to pass in err here. it should only be set internally

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 98 at r9 (raw file):

			expr:   "t::CHAR",
			typ:    types.String,
			expect: "to_char(t)",

this isn't quite right. we'd want the expected output to be to_char(t)::CHAR

the output of to_char is a string, but we need the output to be CHAR. i left a suggestion in the visitor for how to achieve this.

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 109 at r9 (raw file):

			typ:    types.String,
			expect: "lower(to_char(it))",
		},

add tests for the following:

time::string
timetz::string
date::string
timestamp::string

string::time
string::timetz
string::date
string::timestamp

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 120 at r9 (raw file):

			require.NoError(t, err)
			newExpr, _ := tree.WalkExpr(&visitor, expr)
			require.Equal(t, test.expect, newExpr.String())

nit: add require.NoError(t, visitor.err)

[rafiss]

looking closer! left a few comments

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

---

-- commits, line 7 at r9:
I think "wrap" is confusing because it implies that the original cast remains in the expression. In reality, the cast is replaced with the stable builtin entirely.

How about changing "need to be wrapped in builtins containing their immutable version" to "need to be replaced with builtins that are immutable versions of the cast"?

---

pkg/sql/catalog/schemaexpr/expr.go, line 29 at r9 (raw file):

)

// MakeDummyColForTypeCheck replaces the column variables with dummyColumns and returns an expr.

nit: comments should be no longer than 80 characters wide

---

pkg/sql/catalog/schemaexpr/expr.go, line 38 at r9 (raw file):

		*tn, colinfo.ResultColumnsFromColumns(desc.GetID(), nonDropColumns),
	)
	expr, err := dequalifyColumnRefs(ctx, sourceInfo, expr)

Why do we need to dequalify column references here? If we had tab.ts::STRING can we just replace that with to_char(tab.ts)?

---

pkg/sql/fix_cast_for_style_visitor.go, line 32 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it seems like v.err is never set anywhere - so i think we should start doing that

We'll need to actually check the value of v.err after visiting too. Here's an example where we create a helper function that returns the visitor's error:

cockroach/pkg/sql/catalog/schemaexpr/select_name_resolution.go

Line 112 in 49d5f5d

return expr, v.err

---

pkg/sql/fix_cast_for_style_visitor.go, line 23 at r9 (raw file):

)

// FixCastForStyleVisitor is used to rewrite cast expressions that contain cast

nit: "that contain cast" => "that contain casts"

---

pkg/sql/fix_cast_for_style_visitor.go, line 26 at r9 (raw file):

// that cause formatting issues when DateStyle/IntervalStyle is enabled. The
// issue is caused by the volatility of the cast being stable.
// FixCastForStyleVisitor detects these cast and wrap them in a builtin that

nit: "wrap them in a builtin that contains an immutable version of the cast" => "replace them with a builtin that is an immutable version of the cast". See my other comment about "wrap".

---

pkg/sql/fix_cast_for_style_visitor.go, line 29 at r9 (raw file):

// contains an immutable version of the cast. The visitor only checks for string
// to interval and various date/interval types to string cast. This is because
// cast that we thought would cause issues with DateStyle/IntervalStyle are

nit: "cast" => "casts"

---

pkg/sql/fix_cast_for_style_visitor.go, line 30 at r9 (raw file):

// to interval and various date/interval types to string cast. This is because
// cast that we thought would cause issues with DateStyle/IntervalStyle are
// already blocked in computed columns and partial indexes. These casts are as

nit: by "blocked" do you mean "disallowed"?

---

pkg/sql/fix_cast_for_style_visitor.go, line 32 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is timestamptz::string affected? remember, we only are going to be checking the casts where datestyleAffected or intervalStyleAffected is true in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sem/tree/cast.go

Should this list include string::interval and interval::string?

---

pkg/sql/fix_cast_for_style_visitor.go, line 86 at r9 (raw file):

			switch innerTyp.Family() {
			case types.StringFamily:
				if v.typ.Family() == types.IntervalFamily {

I think this should be the CastExpr's type family, expr.Type.Family(), not v.typ.Famiy(). Some test cases with nested casts might catch this. I've left a comment in the test file with some suggestions.

Don't we need to also rewrite casts string::date, string::timestamp, string::timestamptz, string::time, string::timetz, as you mention in the comment above?

---

pkg/sql/fix_cast_for_style_visitor.go, line 91 at r9 (raw file):

				}
			case types.IntervalFamily, types.DateFamily, types.TimestampFamily:
				if v.typ.Family() == types.StringFamily {

I think this should also be expr.Type.Family(), not v.typ.Family().

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 110 at r9 (raw file):

			expect: "lower(to_char(it))",
		},
	}

Can you add some tests where the calls in VisitPost to schemaexpr.MakeDummyColForTypeCheck and tree.TypeCheck return errors? I think you can instigate a failure in schemaexpr.MakeDummyColForTypeCheck with an expression that references a non-existent column. And I think you can instigate a failure in tree.TypeCheck with an expression like ('foo' + 1)::STRING. For these cases it might be helpful to add a comment above the test case describing its intention, since it may not be obvious to a future reader.

Can you also add some tests with nested casts and more complex expressions? This should highlight the potential bug I mentioned above about using v.Typ. Some ideas I had:

• s::TIMESTAMPTZ::STRING
• extract(epoch from t::STRING)

[e-mbrown]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

---

-- commits, line 7 at r9:

Previously, mgartner (Marcus Gartner) wrote…

I think "wrap" is confusing because it implies that the original cast remains in the expression. In reality, the cast is replaced with the stable builtin entirely.

How about changing "need to be wrapped in builtins containing their immutable version" to "need to be replaced with builtins that are immutable versions of the cast"?

Done.

---

pkg/sql/datestyle_visitor.go, line 13 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

it'd be useful to explain what the visitor does in the comment. when i read it by name i wasn't aware it would actually change stuff.

they only have to be public if used outside the sql package. if it is then ok!

Done.

---

pkg/sql/datestyle_visitor_test.go, line 93 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

you don't need the if err != nil anymore!

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 32 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

We'll need to actually check the value of v.err after visiting too. Here's an example where we create a helper function that returns the visitor's error:

cockroach/pkg/sql/catalog/schemaexpr/select_name_resolution.go

Line 112 in 49d5f5d

return expr, v.err

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 46 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

I thought we'd want to use VolatilityImmutable for this

but also, we need to be a bit careful since the volatility for the various casts changes if intervalstyle_enabled or datestyle_enabled is set to true

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 49 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

wow good catch with this and above, my bad for above....

Done

---

pkg/sql/fix_cast_for_style_visitor.go, line 55 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we need to check the input type as well.

to_char should only be used if the input type is interval, timestamp, timestamptz, or date
parse_interval should only be used if the input type is a string

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 74 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

here an error is not expected, so this should set v.err = err

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 81 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

to get the innerTyp, can you just do

innerTyp, err := schemaexpr.DequalifyAndValidateExpr(ctx, tDesc, expr.Expr, types.Any, "FixCastForStyleVisitor", v.semaCtx, tree.VolatilityStable, tree.NewUnqualifiedTableName(tree.Name(v.desc.GetName()))`
if err != nil {
  v.err = err
  return expr
}

(note that here we are passing in VolatilityStable. the visitor will still recurse and then fix any stable casts when it recurses into the inner expr later)

if this way works, then it means you don't have to add the new MakeDummyColForTypeCheck function

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 86 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think this should be the CastExpr's type family, expr.Type.Family(), not v.typ.Famiy(). Some test cases with nested casts might catch this. I've left a comment in the test file with some suggestions.

Don't we need to also rewrite casts string::date, string::timestamp, string::timestamptz, string::time, string::timetz, as you mention in the comment above?

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 89 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you need more cases for:

if v.typ.Family() == types.DateFamily {}
if v.typ.Family() == types.TimeFamily {}
if v.typ.Family() == types.TimestampFamily {}
if v.typ.Family() == types.TimeTZFamily {}

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 90 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this should also include:

case types.IntervalFamily, types.DateFamily, types.TimestampFamily, types.TimeFamily, types.TimeTZFamily:

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 91 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think this should also be expr.Type.Family(), not v.typ.Family().

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 92 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this is a bit too broad, since the StringFamily includes a lot of different types like CHAR, VARCHAR, and more. i think this should be:

newExpr = &tree.CastExpr{
	Expr:       &tree.FuncExpr{Func: tree.WrapFunction("to_char"), Exprs: tree.Exprs{expr.Expr}},
	Type:       expr.Type,
	SyntaxMode: tree.CastShort,
}

that way, we still get the output type we need

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 107 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you don't need to pass in err here. it should only be set internally

Done.

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 98 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this isn't quite right. we'd want the expected output to be to_char(t)::CHAR

the output of to_char is a string, but we need the output to be CHAR. i left a suggestion in the visitor for how to achieve this.

Done.

[mgartner]

Great progress!

Reviewed 1 of 1 files at r9, 1 of 3 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @otan, and @rafiss)

---

pkg/sql/fix_cast_for_style_visitor.go, line 125 at r11 (raw file):

					return newExpr
				}
				if v.typ.Family() == types.TimestampFamily {

Should this be innerType rather than v.typ?

---

pkg/sql/fix_cast_for_style_visitor.go, line 128 at r11 (raw file):

					newExpr = &tree.FuncExpr{Func: tree.WrapFunction("parse_timestamp"), Exprs: tree.Exprs{expr.Expr}}
					return newExpr
				}

nit: These ifs could be replaced with a switch statement.

---

pkg/sql/fix_cast_for_style_visitor.go, line 149 at r11 (raw file):

// MakeFixCastForStyleVisitor creates a FixCastForStyleVisitor instance.
func MakeFixCastForStyleVisitor(
	ctx context.Context, semaCtx *tree.SemaContext, desc *descpb.TableDescriptor, typ *types.T,

By requiring a type to be passed in, this can only replace a single type of cast. It would be nice if it replace all types of casts it supports, otherwise it'd have to run multiple times to completely convert expressions like it::TEXT = 'foo' OR it::VARCHAR = 'bar'.

[e-mbrown]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

---

pkg/sql/fix_cast_for_style_visitor.go, line 125 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Should this be innerType rather than v.typ?

Oop, it should be checking the 'outerTyp',

---

pkg/sql/fix_cast_for_style_visitor.go, line 149 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

By requiring a type to be passed in, this can only replace a single type of cast. It would be nice if it replace all types of casts it supports, otherwise it'd have to run multiple times to completely convert expressions like it::TEXT = 'foo' OR it::VARCHAR = 'bar'.

I see what you mean. I'll move the instance of DequalifyAndValidateExpr that detects if a cast is stable into the migration. That should negate the need for a type to be passed into the visitor.

[e-mbrown]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

---

pkg/sql/datestyle_visitor_test.go, line 64 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah, also, this would be good information to add as a comment on the FixCastForStyleVisitor struct -- it should say that some of the casts were already blocked (and say which ones)

Done.

---

pkg/sql/fix_cast_for_style_visitor.go, line 78 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this should set v.err = err

Done.

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 42 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you also add a TIMESTAMPTZ column and a DATE column? and make sure to test timestamptz::string and date::string

also, i think the c1 column is not needed by this test

Done.

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 60 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you add a test for s::TIMESTAMPTZ, s::TIMESTAMP, and s::DATE

i believe all of those should be rewritten (but again, let's check to see if postgres allows those first)

Done.

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 109 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

add tests for the following:

time::string
timetz::string
date::string
timestamp::string

string::time
string::timetz
string::date
string::timestamp

Done.

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 110 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you add some tests where the calls in VisitPost to schemaexpr.MakeDummyColForTypeCheck and tree.TypeCheck return errors? I think you can instigate a failure in schemaexpr.MakeDummyColForTypeCheck with an expression that references a non-existent column. And I think you can instigate a failure in tree.TypeCheck with an expression like ('foo' + 1)::STRING. For these cases it might be helpful to add a comment above the test case describing its intention, since it may not be obvious to a future reader.

Can you also add some tests with nested casts and more complex expressions? This should highlight the potential bug I mentioned above about using v.Typ. Some ideas I had:

• s::TIMESTAMPTZ::STRING
• extract(epoch from t::STRING)

Done.

[mgartner]

---

pkg/sql/fix_cast_for_style_visitor.go, line 149 at r11 (raw file):

Previously, e-mbrown wrote…

I see what you mean. I'll move the instance of DequalifyAndValidateExpr that detects if a cast is stable into the migration. That should negate the need for a type to be passed into the visitor.

I'm not sure I understand your comment. Why does moving logic to the migration help? Also, if you need to determine if a cast is stable, you can use tree.LookupCastVolatility.

[rafiss]

i had one idea for an optimization, and one more fix needed for an edge case. after that, it should be all good!

Dismissed @otan from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

---

pkg/sql/catalog/schemaexpr/expr.go, line 38 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Why do we need to dequalify column references here? If we had tab.ts::STRING can we just replace that with to_char(tab.ts)?

from my recollection, it seemed like the simplest way to get the type of tab.ts and check the volatility, all in one step

---

pkg/sql/fix_cast_for_style_visitor.go, line 149 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm not sure I understand your comment. Why does moving logic to the migration help? Also, if you need to determine if a cast is stable, you can use tree.LookupCastVolatility.

@mgartner an earlier draft of this was using tree.LookupCastVolatility. we changed to DequalifyAndValidateExpr since we need to get the *types.T, which we get from DequalifyAndValidateExpr, and that can already detect volatility anyway.

---

pkg/sql/fix_cast_for_style_visitor.go, line 46 at r13 (raw file):

		return false, expr
	}

as an optimization, you could throw in one more short-circuit here. check if the expr is immutable, and return false for recurse if so. e.g.

  if _, _, _, err := DequalifyAndValidateExpr(..., VolatilityImmutable); err == nil {
    // The expression is already immutable, so there's nothing for the visitor to change.
    return false, expr
  }

---

pkg/sql/fix_cast_for_style_visitor.go, line 61 at r13 (raw file):

		tDesc := desc.BuildImmutableTable()

		_, outerTyp, _, err := schemaexpr.DequalifyAndValidateExpr(

does this work if we just do:

outerTyp := expr.Type

---

pkg/sql/fix_cast_for_style_visitor.go, line 96 at r13 (raw file):

			switch outerTyp.Family() {
			case types.IntervalFamily:
				newExpr = &tree.FuncExpr{Func: tree.WrapFunction("parse_interval"), Exprs: tree.Exprs{expr.Expr}}

i just thought of one more edge case. intervals and timestamps can have "type modifiers" on them. so i think for all of these we want something similar to what we did in the to_char case below.

newExpr = &tree.CastExpr{
	Expr:       &tree.FuncExpr{Func: tree.WrapFunction("parse_interval"), Exprs: tree.Exprs{expr.Expr}},
	Type:       expr.Type,
	SyntaxMode: tree.CastShort,
}

that way the original type is preserved.

you should be able to test this using test cases like: str::time(5) and str::interval(4)

---

pkg/sql/fix_cast_for_style_visitor.go, line 134 at r13 (raw file):

//ResolveCastForStyleUsingVisitor checks expression for stable cast that affect
//DateStyle/IntervalStyle and rewrites them.

nit: add spaces after //

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 121 at r13 (raw file):

		},
		{
			expr:   "extract(epoch from d::CHAR)",

did you mean for this to be extract(epoch from s::DATE). i don't think passing a string is allowed for extract

root@:26257/defaultdb> select extract(epoch from 'cat'::STRING);
ERROR: unknown signature: extract(string, string)

[mgartner]

---

pkg/sql/fix_cast_for_style_visitor.go, line 149 at r11 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

@mgartner an earlier draft of this was using tree.LookupCastVolatility. we changed to DequalifyAndValidateExpr since we need to get the *types.T, which we get from DequalifyAndValidateExpr, and that can already detect volatility anyway.

I see. It's a bit odd to use the err of DequalifyAndValidateExpr to determine a cast's volatility, because the function can fail for other reasons (even though it shouldn't given that we'll be passing in an expression that is saved in a descriptor and already validated).

An alternative would be to type-check the expression, and walk the typed expression tree directly. That'd avoid the unnecessary work of calling DequalifyAndValidateExpr multiple times - and avoid the necessary work it performs to serialize the expression. You could move this logic to the schemaexpr package (I think it fits in well there) and do something like this:

expr, err := dequalifyColumnRefs(ctx, sourceInfo, expr)
if err != nil {
  return "", nil, colIDs, err
}

// Replace the column variables with dummyColumns so that they can be
// type-checked.
replacedExpr, colIDs, err := replaceColumnVars(desc, expr)
if err != nil {
  return "", nil, colIDs, err
}

typedExpr, err := tree.TypeCheck(ctx, expr, semaCtx, types.Any)
if err != nil {
  return nil, err
}

// Walk typeExpr and use the ResolvedType method at each expression
// to get its type.

This is how some other visitors work, like importDefaultExprVisitor .

[rafiss]

just a few small nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

---

pkg/sql/fix_cast_for_style_visitor.go, line 35 at r17 (raw file):

// is to account for the possibility these casts exist in an older cluster.
// These casts are as follows: string::date, string::timestamp, string::timestamptz, string::time,
// timestamptz::string, string::timetz

could you also mention in this comment that it must be called with a TypedExpr?

---

pkg/sql/fix_cast_for_style_visitor.go, line 36 at r17 (raw file):

// These casts are as follows: string::date, string::timestamp, string::timestamptz, string::time,
// timestamptz::string, string::timetz
type FixCastForStyleVisitor struct {

nit: this can be lower case i think right?

---

pkg/sql/fix_cast_for_style_visitor.go, line 40 at r17 (raw file):

	semaCtx *tree.SemaContext
	desc    *descpb.TableDescriptor
	tDesc   catalog.TableDescriptor

do we need both desc and tDesc? it seems like desc is only used to get the Name, but we can get that from tDesc also.

---

pkg/sql/fix_cast_for_style_visitor.go, line 87 at r17 (raw file):

		volatility, ok := tree.LookupCastVolatility(innerTyp, outerTyp, &sd)
		if !ok {
			v.err = errors.AssertionFailedf("Not a valid cast %s -> %s", innerExpr.String(), outerTyp.String())

nit: i think it should be "Not a valid cast %s -> %s", innerTyp.SQLString(), outerTyp.SQLString()

---

pkg/sql/fix_cast_for_style_visitor.go, line 105 at r17 (raw file):

				return newExpr
			case types.DateFamily:
				newExpr = &tree.FuncExpr{Func: tree.WrapFunction("parse_date"), Exprs: tree.Exprs{expr.Expr}}

nit: should be

				newExpr = &tree.CastExpr{
					Expr:       &tree.FuncExpr{Func: tree.WrapFunction("parse_date"), Exprs: tree.Exprs{expr.Expr}},
					Type:       expr.Type,
					SyntaxMode: tree.CastShort,
				}

---

pkg/sql/fix_cast_for_style_visitor.go, line 115 at r17 (raw file):

				return newExpr
			case types.TimeTZFamily:
				newExpr = &tree.FuncExpr{Func: tree.WrapFunction("parse_timetz"), Exprs: tree.Exprs{expr.Expr}}

nit: should use CastExpr, similar to above comment

---

pkg/sql/fix_cast_for_style_visitor.go, line 118 at r17 (raw file):

				return newExpr
			case types.TimestampFamily:
				newExpr = &tree.FuncExpr{Func: tree.WrapFunction("parse_timestamp"), Exprs: tree.Exprs{expr.Expr}}

nit: should use CastExpr, similar to above comment

---

pkg/sql/fix_cast_for_style_visitor.go, line 136 at r17 (raw file):

// MakeFixCastForStyleVisitor creates a FixCastForStyleVisitor instance.
func MakeFixCastForStyleVisitor(

nit: does this need to be public? you could make the ResolveCastForStyleUsingVisitor function create the visitor inside of that function

---

pkg/sql/fix_cast_for_style_visitor_test.go, line 52 at r17 (raw file):

	tests := []struct {
		expr   string
		typ    *types.T

nit: is this typ needed now?

[rafiss]

lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @mgartner, @otan, and @rafiss)

[e-mbrown]

bors r+

[craig]

Canceled.

[e-mbrown]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)