Unify spatial and temporal aggregation handling across PromQL, SQL, and Elastic

[milindsrivastava1997]

Background

ASAPQuery was built PromQL-first, which distinguishes "temporal" aggregation (sum_over_time(metric[5m]), explicit range) from "spatial" aggregation (sum by (cpu) (metric), implicit "last scrape interval"). That distinction is encoded today as enums (QueryPatternType, QueryType::Spatial) that don't map cleanly onto SQL or Elastic, where a time range is always explicit — and the implicit defaults each query language applies have diverged, producing at least one concrete bug.

Issues

• fix(planner): Elastic DSL window size falls back to t_repeat_ms instead of actual query duration #485 — Elastic planner window size ignores actual query duration, falls back to t_repeat_ms
• PromQL/SQL spatial aggregations rely on implicit time defaults that diverge across query languages #486 — PromQL/SQL spatial aggregations rely on implicit time defaults that diverge across query languages
• QueryPatternType / QueryType::Spatial enums duplicate branching across 3 crates #487 — QueryPatternType/QueryType::Spatial enums duplicate branching across 3 crates

[milindsrivastava1997]

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)

[milindsrivastava1997]

Closing. Only remaining work is tracked in #485