Skip to content

feat(pre-asap): structural CSE via bottom-up hash-consing (stages 1+2) - #235

Merged
zzylol merged 1 commit into
mainfrom
feat/pre-asap-cse
Aug 22, 2026
Merged

zzylol merged 1 commit into
mainfrom
feat/pre-asap-cse

Conversation

@zzylol

@zzylol zzylol commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements stages 1 and 2 of #223's 4-stage landing plan — the pre-ASAP
structural common-subexpression elimination pass, and a real caller wired
alongside it so the pass never becomes unwired dead code (the same fate
asap-plan::cse::dedupe_subtrees met in #192).

  • Stage 1asap_types::pre_asap::cse::share_common_subtrees +
    InternTable (new module, crates/types/src/pre_asap/cse.rs): bottom-up
    hash-consing over an already-resolve_root'd QueryExpr tree.

    • A coarse structural hash (canonical-JSON-string + DefaultHasher, the
      same trick dag_export.rs's own structural_hash uses, deliberately
      kept independent for now — stage 3 unifies them) only narrows the
      candidate set; PartialEq is the actual, non-negotiable sharing
      decision, every time.
    • Gated by Schema::has_unique_key(): a subtree with no provable row
      identity is never hoisted, even when structurally identical to
      something already interned.
    • Single-query CSE (a repeated sub-expression within one query) falls out
      of the same share_common_subtrees call for free — no separate
      mechanism.
    • Traversal scope mirrors canonicalize.rs's children_mut exactly:
      only the relational-skeleton children participate; a scalar
      sub-expression reachable only through a wrapper position (Predicate,
      ProjectItem.expr, Aggregate.having, …) stays embedded, compared as
      part of its owning node's PartialEq — the same scope
      canonicalize.rs already settled on for its own bottom-up walk.
    • Unit tests: distinct-column Quantiles don't merge; no unique_keys
      no merge (including a without(...) grouping sanity check); median
      vs. an explicit q=0.5 percentile do merge (both already lower to
      the identical AggIntent::Quantile, per
      sql_lowering.rs'smedian_is_the_same_intent_as_an_explicit_half_percentile);
      single-query CSE; and Distinct gated the same way as Aggregate.
  • Stage 2asap_aware_mapping::implement_workload /
    implement_workload_with (crates/asap-aware-mapping/src/bind.rs): a
    real caller for stage 1. Memoizes on Rc::as_ptr identity over
    implement_tree/implement_tree_with, so two workload roots sharing an
    Rc<QueryExpr> after share_common_subtrees bind to one shared
    Rc<SummaryNode> instead of being bound twice. New
    crates/integration-tests/tests/cse.rs drives real PromQL text through
    the full lower_promql → share_common_subtrees → implement_workload
    pipeline and asserts the sharing survives end-to-end via Rc::ptr_eq on
    the bound result (plus a negative control for non-identical queries, and
    a single-query-repeated-subexpression case).

Adaptations from the issue's illustrative pseudocode

The issue's own text says its code sketch is illustrative, not verified
against current code, and current main has drifted since it was written:

  • QueryId doesn't exist anywhere in the codebase. share_common_subtrees
    and implement_workload are generic over a caller-chosen Id instead of
    inventing a new pub type.
  • Schema::is_reusable() doesn't exist; the real method is
    has_unique_key().
  • implement_tree/implement_tree_with return Result<Rc<SummaryNode>, ImplementError>, not an infallible Rc<SummaryNode>, so
    implement_workload returns Vec<(Id, Result<Rc<SummaryNode>, ImplementError>)> rather than the sketch's Vec<(QueryId, Rc<SummaryNode>)>. Only successful binds are memoized; a failed bind is
    retried (not cached) if the same Rc reappears.
  • The Merge/Distinct/Scalar/etc. rename in refactor: rename PromQL-specific QueryExpr nodes (and Merge/Distinct/WindowFunc/EvalTime) for clarity #226 had not landed on
    main as of this branch, so this PR uses the pre-rename variant names
    that are actually on main today.

Deliberately deferred (per the issue's own stage sequencing)

Not attempted in this PR, matching #223's landing plan:

  • Stage 3 — unifying dag_export::structural_hash with this pass's
    hashing, and updating tools/dag-viewer's "shared subtree" highlighting
    to reflect real CSE output instead of the current hash-proxy.
  • Stage 4 — wiring workload-level CSE credit into CostModel ("only
    then," per the issue).

Also out of scope, per the issue: subsumption reasoning
(asap_aware_mapping::boundary::Matcher) and cross-batch/persistent
sharing.

Test plan

  • cargo build --workspace --all-targets
  • cargo test --workspace (all green, including the 6 new cse
    unit tests and 3 new integration tests)
  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings

Part of #223.

🤖 Generated with Claude Code

Rebased onto main now that #226 (the QueryExpr rename) and #227 (the
SQL function catalog) have landed there. This PR's own design and
content are unchanged; adapted the new cse.rs module's exhaustive
QueryExpr match to the current post-rename variant names (Merge ->
Concat, Distinct -> Dedup, Scalar -> PromqlScalar, EvalTime ->
QueryTimestamp, WindowFunc -> SQLWindowFunc, Arith -> Arithmetic,
VectorFromScalar/ScalarFromVector/Relabel/InfoJoin/Sample/Subquery ->
their Promql*-prefixed names, ArithOp/CompareOp -> ArithmeticOpKind/
CompareOpKind) and reapplied mod.rs's doc/pub-use additions around the
renamed expr_ir re-export.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the feat/pre-asap-cse branch from a400d1f to eba4fbe Compare August 22, 2026 20:59
@zzylol
zzylol merged commit 5581df2 into main Aug 22, 2026
3 checks passed
@zzylol
zzylol deleted the feat/pre-asap-cse branch August 22, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant