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

fix(server): searching for multiple people yields false positives #15447

Merged
merged 1 commit into from
Jan 19, 2025

Conversation

dav-wolff
Copy link
Contributor

When searching for multiple people, only images with all of those people in them should show up. However, if an image has one person's face detected multiple times it would still show up because the query wasn't counting distinct personIds.

@@ -248,7 +248,7 @@ export function hasPeopleCte(db: Kysely<DB>, personIds: string[]) {
.select('assetId')
.where('personId', '=', anyUuid(personIds!))
.groupBy('assetId')
.having((eb) => eb.fn.count('personId'), '>=', personIds.length),
.having((eb) => eb.fn.count('personId').distinct(), '=', personIds.length),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why equality here? I think it makes sense to include assets that also have other faces.

Copy link
Contributor

@mertalev mertalev Jan 19, 2025

Choose a reason for hiding this comment

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

Maybe this would be better as a series of where exists clauses instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assets with other faces are still included. However the other faces aren't counted in the first place because the where filters for only the people that were searched for so '>=' and '=' actually has the same effect. I think then '=' is the better choice because in my opinion '>=' suggests the possibility of the count being greater than personIds.length and thus is confusing to the reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, you're only counting the filtered set. That makes sense!

@mertalev mertalev merged commit 70809c1 into immich-app:main Jan 19, 2025
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants