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 00:49:54 UTC
[GitHub] [pinot] 61yao opened a new pull request, #9397: [Feature] Support function registry with arg types
61yao opened a new pull request, #9397:
URL: https://github.com/apache/pinot/pull/9397
1) Functions are registered using FunctionName and DataType (converted from Java type)
2) To make this fully work, we have to support all literal passing from SqlNode to Thrift so we can support all LiteralType
--
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
[GitHub] [pinot] 61yao commented on pull request #9397: [Feature] Support function registry with arg types
Posted by GitBox <gi...@apache.org>.
61yao commented on PR #9397:
URL: https://github.com/apache/pinot/pull/9397#issuecomment-1247417085
> This one relies on #9389. We can first finish that and get back to this
SG
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9397: [Feature] Support function registry with arg types
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #9397: [Feature] Support function registry with arg types
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9397:
URL: https://github.com/apache/pinot/pull/9397#issuecomment-1247347794
This one relies on #9389. We can first finish that and get back to this
--
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