[
https://issues.apache.org/jira/browse/DRILL-5330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15907133#comment-15907133
]
ASF GitHub Bot commented on DRILL-5330:
---------------------------------------
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/777#discussion_r105628751
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
---
@@ -160,7 +168,7 @@ public DrillFuncHolder
findDrillFunction(FunctionResolver functionResolver, Func
FunctionResolver exactResolver =
FunctionResolverFactory.getExactResolver(functionCall);
DrillFuncHolder holder = exactResolver.getBestMatch(functions,
functionCall);
- if (holder == null) {
+ if (holder == null && useDynamicUdfs) {
--- End diff --
1. Since you have mentioned I remembered one more issue with
FunctionImplementationRegistry, it can access only system options, so using
`ExecConstants.USE_DYNAMIC_UDFS` won't work properly since it can be set at
session level as well. I guess using bootsrap option you introduced is OK for
now. Regarding your suggestion to have single option OFF, READ_ONLY and ON to
handle the various cases (I love this idea!), we can try to implement this the
scope of MVCC (I'll add this point to the document).
2. Even with boostrap option we need to update `findDrillFunction` to use
provided function resolver when dynamic udfs are turned off (more details in my
first comment). For example, `findDrillFunction` should can be re-written the
following way (please optimize if needed):
```java
public DrillFuncHolder findDrillFunction(FunctionResolver functionResolver,
FunctionCall functionCall) {
AtomicLong version = new AtomicLong();
String newFunctionName = functionReplacement(functionCall);
List<DrillFuncHolder> functions =
localFunctionRegistry.getMethods(newFunctionName, version);
if (!useDynamicUdfs) {
return functionResolver.getBestMatch(functions, functionCall);
}
FunctionResolver exactResolver =
FunctionResolverFactory.getExactResolver(functionCall);
DrillFuncHolder holder = exactResolver.getBestMatch(functions,
functionCall);
if (holder == null) {
syncWithRemoteRegistry(version.get());
List<DrillFuncHolder> updatedFunctions =
localFunctionRegistry.getMethods(newFunctionName, version);
holder = functionResolver.getBestMatch(updatedFunctions,
functionCall);
}
return holder;
}
```
3. Also changes should be done in `findExactMatchingDrillFunction` method
to take into account boostrap option as well. For example (please optimize if
needed):
```java
public DrillFuncHolder findExactMatchingDrillFunction(String name,
List<MajorType> argTypes, MajorType returnType) {
if (useDynamicUdfs) {
return findExactMatchingDrillFunction(name, argTypes, returnType,
true);
}
return findExactMatchingDrillFunction(name, argTypes, returnType,
false);
}
```
> NPE in FunctionImplementationRegistry.functionReplacement()
> -----------------------------------------------------------
>
> Key: DRILL-5330
> URL: https://issues.apache.org/jira/browse/DRILL-5330
> Project: Apache Drill
> Issue Type: Bug
> Affects Versions: 1.10.0
> Reporter: Paul Rogers
> Assignee: Paul Rogers
> Fix For: 1.11.0
>
>
> The code in {{FunctionImplementationRegistry.functionReplacement()}} will
> produce an NPE if ever it is called:
> {code}
> if (optionManager != null
> && optionManager.getOption(
> ExecConstants.CAST_TO_NULLABLE_NUMERIC).bool_val
> ...
> {code}
> If an option manager is provided, then get the specified option. The option
> manager will contain a value for that option only if the user has explicitly
> set that option. Suppose the user had not set the option. Then the return
> from {{getOption()}} will be null.
> The next thing we do is *assume* that the option exists and is a boolean by
> dereferencing the option. This will trigger an NPE. This NPE was seen in
> detail-level unit tests.
> The proper way to handle such options is to use an option validator.
> Strangely, one actually exists in {{ExecConstants}}:
> {code}
> String CAST_TO_NULLABLE_NUMERIC =
> "drill.exec.functions.cast_empty_string_to_null";
> OptionValidator CAST_TO_NULLABLE_NUMERIC_OPTION = new
> BooleanValidator(CAST_TO_NULLABLE_NUMERIC, false);
> {code}
> Then do:
> {code}
> optionManager.getOption(
> ExecConstants.CAST_TO_NULLABLE_NUMERIC_OPTION)
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
