Copilot commented on code in PR #18115:
URL: https://github.com/apache/pinot/pull/18115#discussion_r3044185214
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java:
##########
@@ -290,32 +284,133 @@ public Object execute(GenericRow row) {
@Override
public Object execute(Object[] values) {
try {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
+ for (int i = 0; i < _argumentNodes.length; 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;
- }
+ return invokeResolvedFunction();
+ } catch (Exception e) {
+ throw new RuntimeException("Caught exception while executing function:
" + this + ": " + e.getMessage(), e);
+ }
+ }
+
+ private Object invokeResolvedFunction() {
+ ResolvedFunction resolvedFunction = resolveFunction();
+ if (resolvedFunction == null) {
+ if (hasNullArgument()) {
+ return null;
+ }
+ throw new IllegalStateException(String.format("Unsupported function:
%s with argument types: %s",
+ _functionName, Arrays.toString(getArgumentTypeNames())));
+ }
+ if (!resolvedFunction._functionInfo.hasNullableParameters() &&
hasNullArgument()) {
+ return null;
+ }
+ if (resolvedFunction._functionInvoker.getMethod().isVarArgs()) {
+ return resolvedFunction._functionInvoker.invoke(new
Object[]{_arguments});
+ }
+ resolvedFunction._functionInvoker.convertTypes(_arguments);
+ return resolvedFunction._functionInvoker.invoke(_arguments);
+ }
+
+ private boolean hasNullArgument() {
+ for (Object argument : _arguments) {
+ if (argument == null) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private String[] getArgumentTypeNames() {
+ String[] argumentTypeNames = new String[_arguments.length];
+ for (int i = 0; i < _arguments.length; i++) {
+ Object argument = _arguments[i];
+ argumentTypeNames[i] = argument != null ?
argument.getClass().getSimpleName() : "null";
+ }
+ return argumentTypeNames;
+ }
+
+ private ResolvedFunction resolveFunction() {
+ ColumnDataType[] argumentTypes = getArgumentTypes();
+ if (argumentTypes != null) {
+ ArgumentTypesKey argumentTypesKey = new
ArgumentTypesKey(argumentTypes);
+ ResolvedFunction resolvedFunction =
_resolvedFunctionCache.get(argumentTypesKey);
+ if (resolvedFunction == null) {
+ FunctionInfo functionInfo =
FunctionRegistry.lookupFunctionInfo(_canonicalName, argumentTypes);
+ if (functionInfo != null) {
+ resolvedFunction = new ResolvedFunction(functionInfo);
+ _resolvedFunctionCache.put(argumentTypesKey, resolvedFunction);
}
}
- if (_functionInvoker.getMethod().isVarArgs()) {
- return _functionInvoker.invoke(new Object[]{_arguments});
+ if (resolvedFunction != null) {
+ _lastResolvedFunction = resolvedFunction;
+ return resolvedFunction;
}
- _functionInvoker.convertTypes(_arguments);
- return _functionInvoker.invoke(_arguments);
- } catch (Exception e) {
- throw new RuntimeException("Caught exception while executing function:
" + this + ": " + e.getMessage(), e);
}
+ if (_lastResolvedFunction != null) {
+ return _lastResolvedFunction;
+ }
+ if (_fallbackFunction != null) {
+ _lastResolvedFunction = _fallbackFunction;
Review Comment:
`resolveFunction()` falls back to `_lastResolvedFunction` when no overload
matches the current (non-null/known) `argumentTypes`. This can silently invoke
a previously-resolved overload for an unsupported type combination rather than
failing fast. Consider only reusing `_lastResolvedFunction` when argument types
cannot be determined (e.g., null/unknown), and returning `null` (or throwing)
on a lookup miss for concrete `argumentTypes`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java:
##########
@@ -290,32 +284,133 @@ public Object execute(GenericRow row) {
@Override
public Object execute(Object[] values) {
try {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
+ for (int i = 0; i < _argumentNodes.length; 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;
- }
+ return invokeResolvedFunction();
+ } catch (Exception e) {
+ throw new RuntimeException("Caught exception while executing function:
" + this + ": " + e.getMessage(), e);
+ }
+ }
+
+ private Object invokeResolvedFunction() {
+ ResolvedFunction resolvedFunction = resolveFunction();
+ if (resolvedFunction == null) {
Review Comment:
`invokeResolvedFunction()` always calls `resolveFunction()`, which (via
`getArgumentTypes()`) infers argument `ColumnDataType` on every execution. This
adds per-row overhead even for non-polymorphic functions where
`_fallbackFunction` is already fully resolved by argument count. Consider
short-circuiting to `_fallbackFunction` when it’s non-null (or only doing
runtime type-based resolution when `lookupFunctionInfo(canonicalName, numArgs)`
returned null) to avoid regressions in ingestion transform throughput.
```suggestion
// Use the function resolved by name/arity when available to avoid
per-row runtime type inference.
ResolvedFunction resolvedFunction = _fallbackFunction;
if (resolvedFunction == null) {
resolvedFunction = resolveFunction();
}
if (resolvedFunction == null) {
```
--
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]
