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

Improve query for getting users for review #1891

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions src/handlers/assign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -820,13 +820,21 @@ async fn filter_by_capacity(
.collect::<Vec<&str>>()
.join(",");

// We need to select everyone from the users database here, even if
// they might be missing in the review_prefs table.
// Therefore, we use a LEFT JOIN to get ALL users plus their associated data
// from the review_prefs, if there are any.
//
// Then we filter users whose assigned PRs are large enough.
// The final WHERE clause filters only users from the input list.
let q = format!(
"
SELECT username
FROM review_prefs r
JOIN users on users.user_id=r.user_id
AND username = ANY('{{ {} }}')
AND CARDINALITY(r.assigned_prs) < LEAST(COALESCE(r.max_assigned_prs,1000000))",
SELECT u.username AS username
FROM users AS u
LEFT JOIN review_prefs AS r ON u.user_id = r.user_id
AND COALESCE(CARDINALITY(r.assigned_prs), 0) < COALESCE(r.max_assigned_prs, 100000)
WHERE u.username = ANY('{{ {} }}')
",
Comment on lines +832 to +837
Copy link
Contributor

@apiraino apiraino Jan 27, 2025

Choose a reason for hiding this comment

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

Just an observation: before your change, this function didn't return any record if no "joined" user was found. Changing the query as you suggest also changes the API, now it always return something so the caller must check if the returned candidate actually has capacity.

There is a TODO at the top of has_user_capacity() (which does more or less the same work) and that was implemented in #1893. I'll probably split that patch to make it clearer.

EDIT: in commit b93064a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so my query assumes that the only way the capacity could be affected is through the review_prefs column. Which I assume should be true? Apart from the on vacation list, that has to be checked elsewhere (until we get rid of it).

In other words, if you don't have an entry in review_prefs, then you have capacity.

usernames
);
let result = db.query(&q, &[]).await.context("Select DB error")?;
Expand Down
3 changes: 1 addition & 2 deletions src/handlers/pull_requests_assignment_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ WHERE r.user_id = $1;";
let row = db
.query_one(q, &[&(user_id as i64)])
.await
.context("Error retrieving review preferences")
.unwrap();
.context("Error retrieving review preferences")?;
Ok(row.into())
}
Loading