sql: add ReType to resolveAndRequireType to fix vector engine panic #66885

cucaroach · 2021-06-25T15:49:51Z

The vector engine needs exact type coercion, specifically when piping
computed column values into downstream operators. Without this fix
the computed column was left as an int64 instead of cast back to the
required int16 type.

Exposed by sqlsmith, kudos to @michae2 for the reduce help

Release note: None

cockroach-teamcity · 2021-06-25T15:49:58Z

This change is

cucaroach · 2021-06-25T17:44:05Z

There's some CI failures here that bring to light the shortcomings of my "fix". @RaduBerinde whats the best way to bring those to your attention, should I just rewrite them and add them to the PR so we can discuss?

RaduBerinde

Yes, this is the standard procedure - rewrite all tests so the changes can be reviewed.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach)

pkg/sql/opt/optbuilder/select.go, line 711 at r1 (raw file):

			})
			// Paper over the problem, there's probably a better place to put this...
			if scalar.DataType() != tabCol.DatumType() {

I would call tree.ReType() on texpr before buildScalar instead.

@rytaft it feels that resolveAndRequireType should do this internally, what do you think?

cucaroach

👍

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)

pkg/sql/opt/optbuilder/select.go, line 711 at r1 (raw file):

Previously, RaduBerinde wrote…

I would call tree.ReType() on texpr before buildScalar instead.

@rytaft it feels that resolveAndRequireType should do this internally, what do you think?

That sounds better. resolveAndRequireType sure doesn't seem to be living up to the "Require" bit. I guess I could try that and see what the fallout looks like.

cucaroach

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)

pkg/sql/opt/optbuilder/select.go, line 711 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

That sounds better. resolveAndRequireType sure doesn't seem to be living up to the "Require" bit. I guess I could try that and see what the fallout looks like.

FYI, with ReType instead of my fix the fallout is diminished.

RaduBerinde

LGTM but I think it's a good idea to try adding ReType to resolveAndRequireType and see what else changes.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)

pkg/sql/opt/optbuilder/select.go, line 711 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

FYI, with ReType instead of my fix the fallout is diminished.

The != instead of using Identical() was likely causing some unneeded casts.

cucaroach

RFAL, I pushed the ReType down into resolveAndRequireType and updated the testdata that was affected. Seems okay? Not sure if any of these casts could/would hurt performance. At a high level I think ReType'ing doesn't belong in computed column specific code and it should be handled at a deeper level. At an even higher level I think our execution engine should have a "verifier" to ensure runtime mismatches like this can never happen, not sure what this would look like in practice but something to think about.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)

cucaroach

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)

pkg/sql/opt/optbuilder/select.go, line 711 at r1 (raw file):

Previously, RaduBerinde wrote…

The != instead of using Identical() was likely causing some unneeded casts.

👍

RaduBerinde

Yes, let's keep the Retype in resolveAndRequireType.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

@michae2

Fixes cockroachdb#66708 The vector engine needs exact type coercion, specifically when piping computed column values into downstream operators. Without this fix the computed column was left as an int64 instead of cast back to the required int16 type. Exposed by sqlsmith, kudos to @michae2 for the review help Release note: None

RaduBerinde · 2021-06-28T17:04:09Z

I think this should be backported; adding the labels.

cucaroach

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

pkg/sql/opt/optbuilder/scope.go, line 461 at r5 (raw file):

		panic(err)
	}
	return tree.ReType(s.ensureNullType(texpr, desired), desired)

Is this okay or should I break it up into two lines?

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

pkg/sql/opt/optbuilder/scope.go, line 461 at r5 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Is this okay or should I break it up into two lines?

