Hi Kasper, I've been having a look at the changes and they all look fine with me. I am not really used to this kind of revisions...what should be the next steps?
By the way, I did not have time to work on the insert, delete and update operations, do you think we should implement them? Kind regards, Alberto 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-21 22:55 GMT+02:00 Kasper Sørensen <[email protected]>: > 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>* > >> > > > > > > > >> >> > > >> > > > > > > > >> >> > >> > > > > > > > >> > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > >
