From 10dae8a0af1d4e63ce94f18c76c68c07e87f000a Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 26 Aug 2026 13:42:31 -0600 Subject: [PATCH 1/2] fix(dag-viewer): recategorize QueryExpr node kinds against real IR (#187) node-style.js's KIND_CATEGORY table was keyed on names from the old l2-intent-algebra design doc (InfoJoin, LetBinding/Ref, Window/WindowFunc, Distinct, Merge, plain Sample), not the actual kind strings crates/types/src/dag_export.rs emits at runtime. Most of those old names don't exist as DagNode.kind values at all, and several real kinds (PromqlRelabel, PromqlInfoEnrich, PromqlSeriesSample, Concat, Dedup, SQLWindowFunc, CurrentTimestamp, ...) had no entry and silently fell back to 'derive'. Rebuilt the table against dag_export.rs's build_no_recheck/summary_shape match arms (the real ground truth for what DagNode.kind can be) and re-decided every category assignment: - PromqlInfoEnrich (was 'InfoJoin' in 'join'): has exactly one QueryExpr child, unlike Join's two -- the info metric it enriches from is never a DagNode, it's resolved at runtime. Moved to 'derive' (it grafts extra columns onto passthrough rows, like PromqlRelabel). - PromqlSeriesSample (was 'Sample' in 'filter'): keeps a deterministic subset of series by quota, not a boolean predicate. Given its own new 'sample' category instead of overloading 'filter'. - Concat (was 'Merge', lumped into 'set' with SetOp): exact n-ary UNION ALL, no dedup -- its own doc explicitly contrasts it with SetOp. Split into a new 'combine' category; 'set' keeps only genuine set-semantic ops (Dedup, SetOp). - LetBinding/'bind' category: removed outright. LetBinding has no equivalent in the current QueryExpr enum at all (no let/ref binding concept survives past the front end); nothing else maps into 'bind' either. - TimeShift (was 'derive'): its own doc says it changes when child is evaluated and leaves its schema unchanged -- no value transform at all, so it belongs with the other time-scoping kinds in 'window'. Locks the corrected mapping in with two new cargo tests in crates/types/src/dag_export.rs that parse node-style.js's own source and check its KIND_CATEGORY keys exactly match the literal kind strings build_no_recheck/summary_shape can produce (no missing, no orphaned/stale entries), and that every category value it uses is actually declared. Co-Authored-By: Claude Sonnet 5 --- crates/types/src/dag_export.rs | 192 +++++++++++++++++++++++++++++++++ tools/dag-viewer/node-style.js | 136 +++++++++++++++++------ 2 files changed, 297 insertions(+), 31 deletions(-) diff --git a/crates/types/src/dag_export.rs b/crates/types/src/dag_export.rs index 252d319a..be1f1550 100644 --- a/crates/types/src/dag_export.rs +++ b/crates/types/src/dag_export.rs @@ -1415,4 +1415,196 @@ mod tests { on the Aggregate subtree it represents, not just the root" ); } + + // ── Issue #187: tools/dag-viewer/node-style.js stays in sync ─────────── + // + // node-style.js's `KIND_CATEGORY` table is hand-maintained JS, not + // generated from this file, so nothing stops it drifting from the real + // `&'static str` kind tags below the moment a `QueryExpr`/`SummaryExpr` + // operator variant is added, renamed, or removed. That's exactly how it + // drifted before #187: the table was keyed on names from an old design + // doc (`InfoJoin`, `LetBinding`/`Ref`, `Window`/`WindowFunc`, `Distinct`, + // `Merge`) that don't match what `build_no_recheck`/`summary_shape` + // actually emit today, so most of those entries were dead and several + // real kinds (`PromqlRelabel`, `PromqlInfoEnrich`, `PromqlSeriesSample`, + // `Concat`, `Dedup`, `SQLWindowFunc`, `CurrentTimestamp`, …) had no entry + // at all and silently fell back to the `derive` category. This test + // parses node-style.js's own source and checks its `KIND_CATEGORY` keys + // against the literal kind list below — kept exhaustive by the `other @ + // (...)` unreachable arm in `build_no_recheck` above (a new operator + // variant fails to compile there until it's given a `push_node` call, + // which is this list's own source of truth). + + /// Every `&'static str` kind tag [`build_no_recheck`] and + /// [`summary_shape`]/[`build_summary`] can push onto a [`DagNode`] / + /// [`SummaryDagNode`] — i.e. every legal value of `DagNode.kind` a + /// consumer (`tools/dag-viewer`) can actually see. Scalar `QueryExpr` + /// variants (`Column`, `Literal`, `Compare`, …) are deliberately absent: + /// `build_no_recheck`'s final `unreachable!` arm confirms they never + /// reach `push_node` on their own — they're always embedded as opaque + /// `detail` JSON inside an operator node instead (see that arm's doc). + const DAG_NODE_KINDS: &[&str] = &[ + // Pre-ASAP (QueryExpr operator variants; from build_no_recheck). + "Scan", + "PromqlScalarBridge", + "EvalTimestamp", + "CurrentTimestamp", + "PromqlVectorFromScalar", + "PromqlScalarFromVector", + "PromqlRelabel", + "PromqlInfoEnrich", + "PromqlSeriesSample", + "Filter", + "Project", + "Aggregate", + "Dedup", + "Concat", + "Join", + "SetOp", + "Sort", + "Limit", + "PromqlSubquery", + "TimeRange", + "TimeShift", + "SQLWindowFunc", + "BinaryOp", + // Post-ASAP (SummaryExpr variants; from summary_shape/build_summary). + "KeepPreAsap", + "SummaryAgg", + "SummaryJoin", + "SummarySubtract", + "SummaryDelete", + "SummaryEstimate", + "SummaryMerge", + ]; + + fn node_style_js_source() -> String { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../tools/dag-viewer/node-style.js" + ); + std::fs::read_to_string(path).unwrap_or_else(|e| panic!("failed to read {path}: {e}")) + } + + /// Slice `src` between a `"const {name} = {{"` marker and the next + /// top-level `"\n}};"` close (this file's object literals never nest a + /// `"\n};"`-shaped line, so the first occurrence after the marker is + /// always the matching close) — the body of that object literal. + fn object_literal_body<'a>(src: &'a str, name: &str) -> &'a str { + let marker = format!("const {name} = {{"); + let start = src + .find(&marker) + .unwrap_or_else(|| panic!("node-style.js has no `{marker}`")) + + marker.len(); + let end = src[start..].find("\n};").unwrap_or_else(|| { + panic!("node-style.js's `{name}` object literal never closes with `\\n}};`") + }); + &src[start..start + end] + } + + /// Parse `KIND_CATEGORY`'s `Kind: 'category',` entries (skipping comment + /// and blank lines) into `(kind, category)` pairs, in source order. + fn parse_kind_category(src: &str) -> Vec<(String, String)> { + object_literal_body(src, "KIND_CATEGORY") + .lines() + // Strip a trailing `// ...` line comment (this table annotates + // several entries that way) before trimming, so it never leaks + // into the parsed category value. + .map(|line| line.split("//").next().unwrap_or("").trim()) + .filter(|line| !line.is_empty()) + .map(|line| { + let (key, rest) = line + .split_once(':') + .unwrap_or_else(|| panic!("malformed KIND_CATEGORY line: {line:?}")); + let value = rest + .trim() + .trim_end_matches(',') + .trim_matches('\'') + .to_string(); + (key.trim().to_string(), value) + }) + .collect() + } + + /// Parse `CATEGORIES`'s top-level `name: {` keys (2-space-indented lines + /// opening a nested object; nested fields inside each category are + /// indented 4+ spaces, so this doesn't pick those up) into a `Vec` of + /// category names, in source order. + fn parse_category_names(src: &str) -> Vec { + object_literal_body(src, "CATEGORIES") + .lines() + .filter(|line| line.starts_with(" ") && !line.starts_with(" ")) + .filter_map(|line| { + let trimmed = line.trim(); + if trimmed.starts_with("//") { + return None; + } + trimmed + .strip_suffix('{') + .map(|prefix| prefix.trim().trim_end_matches(':').to_string()) + }) + .collect() + } + + #[test] + fn node_style_js_categorizes_every_dag_node_kind_exactly_once() { + let src = node_style_js_source(); + let entries = parse_kind_category(&src); + + let mut seen = std::collections::HashSet::new(); + for (kind, _) in &entries { + assert!( + seen.insert(kind.clone()), + "node-style.js's KIND_CATEGORY lists {kind:?} more than once" + ); + } + + let mapped: std::collections::HashSet<&str> = + entries.iter().map(|(k, _)| k.as_str()).collect(); + let canonical: std::collections::HashSet<&str> = DAG_NODE_KINDS.iter().copied().collect(); + + let missing: Vec<&&str> = DAG_NODE_KINDS + .iter() + .filter(|k| !mapped.contains(*k)) + .collect(); + assert!( + missing.is_empty(), + "node-style.js's KIND_CATEGORY is missing an entry for: {missing:?} — \ + every DagNode.kind dag_export.rs can produce must have an explicit \ + category, or it silently falls back to `derive` in the viewer" + ); + + let orphaned: Vec<&str> = mapped + .iter() + .filter(|k| !canonical.contains(*k)) + .copied() + .collect(); + assert!( + orphaned.is_empty(), + "node-style.js's KIND_CATEGORY has entries for kinds \ + dag_export.rs's build_no_recheck/summary_shape never produce: \ + {orphaned:?} — likely a stale name left over from a rename \ + (see issue #187)" + ); + } + + #[test] + fn node_style_js_every_kind_category_value_is_a_declared_category() { + let src = node_style_js_source(); + let entries = parse_kind_category(&src); + let categories: std::collections::HashSet = + parse_category_names(&src).into_iter().collect(); + assert!( + !categories.is_empty(), + "failed to parse any category name out of node-style.js's CATEGORIES object" + ); + + for (kind, category) in &entries { + assert!( + categories.contains(category), + "node-style.js maps {kind:?} to category {category:?}, which \ + CATEGORIES never declares" + ); + } + } } diff --git a/tools/dag-viewer/node-style.js b/tools/dag-viewer/node-style.js index e688a80b..b4e22303 100644 --- a/tools/dag-viewer/node-style.js +++ b/tools/dag-viewer/node-style.js @@ -7,34 +7,102 @@ // SummaryAgg, SummaryJoin, SummarySubtract, SummaryDelete, SummaryEstimate, // SummaryMerge) that appear in the post-ASAP lane — see the `summary` // category below. +// +// Issue #187: this table's kind list is kept in exact sync with the literal +// `&'static str` kind tags `build_no_recheck`/`summary_shape` emit in +// crates/types/src/dag_export.rs — NOT with the older `QueryExpr`-algebra +// design doc (old_docs/docs/l2-intent-algebra.md), which used names +// (`InfoJoin`, `Ref`/`LetBinding`, `Window`/`WindowFunc`, `Distinct`, +// `Merge`, plain `Sample`) that predate a rename/restructuring of the actual +// IR and no longer correspond to anything `DagNode.kind` produces at +// runtime. A kind name here that doesn't appear in dag_export.rs's `build_*` +// match arms is dead weight (or worse, silently wrong); a `DagNode.kind` +// dag_export.rs can emit that isn't a key here silently falls back to +// `derive` via `categoryOf`'s `||` — see the categorization rationale below +// for how each of the 23 pre-ASAP + 7 post-ASAP kinds was placed, especially +// the ones issue #187 called out by name. -// kind (DagNode.kind from crates/ir/src/dag_export.rs) -> category name. +// kind (DagNode.kind from crates/types/src/dag_export.rs) -> category name. const KIND_CATEGORY = { + // ── data — leaves that introduce a value, nothing flows in ───────────── Scan: 'data', - Ref: 'data', - Scalar: 'data', - EvalTime: 'data', + PromqlScalarBridge: 'data', // a scalar literal sitting in an operator-tree position (issue #220) — no QueryExpr child of its own; a leaf like Scan, not a transform. + EvalTimestamp: 'data', + CurrentTimestamp: 'data', + + // ── filter — narrows rows by a boolean predicate ──────────────────────── Filter: 'filter', - Sample: 'filter', + + // ── sample — narrows *series*, but not by a predicate (issue #187) ────── + // PromqlSeriesSample (`limitk`/`limit_ratio`) keeps a deterministic subset + // of whole series per group. Its own doc is explicit that it is "not a + // ranking (TopK) and not a reduction" — and it's equally not a Filter: + // nothing here is a boolean predicate over row contents, it's a + // selection-by-quota. Grouping it with Filter (the old mapping) implied it + // narrows rows the same way a WHERE clause does, which overstates the + // similarity; giving it its own category keeps that distinction visible. + PromqlSeriesSample: 'sample', + + // ── derive — transforms or enriches columns; child's rows pass through 1:1 Project: 'derive', - Relabel: 'derive', - VectorFromScalar: 'derive', - ScalarFromVector: 'derive', - TimeShift: 'derive', + PromqlRelabel: 'derive', + // PromqlInfoEnrich (issue #187, was "InfoJoin" bucketed with `join`): it + // has exactly ONE QueryExpr child (`child`), unlike Join's two — the + // "other side" it enriches from (an info metric matched by `selector`) is + // never a QueryExpr/DagNode at all, it's resolved at runtime by the + // post-ASAP binder. So there is no second relation in this graph for it to + // "join" the way Join or SetOp genuinely combine two DAG inputs. What it + // actually does — graft extra label columns onto rows that pass through + // unchanged, same shape of operation as PromqlRelabel's column rewrite — + // is a `derive`, not a `join`. + PromqlInfoEnrich: 'derive', + PromqlVectorFromScalar: 'derive', + PromqlScalarFromVector: 'derive', BinaryOp: 'derive', - InfoJoin: 'join', - Join: 'join', + + // ── aggregate — groups and reduces (fewer rows out than in) ───────────── Aggregate: 'aggregate', - Window: 'window', - WindowFunc: 'window', - Subquery: 'window', + + // ── window — reads or positions a scoped window of rows/time ──────────── TimeRange: 'window', - Distinct: 'set', - Merge: 'set', + PromqlSubquery: 'window', + // TimeShift (issue #187 follow-up, not in the original 4 examples but + // caught by the "review every other kind too" instruction): the old + // mapping put it in `derive` ("transforms values"), but its own doc says + // it "moves *when* `child` is evaluated... but leaves its schema + // unchanged" — no column is transformed at all, only the temporal window + // the rest of the plan reads from shifts. That's the same "scoped window + // of time" concept TimeRange/PromqlSubquery represent, not a value + // derivation, so it belongs here instead. + TimeShift: 'window', + SQLWindowFunc: 'window', + + // ── join — genuinely combines two DAG inputs on a predicate ───────────── + Join: 'join', + + // ── set — enforces or computes set semantics on rows already gathered ─── + // Dedup (SQL DISTINCT, formerly labeled "Distinct" here) and SetOp + // (UNION/INTERSECT/EXCEPT) both make a relation behave like a *set* + // (eliminate duplicates, or combine two relations using set-theoretic + // membership) rather than an arbitrary bag operation. + Dedup: 'set', SetOp: 'set', + + // ── combine — n-ary fan-in with no set semantics (issue #187) ─────────── + // Concat (formerly labeled "Merge" here, and lumped into `set` with + // Dedup/SetOp) 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"). Lumping + // it with `set` implied it carries the same dedup/set-theoretic semantics + // SetOp and Dedup do, which is exactly backwards — Concat is pure + // branch-fan-in (ROLLUP/CUBE grouping-set branches, histogram_quantiles + // branches, sharded/fan-in plans), so it gets its own category instead. + Concat: 'combine', + + // ── sort — orders or caps rows, doesn't change which columns exist ────── Sort: 'sort', Limit: 'sort', - LetBinding: 'bind', + // Post-ASAP SummaryDagNode kinds (post-ASAP lane only). KeepPreAsap: 'summary', SummaryAgg: 'summary', @@ -49,19 +117,25 @@ const KIND_CATEGORY = { const CATEGORIES = { data: { label: 'Data', - description: 'Scan, Ref, Scalar, EvalTime — leaves that introduce a value', + description: 'Scan, PromqlScalarBridge, EvalTimestamp, CurrentTimestamp — leaves that introduce a value', light: { bg: '#eef5fd', border: '#0369a1' }, dark: { bg: '#0c2438', border: '#38bdf8' }, }, filter: { label: 'Filter', - description: 'Filter, Sample — narrows rows', + description: 'Filter — narrows rows by a boolean predicate', light: { bg: '#edf8ec', border: '#0f766e' }, dark: { bg: '#072a20', border: '#2dd4bf' }, }, + sample: { + label: 'Sample', + description: 'PromqlSeriesSample — keeps a deterministic subset of series by quota, not by predicate', + light: { bg: '#fff5ea', border: '#b45309' }, + dark: { bg: '#3a2408', border: '#fb923c' }, + }, derive: { label: 'Derive', - description: 'Project, Relabel, VectorFromScalar, ScalarFromVector, TimeShift, BinaryOp — transforms values', + description: 'Project, PromqlRelabel, PromqlInfoEnrich, PromqlVectorFromScalar, PromqlScalarFromVector, BinaryOp — transforms or enriches columns on otherwise-unchanged rows', light: { bg: '#f5f0fd', border: '#6d28d9' }, dark: { bg: '#241a3d', border: '#a78bfa' }, }, @@ -73,39 +147,39 @@ const CATEGORIES = { }, window: { label: 'Window', - description: 'Window, WindowFunc, Subquery, TimeRange — scopes over a time range', + description: 'TimeRange, PromqlSubquery, TimeShift, SQLWindowFunc — reads or positions a scoped window of rows/time around each row', light: { bg: '#fdf1f6', border: '#be185d' }, dark: { bg: '#3a1626', border: '#f472b6' }, }, join: { label: 'Join', - description: 'Join, InfoJoin — combines two inputs', + description: 'Join — combines two DAG inputs on a predicate', light: { bg: '#edf9f8', border: '#15803d' }, dark: { bg: '#08302c', border: '#4ade80' }, }, set: { label: 'Set', - description: 'Distinct, Merge, SetOp — dedup or combine branches', + description: 'Dedup, SetOp — enforces or computes set semantics (dedup rows, or union/intersect/except of two relations)', light: { bg: '#f1efff', border: '#4f46e5' }, dark: { bg: '#221f3d', border: '#a5b4fc' }, }, + combine: { + label: 'Combine', + description: 'Concat — concatenates n branches with no dedup (UNION ALL-shaped fan-in)', + light: { bg: '#f7fee7', border: '#4d7c0f' }, + dark: { bg: '#1a2e05', border: '#a3e635' }, + }, sort: { label: 'Sort', description: 'Sort, Limit — orders or caps rows', light: { bg: '#eef4fd', border: '#1d4ed8' }, dark: { bg: '#12233d', border: '#60a5fa' }, }, - bind: { - label: 'Bind', - description: 'LetBinding — names a sub-expression for reuse via Ref', - light: { bg: '#fff5ea', border: '#b45309' }, - dark: { bg: '#3a2408', border: '#fb923c' }, - }, // Post-ASAP only (post-ASAP lane): every other category // above is a saturated, hand-picked hue for a pre-ASAP QueryExpr operator. // `summary` is deliberately plain neutral gray instead of another hue — - // partly because a 10th saturated color starts getting hard to - // distinguish at a glance from its 9 neighbors (data's blue and sort's + // partly because an 11th saturated color starts getting hard to + // distinguish at a glance from its 10 neighbors (data's blue and sort's // blue are already close), and partly because "materialized post-ASAP // structure" reads better as a visually distinct *family* (muted, // grayscale) than as one more member of the pre-ASAP rainbow. KeepPreAsap From 8e8ea5b77b0575645896c9696c9e6cd756320e01 Mon Sep 17 00:00:00 2001 From: zz_y Date: Thu, 27 Aug 2026 09:32:36 -0600 Subject: [PATCH 2/2] fix(dag-viewer): close code-review gaps on the #187 node-category fix Addresses three follow-up items from review of the #187 PR: 1. .github/workflows/rust.yml only triggered on crates/**/Cargo.*, so the new dag_export.rs sync tests never ran on a JS-only node-style.js edit -- exactly the kind of change most likely to reintroduce the drift #187 was about. Added tools/dag-viewer/** to both the push and pull_request path filters. 2. crates/types/src/dag_export.rs's 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 DAG_NODE_KINDS (and from node-style.js) with every test still green. Fixed by making the kind string single-sourced instead of independently duplicated: - New 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: &'static str argument -- removed that parameter and the 23 literal arguments at its call sites in build_no_recheck. - summary_shape now derives its returned kind via summary_kind_tag(expr) instead of typing each variant's name out a second time in its own match arms. - The test module no longer hardcodes a canonical 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 -- the same functions production code calls -- so the string values have exactly one source of truth. This does not close the gap all the way: nothing forces the sample list to grow when a brand-new variant is added (that variant's own match arm in kind_tag/summary_kind_tag and build_no_recheck/summary_shape is compiler-enforced, but a new sample here is not). Fully closing that would need a proc-macro/derive that enumerates QueryExpr's variants (e.g. adding a strum dependency) or a third hand-maintained match whose only job is enumeration -- left as follow-up, noted in the PR description and in this file's own test-module comment. 3. node-style.js's categoryOf fell back to 'derive' silently for any unmapped kind -- the exact failure mode that let #187 go unnoticed for the table's whole life. Added a dedicated 'unknown' category (hatched red, distinct from every real category and from 'summary's neutral gray), a legend row for it, a console.warn from categoryOf when it fires, and a matching dashed/thicker-border cytoscape style in viewer.js so it reads as "needs attention" rather than blending in. Verified: cargo test -p asap-types (110 passed, including both node-style sync tests), cargo test -p asap-devtools --bin dag_export (6 passed), cargo check --workspace, cargo clippy -p asap-types --tests (clean), cargo fmt --check -p asap-types (clean), python3 -m unittest tools/dag- viewer/test_render.py (18 passed), and tools/dag-viewer/generate-sample.sh regenerates dag.example.json byte-identical to what's committed. Refs #187. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/rust.yml | 16 ++ crates/types/src/dag_export.rs | 465 +++++++++++++++++++++++---------- tools/dag-viewer/node-style.js | 42 ++- tools/dag-viewer/viewer.js | 10 + 4 files changed, 388 insertions(+), 145 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 3f948fa5..481df511 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -8,6 +8,14 @@ on: - 'Cargo.toml' - 'Cargo.lock' - '.github/workflows/rust.yml' + # crates/types/src/dag_export.rs has a #[cfg(test)] cargo test (issue + # #187) that reads tools/dag-viewer/node-style.js's own source and + # fails if its KIND_CATEGORY table drifts from the real DagNode.kind + # strings — a JS-only edit to that file needs this workflow to + # actually run `cargo test` for that guard to catch drift on the PR + # that introduces it, instead of surfacing later as a confusing + # failure on an unrelated Rust change. + - 'tools/dag-viewer/**' pull_request: types: [opened, synchronize, reopened, ready_for_review] branches: [ main ] @@ -16,6 +24,14 @@ on: - 'Cargo.toml' - 'Cargo.lock' - '.github/workflows/rust.yml' + # crates/types/src/dag_export.rs has a #[cfg(test)] cargo test (issue + # #187) that reads tools/dag-viewer/node-style.js's own source and + # fails if its KIND_CATEGORY table drifts from the real DagNode.kind + # strings — a JS-only edit to that file needs this workflow to + # actually run `cargo test` for that guard to catch drift on the PR + # that introduces it, instead of surfacing later as a confusing + # failure on an unrelated Rust change. + - 'tools/dag-viewer/**' workflow_dispatch: concurrency: diff --git a/crates/types/src/dag_export.rs b/crates/types/src/dag_export.rs index be1f1550..deefcf6d 100644 --- a/crates/types/src/dag_export.rs +++ b/crates/types/src/dag_export.rs @@ -356,7 +356,28 @@ fn family_label(family: &crate::post_asap::SummaryFamilyType) -> String { /// shared [`DagGraph`] node list — see [`export_post_asap`]) can't drift /// apart on how every *other* variant's own shape is described, since /// nothing about that description differs between the two. +/// The [`summary_shape`] counterpart of [`kind_tag`] — the single source +/// `summary_shape` derives its returned kind string from, and (via +/// `summary_shape`) what `build_summary`/`build_summary_hybrid` ultimately +/// give a `SummaryDagNode`/`DagNode`. See [`kind_tag`]'s doc for why this is +/// split out as its own exhaustive match rather than a literal repeated in +/// each of `summary_shape`'s match arms. +fn summary_kind_tag(expr: &SummaryExpr) -> &'static str { + match expr { + SummaryExpr::KeepPreAsap(_) => { + unreachable!("summary_kind_tag's callers special-case KeepPreAsap before calling it") + } + SummaryExpr::SummaryAgg { .. } => "SummaryAgg", + SummaryExpr::SummaryJoin { .. } => "SummaryJoin", + SummaryExpr::SummarySubtract { .. } => "SummarySubtract", + SummaryExpr::SummaryDelete { .. } => "SummaryDelete", + SummaryExpr::SummaryEstimate { .. } => "SummaryEstimate", + SummaryExpr::SummaryMerge { .. } => "SummaryMerge", + } +} + fn summary_shape(expr: &SummaryExpr) -> (&'static str, String, serde_json::Value) { + let kind = summary_kind_tag(expr); match expr { SummaryExpr::KeepPreAsap(_) => { unreachable!("summary_shape's callers special-case KeepPreAsap before calling it") @@ -375,7 +396,7 @@ fn summary_shape(expr: &SummaryExpr) -> (&'static str, String, serde_json::Value "reduction": reduction, "grouping": format!("{grouping:?}"), }); - ("SummaryAgg", label, detail) + (kind, label, detail) } SummaryExpr::SummaryJoin { key, family, .. } => { let label = format!("SummaryJoin({})", family_label(family)); @@ -383,25 +404,23 @@ fn summary_shape(expr: &SummaryExpr) -> (&'static str, String, serde_json::Value "key": key, "family": format!("{family:?}"), }); - ("SummaryJoin", label, detail) + (kind, label, detail) + } + SummaryExpr::SummarySubtract { .. } => { + (kind, "SummarySubtract".into(), serde_json::json!({})) } - SummaryExpr::SummarySubtract { .. } => ( - "SummarySubtract", - "SummarySubtract".into(), - serde_json::json!({}), - ), SummaryExpr::SummaryDelete { key, .. } => { let detail = serde_json::json!({ "key": key }); - ("SummaryDelete", "SummaryDelete".into(), detail) + (kind, "SummaryDelete".into(), detail) } SummaryExpr::SummaryEstimate { query, .. } => { let label = format!("SummaryEstimate({query:?})"); let detail = serde_json::json!({ "query": format!("{query:?}") }); - ("SummaryEstimate", label, detail) + (kind, label, detail) } SummaryExpr::SummaryMerge { children } => { let label = format!("SummaryMerge({} children)", children.len()); - ("SummaryMerge", label, serde_json::json!({})) + (kind, label, serde_json::json!({})) } } } @@ -629,17 +648,80 @@ fn deduplicate_pointer_shared_nodes(nodes: Vec, root: u32) -> DagGraph } } +/// The `&'static str` kind tag for one pre-ASAP `QueryExpr` *operator* node — +/// the single source [`push_node`] derives `DagNode.kind` from, and the exact +/// value `tools/dag-viewer/node-style.js`'s `KIND_CATEGORY` table must have +/// an entry for (see the `dag_export.rs`-and-`node-style.js`-sync tests in +/// this file's own `#[cfg(test)]` module, issue #187). Kept as its own +/// function — rather than a `kind: &'static str` argument threaded through +/// every [`push_node`] call site in [`build_no_recheck`] — so there is +/// exactly one place a kind string is written per variant, not two (a match +/// arm choosing *how* to build the node, plus a separately-typed-out literal +/// choosing its name); a rename here is a rename in one spot, not a +/// find-and-replace across every call site. +/// +/// Exhaustive over every *operator* variant. The scalar variants (`Column`, +/// `Literal`, …) never reach this function in production — +/// `build_no_recheck`'s own `unreachable!` arm is what actually enforces +/// that for [`push_node`]'s callers (see that arm's doc) — but this match is +/// written exhaustively over the *whole* `QueryExpr` enum anyway (rather +/// than a wildcard `_ => unreachable!()`) so that adding a new variant of +/// either kind fails to compile here until it's explicitly placed, the same +/// guarantee `build_no_recheck`'s match already gives. +fn kind_tag(expr: &QueryExpr) -> &'static str { + match expr { + QueryExpr::Scan { .. } => "Scan", + QueryExpr::PromqlScalarBridge(_) => "PromqlScalarBridge", + QueryExpr::EvalTimestamp => "EvalTimestamp", + QueryExpr::CurrentTimestamp => "CurrentTimestamp", + QueryExpr::PromqlVectorFromScalar(_) => "PromqlVectorFromScalar", + QueryExpr::PromqlScalarFromVector(_) => "PromqlScalarFromVector", + QueryExpr::PromqlRelabel { .. } => "PromqlRelabel", + QueryExpr::PromqlInfoEnrich { .. } => "PromqlInfoEnrich", + QueryExpr::PromqlSeriesSample { .. } => "PromqlSeriesSample", + QueryExpr::Filter { .. } => "Filter", + QueryExpr::Project { .. } => "Project", + QueryExpr::Aggregate { .. } => "Aggregate", + QueryExpr::Dedup { .. } => "Dedup", + QueryExpr::Concat { .. } => "Concat", + QueryExpr::Join { .. } => "Join", + QueryExpr::SetOp { .. } => "SetOp", + QueryExpr::Sort { .. } => "Sort", + QueryExpr::Limit { .. } => "Limit", + QueryExpr::PromqlSubquery { .. } => "PromqlSubquery", + QueryExpr::TimeRange { .. } => "TimeRange", + QueryExpr::TimeShift { .. } => "TimeShift", + QueryExpr::SQLWindowFunc { .. } => "SQLWindowFunc", + QueryExpr::BinaryOp { .. } => "BinaryOp", + other @ (QueryExpr::Column(_) + | QueryExpr::Literal(_) + | QueryExpr::Compare { .. } + | QueryExpr::BoolAnd(_) + | QueryExpr::BoolOr(_) + | QueryExpr::Not(_) + | QueryExpr::IsNull(_) + | QueryExpr::IsNotNull(_) + | QueryExpr::Cast { .. } + | QueryExpr::InList { .. } + | QueryExpr::FunctionCall { .. } + | QueryExpr::Arithmetic { .. } + | QueryExpr::Case { .. }) => { + unreachable!("kind_tag reached a scalar QueryExpr variant directly: {other:?}") + } + } +} + /// Push one flattened node for `expr`. `expr` is the *whole* subtree this /// node represents (not just its own fields) — `hash` is /// [`structural_hash(expr)`](structural_hash), the identical function and /// the identical input `InternTable::intern` would hash for this same /// subtree, so this node's `hash` matches what `cse::share_common_subtrees` -/// would bucket it under. +/// would bucket it under. `kind` is [`kind_tag(expr)`](kind_tag), not a +/// caller-supplied argument — see that function's doc for why. fn push_node( nodes: &mut Vec, expr: &QueryExpr, cache: &mut HashCache, - kind: &'static str, label: String, detail: serde_json::Value, children: Vec, @@ -648,7 +730,7 @@ fn push_node( let hash = Some(structural_hash(expr, cache)); nodes.push(DagNode { id, - kind, + kind: kind_tag(expr), label, detail, schema: expr @@ -844,7 +926,7 @@ fn build_no_recheck( "predicates": predicates, "schema": schema, }); - push_node(nodes, expr, cache, "Scan", label, detail, vec![]) + push_node(nodes, expr, cache, label, detail, vec![]) } // The bridged child is a scalar-sub-language node (issue #220), not // an operator node `build` can recurse into — serialize it as opaque @@ -857,7 +939,6 @@ fn build_no_recheck( nodes, expr, cache, - "PromqlScalarBridge", format!("PromqlScalarBridge({inner:?})"), detail, vec![], @@ -867,7 +948,6 @@ fn build_no_recheck( nodes, expr, cache, - "EvalTimestamp", "EvalTimestamp".into(), serde_json::json!({}), vec![], @@ -876,7 +956,6 @@ fn build_no_recheck( nodes, expr, cache, - "CurrentTimestamp", "CurrentTimestamp".into(), serde_json::json!({}), vec![], @@ -887,7 +966,6 @@ fn build_no_recheck( nodes, expr, cache, - "PromqlVectorFromScalar", "vector()".into(), serde_json::json!({}), vec![c], @@ -899,7 +977,6 @@ fn build_no_recheck( nodes, expr, cache, - "PromqlScalarFromVector", "scalar()".into(), serde_json::json!({}), vec![c], @@ -912,7 +989,6 @@ fn build_no_recheck( nodes, expr, cache, - "PromqlRelabel", format!("PromqlRelabel(dst={dst})"), detail, vec![c], @@ -925,7 +1001,6 @@ fn build_no_recheck( nodes, expr, cache, - "PromqlInfoEnrich", "PromqlInfoEnrich".into(), detail, vec![c], @@ -938,7 +1013,6 @@ fn build_no_recheck( nodes, expr, cache, - "PromqlSeriesSample", format!("PromqlSeriesSample({kind:?})"), detail, vec![c], @@ -947,15 +1021,7 @@ fn build_no_recheck( QueryExpr::Filter { pred, child } => { let c = build(child, nodes, cache, find_winner); let detail = serde_json::json!({ "pred": pred }); - push_node( - nodes, - expr, - cache, - "Filter", - "Filter".into(), - detail, - vec![c], - ) + push_node(nodes, expr, cache, "Filter".into(), detail, vec![c]) } QueryExpr::Project { cols, @@ -968,7 +1034,6 @@ fn build_no_recheck( nodes, expr, cache, - "Project", format!("Project({} cols)", cols.len()), detail, vec![c], @@ -992,7 +1057,6 @@ fn build_no_recheck( nodes, expr, cache, - "Aggregate", format!("Aggregate({} measures)", measures.len()), detail, vec![c], @@ -1005,7 +1069,6 @@ fn build_no_recheck( nodes, expr, cache, - "Dedup", format!("Dedup({} cols)", cols.len()), detail, vec![c], @@ -1017,15 +1080,7 @@ fn build_no_recheck( .map(|c| build(c, nodes, cache, find_winner)) .collect(); let label = format!("Concat({} branches)", ids.len()); - push_node( - nodes, - expr, - cache, - "Concat", - label, - serde_json::json!({}), - ids, - ) + push_node(nodes, expr, cache, label, serde_json::json!({}), ids) } QueryExpr::Join { kind, @@ -1040,7 +1095,6 @@ fn build_no_recheck( nodes, expr, cache, - "Join", format!("Join({kind:?})"), detail, vec![l, r], @@ -1059,7 +1113,6 @@ fn build_no_recheck( nodes, expr, cache, - "SetOp", format!("SetOp({kind:?})"), detail, vec![l, r], @@ -1076,7 +1129,6 @@ fn build_no_recheck( nodes, expr, cache, - "Sort", format!("Sort({} keys)", keys.len()), detail, vec![c], @@ -1085,15 +1137,7 @@ fn build_no_recheck( QueryExpr::Limit { n, offset, child } => { let c = build(child, nodes, cache, find_winner); let detail = serde_json::json!({ "n": n, "offset": offset }); - push_node( - nodes, - expr, - cache, - "Limit", - format!("Limit({n})"), - detail, - vec![c], - ) + push_node(nodes, expr, cache, format!("Limit({n})"), detail, vec![c]) } QueryExpr::PromqlSubquery { range, @@ -1102,15 +1146,7 @@ fn build_no_recheck( } => { let c = build(child, nodes, cache, find_winner); let detail = serde_json::json!({ "range": range, "resolution": resolution }); - push_node( - nodes, - expr, - cache, - "PromqlSubquery", - "PromqlSubquery".into(), - detail, - vec![c], - ) + push_node(nodes, expr, cache, "PromqlSubquery".into(), detail, vec![c]) } QueryExpr::TimeRange { range, child } => { let c = build(child, nodes, cache, find_winner); @@ -1119,7 +1155,6 @@ fn build_no_recheck( nodes, expr, cache, - "TimeRange", format!("TimeRange({range:?})"), detail, vec![c], @@ -1128,15 +1163,7 @@ fn build_no_recheck( QueryExpr::TimeShift { shift, child } => { let c = build(child, nodes, cache, find_winner); let detail = serde_json::json!({ "shift": shift }); - push_node( - nodes, - expr, - cache, - "TimeShift", - "TimeShift".into(), - detail, - vec![c], - ) + push_node(nodes, expr, cache, "TimeShift".into(), detail, vec![c]) } QueryExpr::SQLWindowFunc { func, @@ -1160,7 +1187,6 @@ fn build_no_recheck( nodes, expr, cache, - "SQLWindowFunc", format!("SQLWindowFunc({func:?})"), detail, vec![c], @@ -1179,7 +1205,6 @@ fn build_no_recheck( nodes, expr, cache, - "BinaryOp", format!("BinaryOp({op})"), detail, vec![l, r], @@ -1210,7 +1235,10 @@ mod tests { use super::*; use crate::pre_asap::agg_intent::AggIntent; use crate::pre_asap::expr_ir::ScalarValue; - use crate::pre_asap::query_expr::{GroupKeys, Predicate, Reduction}; + use crate::pre_asap::query_expr::{ + BinaryOpKind, GroupKeys, JoinKind, Predicate, Reduction, SampleKind, SetOpKind, TimeShift, + WindowFuncKind, + }; use crate::pre_asap::schema::{Column, DataType, Schema}; use crate::types::AccuracyTarget; @@ -1420,63 +1448,225 @@ mod tests { // // node-style.js's `KIND_CATEGORY` table is hand-maintained JS, not // generated from this file, so nothing stops it drifting from the real - // `&'static str` kind tags below the moment a `QueryExpr`/`SummaryExpr` - // operator variant is added, renamed, or removed. That's exactly how it - // drifted before #187: the table was keyed on names from an old design - // doc (`InfoJoin`, `LetBinding`/`Ref`, `Window`/`WindowFunc`, `Distinct`, - // `Merge`) that don't match what `build_no_recheck`/`summary_shape` - // actually emit today, so most of those entries were dead and several - // real kinds (`PromqlRelabel`, `PromqlInfoEnrich`, `PromqlSeriesSample`, - // `Concat`, `Dedup`, `SQLWindowFunc`, `CurrentTimestamp`, …) had no entry - // at all and silently fell back to the `derive` category. This test - // parses node-style.js's own source and checks its `KIND_CATEGORY` keys - // against the literal kind list below — kept exhaustive by the `other @ - // (...)` unreachable arm in `build_no_recheck` above (a new operator - // variant fails to compile there until it's given a `push_node` call, - // which is this list's own source of truth). - - /// Every `&'static str` kind tag [`build_no_recheck`] and - /// [`summary_shape`]/[`build_summary`] can push onto a [`DagNode`] / - /// [`SummaryDagNode`] — i.e. every legal value of `DagNode.kind` a - /// consumer (`tools/dag-viewer`) can actually see. Scalar `QueryExpr` - /// variants (`Column`, `Literal`, `Compare`, …) are deliberately absent: - /// `build_no_recheck`'s final `unreachable!` arm confirms they never - /// reach `push_node` on their own — they're always embedded as opaque - /// `detail` JSON inside an operator node instead (see that arm's doc). - const DAG_NODE_KINDS: &[&str] = &[ - // Pre-ASAP (QueryExpr operator variants; from build_no_recheck). - "Scan", - "PromqlScalarBridge", - "EvalTimestamp", - "CurrentTimestamp", - "PromqlVectorFromScalar", - "PromqlScalarFromVector", - "PromqlRelabel", - "PromqlInfoEnrich", - "PromqlSeriesSample", - "Filter", - "Project", - "Aggregate", - "Dedup", - "Concat", - "Join", - "SetOp", - "Sort", - "Limit", - "PromqlSubquery", - "TimeRange", - "TimeShift", - "SQLWindowFunc", - "BinaryOp", - // Post-ASAP (SummaryExpr variants; from summary_shape/build_summary). - "KeepPreAsap", - "SummaryAgg", - "SummaryJoin", - "SummarySubtract", - "SummaryDelete", - "SummaryEstimate", - "SummaryMerge", - ]; + // `&'static str` kind tags [`kind_tag`]/[`summary_kind_tag`] produce the + // moment a `QueryExpr`/`SummaryExpr` operator variant is added, renamed, + // or removed. That's exactly how it drifted before #187: the table was + // keyed on names from an old design doc (`InfoJoin`, `LetBinding`/`Ref`, + // `Window`/`WindowFunc`, `Distinct`, `Merge`) that don't match what + // `push_node`/`push_summary_node` actually emit today, so most of those + // entries were dead and several real kinds (`PromqlRelabel`, + // `PromqlInfoEnrich`, `PromqlSeriesSample`, `Concat`, `Dedup`, + // `SQLWindowFunc`, `CurrentTimestamp`, …) had no entry at all and + // silently fell back to the `derive` category. + // + // The canonical kind set the tests below check node-style.js against is + // *not* a third hand-copied list of string literals — that would only + // move the exact same staleness risk into this file instead of fixing + // it (a literal changed at a `kind_tag`/`summary_kind_tag` match arm + // with no matching edit to a separate list here would leave these tests + // green while node-style.js silently drifts, same as before #187). + // Instead, [`canonical_dag_node_kinds`] constructs one representative + // sample `QueryExpr`/`SummaryExpr` per operator variant and reads back + // the actual [`kind_tag`]/[`summary_kind_tag`] result for it — the exact + // same functions [`push_node`]/[`summary_shape`] call in production — so + // the string values themselves have exactly one source of truth. + // + // This does **not** close the gap all the way: nothing forces + // `canonical_dag_node_kinds`'s own sample list to grow when a *new* + // `QueryExpr`/`SummaryExpr` variant is added — that variant would still + // need its own match arm in `kind_tag`/`summary_kind_tag` (enforced at + // compile time, since those matches are exhaustive) and in + // `build_no_recheck`/`summary_shape` (likewise enforced), but adding + // that arm alone compiles fine without a new sample here or a new + // node-style.js entry, so the *presence* check below can still miss a + // brand-new kind silently. Closing that fully would need either a + // proc-macro/derive that enumerates `QueryExpr`'s variants automatically + // (a new dependency, e.g. `strum`) or a third hand-maintained match + // whose only job is enumeration — judged out of scope for this pass; see + // the PR description for this as flagged follow-up work. + + fn dummy_child() -> Rc { + Rc::new(QueryExpr::EvalTimestamp) + } + + fn dummy_predicate() -> Predicate { + Predicate(Rc::new(QueryExpr::Literal(ScalarValue::Boolean(true)))) + } + + fn dummy_summary_leaf() -> Rc { + Rc::new(SummaryNode { + expr: SummaryExpr::KeepPreAsap(dummy_child()), + schema: crate::post_asap::SummarySchema { + fields: vec![], + time_index: None, + }, + }) + } + + /// One representative `QueryExpr` per *operator* variant — deliberately + /// semantically nonsensical in places (e.g. `Filter` over a non-boolean + /// child): these values only ever reach [`kind_tag`], which doesn't + /// inspect field values, so realism doesn't matter, only that each + /// variant is constructed once. + fn sample_query_exprs() -> Vec { + vec![ + QueryExpr::Scan { + source: Source::Table { + table_ref: "t".into(), + }, + predicates: vec![], + schema: Schema { + columns: vec![], + time_index: None, + unique_keys: vec![], + closed: true, + }, + }, + QueryExpr::promql_scalar(1.0), + QueryExpr::EvalTimestamp, + QueryExpr::CurrentTimestamp, + QueryExpr::PromqlVectorFromScalar(dummy_child()), + QueryExpr::PromqlScalarFromVector(dummy_child()), + QueryExpr::PromqlRelabel { + dst: "d".into(), + value: dummy_child(), + child: dummy_child(), + }, + QueryExpr::PromqlInfoEnrich { + selector: vec![], + child: dummy_child(), + }, + QueryExpr::PromqlSeriesSample { + by: GroupKeys::none(), + kind: SampleKind::LimitK(1), + child: dummy_child(), + }, + QueryExpr::Filter { + pred: dummy_predicate(), + child: dummy_child(), + }, + QueryExpr::Project { + cols: vec![], + qualifier: None, + child: dummy_child(), + }, + QueryExpr::Aggregate { + reduction: Reduction::Reduce(GroupKeys::none()), + measures: vec![], + output_names: vec![], + having: None, + child: dummy_child(), + }, + QueryExpr::Dedup { + cols: vec![], + child: dummy_child(), + }, + QueryExpr::Concat { + children: vec![QueryExpr::EvalTimestamp, QueryExpr::EvalTimestamp], + }, + QueryExpr::Join { + kind: JoinKind::Inner, + pred: dummy_predicate(), + left: dummy_child(), + right: dummy_child(), + }, + QueryExpr::SetOp { + kind: SetOpKind::Union, + all: false, + left: dummy_child(), + right: dummy_child(), + }, + QueryExpr::Sort { + keys: vec![], + partition_by: GroupKeys::none(), + child: dummy_child(), + }, + QueryExpr::Limit { + n: 1, + offset: 0, + child: dummy_child(), + }, + QueryExpr::PromqlSubquery { + range: std::time::Duration::from_secs(60), + resolution: None, + child: dummy_child(), + }, + QueryExpr::TimeRange { + range: std::time::Duration::from_secs(60), + child: dummy_child(), + }, + QueryExpr::TimeShift { + shift: TimeShift::default(), + child: dummy_child(), + }, + QueryExpr::SQLWindowFunc { + func: WindowFuncKind::RowNumber, + args: vec![], + partition_by: GroupKeys::none(), + order_by: vec![], + frame: None, + output_name: "rn".into(), + child: dummy_child(), + }, + QueryExpr::BinaryOp { + op: BinaryOpKind::And, + lhs: dummy_child(), + rhs: dummy_child(), + vector_match: None, + }, + ] + } + + /// One representative `SummaryExpr` per non-`KeepPreAsap` variant (that + /// variant is excluded the same way [`summary_kind_tag`] excludes it — + /// it never reaches `summary_kind_tag`/`summary_shape` in production + /// either, see [`build_summary`]'s own `KeepPreAsap` special case). + fn sample_summary_exprs() -> Vec { + vec![ + SummaryExpr::SummaryAgg { + child: dummy_summary_leaf(), + family: crate::post_asap::SummaryFamilyType::Plain(DataType::Int64), + col: crate::pre_asap::expr_ir::ColumnRef::Named("v".into()), + reduction: Reduction::by(vec![]), + grouping: crate::post_asap::sketch::GroupingStrategy::default(), + }, + SummaryExpr::SummaryJoin { + outer: dummy_summary_leaf(), + inner: dummy_summary_leaf(), + key: crate::pre_asap::expr_ir::ColumnRef::Named("k".into()), + family: crate::post_asap::SummaryFamilyType::Plain(DataType::Int64), + }, + SummaryExpr::SummarySubtract { + left: dummy_summary_leaf(), + right: dummy_summary_leaf(), + }, + SummaryExpr::SummaryDelete { + summary_input: dummy_summary_leaf(), + key: crate::pre_asap::expr_ir::ColumnRef::Named("k".into()), + }, + SummaryExpr::SummaryEstimate { + summary_input: dummy_summary_leaf(), + query: crate::post_asap::sketch::SketchQuery::Cardinality, + }, + SummaryExpr::SummaryMerge { + children: vec![dummy_summary_leaf(), dummy_summary_leaf()], + }, + ] + } + + /// The real, current set of every `&'static str` [`DagNode`]/ + /// [`SummaryDagNode`] `.kind` value production code can produce — + /// `KeepPreAsap` plus [`kind_tag`] and [`summary_kind_tag`] applied to + /// one sample of every operator variant. See the module comment above + /// this function for exactly what "single source of truth" this does + /// and doesn't guarantee. + fn canonical_dag_node_kinds() -> std::collections::HashSet<&'static str> { + let mut kinds: std::collections::HashSet<&'static str> = + sample_query_exprs().iter().map(kind_tag).collect(); + kinds.extend(sample_summary_exprs().iter().map(summary_kind_tag)); + kinds.insert("KeepPreAsap"); + kinds + } fn node_style_js_source() -> String { let path = concat!( @@ -1561,12 +1751,9 @@ mod tests { let mapped: std::collections::HashSet<&str> = entries.iter().map(|(k, _)| k.as_str()).collect(); - let canonical: std::collections::HashSet<&str> = DAG_NODE_KINDS.iter().copied().collect(); + let canonical = canonical_dag_node_kinds(); - let missing: Vec<&&str> = DAG_NODE_KINDS - .iter() - .filter(|k| !mapped.contains(*k)) - .collect(); + let missing: Vec<&&str> = canonical.iter().filter(|k| !mapped.contains(*k)).collect(); assert!( missing.is_empty(), "node-style.js's KIND_CATEGORY is missing an entry for: {missing:?} — \ diff --git a/tools/dag-viewer/node-style.js b/tools/dag-viewer/node-style.js index b4e22303..8be5197f 100644 --- a/tools/dag-viewer/node-style.js +++ b/tools/dag-viewer/node-style.js @@ -17,10 +17,12 @@ // IR and no longer correspond to anything `DagNode.kind` produces at // runtime. A kind name here that doesn't appear in dag_export.rs's `build_*` // match arms is dead weight (or worse, silently wrong); a `DagNode.kind` -// dag_export.rs can emit that isn't a key here silently falls back to -// `derive` via `categoryOf`'s `||` — see the categorization rationale below -// for how each of the 23 pre-ASAP + 7 post-ASAP kinds was placed, especially -// the ones issue #187 called out by name. +// dag_export.rs can emit that isn't a key here renders as the dedicated +// `unknown` category (console.warn'd, hatched red) via `categoryOf`, rather +// than silently guessing `derive` the way it used to — see CATEGORIES.unknown +// and categoryOf's own comments. See the categorization rationale below for +// how each of the 23 pre-ASAP + 7 post-ASAP kinds was placed, especially the +// ones issue #187 called out by name. // kind (DagNode.kind from crates/types/src/dag_export.rs) -> category name. const KIND_CATEGORY = { @@ -193,6 +195,25 @@ const CATEGORIES = { light: { bg: '#f1f2f4', border: '#4b5563' }, dark: { bg: '#20242b', border: '#9ca3af' }, }, + // Not a real semantic bucket — the fallback `categoryOf` returns for a + // `DagNode.kind` that isn't a KIND_CATEGORY key at all (issue #187 follow- + // up). Before this category existed, that case silently rendered as + // `derive` with zero visual indication anything was wrong — exactly how + // #187's own staleness went unnoticed for the table's entire life. Given + // a rust-side sync test now guards KIND_CATEGORY against every kind + // `dag_export.rs` can currently produce, this should only ever fire for a + // hand-edited/malformed fixture or an older/newer JSON than this viewer + // version expects — but if it does, it needs to be *loud*, not another + // silent `derive`. Rendered hatched (dashed border, like the `data`/ + // `KeepPreAsap` treatment in viewer.js's buildCyStyle) plus a red-leaning + // border so it reads as "needs attention," distinct from every genuine + // category's saturated hue and from `summary`'s neutral gray. + unknown: { + label: 'Unknown kind', + description: 'A DagNode.kind with no KIND_CATEGORY entry — update node-style.js', + light: { bg: '#fef2f2', border: '#b91c1c' }, + dark: { bg: '#2a1212', border: '#f87171' }, + }, }; const ROOT_BADGE = { @@ -207,11 +228,20 @@ function isDarkMode() { } function categoryOf(kind) { - return KIND_CATEGORY[kind] || 'derive'; + const category = KIND_CATEGORY[kind]; + if (category) return category; + // See CATEGORIES.unknown's own comment: this used to fall back to + // 'derive' silently, which is exactly what let issue #187 go unnoticed. + // Warn loudly instead of guessing a plausible-looking category. + console.warn( + `node-style.js: DagNode.kind ${JSON.stringify(kind)} has no KIND_CATEGORY entry — ` + + 'rendering as "Unknown kind" instead of silently guessing. Add an entry to KIND_CATEGORY.' + ); + return 'unknown'; } function categoryColors(name) { - const cat = CATEGORIES[name] || CATEGORIES.derive; + const cat = CATEGORIES[name] || CATEGORIES.unknown; return isDarkMode() ? cat.dark : cat.light; } diff --git a/tools/dag-viewer/viewer.js b/tools/dag-viewer/viewer.js index feca4f8c..a8e973ea 100644 --- a/tools/dag-viewer/viewer.js +++ b/tools/dag-viewer/viewer.js @@ -211,6 +211,16 @@ function buildCyStyle() { style: { 'corner-radius': 999, 'border-style': 'dashed' }, }, ...categoryStyles, + { + // Issue #187 follow-up: node-style.js's `unknown` category is the + // loud fallback `categoryOf` returns for a `DagNode.kind` with no + // KIND_CATEGORY entry (console.warn'd there too) — give it a visibly + // "flagged" hatched/dashed border on top of its already-distinct red + // fill from CATEGORIES.unknown, so it doesn't read as just another + // saturated category color at a glance. + selector: 'node[category = "unknown"]', + style: { 'border-style': 'dashed', 'border-width': 3 }, + }, { // KeepPreAsap (post-ASAP lane only) is post-ASAP-only // as a *kind*, but represents literally unchanged pre-ASAP content —