test(query-engine): add differential PromQL testing infra (#590, #594)

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

@@ -1834,6 +1834,95 @@ impl SimpleEngine {
     }
 }
 
+/// #590: keyed sliding-window oracle used as the reference model by the
+/// property-style test in `crate::tests::sliding_window_keyed_oracle_tests`.
+/// Pure function -- no engine/store involved.
+///
+/// A sibling to `range_query_tests::simulate_sliding_window` /
+/// `simulate_sliding_window_with_alignment` (same file, below) rather than an
+/// extension of either: those two operate on a single flat `Vec<Box<dyn
+/// AggregateCore>>` and are exercised by ~8 existing call sites via
+/// bucket-index slicing (`buckets[bucket_index..]`) that has no notion of a
+/// key at all. Bolting a key dimension onto that signature would mean either
+/// changing it (breaking every existing call site) or threading an
+/// `Option<KeyByLabelValues>` through index-based slicing that was designed
+/// for one series. A new function keeps those call sites untouched and lets
+/// the keyed model use the simpler representation its own callers actually
+/// have: raw per-key pane values, not `AggregateCore` trait objects (the
+/// property test only exercises `SumAccumulator`, so plain `f64` sums are
+/// sufficient and keep the oracle trivially auditable by hand).
+///
+/// Mirrors the real lookup path exactly: `execute_range_query_pipeline`'s
+/// `single_window` (used for `WindowType::Sliding`, see `mod.rs` above) does
+/// one lookup at `bucket_map[current_time - window_size_ms]`, trusting that
+/// position to already be the fully pre-merged window --
+/// `create_engine_multi_timestamp_with_window` (the test factory used to
+/// drive the real engine) only inserts that pre-merged window into the store
+/// when *every one* of its `window_size_ms / slide_interval_ms` panes is
+/// present; a window missing even one pane is never inserted at all. This
+/// oracle reproduces that same all-or-nothing rule directly over raw panes,
+/// independently of the factory's insertion code, so the property test is
+/// actually checking the engine against a second, independently-written
+/// model rather than against the factory's own logic reflected back at it.
+///
+/// `panes_by_key`: for each key, the list of `(pane_end_timestamp_ms,
+/// sum_value)` pairs that exist for that key. Every pane is
+/// `slide_interval_ms` wide (the real store's Sliding-window layout has no
+/// narrower unit); a key with no entry at a given pane timestamp is treated
+/// as absent there, not zero -- this is how "key disappears mid-range" and
+/// "key appears mid-range" are expressed.
+///
+/// Returns, per key, the `(output_timestamp_ms, merged_value)` pairs a real
+/// `sum_over_time` Sliding-window range query should produce for that key.
+/// A key with zero qualifying output steps is omitted from the map entirely
+/// (never present with an empty `Vec`), matching how a real range query
+/// never emits a zero-sample series for a key.
+#[cfg(test)]
+pub(crate) fn simulate_sliding_window_keyed(
+    panes_by_key: &HashMap<Option<Vec<String>>, Vec<(u64, f64)>>,
+    window_size_ms: u64,
+    slide_interval_ms: u64,
+    start_ms: u64,
+    end_ms: u64,
+    step_ms: u64,
+) -> HashMap<Option<Vec<String>>, Vec<(u64, f64)>> {
+    assert!(slide_interval_ms > 0, "slide_interval_ms must be nonzero");
+    assert!(
+        step_ms > 0,
+        "step_ms must be nonzero, or the output loop never terminates"
+    );
+    assert!(
+        window_size_ms.is_multiple_of(slide_interval_ms),
+        "window_size_ms ({window_size_ms}) must be a whole number of \
+         slide_interval_ms ({slide_interval_ms}) panes"
+    );
+    let num_panes = window_size_ms / slide_interval_ms;
+
+    let mut result = HashMap::new();
+    for (key, panes) in panes_by_key {
+        let pane_map: HashMap<u64, f64> = panes.iter().cloned().collect();
+        let mut samples = Vec::new();
+        let mut current_time = start_ms;
+        while current_time <= end_ms {
+            if current_time >= window_size_ms {
+                let window_start = current_time - window_size_ms;
+                let pane_ends: Vec<u64> = (1..=num_panes)
+                    .map(|i| window_start + i * slide_interval_ms)
+                    .collect();
+                if pane_ends.iter().all(|t| pane_map.contains_key(t)) {
+                    let merged: f64 = pane_ends.iter().map(|t| pane_map[t]).sum();
+                    samples.push((current_time, merged));
+                }
+            }
+            current_time += step_ms;
+        }
+        if !samples.is_empty() {
+            result.insert(key.clone(), samples);
+        }
+    }
+    result
+}
+
 #[cfg(test)]
 mod range_query_tests {
     use crate::data_model::{AggregateCore, AggregationType, KeyByLabelValues, SerializableToSink};
@@ -3421,3 +3510,158 @@ mod sketch_query_tests {
     //     assert!(result.is_none());
     // }
 }
+
+/// Direct unit tests for `validate_range_query_params`'s three error
+/// branches plus its happy path (issue #590, "Boundary/validation tests").
+///
+/// This function is private and has no public wrapper, so it can only be
+/// called directly from a test module that is a descendant of this one in
+/// the module tree (Rust privacy is module-tree scoped) -- hence this module
+/// lives here rather than in `crate::tests`, alongside `range_query_tests`
+/// and `merge_accumulators_regression_tests_596` above, which do the same
+/// for their own private targets.
+///
+/// `crate::tests::range_query_validation_tests` covers the complementary
+/// end-to-end angle: proving through the public `handle_range_query_promql`
+/// entry point that each of these rejections is actually enforced in
+/// practice (that file can't assert the exact error *string*, since
+/// `finish_range_context` in `promql.rs` discards it -- `Result<(), String>`
+/// is folded into `Option` via `.ok()?` -- so the string-level assertions
+/// belong here instead).
+#[cfg(test)]
+mod validate_range_query_params_tests {
+    use crate::data_model::{CleanupPolicy, InferenceConfig, QueryLanguage, StreamingConfig};
+    use crate::engines::simple_engine::SimpleEngine;
+    use crate::stores::{Store, TimestampedBucketsMap};
+    use crate::AggregateCore;
+    use std::collections::HashMap;
+    use std::sync::Arc;
+
+    /// `validate_range_query_params` doesn't touch `self` or the store at
+    /// all -- it's a pure function of its four arguments -- so this store is
+    /// never actually called; it only exists because the method is `&self`.
+    struct NoOpStore;
+
+    impl Store for NoOpStore {
+        fn insert_precomputed_output(
+            &self,
+            _: crate::data_model::PrecomputedOutput,
+            _: Box<dyn AggregateCore>,
+        ) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
+            panic!("NoOpStore should not be called by validate_range_query_params tests");
+        }
+        fn insert_precomputed_output_batch(
+            &self,
+            _: Vec<(crate::data_model::PrecomputedOutput, Box<dyn AggregateCore>)>,
+        ) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
+            panic!("NoOpStore should not be called by validate_range_query_params tests");
+        }
+        fn query_precomputed_output(
+            &self,
+            _: &str,
+            _: u64,
+            _: u64,
+            _: u64,
+        ) -> Result<TimestampedBucketsMap, Box<dyn std::error::Error + Send + Sync>> {
+            panic!("NoOpStore should not be called by validate_range_query_params tests");
+        }
+        fn query_precomputed_output_exact(
+            &self,
+            _: &str,
+            _: u64,
+            _: u64,
+            _: u64,
+        ) -> Result<TimestampedBucketsMap, Box<dyn std::error::Error + Send + Sync>> {
+            panic!("NoOpStore should not be called by validate_range_query_params tests");
+        }
+        fn query_precomputed_output_exact_batch(
+            &self,
+            _: &str,
+            _: u64,
+            _: &[crate::stores::TimestampRange],
+        ) -> Result<TimestampedBucketsMap, Box<dyn std::error::Error + Send + Sync>> {
+            panic!("NoOpStore should not be called by validate_range_query_params tests");
+        }
+        fn get_earliest_timestamp_per_aggregation_id(
+            &self,
+        ) -> Result<HashMap<u64, u64>, Box<dyn std::error::Error + Send + Sync>> {
+            Ok(HashMap::new())
+        }
+        fn close(&self) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
+            Ok(())
+        }
+    }
+
+    fn test_engine() -> SimpleEngine {
+        let inference_config =
+            InferenceConfig::new(QueryLanguage::promql, CleanupPolicy::NoCleanup);
+        let streaming_config = Arc::new(StreamingConfig::default());
+        SimpleEngine::new(
+            Arc::new(NoOpStore),
+            inference_config,
+            streaming_config,
+            15,
+            QueryLanguage::promql,
+        )
+    }
+
+    #[test]
+    fn accepts_valid_params() {
+        let engine = test_engine();
+        assert_eq!(
+            engine.validate_range_query_params(0, 1000, 1000, 1000),
+            Ok(())
+        );
+    }
+
+    #[test]
+    fn rejects_start_after_end() {
+        let engine = test_engine();
+        assert_eq!(
+            engine.validate_range_query_params(1000, 0, 1000, 1000),
+            Err("start must be before end".to_string())
+        );
+    }
+
+    #[test]
+    fn rejects_start_equal_to_end() {
+        // The boundary case specifically called out by #590: `start >= end`
+        // must reject `start == end` too, not just `start > end`.
+        let engine = test_engine();
+        assert_eq!(
+            engine.validate_range_query_params(1000, 1000, 1000, 1000),
+            Err("start must be before end".to_string())
+        );
+    }
+
+    #[test]
+    fn rejects_zero_step() {
+        let engine = test_engine();
+        assert_eq!(
+            engine.validate_range_query_params(0, 1000, 0, 1000),
+            Err("step must be positive".to_string())
+        );
+    }
+
+    #[test]
+    fn rejects_step_not_a_multiple_of_tumbling_window() {
+        let engine = test_engine();
+        assert_eq!(
+            engine.validate_range_query_params(0, 1000, 1500, 1000),
+            Err("step (1500 ms) must be a multiple of tumbling window size (1000 ms)".to_string())
+        );
+    }
+
+    #[test]
+    fn start_after_end_is_checked_before_step_checks() {
+        // Documents the actual branch order (start>=end first, then
+        // step==0, then step-multiple) so a future reordering that changes
+        // which error wins on a doubly-invalid call is a deliberate,
+        // reviewed choice rather than an accident.
+        let engine = test_engine();
+        assert_eq!(
+            engine.validate_range_query_params(1000, 0, 0, 1000),
+            Err("start must be before end".to_string())
+        );
+    }
+}

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

@@ -10,7 +10,10 @@ pub mod native_pipeline_merge_tests;
 pub mod native_range_query_tests;
 pub mod prometheus_forwarding_tests;
 pub mod query_equivalence_tests;
+pub mod range_multistep_instant_equivalence_tests;
 pub mod range_query_arithmetic_tests;
+pub mod range_query_validation_tests;
+pub mod sliding_window_keyed_oracle_tests;
 pub mod sql_pattern_matching_tests;
 pub mod store_correctness_tests;
 pub mod structural_matching_tests;