perf(clickhouse): stop storing native event attributes - #17
Conversation
Source PR: triggerdotdev#4866 Source head: c0fbe91
⛔ Shipwright · BlockedRecommendation: do not merge PR #17 · Tier
Findings (4)
Fireworks usage: 6,424 input · 486 output · 6,910 total tokens · $0.0017 · 13s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| -- stored. Writers must include attributes in an explicit insert column list | ||
| -- because implicit INSERT column lists exclude EPHEMERAL columns. | ||
| ALTER TABLE trigger_dev.task_events_v2 | ||
| MODIFY COLUMN attributes JSON EPHEMERAL; |
There was a problem hiding this comment.
Shipwright · CRITICAL
The migration makes 'attributes' EPHEMERAL, which means it is not stored.
Impact: The migration makes 'attributes' EPHEMERAL, which means it is not stored. Any existing writer that uses implicit INSERT column lists (i.e., INSERT without an explicit column list) will now silently fail or misbehave because EPHEMERAL columns are excluded from implicit column lists. The comment acknowledges this, but the migration does not verify or coordinate that all writers have been updated before applying the sc…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| -- Restoring the stored JSON column safely requires inspecting the live schema | ||
| -- and coordinating the writer rollback. Use a new forward migration instead. | ||
| SELECT throwIf(1, 'This migration cannot be rolled back automatically'); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The Down migration intentionally throws, making rollback impossible.
Impact: The Down migration intentionally throws, making rollback impossible. If the Up migration is applied and then a writer incompatibility or data-loss issue is discovered, there is no automated path to restore the previous schema. This is a release-stopping operational risk for a storage-affecting change.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| name: z.string(), | ||
| default_kind: z.string(), | ||
| default_expression: z.string(), | ||
| }), |
There was a problem hiding this comment.
Shipwright · HIGH
The test asserts 'default_expression' equals 'defaultValueOfTypeName('JSON')' for the EPHEMERAL column.
Impact: The test asserts 'default_expression' equals 'defaultValueOfTypeName('JSON')' for the EPHEMERAL column. This couples the test to ClickHouse's internal default-expression representation, which is brittle across ClickHouse versions and makes the test fail for reasons unrelated to the intended behavior.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| -- Full-text search is served outside the source event table. Keeping these | ||
| -- indexes here adds work to every event insert and merge without serving reads. | ||
| ALTER TABLE trigger_dev.task_events_v2 | ||
| DROP INDEX IF EXISTS idx_attributes_text_search; |
There was a problem hiding this comment.
Shipwright · HIGH
Dropping 'idx_attributes_text_search' and 'idx_message_text_search' removes full-text search indexes.
Impact: Dropping 'idx_attributes_text_search' and 'idx_message_text_search' removes full-text search indexes. If any read path still queries these columns with text-search operators, performance will degrade severely or queries may fail. The migration does not verify that no remaining readers depend on these indexes.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Summary
Stop storing native JSON event attributes while retaining the existing materialized
attributes_textrepresentation. Also remove the two unused full-text indexes from the source event table.Design
This depends on #4860. Its explicit-column writer must be deployed before this migration is applied so attributes continue to feed the materialized string.
The JSON input becomes EPHEMERAL, eliminating its stored subcolumns and merge overhead for new parts. Existing parts retain their JSON files until normal merges or TTL removal.
The migration is intentionally not reversible automatically. Restoring native JSON storage safely requires inspecting the current schema and coordinating the writer change, so it should be done with a new forward migration rather than a retry-sensitive Down migration.
Source merge-base:
04d3264b383a0b5977054755db83d156aa6b7165Source head:
c0fbe91e46adb7140821f1d65c9459a54cf0912c