Skip to content

Commit f2938a1

Browse files
lohanidamodarlohanidamodar
committed
fix(adapter): require day-aligned start for hybrid route; tighten purge boundary widening
Two boundary-alignment bugs Greptile flagged after the prior daily-route hardening landed: - selectAggregateSource() required day-aligned bounds for the closed-day 'daily' route but not for 'hybrid' (windows straddling today). A non-midnight start would route to hybrid; buildDailyTimeFilters then floored the lower bound to start-of-day, over-counting the AM hours of the start day on the daily branch. Fix: hoist isDayAligned(start) before the hybrid decision so any mid-day start falls back to raw. - translateTimeQueriesToDayBoundaries (rollup-purge translation) widened bounds outward: lower bounds floored to start-of-day, upper bounds ceiled to start-of-next-day. That over-deleted rollup rows for any day not entirely covered by the user's purge range. Switched to the conservative direction: lower bounds ceil to next-day-start unless already at midnight, upper bounds floor to start-of-day. Daily rollup purges now only touch days fully inside the caller's window; partial start/end days are left untouched (their rollup rows go slightly stale until the next ingest cycle, which is acceptable since the purge path isn't on the hot read flow). testWindowStraddlesTodayRoutesHybrid pinned to UTC midnight to actually exercise the hybrid route. testHybridSumFloorsDailyLowerBoundToStartOfDay was asserting the old buggy floor-to-midnight behavior; replaced with testMidDayStartWithHybridWindowFallsBackToRaw which proves the fix excludes pre-start events on the start day.
1 parent 5c113d0 commit f2938a1

2 files changed

Lines changed: 35 additions & 27 deletions

File tree

‎src/Usage/Adapter/ClickHouse.php‎

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2313,11 +2313,15 @@ private function selectAggregateSource(array $plan): string
23132313
}
23142314
}
23152315

2316+
if (!$this->isDayAligned($startDt)) {
2317+
return 'raw';
2318+
}
2319+
23162320
if ($endDt >= $boundaryDt) {
23172321
return 'hybrid';
23182322
}
23192323

