sql: Allow DEFAULT/ON UPDATE types that can be assignment-cast #81071

e-mbrown · 2022-05-05T20:57:04Z

Resolves #74854

postgres:

postgres=# create table casts (b BOOL DEFAULT 'foo'::VARCHAR);
 ERROR:  column "b" is of type boolean but default expression is of type character varying>
HINT:  You will need to rewrite or cast the expression.

crdb:

[email protected]:26257/movr> CREATE TABLE t (b BOOL DEFAULT 'foo'::VARCHAR);
CREATE TABLE

Time: 8ms total (execution 8ms / network 0ms)

Release note (sql change): A column's DEFAULT/ON UPDATE clause
can now have a type that differs from the column type, as long as
that type can be assignment-cast into the column's type. This
increases our PostgreSQL compatibility.

cockroach-teamcity · 2022-05-05T20:57:14Z

This change is

e-mbrown · 2022-05-05T22:20:31Z

I noticed this particular solution causes us to support use cases that postgres does not. Not sure if we want to disallow these cases.

pkg/sql/logictest/testdata/logic_test/cast

mgartner

Great start!

mgartner · 2022-05-16T15:29:41Z

pkg/sql/catalog/tabledesc/table.go

-			return nil, err
+			// Check if the default expression type can be assignment-cast into the
+			// column type. If it can allow the default and column type to differ.
+			ret.DefaultExpr, innerErr = d.DefaultExpr.Expr.TypeCheck(ctx, semaCtx, types.Any)


schemaexpr.SanitizeVarFreeExpr above could return an error for reasons other than the expression not matching the column type. For example, it could fail if the expression contains references to other columns. We only want to check if the default expression type can be assignment-cast into the column type if schemaexpr.SanitizeVarFreeExpr fails during type-checking. Unfortunately, this isn't easy to do from outside schemaexpr.SanitizeVarFreeExpr. One option is to modify schemaexpr.SanitizeVarFreeExpr to add a new argument, allowAssignmentCast bool and move his assignment-cast logic into the function directly where type-checking fails.

I think you could add a test case for this too, like:

CREATE TABLE fail_assn_cast ( a BOOL, b STRING DEFAULT a )

mgartner · 2022-05-16T15:32:18Z

pkg/sql/logictest/testdata/logic_test/cast

@@ -1401,3 +1401,28 @@ select CASE WHEN false THEN 1::REGTYPE ELSE 2::REGNAMESPACE END;
 select CASE WHEN false THEN 1::REGTYPE ELSE 2::REGPROC END;
 select CASE WHEN false THEN 1::REGTYPE ELSE 2::REGPROCEDURE END;
 select CASE WHEN false THEN 1::REGTYPE ELSE 2::REGROLE END;
+
+
+# Test that default/on update expression differing from column type


nit: sentence seems incomplete

mgartner · 2022-05-16T15:32:21Z

pkg/sql/logictest/testdata/logic_test/cast

