chore(migrations): add person_id to precalculated properties #43818
Conversation
posthog/clickhouse/migrations/0194_add_person_id_to_precalculated_person_properties.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds a migration to include a person_id column in the precalculated_person_properties table in ClickHouse, which is part of the work referenced in PR #43816.
Key changes:
- Updates max_migration.txt to reference the new migration
- Creates migration 0193 to add
person_idUUID column afterdistinct_idin the sharded table - Recreates distributed tables, writable tables, Kafka tables, and materialized views to incorporate the schema change
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| posthog/clickhouse/migrations/max_migration.txt | Updates migration pointer to 0193 |
| posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py | Adds migration to alter sharded table and recreate dependent tables with person_id column |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py
Outdated
Show resolved
Hide resolved
dustinbyrne
left a comment
There was a problem hiding this comment.
I've looked through this pretty extensively. It looks good. Up to you if you'd prefer someone with deeper ClickHouse knowledge take a look. 👍 👍
…sql.py to current schema with person_id
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py">
<violation number="1" location="posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py:17">
P2: Missing `IF EXISTS` clause in ALTER TABLE statement. Per the migration guidelines in AGENTS.md, all ALTER TABLE statements should include `IF EXISTS` to prevent migration failures if the table doesn't exist.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py">
<violation number="1" location="posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py:18">
P1: Missing `IF NOT EXISTS` clause. Per the ClickHouse migrations documentation, always use `IF NOT EXISTS` for `ADD COLUMN` statements to ensure migrations are idempotent and can be safely re-run.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py">
<violation number="1" location="posthog/clickhouse/migrations/0193_add_person_id_to_precalculated_person_properties.py:17">
P2: Migration removes `IF EXISTS` from `ALTER TABLE`, violating the documented convention in AGENTS.md which states: "Always use `IF EXISTS` or `IF NOT EXISTS` clauses: `ALTER TABLE IF EXISTS`". This makes the migration less resilient - it will fail if the table doesn't exist instead of being a safe no-op.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
posthog/clickhouse/migrations/0194_add_person_id_to_precalculated_person_properties.py
Show resolved
Hide resolved
|
hey @PostHog/clickhouse, if you could please take the time to review this. |
| @@ -0,0 +1,48 @@ | |||
| from posthog.clickhouse.client.connection import NodeRole | |||
There was a problem hiding this comment.
while this is in general OK, I'd suggest to change the order and:
-
drop MV
-
drop kafka table
-
drop writable table
-
drop dist table
-
modify the sharded table
6.+ and recreate them in reverse order
ClickHouse can deal with any order, but logically this is the one following dependencies
Migration to add
person_idtoprecalculated_person_propertiestable.Context: #43816