feat(clickhouse): switch schema setup and retire legacy execution - #111081
rorylshanks wants to merge 2 commits into
Conversation
rorylshanks
added
the
test-new-events-schema
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong.
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/source.py:20 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_step_functions/source.py:17 |
16 | 141 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_sagemaker/source.py:113 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_step_functions/source.py:107 |
29 | 107 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.py:17 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_step_functions/aws_step_functions.py:16 |
11 | 99 |
posthog/hogql/property.py:977 |
products/endpoints/backend/models.py:36 |
12 | 73 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
🚨 Comment density — 9% of added code lines are comments (22 of 233)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
posthog/clickhouse/managed_schema.py |
13 | 151 |
posthog/conftest.py |
4 | 23 |
posthog/management/commands/migrate_clickhouse.py |
4 | 30 |
posthog/test/base.py |
1 | 8 |
This check does not block merging. It updates on every push and clears when the share drops.
✅ Bundle size — no change
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 69.79 MiB · no change
No file changed by more than 1000 B.
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.63 MiB · 22 files | no change | █████████░ 88.7% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.72 MiB · 661 files | no change | █████████░ 92.3% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.59 MiB · 2,409 files | no change | █████████░ 91.0% of 8.34 MiB |
dashboard scenesrc/scenes/dashboard/Dashboard.tsx |
9.67 MiB · 3,392 files | no change | ███████░░░ 71.8% of 13.48 MiB |
today home pathsrc/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx |
7.61 MiB · 2,417 files | no change | █████████░ 88.6% of 8.58 MiB |
events scenesrc/scenes/activity/explore/EventsScene.tsx |
9.29 MiB · 3,244 files | no change | ███████░░░ 73.5% of 12.64 MiB |
replay detail scenesrc/scenes/session-recordings/detail/SessionRecordingDetail.tsx |
12.12 MiB · 4,130 files | no change | ████████░░ 77.1% of 15.72 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
🟢 src/scenes/project-homepage/today/TodayReportPage.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 92.7 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 29.0 KiB | ../node_modules/.pnpm/zod@4.3.6/node_modules/zod/v4/core/schemas.js |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/dashboard/Dashboard.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/activity/explore/EventsScene.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
Largest files eagerly shipped from src/scenes/session-recordings/detail/SessionRecordingDetail.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 279.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 19 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 835.5 KiB | dist/toolbar/toolbar-app-FSDWO46I.css |
| 657.5 KiB | dist/toolbar/chunk-chunk-BALLB66W.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-7JWMBALG.js |
| 138.2 KiB | dist/toolbar/chunk-chunk-RXJG6CZC.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-YIYWW6XJ.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-MM7MZI2L.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-EZFR5QGQ.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 960.35 MiB · no change
|
[Critical risk] Switches ClickHouse schema setup from Python to OpenTofu tooling. The PR is not safe to merge until test resets preserve declared reference data and the standalone reset command works. Reviews (1) · Last reviewed commit: "feat(clickhouse): switch schema setup an..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| FROM system.tables | ||
| WHERE database = '{settings.CLICKHOUSE_DATABASE}' AND name LIKE 'kafka_%' | ||
| WHERE database = %(database)s | ||
| AND ((engine LIKE '%%MergeTree' AND total_rows > 0) OR (engine = 'Kafka' AND %(drop_kafka)s)) |
There was a problem hiding this comment.
--reuse-db, package teardown truncates the nonempty channel_definition table, but seed() reloads only exchange_rate. The next package starts with an empty source for the channel-definition dictionary, changing channel classification results. Exclude schema-declared reference tables from this reset or restore their rows afterward.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/conftest.py
Line: 65
Comment:
**Channel definitions are erased** With `--reuse-db`, package teardown truncates the nonempty `channel_definition` table, but `seed()` reloads only `exchange_rate`. The next package starts with an empty source for the channel-definition dictionary, changing channel classification results. Exclude schema-declared reference tables from this reset or restore their rows afterward.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| ), | ||
| ] | ||
| ) | ||
| ClickHouseDatabase().restore() |
There was a problem hiding this comment.
reset_test_clickhouse_db calls restore() before this process has captured a schema snapshot. restore() raises, so the command cannot reset the test database. Build or capture the schema first, or give the command a reset path that does not require an in-process snapshot.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/base.py
Line: 1912
Comment:
**Standalone reset cannot start** On a fresh invocation, `reset_test_clickhouse_db` calls `restore()` before this process has captured a schema snapshot. `restore()` raises, so the command cannot reset the test database. Build or capture the schema first, or give the command a reset path that does not require an in-process snapshot.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| database.create() | ||
| database.apply_schema(kafka=True) | ||
| database.seed() | ||
| self.stdout.write(self.style.SUCCESS("ClickHouse schema is up to date")) |
There was a problem hiding this comment.
handle() path never calls migrate(), but that method remains and expects upto, fake, and print_sql options that the parser no longer supplies. Remove the unreachable method as part of retiring legacy execution; leaving it in place obscures the supported path and gives direct callers a broken method.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/management/commands/migrate_clickhouse.py
Line: 60-63
Comment:
**Broken legacy method remains** This new `handle()` path never calls `migrate()`, but that method remains and expects `upto`, `fake`, and `print_sql` options that the parser no longer supplies. Remove the unreachable method as part of retiring legacy execution; leaving it in place obscures the supported path and gives direct callers a broken method.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| The OpenTofu catalogue in `posthog/clickhouse/schema/` is being introduced alongside Python migrations. During the foundation stage, normal migration, local setup and test commands still use Python. Apply the new catalogue only to an isolated database or an explicitly adopted infrastructure canary. Ownership cutover and removal of the legacy tooling happen in separate pull requests. | ||
|
|
||
| The ownership cutover switches local, test and Hobby setup to OpenTofu. Cloud deployment hooks stop applying ClickHouse schema; infrastructure owns it. Merge the cutover only after the canary migration succeeds and every cloud object has a verified owner. Legacy definitions remain dormant until cleanup. |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/published/handbook/engineering/databases/schema-changes.md
Line: 54
Comment:
**Setup instructions contradict each other** The preceding paragraph says normal local and test setup still uses Python migrations and limits OpenTofu to isolated databases; this paragraph says those paths now use OpenTofu. Replace the outdated foundation-stage instructions so operators have one accurate setup procedure.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughClickHouse schema application and planning now use a managed database wrapper. Runtime migration and test setup paths use the wrapper for database creation, schema operations, restoration, and seeding. The container installs the schema tools. Rollout documentation and ClickHouse configuration are updated. CI path filters include schema tooling, while automated migration checks and selected workflow triggers are removed. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
posthog/clickhouse/managed_schema.py-82-87 (1)
82-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a timeout to the schema tool subprocess.
subprocess.runhas notimeout. Ifbin/clickhouse-schemahangs, for example on a provider download, a Keeper wait, or a stuck ClickHouse connection, thenmigrate_clickhouse, test setup, and deploy hooks block with no limit. Set a bounded timeout. Convertsubprocess.TimeoutExpiredinto the sameRuntimeErrorpath.Proposed fix
return subprocess.run( [str(REPO_ROOT / "bin" / "clickhouse-schema"), *args, "-no-color"], env=env, capture_output=True, text=True, + timeout=1800, )Based on learnings: "flag missing timeout arguments... Recommend a sensible default (e.g., 300s) and handle subprocess.TimeoutExpired."
Source: Learnings
posthog/clickhouse/test/test_managed_schema.py-11-15 (1)
11-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the dropped table comes back.
The test drops
trace_spans, but it checks onlyexchange_rate. If the rebuild does not recreatetrace_spans, the test still passes. Assert thattrace_spansexists.Proposed fix
assert sync_execute("SELECT count() FROM exchange_rate")[0][0] > 0 + assert sync_execute( + "SELECT count() FROM system.tables WHERE database = currentDatabase() AND name = 'trace_spans'" + )[0][0] == 1posthog/clickhouse/managed_schema.py-117-119 (1)
117-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
declares_columnreturns false for an empty snapshot.
_snapshotis populated only bycreate_test_tables. Ifcleanup_materialized_columnsruns in a process that has not calledsnapshot(),declares_columnreturnsFalsefor every column.posthog/test/base.pythen drops columns that the schema declares. Raise an error when_snapshotis empty instead of returningFalse.
🧹 Nitpick comments (1)
posthog/management/commands/migrate_clickhouse.py (1)
7-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead legacy
migratepath.
handleno longer callsmigrate,get_migrations, or the_create_*helpers. These methods still readoptions["upto"],options["fake"], andoptions["print_sql"], and this change removes those arguments. Any future call to them raisesKeyError. They also keep theinfiimports alive. Delete them in this PR, or state that the cleanup PR deletes them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: d028a590-6983-4698-bfaf-b072ba7adcfb
📒 Files selected for processing (25)
.depot/workflows/ci-backend.yml.github/scripts/test_ci_migration_selection.py.github/workflows/ci-backend.yml.github/workflows/ci-clickhouse-hcl-schema.yml.github/workflows/ci-clickhouse-multinode-migrations.yml.github/workflows/ci-dagster.yml.github/workflows/ci-mcp.yml.github/workflows/ci-nodejs.yml.github/workflows/ci-rust.ymlDockerfilebin/migratebin/migrate-checkdocker-compose.dev.ymldocker/clickhouse/config.d/default.xmldocker/kafka/topics.txtdocs/published/handbook/engineering/databases/schema-changes.mdposthog/clickhouse/managed_schema.pyposthog/clickhouse/schema/README.mdposthog/clickhouse/test/test_managed_schema.pyposthog/conftest.pyposthog/hogql/cost/test/test_statistics.pyposthog/management/commands/migrate_clickhouse.pyposthog/management/commands/setup_test_environment.pyposthog/test/base.pyproducts/batch_exports/backend/tests/temporal/conftest.py
💤 Files with no reviewable changes (5)
- .github/workflows/ci-clickhouse-multinode-migrations.yml
- bin/migrate-check
- .github/workflows/ci-clickhouse-hcl-schema.yml
- .github/scripts/test_ci_migration_selection.py
- bin/migrate
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
46ede39 to
eb14e80
Compare
This branch has not been deployed
1 participant
Problem
Operators and test runners use the declarative schema after cloud ownership has been verified.
Changes
Routes local migration entrypoints and test setup through OpenTofu; disables automatic legacy HCL and multinode workflows. Dormant migrations and helpers remain for separate cleanup.
Rollout gate: Merge after the canary succeeds and infra ownership https://github.com/PostHog/posthog-cloud-infra/pull/10907 has adopted every root and node without DDL. Observe this layer before cleanup. Merge bottom-first; do not land the whole stack together.
Before:
After:
How did you test this code?
Local catalogue validation and table-family checks passed, including the empty second plan.
posthog/clickhouse/test/test_managed_schema.py --reuse-dbpassed against local services. Python lint/format, lockfile and workflow checks passed. GitHub script tests passed with GNU sed, matching Linux CI; the pinned schema formatter passed. Live cloud rollout, the complete backend suite and both events-schema variants remain CI/review gates.Test rationale: The imported managed-schema regression checks reconstruction after fixture restore. Existing table-family checks validate the catalogue and require an empty second plan; no split-only tests were added.
Release status
Automatic notifications
Docs update
Updated the existing schema README and applicable schema-change guidance to describe this rollout stage.
🤖 Agent context
Autonomy: Human-driven (agent-assisted). Agent: Codex, GPT-6.
Staged replacement for #109937, which remains available for comparison. Source changes were split using Git, GitHub CLI and
gh stack; current master changes were retained. Skills: /stacking-prs, /writing-pr-descriptions, /running-ci-preflight, /authoring-ci-workflows, /clickhouse-migrations, /writing-tests, /writing-code-comments and /reviewing-with-coderabbit. Local CodeRabbit review was skipped at the directing user's request because the CLI was missing. No customer material was used. Human review and CI patch coverage are pending on this draft.