drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From paul-rogers <...@git.apache.org>
Subject [GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
Date Sat, 11 Mar 2017 18:37:45 GMT
Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/777#discussion_r105539404
  
    --- 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 --
    
    Taking a step back, here is what we're trying to accomplish. When running unit tests,
the DUDF mechanism is not available -- or needed. So, we want to disable the code path that
uses DUDFs.
    
    The original version of this code causes an NPE when calling the code inside the if. Now,
I don't fully understand what how all this works. So, if the non-DUDF check is to early, where
should I move the check so we do the necessary local lookups but bypass the DUDF code?
    
    I agree that having three distinct options to disable DUDFs is excessive. (This new boot
option and two system options.) As Padma explained, the two system options disable adding
DUDFs but do not disable DUDF lookup. The stated reason is that the user may have DUDFs on
the system, so disabling the DUDF function can't result in those functions becoming unavailable.
And, because of the race conditions we've discussed, that lazy init check still has to be
done, even with DUDFs are off. So, we need yet another option, only for testing, that "really"
turns DUDFs off.
    
    Now, this is a murky state of affairs, so it would be better to have a single option,
maybe with three values: OFF, READ_ONLY and ON to handle the various cases.
    
    The boot option is also an optimization: it says that the option can be set only at boot
time, so there is no need to do an option lookup on every function resolution.
    
    Finally, it turns out that, in the constructor, the option manager is not yet initialized
and so can't be accessed. As a result, we can't cache the value of a system option from the
constructor.
    
    All in all, I'm open to any revision of this change which simply disables DUDFs during
unit testing (except, of course, when we are testing DUDFs...)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message