docs(asap-aware-mapping): correct module doc on where CSE detection runs - #229
Merged
Merged
Conversation
Records the placement decision settled in #222: common sub-expression elimination detection is a primary pass over the pre-ASAP QueryExpr IR (asap-types::pre_asap, design tracked in #223), not something asap-aware-mapping hoists after binding. This crate's module doc previously claimed "which shared sub-expressions to hoist" as its own job, but no such pass existed here or was ever meant to be primary. asap-aware-mapping may still eventually run a secondary, narrower CSE pass over already-implement_tree'd SummaryNodes, recognizing sharing that's invisible at the pre-ASAP level (e.g. two different accuracy targets on the same Quantile can share one built sketch) -- but that is downstream of, not a substitute for, the primary pre-ASAP pass. Part of #222 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merged
5 tasks
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.
Summary
Records the placement decision settled in #222: common sub-expression
elimination (CSE) detection is a primary pass over the pre-ASAP
QueryExprIR (asap-types::pre_asap), not somethingasap-aware-mappingdoes. The algorithm itself is tracked separately in #223 (design doc,
implemented by a different workstream); this PR only corrects the
documentation to reflect the decision.
asap-aware-mapping's own module doc (crates/asap-aware-mapping/src/lib.rs)previously claimed "which shared sub-expressions to hoist" as this crate's
job — that was aspirational text with no backing implementation, and per
#222 it's also simply the wrong layer for CSE detection to live. This PR
corrects that doc comment to say:
asap-types::pre_asap(design in Design: pre-ASAP structural CSE via bottom-up hash-consing over Rc<QueryExpr> #223),running before a tree ever reaches
bind::implement_tree.asap-aware-mappingmay eventually run a secondary, narrower CSE passof its own over an already-
implement_tree'dSummaryNodeDAG —recognizing post-ASAP-only sharing such as
Quantile(x, 0.99)andQuantile(x, 0.95)sharing one built sketch, read out twice — but thatis downstream of, and not a substitute for, the primary pre-ASAP pass.
crates/asap-aware-mapping/src/cost_model.rs's module doc ("workload-levelCSE credit" as a planned cost-model input) was checked and left unchanged —
it only speaks to cost-model ranking, which is downstream of detection, and
doesn't claim this crate does detection itself.
docs/*.mdwere checkedtoo; none currently discuss CSE placement, so there was nothing to correct
there.
I've also posted a comment on #212 (the parent "add CSE" issue) recording
this decision against its own "Where it runs." open question.
Scope
No runtime/behavior changes — module-doc comment only.
cargo build --workspace --all-targets,cargo test --workspace,cargo fmt --all -- --check, andcargo clippy --workspace --all-targets --all-features -- -D warningsall pass unchanged.Closes #222
Test plan
cargo build --workspace --all-targetscargo test --workspacecargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warnings🤖 Generated with Claude Code