Here's my suggested changes to make it single-index based. I also took the liberty to clean up the code style a little to be in line with all the other MM code. Hope that's OK.
https://reviews.apache.org/r/25883/ 2014-09-21 22:28 GMT+02:00 Kasper Sørensen <[email protected]>: > Yeah, let's make it single-schema oriented as a start. We might later add > multi-schema support to it, if we can also get around that in the > superclass. > > The JdbcDataContext does not inherit from QueryPostprocessDataContext at > all - it's a completely separate implementation that has all the schema > model handling and query execution based on JDBC and encoding the queries > into SQL. That's of course by far more effecient, but also a lot more work > than just using the QueryPostprocessDataContext's query handling mechanism. > > 2014-09-21 19:55 GMT+02:00 Alberto Rodriguez <[email protected]>: > >> Yes, we might force the user to pass in the index name on the DataContext >> creation. This way, if the user wants to point to another index he should >> create another DataContext which is not so bad IMHO. >> >> I am out of home this weekend and I can't have a look at MM repo...how do >> you handle this for JDBC? >> >> Kind Regards, >> >> Alberto >> El 21/09/2014 15:18, "Kasper Sørensen" <[email protected]> >> escribió: >> >> > Hmm I imagine that we would not be happy with that solution in the long >> run >> > and then would have to change it in the future anyway. And that kind of >> > change is a bit painful because it's only a runtime backwards >> > incompatibility so people will find out too late. >> > >> > We could also consider to make the implementation index specific, ie. >> only >> > for a single index. That would make a lot of things easy to model too. >> > Den 21/09/2014 12.29 skrev "Alberto Rodriguez" <[email protected]>: >> > >> > > Hi Kasper, >> > > >> > > That's great news!! Thank you for merging the code! >> > > >> > > Regarding the issue you found out, as a simple workaround, what if we >> > > prepend the index name to the "table" name,something like: >> > > indexname.indextable >> > > >> > > Kind regards, >> > > >> > > Alberto >> > > Hi Alberto, >> > > >> > > As you've probably seen, the code has now been merged with the main >> > > MetaModel repo - congratulations and thank you a lot! >> > > >> > > After merging it I took it for another spin to really dive deep into >> some >> > > of the design choices made. I had an observation (and just committed a >> > > javadoc TODO comment about it) - the approach of mapping indexes and >> > > document types is potentially destructable in the sense that if you >> have >> > > multiple indexes with the same document types, those table names are >> > going >> > > to be in conflict. Everything is right now put under the same schema, >> > while >> > > I think actually the ideal approach would be to produce multiple >> schemas, >> > > with their document types represented as tables. That would reflect >> > better >> > > on the hierarchy that ES has. >> > > >> > > Obviously that's harder to do ... And unfortunately right now the >> > > QueryPostprocessDataContext class dictates that you have to produce a >> > > single "main" schema. Which is kinda stupid and circumstantial. So I >> > > propose we see if we can fix that, and then make the ES module >> > multi-schema >> > > oriented. >> > > >> > > 2014-09-17 23:51 GMT+02:00 Alberto Rodriguez <[email protected]>: >> > > >> > > > Hi Kasper, >> > > > >> > > > all your suggestions/changes have been done. >> > > > >> > > > I am also happy to be involved in such an interesting project. >> > > > >> > > > Kind regards, >> > > > >> > > > Alberto >> > > > >> > > > 2014-09-17 21:21 GMT+02:00 Kasper Sørensen < >> > > [email protected] >> > > > >: >> > > > >> > > > > Looks like good changes :-) Happy to co-develop some of this. I >> made >> > a >> > > > few >> > > > > further remarks, but so far looking great! >> > > > > >> > > > > 2014-09-17 10:56 GMT+02:00 Alberto Rodriguez < >> [email protected] >> > >: >> > > > > >> > > > > > Hi Kasper, >> > > > > > >> > > > > > please see my notes on your PR comments, I've also pushed new >> > changes >> > > > to >> > > > > > the repo. >> > > > > > >> > > > > > Kind regards, >> > > > > > >> > > > > > Alberto Rodríguez >> > > > > > >> > > > > > >> > > > > > <http://www.stratio.com/> >> > > > > > Avenida de Europa, 26. Ática 5. 3ª Planta >> > > > > > 28224 Pozuelo de Alarcón, Madrid >> > > > > > Tel: +34 91 352 59 42 // *@stratiobd < >> > https://twitter.com/StratioBD >> > > >* >> > > > > > >> > > > > > 2014-09-17 9:45 GMT+02:00 Kasper Sørensen < >> > > > > [email protected] >> > > > > > >: >> > > > > > >> > > > > > > Hi Alberto, >> > > > > > > >> > > > > > > I added a bunch of remarks to the PR in github. None of them >> > > > absolutely >> > > > > > > critical to do before we merge it, but nice if you anyway >> want to >> > > > spend >> > > > > > > more time on it. >> > > > > > > >> > > > > > > Best regards >> > > > > > > Kasper >> > > > > > > Den 16/09/2014 23.01 skrev "Henry Saputra" < >> > > [email protected] >> > > > >: >> > > > > > > >> > > > > > > > Thanks Alberto, this should help review the patch >> > > > > > > > >> > > > > > > > - Henry >> > > > > > > > >> > > > > > > > On Tue, Sep 16, 2014 at 1:44 PM, Alberto Rodriguez < >> > > > > [email protected]> >> > > > > > > > wrote: >> > > > > > > > > Hi Henry, >> > > > > > > > > >> > > > > > > > > I have just submitted a PR against the MM github mirror. >> > > > > > > > > >> > > > > > > > > Kind regards, >> > > > > > > > > >> > > > > > > > > Alberto Rodríguez >> > > > > > > > > >> > > > > > > > > 2014-09-16 19:01 GMT+02:00 Henry Saputra < >> > > > [email protected] >> > > > > >: >> > > > > > > > > >> > > > > > > > >> Alberto, >> > > > > > > > >> >> > > > > > > > >> Thanks for the ES module patch. >> > > > > > > > >> >> > > > > > > > >> Could you submit PR against Apache MetaModel github >> mirror? >> > > > > > > > >> We may need to manually merge your fixes to ASF Git repo >> > later >> > > > but >> > > > > > > > >> Github PR will help with reviews. >> > > > > > > > >> >> > > > > > > > >> - Henry >> > > > > > > > >> >> > > > > > > > >> On Tue, Sep 16, 2014 at 2:45 AM, Alberto Rodriguez >> > > > > > > > >> <[email protected]> wrote: >> > > > > > > > >> > Hi Kasper, >> > > > > > > > >> > >> > > > > > > > >> > thank you very much for your feedback! >> > > > > > > > >> > >> > > > > > > > >> > I have just modified the getMainSchemaName method to >> get >> > the >> > > > > > cluster >> > > > > > > > name >> > > > > > > > >> > from ES. Regarding the executeQuery method I commented >> it >> > > out >> > > > > > > because >> > > > > > > > >> some >> > > > > > > > >> > tests were failing, anyway I will try to make this >> method >> > > work >> > > > > and >> > > > > > > get >> > > > > > > > >> all >> > > > > > > > >> > the benefits from the awesome elasticsearch java API. >> > > > > > > > >> > >> > > > > > > > >> > My apache username is: albertostratio >> > > > > > > > >> > >> > > > > > > > >> > Kind regards, >> > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > > >> > Alberto Rodríguez >> > > > > > > > >> > >> > > > > > > > >> > >> > > > > > > > >> > <http://www.stratio.com/> >> > > > > > > > >> > Avenida de Europa, 26. Ática 5. 3ª Planta >> > > > > > > > >> > 28224 Pozuelo de Alarcón, Madrid >> > > > > > > > >> > Tel: +34 91 352 59 42 // *@stratiobd < >> > > > > > https://twitter.com/StratioBD >> > > > > > > >* >> > > > > > > > >> > >> > > > > > > > >> > 2014-09-16 10:45 GMT+02:00 Kasper Sørensen < >> > > > > > > > >> [email protected]>: >> > > > > > > > >> > >> > > > > > > > >> >> Hi Alberto, >> > > > > > > > >> >> >> > > > > > > > >> >> Once again, thank you very much for this great work. I >> > > think >> > > > > this >> > > > > > > > >> module is >> > > > > > > > >> >> looking 99% ready to be introduced into the main >> > MetaModel >> > > > > > > codebase. >> > > > > > > > >> >> >> > > > > > > > >> >> Here are my very minor remarks: >> > > > > > > > >> >> >> > > > > > > > >> >> >> > > > > > > > >> >> - The main schema name "ElasticSearchSchema" is >> > > hardcoded >> > > > > and >> > > > > > > not >> > > > > > > > >> >> necesarily what users would want. I was thinking >> if we >> > > > > should >> > > > > > > > either: >> > > > > > > > >> >> - Make it parameterizable in the constructor >> (default >> > > > value >> > > > > > > would >> > > > > > > > IMO >> > > > > > > > >> >> better be just "ElasticSearch" which is in line >> > with >> > > > > > > "MongoDB", >> > > > > > > > >> >> "CouchDB" >> > > > > > > > >> >> etc.) >> > > > > > > > >> >> - Use the ES cluster name. This would give a >> more >> > > > > "native" >> > > > > > > > >> feeling - >> > > > > > > > >> >> then we've mapped it to something that is >> > > recognizable >> > > > > from >> > > > > > > > the ES >> > > > > > > > >> >> connection. >> > > > > > > > >> >> - Or a combination of the above - make a >> > constructor >> > > > name >> > > > > > > > >> available, >> > > > > > > > >> >> but if null we use the cluster name. >> > > > > > > > >> >> - You've now commented out your whole >> > > executeQuery(Query) >> > > > > > > method. >> > > > > > > > >> Might >> > > > > > > > >> >> be on purpose, but I guess you had some nice >> > > optimizations >> > > > > for >> > > > > > > > >> special >> > > > > > > > >> >> cases in there. I hope we can restore that. >> > > > > > > > >> >> >> > > > > > > > >> >> What is your apache username? When we have that, I >> > propose >> > > > that >> > > > > > we >> > > > > > > > take >> > > > > > > > >> >> your contribution for a VOTE and then hopefully >> integrate >> > > it >> > > > > into >> > > > > > > the >> > > > > > > > >> main >> > > > > > > > >> >> codebase of MM. The corrections above can be made >> either >> > > > before >> > > > > > or >> > > > > > > > after >> > > > > > > > >> >> IMO. >> > > > > > > > >> >> >> > > > > > > > >> >> Best regards, >> > > > > > > > >> >> Kasper >> > > > > > > > >> >> >> > > > > > > > >> >> >> > > > > > > > >> >> 2014-09-16 9:28 GMT+02:00 Alberto Rodriguez < >> > > > > > > [email protected] >> > > > > > > > >: >> > > > > > > > >> >> >> > > > > > > > >> >> > Hi all, >> > > > > > > > >> >> > >> > > > > > > > >> >> > I have been working on the ES module. As Kasper >> > suggested >> > > I >> > > > > > have >> > > > > > > > >> >> overriden >> > > > > > > > >> >> > the materializeMainSchemaTable to get a first >> > functional >> > > > > > > version. I >> > > > > > > > >> have >> > > > > > > > >> >> > added quite a few tests to check that the module is >> > > working >> > > > > > fine >> > > > > > > > for >> > > > > > > > >> >> > *SELECT* operations. I have also implemented the >> > > > > > > executeCountQuery >> > > > > > > > >> >> method. >> > > > > > > > >> >> > >> > > > > > > > >> >> > I forked the project from the *master branch, *you >> can >> > > > check >> > > > > > out >> > > > > > > > the >> > > > > > > > >> >> > changes here: incubator-metamodel fork with ES >> module >> > > > > > > > >> >> > < >> https://github.com/albertostratio/incubator-metamodel >> > > >> > > > > > > > >> >> > >> > > > > > > > >> >> > I have also been informed by the ASF that my ICLA >> has >> > > been >> > > > > > filed >> > > > > > > in >> > > > > > > > >> their >> > > > > > > > >> >> > records. >> > > > > > > > >> >> > >> > > > > > > > >> >> > Kind regards, >> > > > > > > > >> >> > >> > > > > > > > >> >> > >> > > > > > > > >> >> > Alberto Rodríguez >> > > > > > > > >> >> > >> > > > > > > > >> >> > >> > > > > > > > >> >> > <http://www.stratio.com/> >> > > > > > > > >> >> > Avenida de Europa, 26. Ática 5. 3ª Planta >> > > > > > > > >> >> > 28224 Pozuelo de Alarcón, Madrid >> > > > > > > > >> >> > Tel: +34 91 352 59 42 // *@stratiobd < >> > > > > > > > https://twitter.com/StratioBD>* >> > > > > > > > >> >> > >> > > > > > > > >> >> >> > > > > > > > >> >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >
