Creating a new issue for this. I've have a testbed with 26 users, alpha to zulu by alphabet. I've done some testing before with adding and removing users, so there are a few connections in there that are probable flagged TW_1_TO_2_D and TW_2_TO_1_D. To test the block I have these relations set up:

Alpha: Charlie, Juliet, Romeo
Charlie: Alpha, Juliet, Hotel
Juliet: Alpha, Charlie, Hotel
Hotel: Charlie, Juliet, Romeo

Since Alpha and Hotel have exactly the same friends, but are not each others friends, it is safe to say, they probably know each other. As per second-degree.

Now the list that alpha returns as possible people is way longer! Eventhough these are the only friends these users have. Now here's the kicker...they all, including the ones listed as possible matches, have had user "Beta" as connection in the past. Beta was a jerk, so everybody removed him form their friendlist. However it seems that his previous relation is still being equated? Is that possible?

That's the only possible solution I can think of why Alpha has 10 people in common instead of just 1 namely "Hotel".

Regards,

Marius

Comments

mariusooms’s picture

mariusooms commented
Title: Block "people you may know already" » Block "people you may know" returning to many results.

Title is not really helpful is it...also I just realzie the block was set to return 10...I changed it to 30...now alpa has 17 possible matches. Something, somewhere it is going wrong. I looked in my db and indeed there are a lot of timecodes for tw_disregarded_time. Would it be possible to only consider fr_requester_id and fr_requestee_id when tw_disregarded_time == 0?

Regards,

Marius

PS. Oops...also my grammar is bad...it should be "too many" not "to many" for the title...oh well

mercmobily’s picture

mercmobily commented

Hi,

Actually, this slipped on me too...

Ice, a friendship is a established when the status is "TW_BOTH". To know the status, you need to left join with friendlist_statuses which should be straightforward.

I can _try_ and change the query, Ice, but if you could, that would be grand...

Merc.

mercmobily’s picture

mercmobily commented
Title: Block "people you may know" returning to many results. » Block "people you may know" returning too many results.

Hi,

I attempted to fix the query, limiting the search to established relationships.

This query is a little beyond me. However, the one problem I have is that it seems to be based on UR's concept of a friendship record that might be with the user being on one side or the other side.

Ice, to check if user A is friends with user B, ALL you have to do is check if requester_id is user A and requestee_id is user B and the status is TW_BOTH. That's it. No need to check both ways.

Doesn't that simplify the query _quite a lot_?

If so, please paste the simplified query and we'll give it a test!

Merc.

mercmobily’s picture

mercmobily commented
Category: support » bug

Hi,

It *IS* a bug after all :-D

Merc.

icecreamyou’s picture

icecreamyou commented

Well first of all... who are Romeo's friends?

Second, I guess AND tw_disregarded_time == 0 should be added to the WHERE clause of the overall query and all three subqueries.

And third... yeah, that means things can be simplified a lot. Will look into it, but I'm pretty pressed for time, and won't have a good way to test.

icecreamyou’s picture

icecreamyou commented

I take it back: I think the query can't be simplified any because we need the UIDs from both sides, not just whether the relation exists. But I'm having a hard time wrapping my head around the DB structure, and I don't have a way to test, so who knows... but I do know that it will work this way whether or not it can be simplified.

I'm not 100% sure whether this will work or not, honestly. I mean, it will return something useful, but it might not be exactly what we want.

SELECT u.uid
FROM {users} AS u
LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
  AND (fr.requester_id IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fr.tw_disregarded_time = 0)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ) OR fr.requestee_id IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fr.tw_disregarded_time = 0)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ))
  AND (fr.rtid = %d)
  AND (u.uid NOT IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fr.tw_disregarded_time = 0)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ))
  AND (fr.tw_disregarded_time = 0)
  AND (fs.status = 'TW_BOTH')
GROUP BY (u.uid)
ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
mercmobily’s picture

mercmobily commented

Hi,

That's exactly how I changed it.
Marius, can you give it a bit of a test?

To answer Ice's questions:

"Well first of all... who are Romeo's friends?"

Anybody for which requester_id == Romeo, and status == TW_BOTH. Simple. No need at all to check if Romeo is in requestee_id.

