fix: compile parent(rel.id) or-filters as EXISTS instead of joining b… - #265
Open
TravelCurry02 wants to merge 1 commit into
Open
TravelCurry02 wants to merge 1 commit into
TravelCurry02 wants to merge 1 commit into
Conversation
Contributor
|
The issue with matching on |
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
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
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.
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.