Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX for the limit query #1540

Merged
merged 1 commit into from
Nov 13, 2018
Merged

FIX for the limit query #1540

merged 1 commit into from
Nov 13, 2018

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Nov 10, 2018

there are differen meanings, what setMaxRows(0) means.

  • does it mean all rows?
  • or does it mean zero rows?

in conjunction with setFirstRow, it may happen, that an "offset 3 limit 0" query is generated (it depends on the SqlLimiter implementation)

So, in this PR, I checke all SqlLimiters. They will ignore the maxRows property if it is zero and if firstRow is already set.

A better solution might be, if we use -1 (or MAX_INT) as default value for maxRows. Unfortunately, there are many places, with checks like maxRow != 0 , maxRow > 0 , maxRow >= 1...

On the other side, a query with maxRows = 0 makes no sense, it will always be empty

OT: The HanaSqlLimiter seems to be the same as the default LimotOffsetSqlLimiter

@rbygrave rbygrave added the bug label Nov 13, 2018
@rbygrave rbygrave added this to the 11.24.2 milestone Nov 13, 2018
@rbygrave rbygrave merged commit e4cb5f0 into ebean-orm:master Nov 13, 2018
@rPraml rPraml deleted the fix-for-limit branch November 13, 2018 13:39
@rPraml
Copy link
Contributor Author

rPraml commented Nov 13, 2018

@rbygrave I think I was a litte bit over-eager. This breaks the Maria-DB / MySQL support now, as they do not allow offset without limit: "SELECT * FROM table OFFSET 3"

https://dev.mysql.com/doc/refman/8.0/en/select.html#id4651990

I think we should revert this change. And maybe we either initialize the limit with -1 or throw an exception if you set offset only.

limit offset Old impl Current impl logically Expected
0 0 return all rows return all rows return 0 rows
0 10 limit 0 offset 10 (no rows) offset 10 (broken) return 0 rows
10 10 rows 11-20 rows 11-20 rows 11-20
10 0 rows 1-10 rows 1-10 rows 1-10
-1 0 ? ? return all rows
-1 10 ? ? return all rows starting with row 10 (seems not very easy in MySQL)

@rbygrave
Copy link
Member

Right, looking ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants