fixed PostgreSQL and Oracle pagination issues #1325

vaheshadunts · 2015-03-09T16:47:57Z

Pagination with ordering on 1:m and m:m relations, was not working
properly because of selecting the ordered fields in main select
statement with distinction (e.g. SELECT DISTINCT e0_.id, p1_.name from
... ) in this case we've received less rows than we've required in
query. So I've modified the subquery generation part.
In case of PostgreSQL and Oracle added the row_number in the subquery
over order by statement, then the main select is grouped by id and
selected min of row number, also ordering by rownumber asc, because they
are on right order already (e.g. select e0_.id, min(rownum) as rownum
from ..... order by rownum).
In case of MySQL, the subselect result with ids are in right order so
there is no need to select that fields(this fixes the same issue too)
In other cases I haven't tested because of that leaved the same.

Pagination with ordering on 1:m and m:m relations, was not working properly because of selecting the ordered fields in main select statement with distinction (e.g. SELECT DISTINCT e0_.id, p1_.name from ... ) in this case we've received less rows than we've required in query. So I've modified the subquery generation part. In case of PostgreSQL and Oracle added the row_number in the subquery over order by statement, then the main select is grouped by id and selected min of row number, also ordering by rownumber asc, because they are on right order already (e.g. select e0_.id, min(rownum) as rownum from ..... order by rownum). In case of MySQL, the subselect result with ids are in right order so there is no need to select that fields(this fixes the same issue too) In other cases I haven't tested because of that leaved the same.

doctrinebot · 2015-03-09T16:48:09Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3606

We use Jira to track the state of pull requests and the versions they got
included in.

TomasVotruba · 2015-03-09T16:57:27Z

lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php

