From 6da0ad90383c9826212f27abe490e502c642dae5 Mon Sep 17 00:00:00 2001 From: "Arun D. Panicker" Date: Sun, 17 Sep 2023 00:18:04 -0700 Subject: [PATCH] Fixing review comments --- velox/docs/functions/presto/math.rst | 1 - velox/functions/prestosql/Rand.h | 12 ---- velox/functions/prestosql/tests/RandTest.cpp | 66 ++++++++------------ 3 files changed, 25 insertions(+), 54 deletions(-) diff --git a/velox/docs/functions/presto/math.rst b/velox/docs/functions/presto/math.rst index 7e1ad79e3ea0..73de0e02c7af 100644 --- a/velox/docs/functions/presto/math.rst +++ b/velox/docs/functions/presto/math.rst @@ -137,7 +137,6 @@ Mathematical Functions Returns a cryptographically secure random value in the range 0.0 <= x < 1.0. .. function:: secure_random(lower, upper) → [same as input] - :noindex: Returns a cryptographically secure random value in the range lower <= x < upper, where lower < upper. diff --git a/velox/functions/prestosql/Rand.h b/velox/functions/prestosql/Rand.h index 0982eda7b8a9..a4d26b2b63d0 100644 --- a/velox/functions/prestosql/Rand.h +++ b/velox/functions/prestosql/Rand.h @@ -66,8 +66,6 @@ struct SecureRandFunction { FOLLY_ALWAYS_INLINE void call(double& out, const double lower, const double upper) { - VELOX_USER_CHECK_GE(lower, 0.0, "lower bound must be positive"); - VELOX_USER_CHECK_GT(upper, 0.0, "upper bound must be positive"); VELOX_USER_CHECK_GT( upper, lower, "upper bound must be greater than lower bound"); out = folly::Random::secureRandDouble(lower, upper); @@ -75,8 +73,6 @@ struct SecureRandFunction { FOLLY_ALWAYS_INLINE void call(float& out, const float lower, const float upper) { - VELOX_USER_CHECK_GE(lower, 0.0, "lower bound must be positive"); - VELOX_USER_CHECK_GT(upper, 0.0, "upper bound must be positive"); VELOX_USER_CHECK_GT( upper, lower, "upper bound must be greater than lower bound"); out = float(folly::Random::secureRandDouble(lower, upper)); @@ -84,8 +80,6 @@ struct SecureRandFunction { FOLLY_ALWAYS_INLINE void call(int64_t& out, const int64_t lower, const int64_t upper) { - VELOX_USER_CHECK_GE(lower, 0, "lower bound must be positive"); - VELOX_USER_CHECK_GT(upper, 0, "upper bound must be positive"); VELOX_USER_CHECK_GT( upper, lower, "upper bound must be greater than lower bound"); out = folly::Random::secureRand64(lower, upper); @@ -93,8 +87,6 @@ struct SecureRandFunction { FOLLY_ALWAYS_INLINE void call(int32_t& out, const int32_t lower, const int32_t upper) { - VELOX_USER_CHECK_GE(lower, 0, "lower bound must be positive"); - VELOX_USER_CHECK_GT(upper, 0, "upper bound must be positive"); VELOX_USER_CHECK_GT( upper, lower, "upper bound must be greater than lower bound"); out = folly::Random::secureRand32(lower, upper); @@ -102,8 +94,6 @@ struct SecureRandFunction { FOLLY_ALWAYS_INLINE void call(int16_t& out, const int16_t lower, const int16_t upper) { - VELOX_USER_CHECK_GE(lower, 0, "lower bound must be positive"); - VELOX_USER_CHECK_GT(upper, 0, "upper bound must be positive"); VELOX_USER_CHECK_GT( upper, lower, "upper bound must be greater than lower bound"); out = int16_t(folly::Random::secureRand32(lower, upper)); @@ -111,8 +101,6 @@ struct SecureRandFunction { FOLLY_ALWAYS_INLINE void call(int8_t& out, const int8_t lower, const int8_t upper) { - VELOX_USER_CHECK_GE(lower, 0, "lower bound must be positive"); - VELOX_USER_CHECK_GT(upper, 0, "upper bound must be positive"); VELOX_USER_CHECK_GT( upper, lower, "upper bound must be greater than lower bound"); out = int8_t(folly::Random::secureRand32(lower, upper)); diff --git a/velox/functions/prestosql/tests/RandTest.cpp b/velox/functions/prestosql/tests/RandTest.cpp index 08c31085ec40..d34ccb19172c 100644 --- a/velox/functions/prestosql/tests/RandTest.cpp +++ b/velox/functions/prestosql/tests/RandTest.cpp @@ -82,7 +82,7 @@ class SecureRandTest : public functions::test::FunctionBaseTest { }; }; -TEST_F(SecureRandTest, zeroArgTest) { +TEST_F(SecureRandTest, zeroArg) { auto result = evaluateOnce("secure_random()", makeRowVector(ROW({}), 1)); EXPECT_LT(result, 1.0); @@ -93,18 +93,18 @@ TEST_F(SecureRandTest, zeroArgTest) { EXPECT_GE(result, 0.0); } -TEST_F(SecureRandTest, int64Test) { +TEST_F(SecureRandTest, int64Args) { auto result = - secureRand((int64_t)2147532562, (int64_t)4611791058295013614); + secureRand((int64_t)-2147532562, (int64_t)4611791058295013614); EXPECT_LT(result, 4611791058295013614); - EXPECT_GE(result, 2147532562); + EXPECT_GE(result, -2147532562); result = secureRand((int64_t)0, (int64_t)46117910582950136); EXPECT_LT(result, 46117910582950136); EXPECT_GE(result, 0); } -TEST_F(SecureRandTest, int32Test) { +TEST_F(SecureRandTest, int32Args) { auto result = secureRand((int32_t)8765432, (int32_t)2145613151); EXPECT_LT(result, 2145613151); EXPECT_GE(result, 8765432); @@ -114,17 +114,17 @@ TEST_F(SecureRandTest, int32Test) { EXPECT_GE(result, 0); } -TEST_F(SecureRandTest, int16Test) { - auto result = secureRand((int16_t)100, (int16_t)23286); +TEST_F(SecureRandTest, int16Args) { + auto result = secureRand((int16_t)-100, (int16_t)23286); EXPECT_LT(result, 23286); - EXPECT_GE(result, 100); + EXPECT_GE(result, -100); result = secureRand((int16_t)0, (int16_t)23286); EXPECT_LT(result, 23286); EXPECT_GE(result, 0); } -TEST_F(SecureRandTest, int8Test) { +TEST_F(SecureRandTest, int8Args) { auto result = secureRand((int8_t)10, (int8_t)120); EXPECT_LT(result, 120); EXPECT_GE(result, 10); @@ -134,7 +134,7 @@ TEST_F(SecureRandTest, int8Test) { EXPECT_GE(result, 0); } -TEST_F(SecureRandTest, doubleTest) { +TEST_F(SecureRandTest, doubleArgs) { auto result = secureRand((double)10.5, (double)120.7895); EXPECT_LT(result, 120.7895); EXPECT_GE(result, 10.5); @@ -144,10 +144,10 @@ TEST_F(SecureRandTest, doubleTest) { EXPECT_GE(result, 0.0); } -TEST_F(SecureRandTest, floatTest) { - auto result = secureRand((float)10.5, (float)120.7); +TEST_F(SecureRandTest, floatArgs) { + auto result = secureRand((float)-10.5, (float)120.7); EXPECT_LT(result, 120.7); - EXPECT_GE(result, 10.5); + EXPECT_GE(result, -10.5); result = secureRand((float)0.0, (float)120.7); EXPECT_LT(result, 120.7); @@ -156,54 +156,38 @@ TEST_F(SecureRandTest, floatTest) { TEST_F(SecureRandTest, invalidBounds) { VELOX_ASSERT_THROW( - secureRand((int64_t)-5, (int64_t)10), - "lower bound must be positive"); - VELOX_ASSERT_THROW( - secureRand((int64_t)0, (int64_t)-1), - "upper bound must be positive"); + secureRand((int64_t)-5, (int64_t)-10), + "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( secureRand((int64_t)15, (int64_t)10), "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( - secureRand((int32_t)-5, (int32_t)10), - "lower bound must be positive"); - VELOX_ASSERT_THROW( - secureRand((int32_t)0, (int32_t)-1), - "upper bound must be positive"); + secureRand((int32_t)5, (int32_t)-10), + "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( secureRand((int32_t)15, (int32_t)10), "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( - secureRand((int16_t)-5, (int16_t)10), - "lower bound must be positive"); - VELOX_ASSERT_THROW( - secureRand((int16_t)0, (int16_t)-1), - "upper bound must be positive"); + secureRand((int16_t)-5, (int16_t)-10), + "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( secureRand((int16_t)15, (int16_t)10), "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( - secureRand((int8_t)-5, (int8_t)10), - "lower bound must be positive"); - VELOX_ASSERT_THROW( - secureRand((int8_t)0, (int8_t)-1), - "upper bound must be positive"); + secureRand((int8_t)5, (int8_t)-10), + "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( secureRand((int8_t)15, (int8_t)10), "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( - secureRand(-5.7, 10.7), "lower bound must be positive"); - VELOX_ASSERT_THROW( - secureRand(0.0, -1.0), "upper bound must be positive"); + secureRand(-5.7, -10.7), + "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( secureRand(15.6, 10.1), "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( - secureRand((float)-5.7, (float)10.7), - "lower bound must be positive"); - VELOX_ASSERT_THROW( - secureRand((float)0.0, (float)-1.0), - "upper bound must be positive"); + secureRand((float)-5.7, (float)-10.7), + "upper bound must be greater than lower bound"); VELOX_ASSERT_THROW( secureRand((float)15.6, (float)10.1), "upper bound must be greater than lower bound");