-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
colexec: disallow planning CASE operators with unknown WHEN type #44756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/sql/colexec/execplan.go, line 1247 at r1 (raw file):
whenResolvedType := whenTyped.ResolvedType() // TODO(asubiotto): Can we just plan these as bools? if whenTyped.ResolvedType() == types.Unknown {
Maybe the check here should be tighter? Like convert the resolved type into coltypes.T
, and if it coltypes.Unhandled
, then bail? Or are the only two options here are types.Bool
and types.Unknown
?
pkg/sql/colexec/execplan.go, line 1253 at r1 (raw file):
var whenInternalMemUsed, thenInternalMemUsed int caseOps[i], resultIdx, ct, whenInternalMemUsed, err = planTypedMaybeNullProjectionOperators( ctx, evalCtx, whenTyped, whenResolvedType, []types.T{*whenResolvedType}, buffer, acc,
I think ct
should be passed in and not []types.T{*whenResolvedType}
.
pkg/sql/logictest/testdata/logic_test/vectorize, line 1099 at r1 (raw file):
# Regression test for 44726 (unknown WHEN expression type). statement ok CREATE TABLE t44726(c0 INT); INSERT INTO t44726(c0) VALUES (0);
[nit]: unnecessary semicolons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)
pkg/sql/colexec/execplan.go, line 1253 at r1 (raw file):
Previously, yuzefovich wrote…
I think
ct
should be passed in and not[]types.T{*whenResolvedType}
.
This is something I don't understand, why ct
? Isn't that the output type of the CASE
operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/sql/colexec/execplan.go, line 1253 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
This is something I don't understand, why
ct
? Isn't that the output type of theCASE
operator?
Updated ct
will be the output of the projection operator (and the passed-in ct
is the schema of the input to the projection operator).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/execplan.go, line 1247 at r1 (raw file):
Previously, yuzefovich wrote…
Maybe the check here should be tighter? Like convert the resolved type into
coltypes.T
, and if itcoltypes.Unhandled
, then bail? Or are the only two options here aretypes.Bool
andtypes.Unknown
?
Done.
pkg/sql/colexec/execplan.go, line 1253 at r1 (raw file):
Previously, yuzefovich wrote…
Updated
ct
will be the output of the projection operator (and the passed-inct
is the schema of the input to the projection operator).
I'm still not sure why ct
needs to contain the output type of the WHEN
clause though. Asking you "offline" to try understand this better. Updated this line as well.
pkg/sql/logictest/testdata/logic_test/vectorize, line 1099 at r1 (raw file):
Previously, yuzefovich wrote…
[nit]: unnecessary semicolons.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release note is not entirely correct - this could occur with auto
setting as well since CASE
operator is streaming, so this fix will also need to be backported.
Reviewed 2 of 2 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)
pkg/sql/colexec/execplan.go, line 1302 at r2 (raw file):
if whenColType == coltypes.Unhandled { return nil, resultIdx, ct, internalMemUsed, errors.Newf( "unsupported type %s in CASE WHEN expression", whenTyped.ResolvedType().String())
s/whenTyped.ResolvedType()/whenResolvedType/g
(super nit).
These will fall back to row execution. Release note (bug fix): CASE operators with an unknown WHEN type no longer return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/execplan.go, line 1253 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I'm still not sure why
ct
needs to contain the output type of theWHEN
clause though. Asking you "offline" to try understand this better. Updated this line as well.
Thanks for the clarification!
pkg/sql/colexec/execplan.go, line 1302 at r2 (raw file):
Previously, yuzefovich wrote…
s/whenTyped.ResolvedType()/whenResolvedType/g
(super nit).
Done.
bors r=yuzefovich |
44756: colexec: disallow planning CASE operators with unknown WHEN type r=yuzefovich a=asubiotto Release note (bug fix): When vectorize=experimental_on, CASE operators with an unknown WHEN type fall back to row execution. Fixes #44726 Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
Build succeeded |
Release note (bug fix): When vectorize=experimental_on, CASE operators with an
unknown WHEN type fall back to row execution.
Fixes #44726