db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Fischer <Fisc...@seitenbau.net>
Subject Re: submitted patch which allows left joins, right joins and explicit inner joins
Date Tue, 10 Aug 2004 09:05:17 GMT




Scott,

thanks for looking through my lengthy mail. I will incorporate your
suggestions, but as I am really busy till the end of August at least, this
will probably take another month. I will then also adjust the patch to the
latest changes in the CVS HEAD.
More comments are inserted below.


Scott Eade <seade@backstagetech.com.au> schrieb am 06.08.2004 16:31:36:

> Thomas Fischer wrote:
>
> >Found that most code in BasePeer.createQuery(Criteria) was duplicated in
> >BasePeer.createPreparedStatement(Criteria, StringBuffer, List). As I had
to
> >make changes in both methods anyway, I decided to split the code and put
> >the parts which are (almost) equal in both methods into private the
methods
> >processSelectColumns(..), processAsColumns(..), processJoins(..),
> >processModifiers(..), processCriterions(..) and processOrderBy(..). I am
> >not sure why BasePeer.createQuery(..) processes the groupBy- and
> >Having-Lists of Criteria while BasePeer.createPreparedStatement(..)
> >doesn't, but I left it as it was, though I think it's a bit fishy. Also,
> >the criteria.getLimit() and criteria.getOffset() are processed
differently
> >by BasePeer.createQuery(..) and BasePeer.createPreparedStatement(..). I
> >could see no reason for this, but I left it also as it was.
> >
> I agree that it sounds fishy, I guess we need someone that uses prepared
> statements to chime in and indicate whether or not they have problems
> using group by or having clauses.  In any case if you have retained the
> existing behaviour you should not have added to the problem.
>

>From the patch which you checked into the CVS two weks ago, I have an idea
why this is so. My guess is that most people don't use prepared statements,
and therefore the submitted patches also apply only for the "normal"
statements. I will check through the CVS history and see whether this is
true. Also, I will write some test cases for order by with prepared
statements and run them on oracle (unfortunalety, I do not have access to a
DB2 installation, so I cannot test it for DB2. If I remember correctly, the
biggest differences are for DB2). A problem with this is that some DB's do
not support prepared statements, so the tests should only be enabled for
the DB's which support prepared Statements. I know that Oracle and DB2 do
support prepared statements, and MySQL doesn't, but I do not know about
other DB's.

> >
> >Changed the treatment of select clauses in BasePeer.createQuery(..) and
> >basePeer.createPreparedStatement(..). Both treatments are quite similar,
> >except that the checks on column names differ: In normal statements, a
> >column does not have to contain a "." to be valid, if it contains a "*".
> >However, criteria.addSelectColumn("*") fails in Torque 3.1 with an
> >StringIndexOutOfBoundsException. Does anybody know why this is so ? I
did
> >not really know what to do, so I changed the behaviour in
> >basePeer.createPreparedStatement(..) so that it is like the behaviour in
> >BasePeer.createQuery(..). Should not "*" be a valid "column name" to
select
> >from ???
> >
> I guess it is better to either add the desired columns specifically or
> to let torque add all of the columns in the table - if you were to allow
> "*", would the information necessary to pull the data out of the result
> be available?
>

I agree that adding "*" to the desired columns could lead to a lot of
confusion, if it is done without thought. I never needed it, so I really
don't insist of enabling it. I also have no idea whether the column
information is needed at other places. I have no problem with leaving the
behaviour as it is.
> >
> >
> >Special cases where I don't know whether they are treated correctly :
> >
> >1) I am not sure whether I have treated added Criterion's correctly
> >(criteria.add(Criterion)). The tablenames for added Criterions are added
> >like the colums from the select Columns (see 4), which is the treatment
> >which they got in BasePeer.java in version 1.78 from the CVS. Therefore
> >this should work with the old join type (join type null), but I have no
> >idea if this treatment is ok for the join types LEFT_JOIN, RIGHT_JOIN
and
> >INNER_JOIN. This is mainly because I have no clue what
> >criteria.add(Criterion) does.
> >
> Look at http://db.apache.org/torque/criteria-howto.html for an example
> of Criterions in action.  I would suggest that your patch should include
> an update to this document (the underlying xdoc that is) to cover the
> new functionality.
>

In the meantime, I have used a criterion with a left join, so this seems to
work ;-) . I will also update the docs illustrating how left joins are
used.

> >
> >If anybody can think of another special case which I did not consider,
> >please tell me.
> >
> Can you please take a look at the coding standard found at
> http://jakarta.apache.org/turbine/common/code-standards.html  You need
> to put your open braces on new lines and to keep an eye on line wrapping
> (most common in torque would be to indent by 8 spaces for a broken line).
>

I didn't find any coding standards at the torque page, so i didn't know
which standards I should use. Perhaps a reference should be put into the
developer guide in the website. I will format my changes accordingly.

> Other things you should include in your patch are updates to:
>     project.xml - add yourself as a contributor
>     changes.xml - mention the enhanced functionality

Will do that as well.

>
> This is an extensive patch that is going to add significant new
> functionality to torque.  I would see HEAD as the appropriate place to
> apply this patch (i.e. I do not see it going into 3.1.x).

I also do not see it as a small fix, so I agree to this.

>
> I know it has taken an age for this response to appear.  Please bear
> with us.  One or two patches on this scale and you will be welcomed with
> open arms as a committer.
>

As you already said, this was quite a large patch, so looking through it
will also have taken a lot of your time, so don't worry abouth the response
time.

> Scott
>
> --
> Scott Eade
> Backstage Technologies Pty. Ltd.
> http://www.backstagetech.com.au

Thomas


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org


Mime
View raw message