Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/Audit/Adapter/ClickHouse.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ class ClickHouse extends SQL

protected bool $asyncCleanup = false;

/** @var int|null Retention in days; when set, setup() applies a TTL on the table. Null disables TTL. */
private ?int $retention = null;

/**
* @param string $host ClickHouse host
* @param string $username ClickHouse username (default: 'default')
Expand Down Expand Up @@ -360,6 +363,34 @@ public function isAsyncCleanup(): bool
return $this->asyncCleanup;
}

/**
* Set the retention window in days. When set, setup() applies a TTL so
* rows older than the window are dropped by background merges. Pass null
* to disable (the default).
*
* @param int|null $days
* @return self
* @throws Exception If $days is not positive
*/
public function setRetention(?int $days): self
{
if ($days !== null && $days < 1) {
throw new Exception('Retention must be a positive number of days');
}
$this->retention = $days;
return $this;
}
Comment on lines +375 to +382

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No test coverage for the new retention feature

setRetention, getRetention, the validation path (negative/zero days → Exception), and the TTL branch inside setup() are not exercised by any test in ClickHouseTest. At minimum, a unit test for the setter/getter contract and the guard clause would protect against regressions without requiring a live ClickHouse instance.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


/**
* Get the retention window in days, or null when TTL is disabled.
*
* @return int|null
*/
public function getRetention(): ?int
{
return $this->retention;
}

/**
* Override getAttributes to provide extended attributes for ClickHouse.
* Includes existing attributes from parent and adds new missing ones.
Expand Down Expand Up @@ -791,6 +822,34 @@ public function setup(): void
";

$this->query($createTableSql);

// Apply retention as a separate, idempotent ALTER. CREATE TABLE IF NOT
// EXISTS won't add a TTL to a table that already exists, and MODIFY TTL
// is a no-op when the TTL already matches, so setup() stays re-runnable.
// materialize_ttl_after_modify = 0 defers the purge to background merges
// rather than an immediate, I/O-heavy mutation.
if ($this->retention !== null) {
$this->query(
"ALTER TABLE {$escapedDatabaseAndTable} "
. "MODIFY TTL toDateTime(time) + INTERVAL {$this->retention} DAY "
. 'SETTINGS materialize_ttl_after_modify = 0'
);
} else {
// Disabling retention must actively strip any TTL a previous run
// applied; otherwise rows keep being purged despite retention being
// null. ClickHouse errors (code 36) when REMOVE TTL runs on a table
// that has no TTL, so swallow that specific case to keep setup()
// idempotent.
try {
$this->query(
"ALTER TABLE {$escapedDatabaseAndTable} REMOVE TTL"
);
} catch (Exception $e) {
if (!str_contains($e->getMessage(), "doesn't have any table TTL expression")) {
throw $e;
}
}
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
}

/**
Expand Down
68 changes: 68 additions & 0 deletions tests/Audit/Adapter/ClickHouseTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,74 @@ public function testSetSecure(): void
$this->assertInstanceOf(ClickHouse::class, $result);
}

/**
* Test setRetention stores the value and getRetention returns it
*/
public function testSetRetention(): void
{
$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
);

$this->assertNull($adapter->getRetention());

$result = $adapter->setRetention(30);
$this->assertInstanceOf(ClickHouse::class, $result);
$this->assertEquals(30, $adapter->getRetention());
}

/**
* Test setRetention accepts null to disable retention
*/
public function testSetRetentionAcceptsNull(): void
{
$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
);

$adapter->setRetention(30);
$adapter->setRetention(null);
$this->assertNull($adapter->getRetention());
}

/**
* Test setRetention rejects zero days
*/
public function testSetRetentionRejectsZero(): void
{
$this->expectException(Exception::class);
$this->expectExceptionMessage('Retention must be a positive number of days');

$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
);

$adapter->setRetention(0);
}

/**
* Test setRetention rejects negative days
*/
public function testSetRetentionRejectsNegative(): void
{
$this->expectException(Exception::class);
$this->expectExceptionMessage('Retention must be a positive number of days');

$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
);

$adapter->setRetention(-1);
}

/**
* Test shared tables configuration
*/
Expand Down
Loading