Repository navigation
refactor(clickhouse): declare each schema object once - #96339
Merged
Merged
Conversation
|
😎 Merged successfully - details. |
Contributor
🤖 CI report
|
orian
changed the title
orian
force-pushed
the
pawel/refactor/hcl-apm-ingest-shared
branch
from
September 8, 2026 11:31
05b1425 to
87a91e1
Compare
rorylshanks
approved these changes
Sep 8, 2026
orian
marked this pull request as ready for review
September 8, 2026 12:15
The apm role restated ten objects the logs and ingestion roles already declared, because a name cannot be declared plainly twice. That copied 803 lines to express 257 lines of real difference, and the once-only guard could not see it: it counts override = true as zero duplicates, so nothing flagged the schema as authored twice. The ingest chain moves to two coshared layers instead. Each role composes what its nodes run: the prod clusters take the metrics and traces ingest into their logs stacks, the local stack takes the logs ingest into its events stack, and dev takes both into apm. dev keeps only what is genuinely its own -- the writable proxies its nodes carry, and a patch for the smaller consumers it runs. Six views stay restated. They differ by where they write: the prod logs nodes write straight into local storage, while dev's apm nodes write through proxies because the storage sits on another node. patch_materialized_view carries query and column operations but no to_table, so that one difference has no patch form. dev's logs stack stops composing the chain, which is the point: dev moved it to the apm nodes, so seven objects the logs golden claimed were there disappear from its drift. Net 821 fewer lines. Every apm golden still matches all forty apm nodes, prod drift is unchanged, and the duplicates baseline is untouched. migration.md gains the rule this violated: the same object on two roles belongs in a layer both compose, not restated with override in the second.
…ating them chschema#239 gave patch_materialized_view a to_table, so a view that differs only in where it writes no longer has to be copied. Four of the six become one-line patches, and roles/apm/dev drops from 889 lines to 646. Two stay restated. Their column lists differ from the logs role's as well as their destination, in an order a positioned patch does not reproduce -- checked against the node dumps rather than assumed. The file says so. Composition is unchanged: every apm golden still matches all forty apm nodes, and no golden or sql file moves. The pin moves to sha-9a52b78 for the to_table support.
chschema#239 added to_table to patch_materialized_view, so five of the six views collapse to patches. roles/apm/dev is 546 lines, down from 1187. One view stays restated. Its column set and types match the logs role's exactly; only the order differs, and no patch can reorder inherited columns. diff reports that as an opaque "columns changed" with no column named, which is filed as PostHog/chschema#240 along with the patch gap. Pin moves to sha-9a52b78. Every apm golden still matches all forty apm nodes.
The data layers declared 54 objects more than once, twelve of them in five places at once: a declaration in one layer and override = true copies in four others. That is 3,443 lines of copied blocks, and the once-only guard could not see any of it, because it counts override as zero duplicates. They came from converging the layers against the node dumps object by object. Where an object already existed, the copy was marked override so it would load, check.sh stayed green, and nothing reported that the schema was authored five times. 52 copies were pure redundancy and are gone. Six carried a real delta and become patch_table. 56 stay, either because their column set or ordering differs from the base or because a patch did not reproduce the composition. Each change was applied one at a time and kept only when the regenerated goldens stayed byte-identical, so composition is unchanged everywhere: every golden and sql file matches, and reconcile against the node dumps still reports 32, 29 and 12 operations.
Column order carries no meaning in these comparisons, and the layers cannot express it: a patch cannot reorder inherited columns. Without the flag an order-only difference reports "columns changed" and names no column, which reads as drift and costs a manual column comparison to disprove. plan has no such flag yet (chschema#241), so the cross-repo reconcile still reports those; 10 of its 26 unsafe rows are pure column order.
chschema 466b780 adds -ignore-column-order to plan and names the columns when order is the only difference. The cross-repo reconcile now passes the flag: 10 of its 26 unsafe rows were pure column order and are gone. kafka_trace_spans_avro_mv no longer needs restating in full. The dev apm shape is the shared one plus six kafka metadata columns, with Map(String, String) where the shared one has Map(LowCardinality(String), String), so it becomes a patch. The apm role now holds no override.
Now that both diff and plan ignore column order, an override whose only difference from its base is the order of its columns says nothing. Six of them go. Twenty-six more looked the same and stay. Five carry a real delta, dropping an inherited index or setting by omission. The other twenty-one are not overriding anything: their base sits in roles/data/local, which no cloud env composes. The flag is there to stop the duplicates guard counting a name that two layers declare but that no composition ever holds at once.
orian
force-pushed
the
pawel/refactor/hcl-apm-ingest-shared
branch
from
September 8, 2026 12:18
87a91e1 to
1991408
Compare
Contributor
Prompt To Fix All With AI### Issue 1
posthog/clickhouse/hcl/README.md:134-135
**Column-order policy remains incomplete**
The new requirement says every `hclexp diff` and `plan` must ignore column order, but `codegen/gen_migration.py` still runs `plan` without the flag and `compare-old.sh` still runs `diff` without it. With the order-only golden changes in this PR, migration generation can treat column movement as migration work, and the parity helper can report a false failure. Add `-ignore-column-order` to both remaining calls before documenting the policy as universal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: 87a91e1 | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+134
to
+135
| **Pass `-ignore-column-order` to every `hclexp diff` and `hclexp plan`.** | ||
| `check.sh`, `check-live.sh`, `diff.sh` and `check-cloud.sh` all do. |
Contributor
There was a problem hiding this comment.
The new requirement says every hclexp diff and plan must ignore column order, but codegen/gen_migration.py still runs plan without the flag and compare-old.sh still runs diff without it. With the order-only golden changes in this PR, migration generation can treat column movement as migration work, and the parity helper can report a false failure. Add -ignore-column-order to both remaining calls before documenting the policy as universal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/clickhouse/hcl/README.md
Line: 134-135
Comment:
**Column-order policy remains incomplete**
The new requirement says every `hclexp diff` and `plan` must ignore column order, but `codegen/gen_migration.py` still runs `plan` without the flag and `compare-old.sh` still runs `diff` without it. With the order-only golden changes in this PR, migration generation can treat column movement as migration work, and the parity helper can report a false failure. Add `-ignore-column-order` to both remaining calls before documenting the policy as universal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Picks up the TimeSeries default fix (chschema#247) and config-file comparison defaults (#244). Composition is unchanged: check.sh matches every committed golden, and the reconcile holds at 32/29/12 operations with 16 unsafe rows.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Someone changing a ClickHouse table can edit its declaration, leave four stale copies behind, and see every gate stay green.
override = true. An override restates an object in full to express a difference of a line or two.common, then a copy incloud,dev,prod-usandprod-eu.locate -duplicatescounts an override copy as zero duplicates, by design. The once-only guard stays green while the schema is authored many times over.apmrole restated ten objects thelogsand ingestion roles already declared: 803 copied lines carrying 257 lines of real difference.Changes
unsaferows to 16 across the three cloud envs. The ten that go were pure column order, on objects already running the declared schema.patch_table, and 56 stay because their columns genuinely differ.roles/coshared/apm_ingestandroles/coshared/logs_avro_ingest. Each role composes what its own nodes run, and theapmrole now holds no override at all.Map(String, String)where the shared declaration hasMap(LowCardinality(String), String). Amodify_columnrecords that, where the copy hid it.-ignore-column-order, which needs the hclexp bump inbin/image.txt. Column order carries no meaning here and a patch cannot express it, so an order difference read as drift and cost a manual column comparison to dismiss.README.mdandmigration.mdgain the two rules this violated: pass the flag, and declare an object once across roles instead of restating it with an override.roles/drop from 149 to 75, and the directory loses 5,231 lines.Note
Seven of the 70 composed outputs move. Four are on dev.
golden/dev/logs.hclsheds the ingest chain, which is the fix rather than a side effect.golden/dev/apm.hcl, the threedatagoldens and their sql reorder columns without changing any column set or type, confirmed withhclexp diff -ignore-column-order. Every other golden and sql file is byte-identical to master.How did you test this code?
check.shpasses: five envs validate, 34 compositions match their goldens, duplicates match the baseline of 2, sql is fresh.hogli ci:preflight --strictreports 0 failures.check-live.sh, which needs a live ClickHouse.check.shin CI.Automatic notifications
Docs update
README.mdgains the-ignore-column-orderrule.migration.mdgains the cross-role case of the defined-once rule, including why the duplicates guard cannot enforce it.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code. Skills invoked:
/writing-pr-descriptions,/merging-prs.Follows #96223, which added the
apmrole and took the shortcut this PR removes.Three chschema gaps surfaced during this work and were fixed upstream: PostHog/chschema#239 (
to_tableon a view patch), #240 (an order-only difference reported without naming a column) and #241 (planmissing-ignore-column-order). The pin moves to the build carrying the last two. A fourth is open: PostHog/chschema#245, because the duplicates guard reads across all layers rather than per composition, which forcesoverride = trueonto 21 declarations that override nothing.Two earlier passes were wrong and are worth recording. Replacing all ten apm objects with patches looked correct while comparing query text alone, but six of them differ in their destination table. Deleting the data override copies that were byte-identical to their base also looked correct, and is not: such a copy restores the base after an intermediate layer changed it, so the composition moved. Both were caught by comparing the composed output rather than the source.
Public artifact: everything here derives from this repository and the node dumps. No hostnames, credentials, or customer identifiers are in the diff.