[
https://issues.apache.org/jira/browse/DRILL-5330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15906218#comment-15906218
]
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_r105534393
--- 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. When dynamic udfs are enabled, we first try to find matching function
using exact function resolver `exactResolver.getBestMatch` and if match if not
found check if remote and local function registries are in sync and then try to
find matching function using provided function resolver
`functionResolver.getBestMatch`.
So when dynamic udfs are disabled, we should not try to find matching
function using exact function resolver, we need to use provided one and skip
function registry sync check,
2. Regarding new bootstap option, since we already have system / session
option `ExecConstants.USE_DYNAMIC_UDFS`, I suggest we use it during this check,
first we won't have two similar options, second if we re-write method as
suggested above (point 1), users who disabled dynamic udfs using system /
session option will have performance benefit since we won't double-check
function existence. Probably we should have done this when
`ExecConstants.USE_DYNAMIC_UDFS` was introduced in first place.
> 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)
