Oracle platform ignores OFFSET in case if LIMIT is not specified

[morozov]

There's a non-empty table users. The following script should return all existing record IDs except the 1st one.

$builder = $conn->createQueryBuilder();
$builder->select('id')
    ->from('users')
    ->setFirstResult(1);
$data = $builder->execute()->fetchAll();
var_dump($data);

On Oracle, it returns all records ignoring the value set by calling setFirstResult().

[deeky666]

Can you please remove the integer casting entirely and just rely on the method signature integer|null for both $limit and $offset? Then just check everywhere against null. Also please remove the $has* variables as they are unnecessary.

[morozov]

@deeky666 The confusing part here is how is $offset = null different from $offset = 0 from the SQL semantics standpoint? Should the DBAL produce additional sub-query in the latter case? Right now it doesn't which makes more sense to me.

UPD: comparing $offset to null will break unit test:

1) Doctrine\Tests\DBAL\Platforms\OraclePlatformTest::testModifyLimitQuery
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'SELECT a.* FROM (SELECT * FROM user) a WHERE ROWNUM <= 10'
+'SELECT * FROM (SELECT a.*, ROWNUM AS doctrine_rownum FROM (SELECT * FROM user) a WHERE ROWNUM <= 10) WHERE doctrine_rownum >= 1'

[deeky666]

@morozov yeah good point. But then leave it as before casting both parameters to int and checking for 0 instead.

[morozov]

@deeky666 but unlike OFFSET, for LIMIT 0 and null have different meanings — "0 records" and "all records" respectively. There's no complete consistency between existing adapters but all of them except SQL Anywhere only omit $limit when it's null. Technically, it is possible to execute a query and retrieve 0 records from it, so the DBAL shouldn't be a limiting factor here. Also from the API consumer standpoint, if there's a bug in the calculation of $limit on the application end and it's calculated as 0, I'd better expect 0 records to be returned instead of all of them.

[deeky666]

@morozov sounds reasonable. Thanks for clearing that up. I'll accept it as is now.

[deeky666]

Can you please use sprintf() here? Guess that makes it more readable.

[deeky666]

@morozov sounds reasonable. Thanks for clearing that up. I'll accept it as is now.

[deeky666]

@morozov thank you very much!

[deeky666]

Backported to 2.5 via b9a3b08

[morozov]

@deeky666 thank you for your patience. Can you also review #2463 which is a similar one?