Hi Florent,
I did some more playing in this area....
> 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".
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). 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.
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.
Jens
-----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
>>
>
>
>
> --
> 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
