-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[crypto] add blake2b and blake2s functions #1081
Conversation
so strange that i cannot reproduce the errors in CI |
Maybe CI doesn't run with the newest code |
@@ -39,7 +39,7 @@ path = "src/lib.rs" | |||
[features] | |||
default = ["crypto_expressions", "regex_expressions", "unicode_expressions"] | |||
simd = ["arrow/simd"] | |||
crypto_expressions = ["md-5", "sha2"] | |||
crypto_expressions = ["md-5", "sha2", "blake2"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 for making these functions optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 there seems to be a build failures but otherwise this looks good to me
https://github.com/apache/arrow-datafusion/pull/1081/checks?check_run_id=3840276390
error[E0004]: non-exhaustive patterns: `&Blake2b` and `&Blake2s` not covered
--> /home/runner/work/arrow-datafusion/arrow-datafusion/datafusion/src/physical_plan/functions.rs:319:15
|
174 | / pub enum BuiltinScalarFunction {
175 | | // math functions
176 | | /// abs
177 | | Abs,
... |
275 | | Blake2b,
| | ------- not covered
276 | | /// Blake2s
277 | | Blake2s,
| | ------- not covered
... |
305 | | RegexpMatch,
306 | | }
| |_- `BuiltinScalarFunction` defined here
...
319 | match self {
| ^^^^ patterns `&Blake2b` and `&Blake2s` not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
= note: the matched value is of type `&BuiltinScalarFunction`
FWIW I looked at postgres and what it does is rather than having individual functions for different hashing algorithms, it appears to have a single digest
function that takes the requested algorithm as a string parameter
F.26.1.1. Digest()
digest(data text, type text) returns bytea
digest(data bytea, type text) returns bytea
@jimexist are you using a rust toolchain with the same version that's being used in our CI? |
1518354
to
def058f
Compare
let's address this in #1090 so merge that first |
cf9fb3a
to
ccf9b63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
Thanks @jimexist |
Which issue does this PR close?
add blake2b and blake2s functions, as provided in https://docs.rs/blake2/0.9.2/blake2/
must be merged after #1090 is merged.
Closes #1093
Closes #1094
Rationale for this change
blake2s is a better alternative for Md5 and blake2b is more secure, modern, etc.
What changes are included in this PR?
see https://en.wikipedia.org/wiki/BLAKE_(hash_function)
Are there any user-facing changes?