We try to keep things at most 100 columns in most cases (see https://wiki.crdb.io/wiki/spaces/CRDB/pages/181371303/Go+Golang+coding+guidelines), other than that it's up to you.

Or "rule" is that we try not to comment too much on formatting nits in PRs; if something is very important, it should be made into a linting test and then it never needs to be discussed.

cucaroach · 2021-06-28T18:35:53Z

I think this should be backported; adding the labels.

Near as I could tell this only affects 21.1 so removing 20.2 label (CRDB needs to support virtual computed columns).

cucaroach · 2021-06-28T18:37:07Z

bors r+

RaduBerinde · 2021-06-28T18:40:53Z

The lack of a cast could affect STORED columns as well.

RaduBerinde · 2021-06-28T19:26:08Z

The lack of a cast could affect STORED columns as well.

I can't come up with a case with a STORED column that is problematic, so I agree we can leave out of 20.2.

craig · 2021-06-28T19:29:53Z

Build succeeded:

GitHub CI (Cockroach)

RaduBerinde · 2021-06-28T19:53:58Z

pkg/sql/opt/xform/testdata/external/trading-mutation, line 1605 at r5 (raw file):

      │              └── q:37
      └── projections
           ├── crdb_internal.round_decimal_values((buyprice:23::DECIMAL - discount:25::DECIMAL)::DECIMAL(10,4), 4) [as=discountbuyprice_comp:53, outer=(23,25), immutable]

Hm, this is giving me second thoughts. If we cast to DECIMAL(10,4), are we missing some better rounding that round_decimal_values does? If not, does this mean we can just rely on casts and get rid of the special round_decimal_values stuff in optbuilder? CC @rytaft @andy-kimball

cucaroach

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index, line 31 at r5 (raw file):

        │ columns: (crdb_internal_a_shard_11_comp, column1)
        │ estimated row count: 2
        │ render crdb_internal_a_shard_11_comp: mod(fnv32(COALESCE(column1::STRING, '')), 11)::INT4

Are these changes right/okay? Isn't "int" in sql supposed to be a 64-bit int?

pkg/sql/opt/xform/testdata/external/trading-mutation, line 1605 at r5 (raw file):

Previously, RaduBerinde wrote…

Hm, this is giving me second thoughts. If we cast to DECIMAL(10,4), are we missing some better rounding that round_decimal_values does? If not, does this mean we can just rely on casts and get rid of the special round_decimal_values stuff in optbuilder? CC @rytaft @andy-kimball

I'll look into it, it does seem redundant.

RaduBerinde · 2021-06-28T20:19:57Z

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index, line 31 at r5 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Are these changes right/okay? Isn't "int" in sql supposed to be a 64-bit int?

It's good, the hidden shard column is INT4 (we don't support BUCKET_COUNT > 2^31)

rytaft

Reviewed 2 of 2 files at r4, 6 of 6 files at r5.
Reviewable status: complete! 2 of 0 LGTMs obtained

pkg/sql/opt/optbuilder/scope.go, line 461 at r5 (raw file):

Previously, RaduBerinde wrote…

