add support for clickhouse for enterprise logstore tables - #5182
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ClickHouse extension-table creation and column reconciliation to direct and hybrid log stores, with validation for identifiers and ordering options. An integration test covers table options, hybrid delegation, row queries, and generated ClickHouse DDL. ChangesClickHouse extension-table management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant HybridLogStore
participant ClickHouseLogStore
participant ClickHouse
Caller->>HybridLogStore: EnsureClickHouseTable(...)
HybridLogStore->>ClickHouseLogStore: Delegate extension-table request
ClickHouseLogStore->>ClickHouse: Create table with partition, order, TTL, and indexes
ClickHouseLogStore->>ClickHouse: Reconcile model columns
ClickHouse-->>Caller: Return success or error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
90d34f7 to
1fe15e8
Compare
Confidence Score: 4/5This is close, but the DDL fragment boundary should be fixed before merging.
framework/logstore/clickhouseextension.go
|
| Filename | Overview |
|---|---|
| framework/logstore/clickhouseextension.go | Adds the extension-table schema API, hybrid delegation, and validation for table names. |
| framework/logstore/clickhousestore_test.go | Adds ClickHouse tests for extension table creation, hybrid delegation, reserved names, and valid ClickHouse syntax. |
Reviews (3): Last reviewed commit: "add support for clickhouse for enterpris..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/logstore/clickhousestore_test.go (1)
94-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing negative test for the "unsupported inner store" branch.
The test only exercises the success path of
HybridLogStore.EnsureClickHouseTabledelegation. The!okbranch (inner store doesn't implementclickHouseSchemaStore) inframework/logstore/clickhouseextension.golines 49-55 has no coverage here. Consider wrapping a store that doesn't implementclickHouseSchemaStore(if one is available in this package, e.g. a bareRDBLogStore) and asserting the "does not support ClickHouse extension tables" error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/clickhousestore_test.go` around lines 94 - 95, Extend the tests for HybridLogStore.EnsureClickHouseTable with a negative case using an inner store that does not implement clickHouseSchemaStore, such as a bare RDBLogStore if available. Assert that the call returns the expected “does not support ClickHouse extension tables” error, while preserving the existing successful delegation test.framework/logstore/clickhouseextension.go (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an options struct instead of 4 positional strings.
table,partitionBy,orderBy,ttlare allstringand adjacent in the signature — easy for a caller to accidentally transpose two arguments with no compiler help. The package already haschTableOpts(inclickhousemigrate.go) with the same shape; exposing an equivalent public struct forEnsureClickHouseTable's parameters would remove this footgun for future callers.♻️ Sketch of an options-struct signature
-type clickHouseSchemaStore interface { - EnsureClickHouseTable(ctx context.Context, model any, table, partitionBy, orderBy, ttl string, skipIndexes []string) error -} +type ClickHouseExtensionTableOptions struct { + Table string + PartitionBy string + OrderBy string + TTL string + SkipIndexes []string +} + +type clickHouseSchemaStore interface { + EnsureClickHouseTable(ctx context.Context, model any, opts ClickHouseExtensionTableOptions) error +}Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/clickhouseextension.go` around lines 12 - 14, Replace the adjacent string parameters in clickHouseSchemaStore.EnsureClickHouseTable with a public options struct equivalent to chTableOpts, while retaining ctx, model, and skipIndexes as appropriate. Update the interface, implementation, and all call sites to pass the named options fields, preserving the existing table, partitionBy, orderBy, and ttl values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/logstore/clickhouseextension.go`:
- Around line 12-14: Replace the adjacent string parameters in
clickHouseSchemaStore.EnsureClickHouseTable with a public options struct
equivalent to chTableOpts, while retaining ctx, model, and skipIndexes as
appropriate. Update the interface, implementation, and all call sites to pass
the named options fields, preserving the existing table, partitionBy, orderBy,
and ttl values.
In `@framework/logstore/clickhousestore_test.go`:
- Around line 94-95: Extend the tests for HybridLogStore.EnsureClickHouseTable
with a negative case using an inner store that does not implement
clickHouseSchemaStore, such as a bare RDBLogStore if available. Assert that the
call returns the expected “does not support ClickHouse extension tables” error,
while preserving the existing successful delegation test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 776107ee-97d1-4eb6-a4fc-7075f6a779be
📒 Files selected for processing (2)
framework/logstore/clickhouseextension.goframework/logstore/clickhousestore_test.go
1fe15e8 to
2c581a3
Compare
2c581a3 to
2bbe767
Compare
Merge activity
|
3 participants
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines