Jackie-Jiang commented on a change in pull request #8382:
URL: https://github.com/apache/pinot/pull/8382#discussion_r833557629
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java
##########
@@ -38,4 +40,8 @@ public Method getMethod() {
public Class<?> getClazz() {
return _clazz;
}
+
+ public boolean getNullableParameters() {
Review comment:
```suggestion
public boolean hasNullableParameters() {
```
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
##########
@@ -63,12 +63,13 @@ private FunctionRegistry() {
if (scalarFunction.enabled()) {
// Annotated function names
String[] scalarFunctionNames = scalarFunction.names();
+ final boolean nullableParameters = scalarFunction.nullableParameters();
Review comment:
(minor) We don't usually put `final` for local variable
```suggestion
boolean nullableParameters = scalarFunction.nullableParameters();
```
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java
##########
@@ -306,7 +306,7 @@ public static long now() {
* "-P6H3M" -- parses as "-6 hours and -3 minutes"
* "-P-6H+3M" -- parses as "+6 hours and -3 minutes"
*/
- @ScalarFunction
+ @ScalarFunction(nullableParameters = true)
Review comment:
They should not be annotated as `nullableParameters` because `null`
argument is not handled. For any function that should return `null` when any
input is `null`, we should not annotate them to avoid the overhead of invoking
them
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/annotations/ScalarFunction.java
##########
@@ -49,4 +49,7 @@
// If empty, FunctionsRegistry registers the method name as function name;
// If not empty, FunctionsRegistry only registers the function names
specified here, the method name is ignored.
String[] names() default {};
+
+ // Whether the scalar function cannot handle nulls, and can get invoked with
null parameter(s).
Review comment:
```suggestion
// Whether the scalar function expects null arguments, and won't return
null when getting null arguments.
```
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java
##########
@@ -124,6 +126,15 @@ public void convertTypes(Object[] arguments) {
* {@link #convertTypes(Object[])} to convert the argument types if needed
before calling this method.
*/
public Object invoke(Object[] arguments) {
+ if (_nullableParameters) {
Review comment:
Most of the caller for this method won't pass in null arguments. To
avoid the overhead of this null check, we may add a method
`hasNullParameters()` and the caller can decide how to handle it. The only
place where we may pass `null` arguments is
`InbuildFunctionEvaluator.FunctionExecutionNode.execute(GenericRow row)`
--
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]
