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

colexec: minor miscellaneous additions #50001

Merged
merged 2 commits into from
Jun 15, 2020

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Jun 9, 2020

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

@yuzefovich yuzefovich requested review from asubiotto and a team June 9, 2020 04:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich changed the title colexec: add casts from datum-backed types to bools colexec: minor miscellaneous additions Jun 10, 2020
Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

Is the first commit tested somewhere?

Reviewed 5 of 5 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: 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?

Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

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: :shipit: 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.

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r3, 4 of 4 files at r4.
Reviewable status: :shipit: 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
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: 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.

@craig
Copy link
Contributor

craig bot commented Jun 15, 2020

Build failed (retrying...)

@otan
Copy link
Contributor

otan commented Jun 15, 2020

bors r-

i don't think this builds

@craig
Copy link
Contributor

craig bot commented Jun 15, 2020

Canceled

@yuzefovich
Copy link
Member Author

Indeed, it didn't. Sorry about that. A function's signature has been changed, but the rebase didn't notice any conflicts.

@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 15, 2020

Build succeeded

@craig craig bot merged commit 5cff000 into cockroachdb:master Jun 15, 2020
@yuzefovich yuzefovich deleted the datum-bool-cast branch June 15, 2020 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants