Skip to content

feat(clickhouse): add optional retention TTL on the snapshot table#20

Open
levivannoort wants to merge 3 commits into
utopia-php:devfrom
levivannoort:feat/retention-ttl
Open

feat(clickhouse): add optional retention TTL on the snapshot table#20
levivannoort wants to merge 3 commits into
utopia-php:devfrom
levivannoort:feat/retention-ttl

Conversation

@levivannoort

Copy link
Copy Markdown

What

Adds setRetention(?int $days) to the ClickHouse usage adapter. When set, setup() applies an idempotent TTL to the snapshot (ReplacingMergeTree) table so latest-state rows past the retention window are dropped by background merges:

$adapter->setRetention(30);
$usage->setup(); // ALTER TABLE {ns}_usage_snapshot ... MODIFY TTL toDateTime(time) + INTERVAL 30 DAY

Default null preserves current behaviour (no TTL).

Scope: snapshot only

Only the snapshot table gets a TTL. The aggregated {ns}_usage (SummingMergeTree) table is intentionally left untouched — it backs long-term usage/billing history, so dropping rows there would delete billing data.

Why in setup()

Retention is part of the table's schema, and setup() already owns everything else about it. Keeping TTL here means callers that already run setup() get retention with no extra plumbing, and no downstream code has to re-derive table names.

How

  • Applied as a separate ALTER … MODIFY TTL, not baked into CREATE TABLE, since CREATE TABLE IF NOT EXISTS won't touch an existing table. MODIFY TTL is a no-op when unchanged, so setup() stays idempotent/re-runnable.
  • SETTINGS materialize_ttl_after_modify = 0 defers the purge to normal background merges instead of an immediate mutation.
  • time is DateTime64(3); toDateTime(time) narrows it for the TTL expression.
  • setRetention() rejects non-positive day counts.

Pint (composer lint) and PHPStan level max (composer check) pass on the changed file.

🤖 Generated with Claude Code

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>
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds an optional retention TTL to the ClickHouse snapshot table (ReplacingMergeTree) via a new setRetention(?int $days) method that drives an idempotent ALTER TABLE … MODIFY/REMOVE TTL in setup(). The aggregated billing table is intentionally left untouched.

  • setRetention() validates that days is positive and returns $this for chaining; getRetention() exposes the current value.
  • setup() either applies MODIFY TTL … SETTINGS materialize_ttl_after_modify = 0 (deferred purge) or runs REMOVE TTL and swallows the error when the table has no TTL, keeping setup() fully re-runnable. Both previously flagged issues — null retention not removing an existing TTL, and the property being protected — are now resolved.

Confidence Score: 5/5

Safe to merge; the TTL logic is well-scoped and idempotent, and the two previously identified issues have been corrected.

The change is narrow (one file, two new public methods, one DDL block in setup()), the snapshot-only scope is clearly enforced, and setup() remains re-runnable in all three states: never had TTL, has TTL, had TTL removed. The only open point is that the catch block for REMOVE TTL matches an English error substring rather than a stable error code, but this does not affect current correctness.

No files require special attention.

Important Files Changed

Filename Overview
src/Usage/Adapter/ClickHouse.php Adds optional retention TTL to the snapshot table via setRetention()/getRetention() and an idempotent MODIFY/REMOVE TTL block in setup(). Previously flagged issues (null-retention not stripping TTL, protected vs private) are both addressed. One minor robustness concern around string-based error matching in the REMOVE TTL catch block.

Reviews (3): Last reviewed commit: "fix(clickhouse): tolerate REMOVE TTL on ..." | Re-trigger Greptile

Comment thread src/Usage/Adapter/ClickHouse.php
Comment thread src/Usage/Adapter/ClickHouse.php Outdated
levivannoort and others added 2 commits July 2, 2026 19:18
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant