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

feat: add bitwise NOT, AND, OR & XOR functions into a yaml #370

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

PHILO-HE
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Should we include not and xor so we have the basic set?

@richtia
Copy link
Member

richtia commented Nov 21, 2022

Should we include not and xor so we have the basic set?

not and xor are here: https://github.com/substrait-io/substrait/blob/main/extensions/functions_boolean.yaml

edit: oops...yeah, there's no bitwise variants of those

@PHILO-HE
Copy link
Contributor Author

Should we include not and xor so we have the basic set?

not and xor are here: https://github.com/substrait-io/substrait/blob/main/extensions/functions_boolean.yaml

edit: oops...yeah, there's no bitwise variants of those

@westonpace @richtia, thanks for your comment! Yes, bitwise NOT and XOR are lacking. I just added them in the latest commit.

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

These functions look good to me. Unfortunately, the CI check is failing right now due to the commit message. You will need to change your commit messages to follow "semantic commit" guidelines. Look at some of the previous commits to get a sense of the style or check out this guide.

2022-11-22T14:01:23.7233121Z ##[group]Run npx commitlint --from=6d7936df1dd369b28aa1f48da06a53c0c605d39f --to=b8c688b5e34cecfaa160374c059c5af77f481cda --verbose
2022-11-22T14:01:23.7233837Z �[36;1mnpx commitlint --from=6d7936df1dd369b28aa1f48da06a53c0c605d39f --to=b8c688b5e34cecfaa160374c059c5af77f481cda --verbose�[0m
2022-11-22T14:01:23.7300817Z shell: /usr/bin/bash -e {0}
2022-11-22T14:01:23.7301130Z ##[endgroup]
2022-11-22T14:01:36.8804031Z npx: installed 199 in 12.888s
2022-11-22T14:01:37.6990085Z ⧗   input: Merge 969119c1d93d35ad741273365f5e18b2de17bdf4 into 6d7936df1dd369b28aa1f48da06a53c0c605d39f
2022-11-22T14:01:37.6994600Z ✔   found 0 problems, 0 warnings
2022-11-22T14:01:37.6995317Z ⧗   input: Add bitwise NOT & XOR
2022-11-22T14:01:37.6996150Z ✖   subject may not be empty [subject-empty]
2022-11-22T14:01:37.6996895Z ✖   type may not be empty [type-empty]
2022-11-22T14:01:37.6997433Z 
2022-11-22T14:01:37.6997896Z ✖   found 2 problems, 0 warnings
2022-11-22T14:01:37.6999890Z ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
2022-11-22T14:01:37.7000269Z 
2022-11-22T14:01:37.7000756Z ⧗   input: Add bitwise_and & bitwise_or functions into a yaml
2022-11-22T14:01:37.7001386Z ✖   subject may not be empty [subject-empty]
2022-11-22T14:01:37.7001982Z ✖   type may not be empty [type-empty]
2022-11-22T14:01:37.7002216Z 
2022-11-22T14:01:37.7002523Z ✖   found 2 problems, 0 warnings
2022-11-22T14:01:37.7003582Z ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

You will need to modify your existing commits. I find an interactive rebase is the easiest way to do this (you might also want to squash all your commits into one but as long as each commit follows semantic commit conventions it should be ok) but there are probably half a dozen git commands that will allow you to modify commits.

@PHILO-HE
Copy link
Contributor Author

@westonpace, thanks so much for providing me this detailed guide about the community convention. I just amended my commit message.

@PHILO-HE PHILO-HE changed the title feat: add bitwise_and & bitwise_or functions into a yaml feat: add bitwise NOT, AND, OR & XOR functions into a yaml Nov 25, 2022
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

This looks good to me. Let's wait for further input from @jacques-n input on the function names to see if he accepts my rationale above.

@PHILO-HE
Copy link
Contributor Author

Anyone has more comment?

@westonpace westonpace merged commit 81e34d4 into substrait-io:main Dec 13, 2022
@westonpace
Copy link
Member

Thanks for the contribution!

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.

5 participants