+                 * which are participated in order by statement, arent selected in
+                 * main select statement.
+                 */
+                if($this->platform->getName() == 'mysql'){


cs, missing spaces if (, ) {

vaheshadunts · 2015-03-09T22:43:43Z

@TomasVotruba please let me know, should I have to modify the tests for CI? Thanks.

TomasVotruba · 2015-03-10T08:15:22Z

lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php

+                }
+                $globalOrderByStmt = SqlWalker::walkOrderByClause($AST->orderByClause);
+
+                $innerRowNumberSelectPart = ' ROW_NUMBER() OVER(%s) as ROWNUM';


AS should be kept capital, to be consistent in the tests.

TomasVotruba · 2015-03-10T08:16:32Z

@vaheshadunts Not sure. The output should remain the same. E. g. you can solve AS vs as. And for new columns, sb else has to check (I don't know much about OutputWalker).

mvrhov · 2015-03-10T09:45:58Z

@vaheshadunts: Have you seen the comments in the issue linked in code. The same affects mssql >= 2008, mariadb >= 5.5, and probably mysql 5.6

vaheshadunts · 2015-03-10T11:02:13Z

@TomasVotruba Thank you for advices, anyway I've modified the tests as my queries should be generated.

vaheshadunts · 2015-03-10T11:05:50Z

@mrhov thanks for your comment, so are you suggesting to override my code on the versions that you've listed ?

mvrhov · 2015-03-11T13:34:38Z

Well I don't know the fix for those versions as I'm using a postgresql for the past few years exclusively. I'm just bringing this to your attention. There were a couple of bug fixes regarding the code around pagination in the past couple of months. So I'm advising you to be careful.

stof · 2015-03-11T16:09:02Z

@deeky666 I think that before merging any new bugfix about the pagination, we should introduce a functional testsuite for the pagination, i.e. running the paginated queries against the DB and asserting the result instead of only doing assertions on the generated SQL. This is the only way we can know that the pagination works properly.

Ocramius · 2015-03-16T22:11:57Z

lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php

-            );
+
+            // http://www.doctrine-project.org/jira/browse/DDC-1958
+            if ($this->platform->getName() == 'postgresql' || $this->platform->getName() == 'oracle') {


We should use instanceof here

Ocramius · 2015-03-16T22:15:39Z

Absolutely in line with what @stof suggested.

vaheshadunts · 2015-03-17T00:47:41Z

@Ocramius thank you for your comments, If you want to see the before/after differences on a small data I can demo it for you, please let me know.

Ocramius · 2015-03-17T23:35:52Z

Consider reviewing #1337 before thinking of working further on this.

…into hotfix/doctrine#1342-paginator-functional-test-integration

Ocramius · 2015-03-24T00:40:19Z

Handled via #1337 and #1342

vaheshadunts · 2015-03-24T10:20:16Z

Hi @Ocramius , I've tested the PR #1337 , works fine, but it doesn't solve the issues which I have listed and solved in this PR, anyway if you if you don't mind, I'll merge the #1337 and this PR, and will made a new PR soon. Please let me know your opinion about this. Thanks.

P.S. please let me know If you have a some notices about code stylistics.

Ocramius · 2015-03-24T10:57:14Z

@vaheshadunts open a new PR: I'll gladly review.
Note that I'm closing off everything for RC1 tomorrow, so we need to work on it today, heh.

vaheshadunts · 2015-03-24T15:03:09Z

ok @Ocramius , I'll do it ASAP

vaheshadunts · 2015-03-24T23:35:31Z

@Ocramius after #1337 issues are more than before.
Before when the ordered fields were only from nested entity, it was working correct. Now it fails too (please see the demo links below)

Also I agree with #1337 and with #1220 that LimitSubqueryOutputWalker should not duplicate orderBy clauses, BUT we couldn't get a correct result with DISTINCT {array_merge(sqlIdentifier, sqlOrderColumns)} clause, please let me know if you're agree with this. Also in SQL we are not able to do ordering without selecting them. Unfortunately in the postgreSQL and Oracle, the subquery order result doesn't affect on the main query (thats how born the idea, to select a row number, like we did in MSSQL). At the other side we don't need to duplicate the ordered fields, thats why I've decided to MOVE that fields into subquery, and do not order them (that solved the SELECT DISTINCTION problem, using order by rownumber expression with grouping id of the entity from 'FROM' clause).

Whats the problem
heres the query generated by #1337

SELECT DISTINCT id0, name1, first_name3 FROM (SELECT f0_.id AS id0, f0_.name AS name1, p1_.id AS id2, p1_.first_name AS first_name3, p1_.last_name AS last_name4 FROM family f0_ INNER JOIN person p1_ ON f0_.id = p1_.family_id) dctrn_result ORDER BY name1 DESC, first_name3 ASC

If we'll continue with #1337 we will not be able to use row_number and group manipulations to solve this issue.

Now please pay attention to the #1325's generated query

SELECT id0, MIN(ROWNUM) AS MINROWNUM FROM (SELECT f0_.id AS id0, f0_.name AS name1, ROW_NUMBER() OVER( ORDER BY f0_.name DESC, p1_.first_name ASC) AS ROWNUM FROM family f0_ INNER JOIN person p1_ ON f0_.id = p1_.family_id ORDER BY f0_.name DESC, p1_.first_name ASC) dctrn_result GROUP BY id0 ORDER BY MINROWNUM ASC

Please let me know
if the problem of #1337 was the duplication of order by statements, could you please tell me the steps how to reproduce the issue regarding that, because the #1325 isn't duplicating them, just moving, maybe this will solve that issue.

Excuse me for such a long comment, I'm ready to discuss with you.

Thanks a lot !!

DEMOS:
PostgreSQL: http://dev.progarithm.com:8000/paging/pgsql/web/
MySQL: http://dev.progarithm.com:8000/paging/mysql/web/

NOTE: Please pay attention to count that printed out in demo, that's the result of count(Paginator instance);

vaheshadunts · 2015-03-25T15:17:56Z

also @Ocramius please let me know your opinion, what about adding rownumber to #1337's generated query, remove DISTINCT keyword from select and wrap it in query with rownumber and group by ?

billschaller · 2015-03-25T16:45:22Z

@vaheshadunts - that could work... is there any way you could add a testcase to https://github.com/doctrine/doctrine2/blob/master/tests/Doctrine/Tests/ORM/Functional/PaginationTest.php? I think I can probably get this fixed this afternoon.

billschaller · 2015-03-26T02:29:35Z

@vaheshadunts @Ocramius I tried to make a failing testcase here: https://github.com/zeroedin-bill/doctrine2/tree/DDC-3606-testcase and was not able to reproduce the problem... Am I missing something?

vaheshadunts · 2015-03-26T08:16:35Z

@zeroedin-bill sorry for late answer, I was away, as it was night in out country.

So to fail tests we need to have 2 departments in one company which names are next to each other with alphabetical order, in that case the test fails. In your testcase you're doing assertions on count and names, but count of paginator working correct (CountOutputWalker class works fine), and with names. that will be ok too, I think we need to do assertion after iteration $i === 0, what do you think ?

Please have a look at my comments in https://github.com/zeroedin-bill/doctrine2/blob/DDC-3606-testcase/tests/Doctrine/Tests/ORM/Functional/PaginationTest.php

billschaller · 2015-03-26T11:13:19Z

@vaheshadunts - There's an assertion on the count of $companies, which will be an array with 9 items - after iteration, $i must be 0 or the test fails, so the extra assertion wouldn't matter.

I can look at this more tonight - Do you think you could convert your example into a self-contained testcase? That might be the easiest way.

stof · 2015-03-26T11:16:53Z

@vaheshadunts if there are still cases where the pagination is broken, please start by adding more cases in the pagination functional tests to show these issues (and prevent regressions breaking them again in the future).

vaheshadunts · 2015-03-26T11:21:17Z

@zeroedin-bill please have a look at my demo examples. the count works correct as the Paginator implements Countable interface, and count is set by CountOutputWalker, so count($paginator) is correct, thats why I want to assert $i === 0, please have a look this demo http://dev.progarithm.com:8000/paging/pgsql/web/ . Pay attention on first result, output is one family but count is 2..

Sure I can build a testcase, but please try with my notes on your testcase, I think it should work..

P.S. please tell me your timezone.

billschaller · 2015-03-26T11:34:40Z

@vaheshadunts I tried your notes on my testcase - Didn't work... have to go catch a plane now... Please build your example as a testcase - thanks!

vaheshadunts · 2015-03-26T11:45:58Z

sure @zeroedin-bill

vaheshadunts · 2015-03-27T03:10:48Z

@zeroedin-bill https://github.com/vaheshadunts/doctrine2/tree/pagination-order-tests

Thanks

TomasVotruba reviewed Mar 9, 2015
View reviewed changes

vaheshadunts added 2 commits March 10, 2015 00:45

Fixed missing spaces around if on line 250

b68c93c

fixed array declaration syntax for <5.4 versions

827d53f

vaheshadunts added 2 commits March 10, 2015 06:47

updated testcases relaying on changes

43d7ef7

Tests

fca3767

TomasVotruba reviewed Mar 10, 2015
View reviewed changes

lexer check 'as' > 'AS'

a09f1cb

Ocramius reviewed Mar 16, 2015
View reviewed changes

Ocramius added WIP Requires merge/review labels Mar 16, 2015

Syntax fixing as suggested

dfc1264

Stylistic changes

c674a15

Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Mar 23, 2015

Merge branch 'hotfix/doctrine#1325-paginator-order-by-sorting-fixes' …

8cf0dc7

…into hotfix/doctrine#1342-paginator-functional-test-integration

Ocramius closed this in dc99ed2 Mar 24, 2015

Ocramius self-assigned this Mar 24, 2015

Ocramius removed the WIP label Mar 24, 2015

doctrinebot mentioned this pull request Dec 6, 2015

DDC-3606: [GH-1325] fixed PostgreSQL and Oracle pagination issues #4430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed PostgreSQL and Oracle pagination issues #1325

fixed PostgreSQL and Oracle pagination issues #1325

vaheshadunts commented Mar 9, 2015

doctrinebot commented Mar 9, 2015

TomasVotruba Mar 9, 2015

vaheshadunts commented Mar 9, 2015

TomasVotruba Mar 10, 2015

TomasVotruba commented Mar 10, 2015

mvrhov commented Mar 10, 2015

vaheshadunts commented Mar 10, 2015

vaheshadunts commented Mar 10, 2015

mvrhov commented Mar 11, 2015

stof commented Mar 11, 2015

Ocramius Mar 16, 2015

Ocramius commented Mar 16, 2015

vaheshadunts commented Mar 17, 2015

Ocramius commented Mar 17, 2015

Ocramius commented Mar 24, 2015

vaheshadunts commented Mar 24, 2015

Ocramius commented Mar 24, 2015

vaheshadunts commented Mar 24, 2015

vaheshadunts commented Mar 24, 2015

vaheshadunts commented Mar 25, 2015

billschaller commented Mar 25, 2015

billschaller commented Mar 26, 2015

vaheshadunts commented Mar 26, 2015

billschaller commented Mar 26, 2015

stof commented Mar 26, 2015

vaheshadunts commented Mar 26, 2015

billschaller commented Mar 26, 2015

vaheshadunts commented Mar 26, 2015

vaheshadunts commented Mar 27, 2015

fixed PostgreSQL and Oracle pagination issues #1325

fixed PostgreSQL and Oracle pagination issues #1325

Conversation

vaheshadunts commented Mar 9, 2015

doctrinebot commented Mar 9, 2015

TomasVotruba Mar 9, 2015

Choose a reason for hiding this comment

vaheshadunts commented Mar 9, 2015

TomasVotruba Mar 10, 2015

Choose a reason for hiding this comment

TomasVotruba commented Mar 10, 2015

mvrhov commented Mar 10, 2015

vaheshadunts commented Mar 10, 2015

vaheshadunts commented Mar 10, 2015

mvrhov commented Mar 11, 2015

stof commented Mar 11, 2015

Ocramius Mar 16, 2015

Choose a reason for hiding this comment

Ocramius commented Mar 16, 2015

vaheshadunts commented Mar 17, 2015

Ocramius commented Mar 17, 2015

Ocramius commented Mar 24, 2015

vaheshadunts commented Mar 24, 2015

Ocramius commented Mar 24, 2015

vaheshadunts commented Mar 24, 2015

vaheshadunts commented Mar 24, 2015

vaheshadunts commented Mar 25, 2015

billschaller commented Mar 25, 2015

billschaller commented Mar 26, 2015

vaheshadunts commented Mar 26, 2015

billschaller commented Mar 26, 2015

stof commented Mar 26, 2015

vaheshadunts commented Mar 26, 2015

billschaller commented Mar 26, 2015

vaheshadunts commented Mar 26, 2015

vaheshadunts commented Mar 27, 2015