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 support for JSONFetchText binary operator #49470

Closed
yuzefovich opened this issue May 23, 2020 · 9 comments · Fixed by #63770
Closed

colexec: add support for JSONFetchText binary operator #49470

yuzefovich opened this issue May 23, 2020 · 9 comments · Fixed by #63770
Labels
A-sql-vec SQL vectorized engine C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@yuzefovich
Copy link
Member

yuzefovich commented May 23, 2020

Add support for JSONFetchText binary operator. For more details, see #49463.

Blocked by #49780.

@yuzefovich yuzefovich added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) good first issue A-sql-vec SQL vectorized engine labels May 23, 2020
@deathname
Copy link

Hi, Can I work on this issue

@yuzefovich
Copy link
Member Author

That would be great! I'll assign the issue to you.

@deathname
Copy link

Thanks, @yuzefovich

I think this issue is similar to https://github.com/cockroachdb/cockroach/pull/49818/files.
I just need to add one more switch case handling for JSONFetchText and return type.DString in overloads_bin.go just as similar to the above-mentioned url or there is something else ?

@yuzefovich
Copy link
Member Author

Yes, I think the solution will be very similar to #49818.

@deathname
Copy link

What should be expected output for EXPLAIN (VEC) SELECT _json -> 2 ->> 'a' FROM many_types as per vectorize_overloads input
--- Node 1 -> *rowexec.tableReader is it correct ?

@yuzefovich
Copy link
Member Author

No, this means that the old row-by-row engine is used, not the vectorized engine.

I think you might be missing a few things. Please take a look at the detailed message here.

@yuzefovich
Copy link
Member Author

I think it is currently pretty hard to implement due to some infrastructure limitations between the interactions of datum-backed arguments and non-datum-backed result, so I'm removing "good first issue" label and making the issue unassigned. Apologies for mis-triaging the issue initially.

@haseth
Copy link
Contributor

haseth commented Oct 10, 2020

Hi @yuzefovich, this should be pretty simple to pick and is not blocked by #49780 I think. Can you check it once?
JSONFetchText only requires types.String as 2nd operand.

@yuzefovich
Copy link
Member Author

yuzefovich commented Oct 10, 2020

@haseth I think you're misunderstanding the limitation described by #49780. Currently we are not able to support a case of binFn(datumVec, datumVec or <native type>) = <native type>, and this is exactly the case we have here - binFn(datumVec, coldata.Bytes) = coldata.Bytes. To be more precise, we don't support a case when at least one argument is represented via datumVec but the result is not represented via datumVec but a "native vector" instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-vec SQL vectorized engine C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants