colexec: minor miscellaneous additions#50001
Conversation
60cfd06 to
8becc09
Compare
8becc09 to
90c8c75
Compare
asubiotto
left a comment
There was a problem hiding this comment.
Is the first commit tested somewhere?
Reviewed 5 of 5 files at r1, 4 of 4 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/execplan.go, line 642 at r2 (raw file):
return result, err } if core.Values.NumRows != 0 {
What does supporting only this specific case give us vs implementing a full values operator?
90c8c75 to
6973015
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Good point, extended the randomized test we have. Unfortunately, I don't remember exactly what prompted me to add the cast from datum-backed type to a boolean (probably it was a typed NULL with a CASE expression or something like that), and the only datum-backed type that has a valid cast to bool is Collated String which is probably not that useful of an addition. However, the first commit does lay some groundwork for future additions of casts from datum-backed types, so I think it's still worth merging.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/sql/colexec/execplan.go, line 642 at r2 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
What does supporting only this specific case give us vs implementing a full values operator?
This particular case came up when there is a subquery that evaluates to an empty set. True, general values operator would be more useful, and I briefly looked into that, it was more than 5 minutes effort, so I didn't proceed since this limited addition was sufficient to debug the thing I was working on. I'll file an issue to add support for Values core.
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r3, 4 of 4 files at r4.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/cast_test.go, line 87 at r3 (raw file):
toPhysType func(tree.Datum) interface{} // getValidSet (when non-nil) is a function that returns a set of valid // datums of fromTyp type that can be casted to toTyp type. The test
nit: s/casted/cast
pkg/sql/colexec/cast_test.go, line 89 at r3 (raw file):
// datums of fromTyp type that can be casted to toTyp type. The test // harness will be randomly choosing a datum from this set. This // function should be specified when random generation using
nit: "should be specified when random generation"
While investigating unrelated test failures, I added this cast, so we might as well merge it. Release note: None
6973015 to
6cc78a1
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto)
pkg/sql/colexec/cast_test.go, line 87 at r3 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
nit:
s/casted/cast
I actually looked up what is the correct form when writing this comment (which is cast), but the comment below already had casted, so I decided to use the incorrect form to be inline with the other comment :) Updated in both places.
Build failed (retrying...) |
|
bors r- i don't think this builds |
Canceled |
Release note: None
6cc78a1 to
743af04
Compare
|
Indeed, it didn't. Sorry about that. A function's signature has been changed, but the rebase didn't notice any conflicts. |
|
bors r+ |
Build succeeded |
colexec: add casts from datum-backed types to bools
While investigating unrelated test failures, I added this cast, so we
might as well merge it.
Addresses: #48135.
Release note: None
colexec: add support for Values core with zero rows
Release note: None