"Second, I guess AND tw_disregarded_time == 0 should be added to the WHERE clause of the overall query and all three subqueries."

No... if the status is TW_BOTH, then there is _nothing_ else to check. The status is _god_ in this module.
And... do we need to check for the connection in the *third* query as well?

"And third... yeah, that means things can be simplified a lot. Will look into it, but I'm pretty pressed for time, and won't have a good way to test."

OK. I saw your other message... let us know!

This is the query as it is now:



SELECT u.uid
FROM {users} AS u
LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
  AND (fr.requester_id IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fs.rid = fr.rid)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ) OR fr.requestee_id IN (
    SELECT u.uid
    FROM {users} AS u
LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fs.rid = fr.rid)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ))
  AND (fr.rtid = %d)
  AND (u.uid NOT IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ))
GROUP BY (u.uid)
ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC

But I am not sure you'd approve. And i am not sure it works -- marius got stuck with a silly bug in the module (my fault), so he wasn't able to test!

Merc.

icecreamyou’s picture

icecreamyou commented

1) I mean that literally. The code could have been returning users who were friends with Romeo, but I can't tell from the information given.

2) Yes, we need to check the connection in the third subquery, as well as the overall query (both of which you left out). That's because the third subquery makes sure that the current user doesn't show up in their own results, and the overall query checks for friends of friends. If the status check was left out, it would grab users who weren't mutual friends.

So, I suggest you use this query:

SELECT u.uid
FROM {users} AS u
LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
  AND (fr.requester_id IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ) OR fr.requestee_id IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ))
  AND (fr.rtid = %d)
  AND (u.uid NOT IN (
    SELECT u.uid
    FROM {users} AS u
    LEFT JOIN {friendlist_relations} AS fr ON (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
    LEFT JOIN {friendlist_statuses} AS fs ON (fr.requester_id = fs.requester_id AND fr.requestee_id = fs.requestee_id)
    WHERE (u.uid = fr.requester_id OR u.uid = fr.requestee_id)
      AND (fr.requester_id = %d OR fr.requestee_id = %d)
      AND (fr.rtid = %d)
      AND (fs.status = 'TW_BOTH')
    GROUP BY (u.uid)
    ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC
  ))
  AND (fs.status = 'TW_BOTH')
GROUP BY (u.uid)
ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC

It is unfortunate that this is rather hefty. I'm inclined to believe that for sites with a lot of users and relationships, it would be faster to use two queries and remove the subqueries. That is, it would be faster to create two database connections and have the second query compare against a predefined set of users (two DB connections, two faster DB lookups) than it would be to have one query with three subqueries (one DB connection, four slow DB lookups). But I don't have any data to support that. (I should add that I think it is possible to store the subquery in a prepared statement in the SQL and re-use it later, but this varies widely in SQL types and I have no experience with it.)

And also, I'm pretty sure the query can't be simplified per se. fs.status = 'TW_BOTH' is the same thing as ur.approved = 1 in my original code. I could be wrong though, as I may still not fully understand the way relationship records are inserted into the database.

Where it possibly could be simplified would be to remove the lookup through the {user} table. I originally did that to grab the username and avatar instead of doing a full user_load(), but since it turns out that user_load() is the right way to go, we *should* just be able to use the requester_id/requestee_id. The problem is that they're different columns, and the best way to combine them is with a UNION, but that would require basically running the entire query twice.

mercmobily’s picture

mercmobily commented

Hi,

OK, you are totally right. Query added.
Now... let's get this one to work first. There is always time for improvements.

Bye,

Merc.

icecreamyou’s picture

icecreamyou commented

Indeed.

Having looked into it, I can safely say now that the {user} lookup must stay in because the only alternative is adding a lot of UNIONs which is way worse performance-wise.

And I must say, I enjoy working with someone who is equally neurotic about fast responses. ;)

mercmobily’s picture

mercmobily commented

Hi,

Ahahahahahah :-D
We are a good team then!

Thanks a lot... let's see what Marius says about the query. We need to get this sorted, because we need to get FriendList finished (!) and start polishing up Activity Log -- another much needed glue module for Drupal.

Bye!

Merc.

mariusooms’s picture

mariusooms commented

