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

Added support for LIMIT 0 query clause #2905

Closed

Conversation

serg1236
Copy link
Contributor

Build on top and should be merged after #2904

@serg1236
Copy link
Contributor Author

Justification for the change

  • LIMIT 0 clause is a valid SQL expression
  • LIMIT 0 is used to check the rest of the query for validity against the DB server
  • setMaxRows(0) is allowed by ebean and produces an unexpected behavior by not limiting the result set at all.

@rbygrave
Copy link
Member

Yes, this all looks good - thanks!!

The outstanding question (maybe to me mostly) is if we can consider this a bug to merge into a patch release (13.10.3) or if this is a change in behavior that needs to merge into a minor version bump (13.11.0).

@rbygrave
Copy link
Member

Right now I think the next release should be 13.11.0 and we merge this.

@serg1236
Copy link
Contributor Author

Yes, this all looks good - thanks!!

The outstanding question (maybe to me mostly) is if we can consider this a bug to merge into a patch release (13.10.3) or if this is a change in behavior that needs to merge into a minor version bump (13.11.0).

I don't have a strong opinion on that. However, I lean towards a bug and a patch version due to inconsistent setMaxRows(0) behavior. If it threw something like UnsupportedException before, I would consider it a feature.

@rbygrave
Copy link
Member

Noting that there are 6 SqlLimiter implementations and this only updates 1 of those implementations. So this PR isn't yet complete per say.

@rbygrave
Copy link
Member

This PR is not complete for all SqlLimiters. For myself, I'm happy for people to use limit 1 instead as a better alternative to limit 0.

I'll close this issue.

@rbygrave rbygrave closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants