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 6a293d8..13d1c3b 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 @@ -1217,4 +1217,158 @@ mod tests { Some(QueryError::InvalidValueCol), ); } + + // ── Half-open range: `time >= A AND time < B` ──────────────────────────── + // + // ClickHouse runs `>=`/`<` as a true half-open `[start, end)` scan, which is + // how ASAP selects precompute windows — so a query written this way is + // semantically equivalent on both backends (issue #401). We accept STRICTLY + // `>=` (lower) + `<` (upper); any other operator combination is rejected. + + /// A half-open `>=/<` clause must produce the same `(start, duration)` as the + /// equivalent inclusive-looking `BETWEEN` (downstream window selection is + /// identical because both reduce to the same TimeInfo). + #[test] + fn test_half_open_matches_between_time_info() { + let between = parse_sql_query( + "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, L2, L3, L4", + ) + .expect("BETWEEN query should parse"); + + let half_open = parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, '2025-10-01 00:00:10') AND time < '2025-10-01 00:00:10' \ + GROUP BY L1, L2, L3, L4", + ) + .expect("half-open query should parse"); + + assert_eq!( + half_open.time_info.get_time_col_name(), + between.time_info.get_time_col_name() + ); + assert_eq!( + half_open.time_info.get_start(), + between.time_info.get_start() + ); + assert_eq!( + half_open.time_info.get_duration(), + between.time_info.get_duration() + ); + assert_eq!(half_open.time_info.get_duration(), 10.0); + } + + /// The two comparisons may appear in either order. + #[test] + fn test_half_open_reversed_operand_order() { + let q = parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time < '2025-10-01 00:00:10' AND time >= DATEADD(s, -10, '2025-10-01 00:00:10') \ + GROUP BY L1", + ) + .expect("reversed-order half-open query should parse"); + assert_eq!(q.time_info.get_duration(), 10.0); + } + + /// The time column may sit on the right-hand side of each comparison + /// (`A <= time AND B > time` ≡ `time >= A AND time < B`). + #[test] + fn test_half_open_column_on_right() { + let q = parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE DATEADD(s, -10, '2025-10-01 00:00:10') <= time AND '2025-10-01 00:00:10' > time \ + GROUP BY L1", + ) + .expect("column-on-right half-open query should parse"); + assert_eq!(q.time_info.get_duration(), 10.0); + } + + /// NOW()-relative half-open ranges parse (used by query templates). + #[test] + fn test_half_open_now_relative() { + let q = parse_sql_query( + "SELECT AVG(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -1, NOW()) AND time < NOW() \ + GROUP BY L1", + ) + .expect("NOW()-relative half-open query should parse"); + assert_eq!(q.time_info.get_duration(), 1.0); + } + + /// A half-open query classifies through the matcher exactly like its BETWEEN + /// counterpart (1s span over a label subset ⇒ Spatial). + #[test] + fn test_half_open_pattern_matching() { + check_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -1, NOW()) AND time < NOW() \ + GROUP BY L1", + vec![QueryType::Spatial], + None, + ); + } + + /// A half-open template and a half-open incoming query (absolute timestamps) + /// match via duration-based `TimeInfo::matches_pattern`. + #[test] + fn test_half_open_template_matches_absolute_query() { + let template = parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, NOW()) AND time < NOW() \ + GROUP BY L1, L2, L3, L4", + ) + .unwrap(); + let incoming = parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, '2025-10-01 00:00:10') AND time < '2025-10-01 00:00:10' \ + GROUP BY L1, L2, L3, L4", + ) + .unwrap(); + assert!(incoming.matches_sql_pattern(&template)); + } + + /// Strict rejection: `>` lower bound is not accepted. + #[test] + fn test_half_open_rejects_gt_lower_bound() { + assert!(parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time > DATEADD(s, -10, '2025-10-01 00:00:10') AND time < '2025-10-01 00:00:10' \ + GROUP BY L1", + ) + .is_none()); + } + + /// Strict rejection: `<=` upper bound is not accepted. + #[test] + fn test_half_open_rejects_lte_upper_bound() { + assert!(parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, '2025-10-01 00:00:10') AND time <= '2025-10-01 00:00:10' \ + GROUP BY L1", + ) + .is_none()); + } + + /// Two lower bounds is not a valid range. + #[test] + fn test_half_open_rejects_two_lower_bounds() { + assert!(parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, '2025-10-01 00:00:10') AND time >= '2025-10-01 00:00:10' \ + GROUP BY L1", + ) + .is_none()); + } + + /// Both comparisons must reference the same column. + #[test] + fn test_half_open_rejects_mismatched_columns() { + assert!(parse_sql_query( + "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, '2025-10-01 00:00:10') AND L1 < '2025-10-01 00:00:10' \ + GROUP BY L1", + ) + .is_none()); + } } diff --git a/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_parser.rs b/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_parser.rs index fa3f655..659ac32 100644 --- a/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_parser.rs +++ b/asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_parser.rs @@ -6,6 +6,28 @@ use std::collections::HashSet; use parse_datetime::parse_datetime; use sqlparser::ast::Value::SingleQuotedString; +/// One side of a half-open `time >= A AND time < B` range, carrying its +/// resolved timestamp (seconds). +enum TimeBound { + /// `time >= ts` — inclusive lower bound. + Lower(f64), + /// `time < ts` — exclusive upper bound. + Upper(f64), +} + +/// Mirror a comparison operator for operand swaps (`A <= time` ≡ `time >= A`). +/// Only the operators relevant to half-open time ranges are mirrored; anything +/// else returns `None` and is rejected upstream. +fn mirror_operator(op: &BinaryOperator) -> Option { + match op { + BinaryOperator::Lt => Some(BinaryOperator::Gt), + BinaryOperator::LtEq => Some(BinaryOperator::GtEq), + BinaryOperator::Gt => Some(BinaryOperator::Lt), + BinaryOperator::GtEq => Some(BinaryOperator::LtEq), + _ => None, + } +} + pub struct SQLPatternParser { #[allow(dead_code)] schema: SQLSchema, @@ -523,9 +545,10 @@ impl SQLPatternParser { _ => None, }, - _ => { - panic!("invalid time syntax {:?}", highlow); - } + // Unrecognized time syntax: return None so the query is treated as + // unmatched (and forwarded / reported as unsupported) rather than + // crashing the engine or planner. + _ => None, } } @@ -556,10 +579,88 @@ impl SQLPatternParser { Some(TimeInfo::new(col_name, start, duration)) } + + // Half-open range: `time >= AND time < `. + // + // ClickHouse executes `>=`/`<` as a true half-open `[start, end)` + // scan, which is exactly how ASAP selects precompute windows — so a + // query written this way answers the same question on both backends + // (unlike inclusive `BETWEEN`). We accept STRICTLY `>=` for the lower + // bound and `<` for the upper bound; any other operator combination + // (`>`, `<=`) returns None and is treated as unmatched, to avoid a + // silent off-by-one against the ClickHouse baseline. + Expr::BinaryOp { + left, + op: BinaryOperator::And, + right, + } => self.get_time_info_from_half_open(left, right), + _ => None, } } + /// Parse a `time >= A AND time < B` conjunction into `TimeInfo`. + /// + /// The two comparisons may appear in either order, and the time column may + /// be on either side of each comparison. Both comparisons must reference the + /// same column. Returns `None` unless there is exactly one `>=` lower bound + /// and exactly one `<` upper bound. + fn get_time_info_from_half_open(&self, left: &Expr, right: &Expr) -> Option { + let (lcol, lbound) = self.parse_time_comparison(left)?; + let (rcol, rbound) = self.parse_time_comparison(right)?; + + if lcol != rcol { + return None; + } + + let (start, end) = match (lbound, rbound) { + (TimeBound::Lower(start), TimeBound::Upper(end)) => (start, end), + (TimeBound::Upper(end), TimeBound::Lower(start)) => (start, end), + // Two lower bounds or two upper bounds is not a valid range. + _ => return None, + }; + + let duration = end - start; + + Some(TimeInfo::new(lcol, start, duration)) + } + + /// Parse a single comparison of the form `time >= ` or `time < ` + /// (column on either side) into `(column_name, TimeBound)`. + /// + /// Strictly accepts only `>=` (lower) and `<` (upper); every other operator + /// returns `None`. + fn parse_time_comparison(&self, expr: &Expr) -> Option<(String, TimeBound)> { + let Expr::BinaryOp { left, op, right } = expr else { + return None; + }; + + // Normalize so the column identifier is on the left. If the column is on + // the right instead, flip the operator to its mirror so the bound + // classification below stays correct (e.g. `A <= time` ≡ `time >= A`). + let (col_expr, op, ts_expr) = match (left.as_ref(), right.as_ref()) { + (Expr::Identifier(_), _) => (left.as_ref(), op.clone(), right.as_ref()), + (_, Expr::Identifier(_)) => (right.as_ref(), mirror_operator(op)?, left.as_ref()), + _ => return None, + }; + + let col_name = match col_expr { + Expr::Identifier(ident) => ident.value.clone(), + _ => return None, + }; + + let ts = self.get_timestamp_from_between_highlow(ts_expr)?; + + let bound = match op { + BinaryOperator::GtEq => TimeBound::Lower(ts), + BinaryOperator::Lt => TimeBound::Upper(ts), + // `>` and `<=` are intentionally rejected (see get_time_info). + _ => return None, + }; + + Some((col_name, bound)) + } + fn parse_dateadd(&self, func: &Function) -> Option { let args = match &func.args { FunctionArguments::List(args) => &args.args, diff --git a/asap-planner-rs/tests/sql_integration.rs b/asap-planner-rs/tests/sql_integration.rs index 759c2a9..1b21074 100644 --- a/asap-planner-rs/tests/sql_integration.rs +++ b/asap-planner-rs/tests/sql_integration.rs @@ -285,6 +285,65 @@ fn temporal_quantile() { assert_eq!(out.inference_cleanup_param(q), Some(1)); } +// ── Half-open time range: `time >= A AND time < B` (issue #408) ─────────────── +// +// The planner shares the SQL time-clause parser with the query engine, so the +// half-open form must produce a plan identical to the equivalent `BETWEEN`. +// These tests close the end-to-end loop at the planner layer and guard against +// future parser refactors silently dropping half-open support. + +/// Half-open `time >= DATEADD(s, -300, NOW()) AND time < NOW()` must generate +/// the exact same plan as the `BETWEEN`-based `temporal_quantile` test +/// (DatasketchesKLL, window = 300 s, grouping = [datacenter], +/// rollup = [hostname, region], cleanup = 1). +#[test] +fn temporal_quantile_half_open() { + let q = "SELECT quantile(0.95)(cpu_usage) FROM metrics_table WHERE time >= DATEADD(s, -300, NOW()) AND time < NOW() GROUP BY datacenter"; + let out = SQLController::from_yaml(&one_query_config(q, 300), sql_opts()) + .unwrap() + .generate() + .unwrap(); + + assert_eq!(out.streaming_aggregation_count(), 1); + assert_eq!(out.inference_query_count(), 1); + assert!(out.has_aggregation_type("DatasketchesKLL")); + assert!(!out.has_aggregation_type("DeltaSetAggregator")); + assert!(out.all_tumbling_window_sizes_eq(300)); + assert_eq!( + out.aggregation_table_name("DatasketchesKLL"), + Some("metrics_table".to_string()) + ); + assert_eq!( + out.aggregation_value_column("DatasketchesKLL"), + Some("cpu_usage".to_string()) + ); + let mut rollup = out.aggregation_labels("DatasketchesKLL", "rollup"); + rollup.sort(); + assert_eq!(rollup, vec!["hostname".to_string(), "region".to_string()]); + assert_eq!( + out.aggregation_labels("DatasketchesKLL", "grouping"), + vec!["datacenter".to_string()] + ); + assert_eq!( + out.aggregation_labels("DatasketchesKLL", "aggregated"), + Vec::::new() + ); + assert_eq!(out.inference_cleanup_param(q), Some(1)); +} + +/// Strict operator policy at the planner layer: a `>` lower bound / `<=` upper +/// bound combination is not a recognized half-open range, so the time clause +/// fails to parse and the controller surfaces a `SqlParse` error rather than +/// generating a plan with the wrong (silently half-open) window. +#[test] +fn half_open_gt_lte_returns_sql_parse_error() { + let q = "SELECT quantile(0.95)(cpu_usage) FROM metrics_table WHERE time > DATEADD(s, -300, NOW()) AND time <= NOW() GROUP BY datacenter"; + let result = SQLController::from_yaml(&one_query_config(q, 300), sql_opts()) + .unwrap() + .generate(); + assert!(matches!(result, Err(ControllerError::SqlParse(_)))); +} + // ── Elastic SQL syntax variants ─────────────────────────────────────────────── // // These three tests exercise syntactic forms used by Elastic: diff --git a/asap-query-engine/src/tests/query_equivalence_tests.rs b/asap-query-engine/src/tests/query_equivalence_tests.rs index 86203d3..b9ca4ca 100644 --- a/asap-query-engine/src/tests/query_equivalence_tests.rs +++ b/asap-query-engine/src/tests/query_equivalence_tests.rs @@ -232,6 +232,90 @@ mod tests { ); } + /// Issue #401: a half-open `time >= A AND time < B` SQL query must build the + /// exact same execution context (window timestamps + aggregation) as the + /// equivalent `BETWEEN A AND B` query. ClickHouse runs `>=`/`<` as a true + /// half-open `[A, B)` scan — matching ASAP's window selection — so the two + /// query forms answer the identical question. + #[test] + fn test_half_open_equivalent_to_between() { + let scrape_interval = 1; + let promql_query = "sum_over_time(cpu_usage[10s])"; + let between_query = "SELECT SUM(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"; + let half_open_query = "SELECT SUM(value) FROM cpu_usage \ + WHERE time >= DATEADD(s, -10, '2025-10-01 00:00:00') AND time < '2025-10-01 00:00:00' \ + GROUP BY L1, L2, L3, L4"; + + // The inference config stores the BETWEEN form as the template; the + // half-open incoming query resolves against it (duration-based match). + let (_, sql_config, streaming_config) = TestConfigBuilder::new("cpu_usage") + .with_grouping_labels(vec!["L1", "L2", "L3", "L4"]) + .with_scrape_interval(scrape_interval) + .add_temporal_query(promql_query, between_query, 1, 10, WindowType::Tumbling) + .build_both(); + + let sql_engine = SimpleEngine::new( + Arc::new(NoOpStore), + sql_config, + streaming_config, + scrape_interval, + QueryLanguage::sql, + ); + + // Fixed-date queries ignore query_time_sec (only NOW() consults it). + let between_context = sql_engine + .build_query_execution_context_sql(between_query.to_string(), 0.0) + .expect("Failed to build BETWEEN context"); + let half_open_context = sql_engine + .build_query_execution_context_sql(half_open_query.to_string(), 0.0) + .expect("Failed to build half-open context"); + + assert_execution_context_equivalent( + &between_context, + &half_open_context, + "half_open_vs_between", + ); + } + + /// Strict rejection at the engine boundary: a `>`/`<=` combination is not a + /// recognized half-open range, so context building returns None (the engine + /// then forwards or reports unsupported, rather than silently using a + /// half-open window that wouldn't match the ClickHouse baseline). + #[test] + fn test_gt_lte_combination_not_matched() { + let scrape_interval = 1; + let promql_query = "sum_over_time(cpu_usage[10s])"; + let between_query = "SELECT SUM(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"; + let gt_lte_query = "SELECT SUM(value) FROM cpu_usage \ + WHERE time > DATEADD(s, -10, '2025-10-01 00:00:00') AND time <= '2025-10-01 00:00:00' \ + GROUP BY L1, L2, L3, L4"; + + let (_, sql_config, streaming_config) = TestConfigBuilder::new("cpu_usage") + .with_grouping_labels(vec!["L1", "L2", "L3", "L4"]) + .with_scrape_interval(scrape_interval) + .add_temporal_query(promql_query, between_query, 1, 10, WindowType::Tumbling) + .build_both(); + + let sql_engine = SimpleEngine::new( + Arc::new(NoOpStore), + sql_config, + streaming_config, + scrape_interval, + QueryLanguage::sql, + ); + + assert!( + sql_engine + .build_query_execution_context_sql(gt_lte_query.to_string(), 0.0) + .is_none(), + "`>`/`<=` must not be treated as a half-open range" + ); + } + #[test] fn test_spatial_of_temporal_sum_equivalence() { let scrape_interval = 1;