Repository navigation
refactor: Hydra row weight enum, TooManyCandidates count, cost_unit rename, stacked-shift pane fix - #753
Conversation
…ow weight A weight column and a constant weight were two parameters, so a column together with a non-unit constant silently dropped the constant. The SharedSummaryBuild operator now takes one HydraRowWeight (Column or a NonZeroU32 Constant at most i32::MAX), serialized as the one field row_weight. The unreachable weight arm now panics as unreachable, and validate_hydra_cms documents the PositiveConstant proof.
The variant's candidates field held a description, not candidates. It is
now TooManyCandidates { counted, found, max }, with the message
"found {found} {counted}, more than {max}", and each of the five cap
sites passes its count.
pane_window_ms checked only the TimeShift directly below the range, so an @-anchored shift under another shift got panes. It now walks every stacked shift and refuses when any has an @ anchor.
The field holds the unit of cost_per_second, so it is now cost_unit, and stage_pipeline's per-candidate cost key follows. The viewer reads and validates cost_unit, and its candidate rows drop the unread unit field; the committed example is regenerated.
| /// Produced by the physical planner from a validated `SummaryUpdate` | ||
| /// weight and read once per row by the build. | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] | ||
| pub enum HydraRowWeight { |
There was a problem hiding this comment.
This seems to be a special case, why hydra row weight is a separate enum definition?
There was a problem hiding this comment.
Agreed, it was a special case. #758 replaces HydraRowWeight with a general RowWeight { Column(usize), Constant(NonZeroU32) }, and both keyed builds use it: the per-group keyed build (CMS or Count Sketch with heap) and the all-groups HydraCms build. The keyed per-group build now also takes a constant weight (1 for a row count, so a count-ranked heap binds at query time and precompute no longer projects a column of ones), an Int64 column and a nullable column. The Hydra-only i32::MAX bound stays in the all-groups constructor.
|
SharedSummaryBuild just reading the name, I cannot tell what it is for, |
|
Re "SharedSummaryBuild just reading the name, I cannot tell what it is for": #758 renames the three executor summary builds by shape, so they read side by side. |
Why
The review of #752 found interfaces that let a caller say something meaningless, plus names that do not say what they hold:
Operator::shared_summary_build(.., weight: Option<usize>, constant_weight: i32, ..)took a weight column and a constant weight as two parameters. Withweight = Some(col)andconstant_weight = 2, the build silently ignored the 2. A runtimeconstant_weight <= 0check was needed because the type allowed both zero and negative values.PerComputationCandidateError::TooManyCandidates { candidates, max }: thecandidatesfield held a description such as"rollup candidates", not candidates. The message also never said how many were found.CandidateCost.unitheld the unit ofcost_per_second, but its name did not say so.pane_window_mschecked for an@anchor only on the shift directly below the range. If an anchored shift sat under anoffsetshift, the build was offered panes.What
row_weight: HydraRowWeight, which is eitherColumn(usize)orConstant(NonZeroU32). A column and a constant can no longer be passed together.shared_summary_buildrefuses a constant abovei32::MAX(asap_sketchlib's Hydra addsi32weights). This applies to direct calls and to deserialized documents. The<= 0check is removed because the type rules it out.TooManyCandidates { counted, found, max }, with the messagefound {found} {counted}, more than {max}. Each of the five cap sites passes the count it found.CandidateCost.unit→cost_unit. Thestage_pipelineJSON key, the viewer readers and the committed example change with it.validate_hydra_cmsdoc now names theUnitCountandPositiveConstantproofs._ => (None, 1)weight arm is nowunreachable!("validated HydraCms weight").SummaryInputExpralso hasTupleandEntityIdentity, so the match cannot be made total.pane_window_msnow walks every stackedTimeShiftbelow the range and refuses panes when any of them has an@anchor.Serialized format changes
OperatorJSON,SharedSummaryBuild: the fieldsweight(optional column index) andconstant_weight(default 1) are replaced by one required field,row_weight. Its value is{"Constant": 2}or{"Column": 2}. Old documents are not readable; keeping them readable would need a custom deserializer, and the repo has no backward-compatibility rule.stage_pipelinedocument,stage3_plan_selection.costs.<id>: the keyunitis nowcost_unit. The viewer validatescost_unit, and old viewer documents fail validation withcost_unit must be a string. The committedtools/dag-viewer/examples/planner-layering-example1.jsonis regenerated; only the 19 renamed keys change.How
HydraRowWeightlives next to the shared summary build inoperators/summaryand is re-exported fromoperators. The physical planner maps the validatedSummaryUpdate.weightto it: aColumnresolves to its input index, and aConstantgoes throughcapability::hydra_constant_weight. That function now returns aNonZeroU32that is at mosti32::MAX, instead of ani32. The build readsrow_weightonce per row: a constant is added as is, a column value is added, and a NULL value adds nothing.Kind::SharedSummaryBuildstoresrow_weightdirectly. Deserialization goes throughUncheckedOperator→shared_summary_build, so thei32::MAXcheck also covers documents.subsetsreportsn, and the combined reuse cap reports the rollup count plus the top-k count.unitfield. No code read it; the lane header and ranking label readcost.cost_unitfrom the cost itself.Key code interfaces added / modified
crates/executor/src/operators/summary/mod.rs,crates/executor/src/operators/mod.rssummary/mod.rs · mod.rs
crates/executor/src/capability.rscapability.rs
crates/logical-asap-optimization/src/per_computation_candidates.rsper_computation_candidates.rs
crates/plan-selection/src/lib.rs,crates/devtools/src/bin/stage_pipeline.rs,tools/dag-viewer/{stages,app,editor}.jslib.rs · stage_pipeline.rs · stages.js
Usage (end to end)
No workload reaches a
HydraRowWeight::Constanttoday: the Stage 1 rules emit onlyUnitCount,ResetAwareCounterDerivativeandCounterSamplesweight proofs, neverPositiveConstant. So the Hydra flow below starts at the Stage 2SummaryAggnode that a future rule would produce; thecost_unitflow starts from a realstage_pipelinerun.Shared HydraCms build with a constant weight. This is the flow of
row_weight_serializes_as_one_field_and_rejects_constants_above_i32andpositive_constant_weight_adds_the_constant_per_rowincrates/executor/tests/hydra_cms_execution.rs, both run at the head commit:The cost unit as
stage_pipelineusers see it (run at the head commit):Before this PR
shared_summary_build(input, family, item, Some(2), 5, groups)built a column-weighted Hydra and dropped the 5 without an error.constant_weight = 0was rejected only at runtime.more than 20 rollup and top-k reuse candidates together (2^n Pass 2 candidates), without the count found."unit": "cost_per_second".SummaryAggoverTimeRange(5m)overTimeShift(offset 1m)overTimeShift(@ 100)was offered panes (pane_window_ms=Some(300000)).After this PR
i32::MAXis refused at construction and at deserialization.found 21 rollup and top-k reuse candidates together, more than 20."cost_unit": "cost_per_second".@anchor anywhere in the stacked shifts below the range means no panes (None).Verification
asap_aware_cse::tests::an_anchor_below_stacked_shifts_has_no_panesfailed before the fix (left: Some(300000), right: None) and passes after it.cargo nextest run -p asap-executor -E 'test(/hydra|shared|constant_weight/) | binary(hydra_cms_execution)': 21 passed. This includes the newrow_weight_serializes_as_one_field_and_rejects_constants_above_i32.cargo nextest run -p asap-logical-asap-optimization -E 'test(/asap_aware_cse::|reuse_candidates/)': 25 passed. This includes the newtoo_many_pane_or_reuse_candidates_name_the_count_foundand the updated exact-message tests for the capability sharing and combined reuse caps.cargo nextest run -p asap-devtools --test stage_pipeline: 22 passed, includingexample1_document_is_valid_and_committed_fixture_is_currentagainst the regenerated example.cargo nextest run -p asap-plan-selection -E 'test(cluster) | test(priced)': 6 passed.python3 -m unittest test_viewer test_serverintools/dag-viewer: 52 passed.cargo fmt --all -- --checkandcargo clippy -p asap-executor -p asap-logical-asap-optimization -p asap-plan-selection -p asap-devtools --all-targets --all-features --locked -- -D warnings: clean.Removed unread field: the viewer's Stage 3 candidate row
unit(stageCandidateRowsinstages.js); nothing read it.Limitations / follow-ups:
pane_chain.rsfolds the pane offset into the shift directly below the range only. That is correct, because the composed offset is additive and the@anchor is now refused at any depth. It is noted here only in case stacked shifts become common.tools/dag-viewer/README.md:84still shows"unit": "cost_per_second"; it is nowcost_unit.