2320-
if (!$this->isDayAligned($startDt) || !$this->isDayAligned($endDt)) {
2324+
if (!$this->isDayAligned($endDt)) {
23212325
return 'raw';
23222326
}
23232327

@@ -2494,10 +2498,11 @@ private function floorToStartOfDay(?string $value): ?string
24942498

24952499
/**
24962500
* Rewrite the time bounds in a query list so a purge against a
2497-
* day-bucketed rollup matches whole-day windows. Lower bounds are
2498-
* floored to start-of-day; upper bounds are floored to the start of the
2499-
* next day so a half-open mid-day end still includes that day's rollup
2500-
* row. Other queries pass through unchanged.
2501+
* day-bucketed rollup only touches days *entirely* covered by the
2502+
* caller's range. Mid-day boundaries shrink inward — lower bounds
2503+
* ceil to the next day's midnight (skipping a partial start day),
2504+
* upper bounds floor to the same day's midnight (skipping a partial
2505+
* end day). Other queries pass through unchanged.
25012506
*
25022507
* @param array<Query> $queries
25032508
* @return array<Query>
@@ -2515,28 +2520,28 @@ private function translateTimeQueriesToDayBoundaries(array $queries): array
25152520
$values = $query->getValues();
25162521

25172522
if ($method === Query::TYPE_GREATER_EQUAL || $method === Query::TYPE_GREATER) {
2518-
$floored = $this->floorToStartOfDay($this->stringifyTime($values[0] ?? null));
2519-
if ($floored === null) {
2523+
$ceiled = $this->ceilLowerToFullyCoveredDayStart($this->stringifyTime($values[0] ?? null));
2524+
if ($ceiled === null) {
25202525
$output[] = $query;
25212526
continue;
25222527
}
2523-
$output[] = Query::greaterThanEqual('time', $floored);
2528+
$output[] = Query::greaterThanEqual('time', $ceiled);
25242529
continue;
25252530
}
25262531

25272532
if ($method === Query::TYPE_LESSER_EQUAL || $method === Query::TYPE_LESSER) {
2528-
$ceiled = $this->floorToStartOfNextDay($this->stringifyTime($values[0] ?? null));
2529-
if ($ceiled === null) {
2533+
$floored = $this->floorToStartOfDay($this->stringifyTime($values[0] ?? null));
2534+
if ($floored === null) {
25302535
$output[] = $query;
25312536
continue;
25322537
}
2533-
$output[] = Query::lessThan('time', $ceiled);
2538+
$output[] = Query::lessThan('time', $floored);
25342539
continue;
25352540
}
25362541

25372542
if ($method === Query::TYPE_BETWEEN) {
2538-
$lower = $this->floorToStartOfDay($this->stringifyTime($values[0] ?? null));
2539-
$upper = $this->floorToStartOfNextDay($this->stringifyTime($values[1] ?? null));
2543+
$lower = $this->ceilLowerToFullyCoveredDayStart($this->stringifyTime($values[0] ?? null));
2544+
$upper = $this->floorToStartOfDay($this->stringifyTime($values[1] ?? null));
25402545
if ($lower !== null) {
25412546
$output[] = Query::greaterThanEqual('time', $lower);
25422547
}
@@ -2553,22 +2558,25 @@ private function translateTimeQueriesToDayBoundaries(array $queries): array
25532558
}
25542559

25552560
/**
2556-
* Floor a stringified timestamp to the UTC start of the next day. Used to
2557-
* expand an inclusive upper bound into a half-open `< floor(next_day)`
2558-
* window for day-bucketed rollups.
2561+
* Round a lower-bound timestamp up to the first day-start that is fully
2562+
* inside the caller's range. Midnight stays as-is; any non-midnight value
2563+
* advances to the next day's midnight so the rollup purge skips the
2564+
* partially-covered start day.
25592565
*/
2560-
private function floorToStartOfNextDay(?string $value): ?string
2566+
private function ceilLowerToFullyCoveredDayStart(?string $value): ?string
25612567
{
2562-
$floored = $this->floorToStartOfDay($value);
2563-
if ($floored === null) {
2568+
if ($value === null) {
25642569
return null;
25652570
}
25662571
try {
2567-
$dt = new DateTime($floored, new DateTimeZone('UTC'));
2572+
$dt = new DateTime($value, new DateTimeZone('UTC'));
25682573
} catch (Exception $e) {
25692574
return null;
25702575
}
2571-
$dt->modify('+1 day');
2576+
if ($dt->format('H:i:s.u') !== '00:00:00.000000') {
2577+
$dt->setTime(0, 0, 0, 0);
2578+
$dt->modify('+1 day');
2579+
}
25722580
return $dt->format('Y-m-d H:i:s.v');
25732581
}
25742582

‎tests/Usage/Adapter/ClickHouseRoutingTest.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ public function testWindowStraddlesTodayRoutesHybrid(): void
136136
{
137137
$this->adapter->clearRouteLog();
138138

139-
$start = (new DateTime('-7 days'))->format('Y-m-d H:i:s');
139+
$start = (new DateTime('-7 days', new DateTimeZone('UTC')))->setTime(0, 0, 0)->format('Y-m-d H:i:s');
140140
$end = (new DateTime('+1 hour'))->format('Y-m-d H:i:s');
141141

142142
$rawSum = $this->sumRaw('routed.metric', $start, $end);
@@ -191,10 +191,10 @@ public function testFilterOnNonDailyColumnForcesRaw(): void
191191
$this->assertSame('raw', $log[0]['route']);
192192
}
193193

194-
public function testHybridSumFloorsDailyLowerBoundToStartOfDay(): void
194+
public function testMidDayStartWithHybridWindowFallsBackToRaw(): void
195195
{
196-
$metric = 'routed.metric.hybrid_boundary';
197-
$this->seedHistoricalRow($metric, 70, '-2 days 03:00:00', ['path' => '/v1/floor']);
196+
$metric = 'routed.metric.hybrid_mid_day';
197+
$this->seedHistoricalRow($metric, 70, '-2 days 03:00:00', ['path' => '/v1/early']);
198198

199199
$this->adapter->clearRouteLog();
200200

@@ -209,8 +209,8 @@ public function testHybridSumFloorsDailyLowerBoundToStartOfDay(): void
209209

210210
$log = $this->adapter->getRouteLog();
211211
$this->assertCount(1, $log);
212-
$this->assertSame('hybrid', $log[0]['route']);
213-
$this->assertSame(70, $sum, 'hybrid daily branch must include rollup row on the start day');
212+
$this->assertSame('raw', $log[0]['route'], 'a mid-day start with a hybrid window must fall back to raw');
213+
$this->assertSame(0, $sum, 'pre-start events on the same day must not be included in the result');
214214
}
215215

216216
public function testIntervalPresentForcesRaw(): void

0 commit comments

Comments
 (0)