Fix paginated association subqueries on MariaDB - #19596
Conversation
5739401 to
41431f0
Compare
|
Any chance of this fix being merged in the master branch? As far as I can tell this could potentially fix the issue I am seeing with MariaDB. But I do not know if this change might potentially screw up something else. |
|
@ADmad What would it take to get this to make it to the master? What needs to be done for this to become an accepted fix? It would mean a lot to me if I would not have to resort to adding |
|
@alexvanniel As you can see by my comments the review is already underway, so please be patient. Repeatedly asking "what will it take to get it merged" isn't going to speed up the process. |
My apologies. I had no idea what the course of action would be on this. Rest assured I was not trying to urge anyone on to speed this up. I was merely concerned this would become stale. So, as you said, I will be patient. |
0daeefc to
7268795
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts ORM subquery-based eager loading to avoid a MariaDB optimizer misbehavior that can return incorrect/empty associated rows on paginated pages beyond the first. It does so by switching deduplication from GROUP BY to SELECT DISTINCT for MariaDB filter subqueries, while preserving existing grouping behavior for other drivers and explicit grouping queries.
Changes:
- Use
SELECT DISTINCT(instead ofGROUP BY) to deduplicate MariaDB subquery-strategy filter subqueries when the source query is not explicitly grouped. - Ensure ordered columns can be included in the reduced subquery select list (needed for
DISTINCT+ORDER BYcompatibility). - Update/add regression assertions around the generated SQL for the subquery join.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ORM/Association/Loader/SelectLoader.php | Switch deduplication strategy to DISTINCT for MariaDB and optionally include ORDER BY fields in the subquery select list. |
| tests/TestCase/ORM/Query/QueryRegressionTest.php | Update regression coverage to assert MariaDB uses DISTINCT and other drivers retain GROUP BY. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use DISTINCT for ungrouped association filter queries so MariaDB does not apply its lateral GROUP BY optimization. Preserve explicit grouping and select ordered columns for PostgreSQL compatibility. Refs cakephp#19595.
7268795 to
daec949
Compare
|
I added one test on top to harden it. |
dereuromark
added
defect
needs squashing
| $driver = $filterQuery->getDriver(); | ||
| $useDistinct = !$filterQuery->clause('group') | ||
| && $driver instanceof Mysql | ||
| && $driver->isMariadb(); |
There was a problem hiding this comment.
Having driver sniffs in the ORM code isn't ideal. Could use one of the DriverFeatureEnum values instead? Adding another DriverFeatureEnum option would be preferable as well.
There was a problem hiding this comment.
I don't see how one of the existing DriverFeatureEnum cases can be used for this and adding a new case in a patch release would be a breaking change for any custom driver implementation as the new case would not be handled by its supports() method.
There was a problem hiding this comment.
Ok, lets move forward with this then, and in 5.5 we can expand DriverFeatureEnum.
Add new cases to DriverFeatureEnum for recently added driver specific behavior related to subquery association loading. I'd like to avoid having driver specific logic in the ORM layer. We have Driver::supports() which allows us to model this driver specific behavior like we do for other driver/dialect specific feature support. Refs #19596 Refs #19552
…9619) * Add DriverFeatureEnum for recently added driver specific behavior Add new cases to DriverFeatureEnum for recently added driver specific behavior related to subquery association loading. I'd like to avoid having driver specific logic in the ORM layer. We have Driver::supports() which allows us to model this driver specific behavior like we do for other driver/dialect specific feature support. Refs #19596 Refs #19552 * Apply suggestions from code review Co-authored-by: othercorey <[email protected]> --------- Co-authored-by: othercorey <[email protected]>
Use
SELECT DISTINCTto deduplicate ungrouped MariaDB association-filter subqueries without triggering the lateralGROUP BYoptimization that can apply pagination per joined row.Explicit grouping semantics and other database drivers retain
GROUP BY. Ordered columns are included in the reduced select list where MariaDB requires them.The regression coverage verifies both the generated SQL and the contained results from a page with a nonzero offset.
Fixes #19595.