diff --git a/crates/sqlbuild-cli/src/native_commands/_helpers/execution.rs b/crates/sqlbuild-cli/src/native_commands/_helpers/execution.rs index b7ea9fa268..a7a8b314c4 100644 --- a/crates/sqlbuild-cli/src/native_commands/_helpers/execution.rs +++ b/crates/sqlbuild-cli/src/native_commands/_helpers/execution.rs @@ -980,3 +980,8 @@ pub(crate) fn run_command( output, )) } + +/// The `{"error": ...}` report a location sends when it has no plan to show. +pub(crate) fn plan_failure(message: &str) -> serde_json::Value { + json!({"error": {"code": "plan_failed", "message": message, "help": null}}) +} diff --git a/crates/sqlbuild-cli/src/native_commands/constants.rs b/crates/sqlbuild-cli/src/native_commands/constants.rs index bd2eb9b498..6406a3a58b 100644 --- a/crates/sqlbuild-cli/src/native_commands/constants.rs +++ b/crates/sqlbuild-cli/src/native_commands/constants.rs @@ -106,3 +106,6 @@ pub(crate) const PASSWORD_INPUT_BYTES: u64 = 4096; /// The command a location runs to answer a plan request. pub(crate) const PLAN_COMMAND: &str = "plan"; +/// How much `sqb plan --json` output a location reads; model diffs over the server's cap are dropped. +pub(crate) const PLAN_OUTPUT_BYTES: usize = + 8 * sqlbuild_orchestration::platform::constants::PLAN_DOCUMENT_BYTES; diff --git a/crates/sqlbuild-cli/src/native_commands/main/_location_plan.rs b/crates/sqlbuild-cli/src/native_commands/main/_location_plan.rs index aee88ba2d4..0f6ed68932 100644 --- a/crates/sqlbuild-cli/src/native_commands/main/_location_plan.rs +++ b/crates/sqlbuild-cli/src/native_commands/main/_location_plan.rs @@ -5,15 +5,16 @@ use std::future::Future; use std::path::Path; use std::process::Stdio; -use serde_json::{Value, json}; -use sqlbuild_orchestration::platform::constants::PLAN_DOCUMENT_BYTES; +use serde_json::Value; use sqlbuild_orchestration::store::errors::ControlError; use tokio::io::AsyncReadExt; use tokio::process::Child; use crate::argv::main::accepted_settings_arguments::accepted_settings_arguments; use crate::native_commands::_helpers::execution::location_child; -use crate::native_commands::constants::PLAN_COMMAND; +use crate::native_commands::_helpers::execution::plan_failure; +use crate::native_commands::constants::{PLAN_COMMAND, PLAN_OUTPUT_BYTES}; +use crate::native_commands::main::_plan_document::plan_document; use crate::native_commands::models::LocationServe; /// The plan document, an `{"error": ...}` report when planning failed, or `None` when stopped first. @@ -24,14 +25,14 @@ pub(super) async fn plan_location( ) -> Result, ControlError> { let mut child = match plan_child((options, script), (selection, settings)) { Ok(child) => child?, - Err(message) => return Ok(Some(failure(&message))), + Err(message) => return Ok(Some(plan_failure(&message))), }; let stdout = child .stdout .take() .ok_or_else(|| ControlError::Invalid("plan output is unavailable".into()))?; let mut output: Vec = Vec::new(); - let limit = u64::try_from(PLAN_DOCUMENT_BYTES).unwrap_or(u64::MAX) + 1; + let limit = u64::try_from(PLAN_OUTPUT_BYTES).unwrap_or(u64::MAX) + 1; tokio::pin!(stopped); let status = tokio::select! { result = &mut stopped => { @@ -46,13 +47,10 @@ pub(super) async fn plan_location( child.wait().await } => result?, }; - if output.len() > PLAN_DOCUMENT_BYTES { - return Ok(Some(failure("the plan is too large to show"))); - } - Ok(Some(match serde_json::from_slice::(&output) { - Ok(document) if document.is_object() && matches!(status.code(), Some(0 | 1)) => document, - _ => failure("sqb plan did not produce a plan; see the location's log"), - })) + Ok(Some(plan_document( + &output, + matches!(status.code(), Some(0 | 1)), + ))) } /// The started child, or why the frozen request cannot become plan arguments. @@ -83,7 +81,3 @@ fn plan_child( .stdout(Stdio::piped()) .spawn()) } - -fn failure(message: &str) -> Value { - json!({"error": {"code": "plan_failed", "message": message, "help": null}}) -} diff --git a/crates/sqlbuild-cli/src/native_commands/main/_plan_document.rs b/crates/sqlbuild-cli/src/native_commands/main/_plan_document.rs new file mode 100644 index 0000000000..fd194c52cb --- /dev/null +++ b/crates/sqlbuild-cli/src/native_commands/main/_plan_document.rs @@ -0,0 +1,34 @@ +//! Reading `sqb plan --json` output as the plan a location sends to the server. + +use serde_json::Value; +use sqlbuild_orchestration::platform::constants::PLAN_DOCUMENT_BYTES; + +use crate::native_commands::_helpers::execution::plan_failure; +use crate::native_commands::constants::PLAN_OUTPUT_BYTES; + +/// The plan `sqb plan --json` printed, without model diffs when they would put it over the server's cap. +pub(in crate::native_commands) fn plan_document(output: &[u8], planned: bool) -> Value { + if output.len() > PLAN_OUTPUT_BYTES { + return plan_failure("the plan is too large to show"); + } + let mut document = match serde_json::from_slice::(output) { + Ok(document) if document.is_object() && planned => document, + _ => return plan_failure("sqb plan did not produce a plan; see the location's log"), + }; + if output.len() <= PLAN_DOCUMENT_BYTES { + return document; + } + let models = document.get_mut("models").and_then(Value::as_array_mut); + for model in models + .into_iter() + .flatten() + .filter_map(Value::as_object_mut) + { + model.remove("query_diff"); + model.remove("config_diff"); + } + match serde_json::to_vec(&document) { + Ok(bytes) if bytes.len() <= PLAN_DOCUMENT_BYTES => document, + _ => plan_failure("the plan is too large to show"), + } +} diff --git a/crates/sqlbuild-cli/src/native_commands/main/mod.rs b/crates/sqlbuild-cli/src/native_commands/main/mod.rs index d308f7d5b8..813d58cd45 100644 --- a/crates/sqlbuild-cli/src/native_commands/main/mod.rs +++ b/crates/sqlbuild-cli/src/native_commands/main/mod.rs @@ -4,6 +4,7 @@ pub(super) mod _location_metadata; pub(super) mod _location_plan; pub(super) mod _location_sensor; pub(super) mod _location_work; +pub(super) mod _plan_document; pub(super) mod _run_admin; pub(super) mod _run_control; pub(super) mod _run_cost; diff --git a/crates/sqlbuild-cli/src/native_commands/tests.rs b/crates/sqlbuild-cli/src/native_commands/tests.rs index 2f86f371e1..36ed1676b5 100644 --- a/crates/sqlbuild-cli/src/native_commands/tests.rs +++ b/crates/sqlbuild-cli/src/native_commands/tests.rs @@ -6,6 +6,8 @@ mod decision; mod helpers; #[path = "tests/test_init.rs"] mod init; +#[path = "tests/test_location_plan.rs"] +mod location_plan; #[path = "tests/test_paths.rs"] mod paths; #[path = "tests/test_project_facts.rs"] diff --git a/crates/sqlbuild-cli/src/native_commands/tests/helpers.rs b/crates/sqlbuild-cli/src/native_commands/tests/helpers.rs index e54ee1dd6d..ec817e0ae9 100644 --- a/crates/sqlbuild-cli/src/native_commands/tests/helpers.rs +++ b/crates/sqlbuild-cli/src/native_commands/tests/helpers.rs @@ -26,3 +26,12 @@ pub(super) fn write_tree(root: &Path, directories: &[&str], files: &[(&str, &str std::fs::write(root.join(relative), contents).expect("write file"); } } + +/// `sqb plan --json` output naming `count` changed models, each with `diff_line` as its query diff. +pub(super) fn plan_output(diff_line: &str, count: usize) -> Vec { + let models: Vec = (0..count) + .map(|i| serde_json::json!({"name": format!("model_{i}"), "reason": "query_changed", "query_diff": [diff_line], "config_diff": ["+a"]})) + .collect(); + serde_json::to_vec(&serde_json::json!({"selected_count": count, "models": models})) + .expect("plan output") +} diff --git a/crates/sqlbuild-cli/src/native_commands/tests/test_location_plan.rs b/crates/sqlbuild-cli/src/native_commands/tests/test_location_plan.rs new file mode 100644 index 0000000000..b74e5070e1 --- /dev/null +++ b/crates/sqlbuild-cli/src/native_commands/tests/test_location_plan.rs @@ -0,0 +1,39 @@ +use serde_json::{Value, json}; +use sqlbuild_orchestration::platform::constants::PLAN_DOCUMENT_BYTES; + +use crate::native_commands::main::_plan_document::plan_document; +use crate::native_commands::tests::helpers::plan_output; +use crate::native_commands::tests::test_types::PlanDocumentTestCase; + +#[test] +fn given_plan_output_when_reading_it_on_a_location_then_only_oversized_diffs_are_dropped() { + let test_cases = [ + PlanDocumentTestCase { + description: "a plan under the cap keeps its diffs", + output: plan_output("+SELECT 2", 2), + planned: true, + expected_first_model: json!({"name": "model_0", "reason": "query_changed", "query_diff": ["+SELECT 2"], "config_diff": ["+a"]}), + }, + PlanDocumentTestCase { + description: "diffs over the cap are dropped and the plan is kept", + output: plan_output(&"x".repeat(1024), PLAN_DOCUMENT_BYTES / 1024 + 1), + planned: true, + expected_first_model: json!({"name": "model_0", "reason": "query_changed"}), + }, + PlanDocumentTestCase { + description: "a failed sqb plan is not read as a plan", + output: plan_output("+SELECT 2", 1), + planned: false, + expected_first_model: Value::Null, + }, + ]; + for test_case in test_cases { + let document: Value = plan_document(&test_case.output, test_case.planned); + assert_eq!( + document["models"][0], + test_case.expected_first_model, + "{description}", + description = test_case.description + ); + } +} diff --git a/crates/sqlbuild-cli/src/native_commands/tests/test_types.rs b/crates/sqlbuild-cli/src/native_commands/tests/test_types.rs index 252ef24b95..93270cf91a 100644 --- a/crates/sqlbuild-cli/src/native_commands/tests/test_types.rs +++ b/crates/sqlbuild-cli/src/native_commands/tests/test_types.rs @@ -81,3 +81,11 @@ pub(super) struct InitFailureTestCase { pub(super) expected_stderr_start: &'static str, pub(super) expected_exit_code: i32, } + +/// One `sqb plan --json` output, whether it exited as a plan, and the first model the server receives. +pub(super) struct PlanDocumentTestCase { + pub(super) description: &'static str, + pub(super) output: Vec, + pub(super) planned: bool, + pub(super) expected_first_model: serde_json::Value, +} diff --git a/crates/sqlbuild-orchestration/src/platform/main/read_person_plan.rs b/crates/sqlbuild-orchestration/src/platform/main/read_person_plan.rs index 6dd96a14dc..5d24032915 100644 --- a/crates/sqlbuild-orchestration/src/platform/main/read_person_plan.rs +++ b/crates/sqlbuild-orchestration/src/platform/main/read_person_plan.rs @@ -11,7 +11,7 @@ use crate::store::errors::ControlError; use crate::store::main::{begin::begin, commit::commit}; use crate::store::models::{ControlStore, TransactionMode}; -/// Python source and dependency diffs stay out of the plan unless the reader may also read SQL on every current node. +/// SQL, config, Python source and dependency diffs stay out unless the reader may read SQL on every current node. pub async fn read_person_plan( store: &ControlStore, (context, principal_id): (&AuditContext, Uuid), @@ -41,12 +41,12 @@ pub async fn read_person_plan( .await?; commit(transaction).await?; if !sql { - without_identity_diffs(&mut record); + without_code_diffs(&mut record); } Ok(Some(record)) } -fn without_identity_diffs(record: &mut LocationWorkRecord) { +fn without_code_diffs(record: &mut LocationWorkRecord) { let Some(LocationWorkOutcome::Plan { document }) = record .result_json .as_mut() @@ -54,12 +54,19 @@ fn without_identity_diffs(record: &mut LocationWorkRecord) { else { return; }; - let nodes = document - .get_mut("python_nodes") - .and_then(serde_json::Value::as_array_mut); - for node in nodes.into_iter().flatten() { - if let Some(node) = node.as_object_mut() { - node.remove("identity_diff"); + for (section, keys) in [ + ("python_nodes", &["identity_diff"][..]), + ("models", &["query_diff", "config_diff"][..]), + ] { + let entries = document + .get_mut(section) + .and_then(serde_json::Value::as_array_mut); + for entry in entries.into_iter().flatten() { + if let Some(entry) = entry.as_object_mut() { + for key in keys { + entry.remove(*key); + } + } } } } diff --git a/crates/sqlbuild-orchestration/tests/api/test_person_plans.rs b/crates/sqlbuild-orchestration/tests/api/test_person_plans.rs index 424004c9ef..a774eac04b 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_person_plans.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_person_plans.rs @@ -304,7 +304,7 @@ fn given_person_plans_when_requested_claimed_and_read_then_they_stay_location_wo ); } let document = json!({ - "models": [{"name": "orders", "action": "create_table", "reason": "first_run"}], + "models": [{"name": "orders", "action": "create_table", "reason": "first_run", "query_diff": ["-SELECT 1", "+SELECT 2"], "config_diff": ["-a", "+b"]}], "warnings": [{"severity": "warning", "message": "connect failed: db_password=orders-pass"}], "python_nodes": [{"name": "orders_export", "identity_diff": {"source_diff": ["-a", "+b"]}}], }); @@ -356,7 +356,9 @@ fn given_person_plans_when_requested_claimed_and_read_then_they_stay_location_wo description = test_case.description ); assert!( - document["python_nodes"][0].get("identity_diff").is_none(), + document["python_nodes"][0].get("identity_diff").is_none() + && document["models"][0].get("query_diff").is_none() + && document["models"][0].get("config_diff").is_none(), "{description}: viewers lack view.sql", description = test_case.description ); diff --git a/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_json.rs b/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_json.rs index a9755d48f3..94b43956a1 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_json.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_json.rs @@ -10,6 +10,7 @@ use crate::bindings::_helpers::cursor::scalars::sentinel_to_token; use crate::bindings::_helpers::plan_output::cursor_details::{ CursorDetails, cursor_details, owner, }; +use crate::bindings::_helpers::plan_output::plan_text::{model_config_diff, model_query_diff}; use crate::bindings::_helpers::plan_output::python_identity::{ definition_source_text, dependency_source_text, }; @@ -231,6 +232,31 @@ fn model_json<'py>(entry: &Bound<'py, PyAny>) -> PyResult> { if !qualified.is_none() { model.set_item("qualified_name", qualified)?; } + let reason: String = text(&native_field(&native_field(entry, "reason")?, "value")?)?; + if let Some(diff) = model_query_diff(entry, &reason)? { + model.set_item("query_diff", diff)?; + } + if let Some(diff) = model_config_diff(entry, &reason)? { + model.set_item("config_diff", diff)?; + } + let findings: Bound<'py, PyAny> = native_field(entry, "schema_findings")?; + if findings.is_truthy()? { + model.set_item( + "schema_findings", + mapped(&findings, |finding| { + Ok(dict( + py, + vec![ + ("kind", enum_value(finding, "kind")?), + ("column", native_field(finding, "column_name")?), + ("expected_type", native_field(finding, "expected_type")?), + ("actual_type", native_field(finding, "actual_type")?), + ], + )? + .into_any()) + })?, + )?; + } let hints: Bound<'py, PyAny> = native_field(entry, "column_rename_hints")?; if hints.is_truthy()? { model.set_item( diff --git a/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_text.rs b/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_text.rs index e53da1d79b..568bf822dc 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_text.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/plan_output/plan_text.rs @@ -151,7 +151,11 @@ fn config_json<'py>(metadata_json: &Bound<'py, PyAny>) -> PyResult, reason: &str) -> PyResult>> { +/// The plain unified diff of a model's config since its last build, when its config changed. +pub(crate) fn model_config_diff( + entry: &Bound<'_, PyAny>, + reason: &str, +) -> PyResult>> { if !native_field(entry, "config_changed")?.is_truthy()? && reason != CONFIG_CHANGED { return Ok(None); } @@ -166,7 +170,35 @@ fn config_diff(entry: &Bound<'_, PyAny>, reason: &str) -> PyResult()?, + ¤t.extract::()?, + ) + .map(Some) +} + +/// The plain unified diff of a changed model's query since its last build, when that query is known. +pub(crate) fn model_query_diff( + entry: &Bound<'_, PyAny>, + reason: &str, +) -> PyResult>> { + let previous: Bound<'_, PyAny> = native_field(entry, "previous_query_sql")?; + if previous.is_none() + || (!native_field(entry, "query_changed")?.is_truthy()? && reason != QUERY_CHANGED) + { + return Ok(None); + } + unified_diff( + entry.py(), + &previous.extract::()?, + &native_field(entry, "fingerprint_query_sql")?.extract::()?, + ) + .map(Some) +} + +fn coloured(diff: Option>) -> Option> { + diff.map(|lines| format_query_diff(&lines, CliStyle { use_color: true })) } fn model_text(entry: &Bound<'_, PyAny>) -> PyResult { @@ -198,17 +230,6 @@ fn model_text(entry: &Bound<'_, PyAny>) -> PyResult { message: text(&native_field(&hint, "message")?)?, }); } - let previous_query: Bound<'_, PyAny> = native_field(entry, "previous_query_sql")?; - let query_diff_lines: Option> = if previous_query.is_none() - || (!native_field(entry, "query_changed")?.is_truthy()? && reason != QUERY_CHANGED) - { - None - } else { - Some(query_diff( - &previous_query, - &native_field(entry, "fingerprint_query_sql")?, - )?) - }; Ok(ModelPlanText { materialization_label: model_materialization_label(entry)?, custom_materialization_name: optional_text(&native_field( @@ -223,8 +244,8 @@ fn model_text(entry: &Bound<'_, PyAny>) -> PyResult { changed_functions: texts(&native_field(entry, "changed_functions")?)?, schema_findings, rename_hints, - config_diff: config_diff(entry, &reason)?, - query_diff: query_diff_lines, + config_diff: coloured(model_config_diff(entry, &reason)?), + query_diff: coloured(model_query_diff(entry, &reason)?), name, action, reason, diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md index 38771ec77c..fc8cb69d9c 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/plan.md @@ -119,7 +119,7 @@ Seeds (1) When query, function, or schema changes are detected, the plan lists each affected model under its own change with its backfill action. A model that calls a changed function directly appears under `Function changed` with `cause: function changed`; models further downstream continue forward. See [Replay decisions](../concepts/planning/replay-decisions.md). -In `sqb plan --json`, each model's `reason` is its own change. A direct caller of a changed function has `"reason": "function_changed"` and a `changed_functions` list, and a renamed model whose query also changed has `"reason": "renamed"` and `"query_changed": true`. +In `sqb plan --json`, each model's `reason` is its own change. A direct caller of a changed function has `"reason": "function_changed"` and a `changed_functions` list, and a renamed model whose query also changed has `"reason": "renamed"` and `"query_changed": true`. A changed model also carries the detail the text plan prints under it: `query_diff` when its query changed and the previous query is known, and `config_diff` when its config changed, each as unified diff lines, and `schema_findings` for added, removed, or retyped columns. Warnings and maintenance notices, such as `migrate_from can be removed`, are listed in a `Warnings` section before the final `Plan complete` line. `sqb plan --json` output is unchanged. diff --git a/tests/unit/src/sqlbuild/cli/output/main/plan_json/test_plan_json.py b/tests/unit/src/sqlbuild/cli/output/main/plan_json/test_plan_json.py index 19192c4d82..e2e0f2e647 100644 --- a/tests/unit/src/sqlbuild/cli/output/main/plan_json/test_plan_json.py +++ b/tests/unit/src/sqlbuild/cli/output/main/plan_json/test_plan_json.py @@ -462,6 +462,25 @@ def test_given_direct_diagnostics_policy_when_formatting_plan_json_then_evidence expected_keys=("models",), expected_fragments=('"reason": "renamed"', '"query_changed": true'), ), + JsonOutputTestCase( + description="plan json includes the query diff a changed model prints", + plan_output=build_plan_output( + model_entries=( + build_model_entry( + name="fact_orders", + action=PlanAction.CREATE_TABLE, + reason=PlanReason.QUERY_CHANGED, + previous_query_sql="SELECT id FROM orders", + ), + ), + ), + expected_keys=("models",), + expected_fragments=( + '"query_diff": [', + '"-SELECT id FROM orders"', + '"+SELECT * FROM fact_orders"', + ), + ), JsonOutputTestCase( description="plan json includes seeds and warnings", plan_output=build_plan_output( diff --git a/website/src/content/docs/docs/cli/plan.mdx b/website/src/content/docs/docs/cli/plan.mdx index 8fb761fc2a..44514fdb97 100644 --- a/website/src/content/docs/docs/cli/plan.mdx +++ b/website/src/content/docs/docs/cli/plan.mdx @@ -109,7 +109,7 @@ Seeds (1) When query, function, or schema changes are detected, the plan lists each affected model under its own change with its backfill action. A model that calls a changed function directly appears under `Function changed` with `cause: function changed`; models further downstream continue forward. See [Replay decisions](/docs/concepts/planning/replay-decisions). -In `sqb plan --json`, each model's `reason` is its own change. A direct caller of a changed function has `"reason": "function_changed"` and a `changed_functions` list, and a renamed model whose query also changed has `"reason": "renamed"` and `"query_changed": true`. +In `sqb plan --json`, each model's `reason` is its own change. A direct caller of a changed function has `"reason": "function_changed"` and a `changed_functions` list, and a renamed model whose query also changed has `"reason": "renamed"` and `"query_changed": true`. A changed model also carries the detail the text plan prints under it: `query_diff` when its query changed and the previous query is known, and `config_diff` when its config changed, each as unified diff lines, and `schema_findings` for added, removed, or retyped columns. Warnings and maintenance notices, such as `migrate_from can be removed`, are listed in a `Warnings` section before the final `Plan complete` line. `sqb plan --json` output is unchanged.