[GitHub] [incubator-pinot] reallocf commented on a change in pull request #5326: Add toDateTime DateTimeFunction (#5313)

[GitBox <gi...@apache.org>]

reallocf commented on a change in pull request #5326:
URL: https://github.com/apache/incubator-pinot/pull/5326#discussion_r422548708



##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/function/DefaultFunctionEvaluatorTest.java
##########
@@ -54,49 +54,73 @@ public void testExpressionWithColumn()
   @Test
   public void testExpressionWithConstant()
       throws Exception {
-    FunctionRegistry
-        .registerStaticFunction(MyFunc.class.getDeclaredMethod("daysSinceEpoch", String.class, String.class));
+    MyFunc myFunc = new MyFunc();
+    FunctionRegistry functionRegistry = new FunctionRegistry(
+        Lists.newArrayList(myFunc.getClass().getDeclaredMethod("daysSinceEpoch", String.class, String.class)));
     String input = "1980-01-01";
     String format = "yyyy-MM-dd";
     String expression = String.format("daysSinceEpoch('%s', '%s')", input, format);
-    DefaultFunctionEvaluator evaluator = new DefaultFunctionEvaluator(expression);
+    DefaultFunctionEvaluator evaluator = new DefaultFunctionEvaluator(expression, functionRegistry);
     Assert.assertTrue(evaluator.getArguments().isEmpty());
     GenericRow row = new GenericRow();
     Object result = evaluator.evaluate(row);
-    Assert.assertEquals(result, MyFunc.daysSinceEpoch(input, format));
+    Assert.assertEquals(result, myFunc.daysSinceEpoch(input, format));
   }
 
   @Test
   public void testMultiFunctionExpression()
       throws Exception {
-    FunctionRegistry.registerStaticFunction(MyFunc.class.getDeclaredMethod("reverseString", String.class));
-    FunctionRegistry
-        .registerStaticFunction(MyFunc.class.getDeclaredMethod("daysSinceEpoch", String.class, String.class));
+    MyFunc myFunc = new MyFunc();
+    FunctionRegistry functionRegistry = new FunctionRegistry(Lists
+        .newArrayList(myFunc.getClass().getDeclaredMethod("reverseString", String.class),
+            myFunc.getClass().getDeclaredMethod("daysSinceEpoch", String.class, String.class)));
     String input = "1980-01-01";
-    String reversedInput = MyFunc.reverseString(input);
+    String reversedInput = myFunc.reverseString(input);
     String format = "yyyy-MM-dd";
     String expression = String.format("daysSinceEpoch(reverseString('%s'), '%s')", reversedInput, format);
-    DefaultFunctionEvaluator evaluator = new DefaultFunctionEvaluator(expression);
+    DefaultFunctionEvaluator evaluator = new DefaultFunctionEvaluator(expression, functionRegistry);
     Assert.assertTrue(evaluator.getArguments().isEmpty());
     GenericRow row = new GenericRow();
     Object result = evaluator.evaluate(row);
-    Assert.assertEquals(result, MyFunc.daysSinceEpoch(input, format));
+    Assert.assertEquals(result, myFunc.daysSinceEpoch(input, format));
   }
 
-  private static class MyFunc {
-    static String reverseString(String input) {
-      return new StringBuilder(input).reverse().toString();
-    }
+  @Test
+  public void testStateSharedBetweenRowsForExecution()
+      throws Exception {

Review comment:
       This test basically just confirms that the internal state of the FunctionRegistry is shared between each row. I agree with the current implementation it's fairly self-explanatory, but you can imagine an implementation where the internals of the FunctionRegistry are different for each row. This is to make sure we don't somehow regress to that, because then we'd see a big performance hit for creating a SDF for each row.
   
   But can definitely remove if it just feels like clutter to you ☺️ 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org