Activity

1. milindsrivastava1997

   added

   asap-planner

   asap-query-engine

   tracking-issue

   on Jun 30, 2026
2. milindsrivastava1997 commented on Jun 30, 2026

   

   milindsrivastava1997

   on Jun 30, 2026

   ContributorAuthor

   More actions

   Spatial/Temporal Aggregation Unification

   Status: Design decided, not yet implemented
   Touches: promql_utilities, asap-planner-rs, asap-query-engine

   ---

   Background

   ASAPQuery was built PromQL-first. PromQL distinguishes:

   • Temporal aggregation: sum_over_time(metric[5m]) — explicit range vector
   • Spatial aggregation: sum by (cpu) (metric) — implicit "last scrape interval"

   This distinction was encoded as QueryPatternType { OnlyTemporal, OnlySpatial, OneTemporalOneSpatial } and caused branching in 8+ places across 3 crates. It also never mapped cleanly to SQL/Elastic, where a time range is always a time range.

   The insight: OnlySpatial is just OnlyTemporal with range = scrape_interval_ms. All three variants are special cases of:

   SELECT agg(col) WHERE time_range AND spatial_filter GROUP BY group_by_cols

   ---

   Decisions

   1. Kill the enums

   • QueryPatternType (in promql_utilities) — deleted entirely
   • QueryType::Spatial (in SQL planner) — deleted entirely

   2. Time ranges are always explicit, filled in at plan time

   • PromQL sum(metric) (no range vector) → planner substitutes scrape_interval_ms
   • SQL spatial query (no time range) → planner substitutes data_ingestion_interval_ms
   • Elastic → fixed to use actual time range (was always using t_repeat_ms — bug)
   • Engine receives only concrete (start_timestamp, end_timestamp), no defaults

   3. do_merge stays explicit in QueryExecutionContext

   • Field stays on the struct for clarity
   • Set by planner using derived check: range_ms > window_size_ms
   • Not set from QueryPatternType anymore

   4. Pattern registry flattened

   • controller_patterns: HashMap<QueryPatternType, Vec<PromQLPattern>> → Vec<PromQLPattern>
   • First match wins; patterns are structurally disjoint so order doesn't affect correctness
   • Also fixes a latent bug: HashMap iteration is non-deterministic

   5. Branch predicate at every former enum site

   Replace match query_pattern_type { ... } with structural inspection of PromQLMatchResult:

   • match_result.get_range_duration().is_some() → has temporal function
   • match_result.get_aggregation_op().is_some() → has spatial aggregation
   • grouping labels (by/without in PromQL, GROUP BY in SQL) extracted directly

   6. Shared promql_utilities functions inspect match result internally

   Functions like get_statistics_to_compute and get_spatial_aggregation_output_labels currently take QueryPatternType as a param. After: they inspect PromQLMatchResult directly and derive what they need. No enum param.

   ---

   What doesn't change

   • QueryExecutionContext struct shape stays the same (spatial_filter, grouping_labels, aggregated_labels were already unified fields)
   • Sketch selection logic (multi-population keyed vs single-population) is driven by whether grouping_labels is non-empty — no change needed there, it already uses the field not the enum

   ---

   Staging: two PRs

   PR 1 — Make all time ranges explicit (behavior change, fixes bugs)

   • PromQL planner: fill in scrape_interval_ms when no range vector present
   • SQL planner: fill in data_ingestion_interval_ms for spatial-style queries
   • Elastic planner: fix bug — use actual query time range instead of always t_repeat_ms
   • do_merge derived from range_ms > window_size_ms instead of pattern type
   • Enum still exists and compiles; all branches become trivially equivalent
   • Independently verifiable: time window correctness can be tested without touching the enum

   PR 2 — Delete the enums (pure cleanup, no behavior change)

   • Delete QueryPatternType from promql_utilities
   • Delete QueryType::Spatial from SQL planner
   • Flatten controller_patterns to Vec<PromQLPattern>
   • Update get_statistics_to_compute, get_spatial_aggregation_output_labels to inspect match result internally
   • Replace all match query_pattern_type sites with structural predicates
   • Easy to review: no behavior should change, compiler enforces completeness

   ---

   TDD tests

   PR 1 — Red tests (fail now, pass after fix)

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

   test_esdsl_time_range_excludes_data_outside_range — stores data at t = now-5s, queries
   gte: now-1s, lte: now, t_repeat_ms=30s. With the bug the 5s-old data is included (30s
   window). After PR 1 the result is empty — data is outside the 1s range. This is the only
   genuinely red test; it targets the Elastic bug directly.

   ---

   PR 2 — Pinning tests (pass now, must still pass after enum is deleted)

   These lock in the correct behavior so PR 2 can be reviewed as a pure mechanical cleanup.

   asap-query-engine/src/tests/query_equivalence_tests.rs — extend existing pattern

   test_spatial_start_timestamp_equals_end_minus_scrape_interval — sum(cpu_usage),
   scrape_interval=15s, query_time=1000s → assert
   context.store_plan.values_query.start_timestamp == 999_985_000.

   test_temporal_start_timestamp_equals_end_minus_range — sum_over_time(cpu_usage[5m]),
   query_time=1000s → assert start_timestamp == 999_700_000.

   test_do_merge_false_for_spatial_context — sum(metric) where window_size == scrape_interval
   → assert context.do_merge == false.

   test_do_merge_true_for_temporal_context — sum_over_time(metric[5m]) where
   range_ms > window_size_ms → assert context.do_merge == true.

   asap-planner-rs/src/planner/window.rs — the OnlySpatial branch has no test today

   spatial_window_size_equals_data_ingestion_interval — set_window_parameters(OnlySpatial, t_repeat=300_000, ingestion=15_000, ...) → assert config.window_size_ms == 15_000.

   temporal_window_size_equals_effective_repeat — set_window_parameters(OnlyTemporal, t_repeat=300_000, step=30_000, ...) → assert config.window_size_ms == 30_000.

   Note: the PR 2 pinning tests will need their QueryPatternType param replaced when the enum
   is deleted, but the numeric assertions stay identical — that's the regression guard.

   ---

   Key files touched

   File	What changes
   promql_utilities/src/query_logics/enums.rs	Delete QueryPatternType (PR 2)
   asap-planner-rs/src/planner/window.rs:51	Spatial window size → derived from explicit range (PR 1)
   asap-planner-rs/src/planner/cleanup.rs:26	Spatial lookback → derived from explicit range (PR 1)
   asap-planner-rs/src/planner/promql.rs	Fill in scrape_interval_ms when no range vector (PR 1); kill enum branches (PR 2)
   asap-planner-rs/src/planner/sql.rs:168	Fill in data_ingestion_interval_ms; kill QueryType::Spatial (PR 1+2)
   asap-planner-rs/src/planner/elastic_dsl.rs	Fix bug: use actual time range (PR 1)
   asap-query-engine/src/engines/simple_engine/mod.rs:140	Flatten controller_patterns (PR 2)
   asap-query-engine/src/engines/simple_engine/promql.rs	Kill all QueryPatternType branches (PR 2)
   asap-query-engine/src/engines/simple_engine/sql.rs	Kill QueryType branch (PR 2)
3. milindsrivastava1997 commented on Aug 28, 2026

   

   milindsrivastava1997

   on Aug 28, 2026

   ContributorAuthor

   More actions

   Closing. Only remaining work is tracked in #485
4. milindsrivastava1997

   closed this as completedon Aug 28, 2026