Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Fix Limit with order by on SQL Server 2008 R2 #344

Closed
wants to merge 1 commit into from

7 participants

Michele Tedeschi doctrinebot Marco Pivetta Benjamin Eberlei Guilherme Blanco Steve Müller Markus Staab
Michele Tedeschi

Fix Limit with order by on SQL Server 2008 R2

Found problem when using this bundle:

  • cedriclombardot/admingenerator-generator-bundle
  • jms/job-queue-bundle

This fix resolve the problem of wrong column name on ROW_NUMBER() OVER.

Tested on:

  • SQL Server 2008 R2
  • php_pdo_sqlsrv_54
  • Symfony 2.3.2
  • Doctrine 2.4.0 RC2

Now work paging and paging with order by.

Michele Tedeschi micheletedeschi Fix Limit with order by on SQL Server 2008 R2
Fix Limit with order by on SQL Server 2008 R2

Found problem when using this bundle:
- cedriclombardot/admingenerator-generator-bundle
- jms/job-queue-bundle

This fix resolve the problem of wrong column name on ROW_NUMBER() OVER.

Tested on:
- SQL Server 2008 R2
- php_pdo_sqlsrv_54
- Symfony 2.3.2
- Doctrine 2.4.0 RC2

Now work paging and paging with order by.
3e538d5
doctrinebot
Collaborator

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/DBAL-560

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

Marco Pivetta
Collaborator

@micheletedeschi apart from minor CS problems: what is the issue solved here? Can you provide a failing test that demonstrates what the problem is and that this patch fixes it?

Benjamin Eberlei
Owner

Please add a failing test-case

Michele Tedeschi

Both modules use pagerfanta/pagerfanta bundle for paging.

With jms/job-queue-bundle the generated query are the following:

SELECT 
  * 
FROM 
  (
    SELECT 
      DISTINCT id0, 
      ROW_NUMBER() OVER (
        ORDER BY 
          j0_.id DESC
      ) AS doctrine_rownum 
    FROM 
      (
        SELECT 
          j0_.id AS id0, 
          j0_.state AS state1, 
          j0_.createdAt AS createdAt2, 
          j0_.startedAt AS startedAt3, 
          j0_.checkedAt AS checkedAt4, 
          j0_.executeAfter AS executeAfter5, 
          j0_.closedAt AS closedAt6, 
          j0_.command AS command7, 
          j0_.args AS args8, 
          j0_.output AS output9, 
          j0_.errorOutput AS errorOutput10, 
          j0_.exitCode AS exitCode11, 
          j0_.maxRuntime AS maxRuntime12, 
          j0_.maxRetries AS maxRetries13, 
          j0_.stackTrace AS stackTrace14, 
          j0_.runtime AS runtime15, 
          j0_.memoryUsage AS memoryUsage16, 
          j0_.memoryUsageReal AS memoryUsageReal17 
        FROM 
          jms_jobs j0_ WITH (NOLOCK) 
        WHERE 
          j0_.originalJob_id IS NULL
      ) dctrn_result
  ) AS doctrine_tbl 
WHERE 
  doctrine_rownum BETWEEN 1 
  AND 20
SELECT 
      j0_.id AS id0, 
      j0_.state AS state1, 
      j0_.createdAt AS createdAt2, 
      j0_.startedAt AS startedAt3, 
      j0_.checkedAt AS checkedAt4, 
      j0_.executeAfter AS executeAfter5, 
      j0_.closedAt AS closedAt6, 
      j0_.command AS command7, 
      j0_.args AS args8, 
      j0_.output AS output9, 
      j0_.errorOutput AS errorOutput10, 
      j0_.exitCode AS exitCode11, 
      j0_.maxRuntime AS maxRuntime12, 
      j0_.maxRetries AS maxRetries13, 
      j0_.stackTrace AS stackTrace14, 
      j0_.runtime AS runtime15, 
      j0_.memoryUsage AS memoryUsage16, 
      j0_.memoryUsageReal AS memoryUsageReal17, 
      j0_.originalJob_id AS originalJob_id18, 
      ROW_NUMBER() OVER (
        ORDER BY 
          closedAt6 DESC
      ) AS doctrine_rownum 
    FROM 
      jms_jobs j0_ WITH (NOLOCK) 
    WHERE 
      j0_.state IN ('terminated', 'failed')
      AND j0_.originalJob_id IS NULL

SQL Server 2008 R2 error are the following:

SQLSTATE[42000]: [Microsoft][SQL Server Native Client 11.0][SQL Server]The multi-part identifier "j0_.id" could not be bound.

and

SQLSTATE[42S22]: [Microsoft][SQL Server Native Client 11.0][SQL Server]Invalid column name 'closedAt6'

The correct queries are:

     ROW_NUMBER() OVER (
        ORDER BY 
          id0 DESC
      ) AS doctrine_rownum 

and

     ROW_NUMBER() OVER (
        ORDER BY 
          closedAt DESC
      ) AS doctrine_rownum 

With cedriclombardot/admingenerator-generator-bundle the generated query is the following:

SELECT 
  * 
FROM 
  (
    SELECT 
      DISTINCT id0, 
      ROW_NUMBER() OVER (
        ORDER BY 
          t0_.body ASC
      ) AS doctrine_rownum 
    FROM 
      (
        SELECT 
          t0_.id AS id0, 
          t0_.title AS title1, 
          t0_.body AS body2 
        FROM 
          Test t0_ WITH (NOLOCK)
      ) dctrn_result
  ) AS doctrine_tbl 
