Repository navigation
chore(flags): drop unused property columns from flag_evaluations - #97027
Conversation
|
😎 Merged successfully - details. |
🤖 CI report
|
| Project | Preview | Updated (UTC) |
|---|---|---|
| posthog.com | Open preview | Sep 9, 2026, 9:29 PM |
The preview should be ready in about 10 minutes. Open the preview at /handbook/engineering/.
🦔 PostHog Review reviewed this pull requestNothing worth raising this time, so here's a calming picture instead:
Resolved comments: 2 already settled |
github-actions
Bot
added
feature/feature-flags
Prompt To Fix All With AI### Issue 1
posthog/clickhouse/migrations/0315_flag_evaluations_drop_property_columns.py:47
**Comments narrate the code**
The numbered comments for the writable, sharded, read, and recreation operations only restate the calls that follow them. The repository requires comments to explain why code exists or describe a non-local consequence, not narrate visible operations. Keep the Kafka-engine rationale above, but remove these redundant labels before merging.
### Issue 2
posthog/models/flag_evaluations/sql.py:84-89
**Producer documentation is stale**
Removing these columns makes the producer contract documentation in `nodejs/src/ingestion/common/flag-evaluations/flag-evaluation-row.ts` inaccurate. It still says `person_properties` and `group0..group4_properties` are omitted so ClickHouse fills existing columns with empty strings, but those columns no longer exist. Update that rationale to describe the narrower schema so future contract changes are not based on obsolete behavior.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(flags): drop unused property colum..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Approval recommended
The migration and regenerated schema artifacts are consistent, with only a minor docstring grammar nit noted.
Pull request overview
Aligns the repo’s ClickHouse schema for the flag_evaluations table family with production by removing six unused person/group property-blob columns, eliminating now-unnecessary prod schema patch overrides, and regenerating the derived HCL/SQL/schema snapshot artifacts.
Changes:
- Drop
person_propertiesandgroup0_properties–group4_propertiesfrom theflag_evaluationsKafka table + MV, writable (ingestion) table, sharded storage table, and distributed read table via migration0315. - Remove prod
patch_tableoverrides that were compensating for this schema drift. - Regenerate local/dev SQL, HCL layers/goldens, and the schema snapshot; update the internal deletion-coverage note accordingly.
File summaries
| File | Description |
|---|---|
| posthog/models/flag_evaluations/sql.py | Removes the six blob columns from the canonical flag_evaluations schema and MV select list; adds DROP SQL constants for migration use. |
| posthog/clickhouse/test/snapshots/test_schema.ambr | Updates schema snapshot expectations to match the narrowed column set. |
| posthog/clickhouse/migrations/max_migration.txt | Bumps max migration to 0315_flag_evaluations_drop_property_columns. |
| posthog/clickhouse/migrations/0315_flag_evaluations_drop_property_columns.py | Adds migration to drop the six columns across ingestion and data roles, and recreate Kafka/MV definitions. |
| posthog/clickhouse/hcl/sql/local-single/all.sql | Regenerates local-single SQL DDL without the dropped columns. |
| posthog/clickhouse/hcl/sql/local-multi/ingestion_medium.sql | Regenerates ingestion-medium DDL (Kafka/MV/writable) without the dropped columns. |
| posthog/clickhouse/hcl/sql/local-multi/data.sql | Regenerates main-cluster DDL (sharded/distributed) without the dropped columns. |
| posthog/clickhouse/hcl/sql/dev/data.sql | Regenerates dev DDL (sharded/distributed) without the dropped columns. |
| posthog/clickhouse/hcl/roles/ingestion_medium/local/tables.hcl | Updates ingestion-medium declarative schema and MV select to remove the six columns. |
| posthog/clickhouse/hcl/roles/data/shared/flag_evaluations.hcl | Updates shared data-role declaration for flag_evaluations to remove the six columns. |
| posthog/clickhouse/hcl/roles/data/prod/tables.hcl | Removes prod patch_table overrides that previously dropped these columns. |
| posthog/clickhouse/hcl/golden/local-single/all.hcl | Regenerates golden HCL for local-single without the dropped columns. |
| posthog/clickhouse/hcl/golden/local-multi/ingestion_medium.hcl | Regenerates golden HCL for local-multi ingestion-medium without the dropped columns. |
| posthog/clickhouse/hcl/golden/local-multi/data.hcl | Regenerates golden HCL for local-multi data role without the dropped columns. |
| posthog/clickhouse/hcl/golden/dev/data.hcl | Regenerates golden HCL for dev data role without the dropped columns. |
| docs/internal/clickhouse-deletion-coverage.md | Updates internal deletion-coverage note to reflect that the blob columns no longer exist on flag_evaluations. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
The Migration risk check has not finished for this commit, so stamphog cannot tell a safe migration from a risky one yet. The review runs again on the next push, or you can re-request it once the check reports.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✓ | 486L, 13F substantive, 533L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (533L, 16F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 6473955 · reviewed head 6473955 |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
The Migration risk check has not finished for this commit, so stamphog cannot tell a safe migration from a risky one yet. The review runs again on the next push, or you can re-request it once the check reports.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✓ | 482L, 13F substantive, 529L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (529L, 16F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ e546f26 · reviewed head e546f26 |
Drops person_properties and group0..group4_properties from the flag_evaluations Kafka table, materialized view, and all three data-role tables, using a single ALTER TABLE per table. The ingestion pipeline stopped writing them and both prod ClickHouse clusters already dropped them directly, so this brings dev, local, and CI schemas in line and removes the now-redundant prod HCL patch_table overrides.
e546f26 to
11f1e13
Compare
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
The Migration risk check has not finished for this commit, so stamphog cannot tell a safe migration from a risky one yet. The review runs again on the next push, or you can re-request it once the check reports.
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✓ | 482L, 13F substantive, 529L/16F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (529L, 16F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 11f1e13 · reviewed head 11f1e13 |
There was a problem hiding this comment.
As these have already been dropped, it would be better to simply modify the previous migration to not create them in the first place, rather than add another migration on top.
There was a problem hiding this comment.
Already done — this is exactly the approach the branch now takes. The dedicated ALTER TABLE ... DROP COLUMN migration you commented on was removed, and the columns are instead dropped from the shared column template in posthog/models/flag_evaluations/sql.py. The table family's original creation migration (0292_flag_evaluations.py) renders every table from that template, so a fresh dev, local, or CI database simply never creates the six property columns, and no new migration is added — max_migration.txt still points at 0314_metrics2_chain.
One caveat worth naming: as you say, prod already has these columns dropped, and nothing here alters an already-created table. Any other environment that already ran 0292 keeps the columns until it is rebuilt — including a persistent local dev ClickHouse from before this PR.
No further changes made for this thread.
0292_flag_evaluations.py already renders its columns from the shared sql.py template, so a fresh install now creates the table without person_properties/group0-4_properties instead of needing a separate ALTER migration. Both prod clusters already dropped the columns directly, so this only changes what new environments create. Claude-Session: https://claude.ai/code/session_013r76pXC5tBHPCo27KdhXJh
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
This is a ClickHouse schema migration on a shared ingestion table (risky territory) by an author outside the owning team with only moderate familiarity, and the reviewer bot's flagged concern — that dropping person_properties breaks the property-removal deletion gate — is confirmed real by both the bot and the author, deliberately left unfixed here, with the author explicitly stating a human must decide the merge order relative to the follow-up PR (#97009) before this is safe to land anywhere.
- Author wrote 23% of the modified lines and has 24 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot].
- Confirmed-real deletion-gate breakage (UNKNOWN_IDENTIFIER on person_properties) is deliberately deferred to a separate PR (#97009) rather than fixed here
- Author explicitly flags that merge order between this PR and #97009 needs human coordination — no automated path can safely decide that
- Author (haacked) is not on the owning @PostHog/clickhouse team and has only MODERATE familiarity, so no independent assurance covers this risky schema change on the current head (both existing reviews are on an older commit)
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 413L, 12F substantive, 458L/14F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (458L, 14F, two-areas, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 5da4714 · reviewed head 5da4714 |
Generated-By: PostHog Desktop Task-Id: 5f23354b-ab54-402e-b9c8-3f6301e43ca6
The property-removal gate test inserted into flag_evaluations.person_properties, a column this branch removes from the table template. The test passed on master and on this branch alone, but failed in the merge queue, where the two combine. The gate drops the person_properties half of its check for flag_evaluations, so the column is not needed to prove that behavior. Every case now writes to the properties column. A gate that kept the person_properties half would query a column the table does not have, so the cases still fail if that behavior regresses. Generated-By: PostHog Desktop Task-Id: 5f23354b-ab54-402e-b9c8-3f6301e43ca6
Problem
Anyone writing a ClickHouse migration against
flag_evaluationssees a schema that no longer matches prod. Both prod clusters already dropped six person and group property columns directly, outside this repo's migration tooling, so dev, local, and CI still create a table prod no longer has.Discussed in Slack.
Changes
flag_evaluationstable, from dev, local, or CI, no longer hasperson_propertiesorgroup0_properties..group4_properties.0292_flag_evaluations.py, the table's original creation migration, already renders every table in the family from the shared column template insql.py.ALTER TABLEmigration.patch_tableoverrides inroles/data/prod/tables.hclthat recorded the same drift, since the base schema now matches prod directly.person_propertieson this table at all now, not just skip the event side.Note
Prod already matches, since its columns were dropped directly on the cluster. Any other environment that already ran the original
0292migration keeps the columns until it's rebuilt. Nothing here alters an already-created table. A persistent local dev ClickHouse from before this PR falls in that group.Note
The property-removal deletion gate in
posthog/dags/data_deletion_requests.pystill queriesperson_propertiesonflag_evaluationsunconditionally, and errors once the column is gone. Both prod clusters already dropped it, so that gate is broken there today regardless of this PR. The fix is a separate PR (#97009).How did you test this code?
test_flag_evaluations_mv_projection_matches_column_templateandtest_flag_evaluations_read_table_declares_every_stored_column, existingtest_schema.pyinvariants, derive their expectations from the same column template this PR narrows. All 386 cases intest_schema.pypass.posthog/clickhouse/hcl/check.shlocally, the pinnedhclexptool, which validates every role in every environment against its golden file and regeneratessql/for freshness. Zero drift anywhere, andsql/stayed up to date.test_migrations.pypasses, including the check thatmax_migration.txtpoints at a migration file that exists.flag_evaluationsandsharded_flag_evaluationsalready carry none of these six columns in prod-us or prod-eu.writable_flag_evaluations,kafka_flag_evaluations, andflag_evaluations_mvare also already column-less on prod. No golden-schema tracking exists for theingestion_mediumprod role, and the Metabase connection to that cluster is currently down.manage.py migrate_clickhousereplay from an empty database. CI's multinode migration smoke test does this on every push.Automatic notifications
Docs update
None. The only doc touched here is internal and already covered above.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
test_schema.pyinvariants above already assert against the column template this PR narrows.0297_flag_evaluations_events_mirror.pyand0301_flag_evaluations_default_columns.pyalready reshape this same table by editing the shared column template rather than anALTER TABLEmigration. Nothing from a private conversation or ticket.Reviewed with Claude Code's
/review-code(7 domain reviewers plus an adversarial validator pass), which found and confirmed a sharded-ALTER routing bug (fixed earlier in this branch) and the deletion-gate break noted above. Used/metabase-prod-queryto confirm the current prod column state cited above. Used/address-pr-reviewsto evaluate and act on Copilot, Greptile, and ReviewHog comments. The initial approach added a dedicatedALTER TABLE ... DROP COLUMNmigration; a review discussion favored editing the shared column template directly, since the table's own creation migration already renders from it, so that migration was removed in favor of the approach above. Session: https://claude.ai/code/session_013r76pXC5tBHPCo27KdhXJh