Making good progress with the changes requested from the review(s).

- Ankush

On Sun, Apr 19, 2020 at 12:36 AM Charles Givre <cgi...@gmail.com> wrote:

> I don't mean to be a pain in the you know where about this...  I very much
> would like to see this as a part of Drill.
> Best,
> -- C
>
>
>
> On Apr 18, 2020, at 4:54 PM, Ankush Kapur <ankush.ka...@gmail.com> wrote:
>
> Apologies for being MIA, got hung-up at work really bad.
>
> Yes, I did start working on the changes requested in the review. Will act
> on the other things you mentioned, as well.
>
> Thank you for following-up though, helps if someone is watching over :)
>
> - Ankush
>
> On Fri, Apr 17, 2020 at 12:56 AM Charles Givre <cgi...@gmail.com> wrote:
>
>> Hi Ankush,
>> How's it going?  I hope you are doing ok with all that is happening
>> around us.  I hate to be a pest but I was wondering if you've had the
>> chance to work on the Druid storage plugin review?  I'm happy to help with
>> some of the changes, but I didn't want to step on your toes or re-do things
>> you're currently working on.
>>
>> Would it be possible for you to rebase on the current master as well?
>> There have been some improvements unrelated to this plugin which will be
>> helpful as we move towards committing this.
>> Stay safe!
>>
>> -- C
>>
>>
>>
>>
>>
>> On Mar 22, 2020, at 8:34 AM, Ankush Kapur <ankush.ka...@gmail.com> wrote:
>>
>> Yes, thank you. both for your time reviewing...
>>
>> I am working on changes based on it.
>>
>> - Ankush
>>
>> On Fri, Mar 20, 2020 at 11:25 AM Charles Givre <cgi...@gmail.com> wrote:
>>
>>> Hi Ankush.
>>> I hope all is well.  Were you able to see Paul and my review comments
>>> for the Druid plugin?
>>> Thanks,
>>> -- C
>>>
>>> On Mar 13, 2020, at 8:17 AM, Charles Givre <cgi...@gmail.com> wrote:
>>>
>>> HI Ankush,
>>> I hope you're doing ok. I live in Maryland and all our schools are now
>>> closed for the next two weeks, so it will be interesting.... Anyway, I
>>> tagged you in a few comments, but it's weird that my comments didn't show
>>> up on the main page.  Take a look here and you should see them:
>>> https://github.com/apache/drill/pull/1888/files
>>>
>>> General comments:
>>> 1.  Please verify that all logger creations are in the correct format.
>>> Also, I'd strongly suggest avoiding info messages and use either debug,
>>> warn or error as the case may be.
>>> 2.  I had a general question about security and passing credentials to
>>> Druid.  I don't really know how Druid handles authentication, but it didn't
>>> seem like there was any way to pass creds to Druid.  In any event, this
>>> will need to be documented in the README file.
>>> 3.  Please go through and remove any commented out code.
>>>
>>> Writing a storage plugin is not easy, and I'm really impressed that your
>>> first contribution to Drill is something of this scale.  This is really
>>> nice work and I'm really hoping we can get it committed for the next
>>> release.
>>> Thanks,
>>> -- C
>>>
>>>
>>> On Mar 13, 2020, at 5:56 AM, Ankush Kapur <ankush.ka...@gmail.com>
>>> wrote:
>>>
>>> Hi Charles,
>>>
>>> Things are good here, and hope the same for you and your family.
>>>
>>> Pardon me, but I do not see your review comments on the four files
>>> mentioned.
>>>
>>> - Ankush
>>>
>>> On Thu, Mar 12, 2020 at 8:36 PM Charles Givre <cgi...@gmail.com> wrote:
>>>
>>>> Hi Ankush,
>>>> I hope all is well.  I started the review on your plugin.  I've
>>>> reviewed about four files so far.  When you have a chance, please take a
>>>> look at the review comments and if you have any questions, please let me
>>>> know.
>>>> Best,
>>>> -- C
>>>>
>>>>
>>>> On Feb 28, 2020, at 8:33 AM, Charles Givre <cgi...@gmail.com> wrote:
>>>>
>>>> Hi Ankush,
>>>> I hope all is well. I've been speaking with Paul about your plugin and
>>>> I'm going to do the code review.  I usually don't do big code reviews like
>>>> this, so I was hoping someone else would pick it up, but that hasn't
>>>> happened and I really want to see this get committed to the next version of
>>>> Drill.
>>>>
>>>> Since this is a large PR, I'm going to do this in small reviews rather
>>>> than one massive review so please expect waves of review.  I think it's
>>>> best that way it doesn't get overwhelming.  My game plan will be to focus
>>>> on a few files at a time rather than trying to look at the entire plugin
>>>> and give a few comments here and there.  I'll send future correspondence
>>>> via the dev alias in the interest of transparency as well.  I'm going to
>>>> ask Paul to look at the pushdown code as he is a lot more familiar with
>>>> that than I am, but we're not there yet.
>>>>
>>>> Thanks for all your work and patience on this and I am excited about
>>>> getting it into Drill.  I know it will be a benefit for the community.
>>>> Best,
>>>> -- C
>>>>
>>>>
>>>>
>>>> On Jan 21, 2020, at 8:43 AM, Charles Givre <cgi...@gmail.com> wrote:
>>>>
>>>> Hey Ankush,
>>>> See responses inline.
>>>>
>>>>
>>>> On Jan 21, 2020, at 8:12 AM, Ankush Kapur <ankush.ka...@gmail.com>
>>>> wrote:
>>>>
>>>> Hi Charles,
>>>>
>>>> Thanks for taking time to make the changes.
>>>>
>>>>
>>>> No problem.  Also FYI, Drill uses 2 space indents not 4.  I just pushed
>>>> an update with all the spacing on 2 space.  Is there anything that I can
>>>> assist with?
>>>>
>>>>
>>>> Could you please point me to the "commented-out" code you are referring
>>>> too. That's actually an issue i was trying to track down last weekend while
>>>> writing tests. I noticed that the "where" clause was not being pushed down.
>>>> This was keeping me from writing the tests I have been working on.
>>>>
>>>>
>>>> The commented out code was in the DruidStoragePlugin, but it does not
>>>> appear to be commented out anymore.  Have you rebased on the most current
>>>> branch?  When I attempted to build your branch, I got an error in the
>>>> RecordReader class. I'll take another look today or tomorrow.  Regarding
>>>> pushdowns, have you seen the Calcite adapter for Druid (
>>>> https://calcite.apache.org/docs/druid_adapter.html).  I'm wondering if
>>>> you can basically extend or import the pushdowns from the Calcite adapter
>>>> so that you don't have to recreate all that.
>>>>
>>>> I've been working on a pluign for Cassandra and am attempting to do
>>>> that as well. Take a look at the Drill JDBC Storage Plugin because I think
>>>> that's sort of what it does.  Potentially this could save you a lot of
>>>> work.
>>>>
>>>>
>>>> Also, can you provide a link to the file where it's failing to compile
>>>> for you? I was not able to repro this.
>>>>
>>>>
>>>> The specific error I am getting is on line 151 in the
>>>> DruidRecordReader.  The error is that writer is a VectorContainerWriter and
>>>> the required type is a ComplexWriter.
>>>>
>>>>
>>>> Fyi, I pushed in some changes I was working on recently, still more
>>>> work to do on the testing side, though.
>>>>
>>>>
>>>> Awesome!
>>>>
>>>>
>>>> Sincerely,
>>>> Ankush
>>>>
>>>> On Mon, Jan 20, 2020 at 10:01 PM Charles Givre <cgi...@gmail.com>
>>>> wrote:
>>>>
>>>>> Hi Ankush,
>>>>> I have a question for you.  I was looking at your plugin and it looks
>>>>> like you commented out the bit which causes the filter pushdown to happen.
>>>>> Is this not working properly?
>>>>>
>>>>> Also, I attempted to build it and I got a compilation error in the
>>>>> RecordReader, line 151.  I corrected the license issues in the test suite,
>>>>> and pushed that to your branch so hopefully it will pass TravisCI.
>>>>>
>>>>> Best,
>>>>> -- C
>>>>>
>>>>>
>>>>>
>>>>> On Jan 12, 2020, at 10:37 AM, Charles Givre <cgi...@gmail.com> wrote:
>>>>>
>>>>> Great to hear!  I'm excited about getting this committed into Drill!
>>>>> -- C
>>>>>
>>>>> On Jan 12, 2020, at 9:06 AM, Ankush Kapur <ankush.ka...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> Hi Charles,
>>>>>
>>>>> Thanks for reaching out. Hope you are having a good 2020 as well.
>>>>>
>>>>> Yes, I am actively working on putting together more tests for the
>>>>> druid storage plugin.
>>>>>
>>>>> I will have something ready very soon, and also make sure the style
>>>>> guide is followed.
>>>>>
>>>>> Will get back soon.
>>>>>
>>>>> Have a wonderful day.
>>>>>
>>>>> Sincerely,
>>>>> Ankush
>>>>>
>>>>> On Fri, Jan 10, 2020 at 10:08 AM Charles Givre <cgi...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Ankush,
>>>>>> How's it going?  I hope you had a good new year!  I wanted to follow
>>>>>> up with you to see if you've had a chance to work on the Druid storage
>>>>>> plugin.  I don't mean to be a pest, but I'm excited about this capability
>>>>>> and would love to see it integrated into Drill.
>>>>>>
>>>>>> Regarding unit tests, I'm currently working on an Elasticsearch
>>>>>> storage plugin for Drill (
>>>>>> https://github.com/cgivre/drill/tree/storage-elastic2/contrib/storage-elastic/src/test/java/org/apache/drill/exec/store/elasticsearch)
>>>>>> that someone else wrote a few years ago.  I got it to work with ES 5.6 
>>>>>> and
>>>>>> Drill 1.17.  If you want to borrow the unit tests, please feel free.
>>>>>>
>>>>>> One other thing that I know the reviewers will say is that Drill has
>>>>>> a formatter (
>>>>>> https://drill.apache.org/docs/apache-drill-contribution-guidelines/)
>>>>>> for source code (spacing, etc.).  Could you please run your code through
>>>>>> that to make sure that the style is consistent with Drill?
>>>>>>
>>>>>> If you need any other assistance, please let me know. Thanks!
>>>>>> -- C
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Dec 15, 2019, at 10:13 PM, Ankush Kapur <ankush.ka...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>> Hi Charles,
>>>>>>
>>>>>> 1. Yes, I was referring the pushing down aggregations to native druid
>>>>>> aggregation queries. Also, DRUID now supports a new flavour of "SELECT"
>>>>>> query called, "SCAN" query, which is expected to be more memory 
>>>>>> efficient.
>>>>>> But I will leave for another revision.
>>>>>>
>>>>>> 2. I will add more unit tests. Also will add integration tests to
>>>>>> exercise the queries.
>>>>>>
>>>>>> - Ankush
>>>>>>
>>>>>> On Mon, Dec 9, 2019 at 7:23 PM Charles Givre <cgi...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi Ankush,
>>>>>>> Thanks for the contribution!!  This looks like a great start.  I
>>>>>>> have a few questions:
>>>>>>> 1.  When you say it only supports SELECT queries, what exactly do
>>>>>>> you mean here?  Just FYI, most storage plugins are query-only, so most 
>>>>>>> only
>>>>>>> support SELECT queries.  If you are referring to pushing down GROUP BY 
>>>>>>> and
>>>>>>> the like to the plugin, then I think that is fine to worry about later 
>>>>>>> ;-).
>>>>>>> If your plugin works with SELECT queries, Drill should be able to 
>>>>>>> perform
>>>>>>> the aggregation after the query has executed.
>>>>>>>
>>>>>>> 2. I saw there is only one file of unit tests.  I know that the
>>>>>>> first review comment will be that you need more unit tests.  Please 
>>>>>>> take a
>>>>>>> look at the other plugins and see what kind of unit tests they have.  
>>>>>>> At a
>>>>>>> minimum, I would expect to see some query tests.  I'm wondering though,
>>>>>>> what is the best way to test this?
>>>>>>>
>>>>>>> Best,
>>>>>>> -- C
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Dec 8, 2019, at 5:36 PM, Ankush Kapur <ankush.ka...@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Charles,
>>>>>>>
>>>>>>> Apologies for the late reply. Been some crazy times here.
>>>>>>>
>>>>>>> I had the plugin working again and it supports the select queries(
>>>>>>> https://druid.apache.org/docs/latest/querying/select-query.html).
>>>>>>> https://github.com/apache/drill/pull/1888
>>>>>>>
>>>>>>> Let me know if this is good for an MVP.
>>>>>>>
>>>>>>> People at DRUID now support a lot more rich queries(Scan,
>>>>>>> Aggregations, SQL syntax) since I first wrote the plugin. So I am 
>>>>>>> thinking
>>>>>>> future changes would be required, which I would love to work on,
>>>>>>> iteratively.
>>>>>>>
>>>>>>> Let me know if this works for you.
>>>>>>>
>>>>>>> Sincerely,
>>>>>>> Ankush
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Dec 3, 2019 at 11:38 AM Charles Givre <cgi...@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Hi Ankush,
>>>>>>>> How's it going?  I thought I'd check in with you to see how the
>>>>>>>> plugin is coming and if you've made any progress? There is currently a
>>>>>>>> change freeze because we're getting ready to release Drill 1.17, but 
>>>>>>>> I'd
>>>>>>>> really love to see this get committed for version 1.18.  If you're 
>>>>>>>> stuck or
>>>>>>>> need assistance, please let me know.
>>>>>>>> Thanks!
>>>>>>>> -- C
>>>>>>>>
>>>>>>>>
>>>>>>>> On Nov 1, 2019, at 3:26 PM, Ankush Kapur <ankush.ka...@gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi Charles,
>>>>>>>>
>>>>>>>> I just started a draft PR for this work.
>>>>>>>> https://github.com/apache/drill/pull/1888
>>>>>>>>
>>>>>>>>
>>>>>>>> Sincerely,
>>>>>>>> Ankush
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, Oct 12, 2019 at 3:30 PM Ankush Kapur <
>>>>>>>> ankush.ka...@gmail.com> wrote:
>>>>>>>>
>>>>>>>>> Hi Charles,
>>>>>>>>>
>>>>>>>>> I apologize for such a late reply. I have been caught up with
>>>>>>>>> product releases at work.
>>>>>>>>>
>>>>>>>>> I will begin working on the PR. Hopefully, it should not be too
>>>>>>>>> long.
>>>>>>>>>
>>>>>>>>> Hope you have a wonderful weekend.
>>>>>>>>>
>>>>>>>>> Sincerely,
>>>>>>>>> Ankush
>>>>>>>>>
>>>>>>>>> On Thu, Sep 19, 2019 at 10:16 AM Charles Givre <cgi...@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Ankush,
>>>>>>>>>> Thanks for getting back with me!  I'm really glad you're open to
>>>>>>>>>> contributing this to Drill!   I'm also really impressed that you 
>>>>>>>>>> were able
>>>>>>>>>> to do this at all.  Writing storage plugins is extremely difficult,
>>>>>>>>>> especially given the lack of documentation, and that you have to 
>>>>>>>>>> have a
>>>>>>>>>> really good understanding of both Druid and Drill's internals.   So 
>>>>>>>>>> my
>>>>>>>>>> hat's off to you!
>>>>>>>>>> Anyway, I had a look at our JIRA board and it turns out that
>>>>>>>>>> there is an open JIRA for a Drill/Druid integration:
>>>>>>>>>> https://issues.apache.org/jira/browse/DRILL-5956
>>>>>>>>>>
>>>>>>>>>> When you're ready to do so, please create a pull request on the
>>>>>>>>>> Drill github repo with the title:  Drill-5956: Add Storage Plugin 
>>>>>>>>>> for Druid
>>>>>>>>>> and add me as a reviewer.   Thank you very much!!
>>>>>>>>>> Best,
>>>>>>>>>> -- C
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Sep 17, 2019, at 5:23 PM, Ankush Kapur <ankush.ka...@gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Charles,
>>>>>>>>>>
>>>>>>>>>> I am good, thanks for asking. Hope the same for you.
>>>>>>>>>>
>>>>>>>>>> Yes, I would be happy to submit a pr. However. It will take
>>>>>>>>>> sometime to get it in shape. The druid connector is a couple of 
>>>>>>>>>> years old
>>>>>>>>>> now.
>>>>>>>>>>
>>>>>>>>>> - Ankush
>>>>>>>>>>
>>>>>>>>>> On Tue, Sep 17, 2019, 2:27 PM Charles Givre <cgi...@gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Ankush,
>>>>>>>>>>> I hope all is well.  I'm the PMC chair for Apache Drill and I I
>>>>>>>>>>> saw your Druid plugin on github.  I wanted to ask if you would 
>>>>>>>>>>> consider
>>>>>>>>>>> contributing that to Drill and submitting a pull request.  I'm 
>>>>>>>>>>> happy to
>>>>>>>>>>> help with the review process.
>>>>>>>>>>> Thanks,
>>>>>>>>>>> -- Charles
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>

Reply via email to