You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/05/04 17:00:34 UTC
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5326: Add toDateTime DateTimeFunction (#5313)
npawar commented on a change in pull request #5326:
URL: https://github.com/apache/incubator-pinot/pull/5326#discussion_r419565919
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -40,4 +47,14 @@ static Long toEpochHours(Long millis) {
static Long toEpochMinutes(Long millis, String bucket) {
return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
}
+
+ DateTime toDateTime(String dateTimeString, String pattern) {
+ if (!_dateTimeFormatterMap.containsKey(pattern)) {
Review comment:
use getOrDefault?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DateTimeFunctions.java
##########
@@ -40,4 +47,14 @@ static Long toEpochHours(Long millis) {
static Long toEpochMinutes(Long millis, String bucket) {
return TimeUnit.MILLISECONDS.toMinutes(millis) / Integer.parseInt(bucket);
}
+
+ DateTime toDateTime(String dateTimeString, String pattern) {
+ if (!_dateTimeFormatterMap.containsKey(pattern)) {
+ _dateTimeFormatterMap.put(pattern, DateTimeFormat.forPattern(pattern));
+ }
+
+ DateTimeFormatter dateTimeFormatter = _dateTimeFormatterMap.get(pattern);
+
+ return dateTimeFormatter.parseDateTime(dateTimeString);
Review comment:
These functions will be applied during record transformation, and then the GenericRecord is directly sent to the indexer
RecordReader -> GenericRecord -> RecordTransformer -> GenericRecord -> Indexer.
DateTime is not a datatype that Pinot can understand. Inputs and outputs from these transform functions should be STRING, INT, LONG.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/FunctionRegistry.java
##########
@@ -18,36 +18,26 @@
*/
package org.apache.pinot.core.data.function;
-import com.google.common.base.Preconditions;
import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* Registry for inbuilt Pinot functions
*/
public class FunctionRegistry {
Review comment:
yes that works
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/DefaultFunctionEvaluator.java
##########
@@ -50,11 +50,16 @@
public DefaultFunctionEvaluator(String expression)
throws Exception {
+ this(expression, FunctionRegistryFactory.getFunctionRegistry());
Review comment:
An instance of `DefaultFunctionEvaluator` is created per transform function in the schema. We don't want to call getFunctionRegistry multiple times, we'll just end up creating many registries right?
##########
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:
i didn't understand why this test was added
----------------------------------------------------------------
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