fix: report the primary key in the combination fieldset - #2836
Merged
zachdaniel merged 2 commits intoAug 4, 2026
Merged
Conversation
Two records that agree on every field a combination selects are still two records, because they differ on the primary key and `select/3` always selects it. Nothing pinned that, so a change to how the combination fieldset is derived could start collapsing them. Passes against unmodified code.
`select/3` always selects the primary key and any always-selected attributes, so every combination query has them regardless of what its `select:` asked for. The fieldset handed to the data layer was built from that raw `select:` instead, so it under-reported what the parts actually select. Derive it through `select/3`, which is what builds the part queries, so the two cannot disagree. The fieldset is what a data layer uses to decide which fields the combination makes available. Under-reporting the primary key tells it a field is absent from a query that does select it — in `ash_sql` that reaches `maybe_subquery_upgrade`, which treats any reference outside the fieldset as requiring a join.
matt-beanland
force-pushed
the
fix/ets-combination-requires-primary-key
branch
from
August 4, 2026 15:39
62de026 to
5046bb8
Compare
matt-beanland
changed the title
Contributor
|
🚀 Thank you for your contribution! 🚀 |
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.
Contributor checklist
Leave anything that you believe does not apply unchecked.
Closes #2835
Summary
Reworked based on guidance from review of #2835
Now the primary key is always selected, so the fix is to make the combination fieldset say so.
Cause
select/3already adds the primary key and the always-selected attributes to every select(
lib/ash/query/query.ex:1702-1705), and each combination part is built through it. But thefieldset was built from the raw
select:on the combination, not from the queryselect/3produced, so it under-reported what the parts select. For a part with
select: [:region]:combination.select[:region]select/3makes of it[:id, :region]combination_fieldset[:region]The change
Derive the fieldset through
select/3, so it cannot disagree with the queries that functionbuilds. Always-selected attributes are now reported too, for the same reason.
Nothing in a data layer changes.
Ash.DataLayer.Etskeys its dedupe off the built query's selectrather than the fieldset, which is why it kept records distinct already.
Commits
test:pin that a union of narrowly selected parts keeps records distinct Passes as is.fix:derive the fieldset throughselect/3, and assert the primary key is in it.