This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 54db091 builtin datetime functions to avoid throwing
NullPointerException when input value is null (#8382)
54db091 is described below
commit 54db0915c9ccb744639b2730b32a540d17ab9cd3
Author: nizarhejazi <[email protected]>
AuthorDate: Fri Mar 25 12:05:24 2022 -0700
builtin datetime functions to avoid throwing NullPointerException when
input value is null (#8382)
---
.../apache/pinot/common/function/FunctionInfo.java | 8 +++++-
.../pinot/common/function/FunctionRegistry.java | 13 +++++----
.../common/function/scalar/JsonFunctions.java | 27 +++++++++---------
.../pinot/common/function/JsonFunctionsTest.java | 2 +-
.../core/data/function/DateTimeFunctionsTest.java | 19 ++++++++++++-
.../function/InbuiltFunctionEvaluatorTest.java | 33 +++++++++++-----------
.../local/function/InbuiltFunctionEvaluator.java | 20 +++++++++++++
.../pinot/spi/annotations/ScalarFunction.java | 3 ++
8 files changed, 87 insertions(+), 38 deletions(-)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java
b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java
index 0169823..7b8fcff 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInfo.java
@@ -24,11 +24,13 @@ import java.lang.reflect.Method;
public class FunctionInfo {
private final Method _method;
private final Class<?> _clazz;
+ private final boolean _nullableParameters;
- public FunctionInfo(Method method, Class<?> clazz) {
+ public FunctionInfo(Method method, Class<?> clazz, boolean
nullableParameters) {
method.setAccessible(true);
_method = method;
_clazz = clazz;
+ _nullableParameters = nullableParameters;
}
public Method getMethod() {
@@ -38,4 +40,8 @@ public class FunctionInfo {
public Class<?> getClazz() {
return _clazz;
}
+
+ public boolean hasNullableParameters() {
+ return _nullableParameters;
+ }
}
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
index 831ece4..368acf9 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java
@@ -63,12 +63,13 @@ public class 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);
+ FunctionRegistry.registerFunction(name, method,
nullableParameters);
}
} else {
- FunctionRegistry.registerFunction(method);
+ FunctionRegistry.registerFunction(method, nullableParameters);
}
}
}
@@ -87,15 +88,15 @@ public class FunctionRegistry {
/**
* Registers a method with the name of the method.
*/
- public static void registerFunction(Method method) {
- registerFunction(method.getName(), method);
+ public static void registerFunction(Method method, boolean
nullableParameters) {
+ registerFunction(method.getName(), method, nullableParameters);
}
/**
* Registers a method with the given function name.
*/
- public static void registerFunction(String functionName, Method method) {
- FunctionInfo functionInfo = new FunctionInfo(method,
method.getDeclaringClass());
+ public static void registerFunction(String functionName, Method method,
boolean nullableParameters) {
+ FunctionInfo functionInfo = new FunctionInfo(method,
method.getDeclaringClass(), nullableParameters);
String canonicalName = canonicalize(functionName);
Map<Integer, FunctionInfo> functionInfoMap =
FUNCTION_INFO_MAP.computeIfAbsent(canonicalName, k -> new HashMap<>());
Preconditions.checkState(functionInfoMap.put(method.getParameterCount(),
functionInfo) == null,
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
b/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
index c109241..a81ec37 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java
@@ -31,6 +31,7 @@ import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
+import javax.annotation.Nullable;
import org.apache.pinot.common.function.JsonPathCache;
import org.apache.pinot.spi.annotations.ScalarFunction;
import org.apache.pinot.spi.utils.JsonUtils;
@@ -66,8 +67,8 @@ public class JsonFunctions {
/**
* Convert Map to Json String
*/
- @ScalarFunction
- public static String toJsonMapStr(Map map)
+ @ScalarFunction(nullableParameters = true)
+ public static String toJsonMapStr(@Nullable Map map)
throws JsonProcessingException {
return JsonUtils.objectToString(map);
}
@@ -75,7 +76,7 @@ public class JsonFunctions {
/**
* Convert object to Json String
*/
- @ScalarFunction
+ @ScalarFunction(nullableParameters = true)
public static String jsonFormat(Object object)
throws JsonProcessingException {
return JsonUtils.objectToString(object);
@@ -89,7 +90,7 @@ public class JsonFunctions {
if (object instanceof String) {
return PARSE_CONTEXT.parse((String) object).read(jsonPath,
NO_PREDICATES);
}
- return object == null ? null : PARSE_CONTEXT.parse(object).read(jsonPath,
NO_PREDICATES);
+ return PARSE_CONTEXT.parse(object).read(jsonPath, NO_PREDICATES);
}
/**
@@ -103,8 +104,8 @@ public class JsonFunctions {
return convertObjectToArray(PARSE_CONTEXT.parse(object).read(jsonPath,
NO_PREDICATES));
}
- @ScalarFunction
- public static Object[] jsonPathArrayDefaultEmpty(Object object, String
jsonPath) {
+ @ScalarFunction(nullableParameters = true)
+ public static Object[] jsonPathArrayDefaultEmpty(@Nullable Object object,
String jsonPath) {
try {
Object[] result = object == null ? null : jsonPathArray(object,
jsonPath);
return result == null ? EMPTY : result;
@@ -134,14 +135,14 @@ public class JsonFunctions {
if (jsonValue instanceof String) {
return (String) jsonValue;
}
- return jsonValue == null ? null : JsonUtils.objectToString(jsonValue);
+ return JsonUtils.objectToString(jsonValue);
}
/**
* Extract from Json with path to String
*/
- @ScalarFunction
- public static String jsonPathString(Object object, String jsonPath, String
defaultValue) {
+ @ScalarFunction(nullableParameters = true)
+ public static String jsonPathString(@Nullable Object object, String
jsonPath, String defaultValue) {
try {
Object jsonValue = jsonPath(object, jsonPath);
if (jsonValue instanceof String) {
@@ -164,8 +165,8 @@ public class JsonFunctions {
/**
* Extract from Json with path to Long
*/
- @ScalarFunction
- public static long jsonPathLong(Object object, String jsonPath, long
defaultValue) {
+ @ScalarFunction(nullableParameters = true)
+ public static long jsonPathLong(@Nullable Object object, String jsonPath,
long defaultValue) {
try {
Object jsonValue = jsonPath(object, jsonPath);
if (jsonValue == null) {
@@ -191,8 +192,8 @@ public class JsonFunctions {
/**
* Extract from Json with path to Double
*/
- @ScalarFunction
- public static double jsonPathDouble(Object object, String jsonPath, double
defaultValue) {
+ @ScalarFunction(nullableParameters = true)
+ public static double jsonPathDouble(@Nullable Object object, String
jsonPath, double defaultValue) {
try {
Object jsonValue = jsonPath(object, jsonPath);
if (jsonValue == null) {
diff --git
a/pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsTest.java
b/pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsTest.java
index 375e186..585afc9 100644
---
a/pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsTest.java
+++
b/pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsTest.java
@@ -260,7 +260,7 @@ public class JsonFunctionsTest {
public static Object[][] jsonPathStringTestCases() {
return new Object[][]{
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")),
"$.foo", "x"},
- {ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")),
"$.qux", null},
+ {ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")),
"$.qux", "null"},
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")),
"$.bar", "{\"foo\":\"y\"}"},
};
}
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/data/function/DateTimeFunctionsTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/data/function/DateTimeFunctionsTest.java
index e291c7f..343dabc 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/data/function/DateTimeFunctionsTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/data/function/DateTimeFunctionsTest.java
@@ -251,6 +251,13 @@ public class DateTimeFunctionsTest {
"fromDateTime(dateTime, 'EEE MMM dd HH:mm:ss ZZZ yyyy')",
Lists.newArrayList("dateTime"), row112, 1251142606000L
});
+ // fromDateTime with null
+ GenericRow row113 = new GenericRow();
+ row113.putValue("dateTime", null);
+ inputs.add(new Object[]{
+ "fromDateTime(dateTime, 'yyyy-MM-dd''T''HH:mm:ss.SSS''Z''')",
Lists.newArrayList("dateTime"), row113, null
+ });
+
// timezone_hour and timezone_minute
List<String> expectedArguments = Collections.singletonList("tz");
GenericRow row120 = new GenericRow();
@@ -326,6 +333,14 @@ public class DateTimeFunctionsTest {
inputs.add(new Object[]{"second(millis, tz)", expectedArguments, row131,
13});
inputs.add(new Object[]{"millisecond(millis, tz)", expectedArguments,
row131, 123});
+ GenericRow row140 = new GenericRow();
+ row140.putValue("duration", null);
+ inputs.add(new Object[]{"ago(duration)", Lists.newArrayList("duration"),
row140, null});
+
+ GenericRow row141 = new GenericRow();
+ row141.putValue("timezoneId", null);
+ inputs.add(new Object[]{"timezoneHour(timezoneId)",
Lists.newArrayList("timezoneId"), row141, null});
+
return inputs.toArray(new Object[0][]);
}
@@ -426,6 +441,8 @@ public class DateTimeFunctionsTest {
testDateTimeConvert(5019675L/* 20170920T03:15:00 */, "5:MINUTES:EPOCH",
"1:MILLISECONDS:EPOCH", "1:HOURS",
1505901600000L/* 20170920T03:00:00 */);
+ testDateTimeConvert(null, "5:MINUTES:EPOCH", "1:MILLISECONDS:EPOCH",
"1:HOURS", null);
+
// EPOCH to SDF
// Test conversion from millis since epoch to simple date format (UTC)
testDateTimeConvert(1505985360000L/* 20170921T02:16:00 */,
"1:MILLISECONDS:EPOCH",
@@ -524,6 +541,6 @@ public class DateTimeFunctionsTest {
row.putValue("timeCol", timeValue);
List<String> arguments = Collections.singletonList("timeCol");
testFunction(String.format("dateTimeConvert(timeCol, '%s', '%s', '%s')",
inputFormatStr, outputFormatStr,
- outputGranularityStr), arguments, row, expectedResult.toString());
+ outputGranularityStr), arguments, row, expectedResult == null ? null :
expectedResult.toString());
}
}
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
index 6e1da8e..3663118 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
@@ -18,6 +18,7 @@
*/
package org.apache.pinot.core.data.function;
+import java.lang.reflect.Method;
import java.util.Collections;
import org.apache.pinot.common.function.FunctionRegistry;
import org.apache.pinot.segment.local.function.InbuiltFunctionEvaluator;
@@ -25,8 +26,8 @@ import org.apache.pinot.spi.data.readers.GenericRow;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
import static org.testng.Assert.assertTrue;
-import static org.testng.Assert.fail;
public class InbuiltFunctionEvaluatorTest {
@@ -111,7 +112,8 @@ public class InbuiltFunctionEvaluatorTest {
public void testStateSharedBetweenRowsForExecution()
throws Exception {
MyFunc myFunc = new MyFunc();
-
FunctionRegistry.registerFunction(myFunc.getClass().getDeclaredMethod("appendToStringAndReturn",
String.class));
+ Method method =
myFunc.getClass().getDeclaredMethod("appendToStringAndReturn", String.class);
+ FunctionRegistry.registerFunction(method, false);
String expression = "appendToStringAndReturn('test ')";
InbuiltFunctionEvaluator evaluator = new
InbuiltFunctionEvaluator(expression);
assertTrue(evaluator.getArguments().isEmpty());
@@ -122,20 +124,19 @@ public class InbuiltFunctionEvaluatorTest {
}
@Test
- public void testExceptionDuringInbuiltFunctionEvaluator()
- throws Exception {
- String expression = "fromDateTime(reverse('2020-01-01T00:00:00Z'),
\"invalid_identifier\")";
- InbuiltFunctionEvaluator evaluator = new
InbuiltFunctionEvaluator(expression);
- assertEquals(evaluator.getArguments().size(), 1);
- GenericRow row = new GenericRow();
- try {
- evaluator.evaluate(row);
- fail();
- } catch (Exception e) {
- // assert that exception contains the full function call signature
-
assertTrue(e.toString().contains("fromDateTime(reverse('2020-01-01T00:00:00Z'),invalid_identifier)"));
- // assert that FunctionInvoker ISE is captured correctly.
- assertTrue(e.getCause() instanceof IllegalStateException);
+ public void testNullReturnedByInbuiltFunctionEvaluatorThatCannotTakeNull() {
+ String[] expressions = {
+ "fromDateTime(\"NULL\", 'yyyy-MM-dd''T''HH:mm:ss.SSS''Z''')",
+ "fromDateTime(\"invalid_identifier\",
'yyyy-MM-dd''T''HH:mm:ss.SSS''Z''')",
+ "toDateTime(1648010797, \"invalid_identifier\",
\"invalid_identifier\")",
+ "toDateTime(\"invalid_identifier\", \"invalid_identifier\",
\"invalid_identifier\")",
+ "toDateTime(\"NULL\", \"invalid_identifier\", \"invalid_identifier\")",
+ "toDateTime(\"invalid_identifier\", \"NULL\", \"invalid_identifier\")"
+ };
+ for (String expression : expressions) {
+ InbuiltFunctionEvaluator evaluator = new
InbuiltFunctionEvaluator(expression);
+ GenericRow row = new GenericRow();
+ assertNull(evaluator.evaluate(row));
}
}
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
index 9ec6f6e..be17cb7 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
@@ -108,11 +108,13 @@ public class InbuiltFunctionEvaluator implements
FunctionEvaluator {
private static class FunctionExecutionNode implements ExecutableNode {
final FunctionInvoker _functionInvoker;
+ final FunctionInfo _functionInfo;
final ExecutableNode[] _argumentNodes;
final Object[] _arguments;
FunctionExecutionNode(FunctionInfo functionInfo, ExecutableNode[]
argumentNodes) {
_functionInvoker = new FunctionInvoker(functionInfo);
+ _functionInfo = functionInfo;
_argumentNodes = argumentNodes;
_arguments = new Object[_argumentNodes.length];
}
@@ -124,6 +126,15 @@ public class InbuiltFunctionEvaluator implements
FunctionEvaluator {
for (int i = 0; i < numArguments; i++) {
_arguments[i] = _argumentNodes[i].execute(row);
}
+ if (!_functionInfo.hasNullableParameters()) {
+ // Preserve null values during ingestion transformation if function
is an inbuilt
+ // scalar function that cannot handle nulls, and invoked with null
parameter(s).
+ for (Object argument : _arguments) {
+ if (argument == null) {
+ return null;
+ }
+ }
+ }
_functionInvoker.convertTypes(_arguments);
return _functionInvoker.invoke(_arguments);
} catch (Exception e) {
@@ -138,6 +149,15 @@ public class InbuiltFunctionEvaluator implements
FunctionEvaluator {
for (int i = 0; i < numArguments; i++) {
_arguments[i] = _argumentNodes[i].execute(values);
}
+ if (!_functionInfo.hasNullableParameters()) {
+ // Preserve null values during ingestion transformation if function
is an inbuilt
+ // scalar function that cannot handle nulls, and invoked with null
parameter(s).
+ for (Object argument : _arguments) {
+ if (argument == null) {
+ return null;
+ }
+ }
+ }
_functionInvoker.convertTypes(_arguments);
return _functionInvoker.invoke(_arguments);
} catch (Exception e) {
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/annotations/ScalarFunction.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/annotations/ScalarFunction.java
index 8cc1e71..6963c21 100644
---
a/pinot-spi/src/main/java/org/apache/pinot/spi/annotations/ScalarFunction.java
+++
b/pinot-spi/src/main/java/org/apache/pinot/spi/annotations/ScalarFunction.java
@@ -49,4 +49,7 @@ public @interface ScalarFunction {
// 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 expects and can handle null arguments.
+ boolean nullableParameters() default false;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
