feat(asap-aware-mapping): AvgToSumOverCountStrategy semantic rewrite (#253) - #260
Merged
Merged
Conversation
zzylol
force-pushed
the
feat/replacement-strategy-251
branch
from
August 23, 2026 19:59
ca60c0c to
66c772d
Compare
zzylol
force-pushed
the
feat/semantic-rewrite-avg-253
branch
from
August 24, 2026 14:11
cdf4ca8 to
8c019f1
Compare
zzylol
added a commit
that referenced
this pull request
Aug 24, 2026
…licability.rs "Site" was an informal synonym introduced when this module's docs were written — every occurrence names the exact same thing the type system already calls TargetSubDAG (or, once discovered, a MemoGroup's own `target`). Replaced every free-standing "site"/"a site's..." with TargetSubDAG (or "the TargetSubDAG" as a phrase) throughout the module doc and the OptimizationKind/collect_locations/test doc comments, so the prose names the real type instead of a parallel, undefined term. Left two references untouched: `crate::search::discover_sites` (a real, unrenamed function name in search.rs) and the quoted section title "Where `for site in plan.bindable_sites()` comes from" (a verbatim quote of search.rs's own doc heading) — both are accurate references to search.rs's own content, which this PR doesn't touch. Also fixed a handful of stale `boundary::`-module doc links found while in here (the module was renamed to `implementation` well before this branch's fork point — same staleness class as PR #263/#262/#261/ #260, just not load-bearing for compilation since these were doc-only intra-doc links). Verified: cargo build --workspace --all-targets, cargo test --workspace (0 failures), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings — all clean.
…urrent tip This branch had been forked at the very first draft of #251 (boundary::implementation_for_with, before the boundary.rs -> implementation.rs rename, the "make binding literally a selector over ReplacementStrategy" refactor, and this session's deletion of bind::implement_tree/implement_tree_with) and never rebased — its own frozen copy of replacement.rs/lib.rs would not compile against the current tip of feat/replacement-strategy-251. Rebuilt fresh: reset this branch onto the current #259 tip, then reapplied only the genuinely new content this PR adds on top — replacement.rs/bind.rs/implementation.rs now come from #259 itself, unmodified. - rewrite.rs (AvgToSumOverCountStrategy, issue #253, part of #33) had almost no stale-API surface to begin with: it's a pure pre-ASAP -> pre-ASAP QueryExpr rewrite (avg -> sum/count) that only touches crate::replacement's stable, unchanged vocabulary (Replacement/ReplacementStrategy/ReplacementSubDAG/TargetSubDAG) and never called implement_tree_with or touched SketchKind/Implementation construction. Reapplied verbatim except for three stale prose spots: two `boundary::implementation_for_with` doc/rationale references updated to `implementation::implementations_for_with` (the module rename), and one `docs/asap_aware_mapping.md` path reference corrected to `docs/design_docs/asap_aware_mapping.md` (the design doc's actual location on this tip). None of these were load-bearing for compilation. - lib.rs: added `pub mod rewrite;`, a `## Status` doc bullet for the new module (mirroring the existing bullets' style; the original PR's own lib.rs diff never added one), and `pub use rewrite::AvgToSumOverCountStrategy` (also missing from the original PR's diff) so the strategy is actually reachable from outside the crate the same way its SketchFamilyStrategy/SharedSubtreeStrategy siblings are. Verified: cargo build --workspace --all-targets, cargo test --workspace (0 failures, including all 12 rewrite:: tests), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings — all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
feat/semantic-rewrite-avg-253
branch
from
August 25, 2026 00:17
8c019f1 to
7c90615
Compare
milindsrivastava1997
approved these changes
Aug 25, 2026
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.
Closes #253. Part of #33. Stacked on #259 (issue #251) — do not merge before that one.
What
A
ReplacementStrategy—AvgToSumOverCountStrategy— implementing the"semantic-equivalent rewriting" degree of freedom from
docs/asap_aware_mapping.md: rewritesAggregate{ measures: [Avg{col}], .. }into
Aggregate{ measures: [Sum{col}, Count], .. } |> Project{ sum/count re-divided under the original avg column name }.Why
boundary::implementation_for_withdispatchesAvgstraight toImplementation::PassThrough— a bareavgnode has no summaryrealization at all, so it can never be a
SharedSubtreeStrategytarget.SumandCountare both ordinary mergeable accumulatorsSharedSubtreeStrategy(and a future sketch-family search) already knowhow to reuse across a workload; this rewrite reshapes
avginto thosepieces without changing what the query computes.
Shape
matches: a singleAvg{col}measure, noHAVING(the samebindable shape
bind::implement_tree_withrequires — mirrorsSketchFamilyStrategy's ownbindable_intentcheck), furtherrestricted to an ordinary
by(...)reduction.without(...)andReduction::PerEntityare excluded, for real correctness reasons, notstyle:
PerEntityis single-measure by construction(
aggregate_output_schema's owndebug_assert, which this rewrite'ssecond measure would violate), and
without(...)leaves the aggregate'sown output schema open (
closed: false) while the wrappingProjectthis rewrite always produces forces
closed: true— a schema driftthis strategy declines to introduce.
replacements: exactly oneReplacement::Rewritecandidate — noranking, no "is it worth it" heuristic. Per the issue's own framing:
earlier drafts needed a manual before/after-CSE cost comparison to
decide whether rewriting helps; with
ReplacementStrategy'sexhaustive-candidate shape in place that's unnecessary — both the
original
Avgand the rewrittenSum/Countform are just twocandidates a future cost-based search (issue asap-aware-mapping: Cascades/Volcano-style candidate-plan search engine over ReplacementStrategy #252) can compare, not
something this strategy pre-decides.
Project'ssum / countdivision is explicitlyCasttoFloat64so the rewrittenavgcolumn matchesAggIntent::Avg::output_column's own(Float64, nullable: false)shape regardless of the summed column's own type — an uncast
Int64 / Int64division would otherwise silently retype the column,which a test in this PR pins down.
Before / after planner output
Example query:
Before —
avghas no summary realization at all (implementations_for_withdispatches it toPassThrough), so the only candidate keeps the whole aggregate logical — nothing to share or sketch:After —
AvgToSumOverCountStrategyadds a second candidate:avgreshaped intosum/countunder the same grouping, re-divided back by a wrappingProject— two ordinary mergeable accumulators a future sketch-family search / CSE can now actually reuse, where a bareavgnode never could:Tests
Positive: an ungrouped and a grouped
Avgaggregate both match andrewrite correctly. Negative: multi-measure,
HAVING-bearing, non-Avgintents,
without(...)grouping,Reduction::PerEntity, and non-Aggregatenodes all correctly don't match. Schema round-tripping:
original.output_schema() == rewritten.output_schema()holds exactly foran ungrouped aggregate (including an explicit-output-name-override
variant); for a grouped aggregate, column-level equality is asserted
directly, with an explicit test documenting the one known gap (
Projectalways resets
unique_keys, a pre-existing property of everyProject-wrapped rewrite in this IR, not specific to this rewrite —unique_keysalso isn't consumed by anything yet perSchema's own doccomment). A dedicated test with an
Int64-typed summed column exercisesthe
Castdirectly.lib.rs
Added only
pub mod rewrite;— kept minimal since sibling PRs #252/#254/#256touch the same file from the same base branch in parallel.
Checks
cargo build --workspace --all-targets,cargo test --workspace,cargo fmt --all -- --check, andcargo clippy --workspace --all-targets --all-features -- -D warningsallpass clean.
🤖 Generated with Claude Code