Thanks Jens. With a few small tweaks my code now works with your changes. Florent
On Fri, Oct 22, 2010 at 3:56 PM, Jens Hübel <[email protected]> wrote: > According to the discussion we had I have checked in the necessary changes > today: > > (1) removed the old walker code > (2) changed walker grammar so that Florent's PredicateWalker is now used. > (3) splitted interface PredicateWalter and let it inherit from > PredicateWalkerBase > so that people can implement their own walking logic if they like without > changing grammar > (4) moved code from (1) to test classes so that we have an example for (3) > (5) changed return value of all boolean methods in interface to Boolean so > that you can > return null > > As a side effect classes are better decoupled now (see past discussions). We > still need a good example for the PredicateWalker and adjust the Wiki > documentation... also exception handling needs to be improved as already > mentioned. > > I hope that I did not break anything, it was mainly cleanup. > > @Florent: To the best of my knowledge I tried to respect all your > requirements mentioned below. Perhaps you could check at some point in time > if all this still works for you and if not make the necessary changes. I do > not expect any bigger issues but you never know... > > In case someone has used the old interface and prefers to keep it, this is > still possible by moving the older walker from (1), (4) to the calling > application. > > Jens > > -----Original Message----- > From: Florent Guillaume [mailto:[email protected]] > Sent: Donnerstag, 7. Oktober 2010 16:12 > To: [email protected] > Subject: Re: JOINs > > Hi Jens, > > On Thu, Oct 7, 2010 at 3:00 PM, Jens Hübel <[email protected]> wrote: >>> 2) >> What about using Boolean instead of boolean? Then an implementation could >> return null instead of false indicating that the return value is not of >> interest in this case. Would be less confusing than "false". > > That would work yes. > >> And one more question: >> >> I think we would need to switch the query clause in the walker grammar so >> that the new implementation is used. >> CmisQueryWalker.g From: >> >> query [QueryObject qo] >> �...@init { >> queryObj = qo; >> }: >> ^(SELECT select_list from_clause order_by_clause? where_clause) >> { >> queryObj.resolveTypes(); >> queryObj.processWhereClause($where_clause.tree); >> } >> ; >> >> To: >> >> query [QueryObject qo, PredicateWalker pw] >> �...@init { >> queryObj = qo; >> predicateWalker = pw; >> }: >> ^(SELECT select_list from_clause order_by_clause? where_clause) >> { >> queryObj.resolveTypes(); >> predicateWalker.walkPredicate($where_clause.tree); >> } >> ; >> >> I wonder how you have currently solved this for Nuxeo? The only way I found >> for a little playground project was this ugly construction: >> >> org.apache.chemistry.opencmis.server.support.query.CmisQueryWalker.query_return >> qr = walker.query(queryObj); >> Tree whereTree = ((Tree) qr.getTree()).getChild(2); >> Tree whereChild = whereTree == null ? null : whereTree.getChild(0); >> if (null != whereChild) >> walkPredicate(whereChild); >> >> I assume that you just did not change this to not break existing code yet. >> (it must not be this construct, but conceptually to >> have a convenient mechanism to kick-off the walker). > > In Nuxeo at the moment I'm doing: > > Tree whereNode = query.getWhereTree() == null ? null : > query.getWhereTree().getChild(0); > if (whereNode != null) { > new AnalyzingWalker().walkPredicate(whereNode); > } > > This works because processWhereClause now saves the tree (I added this > recently) and does nothing if queryProcessor is null. > >> Perhaps it's also worth thinking about making this more flexible, e.g. take >> the walker out of QueryObject (what you already did) and have a base >> interface PredicateWalkerBase that only has a single method walkPredicate() >> and then derive custom walker interfaces from this, so that you can define >> your walker interface as you like it. In this way we even could use your >> mechanism and my original one. We could then deliver PredicateWalker as >> default implementation and a user could define another interface if he >> needs. The CmisQueryWalker.g depends only on this single method. What do you >> think? This would be my favorite at the moment. > > Works for me (as long we allow the walker to be null just in case). > The only important thing I want to keep is that I want to be able to > call the walkers by hand several times on the same tree, so I still > need access to the WhereTree from outside the CmisQueryWalker.g / > QueryObject. I guess the first walker could save its argument... > >> Probably you also have noticed that currently only RuntimeExceptions are >> raised in case that something goes wrong. This is something that we still >> need to fix to get more useful error messages out of the parser. It's just >> not implemented yet, because it took me some time to figure out to do custom >> error handling with antlr and first attempt did not work. > > Yes I want to change this as well, currently some parsing errors go to > stderr... > > Florent > > >> -----Original Message----- >> From: Florent Guillaume [mailto:[email protected]] >> Sent: Dienstag, 5. Oktober 2010 11:41 >> To: [email protected] >> Subject: Re: JOINs >> >> On Tue, Oct 5, 2010 at 9:06 AM, Jens Hübel <[email protected]> wrote: >>> 1) >>> You did a lot of reformatting and changes with line-end conventions. Is >>> there something we could improve in our code formatting templates or other >>> Eclipse settings? >> >> The only one I can think of is getting rid of invisible spaces (tabs >> and trailing spaces) by making sure your Java editor strips trailing >> spaces (it's a Save Action -- note that you shouldn't enable both it >> and automatic code reformatting on save because the Javadoc formatter >> re-adds trailing spaces after a bare *). I don't think I did much >> reformatting besides the whitespace thing. >> >>> 2) >>> Comparing PredicateWalker with QueryConditionProcessor you have introduced >>> a boolean return value on each method. I don't get what's the meaning of >>> this. In your implementation it seems that you always return false. Could >>> you explain a bit what your idea is here? My idea behind this interface was >>> that many implementations will try to translate CMISQL to SQL. So an >>> implementation typically will have a member variable for the target SQL >>> query string and compose this while walking the tree. I did not find a good >>> way to do this as return values of the individual node visitor methods and >>> so just left them void. >> >> You're right to say that the returned values wouldn't be useful for >> query translators, but for query evaluators they are. In my Nuxeo >> implementation (GeneratingWalker in >> http://hg.nuxeo.org/addons/nuxeo-chemistry/file/5.4/nuxeo-opencmis-impl/src/main/java/org/nuxeo/ecm/core/opencmis/impl/server/CMISQLQueryMaker.java) >> indeed I don't use the return values, but they are very useful in >> walkers used for direct evaluation, like the InMemoryQueryProcessor. >> This could also be useful for a number of other simple implementations >> of the CMIS connector of simple servers, and the return values aren't >> a big hindrance otherwise. >> I could streamline all the return values to Object though, to allow >> more generic evaluation even of clauses and allow a simple "return >> null" if you don't want them. The alternative if we want to use a >> walker with methods returning void in InMemoryQueryProcessor is to >> have each method put its return value in an instance scratchpad field >> and get it back from the caller. It works but is ugly -- otoh >> InMemoryQueryProcessor isn't supposed to be pretty... If you think >> InMemoryQueryProcessor is the exception then we can used void. >> >>> 3) >>> Recently I have added additional pre- and post- methods in the interface >>> for AND, OR, NOT. The reason was that you typically have to set parentheses >>> in your generated SQL call for the contained expressions. So my idea was >>> that if you have a ... WHERE ... A AND B in the cmis query you have a >>> preAnd method appending '(' to your generated SQL then process A then have >>> onAnd where you add " AND " call, then process B and then have a postAnd >>> method where you append the ')'. I don't see this any longer in your code. >>> What's the new approach dealing with this? >> >> In Nuxeo for instance (which does SQL generation) I do this: >> �...@override >> public boolean walkAnd(Tree opNode, Tree leftNode, Tree rightNode) { >> buf.append("("); >> walkPredicate(leftNode); >> buf.append(" AND "); >> walkPredicate(rightNode); >> buf.append(")"); >> return false; >> } >> >> I think it's simpler and doesn't add lots of pre- and post- methods. >> Before deciding to code the PredicateWalker I was trying to use the >> QueryConditionProcessor and I had to add lots of pre- post- and >> middle- methods to it to deal with things like IS NULL, LIKE, ANY =, >> IN ANY, and it really broke down for ANY = which can be generated in >> very different manners depending on the backend. >> >>> 4) >>> If we shift to the new interface/implementation you also should update the >>> documentation: >>> http://incubator.apache.org/chemistry/queryintegration.html >> >> Yes I'll do that if we decide on using this. >> >>> Unfortunately the InMemory server is not the best example for an >>> implementation. Usually a repository will translate the query to some form >>> of database query. The InMemory implementation however is somewhat special >>> in that implements the query engine on its own (and given that of course >>> very limited). If we could come up with a good example for translation into >>> an SQL query or something similar this would be very helpful. >> >> Direct SQL (or Lucene) generation is a much bigger endeavor though, as >> it implies having all the storage changed. I'll be content for now >> with showing Nuxeo source code (LGPL) as an example :) >> >> Florent >> >> >>> -----Original Message----- >>> From: Florent Guillaume [mailto:[email protected]] >>> Sent: Mittwoch, 29. September 2010 01:09 >>> To: [email protected] >>> Subject: Re: JOINs >>> >>> It doesn't change the grammar at all, it's an alternate way of doing >>> what the AbstractQueryConditionProcessor does to allow dealing with >>> the WHERE clause. I made InMemoryQueryProcessor use it as it's >>> cleaner, but I didn't want to remove AbstractQueryConditionProcessor >>> yet until I know if people depend on it or what people think is best. >>> >>> I'll let you take the time to look at this. FYI the base class is >>> https://svn.apache.org/repos/asf/incubator/chemistry/opencmis/trunk/chemistry-opencmis-server/chemistry-opencmis-server-support/src/main/java/org/apache/chemistry/opencmis/server/support/query/AbstractClauseWalker.java >>> >>> Florent >>> >>> On Tue, Sep 28, 2010 at 10:47 PM, Jens Hübel <[email protected]> wrote: >>>> I am currently on travel and don't have a chance to look at this for the >>>> moment. If this is a replacement for the existing walker we should have a >>>> discussion on the list which way to proceed (we shouldn't support two in >>>> the long run). Anyway great to see progress in this area... I will >>>> followup once I had a chance to take a look at this. >>>> >>>> Jens >>>> >>>> >>>> -----Original Message----- >>>> From: Florent Guillaume [mailto:[email protected]] >>>> Sent: Dienstag, 28. September 2010 19:31 >>>> To: [email protected] >>>> Subject: Re: JOINs >>>> >>>> I haven't done the JOIN part yet, but I added a new ClauseWalker >>>> interface (now used in InMemoryQueryProcessor). It should be simpler >>>> to use and extend. >>>> >>>> Florent >>>> >>>> On Mon, Sep 27, 2010 at 1:32 PM, Jens Hübel <[email protected]> wrote: >>>>> Hi Florent, >>>>> >>>>> the support for JOINS is currently weak. I don't think that anybody has >>>>> used them so far. Feel free to modify them. Probably we also could >>>>> implement a predefined walker object at some point so that for a simple >>>>> use case a user does not have to know about the tree structure. >>>>> >>>>> Jens >>>>> >>>>> >>>>> -----Original Message----- >>>>> From: Florent Guillaume [mailto:[email protected]] >>>>> Sent: Montag, 27. September 2010 13:04 >>>>> To: List-Chemistry >>>>> Subject: JOINs >>>>> >>>>> Hi, >>>>> >>>>> Is anyone using the structures related to JOINs in QueryObject? I >>>>> think I'll have to modify them a bit to be able to properly implement >>>>> JOIN-based queries in Nuxeo. >>>>> >>>>> Florent >>>>> > > > -- > Florent Guillaume, Director of R&D, Nuxeo > Open Source, Java EE based, Enterprise Content Management (ECM) > http://www.nuxeo.com http://www.nuxeo.org +33 1 40 33 79 87 > -- Florent Guillaume, Director of R&D, Nuxeo Open Source, Java EE based, Enterprise Content Management (ECM) http://www.nuxeo.com http://www.nuxeo.org +33 1 40 33 79 87
