feat: add tags streams #68

tipogi · 2024-08-15T16:23:10Z

Pre-submission Checklist

For tests to work you need a working neo4j instance with the example dataset in docker/db-migration

Code Quality: Clippy has been run with no warnings, cargo clippy
Testing: Implement and pass new tests for all new code, while maintaining existing test suite, cargo test.
Performance: Ensure new code has relevant performance benchmarks, cargo bench

SHAcollision

Nice work!! 🚀 I left a few comments.

Automated tests are not yet existing in this PR as it was opened before we created the workflows. You need to run cargo fmt for tests to not fail in main.

SHAcollision · 2024-08-20T10:14:40Z

src/db/graph/queries.rs

@@ -208,3 +208,71 @@ pub fn get_user_following(user_id: &str, skip: Option<usize>, limit: Option<usiz
    }
    query(&query_string).param("user_id", user_id)
 }
+
+// Retrieves popular tags across the entire network
+// Results ordered by post count (descending), effectively ranking "hot" tags.


This query does not seem to order the results by desceding post count. It would need something like this before the RETURN statement:

ORDER BY uniquePosts DESC

@tipogi This query still does not sort by descending post count. Maybe it works as intended this way, but the docstring is not correct.

SHAcollision · 2024-08-20T10:19:33Z

src/db/graph/queries.rs

+
+// Analyzes tag usage for a specific list of user IDs. Groups tags by name,
+// showing for each: label, post count, list of user IDs and total usage count. 
+// Orders by user count and usage (descending).


The ordering seems to be only by usage count (times), not by user count.

SHAcollision · 2024-08-20T10:21:35Z

src/db/graph/queries.rs