WHERE 
  doctrine_rownum BETWEEN 11 
  AND 20

SQL Server 2008 R2 error are the following:

SQLSTATE[42000]: [Microsoft][SQL Server Native Client 11.0][SQL Server]The multi-part identifier "t0_.body" could not be bound.

The correct query is:

     ROW_NUMBER() OVER (
        ORDER BY 
          body2 DESC
      ) AS doctrine_rownum 
Marco Pivetta
Collaborator

@micheletedeschi what is needed is a test case that asserts correct SQL is being generated, not a description ;)

Michele Tedeschi

I've update test case #351

Benjamin Eberlei
Owner

@micheletedeschi do both PRs belong together?

Guilherme Blanco

Isn't it better to fix only SQL Server 2008 Platform?
I've seen limit-subquery algorithm being updated over and over and I have a tendency to say that fixing one version breaks another one, even though I've never used SQL Server.
I'd try to stick the fix purely on that driver, but I'd also like that @deeky666 take a look into this fix.

Steve Müller deeky666 commented on the diff
lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -818,7 +818,26 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
818 818 return sprintf($format, $query, $start, $end);
819 819 }
820 820
821   - //Clear ORDER BY
  821 + //Clean and split SELECT into parts
  822 + $select = str_replace('DISTINCT','',explode('FROM', stristr($query, 'SELECT')));
  823 + $selectParts = explode(',', preg_replace('/SELECT\s+([^\)]*)(.*)/', '$1', $select[0]));
  824 + $selectColumns = array();
  825 +
  826 + foreach ($selectParts as $part) {
  827 + $alias = explode("AS", $part);
3
Steve Müller Collaborator
deeky666 added a note

Are you sure this works? What if there is a column called "GRASS", then it would be split into "GR" as column name and "S" as alias. This would break the query IMO and seems to be pretty unsafe.

Markus Staab
staabm added a note

it seams only to work on "AS" and not e.g. "as" or "aS" also.

Steve Müller Collaborator
deeky666 added a note

Yes and it would recognize column aliases where actually are no column aliases if the column name itself contains "AS".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Steve Müller
Collaborator

@guilhermeblanco To be honest I cannot say whether this PR makes this whole messy LIMIT query problems better or worse. My personal feeling about this is that all those explodes and strreplaces MAY work on some queries but not on others where the query contains identifier names that also partly match reserved keywords like "FROM", "AS" and so on which get exploded here. This is not exactly a problem introduced in this PR (also this PR tends to make the LIMIT query evaluation even more unsafe) but a general challenge here.
@micheletedeschi Is this problem really only related to SQL Server 2008 R2? Have you tested this on other versions of the platform? If this bug only occurs in this particular version I would tend to stick to @guilhermeblanco 's opinion to fix it only on the particular platform. Although I would like to see some edge case tests for the possible problems I stated before. I'm pretty sure that there will be some and that they have to be covered also by this patch.

Michele Tedeschi

The pull request #383 from flip111/patch-5 solve my problems with a solution better than mine

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

Showing 1 unique commit by 1 author.

Jul 17, 2013
Michele Tedeschi micheletedeschi Fix Limit with order by on SQL Server 2008 R2
Fix Limit with order by on SQL Server 2008 R2

Found problem when using this bundle:
- cedriclombardot/admingenerator-generator-bundle
- jms/job-queue-bundle

This fix resolve the problem of wrong column name on ROW_NUMBER() OVER.

Tested on:
- SQL Server 2008 R2
- php_pdo_sqlsrv_54
- Symfony 2.3.2
- Doctrine 2.4.0 RC2

Now work paging and paging with order by.
3e538d5
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 24 additions and 1 deletion. Show diff stats Hide diff stats

  1. +24 1 lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
25 lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -818,7 +818,26 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
818 818 return sprintf($format, $query, $start, $end);
819 819 }
820 820
821   - //Clear ORDER BY
  821 + //Clean and split SELECT into parts
  822 + $select = str_replace('DISTINCT','',explode('FROM', stristr($query, 'SELECT')));
  823 + $selectParts = explode(',', preg_replace('/SELECT\s+([^\)]*)(.*)/', '$1', $select[0]));
  824 + $selectColumns = array();
  825 +
  826 + foreach ($selectParts as $part) {
  827 + $alias = explode("AS", $part);
  828 + $alias = (count($alias) > 1) ? trim($alias[1]) : false;
  829 +
  830 + if (preg_match('/(([^\s]*)\.)?([^\.\s]*)\s*?/i', trim($part), $matches)) {
  831 + $selectColumns[$matches[0]] = array(
  832 + 'column' => $matches[3],
  833 + 'alias' => $alias,
  834 + 'hasTable' => (!empty($matches[2])),
  835 + 'table' => empty($matches[2]) ? '[^\.\s]*' : $matches[2]
  836 + );
  837 + }
  838 + }
  839 +
  840 + //Clear ORDER BY
822 841 $orderBy = preg_replace('/ORDER\s+BY\s+([^\)]*)(.*)/', '$1', $orderBy);
823 842 $orderByParts = explode(',', $orderBy);
824 843 $orderbyColumns = array();
@@ -845,6 +864,10 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
845 864 ? ($column['hasTable'] ? $column['table'] . '.' : '') . $column['column']
846 865 : $column['column'];
847 866
  867 + if(!isset($selectColumns[$overColumn]) && isset($matches[3])) {
  868 + $overColumn = $matches[3];
  869 + }
  870 +
848 871 if (isset($column['sort'])) {
849 872 $overColumn .= ' ' . $column['sort'];
850 873 }

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.