Skip to content

promql.rs: decouple from QueryPatternType #508

Description

@milindsrivastava1997

Every branch on QueryPatternType in promql.rs (calculate_start_timestamp_promql, extract_quantile_param_promql, extract_topk_param, build_query_kwargs_promql, the label match in build_promql_execution_context_tail) is fully derivable from which tokens are present on PromQLMatchResult (a "function"/"function_args" token vs an "aggregation" token). The enum adds no information beyond what the match result already encodes, but is threaded through 5+ helper method signatures.

Resolution

Scope grew during implementation: rather than inlining token checks at each call site, asap_types::query_requirements::QueryRequirements (metric, statistics, data_range_ms, grouping_labels, spatial_filter_normalized, topk_count_events) became the single canonical derivation — computed once via build_query_requirements_promql immediately after pattern matching, and consumed identically by both asap-query-engine's main path and capability-matching fallback, and by both of asap-planner-rs's independent code paths (SingleQueryProcessor and the AQE extractor). This mirrors how SQL already normalizes every query shape into one struct (SQLQueryData) with zero downstream shape-branching.

Landed as 6 dependency-ordered stages:

  • Stage 0 (correctness fix, independent of the rest): a real bug was found — non-collapsable spatial-of-temporal combinations (e.g. sum(min_over_time(x[5m])); only sum+sum_over_time, sum+count_over_time, min+min_over_time, max+max_over_time are actually collapsable) were structurally accepted by both crates' pattern matchers but silently dropped the outer aggregation, returning wrong (ungrouped) data instead of erroring. Fixed by narrowing the pattern definitions to only the 4 actually-collapsable pairs, so such combinations no longer match any pattern at all.
  • Stage 1: added Statistic::is_approximate(), mirroring the existing PromQLFunction/AggregationOperator methods.
  • Stage 2: promql_utilities::get_statistics_to_compute dropped its pattern_type param.
  • Stage 3: asap_types::build_query_requirements_promql dropped its pattern_type param.
  • Stage 4: asap-query-engine unified onto QueryRequirements; controller_patterns flattened from HashMap<QueryPatternType, _> to Vec<PromQLPattern>; end-timestamp alignment made unconditional (matching SQL's align_end_timestamp_sql exactly, eliminating another shape-dependent branch); the standalone test_offline_precomputes debug binary (no real query/tokens) got its own local, unexported pattern-kind enum.
  • Stage 5: asap-planner-rs unified the same way. Along the way: found and unified a pre-existing divergence between planner and query-engine on modifier-less topk grouping labels; adopted SQL's window-sizing invariant (t_repeat_ms >= data_ingestion_interval_ms, and for genuinely multi-interval queries data_range_ms >= t_repeat_ms) in both set_window_parameters (PromQL) and compute_sql_window (SQL) for consistency — relaxed for single-scrape-interval queries (spatial-only, or a coincidentally narrow temporal query) so an ordinary "refresh less often than the scrape interval" configuration isn't wrongly rejected; get_cleanup_param simplified to take data_range_ms directly; build_patterns() flattened. should_be_performant's OnlyTemporal-only punting gate and quantile/topk kwarg extraction deliberately kept as raw-token checks rather than forced onto QueryRequirements, since collapsing them would have silently changed behavior.
  • Stage 6: deleted QueryPatternType entirely from promql_utilities::query_logics::enums (definition + Display impl) — zero remaining references anywhere in the Rust workspace.

All 6 stages landed independently compiling, tests passing, and clippy-clean (cargo clippy --workspace --all-targets --all-features --locked -- -D warnings, matching CI).

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions