Skip to content
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

colexecbase: add all casts of native types to strings #85681

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

yuzefovich
Copy link
Member

colexecbase: propagate bytes.Buffer as argument in cast templates

This is not used currently but will be in the following commit.

Release note: None

colexecbase: add all casts of native types to strings

This commit adds all of the casts from the types natively supported by
the vectorized engine to strings. This has an additional benefit (apart
from better performance on a lot of data: the concat binary projection
with a string on one side is now supported natively (we recently fixed
an issue to plan a cast of a non-string type to a string for a concat
operation, but since we didn't have the cast support, we'd be falling
back to the row-by-row engine in most cases).

Addresses: #48135.
Fixes: #49463.
Fixes: #55841.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich changed the title String cast colexecbase: add all casts of native types to strings Aug 5, 2022
This is not used currently but will be in the following commit.

Release note: None
This commit adds all of the casts from the types natively supported by
the vectorized engine to strings. This has an additional benefit (apart
from better performance on a lot of data: the concat binary projection
with a string on one side is now supported natively (we recently fixed
an issue to plan a cast of a non-string type to a string for a concat
operation, but since we didn't have the cast support, we'd be falling
back to the row-by-row engine in most cases).

Release note: None
@yuzefovich yuzefovich marked this pull request as ready for review August 5, 2022 20:58
@yuzefovich yuzefovich requested review from a team as code owners August 5, 2022 20:58
@yuzefovich yuzefovich requested review from msirek and mgartner August 5, 2022 20:59
Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I'm wondering if there are any more missing casts in the vectorized engine. Should we open an issue to track researching this?
:lgtm:

Reviewed 5 of 5 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

@yuzefovich
Copy link
Member Author

I'm wondering if there are any more missing casts in the vectorized engine. Should we open an issue to track researching this?

Yeah, #48135 tracks that (and this PR won't close that issue). It doesn't seem worth it to explicitly spell out casts that are still missing in that issue - it seems sufficient just to look at the top of cast_gen_util.go file to see what's present.

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 8, 2022

Build succeeded:

@craig craig bot merged commit ce8bfd4 into cockroachdb:master Aug 8, 2022
@yuzefovich yuzefovich deleted the string-cast branch August 8, 2022 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants