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 LShift binary operator #49467

Closed
yuzefovich opened this issue May 23, 2020 · 7 comments · Fixed by #50417
Closed

colexec: add support for LShift binary operator #49467

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

Comments

@yuzefovich
Copy link
Member

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

@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
@yuzefovich yuzefovich added this to [VECTORIZED BACKLOG] Enhancements/Features/Investigations in BACKLOG, NO NEW ISSUES: SQL Execution May 23, 2020
@tancredosouza
Copy link
Contributor

Good morning! Can I try this one? :)

@yuzefovich
Copy link
Member Author

@tancredosouza of course! I'll assign the issue to you to indicate that it is currently being worked on.

@tancredosouza
Copy link
Contributor

Hi there! I believe to be almost done with both LShift and RShift implementations.

The only thing missing, I believe, is the translation of the following statement:
telemetry.Inc(sqltelemetry.LargeLShiftArgumentCounter)

Since these are the only operations which send telemetry information, I wasn't able to figure out how to translate that to getBinOpAssignFunc.

I could also leave it as a "TODO" item in my code.

What do you think?

Kind regards,
Tancredo.

@yuzefovich
Copy link
Member Author

I wasn't able to figure out how to translate that to getBinOpAssignFunc.

I assume that something like this should just work, no?

if {{.Right}} < 0 || {{.Right}} >= 64 {
  telemetry.Inc(sqltelemetry.LargeLShiftArgumentCounter)
  colexecerror.ExpectedError(pgerror.Newf(pgcode.InvalidParameterValue, "shift argument out of range"))
}

Is there something I'm missing?

@tancredosouza
Copy link
Contributor

That did it. I tried that before but must have misspelt something, which caused it to not work.

Thanks!

@derekkenney
Copy link

Hi there. Is this still an open issue? I'm looking for a good first issue to contribute to. Thanks.

@yuzefovich
Copy link
Member Author

@derekkenney it's still "open" in a sense that it hasn't been "closed" yet, but @tancredosouza is actively working on it and already has a PR out, so please take a look at some other good first issue that doesn't have an "assignee" yet.

@craig craig bot closed this as completed in a5a838a Jun 22, 2020
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) good first issue
Projects
BACKLOG, NO NEW ISSUES: SQL Execution
[VECTORIZED BACKLOG] Enhancements/Fea...
Development

Successfully merging a pull request may close this issue.

3 participants