fix: compile parent(rel.id) or-filters as EXISTS instead of joining b… - #265
TravelCurry02 wants to merge 2 commits into
Conversation
|
The issue with matching on |
|
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. |
|
I rewrote it so it doesn't match on '=' anymore. Now, if a related filter would join two or more parent to-many paths, it skips those joins and rewrites that filter to 'exists', keeping the 'or' / 'and'. So '== id', '== title', and '>' all go through the same path. I re-ran the all_tags case in ash_postgres (three unique tags). I also added a title union and a > filter there, both passed. Those tests aren't in this PR because they are in ash_postgres. |
Contributor checklist
Leave anything that you believe does not apply unchecked.
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:
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.
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.