[GitHub] drill issue #671: DRILL-4347: Propagate distinct row count for joins from lo...

2016-11-29 Thread jacques-n
Github user jacques-n commented on the issue: https://github.com/apache/drill/pull/671 Random question: won't a caching metadata provider achieve the same thing without having to do per rel node changes? At least that is what I thought it was for. Maybe @julianhyde can provide

[GitHub] drill issue #586: DRILL-4886: Merge maprdb format plugin source code

2016-09-11 Thread jacques-n
Github user jacques-n commented on the issue: https://github.com/apache/drill/pull/586 LGTM +1 --- 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

[GitHub] drill issue #567: DRILL-4586: Add ErrorType#CLIENT to UserException for clie...

2016-08-31 Thread jacques-n
Github user jacques-n commented on the issue: https://github.com/apache/drill/pull/567 Since it is all on the client/in the same jvm, why not just throw an exception and catch it? No need to jump through all the hoops that UserException goes through since you're never trying

[GitHub] drill issue #567: DRILL-4586: Add ErrorType#CLIENT to UserException for clie...

2016-08-31 Thread jacques-n
Github user jacques-n commented on the issue: https://github.com/apache/drill/pull/567 It seems error-prone/dangerous to change the wire protocol for something that will never be sent on the wire. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] drill issue #567: DRILL-4586: Add ErrorType#CLIENT to UserException for clie...

2016-08-31 Thread jacques-n
Github user jacques-n commented on the issue: https://github.com/apache/drill/pull/567 This also breaks client/wire compatibility. In previous discussions, we said that we would avoid breaking compatibility. If you proceed with this, you'll need to add a compatibility layer

[GitHub] drill pull request #:

