exec: fix a panic with sum_int aggregate with input types other than Int64 #38941
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @yuzefovich)
pkg/sql/distsqlrun/materializer.go, line 188 at r1 (raw file):
// It is possible that types were messed up during planning, so we want to // catch type conversion panic if it occurs. if err := exec.CatchVectorizedRuntimeError(populateOutputRow); err != nil {
We now have 2 calls to the catcher in this function. Can we hoist it outside? Since the catcher ignores stuff that we didn't generate, it should be safe.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/materializer.go, line 188 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
We now have 2 calls to the catcher in this function. Can we hoist it outside? Since the catcher ignores stuff that we didn't generate, it should be safe.
I agree, it should be safe. Done.
There was a problem hiding this comment.
Reviewable status:
|
TFTRs! bors r+ |
Merge conflict |
|
bors r- |
We used to catch panics only when calling Next() on the the input to materializer. However, it is also possible that types get messed up during planning a vectorized flow which can lead to a type conversion panic. We do not want to crash in such case either, so we wrap full Next() method with a catcher. Release note: None
Currently, our aggregates expect that input and output types are the same. However, sum_int is planned such that flow consumer expects Int64 regardless of the input types. Supporting this would require adding specialized sum aggregates that would take in any int type and would output Int64. It is easier for now to just fallback to DistSQL on non-int64 input types. Release note: None
|
bors r+ |
38941: exec: fix a panic with sum_int aggregate with input types other than Int64 r=yuzefovich a=yuzefovich First commit wraps Next of materializer with a vectorized panic catcher. We used to catch panics only when calling Next() on the the input to materializer. However, it is also possible that types get messed up during planning a vectorized flow which can lead to a type conversion panic. We do not want to crash in such case either, so we wrap full Next() method with a catcher. Second commit falls back to DistSQL on sum_int if not on Int64. Currently, our aggregates expect that input and output types are the same. However, sum_int is planned such that flow consumer expects Int64 regardless of the input types. Supporting this would require adding specialized sum aggregates that would take in any int type and would output Int64. It is easier for now to just fallback to DistSQL on non-int64 input types. Fixes: #38937. Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Build succeeded |
First commit wraps Next of materializer with a vectorized panic catcher.
We used to catch panics only when calling Next() on the the input to
materializer. However, it is also possible that types get messed up
during planning a vectorized flow which can lead to a type conversion
panic. We do not want to crash in such case either, so we wrap full
Next() method with a catcher.
Second commit falls back to DistSQL on sum_int if not on Int64.
Currently, our aggregates expect that input and output types are
the same. However, sum_int is planned such that flow consumer
expects Int64 regardless of the input types. Supporting this would
require adding specialized sum aggregates that would take in any
int type and would output Int64. It is easier for now to just
fallback to DistSQL on non-int64 input types.
Fixes: #38937.