fix(dag-viewer): recategorize QueryExpr node kinds against real IR (#187)

[zzylol]

Summary

Fixes #187. tools/dag-viewer/node-style.js's KIND_CATEGORY table was
built against names from the old old_docs/docs/l2-intent-algebra.md
design doc (InfoJoin, LetBinding/Ref, Window/WindowFunc,
Distinct, Merge, plain Sample) rather than the literal &'static str
kind tags crates/types/src/dag_export.rs's build_no_recheck and
summary_shape actually push onto a DagNode/SummaryDagNode today. That
turned out to be a bigger problem than the four examples in the issue
suggested: most of those old names are dead entries that never match a
real DagNode.kind, and several real, currently-emitted kinds
(PromqlRelabel, PromqlInfoEnrich, PromqlSeriesSample, Concat,
Dedup, SQLWindowFunc, PromqlScalarBridge, EvalTimestamp,
CurrentTimestamp, PromqlVectorFromScalar, PromqlScalarFromVector,
PromqlSubquery) had no entry at all and were silently falling back to
derive via categoryOf's || 'derive'.

This PR rebuilds the table against the real 23 pre-ASAP + 7 post-ASAP kind
strings and re-decides every category assignment on the actual IR shape
(arity, schema effect, semantics), not surface name similarity to the old
doc.

Categorization rationale

The four kinds the issue called out by name:

• InfoJoin → now PromqlInfoEnrich, moved join → derive. It has
  exactly one QueryExpr child (child), unlike Join's two. The
  "other side" it enriches labels from (an info metric matched by
  selector) is never a QueryExpr/DagNode in this graph at all — it's
  resolved at runtime by the post-ASAP binder. So there's no second DAG
  input for it to combine the way Join/SetOp genuinely do. What it does
  — graft extra label columns onto rows that otherwise pass through
  unchanged — is the same shape of operation as PromqlRelabel's column
  rewrite, so it belongs in derive.

• Sample → now PromqlSeriesSample, moved filter → new sample
  category. Filter narrows rows by a boolean predicate; PromqlSeriesSample
  (limitk/limit_ratio) keeps a deterministic subset of whole series
  per group by quota — its own doc is explicit that it's "not a ranking
  (TopK) and not a reduction." Neither predicate-based nor a
  cardinality-reducing aggregate, so it gets its own category instead of
  overloading filter's meaning.

• Merge → now Concat, and SetOp: split the old set bucket.
  Concat is an exact, n-ary UNION ALL — rows are concatenated, never
  deduplicated — and its own doc explicitly contrasts it with SetOp
  ("SQL's UNION/INTERSECT/EXCEPT are QueryExpr::SetOp, not this").
  SetOp is a binary, genuinely set-theoretic combinator
  (union/intersect/except, dedup-by-default). Lumping them together implied
  Concat carries set semantics it explicitly doesn't. Added a new
  combine category for Concat; set now holds only Dedup (SQL
  DISTINCT — also makes a relation behave like a set) and SetOp.

• LetBinding/bind category: removed. There is no LetBinding (or
  Ref) variant in the current QueryExpr enum at all — that concept
  existed only in the old pre-refactor design doc. Nothing in
  dag_export.rs can ever produce a DagNode this category would apply
  to, and nothing else warrants folding into it, so both the kind entry and
  the category were dead weight.

Other corrections found while reviewing every kind (per the "review
every other node kind too" instruction):

• TimeShift: moved derive → window. Its own doc says it "moves
  when child is evaluated... but leaves its schema unchanged" — no
  column is transformed at all, so derive ("transforms values") was
  wrong. It's the same time-scoping concept TimeRange/PromqlSubquery
  represent.
• Added missing leaf kinds CurrentTimestamp and PromqlScalarBridge (the
  new name for the old Scalar) to data, alongside Scan/EvalTimestamp.
• Renamed every other stale-named entry to its real current variant name
  (Relabel→PromqlRelabel, VectorFromScalar→PromqlVectorFromScalar,
  ScalarFromVector→PromqlScalarFromVector, Subquery→PromqlSubquery,
  WindowFunc→SQLWindowFunc, Distinct→Dedup) without changing their
  category, since those reassignments already looked right on review.

Final category set (pre-ASAP; summary is unchanged and untouched):
data, filter, sample (new), derive, aggregate, window,
join, set, combine (new), sort — bind removed.

Other files checked

viewer.js/index.html build cytoscape styles, CSS vars, and the legend
generically from Object.keys(CATEGORIES)/Object.entries(CATEGORIES) —
no other file hardcodes a category name (viewer.js only special-cases the
data category and the KeepPreAsap kind, both untouched), so no CSS or
legend changes were needed beyond node-style.js itself. render.py's one
"kind" string default is an unrelated AggIntent measure-kind fallback,
not a DagNode.kind.

Tests

• New: two cargo tests in crates/types/src/dag_export.rs
  (node_style_js_categorizes_every_dag_node_kind_exactly_once,
  node_style_js_every_kind_category_value_is_a_declared_category) that
  parse tools/dag-viewer/node-style.js's own source and check its
  KIND_CATEGORY keys exactly match every kind string production code can
  emit — no missing, no orphaned/stale entries — and that every category
  value used is actually declared in CATEGORIES. See "Review follow-up"
  below for how the canonical kind list these compare against is itself
  derived (not a third hand-copied list). This is a real regression
  guardrail: it would have caught the exact staleness this issue reports,
  and will catch it again if a future QueryExpr/SummaryExpr variant is
  added/renamed without updating the viewer — and CI now actually runs it
  on a tools/dag-viewer/**-only change (see below).
• cargo test -p asap-types — 110 passed (includes the 2 new tests).
• cargo test -p asap-devtools --bin dag_export — 6 passed.
• cargo check --workspace — clean.
• python3 -m unittest discover -s tools/dag-viewer -p test_render.py — 18
  passed.
• cargo fmt --check -p asap-types / cargo clippy -p asap-types --tests
  — clean.
• tools/dag-viewer/generate-sample.sh regenerates dag.example.json
  byte-identical to what's committed, and every kind it contains
  (Aggregate, BinaryOp, Join, KeepPreAsap, Limit, Project,
  Scan, Sort, SummaryAgg, SummaryEstimate, TimeRange) has a
  KIND_CATEGORY entry.
• No JS engine (node) is available in this sandbox, so node --check
  still couldn't be run directly (see open questions); I hand-verified
  brace/paren/bracket balance and the new cargo tests exercise the file's
  actual object-literal contents end-to-end (parsing the real source, not a
  mock), which gives strong confidence the syntax is valid.

Review follow-up (addressed)

A first review of this PR found no blocking bugs but flagged three real
gaps, all fixed on this branch:

1. CI never ran the new drift guard on the PRs most likely to need it.
   .github/workflows/rust.yml triggered only on crates/**/Cargo.*
   paths, so a JS-only node-style.js edit wouldn't run cargo test at
   all. Added tools/dag-viewer/** to both the push and pull_request
   path filters.
2. DAG_NODE_KINDS was itself a third hand-copied kind list — nothing
   tied its string literals back to what push_node/summary_shape
   actually emit, so a renamed kind literal at a push_node call site
   could drift from both DAG_NODE_KINDS and node-style.js with every
   test still green (the exact bug class dag-viewer: verify QueryExpr node-category mapping #187 was about). Fixed by
   factoring the kind string out to single-sourced functions instead of
   duplicating it: kind_tag(&QueryExpr) -> &'static str and
   summary_kind_tag(&SummaryExpr) -> &'static str, each an exhaustive
   match over the real enum. push_node now computes DagNode.kind via
   kind_tag(expr) instead of taking a separate kind argument (removed
   from all 23 call sites in build_no_recheck); summary_shape derives
   its returned kind via summary_kind_tag(expr) the same way. The test
   module no longer hardcodes a kind list at all —
   canonical_dag_node_kinds() builds one representative sample
   QueryExpr/SummaryExpr per operator variant and reads back
   kind_tag/summary_kind_tag's real output for it, so the string
   values have exactly one source of truth. This doesn't close the gap
   entirely — see open question below.
3. categoryOf's silent || 'derive' fallback was left in place —
   exactly what let dag-viewer: verify QueryExpr node-category mapping #187 hide for the table's whole life. Added a
   dedicated unknown category (hatched red, distinct from every real
   category and from summary's neutral gray) with a legend row, a
   console.warn from categoryOf when it fires, and a matching
   dashed/thicker-border cytoscape style in viewer.js.

Open questions / follow-ups for a human reviewer

1. window still spans two different "window" concepts on purpose:
   PromQL temporal range-scoping (TimeRange, PromqlSubquery,
   TimeShift) and SQL's OVER (...) analytic window functions
   (SQLWindowFunc). I judged these share enough of a "reads/positions a
   scoped window of rows or time around each row" idea to stay one
   category rather than splitting further (the file's own comment already
   flags that a 10th/11th saturated hue is pushing visual distinguishability),
   but a domain expert might prefer splitting this the same way set was
   split from combine.
2. node --check was not run (no Node.js in this environment) — worth
   a human/CI running it once before merge, though the new Rust-side
   parsing test already exercises the real file content.
3. PromqlInfoEnrich's move out of join is the most debatable call in
   here — it is conceptually a join against an info metric, just one
   where the other side never becomes a graph node. If the viewer's
   audience cares more about "this combines data from two logical sources"
   than "this is literally a two-child DAG combinator," join might still
   be defensible. I went with arity/graph-shape as the tiebreaker since
   that's what the rest of the categories track.
4. The canonical_dag_node_kinds() test helper still doesn't
   auto-discover a brand-new variant (item 2 above): adding a new
   QueryExpr/SummaryExpr operator variant forces a new match arm in
   kind_tag/summary_kind_tag/build_no_recheck/summary_shape
   (compiler-enforced, exhaustive matches), but doesn't force a new sample
   in sample_query_exprs/sample_summary_exprs — so it's possible to add
   a variant, give it a real kind tag, and still have the "every kind has a
   node-style.js entry" test silently not cover it. Fully closing that
   would need either a proc-macro/derive enumerating QueryExpr's variants
   (e.g. a new strum dependency) or a third hand-maintained match whose
   only job is enumeration — judged out of scope for this pass.
5. The hand-rolled JS-scraping parser in the Rust tests
   (parse_kind_category/parse_category_names) is format-brittle
   (assumes 2-space indent, single quotes, \n};-style closes). It fails
   loud rather than silent if node-style.js's formatting changes, so
   lower priority — flagged as-is, not hardened in this pass.
6. dag_export.rs's new tests hard-depend on
   $CARGO_MANIFEST_DIR/../../tools/dag-viewer/node-style.js existing at a
   fixed relative path — fine today, but worth a publish = false note (or
   similar) on the crate if it's ever published standalone. Not touched
   here to avoid unrelated crate-config changes.
7. viewer.js's node[kind = "KeepPreAsap"] selector remains a kind-string
   literal outside KIND_CATEGORY (by design — KeepPreAsap gets a
   deliberate one-off override, not a category). Noted for awareness, not a
   defect.

🤖 Generated with Claude Code