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.

Sign up for GitHub

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

Jump to bottom

add digest(utf8, method) function and refactor all current hash digest functions #1090

Merged
merged 1 commit into from
Oct 10, 2021
Merged

add digest(utf8, method) function and refactor all current hash digest functions #1090

merged 1 commit into from
Oct 10, 2021

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Oct 9, 2021 •
edited
Loading

Which issue does this PR close?

add digest(utf8, method) function and refactor all current hash digest functions, refactor the current crypto function module

Closes #1093

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added ballista datafusion Changes in the datafusion crate labels Oct 9, 2021
@jimexist jimexist force-pushed the add-digest branch 3 times, most recently from d9f74cf to 2aad9be Compare October 9, 2021 04:42
@jimexist jimexist changed the title add digest(utf8, method) function add digest(utf8, method) function and refactor all current hash digest functions Oct 9, 2021
@houqp
Copy link
Member

houqp commented Oct 9, 2021

Looks good to me overall, but one of the tests is failing.

@jimexist jimexist force-pushed the add-digest branch 4 times, most recently from bef6f05 to 88cd2d4 Compare October 9, 2021 08:50
@jimexist jimexist mentioned this pull request Oct 9, 2021
Merged
@jimexist
Copy link
Member Author

jimexist commented Oct 9, 2021 •
edited
Loading

please note that md5 returns hex string, digest returns bytes

[postgres] # select md5('1'), digest('1', 'md5'), sha512('1'), digest('1', 'sha512');
-[ RECORD 1 ]------------------------------------------------------------------------------------------------------------------------------
md5    | c4ca4238a0b923820dcc509a6f75849b
digest | \xc4ca4238a0b923820dcc509a6f75849b
sha512 | \x4dff4ea340f0a823f15d3f4f01ab62eae0e5da579ccb851f8db9dfe84c58b2b37b89903a740e1ee172da793a6e79d560e5f7f9bd058a12a280433ed6fa46510a
digest | \x4dff4ea340f0a823f15d3f4f01ab62eae0e5da579ccb851f8db9dfe84c58b2b37b89903a740e1ee172da793a6e79d560e5f7f9bd058a12a280433ed6fa46510a

@houqp
Copy link
Member

houqp commented Oct 9, 2021 •
edited
Loading

builtin pgcrypto support without having to execute CREATE EXTENSION IF NOT EXISTS pgcrypto;, nice 👍

@jimexist
Copy link
Member Author

@houqp all suggestions incorporated, PTAL

houqp
houqp approved these changes Oct 10, 2021
View reviewed changes
Copy link
Member

@houqp houqp left a comment

Choose a reason for hiding this comment

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

LGTM, great work @jimexist !

@houqp houqp added the enhancement New feature or request label Oct 10, 2021
Dandandan
Dandandan reviewed Oct 10, 2021
View reviewed changes
datafusion/tests/sql.rs
test_expression!("sha384('tom')", "096f5b68aa77848e4fdf5c1c0b350de2dbfad60ffd7c25d9ea07c6c19b8a4d55a9187eb117c557883f58c16dfac3e343");
test_expression!("digest('tom','sha384')", "096f5b68aa77848e4fdf5c1c0b350de2dbfad60ffd7c25d9ea07c6c19b8a4d55a9187eb117c557883f58c16dfac3e343");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@Dandandan Dandandan merged commit 613188f into apache:master Oct 10, 2021
@Dandandan
Copy link
Contributor

Thanks @jimexist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houqp houqp houqp approved these changes

@Dandandan Dandandan Dandandan approved these changes

Assignees
No one assigned
Labels
datafusion Changes in the datafusion crate enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement digest function to unify crypto functions
3 participants
@jimexist @houqp @Dandandan