Skip to content

feat(control_plane): classify AggRole via real AggIntent, not a PromQL-string sniff - #426

Closed
zzylol wants to merge 1 commit into
mainfrom
feat/derive-agg-role-via-aggintent
Closed

zzylol wants to merge 1 commit into
mainfrom
feat/derive-agg-role-via-aggintent

Conversation

@zzylol

@zzylol zzylol commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

derive_agg_role() used to guess a workload entry's AggRole (Quantile/Sum/Count/Topk/Other) by sniffing the leading token of its PromQL query_string — the same duplicate-classifier smell already retired from the live serving path (the old two-analyzer comparison in analyzer-parity-matrix.md, #422). Now parses the query through the same canonical pipeline capability_for()/serving uses (query_parser::parse_query_expr_canonical), runs it through the L3 rule-based optimizer (QueryOptimizer::new(0.0).optimizeTopKFusion is a pure structural rewrite, ignores the cost model, so the placeholder throughput doesn't affect the outcome) so topk(k, m) actually reaches an Aggregate{TopK} node instead of staying Sort+Limit, then classifies by the real outer AggIntent (collect_agg_intents, now pub(crate) for this reuse).

Two things this surfaced:

  • (quantile_over_time(0.9, m[5m])) (redundant wrapping parens) — the old leading-token sniff finds an empty token at a ( and mis-defaults to Sum; real classification is unaffected by surface punctuation. New test pins this.
  • topk_over_time(...) was in the old test's query list but isn't a real function this parser (or vanilla PromQL) recognizes at all — confirmed via grep, nowhere in query_parser/promql.rs or intent_algebra/lower.rs. A workload entry with that query_string would fail to parse anywhere else in the real pipeline too, so the old heuristic classifying it as Topk was itself the bug. Test updated to drop it, with an explanatory comment.

Test plan

  • cargo build --workspace — clean, no new warnings
  • cargo test --release --lib -p control_plane workload:: — 33/33 (32 existing + 1 new)
  • cargo test --release --lib -p control_plane — 727/727 (one pre-existing, unrelated skip as before)

🤖 Generated with Claude Code

…L-string sniff

derive_agg_role() used to guess a workload entry's AggRole (Quantile/
Sum/Count/Topk/Other) by sniffing the leading token of its PromQL
query_string -- the same duplicate-classifier smell already retired
from the live serving path (the old two-analyzer comparison in
analyzer-parity-matrix.md, #422). Now parses the query through the
same canonical pipeline capability_for()/serving uses
(query_parser::parse_query_expr_canonical), runs it through the L3
rule-based optimizer (QueryOptimizer::new(0.0).optimize -- TopKFusion
is a pure structural rewrite, ignores the cost model, so the
placeholder throughput doesn't affect the outcome) so topk(k, m)
actually reaches an Aggregate{TopK} node instead of staying
Sort+Limit, then classifies by the real outer AggIntent
(collect_agg_intents, now pub(crate) for this reuse).

Two things this surfaced:
- `(quantile_over_time(0.9, m[5m]))` (redundant wrapping parens) --
  the old leading-token sniff finds an empty token at a `(` and
  mis-defaults to Sum; real classification is unaffected by surface
  punctuation. New test pins this.
- `topk_over_time(...)` was in the old test's query list but isn't a
  real function this parser (or vanilla PromQL) recognizes at all --
  confirmed via grep, nowhere in query_parser/promql.rs or
  intent_algebra/lower.rs. A workload entry with that query_string
  would fail to parse anywhere else in the real pipeline too, so the
  old heuristic classifying it as Topk was itself the bug. Test
  updated to drop it, with an explanatory comment.

cargo build --workspace clean; control_plane 727/727 (one
pre-existing, unrelated skip as before), workload:: 33/33 (32 + 1 new
test).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol

zzylol commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #429 (rebased onto main after #428 changed parse_query_expr_canonical's signature, plus a fix for the histogram_quantile classification fallout).

@zzylol zzylol closed this Jul 29, 2026
@zzylol
zzylol deleted the feat/derive-agg-role-via-aggintent branch July 29, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant