Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MBS-10088: Correctly check for cardinality on l_x_x rels #990

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

reosarevok
Copy link
Member

https://tickets.metabrainz.org/browse/MBS-10088

The current query used "${source_id}_cardinality = 0" if $use_cardinality; to exclude relationships affected by cardinality.

That works great, except on l_x_x relationships where both sides of the relationship need to be checked for cardinality. In that case, only entity1 (the final value of $source_id) was being checked, meaning if entity_1 had cardinality=1 it would always return no results, even if the entity we were looking for was entity0 with cardinality=0.

This changes it so that the condition strings are actually fully built inside the $type checks, which ensures we'll get "WHERE (entity0 IN (?) AND entity0_cardinality = 0) OR (entity1 IN (?) AND entity1_cardinality = 0)" rather than "WHERE (entity0 IN (?) OR entity1 IN (?)) AND entity1_cardinality = 0".

Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic itself makes sense to me. I think the $target variable still makes no sense when type0 = type1 and cardinality is in use, but that was already broken (and it appears to only affect $order below).

lib/MusicBrainz/Server/Data/Relationship.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Data/Relationship.pm Outdated Show resolved Hide resolved
The current query used "${source_id}_cardinality = 0" if $use_cardinality; to exclude relationships affected by cardinality.

That works great, except on l_x_x relationships where *both* sides of the relationship need to be checked for cardinality. In that case, only entity1 (the final value of $source_id) was being checked, meaning if entity_1 had cardinality=1 it would always return no results, even if the entity we were looking for was entity0 with cardinality=0.

This changes it so that the condition strings are actually fully built inside the $type checks, which ensures we'll get "WHERE (entity0 IN (?) AND entity0_cardinality = 0) OR (entity1 IN (?) AND entity1_cardinality = 0)" rather than "WHERE (entity0 IN (?) OR entity1 IN (?)) AND entity1_cardinality = 0".
@reosarevok
Copy link
Member Author

Well, in that case $target will just be $type0, but since both types have the same value, it doesn't really matter - the important thing is that it will be "artist" or whatever and will work. Unless I'm missing something.

@mwiencek
Copy link
Member

mwiencek commented Apr 3, 2019

Oops, I meant more $target_id than $target, since it uses that to join to the entity table for sorting (by name). $target_id may not correspond to the correct entity when cardinality is in use.

@reosarevok
Copy link
Member Author

Oh. I thought about that, and at first was like "oh, I should join both if they're both the same entity type!" - but that just meant the query gave the rels twice. So I decided not to touch it. Open to ideas on how to improve it though. That wouldn't be connected to cardinality though, would it? If there's an issue, there's an issue with every l_x_x table query.

@mwiencek
Copy link
Member

mwiencek commented Apr 3, 2019

Yeah, you are right it isn't connected to cardinality, I suppose it just made it more obvious to me. I think the only real way to fix it is, we'd have to join the target table twice (for both entity0 and entity1) if the types are the same, then use some CASE statement in the ORDER BY to figure out which name to use (but that might be crazy and not work at all). Anyway, this can be merged on its own.

Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@reosarevok reosarevok merged commit 05c2190 into metabrainz:master Apr 8, 2019
@reosarevok reosarevok deleted the MBS-10088 branch April 8, 2019 09:50
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.

3 participants