sql: fix a bug with unset type metadata on some join processors#51481
sql: fix a bug with unset type metadata on some join processors#51481craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rohany)
pkg/sql/rowexec/joinerbase.go, line 115 at r1 (raw file):
// If we didn't pass all of the input types into ProcessorBase.Init() we could // be missing out on hydrating some of the input types. So hydrate the rest here. if len(outputTypes) < len(condTypes) {
Hm, this seems error-prone - we added the hydration to ProcessorBase.InitWithEvalCtx with the assumption that pretty much all processors do call that method, and here we found a case when types argument of that method might not contain the types of all internal columns of a processor. I'm worried that there might be other cases apart from this joinerbase when such scenario can arise although I can't immediately think of any such cases.
Maybe a better place would be to do this hydration in ExprHelper.Init? Do we cache the fact that the hydration has been performed on a particular types.T object? If so, we could just run the hydration on all of the types in that method.
|
ExprHelper.init is only called on with types passed to processorbase.Init. I think that this is just a oneoff case that needs to be considered. The hash joiner did the right job of passing all its types to
yeah we do |
yuzefovich
left a comment
There was a problem hiding this comment.
ExprHelper.init is only called on with types passed to processorbase.Init.
That's not always true - take a look at joinerBase.onCond.Init. Another possible cases where this bug might occur are invertedJoiner and projectSetProcessor.
Reviewable status:
|
Oh, i didn't see that. But I'm still not convinced exprhelper.Init is the right place to do this, since it doesn't look like we are guaranteed to call it -- if there are no filters or renders then we wouldn't call it in some procs. |
|
I guess my point is that putting the hydration into |
Oh you're not suggesting to not hydrate in processorbase -- you're just saying to do it both in processor base and exprhelper? |
|
Actually, maybe a better model is to do what we have in the |
Yeah, basically, just to be safer. |
e4fcc8f to
289583f
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/sql/rowflow/row_based_flow.go, line 272 at r2 (raw file):
// Before we can safely use types we received over the wire in the // operators, we need to make sure they are hydrated. All operators other
nit: s/operators/processors/.
Fixes cockroachdb#51474. This commit fixes a bug where some joiner processors would not hydrate their input type metadata in certain cases, leading to panics at execution. Release note: None
fixed bors r=yuzefovich |
Build succeeded |
Fixes #51474.
This commit fixes a bug where some joiner processors would not hydrate
their input type metadata in certain cases, leading to panics at
execution.
Release note: None