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 > <mailto: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 >> <mailto: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 >> <mailto: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 >>> <mailto: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 >>> <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 >>>> <mailto: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 >>>> <mailto: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 >>>>> <mailto: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 >>>>>> <mailto:cgi...@gmail.com>> wrote: >>>>>> >>>>>> Hey Ankush, >>>>>> See responses inline. >>>>>> >>>>>> >>>>>>> On Jan 21, 2020, at 8:12 AM, Ankush Kapur <ankush.ka...@gmail.com >>>>>>> <mailto: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 >>>>>> <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 >>>>>>> <mailto: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 >>>>>>>> <mailto: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 >>>>>>>>> <mailto: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 >>>>>>>>> <mailto: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 >>>>>>>>> >>>>>>>>> <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/ >>>>>>>>> <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 >>>>>>>>>> <mailto: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 >>>>>>>>>> <mailto: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 >>>>>>>>>>> <mailto: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://druid.apache.org/docs/latest/querying/select-query.html>). >>>>>>>>>>> https://github.com/apache/drill/pull/1888 >>>>>>>>>>> <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 >>>>>>>>>>> <mailto: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 >>>>>>>>>>>> <mailto:ankush.ka...@gmail.com>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Charles, >>>>>>>>>>>> >>>>>>>>>>>> I just started a draft PR for this work. >>>>>>>>>>>> https://github.com/apache/drill/pull/1888 >>>>>>>>>>>> <https://github.com/apache/drill/pull/1888> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Sincerely, >>>>>>>>>>>> Ankush >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Sat, Oct 12, 2019 at 3:30 PM Ankush Kapur >>>>>>>>>>>> <ankush.ka...@gmail.com <mailto: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 >>>>>>>>>>>> <mailto: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 >>>>>>>>>>>> <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 >>>>>>>>>>>>> <mailto: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 >>>>>>>>>>>>> <mailto: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 >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