2016-06-13 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/commit/323849e48fed967d43108e5cf85ba150a50f073e#commitcomment-17843683 In contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePluginConfig.java: In contrib/storage

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-06-01 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/443 Hahaha! Well at least I'm consistent :P --- 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

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-05-31 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/443 One comment about jcl, otherwise lgtm +1 --- 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

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-05-31 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r65300019 --- Diff: contrib/storage-hbase/pom.xml --- @@ -77,6 +77,14 @@ 2.1.1 test + + commons-logging --- End

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-05-29 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r65015118 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePlugin.java --- @@ -21,34 +21,36 @@ import java.util.Set

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-05-29 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r65015084 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseConnectionManager.java --- @@ -0,0 +1,92 @@ +/** + * Licensed

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-05-29 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r65014981 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java --- @@ -175,7 +179,7 @@ public boolean

[GitHub] drill pull request: DRILL-4676: Foreman.moveToState can block fore...

2016-05-15 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/503#discussion_r63294968 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java --- @@ -115,6 +115,8 @@ private static final ObjectMapper

[GitHub] drill pull request: DRILL-4455: Depend on Apache Arrow

2016-04-26 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/398#issuecomment-214922608 Sounds like we need to do some more work to mature and stabilize the Arrow code. Let's pick this up again once we've made more progress on that front. --- If your

[GitHub] drill pull request: DRILL-3317: when ProtobufLengthDecoder couldn'...

2016-04-22 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/446#issuecomment-213647541 Does this get propated as a connection level failure or an RpcRemoteException. (E.g. do we break connection once this happens?) Seems like it would be better

[GitHub] drill pull request: DRILL-3317: when ProtobufLengthDecoder couldn'...

2016-04-22 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/446#issuecomment-213647142 Great, thanks! --- 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

[GitHub] drill pull request: DRILL-4573: Zero copy LIKE, REGEXP_MATCHES, SU...

2016-04-22 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/458#issuecomment-213646957 Yes, we'll get it in there. I was thinking that Hsuan was moving this along. Let me see if one of us can get this in. The contribution is very much appreciated. Sorry

[GitHub] drill pull request: DRILL-4623: Disable epoll transport by default

2016-04-21 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/486#issuecomment-213209764 btw, +1. Thanks for getting this in. --- 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

[GitHub] drill pull request: DRILL-4623: Disable epoll transport by default

2016-04-21 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/486#issuecomment-213209699 I'm inclined not to hardcode. This solve the issue. You may want to add a note to the upgrade documentation that people need to start with the drill-env.sh base

[GitHub] drill pull request: DRILL-3317: when ProtobufLengthDecoder couldn'...

2016-04-21 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/446#issuecomment-213159458 +1 on this. Can you just confirm the behavior you're expecting in the out of memory situation? --- If your project is set up for it, you can reply to this email

[GitHub] drill pull request: DRILL-3317: when ProtobufLengthDecoder couldn'...

2016-04-20 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/446#issuecomment-212587912 Thanks for trying to put together a test. Can you include the fix to correct the issue about the root allocator as well. I haven't had a chance to look

[GitHub] drill pull request: DRILL-4577: Construct a specific path for quer...

2016-04-14 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/461#issuecomment-210167874 Isn't 8 seconds still too long? Remember that the moment we connect Tableau, it will frequently (via the ODBC driver) run several information schema queries on nearly

[GitHub] drill pull request: Drill 4581

2016-04-13 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/477#issuecomment-209730105 Once you've gone through reviews, you should also collapse your change into a single commit and name it using the pattern below. This allows a committer to make sure

[GitHub] drill pull request: Drill 4581

2016-04-13 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/477#issuecomment-209714087 A quick question and an fyi. (I haven't looked at the details of the patch.) q) Does this change the external interface of the scripts. If so, we should

[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...

2016-04-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/463#discussion_r59631885 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java --- @@ -282,15 +283,19 @@ public void interrupted(final InterruptedException ex

[GitHub] drill pull request: DRILL-4576: Add PlannerCallback interface for ...

2016-04-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/466#discussion_r59574906 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/PlannerCallback.java --- @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache

[GitHub] drill pull request: DRILL-4574: Avro Plugin: Flatten does not work...

2016-04-13 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/459#issuecomment-209521505 Nice first patch. Thanks for the contribution. +1 Can you collapse this into a single commit and make sure the commit message matches the Drill style

[GitHub] drill pull request: DRILL-4571: Add link to local Drill logs from ...

2016-04-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/472#discussion_r59571508 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java --- @@ -303,4 +303,9 @@ StringValidator

[GitHub] drill pull request: DRILL-4576: Add PlannerCallback interface for ...

2016-04-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/466#discussion_r59570703 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/PlannerCallback.java --- @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache

[GitHub] drill pull request: DRILL-4576: Add PlannerCallback interface for ...

2016-04-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/466#discussion_r59570591 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/PlannerCallback.java --- @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache

[GitHub] drill pull request: DRILL-4576: Add PlannerCallback interface for ...

2016-04-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/466#discussion_r59570320 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/PlannerCallback.java --- @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache

[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...

2016-04-12 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/463#discussion_r59445000 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java --- @@ -84,7 +115,7 @@ public void operationComplete(ChannelFuture future

[GitHub] drill pull request: DRILL-4596: Drill should do version check amon...

2016-04-12 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/474#issuecomment-209081311 In general, I'm against version number checking. We did that in the code early on but we should be moving towards a capabilities flag approach. Also agree

[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...

2016-04-12 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/463#discussion_r59434232 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java --- @@ -20,51 +20,82 @@ import io.netty.buffer.ByteBuf; import

[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...

2016-04-12 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/463#discussion_r59433373 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java --- @@ -20,51 +20,82 @@ import io.netty.buffer.ByteBuf; import

[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...

2016-04-12 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/463#discussion_r59433244 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RequestIdMap.java --- @@ -20,51 +20,82 @@ import io.netty.buffer.ByteBuf; import

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-04-07 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r58974382 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePlugin.java --- @@ -19,36 +19,75 @@ import

[GitHub] drill pull request: DRILL-4237 DRILL-4478 fully implement hash to ...

2016-04-07 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/430#issuecomment-207147567 Just to clarify since I didn't say so. I'm ok with this to be merged given Aman's +1. --- If your project is set up for it, you can reply to this email and have your

[GitHub] drill pull request: DRILL-4237 DRILL-4478 fully implement hash to ...

2016-04-07 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/430#issuecomment-207147616 Did you guys do a perf analysis to ensure no regressions? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] drill pull request: DRILL-4539: Add support for Null Equality Join...

2016-04-05 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/462#discussion_r58645138 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java --- @@ -169,4 +176,223 @@ private static boolean

[GitHub] drill pull request: DRILL-4539: Add support for Null Equality Join...

2016-04-05 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/462#issuecomment-206092089 @amansinha100, can you take a look? --- 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

[GitHub] drill pull request: DRILL-4576: Add PlannerCallback interface for ...

2016-04-05 Thread jacques-n
GitHub user jacques-n opened a pull request: https://github.com/apache/drill/pull/466 DRILL-4576: Add PlannerCallback interface for additional planner initialization. DRILL-4576: Add PlannerCallback interface for additional planner initialization. - Allow a storage plugin

[GitHub] drill pull request: DRILL-4446: Support mandatory work assignment ...

2016-04-05 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/403#issuecomment-206039007 This looks good to me. Let's get this merged. +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] drill pull request: DRILL-4132 Ability to submit simple type of ph...

2016-04-05 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/368#discussion_r58633147 --- Diff: protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java --- @@ -133,6 +133,10 @@ private RpcChannel(int index, int value

[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...

2016-04-05 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/442#issuecomment-206037954 I came up with an alternative approach as I mentioned above. Basically moving coordination management to the RemoteConnection instead of handling at the RpcBus level

[GitHub] drill pull request: DRILL-3714: Avoid cascading disconnection when...

2016-04-05 Thread jacques-n
GitHub user jacques-n opened a pull request: https://github.com/apache/drill/pull/463 DRILL-3714: Avoid cascading disconnection when a single connection is broken DRILL-3714: Avoid cascading disconnection when a single connection is broken - Move the coordination id

[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...

2016-04-05 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/442#discussion_r58611408 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java --- @@ -159,19 +159,15 @@ public ChannelClosedHandler(C clientConnection, Channel

[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...

2016-04-04 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/442#issuecomment-205413400 I'm working on an alternative approach where the coordination queue is associated with each connection. Upon review, it still needs thread protection because you have

[GitHub] drill pull request: DRILL-3714: Query runs out of memory and remai...

2016-04-01 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/442#discussion_r58286360 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/RpcBus.java --- @@ -159,19 +159,15 @@ public ChannelClosedHandler(C clientConnection, Channel

[GitHub] drill pull request: DRILL-4546: Only generate one zip archive when...

2016-04-01 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/449#issuecomment-204617555 Can you confirm that the release artifacts do not change between applying this and not applying it? --- If your project is set up for it, you can reply to this email

[GitHub] drill pull request: DRILL-2100: Added deleting temporary spill dir...

2016-03-31 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/454#discussion_r58060244 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java --- @@ -110,11 +111,12 @@ private LinkedList

[GitHub] drill pull request: DRILL-4237 DRILL-4478 fully implement hash to ...

2016-03-30 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/430#issuecomment-203726680 Did you consider leveraging this library: https://github.com/OpenHFT/Zero-Allocation-Hashing --- If your project is set up for it, you can reply

[GitHub] drill pull request: DRILL-4544: Improve error messages for REFRESH...

2016-03-29 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/448#issuecomment-203005531 Let's open a follow-up bug to move this to Calcite and get in Drill for now. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] drill pull request: DRILL-3317: when ProtobufLengthDecoder couldn'...

2016-03-26 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/446#discussion_r57518531 --- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/ProtobufLengthDecoder.java --- @@ -82,15 +79,7 @@ protected void decode(ChannelHandlerContext ctx

[GitHub] drill pull request: DRILL-1328: Support table statistics

2016-03-26 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/425#issuecomment-201910127 ObjectHolder should be used cautiously. That is why it is marked as deprecated. I'm pretty sure it was deprecated when we first added it. In order to hold complex

[GitHub] drill pull request: DRILL-4531: Add a Drill customized rule for pu...

2016-03-25 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/444#issuecomment-201505793 Yeah, you're right and thanks for reviewing fully. We can shelve for now but this definitely seems like something we should get to. +1 on the patch

[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...

2016-03-25 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/429#discussion_r57475224 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java --- @@ -50,8 +52,12 @@ public BootStrapContext(DrillConfig

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-03-24 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r57384658 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java --- @@ -25,36 +25,40 @@ import

[GitHub] drill pull request: DRILL-4531: Add a Drill customized rule for pu...

2016-03-24 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/444#discussion_r57384554 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillFilterAggregateTransposeRule.java --- @@ -0,0 +1,28

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-03-24 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r57379100 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java --- @@ -25,36 +25,40 @@ import

[GitHub] drill pull request: DRILL-4525: Allow SqlBetweenOperator to accept...

2016-03-24 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/439#issuecomment-200918110 Got it. We should open a discussion on this on the Calcite list. We shouldn't have to jump through hoops to change the implementation. Maybe the overridden operator

[GitHub] drill pull request: DRILL-4525: Allow SqlBetweenOperator to accept...

2016-03-24 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/439#issuecomment-200902815 I'm confused by a couple things. One, it sounds there is an issue with the convertlet that Sean is trying to manage. Can't we just make are own convertlets? Second

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-03-23 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/443#issuecomment-200592038 I don't know the new semantics of ConnectionFactory. Drill now has plugin lifecycle events so we should close out an hbase connection if a storage plugin is changed

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-03-23 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/443#discussion_r57258088 --- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java --- @@ -25,36 +25,40 @@ import

[GitHub] drill pull request: DRILL-4199: Add Support for HBase 1.X

2016-03-23 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/443#issuecomment-200576391 The jcl-over-slf4j bridge should be all that is necessary. Is HBase doing something weird that means that doesn't work? Or is that just missing for some reason

[GitHub] drill pull request: DRILL-4237 DRILL-4478 fully implement hash to ...

2016-03-21 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/430#discussion_r56925560 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/HashHelper.java --- @@ -17,47 +17,77 @@ */ package

