Skip to content

CSE detection should run in pre-ASAP IR; post-ASAP can run a secondary pass #222

Description

@zzylol

Summary

Resolves the "Where it runs" open question in #212. CSE detection (structural
recognition of shared sub-computations within a query, or across a
QueryWorkload batch) should be implemented as a pass over the pre-ASAP
QueryExpr IR (asap-types::pre_asap), not inside asap-aware-mapping.
Post-ASAP binding can optionally run a second, narrower CSE pass over the
already-implemented SummaryExpr/SummaryNode DAG, but that is a
secondary optimization on top of the primary one, not where detection should
mainly happen.

Why pre-ASAP, not post-ASAP

  1. Batch query optimization needs to see shared work before summary
    binding, not after.
    This is motivated by Peilin's batch query
    optimization work: batching only pays off when the planner recognizes
    duplicate/overlapping computation before deciding how each piece gets
    realized. Recognizing it only after realization means summary choices for
    each query are already made independently, and reconciling them after the
    fact is a strictly harder problem (summary-level equivalence/subsumption
    reasoning) than sharing the pre-ASAP subtree that feeds a single bound
    summary in the first place.
  2. The legality primitive already lives at this layer. Schema::unique_keys
    / is_reusable() is a pre-ASAP schema property, and its own doc comment
    already says it "feeds CSE's producer-sharing legality check" — the IR
    was already built expecting detection to happen here.
  3. asap-aware-mapping::implement_tree runs per query today. If CSE only
    existed post-implement, every query would still get bound to its own
    summary independently, and CSE would have to retroactively merge two
    already-built SummaryAgg nodes instead of sharing one pre-ASAP subtree
    before either is built.
  4. Consistency with "normalize semantics, not syntax" (docs/pre-asap-ir.md)
    — CSE is a normalization-adjacent concern (recognizing equivalent
    computation) and belongs next to canonicalize, which already runs at
    this layer.

Where post-ASAP CSE still has a role

Not every sharing opportunity is visible at the pre-ASAP level. Two
structurally different pre-ASAP nodes — e.g. Quantile(x, 0.99) and
Quantile(x, 0.95) — can legitimately share one built sketch, read out
twice. That's only recognizable once both are bound to the same
(SummaryKind, params) at post-ASAP, since pre-ASAP structural equality
(which includes the accuracy target on AggIntent) will correctly treat them
as different nodes. So the design is two passes, not one:

  • Primary — pre-ASAP structural CSE. Over QueryExpr, before
    implement, keyed on structural/value equality, gated by unique_keys.
  • Secondary — post-ASAP summary-level CSE. Over already-implemented
    SummaryNodes, keyed on (SummaryKind, params, source-subtree) equality,
    recognizing readout-level sharing the pre-ASAP pass can't see by
    construction.

Scope of this issue

This issue settles the placement decision and updates #212 accordingly. The
detection algorithm itself is tracked in a follow-up design-doc issue. The
prerequisite representation work — pre_asap::QueryExpr needs a
shared-subtree representation (Rc instead of Box) before any CSE pass can
express sharing at all — is being landed as mechanical prep ahead of that
design doc.

Related

  • New feature: add common sub expr elimination #212 — parent issue; this resolves its "Where it runs" open question
  • crates/asap-aware-mapping/src/lib.rs module doc (names "which shared
    sub-expressions to hoist" as a stated but unimplemented job)
  • crates/types/src/pre_asap/schema.rs (unique_keys / is_reusable)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions