groupBy() for MySQLQuery throws an exception for an empty result set #1055

Closed
masa67 opened this Issue Nov 26, 2014 · 10 comments

Comments

Projects
None yet
3 participants
@masa67

masa67 commented Nov 26, 2014

My question on Stack Overflow should detail the problem:

http://stackoverflow.com/questions/27075864/count-asterisk-on-querydsl-mysql

It also seems that result.getTotal() returns always value 1 for queries that use groupBy().

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Nov 26, 2014

Member

I see the problem, thanks for the report!

@timowest for no resultsets, there is no 'row' with the result from count(*).
So AbstractSQLQuery at line 484 returns an empty Resultset, line 485 returns false (no next value) and all breaks at line 488

We could easily fix this with

Long count;
if(rs.next()) {
    count = rs.getLong(1);
} else {
    count = 0;
}

Or some other variant, maybe this one, which could also be a ternary return.

boolean hasResult = rs.next();
//...
if (hasResult) {
    return rs.getLong(1);
} else {
    return 0;
}
Member

Shredder121 commented Nov 26, 2014

I see the problem, thanks for the report!

@timowest for no resultsets, there is no 'row' with the result from count(*).
So AbstractSQLQuery at line 484 returns an empty Resultset, line 485 returns false (no next value) and all breaks at line 488

We could easily fix this with

Long count;
if(rs.next()) {
    count = rs.getLong(1);
} else {
    count = 0;
}

Or some other variant, maybe this one, which could also be a ternary return.

boolean hasResult = rs.next();
//...
if (hasResult) {
    return rs.getLong(1);
} else {
    return 0;
}
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 26, 2014

Member

@Shredder121 Both variants look good to me. You can pick one.

Member

timowest commented Nov 26, 2014

@Shredder121 Both variants look good to me. You can pick one.

@masa67

This comment has been minimized.

Show comment
Hide comment
@masa67

masa67 Nov 27, 2014

Please check the implementation of result.getTotal() as well. To me, it seems that it returns always value 1 at least in this particular type of MySQLQuery queries with groupBy().

masa67 commented Nov 27, 2014

Please check the implementation of result.getTotal() as well. To me, it seems that it returns always value 1 at least in this particular type of MySQLQuery queries with groupBy().

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Nov 27, 2014

Member

Does the same query work for that issue?
If not, then we're going to need one that reflects that behaviour.
But I'll look at it.

Member

Shredder121 commented Nov 27, 2014

Does the same query work for that issue?
If not, then we're going to need one that reflects that behaviour.
But I'll look at it.

@masa67

This comment has been minimized.

Show comment
Hide comment
@masa67

masa67 Nov 27, 2014

I think I know where the problem is...

This problem is not related to this query, but to my other question on Stack Overflow:
http://stackoverflow.com/questions/27044582/how-to-call-mysql-select-found-rows-on-querydsl

Based on Timo's answer, I get the impression that results.getTotal(); should execute SELECT FOUND_ROWS() when SQL_CALC_FOUND_ROWS is present, but that is not the case, is it?

The start of the problematic query generated by Querydsl looks like this: select sql_calc_found_rows count(*) from provider..., and to me it looks that the total number is then read from rs.getLong(1) (so these two issues are kind of related, after all), which does not work in this particular case. Instead, a separate SQL query is needed.

This is not necessarily a bug. A MySQL user would expect to run a separate query for FOUND_ROWS(). What would be the Querydsl way of doing it?

masa67 commented Nov 27, 2014

I think I know where the problem is...

This problem is not related to this query, but to my other question on Stack Overflow:
http://stackoverflow.com/questions/27044582/how-to-call-mysql-select-found-rows-on-querydsl

Based on Timo's answer, I get the impression that results.getTotal(); should execute SELECT FOUND_ROWS() when SQL_CALC_FOUND_ROWS is present, but that is not the case, is it?

The start of the problematic query generated by Querydsl looks like this: select sql_calc_found_rows count(*) from provider..., and to me it looks that the total number is then read from rs.getLong(1) (so these two issues are kind of related, after all), which does not work in this particular case. Instead, a separate SQL query is needed.

This is not necessarily a bug. A MySQL user would expect to run a separate query for FOUND_ROWS(). What would be the Querydsl way of doing it?

@timowest timowest added this to the 3.6.0 milestone Nov 27, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 27, 2014

Member

@masa67 @Shredder121 I added a pull request. Queries with group by need subquery wrapping to get the proper count results.

SQL_CALC_FOUND_ROWS optimizations can be considered later, but for now I will fix the general case.

Member

timowest commented Nov 27, 2014

@masa67 @Shredder121 I added a pull request. Queries with group by need subquery wrapping to get the proper count results.

SQL_CALC_FOUND_ROWS optimizations can be considered later, but for now I will fix the general case.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 27, 2014

Member

I created a ticket for SQL_CALC_FOUND_ROWS usage #1060

Member

timowest commented Nov 27, 2014

I created a ticket for SQL_CALC_FOUND_ROWS usage #1060

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 27, 2014

Member

Do you think select count(*) from (<original query without limit and offset>) is valid pattern for this case? This works for SQL, but for JPA we need to think of something else.

Member

timowest commented Nov 27, 2014

Do you think select count(*) from (<original query without limit and offset>) is valid pattern for this case? This works for SQL, but for JPA we need to think of something else.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Nov 29, 2014

Member

I think that it would work yes.

Member

Shredder121 commented Nov 29, 2014

I think that it would work yes.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Nov 30, 2014

Member

Released in 3.6.0

Member

timowest commented Nov 30, 2014

Released in 3.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment