Merge dev into main#23
Conversation
Add setRetention(?int $days) to the ClickHouse adapter. When set, setup() applies an idempotent `ALTER TABLE ... MODIFY TTL toDateTime(time) + INTERVAL <days> DAY` to the snapshot (ReplacingMergeTree) table so latest-state rows older than the window are dropped by background merges. - Only the snapshot table gets a TTL; the aggregated (SummingMergeTree) table is left untouched since it backs long-term usage/billing history. - Applied as a separate ALTER (not in CREATE TABLE) so it also covers already-existing tables; MODIFY TTL is a no-op when unchanged, keeping setup() re-runnable. - materialize_ttl_after_modify = 0 defers the purge to background merges instead of an immediate mutation. - Default null preserves existing behaviour (no TTL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n private Addresses review feedback on the retention TTL: - setup() now issues ALTER TABLE ... REMOVE TTL on the null path so disabling retention actually strips a previously applied TTL, matching the documented "pass null to disable" contract. REMOVE TTL is a no-op on a table without a TTL, so setup() stays idempotent. - $retention is now private, matching the other sensitive fields and preventing subclasses from bypassing the positive-day guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
REMOVE TTL is not a no-op — ClickHouse raises error 36 (BAD_ARGUMENTS, "Table doesn't have any table TTL expression, cannot remove") when the snapshot table has no TTL. That broke setup() on every fresh table when retention is disabled. Swallow that specific error to keep setup() idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
feat(clickhouse): add optional retention TTL on the snapshot table
Greptile SummaryThis PR brings
Confidence Score: 4/5Safe to merge; the retention feature is well-scoped and the only concern is defensive robustness of the error-swallowing path. The change is small and well-contained — The Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/Usage/Adapter/ClickHouse.php:1126-1133
**Fragile error-message matching to detect "no TTL" case**
The exception is suppressed by checking that the message contains `"doesn't have any table TTL expression"`. This substring is not part of a stable ClickHouse API contract — the wording can vary across ClickHouse versions or distributions. If ClickHouse ever rewords that message, the condition will never match, every `REMOVE TTL` on a table without a TTL will surface as an uncaught exception, and `setup()` will stop being idempotent when disabling retention. Consider also guarding on the ClickHouse error code (36 / `NO_SUCH_DATA_PART`) via a dedicated exception type or numeric code check if the client library exposes one, and logging a warning rather than silently swallowing the mismatch.
Reviews (1): Last reviewed commit: "Merge pull request #20 from levivannoort..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| $this->query( | ||
| "ALTER TABLE {$escapedSnapshotDatabaseAndTable} REMOVE TTL" | ||
| ); | ||
| } catch (Exception $e) { | ||
| if (!str_contains($e->getMessage(), "doesn't have any table TTL expression")) { | ||
| throw $e; | ||
| } |
There was a problem hiding this comment.
The exception is suppressed by checking that the message contains "doesn't have any table TTL expression". This substring is not part of a stable ClickHouse API contract — the wording can vary across ClickHouse versions or distributions. If ClickHouse ever rewords that message, the condition will never match, every REMOVE TTL on a table without a TTL will surface as an uncaught exception, and setup() will stop being idempotent when disabling retention. Consider also guarding on the ClickHouse error code (36 / NO_SUCH_DATA_PART) via a dedicated exception type or numeric code check if the client library exposes one, and logging a warning rather than silently swallowing the mismatch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Usage/Adapter/ClickHouse.php
Line: 1126-1133
Comment:
**Fragile error-message matching to detect "no TTL" case**
The exception is suppressed by checking that the message contains `"doesn't have any table TTL expression"`. This substring is not part of a stable ClickHouse API contract — the wording can vary across ClickHouse versions or distributions. If ClickHouse ever rewords that message, the condition will never match, every `REMOVE TTL` on a table without a TTL will surface as an uncaught exception, and `setup()` will stop being idempotent when disabling retention. Consider also guarding on the ClickHouse error code (36 / `NO_SUCH_DATA_PART`) via a dedicated exception type or numeric code check if the client library exposes one, and logging a warning rather than silently swallowing the mismatch.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Changes that landed on
dev(accidental merge target) to bringmainup to date.Commits
🤖 Generated with Claude Code