Skip to content

Commit eb8a314

Browse files
zzylolclaude
andcommitted
fix(precompute): make panes left-open like PromQL ranges
A PromQL range at T covers (T - range, T], and the executor's series windows follow it. Panes were [start, end), so a sample at a pane boundary was summarized in the next pane. Panes are now (end - width, end]. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent a62e7de commit eb8a314

3 files changed

Lines changed: 22 additions & 15 deletions

File tree

‎data_plane/src/precompute.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//! watermark passes its end.
33
//!
44
//! For every [`SummaryDefinition`] the engine buffers the samples of its
5-
//! `Scan`'s metric by tumbling pane. A pane `[end - pane, end)` closes when
5+
//! `Scan`'s metric by tumbling pane. A pane `(end - pane, end]` closes when
66
//! the watermark (the newest sample time minus the allowed lateness) reaches
77
//! `end`. Closing it runs the plan's `Scan` over the buffered samples, which
88
//! applies the label matchers, then the definition's build from its
@@ -205,8 +205,10 @@ impl Precompute {
205205
}
206206

207207
/// End of the tumbling pane of width `pane_ms` that contains `timestamp_ms`.
208+
/// Panes are `(end - pane_ms, end]`, like PromQL ranges: a sample at a pane
209+
/// boundary belongs to the pane it ends.
208210
pub fn pane_end(timestamp_ms: i64, pane_ms: i64) -> i64 {
209-
(timestamp_ms.div_euclid(pane_ms) + 1) * pane_ms
211+
-(-timestamp_ms).div_euclid(pane_ms) * pane_ms
210212
}
211213

212214
/// Runs the plan's `Scan` over `samples`, then the definition's build over

‎data_plane/src/sds.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@
99
//! frontier node <- ... <- TimeRange(width) <- TimeShift(offset)* <- Scan
1010
//! ```
1111
//!
12-
//! At evaluation time `T` it covers `[T - offset - width, T - offset)`.
12+
//! At evaluation time `T` it covers `(T - offset - width, T - offset]`, the
13+
//! PromQL range convention.
1314
//! Outputs that differ only in `offset` compute the same thing over different
1415
//! panes, so they share one [`SummaryDefinition`]: the data plane builds each
1516
//! pane of a definition once, and every output reads the pane it needs.
@@ -30,7 +31,7 @@ pub struct DefinitionId(pub String);
3031
#[derive(Debug, Clone, PartialEq)]
3132
pub struct SummaryDefinition {
3233
pub id: DefinitionId,
33-
/// Pane width: each stored summary covers `[end - pane_ms, end)`.
34+
/// Pane width: each stored summary covers `(end - pane_ms, end]`.
3435
pub pane_ms: i64,
3536
/// The frontier output that builds a pane; any output of the definition
3637
/// computes the same, and this is the first in node order.

‎data_plane/tests/precompute.rs‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ fn sample(metric: &str, job: &str, timestamp_ms: i64, value: f64) -> RawSample {
2929
}
3030
}
3131

32-
/// One sample every 15 s for each job in `[from, to)`.
32+
/// One sample every 15 s for each job, at `from`, `from + 15 s`, ... before `to`.
3333
fn samples(metric: &str, jobs: &[&str], from: i64, to: i64) -> Vec<RawSample> {
3434
(from..to)
3535
.step_by(15_000)
@@ -43,7 +43,8 @@ fn samples(metric: &str, jobs: &[&str], from: i64, to: i64) -> Vec<RawSample> {
4343
fn builds_panes_after_the_first_as_the_watermark_passes() {
4444
let (mut engine, sds) = precompute(common::plan("example4b", false));
4545
let id = sds.definitions.keys().next().unwrap().clone();
46-
engine.ingest(&samples("latency_ms", &["a", "b"], 0, 35 * MINUTE));
46+
// The first sample, at 1 min, opens pane (0, 10 min], which is skipped.
47+
engine.ingest(&samples("latency_ms", &["a", "b"], MINUTE, 35 * MINUTE));
4748
assert_eq!(engine.store.get(&id, PANE).map(|_| ()), None, "first pane");
4849
for end in [2 * PANE, 3 * PANE] {
4950
let pane = engine.store.get(&id, end).expect("built pane");
@@ -57,9 +58,9 @@ fn builds_panes_after_the_first_as_the_watermark_passes() {
5758
let stats = engine.stats;
5859
assert_eq!(stats.panes_built, 2);
5960
assert_eq!(stats.panes_failed, 0);
60-
// Samples in [0, 10 min) are dropped with the first pane.
61-
assert_eq!(stats.dropped, 2 * 40);
62-
assert_eq!(stats.accepted, 2 * 100);
61+
// Samples in (0, 10 min] are dropped with the first pane.
62+
assert_eq!(stats.dropped, 2 * 37);
63+
assert_eq!(stats.accepted, 2 * 99);
6364
}
6465

6566
/// Samples of other metrics are ignored; samples for a built pane are dropped.
@@ -81,7 +82,7 @@ fn stores_empty_panes() {
8182
let (mut engine, sds) = precompute(common::plan("example4b", false));
8283
let id = sds.definitions.keys().next().unwrap().clone();
8384
engine.ingest(&samples("latency_ms", &["a"], 0, 20 * MINUTE));
84-
engine.ingest(&samples("latency_ms", &["a"], 40 * MINUTE, 41 * MINUTE));
85+
engine.ingest(&[sample("latency_ms", "a", 45 * MINUTE, 1.0)]);
8586
assert_eq!(engine.store.get(&id, 2 * PANE).unwrap().rows().len(), 1);
8687
assert_eq!(engine.store.get(&id, 3 * PANE).unwrap().rows().len(), 0);
8788
assert_eq!(engine.store.get(&id, 4 * PANE).unwrap().rows().len(), 0);
@@ -106,11 +107,14 @@ fn applies_label_matchers() {
106107
assert_eq!(engine.store.get(&id, 2 * PANE).unwrap().rows().len(), 1);
107108
}
108109

109-
/// A sample belongs to the pane whose end follows it, including at negative times.
110+
/// Panes are `(end - width, end]`, like PromQL ranges: a sample at a
111+
/// boundary belongs to the pane it ends.
110112
#[test]
111-
fn pane_end_is_the_next_boundary() {
112-
assert_eq!(pane_end(0, PANE), PANE);
113-
assert_eq!(pane_end(PANE - 1, PANE), PANE);
114-
assert_eq!(pane_end(PANE, PANE), 2 * PANE);
113+
fn panes_are_left_open() {
114+
assert_eq!(pane_end(1, PANE), PANE);
115+
assert_eq!(pane_end(PANE, PANE), PANE);
116+
assert_eq!(pane_end(PANE + 1, PANE), 2 * PANE);
117+
assert_eq!(pane_end(0, PANE), 0);
115118
assert_eq!(pane_end(-1, PANE), 0);
119+
assert_eq!(pane_end(-PANE, PANE), -PANE);
116120
}

0 commit comments

Comments
 (0)