diff --git a/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlparser_test.rs b/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlparser_test.rs index 13d1c3b..4db883a 100644 --- a/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlparser_test.rs +++ b/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlparser_test.rs @@ -99,7 +99,7 @@ mod tests { if let Some(query_data) = SQLPatternParser::new(&schema, time).parse_query(&statements) { let result = matcher.query_info_to_pattern(&query_data); assert!(result.is_valid()); - assert_eq!(result.query_type, vec![QueryType::Spatial]); + assert_eq!(result.query_type, vec![QueryType::TemporalGeneric]); } } @@ -127,7 +127,7 @@ mod tests { fn test_dated_spatial_avg() { check_query( "SELECT AVG(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, '2025-10-01 00:00:00') AND '2025-10-01 00:00:00' GROUP BY L1, L2, L3, L4", - vec![QueryType::Spatial], + vec![QueryType::TemporalGeneric], None, ); } @@ -136,7 +136,7 @@ mod tests { fn test_dated_spatial_quantile() { check_query( "SELECT QUANTILE(0.95, value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, '2025-10-01 00:00:00') AND '2025-10-01 00:00:00' GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -145,7 +145,7 @@ mod tests { fn test_dated_spatial_of_temporal_quantile_max() { check_query( "SELECT QUANTILE(0.95, value) FROM (SELECT MAX(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, '2025-10-01 00:00:00') AND '2025-10-01 00:00:00' GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -203,7 +203,7 @@ mod tests { fn test_spatial_sum() { check_query( "SELECT SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -212,7 +212,7 @@ mod tests { fn test_spatial_max() { check_query( "SELECT MAX(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1, L2", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -221,7 +221,7 @@ mod tests { fn test_spatial_min() { check_query( "SELECT MIN(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1, L2, L3", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -230,7 +230,7 @@ mod tests { fn test_spatial_avg() { check_query( "SELECT AVG(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1, L2, L3, L4", - vec![QueryType::Spatial], + vec![QueryType::TemporalGeneric], None, ); } @@ -239,7 +239,7 @@ mod tests { fn test_spatial_quantile() { check_query( "SELECT QUANTILE(0.95, value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -250,7 +250,7 @@ mod tests { fn test_spatial_of_temporal_sum_sum() { check_query( "SELECT SUM(result) FROM (SELECT SUM(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -259,7 +259,7 @@ mod tests { fn test_spatial_of_temporal_sum_min() { check_query( "SELECT SUM(result) FROM (SELECT MIN(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -268,7 +268,7 @@ mod tests { fn test_spatial_of_temporal_sum_max() { check_query( "SELECT SUM(result) FROM (SELECT MAX(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2, L3", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -277,7 +277,7 @@ mod tests { fn test_spatial_of_temporal_sum_avg() { check_query( "SELECT SUM(result) FROM (SELECT AVG(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2, L3, L4", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::TemporalGeneric, QueryType::TemporalGeneric], None, ); } @@ -286,7 +286,7 @@ mod tests { fn test_spatial_of_temporal_max_sum() { check_query( "SELECT MAX(result) FROM (SELECT SUM(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -295,7 +295,7 @@ mod tests { fn test_spatial_of_temporal_max_min() { check_query( "SELECT MAX(result) FROM (SELECT MIN(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -304,7 +304,7 @@ mod tests { fn test_spatial_of_temporal_max_max() { check_query( "SELECT MAX(result) FROM (SELECT MAX(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2, L3", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -313,7 +313,7 @@ mod tests { fn test_spatial_of_temporal_max_avg() { check_query( "SELECT MAX(result) FROM (SELECT AVG(value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2, L3, L4", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::TemporalGeneric, QueryType::TemporalGeneric], None, ); } @@ -322,7 +322,7 @@ mod tests { fn test_spatial_of_temporal_quantile_max() { check_query( "SELECT QUANTILE(0.95, value) FROM (SELECT MAX(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -331,7 +331,7 @@ mod tests { fn test_spatial_of_temporal_quantile_min() { check_query( "SELECT QUANTILE(0.95, value) FROM (SELECT MIN(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -340,7 +340,7 @@ mod tests { fn test_spatial_of_temporal_quantile_sum() { check_query( "SELECT QUANTILE(0.95, value) FROM (SELECT SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -349,7 +349,7 @@ mod tests { fn test_spatial_of_temporal_quantile_avg() { check_query( "SELECT QUANTILE(0.95, value) FROM (SELECT AVG(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -358,7 +358,7 @@ mod tests { fn test_spatial_of_temporal_avg_quantile() { check_query( "SELECT AVG(result) FROM (SELECT QUANTILE(0.95, value) AS result FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2", - vec![QueryType::Spatial, QueryType::TemporalQuantile], + vec![QueryType::SpatioTemporal, QueryType::TemporalQuantile], None, ); } @@ -367,7 +367,7 @@ mod tests { fn test_spatial_of_temporal_quantile_quantile() { check_query( "SELECT QUANTILE(0.95, value) FROM (SELECT QUANTILE(0.95, value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1, L2, L3", - vec![QueryType::Spatial, QueryType::TemporalQuantile], + vec![QueryType::SpatioTemporal, QueryType::TemporalQuantile], None, ); } @@ -434,7 +434,7 @@ mod tests { fn test_spatial_percentile() { check_query( "SELECT PERCENTILE(value, 95) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -452,7 +452,7 @@ mod tests { fn test_spatial_of_temporal_percentile_max() { check_query( "SELECT PERCENTILE(value, 95) FROM (SELECT MAX(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -473,7 +473,7 @@ mod tests { fn test_clickhouse_spatial_quantile() { check_query( "SELECT quantile(0.95)(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -491,7 +491,7 @@ mod tests { fn test_clickhouse_spatial_of_temporal_quantile_max() { check_query( "SELECT quantile(0.95)(value) FROM (SELECT MAX(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1, L2, L3, L4) GROUP BY L1", - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -537,7 +537,7 @@ mod tests { fn test_clickhouse_explicit_datetime_spatial_quantile() { check_query( "SELECT quantile(0.95)(value) FROM cpu_usage WHERE time BETWEEN '2025-10-01 00:00:00' AND '2025-10-01 00:00:01' GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } @@ -738,7 +738,7 @@ mod tests { WHERE time BETWEEN DATEADD(s, -15, '2025-10-01 00:00:00') AND '2025-10-01 00:00:00' \ GROUP BY L1, L2, L3, L4", 15.0, - vec![QueryType::Spatial], + vec![QueryType::TemporalGeneric], None, ); } @@ -792,7 +792,7 @@ mod tests { GROUP BY L1, L2, L3, L4) \ GROUP BY L1", 15.0, - vec![QueryType::Spatial, QueryType::TemporalGeneric], + vec![QueryType::SpatioTemporal, QueryType::TemporalGeneric], None, ); } @@ -1304,7 +1304,7 @@ mod tests { "SELECT SUM(value) FROM cpu_usage \ WHERE time >= DATEADD(s, -1, NOW()) AND time < NOW() \ GROUP BY L1", - vec![QueryType::Spatial], + vec![QueryType::SpatioTemporal], None, ); } diff --git a/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_matcher.rs b/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_matcher.rs index d7f8f34..0a028b7 100644 --- a/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_matcher.rs +++ b/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_matcher.rs @@ -7,6 +7,15 @@ use std::collections::HashSet; #[derive(Debug, Clone, PartialEq)] pub enum QueryType { + /// Dead: `query_info_to_pattern` no longer constructs this variant (#487). A query + /// spanning exactly one scrape interval used to be forced into `Spatial` regardless + /// of its GROUP BY, distinct from `Temporal*`/`SpatioTemporal`, which checked labels. + /// That was never a real structural difference — `Spatial` was just `Temporal` with + /// `range == scrape_interval` — so it's now classified by the same label-coverage + /// check as everything else and lands in `TemporalGeneric`/`TemporalQuantile` + /// (full GROUP BY) or `SpatioTemporal` (partial GROUP BY) like any other duration. + /// Kept as a variant only because downstream `sql.rs` still matches on it; deleted + /// once that's migrated to structural checks. Spatial, TemporalGeneric, TemporalQuantile, @@ -245,70 +254,35 @@ impl SQLPatternMatcher { let mut sql_query = SQLQuery::new(Vec::new(), None, None); - for (i, (metric, aggregation_info, scrape_duration, labels, time_info)) in - query_data.iter().enumerate() - { - if i < query_data.len() - 1 { - // Not the last query - // let time_info = TimeInfo::new("time".to_string(), *start, *scrape_duration); // You may need to adjust this - sql_query.add_subquery( - QueryType::Spatial, - aggregation_info.clone(), - metric.clone(), - labels.clone(), - time_info.clone(), - ); + // Every subquery — outer or inner, whatever its duration — is classified purely + // by aggregation + label coverage. Duration only gates validity above + // (SpatialDurationSmall); it no longer decides the shape. A query that covers + // all metadata columns is a plain temporal read (Quantile/Generic); anything + // grouping by a subset of labels is SpatioTemporal, regardless of how long a + // range it spans — a 1-interval query with a partial GROUP BY is exactly as + // "spatiotemporal" as a 10-interval one. + for (metric, aggregation_info, _scraped_intervals, labels, time_info) in query_data.iter() { + let has_all_labels = self + .schema + .get_metadata_columns(metric) + .map(|schema_metadata_columns| labels == schema_metadata_columns) + .unwrap_or(true); + + let query_type = if !has_all_labels { + QueryType::SpatioTemporal + } else if aggregation_info.get_name() == "QUANTILE" { + QueryType::TemporalQuantile } else { - // Last query - // let time_info = TimeInfo::new("time".to_string(), *start, *scrape_duration); - - if (scrape_duration - 1.0).abs() < f64::EPSILON { - sql_query.add_subquery( - QueryType::Spatial, - aggregation_info.clone(), - metric.clone(), - labels.clone(), - time_info.clone(), - ); - } else if *scrape_duration > 1.0 { - // Check if labels match all metadata columns - let has_all_labels = self - .schema - .get_metadata_columns(metric) - .map(|schema_metadata_columns| labels == schema_metadata_columns) - .unwrap_or(true); - - if has_all_labels { - // Full temporal query with all labels (PromQL-equivalent) - if aggregation_info.get_name() == "QUANTILE" { - sql_query.add_subquery( - QueryType::TemporalQuantile, - aggregation_info.clone(), - metric.clone(), - labels.clone(), - time_info.clone(), - ); - } else { - sql_query.add_subquery( - QueryType::TemporalGeneric, - aggregation_info.clone(), - metric.clone(), - labels.clone(), - time_info.clone(), - ); - } - } else { - // SpatioTemporal: spans multiple scrape intervals but groups by subset of labels - sql_query.add_subquery( - QueryType::SpatioTemporal, - aggregation_info.clone(), - metric.clone(), - labels.clone(), - time_info.clone(), - ); - } - } - } + QueryType::TemporalGeneric + }; + + sql_query.add_subquery( + query_type, + aggregation_info.clone(), + metric.clone(), + labels.clone(), + time_info.clone(), + ); } sql_query diff --git a/asap-query-engine/src/engines/simple_engine/mod.rs b/asap-query-engine/src/engines/simple_engine/mod.rs index b59e213..01309fd 100644 --- a/asap-query-engine/src/engines/simple_engine/mod.rs +++ b/asap-query-engine/src/engines/simple_engine/mod.rs @@ -326,11 +326,13 @@ impl SimpleEngine { .cloned() } - /// Validates and potentially aligns end timestamp based on query pattern + /// Validates and potentially aligns end timestamp based on query duration. + /// `is_single_interval` is true when the query's range spans exactly one + /// data ingestion interval (the old "OnlySpatial" shape). fn validate_and_align_end_timestamp( &self, mut end_timestamp: u64, - query_pattern_type: QueryPatternType, + is_single_interval: bool, ) -> u64 { let interval_ms = self.data_ingestion_interval_ms; @@ -342,13 +344,11 @@ impl SimpleEngine { ); } - // For OnlySpatial, align end_timestamp to nearest scrape interval - if query_pattern_type == QueryPatternType::OnlySpatial - && !end_timestamp.is_multiple_of(interval_ms) - { + // For single-interval queries, align end_timestamp to nearest scrape interval + if is_single_interval && !end_timestamp.is_multiple_of(interval_ms) { let aligned_end_timestamp = (end_timestamp / interval_ms) * interval_ms; debug!( - "OnlySpatial query: Aligning end_timestamp from {} to {} using data ingestion interval of {} ms", + "Single-interval query: Aligning end_timestamp from {} to {} using data ingestion interval of {} ms", end_timestamp, aligned_end_timestamp, self.data_ingestion_interval_ms ); end_timestamp = aligned_end_timestamp; diff --git a/asap-query-engine/src/engines/simple_engine/promql.rs b/asap-query-engine/src/engines/simple_engine/promql.rs index ddf8137..28cd0a5 100644 --- a/asap-query-engine/src/engines/simple_engine/promql.rs +++ b/asap-query-engine/src/engines/simple_engine/promql.rs @@ -65,7 +65,15 @@ impl SimpleEngine { query_time }; - end_timestamp = self.validate_and_align_end_timestamp(end_timestamp, query_pattern_type); + // promql-parser supports a literal `ms` duration suffix (e.g. `[500ms]`), so + // .num_seconds() would truncate genuinely sub-second range vectors to 0. + let range_ms = match_result + .get_range_duration() + .map(|d| d.num_milliseconds() as u64) + .unwrap_or(self.data_ingestion_interval_ms); + let is_single_interval = range_ms == self.data_ingestion_interval_ms; + + end_timestamp = self.validate_and_align_end_timestamp(end_timestamp, is_single_interval); let start_timestamp = self.calculate_start_timestamp_promql(end_timestamp, query_pattern_type, match_result); diff --git a/asap-query-engine/src/engines/simple_engine/sql.rs b/asap-query-engine/src/engines/simple_engine/sql.rs index ba2c70b..2a96596 100644 --- a/asap-query-engine/src/engines/simple_engine/sql.rs +++ b/asap-query-engine/src/engines/simple_engine/sql.rs @@ -197,7 +197,15 @@ impl SimpleEngine { .get_duration() as u64; end_timestamp - (duration_secs * 1000) } - QueryPatternType::OnlySpatial => end_timestamp - self.data_ingestion_interval_ms, + QueryPatternType::OnlySpatial => { + let duration_secs = match_result + .outer_data() + .expect("OnlySpatial pattern guarantees outer_data is present") + .time_info + .clone() + .get_duration() as u64; + end_timestamp - (duration_secs * 1000) + } } } @@ -208,8 +216,24 @@ impl SimpleEngine { query_pattern_type: QueryPatternType, match_result: &SQLQuery, ) -> QueryTimestamps { + // Same duration source `calculate_start_timestamp_sql` uses below: the inner + // (temporal) subquery for nested queries, the single subquery otherwise. + let duration = match query_pattern_type { + QueryPatternType::OneTemporalOneSpatial => match_result + .inner_data() + .expect("OneTemporalOneSpatial pattern guarantees inner_data is present") + .time_info + .get_duration(), + _ => match_result + .outer_data() + .expect("pattern match guarantees outer_data is present") + .time_info + .get_duration(), + } as u64; + let is_single_interval = duration * 1000 == self.data_ingestion_interval_ms; + let mut end_timestamp = query_time; - end_timestamp = self.validate_and_align_end_timestamp(end_timestamp, query_pattern_type); + end_timestamp = self.validate_and_align_end_timestamp(end_timestamp, is_single_interval); let start_timestamp = self.calculate_start_timestamp_sql(end_timestamp, query_pattern_type, match_result); @@ -417,31 +441,28 @@ impl SimpleEngine { // every successful path below. let post = SqlPostProcessing::from_query_data(&query_data); - // Handle SpatioTemporal queries separately - they bypass QueryPatternType mapping - if match_result.query_type == vec![QueryType::SpatioTemporal] { - let query_time = Self::convert_query_time_to_data_time( - query_data.time_info.get_start() + query_data.time_info.get_duration(), - ); - let ctx = self.build_spatiotemporal_context(&match_result, query_time, &query_data)?; - return Some((ctx, post)); - } - let query_pattern_type = match &match_result.query_type[..] { [x] => match x { QueryType::Spatial => QueryPatternType::OnlySpatial, QueryType::TemporalGeneric => QueryPatternType::OnlyTemporal, QueryType::TemporalQuantile => QueryPatternType::OnlyTemporal, - QueryType::SpatioTemporal => unreachable!("SpatioTemporal handled above"), - }, - [x, y] => match (x, y) { - (QueryType::Spatial, QueryType::TemporalGeneric) => { - QueryPatternType::OneTemporalOneSpatial - } - (QueryType::Spatial, QueryType::TemporalQuantile) => { - QueryPatternType::OneTemporalOneSpatial - } - _ => return None, + // Spans multiple scrape intervals but GROUP BY only a subset of labels. + // Every site below keys off query_type.len() (nesting) or duration, not + // this specific tag, so this shares the OnlyTemporal treatment rather + // than a separate function — see build_query_requirements_sql's + // data_range_ms, which is why OnlyTemporal (reads the real duration) + // and not OnlySpatial (hardcodes the ingestion interval) is the right + // bucket to reuse. + QueryType::SpatioTemporal => QueryPatternType::OnlyTemporal, }, + // Nested (spatial-of-temporal) query: the outer layer's own tag no longer has + // to be `Spatial` — the matcher now classifies it by its own labels just like + // any other subquery (#487) — so only the inner (temporal) layer's shape is + // checked here, exactly as before: it must be a full-label-coverage temporal + // read, not itself `SpatioTemporal`. + [_, QueryType::TemporalGeneric | QueryType::TemporalQuantile] => { + QueryPatternType::OneTemporalOneSpatial + } _ => return None, }; @@ -673,95 +694,6 @@ impl SimpleEngine { aggregated_labels, }) } - - /// Build execution context for SpatioTemporal queries. - /// These queries span multiple scrape intervals but GROUP BY a subset of labels. - fn build_spatiotemporal_context( - &self, - match_result: &SQLQuery, - query_time: u64, - query_data: &SQLQueryData, - ) -> Option { - // Output labels are the GROUP BY columns (subset of all labels) - let query_output_labels = KeyByLabelNames::new( - match_result - .outer_data()? - .labels - .clone() - .into_iter() - .collect(), - ); - - // Get the statistic from the aggregation - let statistic_name = match_result - .outer_data()? - .aggregation_info - .get_name() - .to_lowercase(); - - let statistic_to_compute = Self::parse_single_statistic(&statistic_name)?; - - let query_kwargs = self - .build_query_kwargs_sql(&statistic_to_compute, match_result) - .map_err(|e| { - warn!("{}", e); - e - }) - .ok()?; - - let metadata = QueryMetadata { - query_output_labels: query_output_labels.clone(), - statistic_to_compute, - query_kwargs: query_kwargs.clone(), - }; - - // Calculate timestamps - similar to OnlyTemporal - let end_timestamp = - self.validate_and_align_end_timestamp(query_time, QueryPatternType::OnlyTemporal); - let duration_secs = match_result.outer_data()?.time_info.get_duration() as u64; - let start_timestamp = end_timestamp - (duration_secs * 1000); - - let timestamps = QueryTimestamps { - start_timestamp, - end_timestamp, - }; - - // Resolve aggregation: try pre-configured query_configs first, fall back to capability matching. - let agg_info: AggregationIdInfo = if let Some(config) = - self.find_query_config_sql(query_data) - { - self.get_aggregation_id_info(&config) - .map_err(|e| { - warn!("{}", e); - e - }) - .ok()? - } else { - warn!( - "No query_config entry for SQL spatio-temporal query. Attempting capability-based matching." - ); - let requirements = self.build_query_requirements_sql( - match_result, - QueryPatternType::OnlyTemporal, - None, - ); - self.streaming_config - .read() - .unwrap() - .clone() - .find_compatible_aggregation(&requirements)? - }; - let metric = &match_result.outer_data()?.metric; - - self.build_sql_execution_context_tail( - metric, - ×tamps, - metadata, - agg_info, - String::new(), - query_time, - ) - } } #[cfg(test)] diff --git a/asap-query-engine/src/tests/query_equivalence_tests.rs b/asap-query-engine/src/tests/query_equivalence_tests.rs index bd9cbec..a3e6460 100644 --- a/asap-query-engine/src/tests/query_equivalence_tests.rs +++ b/asap-query-engine/src/tests/query_equivalence_tests.rs @@ -183,6 +183,52 @@ mod tests { assert_execution_context_equivalent(&promql_context, &sql_context, "spatial_avg"); } + /// Part of #487's spatial/temporal unification: end-timestamp alignment to the nearest + /// ingestion interval now triggers whenever the *resolved* PromQL range equals one + /// interval, not only when there's no explicit range vector at all. An explicit + /// `sum_over_time(metric[1s])` at a 1s scrape interval is exactly that shape, so an + /// unaligned query time must still get floored to the interval boundary — same as the + /// old bare-`sum(metric)` (implicit range) case already did. + #[test] + fn test_temporal_query_with_interval_length_range_aligns_end_timestamp() { + let scrape_interval_ms = 1000; + let promql_query = "sum_over_time(cpu_usage[1s])"; + let sql_query = "SELECT SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1, L2, L3, L4"; + + let (promql_config, _, streaming_config) = TestConfigBuilder::new("cpu_usage") + .with_grouping_labels(vec!["L1", "L2", "L3", "L4"]) + .with_scrape_interval_ms(scrape_interval_ms) + .add_temporal_query(promql_query, sql_query, 1, 1000, WindowType::Tumbling) + .build_both(); + + let promql_engine = SimpleEngine::new( + Arc::new(NoOpStore), + promql_config, + streaming_config, + scrape_interval_ms, + QueryLanguage::promql, + ); + + // 1_000_500ms — not a multiple of the 1000ms scrape interval. + let query_time_sec: f64 = 1_000.5; + + let promql_context = promql_engine + .build_query_execution_context_promql(promql_query.to_string(), query_time_sec) + .expect("Failed to build PromQL context"); + + let end_ms = promql_context.store_plan.values_query.end_timestamp; + let start_ms = promql_context.store_plan.values_query.start_timestamp; + + assert_eq!( + end_ms, 1_000_000, + "end_timestamp should align down to the nearest 1000ms interval boundary" + ); + assert_eq!( + start_ms, 999_000, + "start_timestamp should be one interval before the aligned end_timestamp" + ); + } + /// Regression test for issue #202. /// With scrape_interval=15s, a 150s SQL temporal query must produce a 150_000ms window. /// Bug: start = end - (150 * 15 * 1000) = end - 2_250_000ms (15× too wide). diff --git a/asap-query-engine/src/tests/sql_pattern_matching_tests.rs b/asap-query-engine/src/tests/sql_pattern_matching_tests.rs index 4c9f579..7bcfa41 100644 --- a/asap-query-engine/src/tests/sql_pattern_matching_tests.rs +++ b/asap-query-engine/src/tests/sql_pattern_matching_tests.rs @@ -128,6 +128,82 @@ mod tests { ); } + /// Characterization test for #487 Step 3: pins today's `build_spatiotemporal_context` + /// output (labels, statistic, window) so the upcoming dispatch merge (folding + /// `SpatioTemporal` into the shared non-nested path) can be checked against it — + /// this assertion must still pass, unchanged, after the merge. + #[test] + fn spatiotemporal_query_context_is_stable_before_dispatch_merge() { + let template = "SELECT SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1"; + let engine = build_sql_engine(template, 1, 10_000); + + let incoming = "SELECT SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, '2025-10-01 00:00:10') AND '2025-10-01 00:00:10' GROUP BY L1"; + let query_time = 1727740810.0_f64; + + let context = engine + .build_query_execution_context_sql(incoming.to_string(), query_time) + .expect("SpatioTemporal query should build a context"); + + assert_eq!(context.metadata.statistic_to_compute, Statistic::Sum); + assert_eq!( + context.metadata.query_output_labels, + KeyByLabelNames::new(vec!["L1".to_string()]) + ); + let start = context.store_plan.values_query.start_timestamp; + let end = context.store_plan.values_query.end_timestamp; + assert_eq!( + end - start, + 10_000, + "window should span the full 10s query duration" + ); + } + + /// #487 Step 3: a SpatioTemporal query (duration > 1 interval, partial GROUP BY) + /// combined with ORDER BY/LIMIT should detect top-k the same way OnlyTemporal/ + /// OnlySpatial already do. Red today — `build_spatiotemporal_context` never calls + /// `detect_sql_topk` — must go green once the dispatch merge lands. + #[test] + fn spatiotemporal_query_with_order_by_limit_detects_topk() { + let template = "SELECT L1, SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() GROUP BY L1"; + let engine = build_sql_engine(template, 1, 10_000); + + let incoming = "SELECT L1, SUM(value) AS total FROM cpu_usage WHERE time BETWEEN DATEADD(s, -10, '2025-10-01 00:00:10') AND '2025-10-01 00:00:10' GROUP BY L1 ORDER BY total DESC LIMIT 10"; + let query_time = 1727740810.0_f64; + + let context = engine + .build_query_execution_context_sql(incoming.to_string(), query_time) + .expect("SpatioTemporal query with ORDER BY/LIMIT should build a context"); + + assert_eq!( + context.metadata.statistic_to_compute, + Statistic::Topk, + "SpatioTemporal queries should detect top-k just like OnlyTemporal/OnlySpatial do" + ); + } + + /// #487 Step 4: same shape as the test above, but at exactly one scrape interval — + /// the case the matcher fold exists for (previously tagged `Spatial`, now + /// `SpatioTemporal` like any other partial-GROUP-BY query). Confirms parity with + /// the long-duration case Step 3 already fixed. + #[test] + fn spatiotemporal_query_with_order_by_limit_detects_topk_at_one_interval() { + let template = "SELECT L1, SUM(value) FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW() GROUP BY L1"; + let engine = build_sql_engine(template, 1, 1_000); + + let incoming = "SELECT L1, SUM(value) AS total FROM cpu_usage WHERE time BETWEEN DATEADD(s, -1, '2025-10-01 00:00:10') AND '2025-10-01 00:00:10' GROUP BY L1 ORDER BY total DESC LIMIT 10"; + let query_time = 1727740810.0_f64; + + let context = engine + .build_query_execution_context_sql(incoming.to_string(), query_time) + .expect("SpatioTemporal query with ORDER BY/LIMIT should build a context"); + + assert_eq!( + context.metadata.statistic_to_compute, + Statistic::Topk, + "1-interval partial-GROUP-BY queries should detect top-k the same way longer-duration ones do" + ); + } + #[test] fn test_spatial_query_matches_now_template() { // Spatial: window equals the scrape interval (1s), GROUP BY all labels diff --git a/asap-tools/experiments/experiment_run_clickhouse.py b/asap-tools/experiments/experiment_run_clickhouse.py index 7bdd370..0f4883a 100644 --- a/asap-tools/experiments/experiment_run_clickhouse.py +++ b/asap-tools/experiments/experiment_run_clickhouse.py @@ -442,7 +442,7 @@ def main(cfg: DictConfig) -> None: query_engine_service.wait_until_ready() steady_state_wait = int(cfg.flow.steady_state_wait) - print(f"Waiting {steady_state_wait}s for precompute ingest to complete...") + print(f"Waiting for {steady_state_wait}s for system to stablize") time.sleep(steady_state_wait) controller_client_config = os.path.join(