Hi Merc and IceCreamYou, thanks for working on this...after reinstalling the whole lot and clearing the db I've done some testing and it works, even when relations statuses get altered.

But...I have a request to still optimize it more, since currently it is to easy to make a connection with a second degree. Let me explain:

Alpha => Bravo, Charlie, Delta
Bravo => Alpha, Charlie, Delta
Charlie => Alpha, Bravo, Delta
Delta => Alpha, Bravo, Charlie, Echo
Echo => Delta

Okay, here the first three users are all friends with each other. Echo is the new guy. Delta makes a connection, so now by rule of second degree, Alpha, Bravo and Charlie receive info that Echo might be a familiar. Vice-versa, Echo sees Alpha, Bravo and Charlie as a possible familiar. HOWEVER:

Just because one of the friends has made a connection with the new guy, it shouldn't, just yet, be shown as a possible. Is it a possibility to only release this new guy as a possible after more connections have been established? To be clear: Alpha should only see Echo as a possible after 3 connections are equal, so Bravo, Charlie and Delta are friends, so the odds now are a lot higher that Alpha knows this person.

I hope that makes sense, otherwise connections will go beserk and will not be accurate by any means...the idea I described could help raising the accuracy, but I don't know if that is possible at all.

Regards,

Marius

mercmobily’s picture

mercmobily commented

Hi,

I am all for it.
Ice...?

Merc.

icecreamyou’s picture

icecreamyou commented

It already works that way. The users who are listed as "people you might know" are listed in the order of how many of your friends know them. So for example if Bravo, Charlie, and Delta all know Foxtrot, then Alpha will see Foxtrot's name appear above Echo's.

At least, I think it works that way. Looking at the query again, it may be that ORDER BY COUNT(fr.requester_id OR fr.requestee_id) DESC needs to be changed to ORDER BY (COUNT(fr.requester_id) + COUNT(fr.requestee_id)) DESC or similar.

mercmobily’s picture

mercmobily commented

Hi,

Can you guys give it a bit of a test, and let me know what the "good" query is?
I am asking because I still don't have an example database to play with (shame on me) and this is the very last thing that is holding back release :-D

Merc.

mariusooms’s picture

mariusooms commented

It already works that way. The users who are listed as "people you might know" are listed in the order of how many of your friends know them.

Thanks for clarifying that...I'm afraid my testbed is not big enough to thoroughly test this out. Since this is a new module, it would be good to let this code settle until user base has grown. This applied to both blocks, mutual connections and this particular block.

this is the very last thing that is holding back release :-D

I suggest that we can leave these two blocks to the development sections, include them in a version 1 release as is, but improve them over time. Once the user base has grown, the functionality can be tested more thoroughly. Let's move forward with the release and move the blocks into the 1.1 version.

Any thoughts on that approach?

Marius

icecreamyou’s picture

icecreamyou commented

I suppose it's worth noting that I'm using code that's almost identical to what's in this thread on a production site with UR (http://www.babelup.com/), but UR has almost the same DB structure that FL has; so I'm fairly confident that this works. The main reason to leave it out would be performance; but because it's in a sub-module (and because, naturally, it's in a block) the issue seems minimal.

In addition - we know that at the very least, it doesn't break anything, and the results it returns aren't wrong. It's possible that it doesn't return the best results, but that's not a particularly critical concern IMHO at this point.

That said, it's obviously not up to me, so make whatever decision you think will benefit the most people. :)

mariusooms’s picture

mariusooms commented

we know that at the very least, it doesn't break anything, and the results it returns aren't wrong. It's possible that it doesn't return the best results, but that's not a particularly critical concern IMHO at this point.

I agree 100% and thanks for taking time to help in this issue even though you don't even use friendlist :) That speaks volumes to me.

The main reason to leave it out would be performance; but because it's in a sub-module (and because, naturally, it's in a block) the issue seems minimal.

YES! That's also what I really appreciate about Mercs' coding practices!

Regards,

Marius

mercmobily’s picture

mercmobily commented
Status: Active » Fixed

Hi,

Marking this as "fixed". We will only be able to improve this block once the module gets traction... so, let's close it, and see what happens.
We just don't have enough data for judging this.

I will release it with the module, version 1.

Bye,

Merc.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.