OFBiz › OFBiz - Dev

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

9 messages Options

Jacopo Cappellato-3

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Hi Jacques,

just curious, what was the bug that you fixed here?

Thanks,

Jacopo

PS: there are some minor formatting issues in the patch

On Jun 27, 2008, at 10:43 AM, [hidden email] wrote:

> Author: jleroux
> Date: Fri Jun 27 01:43:00 2008
> New Revision: 672191
>
> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
> Log:
> Fix a Group By bug when using Count (View Entities)
>
> Modified:
> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
> GenericDAO.java
>
> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/
> datasource/GenericDAO.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
> GenericDAO.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
> GenericDAO.java Fri Jun 27 01:43:00 2008
> @@ -970,7 +970,18 @@
> sqlP.executeQuery();
> long count = 0;
> ResultSet resultSet = sqlP.getResultSet();
> - if (resultSet.next()) {
> + boolean isGroupBy = false;
> + if (modelEntity instanceof ModelViewEntity) {
> + ModelViewEntity modelViewEntity = (ModelViewEntity)
> modelEntity;
> + String groupByString =
> modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ",
> ", "", false);
> +
> + if (UtilValidate.isNotEmpty(groupByString)) isGroupBy
> = true;
> + }
> +
> + if (isGroupBy) {
> + while (resultSet.next()) count++;
> + }
> + else if (resultSet.next()) {
> count = resultSet.getLong(1);
> }
> return count;
>
>

Jacques Le Roux

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Administrator

Hi Jacopo,

I just fixed the formatting issues, I did it too fast, sorry.
When you were using a group by with a count in a view-entity you did not get all the possible results. It's easy to try by creating
a specific entity-view and looking for its results in webtools/entities data maintenance(All).

If you are interested I can send you more informations

Jacques

From: "Jacopo Cappellato" <[hidden email]>

> Hi Jacques,
>
> just curious, what was the bug that you fixed here?
>
> Thanks,
>
> Jacopo
>
> PS: there are some minor formatting issues in the patch
>
>
> On Jun 27, 2008, at 10:43 AM, [hidden email] wrote:
>
>> Author: jleroux
>> Date: Fri Jun 27 01:43:00 2008
>> New Revision: 672191
>>
>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>> Log:
>> Fix a Group By bug when using Count (View Entities)
>>
>> Modified:
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ datasource/GenericDAO.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>> = = = = = = = = ======================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java Fri Jun 27 01:43:00 2008
>> @@ -970,7 +970,18 @@
>> sqlP.executeQuery();
>> long count = 0;
>> ResultSet resultSet = sqlP.getResultSet();
>> - if (resultSet.next()) {
>> + boolean isGroupBy = false;
>> + if (modelEntity instanceof ModelViewEntity) {
>> + ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
>> + String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
>> +
>> + if (UtilValidate.isNotEmpty(groupByString)) isGroupBy = true;
>> + }
>> +
>> + if (isGroupBy) {
>> + while (resultSet.next()) count++;
>> + }
>> + else if (resultSet.next()) {
>> count = resultSet.getLong(1);
>> }
>> return count;
>>
>>
>

Jacopo Cappellato-3

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Jacques,

thanks for your explanation. I don't need extra information from
you... I actually still don't understand this patch, but that's not
really important if you and others are sure it is a correct fix.
By the way, and sorry for being such a pain... there is room for more
formatting enhancements :-)

From

>>> + if (isGroupBy) {
>>> + while (resultSet.next()) count++;
>>> + }
>>> + else if (resultSet.next()) {
>>

to
>>> + if (isGroupBy) {
>>> + while (resultSet.next()) count++;
>>> + } else if (resultSet.next()) {
>>

and probably it would be better to always use { } even for one line
blocks (but this is less important).

Jacopo

On Jun 27, 2008, at 2:00 PM, Jacques Le Roux wrote:

> Hi Jacopo,
>
> I just fixed the formatting issues, I did it too fast, sorry.
> When you were using a group by with a count in a view-entity you did
> not get all the possible results. It's easy to try by creating
> a specific entity-view and looking for its results in webtools/
> entities data maintenance(All).
>
> If you are interested I can send you more informations
>
> Jacques
>
> From: "Jacopo Cappellato" <[hidden email]>
>> Hi Jacques,
>>
>> just curious, what was the bug that you fixed here?
>>
>> Thanks,
>>
>> Jacopo
>>
>> PS: there are some minor formatting issues in the patch
>>
>>
>> On Jun 27, 2008, at 10:43 AM, [hidden email] wrote:
>>
>>> Author: jleroux
>>> Date: Fri Jun 27 01:43:00 2008
>>> New Revision: 672191
>>>
>>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>>> Log:
>>> Fix a Group By bug when using Count (View Entities)
>>>
>>> Modified:
>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
>>> GenericDAO.java
>>>
>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/
>>> datasource/GenericDAO.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>>> = = = = = = = =
>>> =
>>> =
>>> ====================================================================
>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
>>> GenericDAO.java (original)
>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
>>> GenericDAO.java Fri Jun 27 01:43:00 2008
>>> @@ -970,7 +970,18 @@
>>> sqlP.executeQuery();
>>> long count = 0;
>>> ResultSet resultSet = sqlP.getResultSet();
>>> - if (resultSet.next()) {
>>> + boolean isGroupBy = false;
>>> + if (modelEntity instanceof ModelViewEntity) {
>>> + ModelViewEntity modelViewEntity =
>>> (ModelViewEntity) modelEntity;
>>> + String groupByString =
>>> modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(),
>>> ", ", "", false);
>>> +
>>> + if (UtilValidate.isNotEmpty(groupByString))
>>> isGroupBy = true;
>>> + }
>>> +
>>> + if (isGroupBy) {
>>> + while (resultSet.next()) count++;
>>> + }
>>> + else if (resultSet.next()) {
>>> count = resultSet.getLong(1);
>>> }
>>> return count;
>>>
>>>
>>
>

Jacques Le Roux

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Administrator

Jacopo,

From: "Jacopo Cappellato" <[hidden email]>
> Jacques,
>
> thanks for your explanation. I don't need extra information from you... I actually still don't understand this patch, but that's
> not really important if you and others are sure it is a correct fix.

Yes, it's a correct fix and have been seriously tested

> By the way, and sorry for being such a pain... there is room for more formatting enhancements :-)

You don't have to be sorry. Actually it's my sole fault, sorry for that. It's commited in r672355 (and r672357 kind of joke, but
it's not)

Jacques

> From
>
>>>> + if (isGroupBy) {
>>>> + while (resultSet.next()) count++;
>>>> + }
>>>> + else if (resultSet.next()) {
>>>
>
>
> to
>>>> + if (isGroupBy) {
>>>> + while (resultSet.next()) count++;
>>>> + } else if (resultSet.next()) {
>>>
>
>
> and probably it would be better to always use { } even for one line blocks (but this is less important).
>
> Jacopo
>
> On Jun 27, 2008, at 2:00 PM, Jacques Le Roux wrote:
>
>> Hi Jacopo,
>>
>> I just fixed the formatting issues, I did it too fast, sorry.
>> When you were using a group by with a count in a view-entity you did not get all the possible results. It's easy to try by
>> creating
>> a specific entity-view and looking for its results in webtools/ entities data maintenance(All).
>>
>> If you are interested I can send you more informations
>>
>> Jacques
>>
>> From: "Jacopo Cappellato" <[hidden email]>
>>> Hi Jacques,
>>>
>>> just curious, what was the bug that you fixed here?
>>>
>>> Thanks,
>>>
>>> Jacopo
>>>
>>> PS: there are some minor formatting issues in the patch
>>>
>>>
>>> On Jun 27, 2008, at 10:43 AM, [hidden email] wrote:
>>>
>>>> Author: jleroux
>>>> Date: Fri Jun 27 01:43:00 2008
>>>> New Revision: 672191
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>>>> Log:
>>>> Fix a Group By bug when using Count (View Entities)
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java
>>>>
>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ datasource/GenericDAO.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>>>> = = = = = = = = = = ====================================================================
>>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java (original)
>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java Fri Jun 27 01:43:00 2008
>>>> @@ -970,7 +970,18 @@
>>>> sqlP.executeQuery();
>>>> long count = 0;
>>>> ResultSet resultSet = sqlP.getResultSet();
>>>> - if (resultSet.next()) {
>>>> + boolean isGroupBy = false;
>>>> + if (modelEntity instanceof ModelViewEntity) {
>>>> + ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
>>>> + String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
>>>> +
>>>> + if (UtilValidate.isNotEmpty(groupByString)) isGroupBy = true;
>>>> + }
>>>> +
>>>> + if (isGroupBy) {
>>>> + while (resultSet.next()) count++;
>>>> + }
>>>> + else if (resultSet.next()) {
>>>> count = resultSet.getLong(1);
>>>> }
>>>> return count;
>>>>
>>>>
>>>
>>
>

David E Jones-2

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

In reply to this post by Jacopo Cappellato-3

I'm going to complain about this change too. It doesn't make a lot of
sense.

Let me describe what I'm reading, just to check: if there is a group
by, then iterate through all results to count them instead of getting
the count result as a single number.

If that is true, it means the database is ignoring the "COUNT(*)" when
there is a "GROUP BY", and I've never heard of a problem like that
before.

So, in fact this seems like it would BREAK a count when there is a
GROUP BY if the above statement is not true. It would result in a
single row with a count value, but the count value would be ignored by
this code, always returning 1 because there is only one row returned
(the row with the count).

Is there a test case for this or anything that was not working before
but is working after? If there isn't anything OOTB in OFBiz then let's
write something, like a unit test or something crazy like that.

-David

On Jun 27, 2008, at 2:43 AM, [hidden email] wrote:

Jacques Le Roux

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Administrator

David,

There is something, I will write that tomorrow. Hopefully, it will be better than the other fix...

Jacques

From: "David E Jones" <[hidden email]>

>
> I'm going to complain about this change too. It doesn't make a lot of sense.
>
> Let me describe what I'm reading, just to check: if there is a group by, then iterate through all results to count them instead
> of getting the count result as a single number.
>
> If that is true, it means the database is ignoring the "COUNT(*)" when there is a "GROUP BY", and I've never heard of a problem
> like that before.
>
> So, in fact this seems like it would BREAK a count when there is a GROUP BY if the above statement is not true. It would result
> in a single row with a count value, but the count value would be ignored by this code, always returning 1 because there is only
> one row returned (the row with the count).
>
> Is there a test case for this or anything that was not working before but is working after? If there isn't anything OOTB in OFBiz
> then let's write something, like a unit test or something crazy like that.
>
> -David
>
>
> On Jun 27, 2008, at 2:43 AM, [hidden email] wrote:
>
>> Author: jleroux
>> Date: Fri Jun 27 01:43:00 2008
>> New Revision: 672191
>>
>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>> Log:
>> Fix a Group By bug when using Count (View Entities)
>>
>> Modified:
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ datasource/GenericDAO.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>> = = = = = = = = ======================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java Fri Jun 27 01:43:00 2008
>> @@ -970,7 +970,18 @@
>> sqlP.executeQuery();
>> long count = 0;
>> ResultSet resultSet = sqlP.getResultSet();
>> - if (resultSet.next()) {
>> + boolean isGroupBy = false;
>> + if (modelEntity instanceof ModelViewEntity) {
>> + ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
>> + String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
>> +
>> + if (UtilValidate.isNotEmpty(groupByString)) isGroupBy = true;
>> + }
>> +
>> + if (isGroupBy) {
>> + while (resultSet.next()) count++;
>> + }
>> + else if (resultSet.next()) {
>> count = resultSet.getLong(1);
>> }
>> return count;
>>
>>
>

Jacques Le Roux

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Administrator

Hi David,

It's easy to test simply create the following view-entity
<view-entity entity-name="TestGrouping" package-name="org.ofbiz.learning">
<member-entity entity-alias="PA" entity-name="PostalAddress"/>
<alias entity-alias="PA" name="count" field="contactMechId" function="count"/>
<alias entity-alias="PA" name="city" group-by="true"/>
</view-entity>

and check it in both cases in Entity Data Maintenance (numbers of each city)

For ease of testing here is the concerned snippet of code (near GenericDAO.java[570]
try {
sqlP.executeQuery();
long count = 0;
ResultSet resultSet = sqlP.getResultSet();
if (resultSet.next()) {
// boolean isGroupBy = false;
// if (modelEntity instanceof ModelViewEntity) {
// ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
// String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
// if (UtilValidate.isNotEmpty(groupByString)) {
// isGroupBy = true;
// }
// }
// if (isGroupBy) {
// while (resultSet.next()) {
// count++;
// }
// } else if (resultSet.next()) {
count = resultSet.getLong(1);
}
return count;

I did not digg into details but I guess the genuine code was written at a time we were using Minerva, and I suppose it has been
tested. Could it be that DBCP has a different behaviour regarding this aspect ? Or maybe this bug has never been catched before ?

Jacques

From: "Jacques Le Roux" <[hidden email]>

> David,
>
> There is something, I will write that tomorrow. Hopefully, it will be better than the other fix...
>
> Jacques
>
> From: "David E Jones" <[hidden email]>
>>
>> I'm going to complain about this change too. It doesn't make a lot of sense.
>>
>> Let me describe what I'm reading, just to check: if there is a group by, then iterate through all results to count them instead
>> of getting the count result as a single number.
>>
>> If that is true, it means the database is ignoring the "COUNT(*)" when there is a "GROUP BY", and I've never heard of a problem
>> like that before.
>>
>> So, in fact this seems like it would BREAK a count when there is a GROUP BY if the above statement is not true. It would result
>> in a single row with a count value, but the count value would be ignored by this code, always returning 1 because there is only
>> one row returned (the row with the count).
>>
>> Is there a test case for this or anything that was not working before but is working after? If there isn't anything OOTB in
>> OFBiz then let's write something, like a unit test or something crazy like that.
>>
>> -David
>>
>>
>> On Jun 27, 2008, at 2:43 AM, [hidden email] wrote:
>>
>>> Author: jleroux
>>> Date: Fri Jun 27 01:43:00 2008
>>> New Revision: 672191
>>>
>>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>>> Log:
>>> Fix a Group By bug when using Count (View Entities)
>>>
>>> Modified:
>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java
>>>
>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ datasource/GenericDAO.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>>> = = = = = = = = ======================================================================
>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java (original)
>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java Fri Jun 27 01:43:00 2008
>>> @@ -970,7 +970,18 @@
>>> sqlP.executeQuery();
>>> long count = 0;
>>> ResultSet resultSet = sqlP.getResultSet();
>>> - if (resultSet.next()) {
>>> + boolean isGroupBy = false;
>>> + if (modelEntity instanceof ModelViewEntity) {
>>> + ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
>>> + String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
>>> +
>>> + if (UtilValidate.isNotEmpty(groupByString)) isGroupBy = true;
>>> + }
>>> +
>>> + if (isGroupBy) {
>>> + while (resultSet.next()) count++;
>>> + }
>>> + else if (resultSet.next()) {
>>> count = resultSet.getLong(1);
>>> }
>>> return count;
>>>
>>>
>>
>

David E Jones

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Thanks for the details Jacques. I was able to reproduce the problem,
and discovered some interesting SQL quirks along the way.

The biggest problem I had with this fix was that it was a serious hack
and involved iterating over all of the results to count them, which
will not perform well for large databases.

In SVN rev 673014 I've committed an alternative that will have the
database do the counting so it performs much better.

-David

On Jun 30, 2008, at 5:54 AM, Jacques Le Roux wrote:

> Hi David,
>
> It's easy to test simply create the following view-entity
> <view-entity entity-name="TestGrouping" package-
> name="org.ofbiz.learning">
> <member-entity entity-alias="PA" entity-name="PostalAddress"/>
> <alias entity-alias="PA" name="count" field="contactMechId"
> function="count"/>
> <alias entity-alias="PA" name="city" group-by="true"/>
> </view-entity>
>
> and check it in both cases in Entity Data Maintenance (numbers of
> each city)
>
> For ease of testing here is the concerned snippet of code (near
> GenericDAO.java[570]
> try {
> sqlP.executeQuery();
> long count = 0;
> ResultSet resultSet = sqlP.getResultSet();
> if (resultSet.next()) {
> // boolean isGroupBy = false;
> // if (modelEntity instanceof ModelViewEntity) {
> // ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
> // String groupByString =
> modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ",
> ", "", false);
> // if (UtilValidate.isNotEmpty(groupByString)) {
> // isGroupBy = true;
> // }
> // }
> // if (isGroupBy) {
> // while (resultSet.next()) {
> // count++;
> // }
> // } else if (resultSet.next()) {
> count = resultSet.getLong(1);
> }
> return count;
>
> I did not digg into details but I guess the genuine code was written
> at a time we were using Minerva, and I suppose it has been tested.
> Could it be that DBCP has a different behaviour regarding this
> aspect ? Or maybe this bug has never been catched before ?
>
> Jacques
>
> From: "Jacques Le Roux" <[hidden email]>
>> David,
>>
>> There is something, I will write that tomorrow. Hopefully, it will
>> be better than the other fix...
>>
>> Jacques
>>
>> From: "David E Jones" <[hidden email]>
>>>
>>> I'm going to complain about this change too. It doesn't make a lot
>>> of sense.
>>>
>>> Let me describe what I'm reading, just to check: if there is a
>>> group by, then iterate through all results to count them instead
>>> of getting the count result as a single number.
>>>
>>> If that is true, it means the database is ignoring the "COUNT(*)"
>>> when there is a "GROUP BY", and I've never heard of a problem
>>> like that before.
>>>
>>> So, in fact this seems like it would BREAK a count when there is
>>> a GROUP BY if the above statement is not true. It would result
>>> in a single row with a count value, but the count value would be
>>> ignored by this code, always returning 1 because there is only
>>> one row returned (the row with the count).
>>>
>>> Is there a test case for this or anything that was not working
>>> before but is working after? If there isn't anything OOTB in
>>> OFBiz then let's write something, like a unit test or something
>>> crazy like that.
>>>
>>> -David
>>>
>>>
>>> On Jun 27, 2008, at 2:43 AM, [hidden email] wrote:
>>>
>>>> Author: jleroux
>>>> Date: Fri Jun 27 01:43:00 2008
>>>> New Revision: 672191
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>>>> Log:
>>>> Fix a Group By bug when using Count (View Entities)
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
>>>> GenericDAO.java
>>>>
>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/
>>>> datasource/GenericDAO.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>>>> = = = = = = = =
>>>> =
>>>> =
>>>> =
>>>> ===================================================================
>>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
>>>> GenericDAO.java (original)
>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/
>>>> GenericDAO.java Fri Jun 27 01:43:00 2008
>>>> @@ -970,7 +970,18 @@
>>>> sqlP.executeQuery();
>>>> long count = 0;
>>>> ResultSet resultSet = sqlP.getResultSet();
>>>> - if (resultSet.next()) {
>>>> + boolean isGroupBy = false;
>>>> + if (modelEntity instanceof ModelViewEntity) {
>>>> + ModelViewEntity modelViewEntity =
>>>> (ModelViewEntity) modelEntity;
>>>> + String groupByString =
>>>> modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(),
>>>> ", ", "", false);
>>>> +
>>>> + if (UtilValidate.isNotEmpty(groupByString))
>>>> isGroupBy = true;
>>>> + }
>>>> +
>>>> + if (isGroupBy) {
>>>> + while (resultSet.next()) count++;
>>>> + }
>>>> + else if (resultSet.next()) {
>>>> count = resultSet.getLong(1);
>>>> }
>>>> return count;
>>>>
>>>>
>>>
>>
>

Jacques Le Roux

Re: svn commit: r672191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java

Administrator

From: "David E Jones" <[hidden email]>
>
> Thanks for the details Jacques. I was able to reproduce the problem, and discovered some interesting SQL quirks along the way.
>
> The biggest problem I had with this fix was that it was a serious hack and involved iterating over all of the results to count
> them, which will not perform well for large databases.

Yes, I totally agree on that ;o)

Jacques

> In SVN rev 673014 I've committed an alternative that will have the database do the counting so it performs much better.
>
> -David
>
>
> On Jun 30, 2008, at 5:54 AM, Jacques Le Roux wrote:
>
>> Hi David,
>>
>> It's easy to test simply create the following view-entity
>> <view-entity entity-name="TestGrouping" package- name="org.ofbiz.learning">
>> <member-entity entity-alias="PA" entity-name="PostalAddress"/>
>> <alias entity-alias="PA" name="count" field="contactMechId" function="count"/>
>> <alias entity-alias="PA" name="city" group-by="true"/>
>> </view-entity>
>>
>> and check it in both cases in Entity Data Maintenance (numbers of each city)
>>
>> For ease of testing here is the concerned snippet of code (near GenericDAO.java[570]
>> try {
>> sqlP.executeQuery();
>> long count = 0;
>> ResultSet resultSet = sqlP.getResultSet();
>> if (resultSet.next()) {
>> // boolean isGroupBy = false;
>> // if (modelEntity instanceof ModelViewEntity) {
>> // ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
>> // String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
>> // if (UtilValidate.isNotEmpty(groupByString)) {
>> // isGroupBy = true;
>> // }
>> // }
>> // if (isGroupBy) {
>> // while (resultSet.next()) {
>> // count++;
>> // }
>> // } else if (resultSet.next()) {
>> count = resultSet.getLong(1);
>> }
>> return count;
>>
>> I did not digg into details but I guess the genuine code was written at a time we were using Minerva, and I suppose it has been
>> tested. Could it be that DBCP has a different behaviour regarding this aspect ? Or maybe this bug has never been catched before
>> ?
>>
>> Jacques
>>
>> From: "Jacques Le Roux" <[hidden email]>
>>> David,
>>>
>>> There is something, I will write that tomorrow. Hopefully, it will be better than the other fix...
>>>
>>> Jacques
>>>
>>> From: "David E Jones" <[hidden email]>
>>>>
>>>> I'm going to complain about this change too. It doesn't make a lot of sense.
>>>>
>>>> Let me describe what I'm reading, just to check: if there is a group by, then iterate through all results to count them
>>>> instead
>>>> of getting the count result as a single number.
>>>>
>>>> If that is true, it means the database is ignoring the "COUNT(*)" when there is a "GROUP BY", and I've never heard of a
>>>> problem
>>>> like that before.
>>>>
>>>> So, in fact this seems like it would BREAK a count when there is a GROUP BY if the above statement is not true. It would
>>>> result
>>>> in a single row with a count value, but the count value would be ignored by this code, always returning 1 because there is
>>>> only
>>>> one row returned (the row with the count).
>>>>
>>>> Is there a test case for this or anything that was not working before but is working after? If there isn't anything OOTB in
>>>> OFBiz then let's write something, like a unit test or something crazy like that.
>>>>
>>>> -David
>>>>
>>>>
>>>> On Jun 27, 2008, at 2:43 AM, [hidden email] wrote:
>>>>
>>>>> Author: jleroux
>>>>> Date: Fri Jun 27 01:43:00 2008
>>>>> New Revision: 672191
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=672191&view=rev
>>>>> Log:
>>>>> Fix a Group By bug when using Count (View Entities)
>>>>>
>>>>> Modified:
>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java
>>>>>
>>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ datasource/GenericDAO.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/GenericDAO.java?rev=672191&r1=672190&r2=672191&view=diff
>>>>> = = = = = = = = = = = ===================================================================
>>>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java (original)
>>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/datasource/ GenericDAO.java Fri Jun 27 01:43:00 2008
>>>>> @@ -970,7 +970,18 @@
>>>>> sqlP.executeQuery();
>>>>> long count = 0;
>>>>> ResultSet resultSet = sqlP.getResultSet();
>>>>> - if (resultSet.next()) {
>>>>> + boolean isGroupBy = false;
>>>>> + if (modelEntity instanceof ModelViewEntity) {
>>>>> + ModelViewEntity modelViewEntity = (ModelViewEntity) modelEntity;
>>>>> + String groupByString = modelViewEntity.colNameString(modelViewEntity.getGroupBysCopy(), ", ", "", false);
>>>>> +
>>>>> + if (UtilValidate.isNotEmpty(groupByString)) isGroupBy = true;
>>>>> + }
>>>>> +
>>>>> + if (isGroupBy) {
>>>>> + while (resultSet.next()) count++;
>>>>> + }
>>>>> + else if (resultSet.next()) {
>>>>> count = resultSet.getLong(1);
>>>>> }
>>>>> return count;
>>>>>
>>>>>
>>>>
>>>
>>
>