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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >> >> Hey Ankush, >> See responses inline. >> >> >> On Jan 21, 2020, at 8:12 AM, Ankush Kapur <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >>> >>> Great to hear! I'm excited about getting this committed into Drill! >>> -- C >>> >>> On Jan 12, 2020, at 9:06 AM, Ankush Kapur <[email protected]> >>> 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 <[email protected]> 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 <[email protected]> >>>> 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 <[email protected]> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>>> 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 <[email protected]> >>>>>> 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 <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>> 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 >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>>> >>>> >>> >>> >> >> >> > >
