fix: drop v2 key verification aggregation chain from default db#5231
fix: drop v2 key verification aggregation chain from default db#5231
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes ClickHouse v2 aggregation tables and materialized views for key verifications analytics, including per-minute, per-hour, per-day, and per-month aggregation tables, along with their supporting materialized views and migration steps. Legacy schema definitions are also deleted. Materialized views are subsequently recreated pointing to v3 tables instead. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
8b299ee to
67e4ad3
Compare
67e4ad3 to
97f9b1b
Compare
97f9b1b to
2846c4a
Compare
2846c4a to
03fc171
Compare
03fc171 to
79f77fd
Compare
79f77fd to
e1740cc
Compare
e1740cc to
b920cc7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/clickhouse/migrations/20260306000001.sql (1)
1-3: Confirm public alias behavior stays stable during this cleanup.Since this removes internal v2 objects, just ensure public-facing alias mappings remain unchanged unless query results changed for users.
Based on learnings: public-facing table aliases should only be bumped when changes affect user-visible behavior or query results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/clickhouse/migrations/20260306000001.sql` around lines 1 - 3, Verify that removing the v2 aggregation chain does not change any public aliases: audit all public-facing alias mappings and view/table names that reference the "v2" chain (leaf MVs, upstream MVs, tables) and ensure they either (a) continue to point to the same query results or (b) are explicitly remapped to the v3 equivalents only if query results are identical; update the migration to keep existing public aliases unchanged unless you intentionally replace them, add atomic swaps or create temporary aliases to preserve queries during the drop, and add/execute automated integration tests and a diff-of-query-results check comparing v2 vs v3 for each public alias to confirm no user-visible change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/clickhouse/migrations/20260306000001.sql`:
- Around line 1-3: Verify that removing the v2 aggregation chain does not change
any public aliases: audit all public-facing alias mappings and view/table names
that reference the "v2" chain (leaf MVs, upstream MVs, tables) and ensure they
either (a) continue to point to the same query results or (b) are explicitly
remapped to the v3 equivalents only if query results are identical; update the
migration to keep existing public aliases unchanged unless you intentionally
replace them, add atomic swaps or create temporary aliases to preserve queries
during the drop, and add/execute automated integration tests and a
diff-of-query-results check comparing v2 vs v3 for each public alias to confirm
no user-visible change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfb19dda-183f-48d6-b42e-0ac1a77155f3
📒 Files selected for processing (5)
pkg/clickhouse/migrations/20260306000001.sqlpkg/clickhouse/schema/002_key_verifications_per_minute_v2.sqlpkg/clickhouse/schema/003_key_verifications_per_hour_v2.sqlpkg/clickhouse/schema/004_key_verifications_per_day_v2.sqlpkg/clickhouse/schema/005_key_verifications_per_month_v2.sql
💤 Files with no reviewable changes (4)
- pkg/clickhouse/schema/004_key_verifications_per_day_v2.sql
- pkg/clickhouse/schema/002_key_verifications_per_minute_v2.sql
- pkg/clickhouse/schema/003_key_verifications_per_hour_v2.sql
- pkg/clickhouse/schema/005_key_verifications_per_month_v2.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/clickhouse/migrations/20260306000000.sql`:
- Around line 79-84: Before executing each DROP DATABASE IF EXISTS (for
databases `verifications`, `ratelimits`, `metrics`, `billing`, and `business`)
add a safety guard that checks the database is empty and aborts the migration if
any tables remain; specifically, query the system.tables (or information_schema)
for each database name and raise an error or fail the migration if the query
returns any rows, only proceeding to the DROP DATABASE when the check confirms
zero tables. Ensure this emptiness check runs for each of the five databases and
provides a clear error message identifying the database with leftover objects so
the migration fails fast rather than silently deleting unexpected data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b1747880-8623-4a49-acb2-351cd05689e6
📒 Files selected for processing (6)
pkg/clickhouse/migrations/20250903085516_init.sqlpkg/clickhouse/migrations/20260306000000.sqlpkg/clickhouse/schema/000_legacy.sqlpkg/clickhouse/schema/001_key_verifications_raw_v2.sqlpkg/clickhouse/schema/006_ratelimits_raw_v2.sqlpkg/clickhouse/schema/012_api_requests_raw_v2.sql
💤 Files with no reviewable changes (5)
- pkg/clickhouse/schema/006_ratelimits_raw_v2.sql
- pkg/clickhouse/schema/001_key_verifications_raw_v2.sql
- pkg/clickhouse/schema/012_api_requests_raw_v2.sql
- pkg/clickhouse/schema/000_legacy.sql
- pkg/clickhouse/migrations/20250903085516_init.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/clickhouse/migrations/20250911070454.sql`:
- Line 209: The change to the materialized view in migration 20250911070454.sql
will not affect clusters that already ran that migration; create a new forward
migration that drops the existing materialized view `ratelimits_last_used_mv_v2`
and recreates it with the updated definition targeting `ratelimits_last_used_v2`
(SELECT workspace_id, namespace_id, identifier, maxSimpleState(time) AS time
FROM default.ratelimits_raw_v2 GROUP BY ...), and include a backfill step or
one-time populate of `ratelimits_last_used_v2` as needed so already-migrated
environments get the new MV behavior without editing the old migration file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12ccce9c-ba7a-43d0-9483-81cdbf803d50
📒 Files selected for processing (4)
pkg/clickhouse/migrations/20250911070454.sqlpkg/clickhouse/migrations/20250925091254.sqlpkg/clickhouse/migrations/20251010160229.sqlpkg/clickhouse/migrations/20251125075943.sql
💤 Files with no reviewable changes (3)
- pkg/clickhouse/migrations/20250925091254.sql
- pkg/clickhouse/migrations/20251010160229.sql
- pkg/clickhouse/migrations/20251125075943.sql

What does this PR do?
Removes the v2 key verification aggregation chain from ClickHouse, which has been fully replaced by the v3 chain that provides better partitioning and longer TTL. This cleanup removes the deprecated materialized views and tables for minute, hour, day, and month aggregations.
The migration drops the v2 aggregation chain in the correct order (leaf materialized views first, then upstream views, then tables) to avoid dependency conflicts.
Fixes # (issue)
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated