Jackie-Jiang commented on code in PR #9602:
URL: https://github.com/apache/pinot/pull/9602#discussion_r999975649
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -91,29 +86,28 @@ private FunctionRegistry() {
public static void init() {
}
+ public static void registerFunction(Method method) {
+ registerFunction(method.getName(), method);
+ }
+
/**
* Registers a method with the name of the method.
*/
- public static void registerFunction(Method method, boolean
nullableParameters) {
- registerFunction(method.getName(), method, nullableParameters);
-
- // Calcite ScalarFunctionImpl doesn't allow customized named functions.
TODO: fix me.
+ public static void registerFunction(String name, Method method) {
if (method.getAnnotation(Deprecated.class) == null) {
Review Comment:
(MAJOR) This will cause deprecated method not registered, which is backward
incompatible
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -140,8 +134,33 @@ public static boolean containsFunction(String
functionName) {
*/
@Nullable
public static FunctionInfo getFunctionInfo(String functionName, int
numParameters) {
- Map<Integer, FunctionInfo> functionInfoMap =
FUNCTION_INFO_MAP.get(canonicalize(functionName));
- return functionInfoMap != null ? functionInfoMap.get(numParameters) : null;
+ Collection<Map.Entry<String, Function>> allFunctions =
FUNCTION_MAP.range(canonicalize(functionName), true);
+ if (allFunctions.isEmpty()) {
+ return null;
+ }
+
+ List<Function> matchingFunctions = new ArrayList<>();
+ for (Map.Entry<String, Function> allFunction : allFunctions) {
+ Function candidate = allFunction.getValue();
+ if (candidate.getParameters().size() == numParameters) {
+ matchingFunctions.add(candidate);
+ }
+ }
+
+ if (matchingFunctions.size() > 1) {
+ // this should never happen (yet) because we restrict the registering of
multiple methods
+ // with the same number of arguments and the same name
+ throw new BadQueryRequestException(
+ String.format("Could not resolve function %s with %s parameters as
multiple were registered: %s",
+ functionName,
Review Comment:
(minor, format) Re-format the code
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -69,18 +65,17 @@ private FunctionRegistry() {
if (scalarFunction.enabled()) {
// Annotated function names
String[] scalarFunctionNames = scalarFunction.names();
- boolean nullableParameters = scalarFunction.nullableParameters();
- if (scalarFunctionNames.length > 0) {
- for (String name : scalarFunctionNames) {
- FunctionRegistry.registerFunction(name, method,
nullableParameters);
- }
- } else {
- FunctionRegistry.registerFunction(method, nullableParameters);
+ if (scalarFunctionNames.length == 0) {
+ registerFunction(method);
+ }
+
+ for (String name : scalarFunctionNames) {
+ FunctionRegistry.registerFunction(name, method);
Review Comment:
(minor)
```suggestion
if (scalarFunctionNames.length == 0) {
registerFunction(method);
} else {
for (String name : scalarFunctionNames) {
registerFunction(name, method);
```
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java:
##########
@@ -19,17 +19,27 @@
package org.apache.pinot.common.function;
import java.lang.reflect.Method;
+import org.apache.calcite.schema.Function;
+import org.apache.calcite.schema.impl.ScalarFunctionImpl;
+import org.apache.pinot.spi.annotations.ScalarFunction;
public class FunctionInfo {
private final Method _method;
private final Class<?> _clazz;
private final boolean _nullableParameters;
- public FunctionInfo(Method method, Class<?> clazz, boolean
nullableParameters) {
- _method = method;
- _clazz = clazz;
- _nullableParameters = nullableParameters;
+ public FunctionInfo(Function calciteFunction) {
Review Comment:
We don't want to do reflection on a per function per query basis. One way to
avoid it is to have a wrapper class over `Function` and `FunctionInfo`, where
`FunctionInfo` is pre-constructed
--
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]