+# Test that default/on update expression differing from column type
+statement ok
+CREATE TABLE def_assn_cast (
+a INT4 DEFAULT 1.0::FLOAT4,


nit: indent the lines with column definitions here and below.

mgartner · 2022-05-16T15:32:46Z

pkg/sql/catalog/tabledesc/table.go

 		ret.OnUpdateExpr, err = schemaexpr.SanitizeVarFreeExpr(
 			ctx, d.OnUpdateExpr.Expr, resType, "ON UPDATE", semaCtx, volatility.Volatile,
 		)
 		if err != nil {
+			// Check if the on update expression type can be assignment-cast into the
+			// column type. If it can allow the on update expr and column type to differ.
+			ret.OnUpdateExpr, innerErr = d.OnUpdateExpr.Expr.TypeCheck(ctx, semaCtx, types.Any)


Can you add tests for ON UPDATE too?

mgartner · 2022-05-25T17:55:22Z

pkg/sql/opt/optbuilder/scope.go

 	expr = s.walkExprTree(expr)
-	texpr, err := tree.TypeCheckAndRequire(s.builder.ctx, expr, s.builder.semaCtx, desired, s.context.String())
+	texpr, err := tree.TypeCheckAndRequire(s.builder.ctx, expr, s.builder.semaCtx, desired, s.context.String(), true, mutSuffix)


I don't think we can allow assignment casts here in all cases. But, taking a step back, I'm curious why the optbuilder changes were necessary. What was failing before this change?

After making the table with differing column and default types, trying to insert into the table would result in an error. This was caused by the condition in TypeCheckRequire
if typ := typedExpr.ResolvedType(); !(typ.Equivalent(required) || typ.Family() == types.UnknownFamily) {
I changed the function in opt builder because I noticed the mutation suffix could be passed here to be used in TypeCheckRequire.
Would specifying that the mutation suffix has to be default or update narrow it down to only the cases we want?

Interesting. I'd expect the types to be fine given that assignment casts should be added in these cases by optbuilder automatically:

cockroach/pkg/sql/opt/optbuilder/insert.go

Line 661 in e55a7bf

mb.addAssignmentCasts(mb.insertColIDs)

I'll check out your branch and play around with it a bit to better understand what to do here. Thanks for being patient with me!

Ok, I think the change required in optbuilder was simpler, though tricky to figure out. I put up a PR: #81863. If you rebase your changes on that, you shouldn't need to make any changes in optbuilder.

That PR has been merged, so optbuilder should behave as expected if you rebase on the latest version of master.

mgartner · 2022-05-25T17:56:08Z

pkg/sql/catalog/tabledesc/table.go

@@ -168,7 +168,7 @@ func MakeColumnDefDescs(
 		// Verify the on update expression type is compatible with the column type
 		// and does not contain invalid functions.
 		ret.OnUpdateExpr, err = schemaexpr.SanitizeVarFreeExpr(
-			ctx, d.OnUpdateExpr.Expr, resType, "ON UPDATE", semaCtx, volatility.Volatile,
+			ctx, d.OnUpdateExpr.Expr, resType, "ON UPDATE", semaCtx, volatility.Volatile, true,


nit: It's helpful to label arguments with simple constants to make it more clear, like true /* allowAssignmentCast */.

pkg/sql/logictest/testdata/logic_test/cast

This commit allows the optimizer to build assignment casts for `DEFAULT` and `ON UPDATE` expressions that have types that are not equivalent with their column's type. If the cast from the `DEFAULT` or `ON UPDATE` expression type to the column type is not a valid assignment cast, an error is returned. In practice, this error should never occur because 1) it is currently impossible to create `DEFAULT` and `ON UPDATE` expressions that do not match their column's type (the tests here get around this limitation because they use the optimizer test catalog which has much looser restrictions) and 2) a future PR will allow creating `DEFAULT` and `ON UPDATE` expressions that do not match their column's type only if the cast from the expression type to the column type is a valid assignment cast. Unblocks cockroachdb#81071 Release note: None

81863: opt: assignment casts of non-equivalent types in DEFAULT and ON UPDATE r=mgartner a=mgartner This commit allows the optimizer to build assignment casts for `DEFAULT` and `ON UPDATE` expressions that have types that are not equivalent with their column's type. If the cast from the `DEFAULT` or `ON UPDATE` expression type to the column type is not a valid assignment cast, an error is returned. In practice, this error should never occur because 1) it is currently impossible to create `DEFAULT` and `ON UPDATE` expressions that do not match their column's type (the tests here get around this limitation because they use the optimizer test catalog which has much looser restrictions) and 2) a future PR will allow creating `DEFAULT` and `ON UPDATE` expressions that do not match their column's type only if the cast from the expression type to the column type is a valid assignment cast. Unblocks #81071 Release note: None Co-authored-by: Marcus Gartner <[email protected]>

rafiss

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

pkg/sql/catalog/tabledesc/table.go line 168 at r5 (raw file):

			ctx, d.DefaultExpr.Expr, resType, "DEFAULT", semaCtx, volatility.Volatile,
		)
		if err != nil {

i think it would be more readable if var innerErr error was here (near where the code where it's assigned), instead of having it earlier in the function

pkg/sql/catalog/tabledesc/table.go line 173 at r5 (raw file):

			ret.DefaultExpr, innerErr = d.DefaultExpr.Expr.TypeCheck(ctx, semaCtx, types.Any)
			if innerErr != nil {
				return nil, err

is it intentional to not return innerErr here? (i think maybe it is, but in that case, add a comment explaining why)

pkg/sql/catalog/tabledesc/table.go line 197 at r5 (raw file):

			ctx, d.OnUpdateExpr.Expr, resType, "ON UPDATE", semaCtx, volatility.Volatile,
		)
		if err != nil {

i think it would be more readable if var innerErr error was here (near where the code where it's assigned), instead of having it earlier in the function

pkg/sql/catalog/tabledesc/table.go line 202 at r5 (raw file):

			ret.OnUpdateExpr, innerErr = d.OnUpdateExpr.Expr.TypeCheck(ctx, semaCtx, types.Any)
			if innerErr != nil {
				return nil, err

nit: add a comment for why we don't return innerErr

pkg/sql/catalog/tabledesc/table.go line 184 at r6 (raw file):

		// and does not contain invalid functions.
		ret.OnUpdateExpr, err = schemaexpr.SanitizeVarFreeExpr(
			ctx, d.OnUpdateExpr.Expr, resType, "ON UPDATE", semaCtx, volatility.Volatile, true,

nit: add an inline comment here, and all the other places where it's called

SanitizeVarFreeExpr(..., true /* allowAssignmentCasts */)

pkg/sql/opt/optbuilder/scope.go line 480 at r6 (raw file):

// desired type.
func (s *scope) resolveAndRequireType(
	expr tree.Expr, desired *types.T, mutSuffix string,

is mutSuffix an accident? it doesn't seem to be used

pkg/sql/opt/optbuilder/scope.go

Release note (sql change): A column's DEFAULT/ON UPDATE clause can now have a type that differs from the column type,as long as that type can be assignment-cast into the column's type. This increases our PostgreSQL compatibility.

mgartner

LGTM. Very nice!

default/on update expr with differing types Release note: None

rafiss

lgtm!

Reviewed 14 of 17 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @e-mbrown, and @mgartner)

e-mbrown · 2022-06-09T18:18:59Z

bors r+

craig · 2022-06-09T19:40:53Z

Build succeeded:

GitHub CI (Cockroach)

e-mbrown requested review from mgartner and a team May 5, 2022 20:57

e-mbrown force-pushed the eb/defassigncast branch from bbdcd60 to fccdc69 Compare May 5, 2022 22:18

e-mbrown marked this pull request as ready for review May 5, 2022 22:19

e-mbrown requested a review from a team May 5, 2022 22:19

mgartner reviewed May 9, 2022

View reviewed changes

pkg/sql/logictest/testdata/logic_test/cast Outdated Show resolved Hide resolved

e-mbrown force-pushed the eb/defassigncast branch from fccdc69 to 017cc36 Compare May 16, 2022 14:20

mgartner requested changes May 16, 2022

View reviewed changes

e-mbrown requested review from a team as code owners May 17, 2022 19:39

e-mbrown requested review from ajwerner and removed request for a team May 17, 2022 19:39

e-mbrown force-pushed the eb/defassigncast branch 3 times, most recently from 016c1b0 to c14a307 Compare May 18, 2022 16:49

e-mbrown requested a review from mgartner May 18, 2022 18:21

mgartner reviewed May 25, 2022

View reviewed changes

pkg/sql/logictest/testdata/logic_test/cast Show resolved Hide resolved

mgartner mentioned this pull request May 25, 2022

opt: assignment casts of non-equivalent types in DEFAULT and ON UPDATE #81863

Merged

e-mbrown force-pushed the eb/defassigncast branch from c14a307 to e9962a8 Compare May 31, 2022 19:12

rafiss requested changes May 31, 2022

View reviewed changes

e-mbrown force-pushed the eb/defassigncast branch from e9962a8 to 31f6209 Compare June 1, 2022 15:45

e-mbrown force-pushed the eb/defassigncast branch from 31f6209 to 257f561 Compare June 1, 2022 19:20

e-mbrown requested review from mgartner and rafiss June 1, 2022 19:25

mgartner reviewed Jun 2, 2022

View reviewed changes

pkg/sql/opt/optbuilder/scope.go Outdated Show resolved Hide resolved

mgartner requested a review from a team June 2, 2022 16:40

e-mbrown force-pushed the eb/defassigncast branch 2 times, most recently from 67e73b1 to f042fae Compare June 6, 2022 20:25

e-mbrown force-pushed the eb/defassigncast branch from f042fae to e4c2cf1 Compare June 6, 2022 21:19

mgartner approved these changes Jun 6, 2022

View reviewed changes

sql: Ensure inserts can be performed on columns with

e4a01e0

default/on update expr with differing types Release note: None

e-mbrown force-pushed the eb/defassigncast branch from e4c2cf1 to e4a01e0 Compare June 7, 2022 20:44

rafiss approved these changes Jun 9, 2022

View reviewed changes

craig bot merged commit e09ce91 into cockroachdb:master Jun 9, 2022

cockroach-teamcity mentioned this pull request Jun 9, 2022

sql: Allow DEFAULT/ON UPDATE types that can be assignment-cast cockroachdb/docs#14246

Open

yuzefovich mentioned this pull request Aug 3, 2022

sql: Unexpected Error in SHOW COLUMNS #85394

Open

mgartner mentioned this pull request Mar 7, 2023

sqlsmith/randgen: generate DEFAULT expressions that don't match the column's type #98133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Allow DEFAULT/ON UPDATE types that can be assignment-cast #81071

sql: Allow DEFAULT/ON UPDATE types that can be assignment-cast #81071

e-mbrown commented May 5, 2022 •

edited

Loading

cockroach-teamcity commented May 5, 2022

e-mbrown commented May 5, 2022

mgartner left a comment

mgartner May 16, 2022

mgartner May 16, 2022

mgartner May 16, 2022

mgartner May 16, 2022

mgartner May 25, 2022

e-mbrown May 25, 2022

mgartner May 25, 2022

mgartner May 25, 2022

mgartner May 26, 2022

mgartner May 25, 2022

rafiss left a comment

mgartner left a comment

rafiss left a comment

e-mbrown commented Jun 9, 2022

craig bot commented Jun 9, 2022

sql: Allow DEFAULT/ON UPDATE types that can be assignment-cast #81071

sql: Allow DEFAULT/ON UPDATE types that can be assignment-cast #81071

Conversation

e-mbrown commented May 5, 2022 • edited Loading

cockroach-teamcity commented May 5, 2022

e-mbrown commented May 5, 2022

mgartner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

e-mbrown commented Jun 9, 2022

craig bot commented Jun 9, 2022

e-mbrown commented May 5, 2022 •

edited

Loading