Skip to content

Converge grouped *_over_time onto positional Aggregate.by (residual of review D) #8

Description

@zzylol

Context

PR #5 (review D) converged the dominant PromQL grouped-aggregate patterns onto a positional Aggregate.by — the same shape SQL GROUP BY produces:

  • sum by(job)(m) (instant vector) → Aggregate { by: [job], [Sum] }
  • sum by(job)(rate(m[5m]))Aggregate { by: [job], [Sum], child: Aggregate{[Rate]} } (the inner rate is modeled as a per-series label-preserving reduction)

This removed the name-based Partition-as-grouping shape (and the "documented droppable L5 hint, yet load-bearing" hazard) for those cases.

What is left

Grouped window reductionssum by(x)(avg_over_time(m[5m])), max by(x)(quantile_over_time(...)), topk by(x)(avg_over_time(...)), etc. — still fall back to the legacy name-based Partition wrapper. Two coupled reasons:

  1. Shared AggIntent variants. avg_over_time (per-series) and cross-series avg both lower to AggIntent::Avg; sum_over_time and sum both to AggIntent::Sum. So per-series-ness can't be read off the intent variant — only off structure (the enclosing time Window).
  2. Window hoisting. The converter's canonical-shape logic hoists the *_over_time Window above the outer cross-series aggregate, separating the Window from its Aggregate — so a "Window-over-Aggregate ⇒ label-preserving" rule in schema derivation doesn't see them adjacent. Folding keys into the per-series by naively also breaks downstream sample-value resolution (e.g. topk by(h)(avg_over_time(…)) — fixed in PR feat(promql+sql): unified positional L3 intent algebra (PromQL & SQL lowering) #5 by gating convergence to non-windowed reductions).

Acceptance

  • sum by(x)(avg_over_time(m[5m])) → positional Aggregate { by: [x], [Avg], … } (no Partition), with x resolved against a label-preserving per-series window reduction.
  • The same for *_over_time under topk/bottomk/count.
  • Once all grouped PromQL aggregates are positional, Partition can be reserved strictly for the L5 sharding hint (re-document) and PartitionKeys retired from the grouping path.
  • Full PromQL + SQL suite green; clippy -D warnings + fmt clean.

Likely approach: distinguish per-series window reductions structurally (don't hoist the window above the outer aggregate, or carry an explicit per-series marker) so the inner reduction preserves labels — then the existing converter convergence path applies unchanged.

Follow-up to #5 (feat/promql-l1-l3).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions