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

Skip Paginator LIMIT subquery and WHERE IN if query do not have LIMIT #7829

Closed
Seb33300 opened this issue Sep 24, 2019 · 7 comments
Closed
Assignees
Milestone

Comments

@Seb33300
Copy link
Contributor

Seb33300 commented Sep 24, 2019

I ran into an issue with the Paginator which I think could be handled to prevent it.

I am using the Paginator to paginate a table.
The user can choose the number of items by page (10 ,20, ... but also All items).

In the case the user choose to display All items, the LIMIT clause will NOT be added to the query.
Even if the Paginator is not required in this scenario, the same part of code handle this case so we also give the query to the Paginator.

This is working property unless the query return too many results.
Because the Paginator will use a WHERE IN query and it could reach the limit of allowed number of parameters by the database engine.
See: https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/tutorials/pagination.html

This is what we are encountering on tables with thousands of items on SQL Server.

@Seb33300 Seb33300 changed the title Skip Paginator LIMIT and WHERE IN if query do not have LIMIT Skip Paginator LIMIT subquery and WHERE IN if query do not have LIMIT Sep 24, 2019
@lcobucci
Copy link
Member

@Seb33300 I'd say that handle this case isn't an ORM's responsibility. The paginator component is quite generic and having a flag to enable its pagination behaviour or not smells like a bad design for the library.

I'd suggest you to encapsulate this in your application (perhaps decorating the paginator?).

@TomHAnderson
Copy link
Member

It sounds to me like you already know if you don't need a limit clause. So, if you're not using a limit don't use a Paginator.

@Seb33300
Copy link
Contributor Author

I know this is useless to use the Paginator feature if we are not intended to paginate results.
But I felt it more like an optimization, since this could prevent to execute useless queries.

Btw even if we do not use it to paginate results, it could also be used to count distinct entries.

@lcobucci not sure what you mean by a flag, but it just require to check if a limit has been added to the query. No extra parameter or something else required.

@Ocramius
Copy link
Member

Basically you want to say that if the setMaxResults() is null, you just want to execute the query, right?

I think that may be an acceptable feature, if it doesn't raise complexity too much. Could you hack up an example, just to post it for the purposes of discussion?

@Seb33300
Copy link
Contributor Author

Seb33300 commented Sep 24, 2019

Yes, this is exactly what I mean.
No need for filtering the original query with ids and no need for an extra query to get distinct ids since we want all of them.

Not sure about the example you want.

An example of use case or a pull request to implement it?

@lcobucci
Copy link
Member

@lcobucci not sure what you mean by a flag, but it just require to check if a limit has been added to the query. No extra parameter or something else required.

@Seb33300 setting setMaxResults() to null is kind of a flag 😄.

Not sure about the example you want.

An example of use case or a pull request to implement it?

A PR with (at least) a functional test showing what you expect from the library.

You can use this test as example:
https://github.com/doctrine/orm/blob/b52ef5a1002f99ab506a5a2d6dba5a2c236c5f43/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php

@lcobucci
Copy link
Member

Please send it to v2.7, btw 👍

@lcobucci lcobucci added this to the 2.7.0 milestone Nov 19, 2019
vilartoni added a commit to Emagister/doctrine-orm that referenced this issue Jan 14, 2020
…gin-master

v2.7.0

[![Build Status](https://travis-ci.org/doctrine/orm.svg?branch=v2.7.0)](https://travis-ci.org/doctrine/orm)

This release solves Symfony 5.0 compatibility issues, some small improvements, and adds
various deprecation notices.

Please read carefully the [upgrade to 2.7
notes](https://github.com/doctrine/orm/blob/2.7/UPGRADE.md#upgrade-to-27) to know more
about the reasons and how to fix the deprecation messages.

---

- Total issues resolved: **1**
- Total pull requests resolved: **15**
- Total contributors: **10**

Deprecation
-----------

 - [7911: Be explicit about which Doctrine package in message](doctrine#7911) thanks to @lcobucci
 - [7909: Add deprecation messages](doctrine#7909) thanks to @lcobucci
 - [7901: Add deprecation warnings for 2.7.x](doctrine#7901) thanks to @lcobucci
 - [7701: Split and deprecate AbstractQuery#useResultCache()](doctrine#7701) thanks to @someniatko

CI
--

 - [7904: Make sure composer files are valid](doctrine#7904) thanks to @greg0ire
 - [7600: &doctrine#91;2.7&doctrine#93; CI: Test against PHP 7.4snapshot instead of nightly (8.0)](doctrine#7600) thanks to @Majkl578

Improvement
-----------

 - [7876: Fix compat of commands with Symfony 5](doctrine#7876) thanks to @nicolas-grekas
 - [7829: Skip Paginator LIMIT subquery and WHERE IN if query do not have LIMIT](doctrine#7829) thanks to @Seb33300
 - [7723: Allow Symfony 5.0](doctrine#7723) thanks to @nicolas-grekas
 - [7710: Prettified arrays in tool command orm:mapping:describe](doctrine#7710) thanks to @rtek
 - [7340: Fix config template for PHPUnit >= 7.2](doctrine#7340) thanks to @guilliamxavier

BC Break,Improvement
--------------------

 - [7863: Paginator: Skip limit subquery if not required](doctrine#7863) thanks to @Seb33300

Documentation
-------------

 - [7382: Update homepage](doctrine#7382) thanks to @Majkl578

Bug
---

 - [7326: Cherry-pick doctrine#7307 to fix remaining usages of deprecated ClassLoader and Inflector from doctrine/common](doctrine#7326) thanks to @nicolas-grekas
 - [7079: Fix getJoinTableName for sqlite with schema attribute](doctrine#7079) thanks to @mairo744

BC Break,Deprecation,Improvement
--------------------------------

 - [6803: Deprecation of EntityManager copy method](doctrine#6803) thanks to @SenseException
lcobucci added a commit to lcobucci/doctrine2 that referenced this issue Feb 15, 2022
Since v2.7.0 the ORM avoids using extra queries via the paginator
component when maximum results isn't set on the original query. With
that change, this test was not executing the code path that it was
expected to run.

This makes sure we trigger the forcing of custom DBAL types when
hydrating the identifiers, making sure we don't introduce bugs.

More info:
- Forcing DBAL type conversion: doctrine#7905
- Issue on optimisation: doctrine#7829
- PR on optimisation: doctrine#7863
- Minor BC-break docs: https://github.com/doctrine/orm/blob/2.11.x/UPGRADE.md#minor-bc-break-paginator-output-walkers-arent-be-called-anymore-on-sub-queries-for-queries-without-max-results
lcobucci added a commit to lcobucci/doctrine2 that referenced this issue Feb 15, 2022
Since v2.7.0 the ORM avoids using extra queries via the paginator
component when maximum results isn't set on the original query. With
that change, this test was not executing the code path that it was
expected to run.

This makes sure we trigger the forcing of custom DBAL types when
hydrating the identifiers, making sure we don't introduce bugs.

More info:
- Forcing DBAL type conversion: doctrine#7905
- Issue on optimisation: doctrine#7829
- PR on optimisation: doctrine#7863
- Minor BC-break docs: https://github.com/doctrine/orm/blob/2.11.x/UPGRADE.md#minor-bc-break-paginator-output-walkers-arent-be-called-anymore-on-sub-queries-for-queries-without-max-results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants