Skip to content

Fix not_in search with deep nesting #1537

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

giovannelli
Copy link

Description

This pull request addresses issue #1411, where the generated SQL query for excluding articles based on associated tags was incorrect (I've used tags the my example).

The current query joins and filters tags improperly, resulting in an unintended query structure. Specifically, the subquery logic used for filtering tags produces incorrect results and may impact performance.

SELECT "articles".* 
FROM "articles" 
LEFT OUTER JOIN "comments" 
  ON "comments"."disabled" = 0 
  AND "comments"."article_id" = "articles"."id" 
WHERE ('default_scope' = 'default_scope') 
  AND "articles"."id" NOT IN (
    SELECT "comments_tags"."comment_id" 
    FROM "comments_tags" 
    INNER JOIN "tags" 
      ON "tags"."id" = "comments_tags"."tag_id" 
    WHERE "comments_tags"."comment_id" = "articles"."id" 
      AND NOT ("tags"."name" NOT IN ('Fantasy', 'Scifi'))
  )

The solution modifies the query structure to use proper joins and filtering, simplifying the SQL and ensuring correct results.

SELECT "articles".* 
FROM "articles" 
LEFT OUTER JOIN "comments" 
  ON "comments"."disabled" = 0 
  AND "comments"."article_id" = "articles"."id" 
LEFT OUTER JOIN "comments_tags" 
  ON "comments_tags"."comment_id" = "comments"."id" 
LEFT OUTER JOIN "tags" 
  ON "tags"."id" = "comments_tags"."tag_id" 
WHERE ('default_scope' = 'default_scope') 
  AND "tags"."name" NOT IN ('Fantasy', 'Scifi')

@giovannelli giovannelli force-pushed the main branch 2 times, most recently from eafb922 to 4566158 Compare November 18, 2024 14:12
@bopm
Copy link

bopm commented Jun 2, 2025

@clgiovannelli I've incorporated your changes into #1561 as I'm dealing with the complex scenario that requires multiple interconnected changes from your PR, #1279, and my own to be applied at the same time. I also added additional test coverage. Tell me if you're ok with that.

@clgiovannelli
Copy link

@clgiovannelli I've incorporated your changes into #1561 as I'm dealing with the complex scenario that requires multiple interconnected changes from your PR, #1279, and my own to be applied at the same time. I also added additional test coverage. Tell me if you're ok with that.

Thank you for taking the time to look into this. I'll check your changes today.

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