Group the subquery filter by the columns it is ordered on - #19580
Open
dereuromark wants to merge 2 commits into
Open
Group the subquery filter by the columns it is ordered on#19580dereuromark wants to merge 2 commits into
dereuromark wants to merge 2 commits into
Conversation
Since 5.3 the filtering subquery of the subquery strategy keeps its ORDER BY
when the source query is limited, and since 5.4 that ORDER BY actually survives
into the generated SQL. The subquery reduces its SELECT to the binding key and
groups by it, so ordering it by anything else leaves Postgres with a column that
appears in neither the GROUP BY nor an aggregate:
SELECT Countries.iso2 FROM countries Countries
WHERE Countries.id = 1
GROUP BY Countries.iso2
ORDER BY Countries.sort DESC
LIMIT 1
SQLSTATE[42803]: Grouping error: column "Countries.sort" must appear in the
GROUP BY clause or be used in an aggregate function
Grouping by a primary key hides this, as Postgres then resolves the other
columns of that table through functional dependency, which is why the existing
coverage stayed green. Any other binding key, or an ORDER BY on a joined table,
fails outright.
The ordered columns now join the GROUP BY. Constant SELECT values and aggregates
stay out of it, and plain SQL fragments are left alone since their column cannot
be told apart from the sort direction. Grouping by more columns never drops a
binding key the parent query matched, so the filtered set stays complete.
ADmad
reviewed
Aug 2, 2026
…ore prefix The SQL assertion filtered the logged statements by "FROM comments", which only matches while identifier quoting is off, so it found nothing in a full suite run. It now collects the statements through a local logger and picks the only grouped one, independent of quoting and of the global Log configuration.
Member
Author
|
Renamed both, thanks. Also pushed a fix for the SQL assertion in the new test: it filtered logged statements by a raw |
markstory
reviewed
Aug 3, 2026
Comment on lines
+1972
to
+1985
| $logger = new class extends AbstractLogger { | ||
| /** | ||
| * @var array<string> | ||
| */ | ||
| public array $messages = []; | ||
|
|
||
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function log($level, string|Stringable $message, array $context = []): void | ||
| { | ||
| $this->messages[] = (string)$message; | ||
| } | ||
| }; |
Member
There was a problem hiding this comment.
Couldn't we use Cake\Log\Engine\ArrayLog here?
|
@dereuromark thanks for this PR. I had noticed similar Postgres errors since moving to 5.4.1 (I assume due to the subquery strategy change) but the updates here resolve all my issues so far. I look forward to hopefully seeing this in 5.4.2, very much appreciated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #19538.
The filtering subquery of the
subquerystrategy reduces its SELECT to the binding key and groups by it. Since 5.3 it keeps the ORDER BY when the source query is limited, and since #19538 that ORDER BY actually reaches the generated SQL - it used to be wiped as a side effect ofiterateParts(). Ordering the subquery by a column that is in neither the GROUP BY nor an aggregate is invalid on Postgres:Grouping by a primary key hides it, because Postgres then resolves the remaining columns of that table through functional dependency. That is exactly what the existing coverage does (
orderBy(['Authors.id' => 'DESC'])), so CI stayed green. It breaks as soon as the binding key is not the primary key, or the ORDER BY points at a joined table.Real-world report: a
hasManywith'bindingKey' => 'iso2'plus a default table order started erroring on Postgres the day 5.4.0 was released, on plugin code that had not changed.What changed
_subqueryFields()now adds the ordered columns to the GROUP BY, and only looks at the ORDER BY when the subquery is actually going to keep it (_buildSubquery()drops it when there is no limit, and the columns would then be dead weight in the reduced SELECT).Left out of the GROUP BY on purpose:
select(['score' => 100]), which are not columnsorderBy('Articles.title DESC'), where the column cannot be told apart from the sort directionNamed parts,
orderByAsc()/orderByDesc()and expression parts are all covered.Grouping by additional columns can only split groups, never merge them, so the first N groups still cover the N rows the parent query matched - the filtered key set stays complete.
The aggregate detection that the HAVING branch already carried is extracted into
isAggregate()and shared.