Add Result::fetchAllKeyValue() and ::iterateKeyValue()

[morozov]

Q	A
Type	improvement
BC Break	no

Fixes #4289.

The behavior of the newly added methods is a replacement of PDO::FETCH_KEY_PAIR which is implemented at the wrapper level and is available for all supported drivers.

Unlike PDO which requires the result to have exactly two columns, DBAL will require at least two columns in order to allow the ROWNUM columns used by the platforms that do not natively support LIIMIT queries (Oracle, IBM DB2).

[greg0ire]

Unlike PDO which requires the result to have exactly two columns, DBAL will require at least two columns in order to allow the ROWNUM columns used by the platforms that do not natively support LIIMIT queries (Oracle, IBM DB2).

Can you please elaborate on this? I'm unfamiliar with these platforms. Is that kind of column always returned in a result set?

[greg0ire]

Suggested change

	~~~~~~~~~~~~~~~~~~~~~
	~~~~~~~~~~~~~~~~~~

[greg0ire]

Suggested change

	~~~~~~~~~~~~~~~~~~~~~
	~~~~~~~~~~~~~~~~~

[morozov]

Can you please elaborate on this? I'm unfamiliar with these platforms. Is that kind of column always returned in a result set?

Yes. It's appended to the inner query for counting rows and used by the outer query for filtering.

For instance, on DB2:

$conn = DriverManager::getConnection([
    'driver'   => 'ibm_db2',
    'user'     => 'db2inst1',
    'password' => 'Passw0rd',
    'dbname'   => 'doctrine',
]);

$sql = $conn->getDatabasePlatform()
    ->modifyLimitQuery('SELECT TEST_INT, TEST_STRING from FETCH_TABLE', 1, 1);

echo $sql, PHP_EOL; // formatted for readability
/*
SELECT db22.*
FROM (
    SELECT db21.*, ROW_NUMBER() OVER () AS DC_ROWNUM
    FROM (
        SELECT TEST_INT, TEST_STRING from FETCH_TABLE
    ) db21
) db22
WHERE db22.DC_ROWNUM >= 2
  AND db22.DC_ROWNUM <= 2
*/

$data = $conn->fetchAllAssociative($sql);
var_dump($data);
/*
array(1) {
  [0] =>
  array(3) {
    'TEST_INT' =>
    int(2)
    'TEST_STRING' =>
    string(3) "foo"
    'DC_ROWNUM' =>
    string(1) "2"
  }
}
*/

Despite the fact that we selected two columns, the result contains three.

UPD: I added a test for it.

[greg0ire]

I see, the DBAL does that here:

dbal/lib/Doctrine/DBAL/Platforms/DB2Platform.php

Lines 816 to 820 in 0d4e1a8

	return sprintf(
	'SELECT db22.* FROM (SELECT db21.*, ROW_NUMBER() OVER() AS DC_ROWNUM FROM (%s) db21) db22 WHERE %s',
	$query,
	implode(' AND ', $where)
	);

Thanks for the explanation, this looks good.

Good point about adding a test also, I think you discovered a bug 😅

[morozov]

I think you discovered a bug 😅

Yeah, 4½ years ago (#2374) 😅

[morozov]

@tswcode does it look like what you're looking for?

[tswcode]

Indeed, that looks promising. I will test it as soon as I am back at work (on monday). :)

[SenseException]

My thoughts about the behaviour of these methods are written to the related lines of code.

I'm not sure about missing tests for the catched exceptions for rowCount() and columnCount(). There might be value that a DriverException is only internally handled and that the methods are converting them to the expected exceptions.

[SenseException]

From the docblock of fetchAllKeyValue:

The result must contain at least two columns.

SELECT key, value, foobar FROM my_table;

What happens to the rest of the columns? first is key, second is value, third (unrelated to rownumber) is... ? Is this defined or currently an open behaviour? If there is a fixed definition of what happens with every other column after the second, it should be put into the documentation. Like that they fetched but ignored by the methods.

This might be an interesting information for design decisions of users.

[morozov]

What happens to the rest of the columns?

Nothing. As well as nothing happens to other columns when the first column is fetched via fetchFirstColumn().

it should be put into the documentation

How would you put it?

[SenseException]

.. note::
   Any additional column to the key and value columns will be ignored by this method.

[morozov]

While I agree that this note is better than nothing, I wouldn't like the users to rely on this behavior just because it's currently implemented. How about this:

All additional columns will be ignored and are only allowed to be selected by DBAL for its internal purposes.

[SenseException]

Same as in my comment in fetchAllKeyValue doc.

[morozov]

I'm not sure about missing tests for the catched exceptions for rowCount() and columnCount(). There might be value that a DriverException is only internally handled and that the methods are converting them to the expected exceptions.

The tests are missing because there's no clear understanding of how these methods could throw an exception in any of the existing drivers. Not that it's impossible, they just were never added.

The general concept is that the driver-level methods may throw driver exceptions while the wrapper should convert them into a generic or specialized wrapper-level exception (e.g. ConnectionLost). The only reason I'm making this change now is that the new fetch mode calls columnCount() that doesn't handle the documented driver exception. But a method in the wrapper layer cannot throw a driver exception, so it must be handled. The only right place to handle the exception from the driver columnCount() is in the wrapper columnCount().

There's absolutely a lot of work on exception handling (#4160 is just the beginning).

The suggestions on improving the documentation have been addressed.

[tswcode]

Unfortunately I was busy this week, I just noticed that this feature has been added to the branch 3.0.x.

@morozov Could you also add it to the branch 2.11.x ?

[morozov]

@tswcode see #4338.