Skip to content

fix(promql): lower workloads with ingestion intervals - #417

Open
milindsrivastava1997 wants to merge 8 commits into
mainfrom
408-promql-instant-aggregates-lose-their-evaluation-horizon-and-lower-directly-over-scan
Open

milindsrivastava1997 wants to merge 8 commits into
mainfrom
408-promql-instant-aggregates-lose-their-evaluation-horizon-and-lower-directly-over-scan

Conversation

@milindsrivastava1997

@milindsrivastava1997 milindsrivastava1997 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Why

Issue #408 showed that context-free PromQL lowering loses the source cadence needed to plan instant selectors and materialize summaries.

What

  • Add DataWorkload.data_ingestion_interval: Evidence<DurationMs>.
  • Require every PromQL workload to provide a nonzero interval.
  • Replace standalone lowering APIs with all-or-nothing workload APIs: lower_promql_workload and lower_promql_workload_with_histograms.
  • Inject TimeRange(data_ingestion_interval) for every bare instant selector.
  • Preserve explicit range selectors unchanged.
  • Migrate planner tests, devtools, examples, and user documentation to explicit workloads.

How

lower_promql_workload validates the workload, lowers normalized workload.entries() for batch and repeating queries, and passes the declared interval to the PromQL lowerer. Validation failures remain PromqlErrors through PromqlError::InvalidWorkload.

The histogram-aware API keeps one supplied catalog scoped to the whole workload. The raw PromqlLowerer and old standalone helpers are no longer public escape hatches.

Before this PR

sum(data) lowered as:

Aggregate { … }
└── Scan(data)

The materializer had no selection horizon and rejected the raw scan input.

After this PR

With data_ingestion_interval = 1s, sum(data) lowers as:

Aggregate { … }
└── TimeRange { range: 1s }
    └── Scan(data)

sum_over_time(data[5m]) retains only its explicit five-minute range. A bare selector with offset 5m is bounded around the shifted evaluation instant.

Devtools that lower PromQL now require --data-ingestion-interval-ms; examples and tests pass an explicit 1-second value.

Compatibility with maintained populations

The current-series maintained-population replacement recognizes simple instant inputs such as sum(a) and topk(5, a). It previously recognized a direct Scan(a). After this PR, the same input is TimeRange(interval) → Scan(a).

The strategy and its execution-data-state validation now look through this selector-horizon wrapper when identifying the current-series population, but retain it in the actual maintained input. A shifted selector remains distinct: TimeRange(interval) → TimeShift → Scan(a) is still rejected.

The test-only workload fixture used by the affected planner modules is centralized in one #[cfg(test)] helper.

Bug fixed during implementation

The frontend previously treated a direct TimeRange child as a signal to choose Aggregate.reduction = PerEntity. #408 injects TimeRange(data_ingestion_interval) for ordinary instant selectors, so that shortcut incorrectly changed sum(data) from a cross-series Reduce([]) into a per-series aggregate.

The fix chooses the existing aggregate reduction from PromQL operation semantics before attaching the injected selector horizon:

sum(data)                 → Reduce([]) over TimeRange(interval) → Scan
sum_over_time(data[5m])   → PerEntity over TimeRange(5m) → Scan

TimeRange now represents temporal input scope only. In particular, range functions over subqueries explicitly select per-series reduction because an intent such as max is cross-series in max(v) but per-series in max_over_time(v[...]).

Grilling Q&A

  1. Question: Should lowering add LastOverTime before the cross-series aggregate?

    Answer: No. "we can assume that promql series are getting a sample every scrape interval".

  2. Question: Should every plan-ready PromQL workload require data_ingestion_interval, including queries with explicit ranges such as rate(data[5m])?

    Answer: Yes.

  3. Question: Should the interval retain provenance/freshness metadata like other data facts, as DataWorkload.data_ingestion_interval: Evidence<DurationMs> with a required nonzero value?

    Answer: Yes.

  4. Question: Should we replace both existing public helpers with one authoritative lower_promql_workload(&QueryWorkload) that covers normalized batch and repeating entries?

    Answer: Yes.

  5. Question: Is a nested Result too complex? There is already QueryWorkload::validate(), so should it own the required-interval validation and should workload lowering be all-or-nothing?

    Answer: "yes, this makes sense."

  6. Question: What happens to the existing PromqlError?

    Answer: Keep it as the sole error type and add an invalid-workload wrapper.

  7. Question: Should interval injection apply to every bare instant selector, rather than only aggregate inputs?

    Answer: "injection -- yeah cool every bare instant selector"

  8. Question: Does this mean that, irrespective of offset, an instant selector gets data_ingestion_interval and a range selector does not? Does offset change whether the time range is at now() or now() - offset?

    Answer: "sorry, yes range selectors. So irrespective of whether offset of not, instant selector gets the data_ingestion_interval and range selector does not. the offset changes where the time range starts -- where at now() or now() - offset"

  9. Question: Should the old public raw lowering helpers be removed immediately, with callers migrated to workload lowering?

    Answer: Yes.

Verification

  • cargo test --workspace --locked
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
  • GitHub Actions: PR-title, format-and-lint, and full workspace test checks pass.

Follow-up

ASAPQuery-backend must migrate its callers to the workload API and provide the source cadence. That coordinated work is tracked in ASAPQuery-backend#729.

@milindsrivastava1997
milindsrivastava1997 force-pushed the 408-promql-instant-aggregates-lose-their-evaluation-horizon-and-lower-directly-over-scan branch from c431b36 to 751a16d Compare September 14, 2026 14:22
@milindsrivastava1997 milindsrivastava1997 changed the title Lower PromQL workloads with explicit ingestion intervals fix(promql): lower workloads with ingestion intervals Sep 14, 2026
@milindsrivastava1997

Copy link
Copy Markdown
Collaborator Author

@zzylol Pls check the entire PR description. Code is in progress.

@milindsrivastava1997
milindsrivastava1997 marked this pull request as ready for review September 14, 2026 23:05
@milindsrivastava1997

Copy link
Copy Markdown
Collaborator Author

@zzylol code is done

zzylol
zzylol previously approved these changes Sep 18, 2026

@zzylol zzylol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why data ingestion interval is a requirement to lower for PromQL, it may be useful for dataworkload as input to planner in general.

Also, the data ingestion interval (sample scraping interval in prometheus), and the query window range are two concepts, not sure how they are related in this PR.

@milindsrivastava1997

Copy link
Copy Markdown
Collaborator Author

@zzylol See issue 408. To convert PromQL instant selector queries to pre ASAP IR, we need a scrape interval. For range selector queries too, the scrape interval helps guide the possible sliding window choices we can use for precomputation.

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.

PromQL instant aggregates lose their evaluation horizon and lower directly over Scan

2 participants