friends-of-reactphp / mysql

Async MySQL database client for ReactPHP.
MIT License
331 stars 66 forks source link

How to support duplicate column names? #151

Open clue opened 2 years ago

clue commented 2 years ago

How should we handle queries that use the same name column name multiple times?

-- Bob wins
SELECT "Alice" AS name, "Bob" AS name

-- Bob wins
SELECT "Alice" AS name, user.* FROM (SELECT "Bob" AS name) AS user

-- Alice wins
SELECT user.*, "Alice" AS name FROM (SELECT "Bob" AS name) AS user

At the moment, the value from the last occurrence wins because we expose each row as an associative array (later values with the same key overwrite previous values).

Technically, the protocol exposes a list of all values, so we do have access to all values inside the protocol implementation, but do not currently expose this to the user. Other projects expose a "fetch mode" to control whether the data should be exposed as a plain list (PDO::FETCH_NUM) or as an associative array (PDO::FETCH_ASSOC) or object (PDO::FETCH_OBJ / PDO::FETCH_CLASS), etc. (https://www.php.net/manual/en/pdostatement.fetch.php)

The current behavior can be seen as inconvenient or inconsistent, given that QueryResult::$resultFields contains a list of all(!) column names, yet QueryResult::$resultRows[0] contains an associative array with unique column names only. If you run any of the above queries with the examples/01-query.php script, you'll see that it reports a table with 2 header columns, but only reports one value per row.

Providing this kind of API isn't too much work, but I wonder how big of a problem this actually is and whether this actually warrants increasing our API surface?

boenrobot commented 1 year ago

IMHO, adding a fetch mode in general is worth it.

Keep the current default of taking the last value in an associate array (both for the sake of BC, but also for the sake of people coming from PDO), but add an indexed array option, which would return all column values mapped to the same indexes (i.e. count($queryResult->resultFields) === count($queryResult->resultRows[0])).

Adding a object mode could be worth it if that object combines the two other fetch modes, i.e. it provides ArrayAccess that would give the column at that index when given an integer, or the last column of that name if given a string. On such an object, count() and foreach behavior could perhaps default to the associative array behavior, but provide a setter to switch both to an indexed mode. This object mode would obviously be more memory consuming, since it would need to contain a reference to some "string to index" map which would add some overhead.

A "compact" object mode (akin to PDO's object mode) is not worth it I think, as in PHP, it has the same ergonomics as an associative array. If you need an stdClass, you can always just cast the associative array to object.

A "class" mode is also not worth it I think, as you still need to define a mapping anyway - might as well define it out of the built in fetch modes. Less magic = easier to reason about.

SimonFrings commented 1 year ago

Hey @boenrobot, sounds like an interesting approach :+1:

Providing this kind of API isn't too much work, but I wonder how big of a problem this actually is and whether this actually warrants increasing our API surface?

As @clue wrote above, I am not sure how big of a problem this really is or in which use cases you run into this exact scenario. As I see it there are still ways to get around this without adding new functionality to this project. But if you have a specific use case and an idea to implement this, PR's are always welcome ;)