fix: two sort bugs with uniq? aggregates and belongs_to joins - #263
Open
jeroen11dijk wants to merge 2 commits into
Open
jeroen11dijk wants to merge 2 commits into
jeroen11dijk wants to merge 2 commits into
Conversation
…ield `array_agg(DISTINCT x ORDER BY y)` is rejected by postgres with 42P10 unless every ORDER BY expression also appears in the argument list. A `uniq?: true` list aggregate reached that error two ways: an explicit `sort` on the aggregate, and a `sort` declared on the relationship it traverses, which was inherited silently. The ordering key is exactly what deduplication discards, so the sort cannot be honoured either way. Keep only the sort terms on the aggregated field and fall through to the existing unsorted `array_agg(DISTINCT ?)` branch otherwise. Sorting a uniq? aggregate by the field it aggregates is unaffected.
`related_subquery` was passed `sort?` twice in the same keyword list. It reads the option with `Keyword.get/3`, which returns the first match, so `sort?: Map.get(relationship, :from_many?)` never took effect. Keyword lists permit duplicate keys, so nothing warned. The result is that a sort declared on a relationship's read action is carried into the join subquery for a `belongs_to`, where the join discards the ordering again. On a large table that is a full sort per query. Combine both intents rather than dropping either, so a caller passing `sort?: false` can still opt out.
7 tasks
zachdaniel
reviewed
Sep 19, 2026
| # than emitted as invalid SQL - it is unsatisfiable either way, since the ordering | ||
| # key is exactly what deduplication throws away. This also keeps a `sort` declared | ||
| # on the relationship from reaching an aggregate that never asked to be sorted. | ||
| defp distinct_safe_sort(sort, aggregate) do |
Contributor
There was a problem hiding this comment.
Hmm...I merged the other tests PR, but looking at the implementation here I'm not so sure. We're basically discarding the sort here in practice. The results are no longer sorted in the way you'd expect them to have been sorted.
I'm not actually sure what SQL formulation here would satisfy it though so it makes me think that we may actually want to reject this formulation entirely somehow, requiring that users set like a sorted? false option that will discard the underlying sort?
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
2 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.
A list aggregate with
uniq?: truecan have a sort on a field that it does not aggregate. The code then makes the SQLarray_agg(DISTINCT x ORDER BY y). Postgres rejects this SQL with error 42P10. A sort on the aggregate causes this. A sort on the relationship also causes this, because the aggregate takes that sort. DISTINCT removes the field that you sort on, so the sort cannot work. This commit removes the sort.In lib/join.ex,
related_subquerygetssort?two times in the same keyword list. Only the first value applies, and the check below it never operates. A belongs_to join then sorts a subquery, but the join removes the sort again. My table has 221946 rows, and the query took 341 ms. Without the ORDER BY, it takes 2 ms. The tests are in ash-project/ash_postgres#858. Tell me if the first fix must give an error instead, when the user writes the sort.Contributor checklist
Leave anything that you believe does not apply unchecked.