Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the Result::getColumnName method #6428

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Conversation

stof
Copy link
Member

@stof stof commented Jun 7, 2024

Q A
Type feature
Fixed issues #5980

Summary

This method allows introspecting the shape of results by knowing the name of columns (combined with the columnCount which already exists).

@stof stof force-pushed the column_name branch 4 times, most recently from da1b092 to 9cb8d6c Compare June 7, 2024 12:24
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think the portability middleware needs a special implementation of that new method.

src/Driver/Middleware/AbstractResultMiddleware.php Outdated Show resolved Hide resolved
src/Result.php Outdated Show resolved Hide resolved
@stof stof force-pushed the column_name branch 2 times, most recently from fac07c7 to 2d8ee65 Compare June 7, 2024 13:43
@stof
Copy link
Member Author

stof commented Jun 7, 2024

@derrabus indeed, the Portability result needed to handle the case conversion when using its case conversion feature. I added support for it. And

This method allows introspecting the shape of results by knowing the
name of columns.
@derrabus derrabus merged commit ee586a8 into doctrine:4.1.x Jun 19, 2024
76 of 77 checks passed
derrabus added a commit that referenced this pull request Jun 19, 2024
* 4.1.x:
  Add the Result::getColumnName method (#6428)
@derrabus
Copy link
Member

Thank you! I've opened #6452 as a follow-up.

derrabus added a commit that referenced this pull request Jun 19, 2024
|      Q       |   A
|------------- | -----------
| Type         | improvement
| Fixed issues | Follows #6428

#### Summary

Following #6428, the new `getColumnName()` method becomes mandatory with
this PR for all `Result` implementations.
@stof stof deleted the column_name branch June 19, 2024 16:21
@morozov
Copy link
Member

morozov commented Jul 22, 2024

Thank you @stof.

This is a great addition to the API which should unblock some improvements to the result caching. Specifically, the following issues now can be addressed:

  1. If the result contains no rows, its cached version won't return the number of columns.
  2. If the result contains columns with the same name (e.g. SELECT a.id, b.id FROM a, b), it won't be cached properly.

Note that the just implemented ArrayResult::getColumnName() is prone to a similar issue: if the result contains no rows, the original result will return column names but the cached version will throw an exception for any column number.

In order to address these issues, instead of accepting list<array<string, mixed>> $data, the ArrayResult constructor could accept (list<string> $columnNames, list<list<mixed>> $data).

derrabus pushed a commit that referenced this pull request Aug 29, 2024
|      Q       |   A
|------------- | -----------
| Type         | bug

#### Summary

Prior to #6428, there were two problems with caching results by design:

1. If the result contains no rows, its cached version won't return the
number of columns.
2. If the result contains columns with the same name (e.g. `SELECT a.id,
b.id FROM a, b`), they will clash even if fetched in the numeric mode.

See #6428 (comment)
for reference.

The solution is:
1. Handle column names and rows in the cache result separately. This
way, the column names are available even if there is no data.
2. Store rows as tuples (lists) instead of maps (associative arrays).
This way, even if the result contains multiple columns with the same
name, they can be correctly fetched with `fetchNumeric()`.

**Additional notes**:
1. The changes in `TestUtil::generateResultSetQuery()` were necessary
for the integration testing of the changes in `ArrayResult`. However,
later, I realized that the modified code is database-agnostic, so
running it in integration with each database platform would effectively
test the same code path and would be redundant. I replaced the
integration tests with the unit ones but left the change in `TestUtil`
as it makes the calling code cleaner.
2. `ArrayStatementTest` was renamed to `ArrayResultTest`, which Git
doesn't seem to recognize due to the amount of changes.

**TODO**:
- [x] Decide on the release version. This pull request changes the cache
format, so the users will have to clear their cache during upgrade. I
wouldn't consider it a BC break. We could also migrate the cache at
runtime, but given that this cache can be cleared, I don't think it is
worth the effort.
- [x] Add upgrade documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants