Repository navigation
feat(flags): mirror the events schema in flag_evaluations - #80994
Conversation
|
😎 Merged successfully - details. |
🤖 CI reportℹ️ ClickHouse migration SQL — 1 migration(s)ClickHouse migration SQL per cloud environment
|
Prompt To Fix All With AI### Issue 1
posthog/clickhouse/migrations/0294_flag_evaluations_events_mirror.py:43-54
**Replacement schema lacks compatibility window**
If old and new workers overlap during deployment, this migration removes the typed columns while installing their properties-based replacements, leaving no schema version compatible with both worker generations and causing old inserts or queries to reference missing columns.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(flags): mirror the events schema in..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Updates the ClickHouse flag_evaluations table family to mirror the core events schema (while keeping the full properties JSON), so existing queries/integrations built on $feature_flag_called properties will keep working when these events are routed off the events table.
Changes:
- Replaces the previous typed/minimal
flag_evaluationsschema with an events-like column set including rawpropertiesJSON, plus a 90-day TTL and events-aligned partitioning/sharding/sort key. - Adds 9 materialized columns derived from
properties($group_0..$group_4,flag_key,response,session_id,request_id) on the sharded storage table, and mirrors them as plain columns on the Distributed read table (omitted from Kafka + writable tables). - Adds/updates schema verification to assert the read proxy declares every stored column, preventing drift between sharded storage and Distributed read schemas.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| posthog/models/flag_evaluations/sql.py | Reworks the ClickHouse DDL templates for the whole flag_evaluations family (events-like columns, materialized props, indexes, MV projection). |
| posthog/clickhouse/test/test_schema.py | Adds a regression test that compares sharded vs Distributed-read declared columns/types (after stripping modifiers). |
| posthog/clickhouse/test/snapshots/test_schema.ambr | Updates the golden snapshot for the new table definitions and MV projection. |
| posthog/clickhouse/migrations/max_migration.txt | Advances the max ClickHouse migration pointer to 0294. |
| posthog/clickhouse/migrations/0294_flag_evaluations_events_mirror.py | Drop-and-recreate migration for the entire table family to apply the new schema safely (given it’s empty). |
| posthog/clickhouse/hcl/sql/local-multi/data.sql | Regenerates local-multi SQL output to reflect the updated schema. |
| posthog/clickhouse/hcl/roles/data/local/tables.hcl | Updates the HCL source-of-truth for the flag_evaluations columns and materialized column patches. |
| posthog/clickhouse/hcl/golden/local-multi/data.hcl | Updates the generated HCL golden file for local-multi. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ), | ||
| run_sql_with_exceptions( | ||
| FLAG_EVALUATIONS_TABLE_SQL(), | ||
| node_roles=[NodeRole.DATA], |
There was a problem hiding this comment.
Can we put this table onto AUX cluster?
There was a problem hiding this comment.
We discussed offline. The scale warrants keeping this on main for now.
382d189 to
ecbfe59
Compare
The table family now carries the events table's column set, narrowed to what a flag evaluation actually has, with the full properties JSON as the source of truth. Queries and integrations written against $feature_flag_called properties keep working once evaluations move off the events table. A 90-day TTL makes rows that wide affordable. Nine materialized columns come out of properties: the events table's $group_0..$group_4 keys, verbatim, plus flag_key, response, session_id and request_id. A property earns one only when hot-path queries filter or group on it across many rows, or a skip index needs a real column to sit on. They are declared on the sharded table, which computes them, and repeated plain on the Distributed read table, which computes nothing. No column carries a codec. Migration 0297 drops and recreates the family rather than altering it. Nothing produces to the topic yet, so every table is empty; recreating from the canonical template cannot drift from a fresh install, and it avoids DROP COLUMN mutations that can get stuck and block a release. Generated-By: PostHog Code Task-Id: a95149f1-e894-43a4-a0ea-5f93010cf268
ecbfe59 to
94d0eb6
Compare
|
/trunk merge |

Problem
$feature_flag_calledproperties.propertiesJSON.Changes
propertiesJSON as the source of truth, replacing the typed flag columns from 0292.properties: the events table's$group_0..$group_4keys, copied verbatim, plusflag_key,response,session_id, andrequest_id.flag_key,response), or a skip index needs a real column to sit on (session_id,request_id).responsewithtrim_quotes(JSONExtractRaw(...)), notJSONExtractString. The two agree on strings, booleans, and numbers. They differ only on JSON null, where this form stores the'null'sentinel that HogQL maps back to SQL NULL, matching how the events table materializes its own columns.The physical design is the part to review closely: monthly
PARTITION BY toYYYYMM(timestamp),ORDER BY (team_id, flag_key, toDate(timestamp), cityHash64(distinct_id)), a 90-day TTL withttl_only_drop_parts, andsipHash64(distinct_id)sharding to match events so a later backfill runs shard-local. The rest of the diff is the SQL templates, the migration, the regenerated HCL and snapshot, and one test.Note
This table stores the same person and group PII as events (
properties,person_properties,person_id, group properties), but the property-deletion scanners are scoped to theeventstable (posthog/models/data_deletion_request.py:814). A deletion request would not sweep this table, so the 90-day TTL is the only erasure control. Nothing stores rows today. Tracked as #81002, which has to be resolved before this table gets a producer: either the deletion scanners cover it, or we document that the TTL is the accepted erasure window.How did you test this code?
$feature_flag_calledmessage to theclickhouse_flag_evaluationstopic and read it back through theflag_evaluationsproxy. It flowed Kafka table, materialized view, writable Distributed, and sharded storage, where the shard computed the materialized columns. A boolean$feature_flag_responsestored as'false'.flag_keyworks inside the sort key, both in ClickHouse and against repo precedent (logs34andtrace_spanssort on materialized columns).hcl/check.shreports no differences for every env and role, and the generated DDL is byte-identical tosql.py.test_flag_evaluations_read_table_declares_every_stored_column. It catches drift between the storage table's materialized columns and the read proxy's plain copies, which nothing else asserts. I mutation-tested it against a dropped column, a type change, and a broken parse anchor.Automatic notifications
Docs update
None. No user-facing behavior or documented workflow changes, and the table has no query surface yet.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
/clickhouse-migrations,/writing-tests,/writing-code-comments,/simplify,/review-code,/writing-pr-descriptions.responseextraction from the original spec'sJSONExtractString. Measured against a live ClickHouse node, the spec's assumption about boolean handling was wrong, and the real difference is only the JSON-null sentinel./simplifypass hoisted duplicated column comments onto the shared abstract HCL block, removing 27 source lines with byte-identical generated output. A/review-code --fixpass then ran seven review dimensions and renamed one test helper.Created with PostHog Code