opt: pass Const type explicitly #48652

RaduBerinde · 2020-05-10T02:18:41Z

The Const operator infers its type using Datum.ResolvedType(). The Datum's
type loses information in some cases (e.g. see #48563). We now pass the type
explicitly to ConstructConst, similar to the Null operator.

Since Const already had a private member, optgen needed to be modified to
tolerate a Typ field in this case (only allowed for Scalars). Note that this
field was already special - for any Scalar that doesn't have a Typ field, one
is added automatically and InferType is used to populate it.

Release note: None

cockroach-teamcity · 2020-05-10T02:18:48Z

This change is

jordanlewis

Nice. Did you check if this solves #48563?

jordanlewis · 2020-05-10T02:22:03Z

pkg/sql/opt/memo/testdata/stats/join

@@ -1425,7 +1425,7 @@ full-join (cross)
 expr
 (SemiJoin
    (Values
-      [ (Tuple [ (Const 1) (Const 2) ] "tuple{int}" ) ]
+      [ (Tuple [ (Const 1 "int") (Const 2 "int") ] "tuple{int}" ) ]


Could these be MakeIntConst?

No, there are no custom functions in exprgen

blathers-crl · 2020-05-10T02:55:56Z

❌ The GitHub CI (Cockroach) build has failed on 3eedae63.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

RaduBerinde · 2020-05-10T06:18:08Z

Hm, I haven't. I assumed we'd need some other fixes from your PR (maybe not?)

andy-kimball · 2020-05-10T18:06:33Z

pkg/sql/opt/xform/custom_funcs.go

@@ -1388,11 +1388,13 @@ func (c *CustomFuncs) GenerateLookupJoins(
 				constFilters = make(memo.FiltersExpr, 0, numIndexKeyCols-j)
 			}

+			foundValType := foundVal.ResolvedType()


This is probably not right, but no worse than it was before, so I guess we just leave for now...

Once we actually fix the typing, we should add a lint rule to the optimizer tree to not allow calling ResolvedType. It's just not safe if we care about OIDs.

andy-kimball · 2020-05-10T18:06:55Z

pkg/sql/opt/norm/custom_funcs.go

@@ -60,6 +60,11 @@ func (c *CustomFuncs) DerefOrderingChoice(result *physical.OrderingChoice) physi
 	return *result
 }

+// MakeIntConst constructs a Const holding a DInt.
+func (c *CustomFuncs) MakeIntConst(d *tree.DInt) opt.ScalarExpr {


Maybe just IntConst?

andy-kimball · 2020-05-10T18:06:59Z

pkg/sql/opt/norm/custom_funcs.go

@@ -2348,7 +2353,7 @@ func (c *CustomFuncs) CastToCollatedString(str opt.ScalarExpr, locale string) op
 	if err != nil {
 		panic(err)
 	}
-	return c.f.ConstructConst(d)
+	return c.f.ConstructConst(d, d.ResolvedType())


This isn't quite right, since you can have CollatedString{Char}, CollatedString{NVarChar}, etc, and they're different OIDs. I think should be:

return c.f.ConstructConst(d, types.MakeCollated(str.DataType(), locale)

This subtlety shows what we're up against. ResolvedType can't be relied upon anywhere if OID's matter, unless we're always careful to use DoidWrapper (but we're not careful). For us to truly fix the typing, we'd need to audit all of the optbuilder/optimizer code.

andy-kimball · 2020-05-10T18:07:14Z

pkg/sql/opt/norm/factory.go

@@ -340,5 +340,5 @@ func (f *Factory) ConstructConstVal(d tree.Datum, t *types.T) opt.ScalarExpr {
 		}
 		return memo.FalseSingleton
 	}
-	return f.ConstructConst(d)
+	return f.ConstructConst(d, t)


Should we just kill this method altogether, and then add a couple rules for converting (Const Null) to (Null), (Const True) to (True), etc?

The `Const` operator infers its type using `Datum.ResolvedType()`. The Datum's type loses information in some cases (e.g. see cockroachdb#48563). We now pass the type explicitly to `ConstructConst`, similar to the `Null` operator. Since Const already had a private member, optgen needed to be modified to tolerate a Typ field in this case (only allowed for Scalars). Note that this field was already special - for any Scalar that doesn't have a `Typ` field, one is added automatically and `InferType` is used to populate it. Release note: None

RaduBerinde

How can I check if this fixes #48563? Or @rafiss maybe you can check if you already have wireshark or whatnot set up?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jordanlewis)

pkg/sql/opt/norm/custom_funcs.go, line 64 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Maybe just IntConst?

Done.

pkg/sql/opt/norm/custom_funcs.go, line 2356 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

This isn't quite right, since you can have CollatedString{Char}, CollatedString{NVarChar}, etc, and they're different OIDs. I think should be:
return c.f.ConstructConst(d, types.MakeCollated(str.DataType(), locale)
This subtlety shows what we're up against. ResolvedType can't be relied upon anywhere if OID's matter, unless we're always careful to use DoidWrapper (but we're not careful). For us to truly fix the typing, we'd need to audit all of the optbuilder/optimizer code.

Done.

pkg/sql/opt/norm/factory.go, line 343 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Should we just kill this method altogether, and then add a couple rules for converting (Const Null) to (Null), (Const True) to (True), etc?

Not sure. I like it in principle, one downside is that it would be visible in the expressions in the optbuilder tests (build). They would probably also need to be "mandatory" rules because I assume some xform/exec code would break if we have a null Const.
Perhaps we do more generally want to be able to tag a subset of rules so that they are always applied, even in build mode and don't show up as a step during optsteps. A lot of simple scalar rules would be candidates.

pkg/sql/opt/xform/custom_funcs.go, line 1391 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

This is probably not right, but no worse than it was before, so I guess we just leave for now...

Once we actually fix the typing, we should add a lint rule to the optimizer tree to not allow calling ResolvedType. It's just not safe if we care about OIDs.

Yeah, I meant this PR to be used as a "base" on top of which to make some of these fixes (which I don't understand well enough yet). I fixed this one by using the type of the table column (though this column never exists outside of the lookup join so I don't think it matters in practice).

jordanlewis · 2020-05-10T18:31:58Z

This test should work, add it to pkg/sql/pgwire/testdata/pgtest/char:

send
Query {"String": "CREATE TABLE a (a INT PRIMARY KEY)"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"CREATE TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Query {"String": "INSERT INTO a VALUES(1)"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"INSERT 0 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

# Make sure that values casted to "char" get their type oid and type size
# reported correctly via pgwire.

send
Query {"String": "SELECT 'a'::\"char\" FROM a"}
----

until
ReadyForQuery
----
{"Type":"RowDescription","Fields":[{"Name":"char","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":18,"DataTypeSize":1,"TypeModifier":-1,"Format":0}]}
{"Type":"DataRow","Values":[{"text":"a"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

RaduBerinde · 2020-05-10T21:15:47Z

Thanks, I'll try it. But I think we do want the change to pass types to the exec factory for projections (instead of that code using ResolvedType) in any case.

jordanlewis · 2020-05-10T23:12:55Z

Oh yeah, I didn't realize you didn't include that one.

andy-kimball · 2020-05-11T00:12:58Z

@RaduBerinde, I may be mis-understanding, but I don't think we should set types on every Render. Instead, we should be setting types on the final result of the plan (somehow). The problem with render is that it just handles a subset of cases we care about. What if Union was the top-level statement, or anything else? Why is Render special?

RaduBerinde

I don't think it should be special. Every planNode should have ResultColumns with types that match the optimizer operators. We should audit all nodes, but for most of them it is already the case - for scans the types match the table column types, for most other operators the types are inherited from the input node(s). Projections are one of the very few places where we actually create new kinds of values (along with Values but we already pass the types for that one).

If we want to make doubly sure, we can make sure to always apply a final renderNode if the types don't match exactly (just like we do if the presentation doesn't match).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jordanlewis)

andy-kimball · 2020-05-11T04:38:21Z

How come it isn't sufficient to only apply the final renderNode? Why do the full static types matter for intermediate execution results?

RaduBerinde · 2020-05-11T05:35:19Z

I think it's technically sufficient, but why not make everything consistent? Also, the types produced by each node will show up in EXPLAIN (TYPES).

RaduBerinde · 2020-05-11T15:17:18Z

We should audit all nodes, but for most of them it is already the case - for scans the types match the table column types, for most other operators the types are inherited from the input node(s). Projections are one of the very few places where we actually create new kinds of values (along with Values but we already pass the types for that one).

I checked and the renderNode is the only usage of ResolvedExpr() in the exec factory.

This PR was intended as just an optgen improvement to allow the Type to be specified without needing a ConstPrivate. I will merge it as is and then we can rebase #48619 and discuss more there.

rytaft

Reviewed 18 of 23 files at r1, 7 of 7 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @jordanlewis)

RaduBerinde

bors r+

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @jordanlewis)

craig · 2020-05-13T03:23:29Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-05-13T04:37:31Z

Build succeeded

GitHub CI (Cockroach)

RaduBerinde requested review from jordanlewis and andy-kimball May 10, 2020 02:18

RaduBerinde requested a review from a team as a code owner May 10, 2020 02:18

jordanlewis reviewed May 10, 2020

View reviewed changes

andy-kimball reviewed May 10, 2020

View reviewed changes

RaduBerinde force-pushed the typed-const branch from 3eedae6 to 0c81947 Compare May 10, 2020 18:24

RaduBerinde commented May 10, 2020

View reviewed changes

RaduBerinde commented May 11, 2020

View reviewed changes

rytaft approved these changes May 13, 2020

View reviewed changes

RaduBerinde commented May 13, 2020

View reviewed changes

craig bot merged commit 3531318 into cockroachdb:master May 13, 2020

RaduBerinde deleted the typed-const branch May 13, 2020 16:15

rafiss mentioned this pull request May 20, 2020

release-20.1: opt: pass Const type explicitly #49331

Merged

jordanlewis mentioned this pull request May 21, 2020

release-20.1: sql: propagate opt types through renders #49353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: pass Const type explicitly #48652

opt: pass Const type explicitly #48652

RaduBerinde commented May 10, 2020

cockroach-teamcity commented May 10, 2020

jordanlewis left a comment

jordanlewis May 10, 2020

RaduBerinde May 10, 2020

blathers-crl bot commented May 10, 2020

RaduBerinde commented May 10, 2020

andy-kimball May 10, 2020

andy-kimball May 10, 2020

andy-kimball May 10, 2020

andy-kimball May 10, 2020

RaduBerinde left a comment

jordanlewis commented May 10, 2020

RaduBerinde commented May 10, 2020

jordanlewis commented May 10, 2020

andy-kimball commented May 11, 2020

RaduBerinde left a comment

andy-kimball commented May 11, 2020

RaduBerinde commented May 11, 2020

RaduBerinde commented May 11, 2020

rytaft left a comment

RaduBerinde left a comment

craig bot commented May 13, 2020

craig bot commented May 13, 2020

opt: pass Const type explicitly #48652

opt: pass Const type explicitly #48652

Conversation

RaduBerinde commented May 10, 2020

cockroach-teamcity commented May 10, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis May 10, 2020

Choose a reason for hiding this comment

RaduBerinde May 10, 2020

Choose a reason for hiding this comment

blathers-crl bot commented May 10, 2020

RaduBerinde commented May 10, 2020

andy-kimball May 10, 2020

Choose a reason for hiding this comment

andy-kimball May 10, 2020

Choose a reason for hiding this comment

andy-kimball May 10, 2020

Choose a reason for hiding this comment

andy-kimball May 10, 2020

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

jordanlewis commented May 10, 2020

RaduBerinde commented May 10, 2020

jordanlewis commented May 10, 2020

andy-kimball commented May 11, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

andy-kimball commented May 11, 2020

RaduBerinde commented May 11, 2020

RaduBerinde commented May 11, 2020

rytaft left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

craig bot commented May 13, 2020

Build failed (retrying...)

craig bot commented May 13, 2020

Build succeeded