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