+// showing for each: label, post count, list of user IDs and total usage count. 
+// Orders by user count and usage (descending).
+// Note: Only considers users from the provided ID list.
+pub fn get_tags_from_user_ids(user_ids: &[&str]) -> Query {


NIT: when naming functions we are typically using ..._by_user_ids instead of from

SHAcollision · 2024-08-20T10:35:20Z

src/reindex.rs

+
+impl RedisOps for HotTagsScore {}
+
+async fn retrive_global_hot_tags() -> Result<(), Box<dyn Error + Send + Sync>> {


Does not belong in the reindexer.rs All this logic for reindexing should be in the Tag Stream model.

SHAcollision · 2024-08-20T10:38:33Z

src/models/tag/stream.rs

+    }
+}
+
+async fn retrieve_users_tags_by_reach(users: &[String]) -> Result<Option<Vec<HotTag>>, Box<dyn Error + Send + Sync>> {


We typically use get_ on our function names. Let's avoid using new lexicon (retrieve_) for the same meaning and functionality.

If all of these functions are specific for HotTags maybe they belong as HotTags methods. We can also avoid using hot_tags in the function names if that is the case.

SHAcollision · 2024-08-20T11:01:54Z

src/routes/v0/stream/tags.rs

+}
+
+#[derive(Deserialize)]
+pub struct StreamTagsReachInput {


So far we call these ones Query instead of Input for example PostStreamReachQuery

SHAcollision · 2024-08-20T11:06:32Z

src/models/tag/stream.rs

+pub const GLOBAL_HOT_TAGS: [&str; 3] = ["Tags", "Global", "Hot"];
+
+#[derive(Deserialize, Serialize, ToSchema, Debug)]
+pub struct HotTag {


Not sure if too late or if it matters. It feels like we have HotTag as unit and HotTags should the a Vec<HotTags> that implements all of the functionality? In the same way as our other streams are pub struct UserStream(Vec<UserView>); or pub struct PostStream(Vec<PostView>);

I do not think it matters much at all, but it is a slightly different approach :)

SHAcollision · 2024-08-20T11:09:07Z

src/routes/v0/stream/tags.rs

+
+    match HotTag::get_stream_tags_by_reach(user_id, reach).await {
+        Ok(Some(hot_tags)) => Ok(Json(hot_tags)),
+        Ok(None) => Err(Error::TagsNotFound { reach: String::from("REACH") }),


Maybe here we intended to enter the query input reach as part of the tags not found message error?

SHAcollision · 2024-08-20T11:11:26Z

src/routes/v0/stream/tags.rs

+        ("reach" = UserStreamType, Path, description = "Reach type: Follower | Following | Friends")
+    ),
+    responses(
+        (status = 200, description = "Retrieve tags by reach cluster", body = StreamTags),


StreamTags does not exist. This is maybe a good reason whay having HotTags as Vec is a good idea, makes utoipa documentation easier. Right now we get this error:

SHAcollision · 2024-08-20T11:16:11Z

src/reindex.rs

+
+    if let Some(row) = result.next().await? {
+        let hot_tags: Vec<(f64, &str)> = row.get("hot_tags")?;
+        HotTagsScore::put_index_sorted_set(


Just move this (putting the index_sorted_set) within HotTag::retrieve_hot_tags_from_graph(query) and we call it once our reindexer. Then take we take it out of ``HotTag::get_global_tags_stream(.....)` for global tags as it should always be served from reddis.

This whole function does not belong in the reindexer, there should be no need to import GLOBAL_HOT_TAGS or RedisOps, or Serde, and queries and such.

SHAcollision

Hey @tipogi, I left some comments for a few smaller changes! I think once we fix those small things and fix the merge conflicts we can merge!

SHAcollision · 2024-08-21T07:38:47Z

src/db/graph/queries.rs

        MATCH (u:User)-[tag:TAGGED]->(p:Post)
        WITH tag.label AS label, COUNT(DISTINCT p) AS uniquePosts
        WHERE uniquePosts >= $threshold
        RETURN COLLECT([toFloat(uniquePosts), label]) AS hot_tags
-    ")
+    ",
+    )
    .param("threshold", threshold as f64)


I'm not sure this threshold is a good idea. It's okay if we use more memory on this sorted set.

SHAcollision · 2024-08-21T07:38:51Z

src/db/graph/queries.rs

@@ -208,3 +208,71 @@ pub fn get_user_following(user_id: &str, skip: Option<usize>, limit: Option<usiz
    }
    query(&query_string).param("user_id", user_id)
 }
+
+// Retrieves popular tags across the entire network
+// Results ordered by post count (descending), effectively ranking "hot" tags.


@tipogi This query still does not sort by descending post count. Maybe it works as intended this way, but the docstring is not correct.

SHAcollision · 2024-08-21T07:52:59Z

src/models/tag/stream.rs

        // We cannot use here limit and skip because we want to get all the users reach by
-        let users = UserStream::get_user_list_from_reach(&user_id, reach, None, Some(10000)).await?;
+        let users =
+            UserStream::get_user_list_from_reach(&user_id, reach, None, Some(10000)).await?;


If we want to make sure to get all users we can use Some(isize::MAX as usize) instead of Some(10000). But I believe we might want to limit this to maybe ~500 users as for really big accounts this will become expensive. Same problem we have with the stream of posts by reach.

SHAcollision · 2024-08-21T07:56:11Z

src/reindex.rs

@@ -66,7 +62,10 @@ pub async fn reindex() {
        }
    }

-    retrive_global_hot_tags().await.expect("Failed to store the global hot tags");
+    info!("Retrieving tags global score...");


We are not logging info! during the rest of the reindexer. So we should either keep everything as debug! or be consistent and log the other reindexing routines with info!

Add cypher queries for tags

2a0c393

tipogi added enhancement New feature or request 🔮 nexus labels Aug 15, 2024

tipogi closed this Aug 16, 2024

tipogi deleted the feat/hot-tags branch August 16, 2024 05:51

tipogi restored the feat/hot-tags branch August 16, 2024 05:53

tipogi reopened this Aug 16, 2024

tipogi added 4 commits August 16, 2024 15:49

Add hot tags endpoint

365ee4d

Add hot tags by reach: Following, Followers, Friend, Most Followed

1b7011d

Add unit tests and benchmarks

8260a06

Hot tags scores count by post number not by tagged number

f8b4594

tipogi requested a review from SHAcollision August 20, 2024 07:45

Minor fixes

e0ea9ee

tipogi self-assigned this Aug 20, 2024

tipogi marked this pull request as ready for review August 20, 2024 09:17

Add threshold for hot tags and fix Swagger endpoints return types

6736b5c

SHAcollision reviewed Aug 20, 2024

View reviewed changes

Improvements from PR review

f62e0e7

SHAcollision approved these changes Aug 21, 2024

View reviewed changes

SHAcollision and others added 4 commits August 21, 2024 10:27

Add graph index on tag labels

58d55fb

Changes before merge to main

88692b2

Merge branch 'main' into feat/hot-tags

2530f8d

fix unclosed delimiter error

3c7b3f2

tipogi merged commit 9c96ee5 into main Aug 21, 2024
3 checks passed

tipogi deleted the feat/hot-tags branch September 11, 2024 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tags streams #68

feat: add tags streams #68

tipogi commented Aug 15, 2024 •

edited

Loading

SHAcollision left a comment

SHAcollision Aug 20, 2024 •

edited

Loading

SHAcollision Aug 21, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision Aug 20, 2024

SHAcollision left a comment

SHAcollision Aug 21, 2024

SHAcollision Aug 21, 2024

SHAcollision Aug 21, 2024

SHAcollision Aug 21, 2024


		impl RedisOps for HotTagsScore {}

		async fn retrive_global_hot_tags() -> Result<(), Box<dyn Error + Send + Sync>> {

feat: add tags streams #68

feat: add tags streams #68

Conversation

tipogi commented Aug 15, 2024 • edited Loading

Pre-submission Checklist

SHAcollision left a comment

Choose a reason for hiding this comment

SHAcollision Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SHAcollision left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tipogi commented Aug 15, 2024 •

edited

Loading

SHAcollision Aug 20, 2024 •

edited

Loading