db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Scott Eade <se...@backstagetech.com.au>
Subject Re: submitted patch which allows left joins, right joins and explicit inner joins
Date Fri, 06 Aug 2004 14:31:36 GMT
Thomas Fischer wrote:

>Hi,
>
>I have added functionality to Torque that allows one to do left joins,
>right joins (and explicit inner joins, in case anybody needs it). I posted
>the diffs on scarab, the ticket id is TRQS219.
>(http://nagoya.apache.org/scarab/issues/id/TRQS219)
>
Wow - great stuff.

>
>To create a left join, the following code has to be used:
>criteria.addJoin(AuthorPeer.AUTHOR_ID, BookPeer.AUTHOR_ID,
>Criteria.LEFT_JOIN);
>It is, of course, still possible to use
>criteria.addJoin(AuthorPeer.AUTHOR_ID, BookPeer.AUTHOR_ID);
>This is equivalent to
>criteria.addJoin(AuthorPeer.AUTHOR_ID, BookPeer.AUTHOR_ID, null);
>and produces a join in the as it was done up to now, i.e. by adding the
>join condition to the "where" clause of the query.
>I tested my changes on an Oracle9i database. I do not know whether the code
>runs on any other database (it produces SQL like "SELECT author.AUTHOR_ID,
>author.NAME FROM book RIGHT JOIN author ON book.AUTHOR_ID=author.AUTHOR_ID
>LEFT JOIN book b ON author.AUTHOR_ID=b.AUTHOR_ID").
>
>Note that I did NOT change the code in generator which generates code for
>selets with joins, e.g. BookPeer.doSelectJoinAuthor(). In my opinion, it
>would be nice to use a left join there to get also the books without an
>author, but I did not want to make too many changes at one time.
>
For backwards compatibility we would need to leave the existing methods 
as is.  There are I guess two ways forward, either we generate a second 
set of methods that do left joins or we add an option to the generator 
to determine if the existing or a left join should be used (I think the 
latter might be the best option).

>
>I hope that I have made the changes such that fully qualified table names
>(database.schema.columnname) are possible, though I did not test it.
>
>Below, I summarize the changes which I made, then I give an example how the
>joins are handled internally, and then I discuss some special cases.
>
>Any comments or questions are welcome.
>
More comments below.

>
>    Cheers
>
>            Thomas
>
>
>Changes:
>
>added the necessary Constants (Join types and "ON") to SqlEnum and Criteria
>
>added the methods equals(Object) and hashCode() to SqlEnum. Thinking it
>through, this would not have been strictly necessary, because SqlEnum has
>no public constructor and only the predefined constants are used, so
>Object.equals() should give the same result as the new equals()-method.
>However, in my opinion adding the new method makes the code more robust and
>better readable, but this modification can be left out if the developers
>think it's not necessary.)
>
>Added a new method addJoin(String left, String right, SqlEnum joinType) to
>the Criteria class
>
>Added an inner class called Join to the Criteria class. A Join contains the
>left column, the right column and the join type. Replaced the Lists joinL
>and joinR by a new list joins which contains elements of type Join.
>
>Added an inner Class called FromElement to Query. A fromElement contains
>the table name in the FROM clause. It might also contain the join type and
>the join condition.
>
>Added a private helper function Criteria.getTablenameForFromClause(String
>tableOrAliasName, Criteria criteria) which returns either the tablename
>itself if tableOrAliasName is not an alias, or a String of the form
>"tableName tableOrAliasName" if tableOrAliasName is an alias for a table
>name.
>
>Added a public helper function getRawColumnName(String
>fullyQualifiedColumnName) which strips the database (and schema) names from
>columns. This is useful for working with aliases, as the constants for
>column names in the peer classes are fully qualified.
>
>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.

>
>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?

>
>
>Example illustrating the used algorithm:
>
>A Criteria should contain a single left join. The SQL snippet which is
>expected is
>table_a left join table_b on table_a.id = table_b.a_id
>
>First, the method criteria.addJoin("table_a.id", "table_b.a_id",
>Criteria.LEFT_JOIN) is called.
>In this function call, criteria.joins would be populated with a single
>element of type Join where the left column is table_a.id, the right column
>is table_b.a_id and the join type is SqlEnum.LEFT_JOIN.
>
>calling BasePeer.createQuery(criteria) would create a Query element where
>fromTables contains two elements of type FromElement. In the first
>FromElement, the table name is table_a and the join type and the join
>condition are both null. In the second FromElement, the table name is
>table_b, the join type is SqlEnum.LEFT_JOIN and the join condition is
>table_a.id = table_b.a_id
>
>In BasePeer.toString(), the first element of the fromTables would be
>appended at the appropriate place by calling its toString() method. Every
>other element is checked whether it contains a join type. If not, a comma
>is appended before the element itself is appended; if yes, a space is
>appended before the element itself is appended. This leads to the desired
>result.
>
>
>Special cases which are considered in the code:
>
>1) criteria.addJoin("table_a.id", "table_b.a_id") is called (corresponds to
>criteria.addJoin("table_a.id", "table_b.a_id", null) ).
>In this case, criteria.joins would still be populated with a single element
>of type Join where the left column is table_a.id and the right column is
>table_b.a_id. However, in contrast to the previous example, the join type
>is null. On calling BasePeer.createQuery(criteria), a query element would
>be created where the to fromElements contain only the two tablenames, and
>the join condition would be added to the where clause (and not to the
>second fromElement, as was done in the previous example).
>
>2) "Simple" multiple joins are considered.
>criteria.addJoin("table_a.id", "table_b.a_id", Criteria.LEFT_JOIN);
>criteria.addJoin("table_b.id", "table_c.b_id", Criteria.LEFT_JOIN);
>The sql should be
>table_a left join table_b on table_a.id = table_b.a_id left join table_c on
>table_b.id = table_c.second_a_id
>i.e. it must be taken into account that table_b is already "in use", it
>must not be added a second time to query.fromTables.
>This is implemented as follows: In the past, this behaviour was ensured by
>using a uniqueList in the fromTables. To retain that behaviour, one could
>define that two FromElements are equal if their tablenames are equal.
>However, this is not the "intuitive" behaviour of equals() which is
>expected from a FromElement, therefore this approach was not implemented.
>Instead, the method BasePeer.createQuery(criteria) checks "by hand" (i.e.
>by using the method BasePeer.fromClauseContainsTableName(..)) to see if a
>tablename already exists
>
>3) "More difficult" multiple joins : consider the following code snippet:
>criteria.addAlias("b", "table_b");
>criteria.addJoin("table_a.id", "table_b.a_id", Criteria.LEFT_JOIN);
>criteria.addJoin("b.second_a_id", "table_a.id", Criteria.RIGHT_JOIN);
>The expected SQL is NOT
>table_a left join table_b on table_a.id = table_b.a_id, table_b b right
>join table_a on b.second_a_id = table_a.id
>but is is instead
>table_a left join table_b on table_a.id = table_b.a_id left join table_b b
>on table_a.id = b.second_a_id
>i.e. if the second table name is already "in use", the join order and the
>join type should be "reversed". This is done in
>BasePeer.createQuery(criteria). (Note that this works only because joins
>are now processed before other code which may also add tables to the from
>clause). If both columns are already "in use", an exception is thrown.
>
>4) Tablenames can also be added to a criteria by adding columns from
>foreign tables explicity to criteria : Consider
>criteria.addSelectColumn("table_b.id");
>criteria.addJoin("table_a.id", "table_b.a_id", Criteria.LEFT_JOIN);
>as opposed to
>criteria.addSelectColumn("table_c.id");
>criteria.addJoin("table_a.id", "table_b.a_id", Criteria.LEFT_JOIN);
>It is ensured that in the first case, table_b is not added (again) to the
>from clause, while in the second case, table_c must be added to the from
>clause. (This was not an issue in Torque 3.1 because the table names were
>stored in an UniqueList, but now this has to be considered.)
>
>
>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.

>
>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).

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

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 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.

Scott

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


---------------------------------------------------------------------
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