Conversation
This was referenced Oct 3, 2026
zzylol
marked this pull request as draft
October 3, 2026 19:29
Contributor
Author
|
Parked as draft: PR priorities changed (see #528). Order is now (A) finish #511 operator sharing, (B) the #572 crate/module reorganization, (C) #509 end-to-end stages. This PR sits on the old 🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: native compilation of a per-series pane merge drops the timestamp
#509 §Example 4 (Materialization of window summaries in physical planning), Pattern B, answers
quantile_over_time(0.99, latency_ms[5m])every minute from 1-min tumbling KLLs:#509 §Physical operator implementation says a KLL node becomes "summary build, merge and quantile estimation operators", and §4 Execution says the deployment "runs the selected plan as given". So every pane plan that #566 generates and #568 can select must compile and run natively.
That fails when the panes are per-series (PromQL). A per-series state carries a timestamp column (
time_indexis set). The native binding ofSummaryMerge(bind_operation) groups by every column except the state and the time index:So the merged native output has no timestamp column, while the Planner output schema still has one.
with_output_schemacompares the two and fails:The timestamp cannot just be kept as a group key either. Each retained pane carries its own build timestamp (e.g. 00:01, 00:02, … 00:05). Grouping by it would keep five rows instead of merging them, and picking one of them would give the answer a pane's build time instead of the query's evaluation time.
Scope: this PR fixes native compilation of temporal
SummaryMergefor both B1 (merge retained panes) and B2 (rebuild panes at query time). It does not add a rotating pane cache, historical EH buckets, or any new planning rule.Proposed method
The fix is in physical compilation (
compile_internalin the native physical planner). When a node isSummaryMergeand its Planner output has atime_index, it lowers to two native operators instead of one:bind_operationbuilds the usualsummary_merge, which groups by all non-state, non-time columns. Its output schema (compact) has no timestamp. It is added under a helper id,helper_id(id, 1). With more than one input, the existing code already inserts aunionin front, so the merge reads all panes at once.Operator::scope_timestamp(compact, output)maps everycompactcolumn to the Planner output schema and fills the time column from the run scope (the query's evaluation time). It is added under the node's own id, so consumers are unchanged.This is the same pattern the native planner already uses for per-series
SummaryAgg(summary_buildthenscope_timestamp) and forSummaryMergein the population precompute adapter (precompute.rs:union→summary_merge→scope_timestamp). Non-temporal merges (notime_index) take the old path unchanged.Key code interfaces
No public API changes. The change is one branch in
crates/asap-physical-operators/src/physical_planner/mod.rs,compile_internal:It uses these existing crate-internal functions:
Test helper:
crates/integration-tests/tests/automatic_window_composition.rsnow runs one body,fn execute_generated_panes(per_series: bool), from two tests.Fields
node.payloadPayloadPayload::SummaryMerge.outputSchemaRefoutput.time_indexisSome, i.e. the merged state is temporal (per-series).schemasVec<SchemaRef>inputsVec<NodeId>mergedOperatorsummary_mergefrombind_operation: groups by every column except the state column andtime_index.compactSchemaRefmerged.schema(): the Planner output schema without the timestamp column.merge_idNodeIdhelper_id(id, 1): a helper id derived only from the Planner node id, above the u32 Planner id range. It is the same in every materialization candidate, so frontier cuts need no renumbering.idNodeIdscope_timestampoperator, so downstream nodes read the timestamped output.scope_timestamp(input, output)input=compact,output= the Planner schema. Requiresoutput.time_indexto be a non-nullTimestamp. Every other output column must match exactly one input column (by type and nullability, and by name for plain columns). Errors if an input column is dropped or repeated. At run time the time column is filled from the executionScope:evaluation_time_msforScope::Query,window_end_msforScope::Ingestion.per_seriesbooltrue: scan schema(ts: Timestamp, value: Float64)withtime_index = Some(0)andReduction::PerEntity.false: the old non-temporal fixture(value: Float64)withReduction::by(vec![]).Examples
End to end (
automatic_per_series_panes_preserve_quantile)Input:
SummaryAggKLLk=200overTimeRange(300 s)of metricevents,Reduction::PerEntity.quantile_over_time(0.99, events[5m]), repeated every 60 000 ms.enumerate_window_compositionsreturns 2 variants; the test takes the composed one (5 panes merged bySummaryMerge) and addsSummaryEstimate { Quantile { q: 0.99 } }.(pane*20 + i) * 1000ms.Native lowering of the merge node after this PR:
Two executions, both with
Scope::Query { evaluation_time_ms: 300_000, revision: 1 }:ts = 300_000, p99 =98.0SummaryAggpanes; panes built underScope::Ingestion { 0..300_000 }, then retained(pane + 1) * 60_000(60 000 … 300 000), then runs the query plan on themts = 300_000, p99 =98.0Before this PR, the test failed while compiling with "native output type differs from Planner output".
Accepted vs. changed cases
SummaryMerge, output hastime_index(per-series)summary_merge→scope_timestamp(new)SummaryMerge, notime_indexsummary_merge, unchanged;automatic_panes_and_materialization_preserve_quantilestill passesSummaryAggsummary_build→scope_timestamp, unchanged (the pattern this PR follows)Note for review: the last retained pane's overwritten timestamp (300 000) equals the evaluation time, so the B1 assertion alone does not rule out "take the latest pane timestamp". The code path itself does not read pane timestamps; it groups them away and fills the time column from the scope.
Out of scope
Stack and validation
Stack: #566 → #568 → #569. Base:
stack/509-22-complete-workload-selection(#568). Completes native temporal binding for the automatically generated exact pane plans (#509 §Example 4, Pattern B).docs/develop_docs/planner-layering-status.mdnotes that per-series native merges ignore pane build timestamps and attach the query evaluation timestamp.Validation: the new regression first failed with "native output type differs from Planner output"; it now passes through direct execution and an automatically enumerated materialization frontier, with different timestamps on retained panes and the query evaluation timestamp asserted. Final workspace validation: 1,638 tests/doctests pass, 2 existing ignores; all-feature Clippy, formatting and diff checks pass.
🤖 Generated with Claude Code