Skip to content

Commit 88da02f

Browse files
fix(simple-engine): SQL start timestamp multiplied by scrape_interval (#202) (#330)
get_duration() returns seconds, not a count of scrape intervals, so multiplying by prometheus_scrape_interval again produced a window scrape_interval× too wide. Drop the extra factor and rename the variable from scrape_intervals to duration_secs. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 605b633 commit 88da02f

2 files changed

Lines changed: 59 additions & 11 deletions

File tree

asap-query-engine/src/engines/simple_engine/sql.rs

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -59,22 +59,22 @@ impl SimpleEngine {
5959
) -> u64 {
6060
match query_pattern_type {
6161
QueryPatternType::OnlyTemporal => {
62-
let scrape_intervals = match_result
62+
let duration_secs = match_result
6363
.outer_data()
6464
.expect("OnlyTemporal pattern guarantees outer_data is present")
6565
.time_info
6666
.clone()
6767
.get_duration() as u64;
68-
end_timestamp - (scrape_intervals * self.prometheus_scrape_interval * 1000)
68+
end_timestamp - (duration_secs * 1000)
6969
}
7070
QueryPatternType::OneTemporalOneSpatial => {
71-
let scrape_intervals = match_result
71+
let duration_secs = match_result
7272
.inner_data()
7373
.expect("OneTemporalOneSpatial pattern guarantees inner_data is present")
7474
.time_info
7575
.clone()
7676
.get_duration() as u64;
77-
end_timestamp - (scrape_intervals * self.prometheus_scrape_interval * 1000)
77+
end_timestamp - (duration_secs * 1000)
7878
}
7979
QueryPatternType::OnlySpatial => {
8080
end_timestamp - (self.prometheus_scrape_interval * 1000)
@@ -172,17 +172,17 @@ impl SimpleEngine {
172172
let data_range_ms = match query_pattern_type {
173173
QueryPatternType::OnlySpatial => None,
174174
QueryPatternType::OnlyTemporal => {
175-
let scrape_intervals = query_data.time_info.clone().get_duration() as u64;
176-
Some(scrape_intervals * self.prometheus_scrape_interval * 1000)
175+
let duration_secs = query_data.time_info.clone().get_duration() as u64;
176+
Some(duration_secs * 1000)
177177
}
178178
QueryPatternType::OneTemporalOneSpatial => {
179-
let scrape_intervals = match_result
179+
let duration_secs = match_result
180180
.inner_data()
181181
.expect("OneTemporalOneSpatial pattern guarantees inner_data is present")
182182
.time_info
183183
.clone()
184184
.get_duration() as u64;
185-
Some(scrape_intervals * self.prometheus_scrape_interval * 1000)
185+
Some(duration_secs * 1000)
186186
}
187187
};
188188

@@ -519,9 +519,8 @@ impl SimpleEngine {
519519
// Calculate timestamps - similar to OnlyTemporal
520520
let end_timestamp =
521521
self.validate_and_align_end_timestamp(query_time, QueryPatternType::OnlyTemporal);
522-
let scrape_intervals = match_result.outer_data()?.time_info.get_duration() as u64;
523-
let start_timestamp =
524-
end_timestamp - (scrape_intervals * self.prometheus_scrape_interval * 1000);
522+
let duration_secs = match_result.outer_data()?.time_info.get_duration() as u64;
523+
let start_timestamp = end_timestamp - (duration_secs * 1000);
525524

526525
let timestamps = QueryTimestamps {
527526
start_timestamp,

asap-query-engine/src/tests/query_equivalence_tests.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,55 @@ mod tests {
183183
assert_execution_context_equivalent(&promql_context, &sql_context, "spatial_avg");
184184
}
185185

186+
/// Regression test for issue #202.
187+
/// With scrape_interval=15s, a 150s SQL temporal query must produce a 150_000ms window.
188+
/// Bug: start = end - (150 * 15 * 1000) = end - 2_250_000ms (15× too wide).
189+
#[test]
190+
fn test_bug_sql_start_timestamp_multiplied_by_scrape_interval() {
191+
let scrape_interval = 15u64;
192+
let window_seconds = 150u64;
193+
let sql_query = "SELECT SUM(value) FROM cpu_usage \
194+
WHERE time BETWEEN DATEADD(s, -150, '2025-10-01 00:00:00') AND '2025-10-01 00:00:00' \
195+
GROUP BY L1, L2, L3, L4";
196+
197+
let (_, sql_config, streaming_config) = TestConfigBuilder::new("cpu_usage")
198+
.with_grouping_labels(vec!["L1", "L2", "L3", "L4"])
199+
.with_scrape_interval(scrape_interval) // USED: propagated to SimpleEngine::prometheus_scrape_interval
200+
.add_temporal_query(
201+
"sum_over_time(cpu_usage[150s])", // NOT USED: TestConfigBuilder requires a PromQL string; timestamp calculation is SQL-only
202+
sql_query, // USED: parsed to extract the 150s duration
203+
1, // NOT USED: agg_id just satisfies the builder; calculate_start_timestamp_sql never consults it
204+
window_seconds, // NOT USED: stored in AggregationConfig, not read by timestamp calculation
205+
WindowType::Tumbling, // NOT USED: same as above
206+
)
207+
.build_both();
208+
209+
let sql_engine = SimpleEngine::new(
210+
Arc::new(NoOpStore),
211+
sql_config,
212+
streaming_config,
213+
scrape_interval,
214+
QueryLanguage::sql,
215+
);
216+
217+
// query_time_sec is ignored for fixed-date SQL (only used for NOW() resolution)
218+
let sql_context = sql_engine
219+
.build_query_execution_context_sql(sql_query.to_string(), 0.0)
220+
.expect("Failed to build SQL context");
221+
222+
let start_ms = sql_context.store_plan.values_query.start_timestamp;
223+
let end_ms = sql_context.store_plan.values_query.end_timestamp;
224+
let actual_window_ms = end_ms - start_ms;
225+
let expected_window_ms = window_seconds * 1000;
226+
227+
assert_eq!(
228+
actual_window_ms, expected_window_ms,
229+
"SQL window is {}ms but should be {}ms — \
230+
get_duration() returns seconds, not a count of scrape intervals",
231+
actual_window_ms, expected_window_ms
232+
);
233+
}
234+
186235
#[test]
187236
fn test_spatial_of_temporal_sum_equivalence() {
188237
let scrape_interval = 1;

0 commit comments

Comments
 (0)