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(delete-persons): Use tuple to compose query #28080

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

tomasfarias
Copy link
Contributor

Problem

Composing the query produces something like id IN (['a', 'b']) which is not valid SQL.

Changes

  • Cast person_ids input to a tuple to compose a sql query like id IN ('a', 'b').

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses an SQL query composition issue in the delete_persons workflow, fixing invalid SQL syntax when filtering by person IDs.

  • Fixed SQL query composition in posthog/temporal/delete_persons/delete_persons_workflow.py by converting person_ids list to tuple format for valid IN clause syntax (e.g. IN ('a', 'b') instead of IN (['a', 'b']))
  • Potential inconsistency identified between mogrify_delete_queries_activity and delete_persons_activity functions where the latter still uses incorrect format
  • String formatting in log message may need review for consistency with the new tuple format

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@tomasfarias tomasfarias force-pushed the fix/use-tuple-to-compose-queries branch from 3732e92 to 0aa7a43 Compare January 30, 2025 10:58
@tomasfarias tomasfarias enabled auto-merge (squash) January 30, 2025 11:00
@tomasfarias tomasfarias force-pushed the fix/use-tuple-to-compose-queries branch from 0aa7a43 to 77ee022 Compare January 30, 2025 12:56
@tomasfarias
Copy link
Contributor Author

Tests seem broken unrelated to this PR. I'm rebasing to see if that helps.

@benjackwhite benjackwhite disabled auto-merge January 30, 2025 13:32
@benjackwhite benjackwhite merged commit 7cee8c1 into master Jan 30, 2025
90 of 92 checks passed
@benjackwhite benjackwhite deleted the fix/use-tuple-to-compose-queries branch January 30, 2025 13:32
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.

2 participants