Add support for LIMIT OFFSET for db2 #275

akrabat · 2017-10-13T20:20:00Z

Since 2016, DB2 has supported the LIMIT x OFFSET y construct. This PR supports the new construct in a BC manner via a feature flag. To use with TableGateway:

$tableGateway = new TableGateway($table, $adapter);
$decorator = $tableGateway->getSql()->getSqlPlatform()->getDecorators()['Zend\Db\Sql\Select'];
$decorator->setSupportsLimitOffset(true);

samsonasik · 2017-10-15T00:41:24Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

+                    array_push($sqls, sprintf("LIMIT %s", $limit));
+                }
+            }
+            return;


I think deep if can be simplified with return early:

if (! $this->limit) { return; } if ($this->offset) { array_push($sqls, sprintf("LIMIT %s OFFSET %s", $limit, $offset)); return; } array_push($sqls, sprintf("LIMIT %s", $limit)); return;

samsonasik · 2017-10-15T01:04:00Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

+            // Note: db2_prepare/db2_execute fails with positional parameters, for LIMIT & OFFSET
+            $limit = (int)$this->limit;
+            $offset = (int)$this->offset;
+            if ($this->limit) {


$this->limit is not casted to (int) one, I think should use $limit instead

samsonasik · 2017-10-15T01:05:02Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

+            $limit = (int)$this->limit;
+            $offset = (int)$this->offset;
+            if ($this->limit) {
+                if ($this->offset) {


I think should use if ($offset) instead as it is the casted with (int) one

akrabat · 2017-10-16T09:22:53Z

@samsonasik Addressed.

samsonasik · 2017-10-16T11:12:03Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

+        if ($this->supportsLimitOffset) {
+            // Note: db2_prepare/db2_execute fails with positional parameters, for LIMIT & OFFSET
+            $limit = (int)$this->limit;
+            $offset = (int)$this->offset;


$offset never used if ! $limit, can be define later (after line 114)

mwillbanks

Just formatting, other than the formatting, everything here looks great.

mwillbanks · 2017-10-20T02:38:25Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

@@ -82,6 +104,23 @@ protected function processLimitOffset(PlatformInterface $platform, DriverInterfa
            return;
        }

+        if ($this->supportsLimitOffset) {
+            // Note: db2_prepare/db2_execute fails with positional parameters, for LIMIT & OFFSET
+            $limit = (int)$this->limit;


space after the cast.

mwillbanks · 2017-10-20T02:38:37Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

+        if ($this->supportsLimitOffset) {
+            // Note: db2_prepare/db2_execute fails with positional parameters, for LIMIT & OFFSET
+            $limit = (int)$this->limit;
+            if (! $limit) {


no space after the !

most of zf code has space after !, I think already part of zf cs /cc @webimpress

@mwillbanks
@samsonasik is right. See zendframework/zend-coding-standard#1 - Generic.Formatting.SpaceAfterNot

@mwillbanks
Yes, and this is even part of current version of zend-coding-standard (1.0.0): https://github.com/zendframework/zend-coding-standard/blob/master/ruleset.xml#L12

mwillbanks · 2017-10-20T02:38:48Z

src/Sql/Platform/IbmDb2/SelectDecorator.php

+                return;
+            }
+
+            $offset = (int)$this->offset;


space after the cast.

guidofaecke · 2017-10-31T16:10:25Z

Just took a quick look at the code.
I assume "ORDER BY" is still functional, since "ROW_NUMBER() OVER()" is now "missing"?

akrabat · 2017-11-02T14:36:39Z

Just took a quick look at the code.
I assume "ORDER BY" is still functional, since "ROW_NUMBER() OVER()" is now "missing"?

Correct - ORDER BY is supported as you'd expect if you've used Zend\Db with MySQL for instance.

akrabat · 2017-11-02T14:37:34Z

@mwillbanks Requested changes are done.

ezimuel · 2017-11-23T13:31:35Z

@akrabat thanks for your contribution!

akrabat added 2 commits October 13, 2017 21:13

Add support for LIMIT OFFSET for db2

ea15efa

Add tests for DB2 limit offset

ead49e6

samsonasik reviewed Oct 15, 2017

View reviewed changes

Return early

0d7d8ef

samsonasik suggested changes Oct 16, 2017

View reviewed changes

Only define $offset if needed

fecf111

mwillbanks suggested changes Oct 20, 2017

View reviewed changes

froschdesign added the enhancement label Oct 20, 2017

Add a space after cast

547215b

ezimuel merged commit 547215b into zendframework:develop Nov 23, 2017

Add support for LIMIT OFFSET for db2 #275

Add support for LIMIT OFFSET for db2 #275

Uh oh!

Conversation

akrabat commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akrabat commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwillbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidofaecke commented Oct 31, 2017

Uh oh!

akrabat commented Nov 2, 2017

Uh oh!

akrabat commented Nov 2, 2017

Uh oh!

ezimuel commented Nov 23, 2017

Uh oh!

Uh oh!