OrderByColumn for COUNT, SUM

• Key: TORQUE-89
• Type: Bug
• Status: Open
• Priority: Major
• Assignee: Thomas Fischer
• Reporter: Stefan Birrer
• Component/s: Runtime
• Affects Version/s: 3.3-RC2

Description

I've a "complicated" JOIN statement that features some custom column like "COUNT(a) AS ca".

Torque runtime fails on the

Criteria::addAscendingOrderByColumn("ca")

because it expects a full qualified table.columnName which is not the case for the example "ca" column.

The problem can be fixed by adapting the following functions (3.3-RC2):
src/java/org/apache/torque/util/SQLBuilder.java
removeSQLFunction(final String name) : name must not necessarily contain '.' or '*" thus replacing the first thrown exception with a "return name" fixes that issue
processOrderBy(...) : in the for loop, strippedColumnName must not contain '.' hence replacing the first thrown exception with "orderByColumn.add(orderByColumn); break;" fixes the issue

This are quick fixes and I guess you want to reconsider whether you want to implement it that way. After these two changes, it was working correctly though.

Comments

[Thomas Fischer]

I do not believe that the proposed change to removeSQLFunction() will have the desired results. One still would want to remove any sql functions from the table name even if it does not contain a dot (because if one does not, this would mean that table names may not be found in the table Map, leading to errors in creating the sql statement which are not attributed easily to this).
The dot is an easy method to determine where the column name sits. If this would not be used, one would have to do a sophisticated parsing of the column string. And I'd like to avoid different algorithms for "table name containing a dot" and "table name not containing a dot".

I am reluctant to change the algorithm of removeSQLFunction() between release candidates, this may induce subtle errors in user's code. After 3.3 is out, it is planned to replace strings for table names by structured objects, so this kind of problem can be solved more easily then.

[Stefan Birrer]

It sounds reasonable to postpone fixing that bug in a later full release.

However, I think you misinterpret my proposed fixes. In fact, they do not
change any existing "good" behavior. The only thing I proposed below is to
replace two exceptions that make a previous failure path a new "good" path
to account for this use case.

Let me know if I need to be more verbose in my explanations. I can also
submit a patched file that shows my changes.

stefan



--
CSA, Metis Enterprise Technologies LLC
http://www.neokastblog.com

[Thomas Fischer]

I have realized that, but maybe I have been unclear in my answer. I'll try again.
The change you propose to removeSQLFunction() violates the contract of removeSQLFunction(), namely to remove the sql function whenever possible. And I see no simple way to comply with the contract if no dot or star is present.

[Stefan Birrer]

I see your point, so you basically mean this:

F(a) AS a.ca

should return a.ca, right?

So, I totally agree on your point about the removeSQLFunction() and it's
contract. You don't want to change that.

However, you may want to change processOrderBy() to fulfill it's contract of
ordering by a given column name. Currently it only supports columns that map
to an table column and don't support aggregation columns as in (COUNT(a) as
ca) which is at least supported in mysql.

To problem with this bug is that you can't work around b/c of the exception
being thrown at runtime. So the only way to solve is to modify the
processOrderBy(). The reason I touched removeSQLFunction is just to avoid it
to throw, conceptually the following lines have the same purposes (and this
is bad style I know but just to illustrate):

orderByColumn = "ca"; // from the example above

try {
    strippedColumnName = removeSQLFunction(orderByColumn);
} catch (TorqueException e) {
    // this is currently a failure scenario while indeed it could be an
aggregated column name
    strippedColumnName = orderByColumn;
}



--
CSA, Metis Enterprise Technologies LLC
http://www.neokastblog.com

I did not understand that the change in processOrderBy was the main point. I was under the impression that you needed both changes for your query to work.

I have made the requested change to processOrderBy. Could you please check whether this works as you expected ?
The change in removeSQLFunction will not be made, instead the problem will be resolved by improved column definitions after 3.3.

[Stefan Birrer]

Thanks Thomas,

How do I best get to the jar with the patches?

Do I have to check out from CVS? Or is there a simpler way.

thanks
stefan



--
CSA, Metis Enterprise Technologies LLC
http://www.neokastblog.com

[Thomas Fischer]

I'm afraid you have to check out from SVN and use maven to build the project.
Did this work for you ?