Disambiguate variable names in asap-query-engine #620

Activity

1. milindsrivastava1997

   self-assigned this

   on Aug 26, 2026
2. milindsrivastava1997 commented on Aug 26, 2026

   

   milindsrivastava1997

   on Aug 26, 2026

   ContributorAuthor

   More actions

   Scoped this out via a grilling session. Finalized scope below.

   In scope for #620 (asap-query-engine only)

   1. Rename tumbling_window_ms / keys_tumbling_window_ms — struct fields on RangeQueryExecutionContext (simple_engine/mod.rs), the locals throughout promql.rs, and the param in validate_range_query_params — to window_slide_ms / keys_window_slide_ms. These never actually held "the tumbling window size"; they held the bucket-grid spacing (slide_interval_ms for Sliding, window_size_ms for Tumbling since Tumbling is defined as slide == size), computed by the existing bucket_step_ms() helper. The old name actively lied about Sliding aggregations.

   2. Rename bucket_step_ms() itself to match, and fix its slide_interval_ms == 0 sentinel ("0 means tumbling") to use the config's explicit window_type: WindowType field instead. AggregationConfig already carries window_type — the == 0 convention was a second, implicit way of asking the same question, free to disagree with the explicit field.

   3. Same fix at the sentinel's actual origin: WindowManager::new (precompute_engine/window_manager.rs:18-21) has the identical 0 => window_size_ms // tumbling window convention, and bucket_step_ms()'s own doc comment says it was written to mirror this. WindowManager::new gets a window_type: WindowType param too.

   4. The fixed constructor validates, it doesn't override: for window_type == Tumbling, it requires slide_interval_ms == window_size_ms rather than silently forcing one to match the other. Silently overriding would just move today's phantom-substitution bug one level down instead of removing it.

   5. Made fallible, not a panic: WindowManager::new returns Result. Its one production caller, get_or_create_group_state in worker.rs (runs on the hot path — once per newly-seen streaming group, not just at startup), already returns Option and already has a "no config for this agg_id → warn + skip + keep the worker alive" path (worker.rs:399-408). A validation failure reuses that same shape instead of crashing the worker.

   6. That existing warning message ("skipping samples for unknown agg_id") gets split into two distinct messages — "unknown agg_id" (benign, e.g. a race with config propagation) vs. "invalid window config for agg_id={}: {err}" (a real bug) — so the two failure modes aren't indistinguishable in logs.

   7. Rather than duplicating the window_type-branch/validation logic independently in bucket_step_ms() (query-serving side) and WindowManager::new (precompute side), both call one canonical method added to AggregationConfig itself (asap_types, already a shared dependency of both asap-query-engine and asap-planner-rs) — something like AggregationConfig::validated_slide_ms(&self) -> Result<u64, String>. One source of truth for "what's the real slide value, and is this config internally consistent," instead of two hand-maintained copies that could drift apart again exactly like this did.

   8. Test fallout: the test helpers that re-derive window_type from the same slide_interval_ms == 0 || == window_size_ms sentinel pattern (worker.rs's #[cfg(test)] mod tests, test_e2e_precompute.rs, e2e_precompute_equivalence.rs) get simplified to pass window_type directly. Every WindowManager::new(x, 0) call site in window_manager.rs's own tests gets updated for the new signature.

   Explicitly out of scope, tracked elsewhere

   • The Sliding-window bracket-substitution correctness bug (sum_over_time(cpu_load[10s]) and [5s] returning identical results against a 5s-configured Sliding aggregation, because create_store_query_plan's Sliding branch in mod.rs:432-440 discards the bracket-derived start timestamp in favor of end_timestamp - window_size_ms) — this is real and reproduces today, but it's already Sliding window execution: support multi-window merge/subtract in query engine #554/Support sliding window candidates in the query engine (execution) #557's territory (Support sliding window candidates in the query engine (execution) #557 is explicitly scoped to fix create_store_query_plan). Posted a note to Support sliding window candidates in the query engine (execution) #557 flagging that the query_configs-pre-bound fast path (promql.rs:1178, get_aggregation_id_info at mod.rs:995) bypasses find_compatible_aggregation()'s exact-match check entirely, so Support sliding window candidates in the query engine (execution) #557's fix needs to reach both entry points, not just the capability-matching one.

   • asap-planner-rs's own naming (t_repeat_ms, t_lookback_ms, data_range_ms threaded through planner/window.rs, elastic_dsl.rs, cleanup.rs, aqe_extractor.rs) has the same disease but is a different crate with its own blast radius — candidate for a follow-up issue, not folded into Disambiguate variable names in asap-query-engine #620.

   • Query-side param naming (lookback_ms, query_time, RangeQueryParams { start, end, step }) — checked these against the actual code; they're already reasonably distinct, no second big rename cluster hiding there. repetition_interval as originally described doesn't exist as a query-engine concept at all — that's t_repeat_ms, planner-only.