[GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/777#discussion_r105540304
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
---
@@ -133,7 +134,7 @@ public void resolveHash(DrillConfig config,
LogicalExpression arg, TypeProtos.Ma
FunctionImplementationRegistry
registry) throws JClassAlreadyExistsException, IOException {
final List args = new ArrayList<>();
args.add(arg);
-final String[] registeredNames = { "hash" };
+//final String[] registeredNames = { "hash" };
--- End diff --
Same reason. Removed this and the previous commented-out code.
---
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.
---
Re: [VOTE] Release Apache Drill 1.10.0 - rc0
+1 - Downloaded tarball and started drill in embedded mode on Windows. Run simple queries, checked CTTAS and dynamic UDFs features. - Downloaded from the source on Linux VM and did build with unit tests. - Deployed on 4 node cluster, run tests from test framework, executed simple queries. Kind regards Arina On Sat, Mar 11, 2017 at 4:09 AM, Aman Sinha wrote: > Never mind. Release note link seems fine. > > On 3/10/17, 6:06 PM, "Aman Sinha" wrote: > > +1 (binding) > > - Downloaded the binaries on my mac, checked the git.properties and > KEYS file (for PGP key) > - Started Drill in embedded mode and ran a few sample queries with and > without metadata cache. Checked query profiles. > -Tested query cancellation for couple of queries > - Downloaded source on my Linux VM, did a build and ran unit tests > successfully. > - Checked Maven artifacts on repository.apache.org > > Release notes link in [1] seems broken. Can you provide the correct > link ? > > -Aman > > On 3/10/17, 11:44 AM, "Jinfeng Ni" wrote: > > Agreed that DRILL-5338 is not a blocker for the release. > > Just a friendly reminder. Vote ends on Monday 8:00AM. Please try > out > RC0, and cast your vote before it ends. > > Thanks, > > Jinfeng > > > On Thu, Mar 9, 2017 at 9:53 AM, Paul Rogers > wrote: > > Added a note to the JIRA. > > > > Yes, I had asked Kunal to provide some visual indication that > the detail values are different parts of the total run time. Neither of us > are UI designers, so if anyone has a better visual solution, please let us > know! > > > > This should not be a blocker. > > > > Thanks, > > > > - Paul > > > >> On Mar 9, 2017, at 8:30 AM, Zelaine Fong > wrote: > >> > >> This looks like it’s due to the changes Kunal made in > https://issues.apache.org/jira/browse/DRILL-5190. Looking at the code > changes, it appears that the indentation was intended, as it denotes that > those indented timings are a breakdown of the overall duration. > >> > >> Kunal? > >> > >> -- Zelaine > >> > >> On 3/9/17, 8:20 AM, "Arina Yelchiyeva" < > arina.yelchiy...@gmail.com> wrote: > >> > >>So far found one issue: > >> > >>https://issues.apache.org/jira/browse/DRILL-5338 > >> > >> > >>Kind regards > >>Arina > >> > >>On Thu, Mar 9, 2017 at 9:31 AM, Jinfeng Ni > wrote: > >> > >>> Hello all, > >>> > >>> I'd like to propose the first release candidate (rc0) of > Apache Drill, > >>> version 1.10.0. > >>> > >>> The release candidate covers a total of 126 resolved JIRAs > [1]. Thanks > >>> to everyone who contributed to this release. > >>> > >>> The tarball artifacts are hosted at [2] and the maven > artifacts are > >>> hosted at [3]. > >>> > >>> This release candidate is based on commit > >>> 38ef562b1ced59efe57b0dc606f2c36694569102 located at [4]. > >>> > >>> The vote will be open for the next ~96 hours (including an > extra day > >>> as the vote is happening over a weekend) ending at 8:00AM > Pacific, > >>> March 13th, 2017. > >>> > >>> [ ] +1 > >>> [ ] +0 > >>> [ ] -1 > >>> > >>> Here's my vote: +1 > >>> > >>> Thanks, > >>> > >>> Jinfeng > >>> > >>> 1. https://issues.apache.org/jira/secure/ReleaseNote.jspa? > >>> version=12338769&projectId=12313820 > >>> 2. http://home.apache.org/~jni/drill/1.10.0/rc0/ > >>> 3. https://repository.apache.org/content/repositories/ > orgapachedrill-1041/ > >>> 4. https://github.com/jinfengni/incubator-drill/commits/drill- > 1.10.0 > >>> > >> > >> > > > > > > >
[GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/777#discussion_r105539420
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
---
@@ -62,10 +62,11 @@
public class TestSimpleFunctions extends ExecTest {
//private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(TestSimpleFunctions.class);
- private final DrillConfig c = DrillConfig.create();
+// private final DrillConfig c = DrillConfig.create();
--- End diff --
I can remove it. I initially commented it just to see if things would still
work.
---
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.
---
[GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
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.
---
[GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/777#discussion_r105534413
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
---
@@ -133,7 +134,7 @@ public void resolveHash(DrillConfig config,
LogicalExpression arg, TypeProtos.Ma
FunctionImplementationRegistry
registry) throws JClassAlreadyExistsException, IOException {
final List args = new ArrayList<>();
args.add(arg);
-final String[] registeredNames = { "hash" };
+//final String[] registeredNames = { "hash" };
--- End diff --
Why we need to comment this? If it's not need, can we just remove it?
---
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.
---
[GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/777#discussion_r105534412
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java
---
@@ -62,10 +62,11 @@
public class TestSimpleFunctions extends ExecTest {
//private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(TestSimpleFunctions.class);
- private final DrillConfig c = DrillConfig.create();
+// private final DrillConfig c = DrillConfig.create();
--- End diff --
Why we need to comment this? If it's not need, can we just remove it?
---
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.
---
[GitHub] drill pull request #777: DRILL-5330: NPE in FunctionImplementationRegistry
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.
---
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.
---
