Jackie-Jiang commented on code in PR #9397:
URL: https://github.com/apache/pinot/pull/9397#discussion_r971333545
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -18,31 +18,77 @@
*/
package org.apache.pinot.common.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 java.util.Set;
import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.FunctionContext;
+import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.spi.annotations.ScalarFunction;
+import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Registry for scalar functions.
+ *
+ * The registry registers functions using canonical functionName and argument
type as DataType.
+ * It doesn't differentiate function name in different canonical forms or
argument types whose DataType is the same such
+ * as primitive numerical type and its wrapper class.
+ *
+ * To be backward compatible, the registry falls back functionName + param
number matching when there are no type match
+ * for parameters.
* <p>TODO: Merge FunctionRegistry and FunctionDefinitionRegistry to provide
one single registry for all functions.
*/
public class FunctionRegistry {
private FunctionRegistry() {
}
private static final Logger LOGGER =
LoggerFactory.getLogger(FunctionRegistry.class);
+
+ // Map from function name to parameter types to function info.
+ private static final Map<String, Map<List<FieldSpec.DataType>,
FunctionInfo>> FUNC_PARAM_INFO_MAP = new HashMap<>();
Review Comment:
Suggest making the value a separate class, which can be used to look up the
`FunctionInfo` with a list of argument types. That way we don't need to
maintain 2 maps, and can support type matching (match int argument to long
parameter function) in the future.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -18,31 +18,77 @@
*/
package org.apache.pinot.common.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 java.util.Set;
import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.FunctionContext;
+import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.spi.annotations.ScalarFunction;
+import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.utils.PinotReflectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Registry for scalar functions.
+ *
+ * The registry registers functions using canonical functionName and argument
type as DataType.
+ * It doesn't differentiate function name in different canonical forms or
argument types whose DataType is the same such
+ * as primitive numerical type and its wrapper class.
+ *
+ * To be backward compatible, the registry falls back functionName + param
number matching when there are no type match
+ * for parameters.
* <p>TODO: Merge FunctionRegistry and FunctionDefinitionRegistry to provide
one single registry for all functions.
*/
public class FunctionRegistry {
private FunctionRegistry() {
}
private static final Logger LOGGER =
LoggerFactory.getLogger(FunctionRegistry.class);
+
+ // Map from function name to parameter types to function info.
+ private static final Map<String, Map<List<FieldSpec.DataType>,
FunctionInfo>> FUNC_PARAM_INFO_MAP = new HashMap<>();
Review Comment:
We might want to use `ColumnDataType` here which contains the SV/MV info
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -92,11 +138,17 @@ public static void registerFunction(Method method, boolean
nullableParameters) {
* Registers a method with the given function name.
*/
public static void registerFunction(String functionName, Method method,
boolean nullableParameters) {
- FunctionInfo functionInfo = new FunctionInfo(method,
method.getDeclaringClass(), nullableParameters);
- String canonicalName = canonicalize(functionName);
+ final FunctionInfo functionInfo = new FunctionInfo(method,
method.getDeclaringClass(), nullableParameters);
Review Comment:
(convention) We don't usually put `final` for local variables
--
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]
