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 > <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 >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
