Copilot commented on code in PR #17208:
URL: https://github.com/apache/pinot/pull/17208#discussion_r2525730666
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapperTest.java:
##########
@@ -1125,6 +1126,63 @@ public void testBigDecimalSerDeTransformFunction() {
testTransformFunction(transformFunction, _bigDecimalSVValues);
}
+ @Test
+ public void testRandTransformFunction() {
+ ExpressionContext expression = RequestContextUtils.getExpression("rand()");
+ TransformFunction transformFunction =
TransformFunctionFactory.get(expression, _dataSourceMap);
+ assertTrue(transformFunction instanceof ScalarTransformFunctionWrapper);
+ assertEquals(transformFunction.getName(), "rand");
+ double[] firstValues =
Arrays.copyOf(transformFunction.transformToDoubleValuesSV(_projectionBlock),
NUM_ROWS);
+ for (double value : firstValues) {
+ assertTrue(value >= 0d && value < 1d, "rand() should return values in
[0, 1)");
+ }
+ double[] secondValues =
Arrays.copyOf(transformFunction.transformToDoubleValuesSV(_projectionBlock),
NUM_ROWS);
+ boolean arraysEqual = true;
+ for (int i = 0; i < NUM_ROWS; i++) {
+ if (Double.compare(firstValues[i], secondValues[i]) != 0) {
+ arraysEqual = false;
+ break;
+ }
+ }
+ assertFalse(arraysEqual, "rand() should yield non-deterministic results
across evaluations");
Review Comment:
Replace the manual array comparison loop with `!Arrays.equals(firstValues,
secondValues)` for cleaner, more maintainable code that's consistent with the
pattern used elsewhere in the test (line 1158).
```suggestion
assertFalse(Arrays.equals(firstValues, secondValues), "rand() should
yield non-deterministic results across evaluations");
```
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArithmeticFunctions.java:
##########
@@ -201,6 +204,29 @@ public static double truncate(double a) {
return Math.signum(a) * Math.floor(Math.abs(a));
}
+ @ScalarFunction
+ public static double rand() {
+ return ThreadLocalRandom.current().nextDouble();
+ }
+
+ @ScalarFunction
+ public static double rand(long seed) {
+ return deterministicRand(seed);
+ }
+
+ private static double deterministicRand(long seed) {
+ // 0x5DEECE66DL is the multiplier used by Java's java.util.Random LCG
implementation.
+ // Reference:
https://docs.oracle.com/javase/8/docs/api/java/util/Random.html
+ long mixed = mix64(seed ^ 0x5DEECE66DL);
+ return (mixed >>> 11) * DOUBLE_UNIT;
+ }
Review Comment:
The comment references Java's LCG multiplier but the code uses a different
mixing function (mix64) rather than an LCG. The comment should clarify that the
XOR with 0x5DEECE66DL is for compatibility/similarity with java.util.Random's
initial scrambling, but the actual randomization uses the MurmurHash3-style
mixing function below, not an LCG.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
