optbuilder: remove spurious casts in binary ops#85108
optbuilder: remove spurious casts in binary ops#85108craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
77a068b to
832cc32
Compare
|
Hi @mgartner, tagging you because you've dealt with these auto-casts a bunch. I think that all of the casts here are spurious, but obviously I'm not sure and would be concerned I'm missing something. If this is an unpalatable approach I can scope it down quite a bit, just thought I'd check to see if we can remove these excess casts while we're at it. |
mgartner
left a comment
There was a problem hiding this comment.
The fact that we add these casts at all is questionable - we can potentially add casts which are supposed to only be allowed when appearing explicitly in a query. Notice this comment in eval.ReType which is called by the reType in this commit:
cockroach/pkg/sql/sem/eval/cast.go
Lines 47 to 53 in a43abd7
So this seems like a step in the right direction, regardless of the fact that it fixes trigrams.
Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/opt/optbuilder/scalar.go line 221 at r1 (raw file):
// issues for the execbuilder, which doesn't have enough information to // select the right overload. The solution is to wrap any mismatched // arguments with a CastExpr that preserves the static type.
It sounds like we could avoid these casts entirely if we attached t.ResolvedBinOp().[Left|Right]Type to the NullExpr constructed in b.buildScalar below. If the Null was typed, then execbuilder should be able to determine the correct overload. I created an issue to track this: #85135
|
Thanks for the review! @yuzefovich could you briefly look at the changed vectorized execution plans and let me know if you see any potential issues too - i know we've had some trouble in the past with integer width casts getting messed up in vectorized and want to make sure this doesn't regress us. |
yuzefovich
left a comment
There was a problem hiding this comment.
LGTM - we also run all of the queries in vectorize_overloads file as a logic test. One thing that'd be good to do (to get more confidence in this PR) is to include fakedist config into pkg/sql/logictest/testdata/logic_test/vectorize_overloads. (I think originally we had both the query execution and the EXPLAIN (VEC) in the same file, so we couldn't use fakedist config, but we have since split those two up, so it should be possible to run with more configs.) Actually, maybe just remove the config specification in that file completely so that we run with all default configs.
Reviewed 2 of 19 files at r1.
Reviewable status:
832cc32 to
1ccbcae
Compare
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
Good suggestion, I added all the configs to that file (and a few missing bors r+ |
|
I think this will come into a conflict with the updated logic test framework, so I'll unbors it for now. bors r- |
|
Canceled. |
Previously, the optbuilder would insert spurious casts in binary operators while building a plan to avoid a tricky type checking case. This got in the way of checking for inverted filter support for trigrams. Now, the code only inserts the cast if necessary (if the input expression is NULL). Release note: None
1ccbcae to
5a03f03
Compare
|
Courtesy fixup :) bors r+ |
|
Build failed: |
|
backupccl package level flake bors r+ |
|
Build succeeded: |
Fixes #85106
Previously, the optbuilder would insert spurious casts in binary
operators while building a plan to avoid a tricky type checking case.
This got in the way of checking for inverted filter support for
trigrams. Now, the code only inserts the cast if necessary (if the input
expression is NULL).
Release note: None