Skip to content

Support Trino bitwise shift functions in Presto#15730

Merged
mbasmanova merged 1 commit intoprestodb:masterfrom
djsagain:add-bitwise-functions
Mar 16, 2021
Merged

Support Trino bitwise shift functions in Presto#15730
mbasmanova merged 1 commit intoprestodb:masterfrom
djsagain:add-bitwise-functions

Conversation

@djsagain
Copy link
Copy Markdown

To promote convergence between Trino and Presto, this commit
adds to Presto the bitwise functions present in Trino but missing
in Presto.

Test plan - Appropriate tests have been added to class TestBitwiseFunctions as part of this commit.

== RELEASE NOTES ==

General Changes

  • The 12 integral shift functions defined by Trino's BitwiseFunctions class have been added to Presto's BitwiseFunctions class. These shift functions represent a complete set of shift operations on tinyints, smallints, integers and longs.

@aweisberg
Copy link
Copy Markdown
Contributor

@Lewuathe appears to be the original author of this code. Please add them as a co-author. See our attribution guidelines.

@aweisberg aweisberg self-requested a review March 1, 2021 14:55
@djsagain djsagain force-pushed the add-bitwise-functions branch from c0f3961 to 2582e96 Compare March 1, 2021 15:09
@djsagain
Copy link
Copy Markdown
Author

djsagain commented Mar 1, 2021

@Lewuathe appears to be the original author of this code. Please add them as a co-author. See our attribution guidelines.

Done.

Copy link
Copy Markdown
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

LGTM. Checks were originally passing, but I imagine it has fallen far enough behind at this point that the FB integration check fails.

Thank you!

@djsagain djsagain force-pushed the add-bitwise-functions branch from 2582e96 to 469c0d0 Compare March 1, 2021 15:41
@djsagain
Copy link
Copy Markdown
Author

djsagain commented Mar 1, 2021

LGTM. Checks were originally passing, but I imagine it has fallen far enough behind at this point that the FB integration check fails.

Thanks for pointing that out - - I just rebased on tip master and force-pushed, so that should fix it.

Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@djsstarburst Would you add documentation for these functions?

@djsagain djsagain force-pushed the add-bitwise-functions branch from 469c0d0 to 8b90dc3 Compare March 11, 2021 15:59
@djsagain
Copy link
Copy Markdown
Author

@djsstarburst Would you add documentation for these functions?

@mbasmanova, I added documentation and examples for the new shift functions in bitwise.rst. While I was at it, I fixed the broken formatting in the examples of the previously existing shift functions.

To promote convergence between Trino and Presto, this commit
adds to Presto the bitwise functions present in Trino but missing
in Presto.

Co-authored-by: Lewuathe <lewuathe@me.com>
@djsagain djsagain force-pushed the add-bitwise-functions branch from 8b90dc3 to cfb9b53 Compare March 11, 2021 16:34
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

Thanks.

@aweisberg
Copy link
Copy Markdown
Contributor

@mbasmanova I restarted it and it looks like it is passing this time.

@mbasmanova mbasmanova merged commit 1126c2c into prestodb:master Mar 16, 2021
@mbasmanova
Copy link
Copy Markdown
Contributor

@djsstarburst Thank you, David, for the contribution. Thanks @aweisberg for restarting a failing job.

@varungajjala varungajjala mentioned this pull request Mar 23, 2021
6 tasks
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.

3 participants