feat(asap-aware-mapping): wire AvgToSumOverCountStrategy into default_strategies - #282
Merged
Merged
Conversation
…_strategies AvgToSumOverCountStrategy (issue #253) has existed as a full ReplacementStrategy impl since rewrite.rs was added, but was never included in default_strategies()/default_strategies_with() — so search_workload (and everything built on it: explain_replacements, dag_export, sketch_coverage) never actually ran it. A bare avg node has no summary realization of its own (PassThrough) and can't be a SharedSubtreeStrategy target either, so without this rewrite running first, avg aggregates were invisible to every sketch/CSE strategy downstream. It's context-free (matches()/replacements() need nothing beyond the target itself, same as SharedSubtreeStrategy) rather than workload-dependent like RollupStrategy, so it belongs in the static default_strategies() list rather than being derived per-workload. Verified: full asap-aware-mapping (142 tests) and asap-devtools test suites pass unchanged; cargo check --workspace is clean; manually confirmed dag_export on an AVG query produces no new stderr warnings (no explanation ends up unmatched against the original exported graph). Also corrected a stale doc-comment table row in explanation.rs that predated this change and already contradicted the code (claimed no ReplacementStrategy impl existed at all). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 26, 2026
default_strategies() already includes it as of #282 — this binary no longer needs its own custom strategy list, just plain search_workload(). Also reworded the 'unmatched winner' diagnostic: with AvgToSumOverCountStrategy now running by default, its rewrite output routinely exposes new sum/count descendants that the same search pass independently sketch-ranks — real winners, but ones with no node in any query's original pre-rewrite graph to attach a flat replacements[] entry to (expected, not a bug: still fully represented in that query's post_graph). The old wording read like an error for what is normally an expected case. Verified: cargo test --workspace, fmt, and clippy all clean; manually re-ran --post-asap on an AVG query and confirmed the JSON output is identical (only the diagnostic wording changed).
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
1 participant
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.
What
AvgToSumOverCountStrategy(issue #253) has existed as a fullReplacementStrategyimplementation sincerewrite.rswas added, but was never included indefault_strategies()/default_strategies_with()— sosearch_workload(and everything built on it:explain_replacements,dag_export,sketch_coverage) never actually ran it.Why
A bare
avgnode has no summary realization of its own (Implementation::PassThrough) and can't be aSharedSubtreeStrategytarget either (no mergeable accumulator to share). Without this rewrite running first,avgaggregates were invisible to every sketch/CSE strategy downstream — rewritingavg(x)intosum(x)/count(*)is exactly what lets those strategies see something to work with.It's context-free (
matches/replacementsneed nothing beyond the target itself, same asSharedSubtreeStrategy) rather than workload-dependent likeRollupStrategy(which is already auto-added per-workload insidesearch_workload_with), so it belongs in the staticdefault_strategies()list.Also corrected a stale doc-comment table row in
explanation.rsthat predated this change and already contradicted the code (it claimed noReplacementStrategyimplementation existed for this at all).Verification
cargo test -p asap-aware-mapping— 142 tests, all pass unchangedcargo test -p asap-devtools— all pass unchangedcargo check --workspace— cleancargo fmt --all -- --check/cargo clippy --workspace --all-targets --all-features --locked -- -D warnings— cleandag_exporton anAVGquery produces no new stderr warnings (no explanation ends up unmatched against the originally-exported graph)🤖 Generated with Claude Code