Add secure_random() and secure_random(lower,upper) Presto functions #6085
Conversation
✅ Deploy Preview for meta-velox canceled.
|
a8520ff to
98aa9e8
Compare
velox/docs/functions/presto/math.rst
Outdated
| Returns a cryptographically secure random value in the range 0.0 <= x < 1.0. | ||
|
|
||
| .. function:: secure_random(lower, upper) → [same as input] | ||
| :noindex: |
98aa9e8 to
6da0ad9
Compare
|
@aditi-pandit Can you take another look at this PR? |
|
@arundpanicker : Please can you add the fuzzer output for this function https://facebookincubator.github.io/velox/develop/testing/fuzzer.html#expression-and-aggregation-fuzzer |
|
|
||
| FOLLY_ALWAYS_INLINE void | ||
| call(double& out, const double lower, const double upper) { | ||
| VELOX_USER_CHECK_GT( |
There was a problem hiding this comment.
Maybe have an common inline function (with template type for lower and upper) with just this check statement so that its not repeated in all the functions.
| @@ -74,5 +74,124 @@ TEST_F(RandTest, nonNullInt8) { | |||
| EXPECT_LT(rand(4), 4); | |||
| } | |||
|
|
|||
| class SecureRandTest : public functions::test::FunctionBaseTest { | |||
There was a problem hiding this comment.
There isn't a need to make a separate SecureRandTest class for a single secureRand function. You can add secureRand function in RandTest itself.
| } | ||
|
|
||
| TEST_F(SecureRandTest, int64Args) { | ||
| auto result = |
There was a problem hiding this comment.
Please add test cases with kInf, kNan, numeric_limits::max(), numeric_limits::min() and null values for all the functions.
|
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions! |
|
@aditi-pandit I'm handling this one in #9295 |
|
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions! |
Add secure_random() and secure_random(lower,upper) Presto functions using the secure random functions available in the folly library. Alias secure_rand(), secure_rand(lower, upper).
Resolves : #5762 , prestodb/presto#20530
Reference : https://github.com/prestodb/presto/blob/8a9e1b6feae2d6caea3d2723c5d6a129448052bb/presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java#L704
https://github.com/facebook/folly/blob/e9e2c3f5ae87aa02b3740c935296efddecefd18f/folly/Random.h