Skip to content

fix: compile parent(rel.id) or-filters as EXISTS instead of joining b… - #265

Open
TravelCurry02 wants to merge 1 commit into
ash-project:mainfrom
TravelCurry02:fix/parent-or-exists
Open

TravelCurry02 wants to merge 1 commit into
ash-project:mainfrom
TravelCurry02:fix/parent-or-exists

Conversation

@TravelCurry02

@TravelCurry02 TravelCurry02 commented Sep 23, 2026 •

Copy link
Copy Markdown

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • I accept the AI Policy, or AI was not used in the creation of this PR.
  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

What was wrong

Some of the relationships are a union of two lists. The example from the original issue is 'all_tags' it should be "this post's genre tags, or this post's mood tags."

It would be written like this:
no_attributes? true
sort :id
filter expr(parent(genre_tags.id) == id or parent(mood_tags.id) == id)

'no_attributes?' means there is no direct foreign key. Ash needs to use the 'parent( )' filter to make a decision on what rows belong.

The old SQL joined both parent lists. if a post had 2 genre tags and 1 mood tag the join could produce extra combinations of those rows. It would then 'sort :id' added a 'row_number( )' column called 'order'. 'SELECT DISTINCT' also looked at 'order' so because of this, two copies of the same tag could survive due to them looking different. You would end up with 4 tags instead of 3.

The main issue boiled down to a Cartesian product. This shouldn't be a possibility. The fix that I implemented was to turn this into EXISTS, and that is the fix that I used.

The changes

it was done in two separate pieces both in ash_sql:

  1. lib/join.ex
    If the filter is "parent(this.id) == id or parent(that.id) == id", we do not join both parent lists. When you would join them is when extra rows would be created.

  2. lib/expr.ex
    For 'parent(rel.id)== id' it compiles EXISTS instead of a normal ==. Now EXISTS looks at whether the parent has this related id. This makes it more of a true-or-false per row so the same tag cannot show up twice from two join combinations.

I did testing through local ash_postgres due to there being no Postgres test suite. I added a small 'all_tags' relationship on Post and a test that created three tags that had ids in order of genre, mood, genre. It used to duplicate the 2nd tag, but now the test returns 3 unique rows, so it passed.

@zachdaniel

Copy link
Copy Markdown
Contributor

The issue with matching on = like this is that there are many other formulations that can have this issue. I think what we need to do is make it so that when constructing a related filter, if it contains the construct that would cause this, change the whole thing to use exists.

@TravelCurry02

Copy link
Copy Markdown
Author

That makes sense, matching on only = handles the one all_tags shape. While other parent filters can still run into the join issue.

I'll look into rewriting the whole related filter to EXISTS when it would join multiple parent paths.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants