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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