We try to keep things at most 100 columns in most cases (see https://wiki.crdb.io/wiki/spaces/CRDB/pages/181371303/Go+Golang+coding+guidelines), other than that it's up to you.

Or "rule" is that we try not to comment too much on formatting nits in PRs; if something is very important, it should be made into a linting test and then it never needs to be discussed.

Nice clean change!

pkg/sql/opt/xform/testdata/external/trading-mutation, line 1605 at r5 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

I'll look into it, it does seem redundant.

Good question -- I'm not sure off the top of my head. @andy-kimball was the one who did the rounding work.

pkg/sql/opt/optbuilder/select.go, line 711 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

:+!:

Sorry for the delay -- resolveAndRequireType looks like the right place.

cucaroach

Reviewable status: complete! 2 of 0 LGTMs obtained

pkg/sql/opt/xform/testdata/external/trading-mutation, line 1605 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Good question -- I'm not sure off the top of my head. @andy-kimball was the one who did the rounding work.

Hmm, so this is a stored computed column and this is an update query on a unrelated field, I think this has no material affect at runtime. However simple update query on a one of the fields in the computed column reveals that my change does introduce an extra cast operator into the vec plans. Could that be a problem?

rytaft · 2021-06-30T14:19:01Z

pkg/sql/opt/xform/testdata/external/trading-mutation, line 1605 at r5 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Hmm, so this is a stored computed column and this is an update query on a unrelated field, I think this has no material affect at runtime. However simple update query on a one of the fields in the computed column reveals that my change does introduce an extra cast operator into the vec plans. Could that be a problem?

I don't think an extra cast operator is a big problem, but we could avoid it by adding a normalization rule similar to the existing EliminateCast rule that eliminates the round_decimal_values function if it's redundant.

But @RaduBerinde does bring up a good question about whether we could get rid of the round_decimal_values stuff altogether in the optbuilder... I experimented with commenting out that logic, and it does seem like it might still be necessary in general. But maybe we can get rid of the special case for computed columns, and then a normalization rule would be unnecessary?

A separate question is whether we should actually be using round_decimal_values instead of ReType in some cases inside resolveAndRequireType. Poking around and experimenting a bit, it seems like under the covers round_decimal_values is doing the same thing as the cast, so I think it's fine as is. But I'm not 100% sure...

RaduBerinde · 2021-06-30T18:55:39Z

pkg/sql/opt/xform/testdata/external/trading-mutation, line 1605 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I don't think an extra cast operator is a big problem, but we could avoid it by adding a normalization rule similar to the existing EliminateCast rule that eliminates the round_decimal_values function if it's redundant.

But @RaduBerinde does bring up a good question about whether we could get rid of the round_decimal_values stuff altogether in the optbuilder... I experimented with commenting out that logic, and it does seem like it might still be necessary in general. But maybe we can get rid of the special case for computed columns, and then a normalization rule would be unnecessary?

A separate question is whether we should actually be using round_decimal_values instead of ReType in some cases inside resolveAndRequireType. Poking around and experimenting a bit, it seems like under the covers round_decimal_values is doing the same thing as the cast, so I think it's fine as is. But I'm not 100% sure...

Filed #67083 to investigate more.

cucaroach requested review from RaduBerinde and a team June 25, 2021 15:49

cucaroach linked an issue Jun 25, 2021 that may be closed by this pull request

roachtest: sqlsmith/setup=rand-tables/setting=no-ddl failed #66708

Closed

RaduBerinde reviewed Jun 25, 2021

View reviewed changes

cucaroach force-pushed the comp-col-typ-fix branch from 6de5c6c to 031c5b1 Compare June 25, 2021 21:02

cucaroach commented Jun 25, 2021

View reviewed changes

cucaroach force-pushed the comp-col-typ-fix branch from 031c5b1 to 070380e Compare June 25, 2021 22:45

cucaroach commented Jun 25, 2021

View reviewed changes

RaduBerinde reviewed Jun 25, 2021

View reviewed changes

cucaroach force-pushed the comp-col-typ-fix branch from 9ac2b9a to 0d6e00e Compare June 28, 2021 14:46

cucaroach commented Jun 28, 2021

View reviewed changes

RaduBerinde approved these changes Jun 28, 2021

View reviewed changes

cucaroach force-pushed the comp-col-typ-fix branch from 0d6e00e to 394c407 Compare June 28, 2021 16:51

cucaroach marked this pull request as ready for review June 28, 2021 16:52

cucaroach changed the title ~~sql: WIP: add cast to massage computed col type to desired~~ sql: add ReType to resolveAndRequireType to fix vector engine panic Jun 28, 2021

cucaroach requested a review from a team June 28, 2021 16:57

RaduBerinde added backport-20.2.x labels Jun 28, 2021

cucaroach commented Jun 28, 2021

View reviewed changes

RaduBerinde reviewed Jun 28, 2021

View reviewed changes

cucaroach removed the backport-20.2.x label Jun 28, 2021

craig bot merged commit 858fc12 into cockroachdb:master Jun 28, 2021

RaduBerinde mentioned this pull request Jun 28, 2021

release-21.1: sql: add ReType to resolveAndRequireType to fix vector engine panic #66986

Closed

cucaroach commented Jun 28, 2021

View reviewed changes

rytaft reviewed Jun 28, 2021

View reviewed changes

cucaroach commented Jun 30, 2021

View reviewed changes

RaduBerinde mentioned this pull request Jun 30, 2021

opt: revisit use of round_decimal_values in optbuilder #67083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add ReType to resolveAndRequireType to fix vector engine panic #66885

sql: add ReType to resolveAndRequireType to fix vector engine panic #66885

cucaroach commented Jun 25, 2021 •

edited

Loading

cockroach-teamcity commented Jun 25, 2021

cucaroach commented Jun 25, 2021

RaduBerinde left a comment

cucaroach left a comment

cucaroach left a comment

RaduBerinde left a comment

cucaroach left a comment

cucaroach left a comment •

edited

Loading

RaduBerinde left a comment

RaduBerinde commented Jun 28, 2021

cucaroach left a comment

RaduBerinde left a comment

cucaroach commented Jun 28, 2021

cucaroach commented Jun 28, 2021

RaduBerinde commented Jun 28, 2021

RaduBerinde commented Jun 28, 2021

craig bot commented Jun 28, 2021

RaduBerinde commented Jun 28, 2021

cucaroach left a comment

RaduBerinde commented Jun 28, 2021

rytaft left a comment

cucaroach left a comment

rytaft commented Jun 30, 2021

RaduBerinde commented Jun 30, 2021

sql: add ReType to resolveAndRequireType to fix vector engine panic #66885

sql: add ReType to resolveAndRequireType to fix vector engine panic #66885

Conversation

cucaroach commented Jun 25, 2021 • edited Loading

cockroach-teamcity commented Jun 25, 2021

cucaroach commented Jun 25, 2021

RaduBerinde left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

cucaroach left a comment • edited Loading

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde commented Jun 28, 2021

cucaroach left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

cucaroach commented Jun 28, 2021

cucaroach commented Jun 28, 2021

RaduBerinde commented Jun 28, 2021

RaduBerinde commented Jun 28, 2021

craig bot commented Jun 28, 2021

RaduBerinde commented Jun 28, 2021

cucaroach left a comment

Choose a reason for hiding this comment

RaduBerinde commented Jun 28, 2021

rytaft left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

rytaft commented Jun 30, 2021

RaduBerinde commented Jun 30, 2021

cucaroach commented Jun 25, 2021 •

edited

Loading

cucaroach left a comment •

edited

Loading