fix: remove QueryExpr::Ref / LetBinding — no producer exists - #192
Merged
Merged
Conversation
Both variants (L3 in crates/ir, plus their L2 mirror in crates/l2's relational::QueryExpr) were marked Reserved: no front end ever emits them, and lowering every query across all 7 corpora we have confirms neither variant is reachable, including the SQL-CTE case they existed for (DataFusion inlines WITH bodies at every use site instead of sharing them). The only code that ever constructed a Ref node was crates/plan/src/cse.rs (dedupe_subtrees), unwired scaffolding for issue #6 that's never called outside its own unit tests. Deleted it along with the support types that existed solely for it: BindingScope/BindingName/QueryId and schema::cse_reuse_is_legal/CseError. Collapsed QueryExpr::output_schema_in's scope threading (crates/plan/src/bind.rs's implement_tree_in/_with) back into a plain output_schema(), since there's no longer a scope to resolve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
milindsrivastava1997
changed the title
4 tasks
This was referenced Aug 22, 2026
zzylol
added a commit
that referenced
this pull request
Aug 23, 2026
Implements #186: two new view modes for tools/dag-viewer, reachable via a Single/Compare/Union toggle next to the drop zone (Single stays the default, unchanged behavior). - Compare mode lays every checked query out in its own compound-node lane (cytoscape.js's built-in lane grouping), side by side. A dashed link, added *after* the per-lane dagre layout runs, connects nodes across lanes whenever their structural hash matches -- so sharing is a line you can trace instead of a ring you hunt for one node at a time. Because the link edges are added post-layout, they never influence dagre's ranking or pull nodes out of their lane. - Union mode merges the checked queries into a single graph: any node whose hash is shared by >= 2 of them collapses into one node (double border), with edges from every query that reaches it converging onto it instead of each drawing a disconnected copy. QueryExpr graphs branch at arbitrary depth, so a merged node can end up with several parents at once -- dagre lays out multi-parent DAGs natively, so no special-cased layout was needed beyond building the merged node/edge set correctly. - The issue's stale references are corrected along the way: the exporter lives at crates/types/src/dag_export.rs (not crates/ir/...), and the hash-based "shared" signal both modes build on is the same proxy the existing single-view ring highlighting already used, now described accurately in both the UI copy (a header note, shown only in Compare/Union) and README.md -- explicitly not real Rc identity and not asap_plan::cse::dedupe_subtrees (deleted in #192; the real pass today, asap_types::pre_asap::cse::share_common_subtrees, still isn't wired into a caller that would give this viewer real CSE output to render). - Two small pre-existing doc bugs fixed in passing: the empty-state instructions referenced the old asap-lower crate name (now asap-devtools, matching the rest of the docs), and the "shared subtree" JS comment pointed at crates/ir/src/dag_export.rs. No Rust changes: WorkloadGraph/DagGraph/DagNode already carry everything needed (per-node hash, children, each query's root) -- confirmed by reading crates/types/src/dag_export.rs before writing any JS. Validation (no browser available in this environment): the JS syntax was checked with esprima, and the real inline script from index.html (plus node-style.js, unmodified) was loaded into a genuine V8 context via py_mini_racer with minimal DOM stubs, then exercised against real dag_export output -- a freshly cargo-run multi-query export with overlapping and disjoint queries, plus the committed dag.example.json -- capturing the exact elements arrays index.html would hand to cytoscape+dagre and asserting on them: no duplicate node/edge ids, every edge references a real node, no Compare-mode structural edge ever crosses a lane boundary, Union-mode merge groups match hand-computed expected sharing (including a merged Scan node with two structurally different parents converging on it -- the multi-parent branching case), multi-root tracking for a node that's the root of more than one merged query, the <2-selected hint path never calls cytoscape(), and single-view's element shape is byte-for-byte unchanged from before the refactor. Separately, dagre.min.js itself was loaded standalone and given a compound/lane graph directly, confirming dagre's compound-node support does lay disconnected lanes out side by side without overlap, in insertion order -- the assumption Compare mode's lane layout relies on. Closes #186 🤖 Generated with [Claude Code](https://claude.com/claude-code)
zzylol
added a commit
that referenced
this pull request
Aug 23, 2026
Implements #186: two new view modes for tools/dag-viewer, reachable via a Single/Compare/Union toggle next to the drop zone (Single stays the default, unchanged behavior). - Compare mode lays every checked query out in its own compound-node lane (cytoscape.js's built-in lane grouping), side by side. A dashed link, added *after* the per-lane dagre layout runs, connects nodes across lanes whenever their structural hash matches -- so sharing is a line you can trace instead of a ring you hunt for one node at a time. Because the link edges are added post-layout, they never influence dagre's ranking or pull nodes out of their lane. - Union mode merges the checked queries into a single graph: any node whose hash is shared by >= 2 of them collapses into one node (double border), with edges from every query that reaches it converging onto it instead of each drawing a disconnected copy. QueryExpr graphs branch at arbitrary depth, so a merged node can end up with several parents at once -- dagre lays out multi-parent DAGs natively, so no special-cased layout was needed beyond building the merged node/edge set correctly. - The issue's stale references are corrected along the way: the exporter lives at crates/types/src/dag_export.rs (not crates/ir/...), and the hash-based "shared" signal both modes build on is the same proxy the existing single-view ring highlighting already used, now described accurately in both the UI copy (a header note, shown only in Compare/Union) and README.md -- explicitly not real Rc identity and not asap_plan::cse::dedupe_subtrees (deleted in #192; the real pass today, asap_types::pre_asap::cse::share_common_subtrees, still isn't wired into a caller that would give this viewer real CSE output to render). - Two small pre-existing doc bugs fixed in passing: the empty-state instructions referenced the old asap-lower crate name (now asap-devtools, matching the rest of the docs), and the "shared subtree" JS comment pointed at crates/ir/src/dag_export.rs. No Rust changes: WorkloadGraph/DagGraph/DagNode already carry everything needed (per-node hash, children, each query's root) -- confirmed by reading crates/types/src/dag_export.rs before writing any JS. Validation (no browser available in this environment): the JS syntax was checked with esprima, and the real inline script from index.html (plus node-style.js, unmodified) was loaded into a genuine V8 context via py_mini_racer with minimal DOM stubs, then exercised against real dag_export output -- a freshly cargo-run multi-query export with overlapping and disjoint queries, plus the committed dag.example.json -- capturing the exact elements arrays index.html would hand to cytoscape+dagre and asserting on them: no duplicate node/edge ids, every edge references a real node, no Compare-mode structural edge ever crosses a lane boundary, Union-mode merge groups match hand-computed expected sharing (including a merged Scan node with two structurally different parents converging on it -- the multi-parent branching case), multi-root tracking for a node that's the root of more than one merged query, the <2-selected hint path never calls cytoscape(), and single-view's element shape is byte-for-byte unchanged from before the refactor. Separately, dagre.min.js itself was loaded standalone and given a compound/lane graph directly, confirming dagre's compound-node support does lay disconnected lanes out side by side without overlap, in insertion order -- the assumption Compare mode's lane layout relies on. Closes #186 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
Summary
QueryExpr::Ref/QueryExpr::LetBindingfrom L3 (crates/ir) and their L2 mirror (crates/l2::relational) — both were marked Reserved, and lowering every query across all 7 corpora we have (~2600 queries, including the SQL-CTE cases they existed for) confirms neither is ever produced.crates/plan/src/cse.rs(dedupe_subtrees) — the only code that ever constructed aRefnode, but itself unwired scaffolding for Wire workload-level CSE into a cost model #6 (never called outside its own unit tests).BindingScope,BindingName,QueryId,schema::cse_reuse_is_legal/CseError.QueryExpr::output_schema_in(scope)back into a plainoutput_schema(), andplan::bind'simplement_tree_in/implement_tree_in_withintoimplement_tree/implement_tree_with, since there's no longer a binding scope to thread.dag_export,canonicalize,lower,sql,promql, and thevariant_coveragecorpus tool that referenced the removed variants/module.Closes #181.
Note: this removes the current scaffolding for #6 ("Wire workload-level CSE into a cost model"). That issue's CSE approach will need a fresh design that doesn't route through
Ref/LetBinding-shaped tree nodes.Test plan
cargo build --workspace --all-targetscargo test --workspace(all green)cargo clippy --workspace --all-targets(clean)cargo fmt --check(clean)🤖 Generated with Claude Code