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: add JSONFetchVal operator for vectorized engine #49818

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

yongyanglai
Copy link
Contributor

@yongyanglai yongyanglai commented Jun 3, 2020

Previously, the vectorized engine had no support for JSONFetchVal
operator. This commit added JSONFetchVal operator.
In this commit, I added BytesFamily into compatible canonical type
family group of DatumVecCanonicalTypeFamily. Then I declared
JSONFetchVal as supported binary operator and registered output
type of JSONFetchVal to generate operators.

Fixes #49469

Release note (sql change): Vectorized engine now supports JSONFetchVal(->) operator.

@blathers-crl
Copy link

blathers-crl bot commented Jun 3, 2020

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your pull request description contains a release note - this can be the same as the one in your commit message.
  • When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Jun 3, 2020
@blathers-crl blathers-crl bot requested a review from yuzefovich June 3, 2020 01:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yongyanglai yongyanglai force-pushed the jsonfetchval branch 2 times, most recently from fc86165 to 8168755 Compare June 3, 2020 09:18
@cockroachdb cockroachdb deleted a comment from blathers-crl bot Jun 3, 2020
@cockroachdb cockroachdb deleted a comment from blathers-crl bot Jun 3, 2020
Copy link
Member

@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.

Thanks for working on this!

nit: in the commit message, s/Previvously/Previously/g.

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @yongyanglai)


pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 165 at r1 (raw file):

	binOpOutputTypes[tree.JSONFetchVal] = map[typePair]*types.T{
		{typeconv.DatumVecCanonicalTypeFamily, anyWidth, types.IntFamily, anyWidth}:   types.Any,

types.IntFamily supports multiple widths and we should set the output type for each, look for an example above in this function.


pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 649 at r1 (raw file):

		runtimeConversion = fmt.Sprintf("tree.DInterval{Duration: %s}", nativeElem)
	case types.BytesFamily:
		switch op {

This works for now, but we probably need to figure out a better way to do this type resolution. Could you please add this TODO to the code?

// TODO(yuzefovich): figure out a better way to perform type resolution
// for types that have the same physical representation.

@blathers-crl
Copy link

blathers-crl bot commented Jun 4, 2020

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@yongyanglai
Copy link
Contributor Author

Thanks for reviews.
RFAL

Copy link
Member

@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.

Thanks! Last two nits and then it is good to go.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @yongyanglai)


pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 167 at r2 (raw file):

		{typeconv.DatumVecCanonicalTypeFamily, anyWidth, types.BytesFamily, anyWidth}: types.Any,
	}
	for _, leftIntWidth := range supportedWidthsByCanonicalTypeFamily[types.IntFamily] {

nit: I'd name it simply intWidth because we have IntFamily only on one side.


pkg/sql/logictest/testdata/logic_test/vectorize_overloads, line 240 at r2 (raw file):


query T
EXPLAIN (VEC) SELECT _json -> 0 FROM many_types

nit: it would be nice to try non-constant on the right, maybe SELECT _json -> _int2 FROM many_types?

Previously, the vectorized engine had no support for JSONFetchVal
operator. This commit added JSONFetchVal operator.
In this commit, I added BytesFamily into compatible canonical type
family group of DatumVecCanonicalTypeFamily. Then I declared
JSONFetchVal as supported binary operator and registered output
type of JSONFetchVal to generate operators.

Release note (sql change): Vectorized engine now support JSONFetchVal(->) operator.
@yongyanglai
Copy link
Contributor Author

Thanks, I have added more logic test
PTAL

Copy link
Member

@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.

Thanks! :lgtm:

We're currently experiencing some issues with our CI system, and I will merge this PR once those are resolved.

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@yuzefovich
Copy link
Member

Thanks for your contribution!

bors r=yuzefovich

@craig
Copy link
Contributor

craig bot commented Jun 4, 2020

Build succeeded

@craig craig bot merged commit f98432f into cockroachdb:master Jun 4, 2020
@yongyanglai yongyanglai deleted the jsonfetchval branch June 25, 2020 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

colexec: add support for JSONFetchVal binary operator
3 participants