diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 3f948fa..481df51 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 252d319..deefcf6 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; @@ -1415,4 +1443,355 @@ 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 [`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!( + 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 = canonical_dag_node_kinds(); + + 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:?} — \ + 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 e688a80..8be5197 100644 --- a/tools/dag-viewer/node-style.js +++ b/tools/dag-viewer/node-style.js @@ -7,34 +7,104 @@ // 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 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/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 +119,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 +149,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 @@ -119,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 = { @@ -133,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 feca4f8..a8e973e 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 —