[clickhouse] Setup creation of dependencies table in ClickHouse storage#8329
[clickhouse] Setup creation of dependencies table in ClickHouse storage#8329mahadzaryab1 wants to merge 7 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds initial ClickHouse schema support for dependency storage (towards implementing the v2 depstore.Reader on ClickHouse) by introducing a new dependencies table and wiring it into schema creation and purge flows.
Changes:
- Add
create_dependencies_table.sqland embed it assql.CreateDependenciesTable. - Create the dependencies table during ClickHouse factory schema initialization.
- Include dependencies table truncation in
Factory.Purge()and extend unit tests for the new create/truncate paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/storage/v2/clickhouse/sql/queries.go | Adds embedded schema query and a truncate query constant for the new dependencies table. |
| internal/storage/v2/clickhouse/sql/create_dependencies_table.sql | Introduces the ClickHouse dependencies table DDL. |
| internal/storage/v2/clickhouse/factory.go | Ensures the dependencies table is created when CreateSchema is enabled and purged via truncate. |
| internal/storage/v2/clickhouse/factory_test.go | Adds negative-path unit tests for dependencies table creation and truncation failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CREATE TABLE IF NOT EXISTS | ||
| dependencies ( | ||
| timestamp DateTime64 (9), | ||
| dependencies String |
There was a problem hiding this comment.
The new table schema uses a column named dependencies inside a table also named dependencies. This makes queries/readability harder (e.g., SELECT dependencies FROM dependencies) and can be confusing when adding readers/writers later. Consider renaming the column to something more specific (e.g., dependencies_json, links, or payload) to avoid the table/column name collision.
| dependencies String | |
| dependencies_json String |
internal/storage/v2/clickhouse/sql/create_dependencies_table.sql
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8329 +/- ##
===========================================
+ Coverage 37.88% 95.59% +57.71%
===========================================
Files 169 314 +145
Lines 10121 16457 +6336
===========================================
+ Hits 3834 15732 +11898
+ Misses 5893 571 -5322
+ Partials 394 154 -240
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const TruncateSpans = `TRUNCATE TABLE IF EXISTS spans` | ||
|
|
||
| const TruncateServices = `TRUNCATE TABLE services` | ||
| const TruncateServices = `TRUNCATE TABLE IF EXISTS services` | ||
|
|
||
| const TruncateOperations = `TRUNCATE TABLE operations` | ||
| const TruncateOperations = `TRUNCATE TABLE IF EXISTS operations` |
There was a problem hiding this comment.
Switching the TRUNCATE statements to TRUNCATE TABLE IF EXISTS ... changes Purge behavior: it will now succeed even when tables are missing (e.g., wrong DB/schema), which can mask misconfiguration and make tests/ops think data was purged when it wasn’t. Consider keeping TRUNCATE strict (no IF EXISTS) or explicitly validating required tables exist and returning a clear error if they don’t.
Which problem is this PR solving?
Description of the changes
dependenciestable for ClickHouse as part of an effort to implement dependency storage in ClickHouse.How was this change tested?
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.