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 2022/09/14 22:01:38 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9397: [Feature] Support function registry with arg types
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: commits-unsubscribe@pinot.apache.org
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