Repository navigation
refactor!: name summary builds by shape; one RowWeight for keyed builds - #758
Merged
Merged
Conversation
Rename the executor's three summary builds by what each holds:
SummaryBuild -> PerGroupSummaryBuild, KeyedSummaryBuild ->
PerGroupKeyedSummaryBuild, SharedSummaryBuild -> AllGroupsSummaryBuild,
with their constructors (per_group_summary_build,
per_group_keyed_summary_build, all_groups_summary_build).
Generalize HydraRowWeight to RowWeight { Column, Constant } and use it in
the keyed per-group build too: it now accepts a constant weight (1 for a
row count), an Int64 weight column and a nullable weight column (NULL
adds nothing). The query-time physical planner binds a keyed heap over
the unit count, which it refused before, and the raw-sample precompute
adds the constant without projecting a column of ones. The i32::MAX bound
stays in the all-groups (HydraCms) constructor; hydra_constant_weight
becomes constant_row_weight.
BREAKING CHANGE: serialized executor Operator variants are renamed, and
the keyed build's `value` field is replaced by `row_weight`.
…test the ±2^53 Int64 weight edge validate_hydra_cms now applies the i32::MAX bound after constant_row_weight, so the kernel check and the all-groups constructor agree. Reword the no-kernel error for a shared-grouping (Hydra) family.
zzylol
force-pushed
the
refactor/summary-build-names-row-weight
branch
from
October 18, 2026 20:30
08f680f to
48ac301
Compare
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.
Why
Two review comments on #753:
The executor had three summary builds named
SummaryBuild,KeyedSummaryBuildandSharedSummaryBuild. The names don't say what each one holds, and they don't read well next to each other. The row weight was also a Hydra-only type (HydraRowWeight), while the keyed per-group build (CMS or Count Sketch with heap) took a barevalue: usizethat had to be a non-null Float64 column. A keyed heap over a row count (Constant(1.0), the unit count) therefore either failed at query time ("keyed summary weight must be a finalized value column") or, in precompute, projected a column of ones.What
PerGroupSummaryBuild/per_group_summary_build: one state per group, updated from one value column.PerGroupKeyedSummaryBuild/per_group_keyed_summary_build: one keyed state per group whose items take a row weight, e.g. a CMS with heap.AllGroupsSummaryBuild/all_groups_summary_build: one state shared by all groups and emitted per group. Today that is HydraCms.HydraRowWeightbecomesRowWeight { Column(usize), Constant(NonZeroU32) }. Both keyed builds now use it. With it, the keyed per-group build also accepts:f64, so an Int64 value beyond 2^53 is refused at run time rather than rounded;RowWeight::Constant(1). It used to refuse this.valuecolumn of ones.i32::MAXbound is Hydra-specific. It is checked by the HydraCms kernel check (validate_hydra_cms) and byall_groups_summary_build, not by the keyed per-group build.hydra_constant_weightis renamed toconstant_row_weight, because both builds use it now. It bounds a constant byu32::MAX.OperatorJSONkindtags change:SummaryBuild→PerGroupSummaryBuild,KeyedSummaryBuild→PerGroupKeyedSummaryBuild,SharedSummaryBuild→AllGroupsSummaryBuild."value": <column>field is replaced by"row_weight": {"Column": <column>}or{"Constant": n}.Operator::name()string stays"SummaryAgg"for all three.How
operators/summary/mod.rs:RowWeightreplacesHydraRowWeight.per_group_keyed_summary_build(input, family, items, row_weight, groups)checks that a weight column is Float64 or Int64.build_per_group_keyed_summaryreads each row's weight from the constant, or from the column through the newkeyed_weight. NULL skips the row; an Int64 value must be within ±2^53.physical_planner/mod.rs:row_weight(input, &update.weight)turns a validatedSummaryUpdate.weightinto aRowWeight. A column becomesColumn(index); a constant goes throughconstant_row_weightand becomesConstant.summary_builddispatcher is renamedsummary_agg_build, so it does not read as one of the three builds.physical_planner/precompute.rs: for a keyed update with a constant weight, the projection drops thevaluecolumn and the build getsRowWeight::Constant. A non-integer keyed constant is now refused there. Stage 1 only producesConstant(1.0)for keyed updates, so no planner output is affected.capability.rs:validate_hydra_cmskeeps the "HydraCms" prefix on constant-weight errors.constant_row_weight, it also refuses a constant abovei32::MAXwith the same error as the constructor.Key code interfaces added / modified
crates/executor/src/operators/summary/mod.rscrates/executor/src/operators/mod.rscrates/executor/src/capability.rsUsage (end to end)
This is the new test
count_ranked_heap_precompute_adds_a_constant_row_weightincrates/integration-tests/tests/precompute_raw_samples.rs. It passes at the linked commit.Before this PR
SummaryBuild,KeyedSummaryBuildandSharedSummaryBuilddidn't say which build holds one state per group and which holds one state for all groups.HydraRowWeightexisted for HydraCms only.Constant(1.0)) failed to bind with "keyed summary weight must be a finalized value column". In precompute, it projectedFiniteFloat64(1.0)as avaluecolumn on every row.After this PR
RowWeightserves both keyed builds.i32::MAXbound stays in the all-groups constructor.Verification
Each new test failed before the change:
query_time_count_heap_binds_a_constant_row_weightfailed with "keyed summary weight must be a finalized value column".raw_sample_heaps_resolve_items_from_labels's new assertion failed with the old precompute behaviour (constant projected as a column).New tests in
crates/executor/tests/keyed_summary_row_weight.rs:query_time_count_heap_binds_a_constant_row_weight: compile a SummaryAgg CmsWithHeap withConstant(1.0)→row_weight {"Constant":1}, counts api/checkout 2, api/search 1, batch/export 3.constant_row_weight_equals_a_column_of_that_constant:Constant(3)equalsColumnover Float64 3.0, both 3× the counts.int64_weight_column_equals_float64_column: Int64 2 gives 2× the counts. 2^53 + 1 is refused with "no exact Float64", and a Utf8 weight is refused at construction.int64_weight_column_equals_float64_columnalso checks that exactly 2^53 builds on the Count-Min heap and −2^53 builds on the Count Sketch heap (signed weights).null_weight_adds_nothing: NULL weights add no items, and both groups keep a state.New test in
crates/executor/tests/hydra_cms_execution.rs:constant_weight_above_i32_is_refused_at_validation:Constant(2147483648.0)+PositiveConstantis refused with the "HydraCms … i32::MAX" error byvalidate_summary_kerneland bycompile_node. It failed before the validation fix.Integration test:
count_ranked_heap_precompute_adds_a_constant_row_weight(the Usage above), plus a new assertion inraw_sample_heaps_resolve_items_from_labels.Commands run:
cargo nextest run -p asap-executor --lib --test hydra_cms_execution --test physical_dag --test physical_semantics --test univmon_execution --test weighted_topk_binding --test current_series_heap --test blocking_resources --test keyed_summary_row_weight --test plan_properties: 173 passed, 4 skipped (pre-existing ignores).cargo nextest run -p asap-integration-tests --test precompute_raw_samples --test kll_pane_execution: 6 passed.4ef9f3e5.cargo fmt --all -- --checkandcargo clippy -p asap-executor -p asap-integration-tests --all-targets --all-features --locked -- -D warnings: clean.Limitations and follow-ups
docs/develop_docs/physical-compile-coverage.mdrow 26 sayskeyed_summary_build;docs/design_docs/proposals/asap-aware-mapping/analytical-resource-cost.md:200saysSummaryBuild.crates/executor/src/physical_planner/mod.rs. Whichever PR merges second rebases.