fix(clickhouse): drop obsolete task event index variants - #4880
Conversation
|
WalkthroughThe ClickHouse migration now drops four text-search indexes in one Merge Risk: ⚪ Minimal · up to This localized migration now removes the obsolete indexes without changing the attributes column or introducing a new production behavior risk. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
carderne
changed the title
carderne
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql (1)
11-12: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine the rollback behavior for migration 042. The empty
Downsection can leavetrigger_dev.task_events_v2without the search indexes added by migrations 015 and 016.idx_attributes_textwas already removed by migration 018, andmessage_text_searchhas no definition in the schema files, so do not restore all four blindly. If this migration is intentionally irreversible, make theDownsection fail explicitly; otherwise restore only indexes present before migration 042.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 706868ba-7698-498b-9043-b0c91d354c38
📒 Files selected for processing (1)
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (32)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: typecheck / typecheck
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql
Migration file numbering: name files as `0(N+1)_descriptive_name.sql` where N is the largest existing migration number in `schema/`; rebase and renumber if main adds migrations before opening a PR DDL in migrations must be idempotent: use `...
📄 CodeRabbit inference engine (internal-packages/clickhouse/CLAUDE.md)
Files:
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql
🔇 Additional comments (1)
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql (1)
9-9: Add the required crumb marker.The previous review comment still applies. Add a SQL-safe
//@crumbs`` marker near this changed DDL block. Remove it withagentcrumbs stripbefore merge.Proposed change
ALTER TABLE trigger_dev.task_events_v2 +-- // `@crumbs` DROP INDEX IF EXISTS idx_attributes_text_search,As per coding guidelines, files matching
**/*must add crumbs as code is written, using//@Crumbsor `// `#region` `@crumbs;agentcrumbs stripremoves them before merge.Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/clickhouse/src/taskEvents.test.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd crumbs to the changed test paths.
Add a
//@Crumbsmarker or a `// `#region` `@crumbsblock around the changed test paths before merge.agentcrumbs stripwill remove this instrumentation.As per coding guidelines, add crumbs as you write code and mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs.Also applies to: 45-45, 85-86, 101-106
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: c3ed8d80-7ddd-4f56-9913-28471456108c
📒 Files selected for processing (1)
internal-packages/clickhouse/src/taskEvents.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (32)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: typecheck / typecheck
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
**Prefer static imports over dynamic imports.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs Do not use high-cardinality attributes in OTEL metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
2 participants
Summary
Removes the obsolete task event full-text indexes in one
ALTER TABLE, accounting for both known index naming variants.Fix
The original migration issued back-to-back alters while ClickHouse could still be processing the preceding index mutation. Combining the index removals avoids scheduling a subsequent alter against an unfinished mutation.
The native JSON storage change is deferred until it can be performed without rewriting populated parts.