[GitHub] drill pull request: Map variance() and stddev() to var_samp() and ...

2016-03-21 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/437#discussion_r56920816 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestAggregationQueries.java --- @@ -0,0 +1,53 @@ +/** + * Licensed to the Apache Software

[GitHub] drill pull request: DRILL-4514 : Add describe schema ...

2016-03-21 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/436#discussion_r56919932 --- Diff: exec/java-exec/src/main/codegen/includes/parserImpls.ftl --- @@ -278,3 +278,19 @@ SqlNode SqlRefreshMetadata

[GitHub] drill pull request: DRILL-4237 DRILL-4478 fully implement hash to ...

2016-03-21 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/430#discussion_r56907606 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/HashHelper.java --- @@ -17,47 +17,77 @@ */ package

[GitHub] drill pull request: DRILL-4504: Create an event loop for each of [...

2016-03-14 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/429#discussion_r56082440 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java --- @@ -74,73 +74,148 @@ /** * Thin wrapper around

[GitHub] drill pull request: Drill 4504: Create an event loop for each of [...

2016-03-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/429#discussion_r55952628 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/client/DrillClientSystemTest.java --- @@ -51,7 +51,7 @@ public void tearDownTest

[GitHub] drill pull request: Drill 4504: Create an event loop for each of [...

2016-03-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/429#discussion_r55952611 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java --- @@ -50,8 +52,12 @@ public BootStrapContext(DrillConfig

[GitHub] drill pull request: DRILL-4503: Write nulls as varchar when all_te...

2016-03-13 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/428#discussion_r55946493 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -402,7 +402,8 @@ private void writeDataAllText

[GitHub] drill pull request: DRILL-4493 - Fixed issues in various POMs with...

2016-03-09 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/421#discussion_r55566774 --- Diff: exec/jdbc/pom.xml --- @@ -134,6 +134,7 @@ org.apache.hadoop hadoop-common + test

[GitHub] drill pull request: DRILL-4474: Ensure that ConvertCountToDirectSc...

2016-03-08 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/416#discussion_r55455592 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/ConvertCountToDirectScan.java --- @@ -117,6 +117,24 @@ public void onMatch

[GitHub] drill pull request: DRILL-4474: Ensure that ConvertCountToDirectSc...

2016-03-08 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/416#discussion_r55453543 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/ConvertCountToDirectScan.java --- @@ -117,6 +117,24 @@ public void onMatch

[GitHub] drill pull request: DRILL-4474: Use varchar for default column whe...

2016-03-08 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/415#issuecomment-193880193 Can you generate the test file as part of the test rather than check in static? --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] drill pull request: DRILL-4281: Support authorized proxy users to ...

2016-03-05 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/400#issuecomment-192755393 lg. best to separate test operations into separate tests. Otherwise let's get this merged. --- If your project is set up for it, you can reply to this email and have

[GitHub] drill pull request: DRILL-4474: Ensure that ConvertCountToDirectSc...

2016-03-04 Thread jacques-n
GitHub user jacques-n opened a pull request: https://github.com/apache/drill/pull/406 DRILL-4474: Ensure that ConvertCountToDirectScan only pushes through project when project is trivial. There are probably additional optimizations here but as the code stood, we completely ignored

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54990639 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/util/UserDelegationUtil.java --- @@ -0,0 +1,147 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/400#issuecomment-192046477 Generally looks good. +1 with the few small items above addressed. Updating the names to something else would be good. Since this is also impersonation (just client

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54977484 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/delegation/TestUserDelegation.java --- @@ -0,0 +1,124 @@ +/** + * Licensed

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54977306 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/delegation/TestDelegationPrivileges.java --- @@ -0,0 +1,137 @@ +/** + * Licensed

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54977178 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/util/UserDelegationUtil.java --- @@ -0,0 +1,147 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54977126 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/util/UserDelegationUtil.java --- @@ -0,0 +1,147 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54976899 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java --- @@ -116,14 +137,38 @@ public OptionManager getOptions

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/400#discussion_r54976762 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java --- @@ -88,6 +89,7 @@ String USER_AUTHENTICATION_ENABLED

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-03 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/400#issuecomment-191963668 Are delegate and delegator common used terms for these things? Frankly, I would have to look these up to confirm which is which and could see making a mistake

[GitHub] drill pull request: DRILL-4465: Simplify Calcite parsing & plannin...

2016-03-03 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/401#issuecomment-191890837 @jinfengni: I've addressed your review comments. Let me know any additional feedback. thanks! --- If your project is set up for it, you can reply

[GitHub] drill pull request: DRILL-4465: Simplify Calcite parsing & plannin...

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/401#discussion_r54840955 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java --- @@ -273,12 +282,90 @@ public RelNode visit

[GitHub] drill pull request: DRILL-4465: Simplify Calcite parsing & plannin...

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/401#discussion_r54840884 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java --- @@ -273,12 +282,90 @@ public RelNode visit

[GitHub] drill pull request: DRILL-4465: Simplify Calcite parsing & plannin...

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/401#discussion_r54840816 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java --- @@ -273,12 +282,90 @@ public RelNode visit

[GitHub] drill pull request: Drill 4372 review

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/397#discussion_r54765921 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java --- @@ -92,38 +94,110 @@ public DrillFunctionRegistry

[GitHub] drill pull request: Drill 4372 review

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/397#discussion_r54763735 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java --- @@ -92,38 +94,110 @@ public DrillFunctionRegistry

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-02 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/400#issuecomment-191350797 The in-memory representation should be independent of any option parsing. As such, it shouldn't matter what the user interface is. --- If your project is set up

[GitHub] drill pull request: Drill 4372 review

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/397#discussion_r54762638 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java --- @@ -92,38 +94,110 @@ public DrillFunctionRegistry

[GitHub] drill pull request: Drill 4372 review

2016-03-02 Thread jacques-n
Github user jacques-n commented on a diff in the pull request: https://github.com/apache/drill/pull/397#discussion_r54762584 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java --- @@ -92,38 +94,110 @@ public DrillFunctionRegistry

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-02 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/400#issuecomment-191318449 Something like (1). Basically you're using multiple options plus string encoding to structure the configuration information. I'm proposing that we come up

[GitHub] drill pull request: DRILL-4465: Simplify Calcite parsing & plannin...

2016-03-02 Thread jacques-n
GitHub user jacques-n opened a pull request: https://github.com/apache/drill/pull/401 DRILL-4465: Simplify Calcite parsing & planning integration - Canonicalize Planning phases with PlannerPhase enumeration - Canonicalize PlannerType transforms - Remove depend

[GitHub] drill pull request: DRILL-4281: Support authorized users to delega...

2016-03-01 Thread jacques-n
Github user jacques-n commented on the pull request: https://github.com/apache/drill/pull/400#issuecomment-191014420 I think it would be better to have a single delegation setting that is json that describes the entirety of what we need to express. I think that will be much clearer

  1   2   3   4   >