From 83f5d8d9317f68fa80dde12cacac50b966a6287b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Tue, 20 Oct 2026 20:26:33 +0100 Subject: [PATCH 1/6] perf(format): keep format runs native and stop repeating header lint and tokenization Format runs now hold file texts in Rust between passes instead of round-tripping every file through Python dictionaries, reuse each text's header lint for the final report, skip per-line keyword allocations in the header scanner, and reuse token and oracle lexemes across the formatter's verification and idempotency passes. --- .../bindings/_helpers/format_command/files.rs | 74 +- .../bindings/_helpers/format_command/mod.rs | 1 + .../_helpers/format_command/report.rs | 12 +- .../bindings/_helpers/format_command/run.rs | 715 +++++++++++++----- .../_helpers/format_command/stages.rs | 45 ++ .../src/bindings/_helpers/rules/lint_text.rs | 73 +- .../_helpers/rules/native_header_parser.rs | 7 +- .../src/dsl_headers/_helpers/scan.rs | 12 +- .../src/sql_lint/_helpers/formatter.rs | 50 +- .../src/sql_lint/_helpers/token_layout.rs | 57 +- .../sql_lint/tests/test_authored_tokens.rs | 11 +- fensu.toml | 2 +- 12 files changed, 723 insertions(+), 336 deletions(-) create mode 100644 crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs index 0b4e817f02..9fcfa8a121 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs @@ -1,4 +1,4 @@ -//! Authored SQL files as `{Path: str}` dictionaries, and writing formatted files back. +//! Writing formatted files back atomically, in the authored newline convention. use std::fs::{self, File, OpenOptions}; use std::io::Write; @@ -6,9 +6,8 @@ use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; use pyo3::exceptions::PyOSError; -use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyModule, PyModuleMethods, PyResult, Python}; -use pyo3::types::{PyDict, PyDictMethods}; -use pyo3::{pyfunction, wrap_pyfunction}; +use pyo3::prelude::{Bound, PyAny, PyModule, PyModuleMethods, PyResult, Python}; +use pyo3::{IntoPyObject, pyfunction, wrap_pyfunction}; const LINE_FEED: &str = "\n"; const CARRIAGE_RETURN_LINE_FEED: &str = "\r\n"; @@ -20,32 +19,13 @@ pub(crate) struct FormatChange<'py> { pub(crate) after: String, } -/// The entries of a `{Path: value}` dictionary, sorted as Python sorts paths. -pub(crate) fn sorted_items<'py>( - dictionary: &Bound<'py, PyDict>, -) -> PyResult, Bound<'py, PyAny>)>> { - let mut items: Vec<(PathBuf, Bound<'py, PyAny>, Bound<'py, PyAny>)> = dictionary - .iter() - .map(|(key, value)| Ok((key.extract::()?, key, value))) - .collect::>()?; - items.sort_by(|left, right| left.0.cmp(&right.0)); - Ok(items - .into_iter() - .map(|(_, key, value)| (key, value)) - .collect()) -} - -/// `{path: overrides.get(path, contents) for path, contents in files.items()}`. -pub(crate) fn overlaid<'py>( - files: &Bound<'py, PyDict>, - overrides: &Bound<'py, PyDict>, -) -> PyResult> { - let merged: Bound<'py, PyDict> = PyDict::new(files.py()); - for (path, contents) in files.iter() { - let value: Bound<'py, PyAny> = overrides.get_item(&path)?.unwrap_or(contents); - merged.set_item(path, value)?; - } - Ok(merged) +/// One file a format run would leave in its final text. +pub(crate) struct PendingWrite<'t> { + pub(crate) path: &'t Path, + /// The text read, or empty for a file the run did not collect. + pub(crate) authored: &'t str, + pub(crate) contents: &'t str, + pub(crate) newline: &'static str, } /// Generated contents in the authored newline convention. @@ -99,45 +79,27 @@ pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_function(wrap_pyfunction!(write_file_atomically, module)?) } -/// The changed files in path order, written in place when `write` is set. +/// The changed files of `writes`, in their order, written in place when `write` is set. pub(crate) fn persist_changes<'py>( - (files, updated): (&Bound<'py, PyDict>, &Bound<'py, PyDict>), - newlines: &Bound<'py, PyDict>, + py: Python<'py>, + writes: &[PendingWrite<'_>], write: bool, ) -> PyResult>> { let mut changes: Vec> = Vec::new(); - for (path, contents) in sorted_items(updated)? { - let newline: String = newlines - .get_item(&path)? - .map_or_else(|| Ok(LINE_FEED.to_owned()), |value| value.extract())?; - let authored: String = files - .get_item(&path)? - .map_or_else(|| Ok(String::new()), |value| value.extract())?; - let after: String = with_newline_style(&contents.extract::()?, &newline); - let before: String = with_newline_style(&authored, &newline); + for pending in writes { + let after: String = with_newline_style(pending.contents, pending.newline); + let before: String = with_newline_style(pending.authored, pending.newline); if after == before { continue; } if write { - write_atomically(&path.extract::()?, &after)?; + write_atomically(pending.path, &after)?; } changes.push(FormatChange { - path, + path: pending.path.into_pyobject(py)?.into_any(), before, after, }); } Ok(changes) } - -/// `{Path: contents}` of the collected files, in collection order. -pub(crate) fn file_dictionary<'py>( - py: Python<'py>, - files: Vec<(PathBuf, String)>, -) -> PyResult> { - let dictionary: Bound<'py, PyDict> = PyDict::new(py); - for (path, contents) in files { - dictionary.set_item(path, contents)?; - } - Ok(dictionary) -} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs index 7404b5ea9d..0a1e237715 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs @@ -5,3 +5,4 @@ pub(crate) mod registration; pub(crate) mod report; pub(crate) mod run; pub(crate) mod selection; +pub(crate) mod stages; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/report.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/report.rs index 037dab8908..0ea13e5f02 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/report.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/report.rs @@ -1,10 +1,9 @@ //! A format run's Python violations and Rule fix reports as plain report data. -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use std::path::{Path, PathBuf}; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult}; -use pyo3::types::{PyDict, PyDictMethods}; use sqlbuild_core::format_command::models::{LintFinding, LintReport, RuleFixLine}; use sqlbuild_core::text::main::python_splitlines::python_splitlines; @@ -28,25 +27,24 @@ fn display_path(path: &Path, root: &Path) -> String { } fn source_line( - contents: &Bound<'_, PyDict>, + contents: &HashMap, path: &Bound<'_, PyAny>, line: i64, ) -> PyResult> { - let Some(source) = contents.get_item(path)? else { + let Some(source) = contents.get(&path.extract::()?) else { return Ok(None); }; - let source: String = source.extract()?; if line < 1 { return Ok(None); } - let lines: Vec<&str> = python_splitlines(&source); + let lines: Vec<&str> = python_splitlines(source); let index: usize = usize::try_from(line - 1).unwrap_or(usize::MAX); Ok(lines.get(index).map(|line| (*line).to_owned())) } fn finding( violation: &Bound<'_, PyAny>, - contents: &Bound<'_, PyDict>, + contents: &HashMap, root: &Path, ) -> PyResult { let path: Bound<'_, PyAny> = native_field(violation, "file_path")?; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs index 75c4fc9947..12c0cc3cb4 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs @@ -1,11 +1,18 @@ //! Format every authored SQL file and collect the violations that remain. -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use std::path::PathBuf; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, Python}; -use pyo3::types::{PyBool, PyDict, PyDictMethods, PyList, PyListMethods}; +use pyo3::types::{PyBool, PyDict, PyDictMethods}; +use pyo3::{IntoPyObject, IntoPyObjectExt}; use sqlbuild_core::format_command::main::collect_lint_files::collect_lint_files; +use sqlbuild_rules::dsl_headers::models::{HeaderRulesConfig, HeaderViolation}; +use sqlbuild_rules::lint_text::main::format_sql_bodies::format_sql_bodies; +use sqlbuild_rules::lint_text::main::reject_inline_suppressions::reject_inline_suppressions; +use sqlbuild_rules::lint_text::models::{ + FormatFault, FormatFile, FormattedBodies, SuppressionFinding, +}; use crate::bindings::_helpers::boundary::panics::value_error; use crate::bindings::_helpers::boundary::python_calls::keywords; @@ -14,23 +21,25 @@ use crate::bindings::_helpers::compile_driver::stage_work::{ }; use crate::bindings::_helpers::discovery::description_inputs::externally_described_models; use crate::bindings::_helpers::format_command::files::{ - FormatChange, file_dictionary, overlaid, persist_changes, sorted_items, + FormatChange, PendingWrite, persist_changes, +}; +use crate::bindings::_helpers::format_command::stages::{ + each_file, format_header_text, lint_header_text, parse_faults, }; use crate::bindings::_helpers::records::construct::error_class; use crate::bindings::_helpers::records::fields::field as native_field; use crate::bindings::_helpers::records::ids::ErrorId; -use crate::bindings::_helpers::rules::dsl_headers::format_native_headers; -use crate::bindings::_helpers::rules::dsl_headers::reject_unparseable_header_rewrites; -use crate::bindings::_helpers::rules::dsl_headers::safe_format_files; -use crate::bindings::_helpers::rules::dsl_headers::{header_violations, rules_config}; +use crate::bindings::_helpers::rules::dsl_headers::rules_config; use crate::bindings::_helpers::rules::fixture_formatting::format_redundant_fixture_nulls; -use crate::bindings::_helpers::rules::lint_pipeline::{LintViolationRow, ViolationClasses}; -use crate::bindings::_helpers::rules::lint_text::format_lint_sql_bodies; -use crate::bindings::_helpers::rules::lint_text::reject_lint_suppressions; -use sqlbuild_rules::dsl_headers::main::scan_headers::scan_headers; -use sqlbuild_rules::dsl_headers::models::{HeaderRulesConfig, HeaderViolation}; +use crate::bindings::_helpers::rules::lint_pipeline::{ + LintViolationRow, ViolationClasses, project_relative_path, +}; +use crate::bindings::_helpers::rules::lint_text::{relative_parts, unsafe_fault}; +use crate::bindings::types::CompilerDetach; const DESCRIPTION_PRESENT: &str = "description-present"; +const LINE_FEED: &str = "\n"; +const CARRIAGE_RETURN_LINE_FEED: &str = "\r\n"; /// What one format run formats and how. pub(crate) struct FormatRequest<'a, 'py> { @@ -52,127 +61,314 @@ pub(crate) struct FormatOutcome<'py> { pub(crate) rule_fixes: Vec>, /// Paths whose formatting was declined, so their applied fixes were not kept. pub(crate) declined: BTreeSet, - pub(crate) final_contents: Bound<'py, PyDict>, + /// The final text of every collected file. + pub(crate) final_contents: HashMap, } -/// Fixture-null rewrites proven safe by the contracts, when fixture inputs are known. -fn fixture_autofix<'py>( - request: &FormatRequest<'_, 'py>, - fixture_inputs: Option<&(Bound<'py, PyAny>, Bound<'py, PyAny>)>, - files: &Bound<'py, PyDict>, -) -> PyResult> { - let py: Python<'py> = files.py(); - let Some((discovered, syntax)) = fixture_inputs else { - return Ok(PyDict::new(py)); - }; - Ok(format_redundant_fixture_nulls( - &files.clone().into_any().cast_into::()?, - &request.project_dir.clone(), - &discovered.clone(), - &syntax.clone(), - ) - .and_then(|value| pyo3::IntoPyObjectExt::into_bound_py_any(value, py))? - .downcast_into::()?) +/// One file the run formats: where it is, the text read, and whether it was collected. +struct Entry { + path: PathBuf, + original: String, + /// False for a file only Rule fixes introduced: it is written, but neither linted nor shown. + collected: bool, } -/// SQL bodies of `files` formatted natively: changed texts and declined-body faults. -fn format_bodies<'py>( - request: &FormatRequest<'_, 'py>, - files: &Bound<'py, PyDict>, -) -> PyResult<(Bound<'py, PyDict>, Bound<'py, PyAny>)> { - let py: Python<'py> = files.py(); - let items: Vec<(Bound<'py, PyAny>, String)> = sorted_items(files)? - .into_iter() - .map(|(path, contents)| Ok((path, contents.extract::()?))) - .collect::>()?; - count_stage(py, FORMAT_LINT_BATCHES, 1)?; - count_stage(py, FORMAT_LINT_FILES, items.len() as u64)?; - let result: Bound<'py, PyAny> = format_lint_sql_bodies( - py, - items, - request.project_dir.extract::()?, - request.config, - ) - .and_then(|value| pyo3::IntoPyObjectExt::into_bound_py_any(value, py))?; - Ok(( - result.get_item(0)?.downcast_into::()?, - result.get_item(1)?, - )) +/// A violation, kept as native data until the report needs Python records. +enum Finding<'py> { + Header(usize, HeaderViolation), + Unsafe(usize, FormatFault), + Suppression(usize, SuppressionFinding), + Python(Bound<'py, PyAny>), +} + +/// Rule fix reports and the faults of fixes that could not be kept. +type RuleFixOutcome<'py> = (Vec>, Vec>); + +/// Changed texts by entry, and the findings of declined bodies. +type FormattedTexts<'py> = (Vec<(usize, String)>, Vec>); + +/// Rewritten texts by entry, in the order each entry was first rewritten. +#[derive(Default)] +struct Rewrites { + texts: HashMap, + order: Vec, +} + +impl Rewrites { + fn set(&mut self, index: usize, text: String) { + if self.texts.insert(index, text).is_none() { + self.order.push(index); + } + } + + fn get(&self, index: usize) -> Option<&str> { + self.texts.get(&index).map(String::as_str) + } +} + +/// The rewrites a run kept, the faults that declined the others, and the lint of each rewrite. +struct Applied<'py> { + rewrites: Rewrites, + rejected: BTreeSet, + faults: Vec>, + rewrite_lints: HashMap>, +} + +/// The native state of one format run. +struct Run<'a, 'r, 'py> { + request: &'r FormatRequest<'a, 'py>, + py: Python<'py>, + directory: PathBuf, + config: HeaderRulesConfig, + entries: Vec, + index: HashMap, + /// The entries headers parse in, which Rule fixes and formatting rewrite. + safe: Vec, + /// Texts Rule fixes changed. + fixed: HashMap, +} + +fn native( + py: Python<'_>, + operation: impl FnOnce() -> Result + Send, +) -> PyResult { + py.compiler_detach(operation).map_err(value_error) } -/// Contents after header, SQL body and fixture fixes, and the faults of declined rewrites. -fn apply_fixes<'py>( - request: &FormatRequest<'_, 'py>, - files: &Bound<'py, PyDict>, - fixture_inputs: Option<&(Bound<'py, PyAny>, Bound<'py, PyAny>)>, -) -> PyResult<(Bound<'py, PyDict>, Bound<'py, PyList>)> { - let py: Python<'py> = files.py(); - let updated: Bound<'py, PyDict> = fixture_autofix(request, fixture_inputs, files)?; - let faults: Bound<'py, PyList> = PyList::empty(py); - if request.fixtures_only { - return Ok((updated, faults)); +impl<'py> Run<'_, '_, 'py> { + /// The text formatting starts from: the Rule-fixed text, else the text read. + fn base_text(&self, index: usize) -> &str { + self.fixed + .get(&index) + .map_or(self.entries[index].original.as_str(), String::as_str) + } + + fn text<'t>(&'t self, index: usize, rewrites: &'t Rewrites) -> &'t str { + rewrites.get(index).unwrap_or_else(|| self.base_text(index)) + } + + fn sorted(&self, indices: &[usize]) -> Vec { + let mut sorted: Vec = indices.to_vec(); + sorted.sort_by(|left, right| self.entries[*left].path.cmp(&self.entries[*right].path)); + sorted + } + + /// The entry of `path`, adding one for a path the run did not collect. + fn entry_of(&mut self, path: PathBuf) -> usize { + if let Some(index) = self.index.get(&path) { + return *index; + } + let index: usize = self.entries.len(); + let _ = self.index.insert(path.clone(), index); + self.entries.push(Entry { + path, + original: String::new(), + collected: false, + }); + index + } + + /// `{Path: text}` of the safe entries, as the Python formatting passes read them. + fn safe_dictionary(&self, rewrites: &Rewrites) -> PyResult> { + let dictionary: Bound<'py, PyDict> = PyDict::new(self.py); + for index in &self.safe { + dictionary.set_item(&self.entries[*index].path, self.text(*index, rewrites))?; + } + Ok(dictionary) } - let current: Bound<'py, PyDict> = overlaid(files, &updated)?; - for (path, contents) in sorted_items(¤t)? { + + /// Rule fixes planned in memory and verified by compilation: reports and faults. + fn plan_rule_fixes(&mut self) -> PyResult> { + let py: Python<'py> = self.py; + let request: &FormatRequest<'_, 'py> = self.request; + let none = || py.None().into_bound(py); let result: Bound<'py, PyAny> = - format_native_headers(&contents.clone(), &path.clone(), &request.config.clone()) - .and_then(|value| pyo3::IntoPyObjectExt::into_bound_py_any(value, py))?; - for fault in result.get_item(1)?.try_iter()? { - faults.append(fault?)?; + crate::bindings::_helpers::rules::fix_session::plan_rule_fixes( + py, + Some(keywords( + py, + &[ + ("project_dir", request.project_dir.clone()), + ( + "files", + self.safe_dictionary(&Rewrites::default())?.into_any(), + ), + ("config", request.config.clone()), + ("adapter", request.adapter.map_or_else(none, Clone::clone)), + ( + "discovered_inputs", + request.discovered.map_or_else(none, Clone::clone), + ), + ("enabled", PyBool::new(py, true).to_owned().into_any()), + ], + )?), + )?; + let mut safe: Vec = Vec::new(); + for (path, contents) in result.get_item(0)?.cast_into::()?.iter() { + let index: usize = self.entry_of(path.extract()?); + let text: String = contents.extract()?; + if text != self.entries[index].original || !self.entries[index].collected { + let _ = self.fixed.insert(index, text); + } + safe.push(index); } - let formatted: Bound<'py, PyAny> = result.get_item(0)?; - if !formatted.eq(&contents)? { - updated.set_item(path, formatted)?; + self.safe = safe; + Ok(( + result.get_item(1)?.try_iter()?.collect::>()?, + result.get_item(2)?.try_iter()?.collect::>()?, + )) + } + + /// Fixture-null rewrites proven safe by the contracts, by entry, in the order Python made them. + fn fixture_autofix( + &mut self, + (discovered, syntax): &(Bound<'py, PyAny>, Bound<'py, PyAny>), + rewrites: &Rewrites, + ) -> PyResult> { + let updated: Bound<'py, PyAny> = format_redundant_fixture_nulls( + &self.safe_dictionary(rewrites)?, + &self.request.project_dir.clone(), + &discovered.clone(), + &syntax.clone(), + )? + .into_bound_py_any(self.py)?; + let mut texts: Vec<(usize, String)> = Vec::new(); + for (path, contents) in updated.cast_into::()?.iter() { + let index: usize = self.entry_of(path.extract()?); + texts.push((index, contents.extract()?)); } + Ok(texts) } - let (native_formatted, native_faults) = format_bodies(request, &overlaid(files, &updated)?)?; - for fault in native_faults.try_iter()? { - faults.append(fault?)?; + + /// The SQL bodies of `items` formatted natively: changed texts and declined-body faults. + fn format_bodies(&self, items: &[(usize, &str)]) -> PyResult> { + count_stage(self.py, FORMAT_LINT_BATCHES, 1)?; + count_stage(self.py, FORMAT_LINT_FILES, items.len() as u64)?; + let inputs: Vec = items + .iter() + .map(|(index, contents)| FormatFile { + relative_parts: relative_parts(&self.entries[*index].path, &self.directory), + contents: (*contents).to_owned(), + }) + .collect(); + let dialect: String = native_field(self.request.config, "dialect")?.extract()?; + let line_width: usize = native_field(self.request.config, "line_width")?.extract()?; + let result: FormattedBodies = native(self.py, || { + format_sql_bodies(&inputs, &dialect, line_width).map_err(|error| error.to_string()) + })?; + Ok(( + result + .formatted + .into_iter() + .map(|(position, text)| (items[position].0, text)) + .collect(), + result + .faults + .into_iter() + .map(|fault| Finding::Unsafe(items[fault.file].0, fault)) + .collect(), + )) } - updated.update(native_formatted.as_mapping())?; - let fixture_formatted: Bound<'py, PyDict> = - fixture_autofix(request, fixture_inputs, &overlaid(files, &updated)?)?; - if !fixture_formatted.is_empty() { - let (final_native, final_faults) = format_bodies(request, &fixture_formatted)?; - for fault in final_faults.try_iter()? { - faults.append(fault?)?; + + /// Contents after header, SQL body and fixture fixes, and the faults of declined rewrites. + fn apply_fixes( + &mut self, + fixture_inputs: Option<&(Bound<'py, PyAny>, Bound<'py, PyAny>)>, + ) -> PyResult> { + let mut applied: Applied<'py> = Applied { + rewrites: Rewrites::default(), + rejected: BTreeSet::new(), + faults: Vec::new(), + rewrite_lints: HashMap::new(), + }; + if let Some(inputs) = fixture_inputs { + for (index, text) in self.fixture_autofix(inputs, &applied.rewrites)? { + applied.rewrites.set(index, text); + } + } + if self.request.fixtures_only { + return Ok(applied); + } + let sorted: Vec = self.sorted(&self.safe); + let config: HeaderRulesConfig = self.config; + let headers: Vec<(String, Vec)> = { + let texts: Vec<&str> = sorted + .iter() + .map(|index| self.text(*index, &applied.rewrites)) + .collect(); + native(self.py, || { + each_file(&texts, |text| format_header_text(text, config)) + })? + }; + for (index, (formatted, faults)) in sorted.iter().zip(headers) { + applied.faults.extend( + faults + .into_iter() + .map(|fault| Finding::Header(*index, fault)), + ); + if formatted != self.text(*index, &applied.rewrites) { + applied.rewrites.set(*index, formatted); + } + } + let (formatted, faults) = { + let items: Vec<(usize, &str)> = sorted + .iter() + .map(|index| (*index, self.text(*index, &applied.rewrites))) + .collect(); + self.format_bodies(&items)? + }; + applied.faults.extend(faults); + for (index, text) in formatted { + applied.rewrites.set(index, text); + } + if let Some(inputs) = fixture_inputs { + let fixtures: Vec<(usize, String)> = self.fixture_autofix(inputs, &applied.rewrites)?; + if !fixtures.is_empty() { + let mut items: Vec<(usize, &str)> = fixtures + .iter() + .map(|(index, text)| (*index, text.as_str())) + .collect(); + items.sort_by(|left, right| { + self.entries[left.0].path.cmp(&self.entries[right.0].path) + }); + let (formatted, faults) = self.format_bodies(&items)?; + applied.faults.extend(faults); + for (index, text) in fixtures.into_iter().chain(formatted) { + applied.rewrites.set(index, text); + } + } } - updated.update(fixture_formatted.as_mapping())?; - updated.update(final_native.as_mapping())?; + self.reject_unparseable_rewrites(applied) } - let result: Bound<'py, PyAny> = reject_unparseable_header_rewrites( - &updated.into_any().cast_into::()?, - &request.config.clone(), - &faults.into_any().cast_into::()?, - ) - .and_then(|value| pyo3::IntoPyObjectExt::into_bound_py_any(value, py))?; - Ok(( - result.get_item(0)?.downcast_into::()?, - result.get_item(1)?.downcast_into::()?, - )) -} -/// Formatter-owned header diagnostics of the final contents, without configured Rules. -fn lint_final_contents<'py>( - request: &FormatRequest<'_, 'py>, - files: &Bound<'py, PyDict>, - updated: &Bound<'py, PyDict>, -) -> PyResult>> { - let py: Python<'py> = files.py(); - let mut violations: Vec> = Vec::new(); - let header_rules: HeaderRulesConfig = rules_config(request.config)?; - let classes: ViolationClasses<'py> = ViolationClasses::new(py)?; - for (path, contents) in sorted_items(files)? { - let final_contents: String = updated.get_item(&path)?.unwrap_or(contents).extract()?; - let text: Vec = final_contents.chars().collect(); - let found: Vec = - header_violations(&text, &scan_headers(&text, false), header_rules) - .map_err(value_error)?; - for violation in found { - violations.push(classes.violation(&path, &LintViolationRow::header(0, violation))?); + /// Decline every rewrite whose header no longer parses, adding its header-parse faults. + fn reject_unparseable_rewrites(&self, mut applied: Applied<'py>) -> PyResult> { + let config: HeaderRulesConfig = self.config; + let order: Vec = applied.rewrites.order.clone(); + let lints: Vec> = { + let texts: Vec<&str> = order + .iter() + .map(|index| applied.rewrites.get(*index).unwrap_or_default()) + .collect(); + native(self.py, || { + each_file(&texts, |text| lint_header_text(text, config)) + })? + }; + for (index, violations) in order.into_iter().zip(lints) { + let faults: Vec = parse_faults(&violations); + if faults.is_empty() { + let _ = applied.rewrite_lints.insert(index, violations); + continue; + } + let _ = applied.rejected.insert(index); + applied.faults.extend( + faults + .into_iter() + .map(|fault| Finding::Header(index, fault)), + ); } + Ok(applied) } - Ok(violations) } fn code_is(violation: &Bound<'_, PyAny>, code: &str) -> PyResult { @@ -207,44 +403,101 @@ fn drop_resolved_description_faults<'py>( Ok(kept) } -/// Rule fixes planned in memory and verified by compilation, when `--fix` is set. -fn plan_rule_fixes<'py>( - request: &FormatRequest<'_, 'py>, - files: &Bound<'py, PyDict>, -) -> PyResult<(Bound<'py, PyDict>, Bound<'py, PyAny>, Bound<'py, PyAny>)> { - let py: Python<'py> = files.py(); - let result: Bound<'py, PyAny> = crate::bindings::_helpers::rules::fix_session::plan_rule_fixes( - py, - Some(keywords( - py, - &[ - ("project_dir", request.project_dir.clone()), - ("files", files.clone().into_any()), - ("config", request.config.clone()), - ( - "adapter", - request - .adapter - .map_or_else(|| py.None().into_bound(py), Clone::clone), - ), - ( - "discovered_inputs", - request - .discovered - .map_or_else(|| py.None().into_bound(py), Clone::clone), - ), - ( - "enabled", - PyBool::new(py, request.fix).to_owned().into_any(), - ), - ], - )?), - )?; - Ok(( - result.get_item(0)?.downcast_into::()?, - result.get_item(1)?, - result.get_item(2)?, - )) +/// Python `LintViolation` records of `findings`, in order, sharing one path object per file. +fn violation_records<'py>( + py: Python<'py>, + entries: &[Entry], + findings: Vec>, +) -> PyResult>> { + let classes: ViolationClasses<'py> = ViolationClasses::new(py)?; + let mut paths: HashMap> = HashMap::new(); + let mut path = |index: usize| -> PyResult> { + if let Some(path) = paths.get(&index) { + return Ok(path.clone()); + } + let path: Bound<'py, PyAny> = entries[index].path.clone().into_pyobject(py)?.into_any(); + let _ = paths.insert(index, path.clone()); + Ok(path) + }; + findings + .into_iter() + .map(|finding| match finding { + Finding::Header(index, violation) => { + classes.violation(&path(index)?, &LintViolationRow::header(0, violation)) + } + Finding::Unsafe(index, fault) => unsafe_fault(&path(index)?, &fault), + Finding::Suppression(index, finding) => { + classes.violation(&path(index)?, &LintViolationRow::suppression(finding)) + } + Finding::Python(violation) => Ok(violation), + }) + .collect() +} + +/// The path a fault declines the formatting of. +fn fault_path(entries: &[Entry], fault: &Finding<'_>) -> PyResult { + match fault { + Finding::Header(index, _) | Finding::Unsafe(index, _) | Finding::Suppression(index, _) => { + Ok(entries[*index].path.clone()) + } + Finding::Python(violation) => native_field(violation, "file_path")?.extract(), + } +} + +/// Header violations of each collected file's final text, in path order, reusing earlier lints. +fn lint_final_contents<'py>( + run: &Run<'_, '_, 'py>, + (final_texts, original_lints): (&HashMap, &[Vec]), + applied: &Applied<'py>, +) -> PyResult>> { + let collected: Vec = (0..run.entries.len()) + .filter(|index| run.entries[*index].collected) + .collect(); + let sorted: Vec = run.sorted(&collected); + let final_text = |index: usize| -> &str { + final_texts + .get(&index) + .copied() + .unwrap_or(run.entries[index].original.as_str()) + }; + let known = |index: usize| -> Option<&Vec> { + let text: &str = final_text(index); + if text == run.entries[index].original { + return original_lints.get(index); + } + applied + .rewrite_lints + .get(&index) + .filter(|_| applied.rewrites.get(index) == Some(text)) + }; + let unknown: Vec = sorted + .iter() + .copied() + .filter(|index| known(*index).is_none()) + .collect(); + let config: HeaderRulesConfig = run.config; + let computed: Vec> = { + let texts: Vec<&str> = unknown.iter().map(|index| final_text(*index)).collect(); + native(run.py, || { + each_file(&texts, |text| lint_header_text(text, config)) + })? + }; + let computed: HashMap> = + unknown.into_iter().zip(computed).collect(); + let mut findings: Vec> = Vec::new(); + for index in sorted { + let violations: &Vec = match known(index) { + Some(violations) => violations, + None => &computed[&index], + }; + findings.extend( + violations + .iter() + .cloned() + .map(|violation| Finding::Header(index, violation)), + ); + } + Ok(findings) } /// Format the project's authored SQL files and report what remains. @@ -267,53 +520,133 @@ pub(crate) fn run_format<'py>(request: &FormatRequest<'_, 'py>) -> PyResult
= collect_lint_files(&directory, request.selected) .map_err(|error| pyo3::exceptions::PyOSError::new_err(error.to_string()))?; let files_checked: usize = collected.len(); - let files: Bound<'py, PyDict> = file_dictionary(py, collected)?; - let safe: Bound<'py, PyAny> = safe_format_files( - &files.clone().into_any().cast_into::()?, - &request.config.clone(), - ) - .and_then(|value| pyo3::IntoPyObjectExt::into_bound_py_any(value, py))?; - let newlines: Bound<'py, PyDict> = safe.get_item(1)?.downcast_into::()?; - let (safe_files, rule_fixes, rule_faults) = - plan_rule_fixes(request, &safe.get_item(0)?.downcast_into::()?)?; - let (fixed, format_faults) = apply_fixes(request, &safe_files, fixture_inputs.as_ref())?; - for fault in rule_faults.try_iter()? { - format_faults.append(fault?)?; - } - let merged: Bound<'py, PyDict> = safe_files.copy()?; - merged.update(fixed.as_mapping())?; - let declined: BTreeSet = format_faults + let entries: Vec = collected + .into_iter() + .map(|(path, original)| Entry { + path, + original, + collected: true, + }) + .collect(); + let config: HeaderRulesConfig = rules_config(request.config)?; + let original_lints: Vec> = native(py, || { + each_file(&entries, |entry| lint_header_text(&entry.original, config)) + })?; + let mut run: Run<'_, '_, 'py> = Run { + request, + py, + directory, + config, + index: entries + .iter() + .enumerate() + .map(|(index, entry)| (entry.path.clone(), index)) + .collect(), + safe: (0..entries.len()) + .filter(|index| parse_faults(&original_lints[*index]).is_empty()) + .collect(), + entries, + fixed: HashMap::new(), + }; + let (rule_fixes, rule_faults) = if request.fix { + run.plan_rule_fixes()? + } else { + (Vec::new(), Vec::new()) + }; + let mut applied: Applied<'py> = run.apply_fixes(fixture_inputs.as_ref())?; + applied + .faults + .extend(rule_faults.into_iter().map(Finding::Python)); + let declined: BTreeSet = applied + .faults .iter() - .map(|fault| native_field(&fault, "file_path")?.extract()) + .map(|fault| fault_path(&run.entries, fault)) .collect::>()?; - let updated: Bound<'py, PyDict> = PyDict::new(py); - for (path, contents) in merged.iter() { - if !declined.contains(&path.extract::()?) { - updated.set_item(path, contents)?; - } - } - let changes: Vec> = - persist_changes((&files, &updated), &newlines, request.write)?; - let mut violations: Vec> = if request.fixtures_only { + let final_texts: HashMap = run + .safe + .iter() + .filter(|index| !declined.contains(&run.entries[**index].path)) + .map(|index| { + let text: &str = if applied.rejected.contains(index) { + run.base_text(*index) + } else { + run.text(*index, &applied.rewrites) + }; + (*index, text) + }) + .collect(); + let writes: Vec> = run + .sorted(&final_texts.keys().copied().collect::>()) + .into_iter() + .map(|index| { + let entry: &Entry = &run.entries[index]; + PendingWrite { + path: &entry.path, + authored: &entry.original, + contents: final_texts[&index], + newline: if entry.collected && entry.original.contains(CARRIAGE_RETURN_LINE_FEED) { + CARRIAGE_RETURN_LINE_FEED + } else { + LINE_FEED + }, + } + }) + .collect(); + let changes: Vec> = persist_changes(py, &writes, request.write)?; + let mut findings: Vec> = if request.fixtures_only { Vec::new() } else { - lint_final_contents(request, &files, &updated)? + lint_final_contents(&run, (&final_texts, &original_lints), &applied)? }; - violations.extend(format_faults.iter()); - let violations: Vec> = - drop_resolved_description_faults(request.project_dir, violations)?; - let final_contents: Bound<'py, PyDict> = overlaid(&files, &updated)?; - let violations: Vec> = reject_lint_suppressions( - py, - violations, - final_contents.clone(), - request.project_dir.extract()?, + findings.append(&mut applied.faults); + let violations: Vec> = drop_resolved_description_faults( + request.project_dir, + violation_records(py, &run.entries, findings)?, )?; + let collected: Vec = (0..run.entries.len()) + .filter(|index| run.entries[*index].collected) + .collect(); + let relative: Vec = collected + .iter() + .map(|index| project_relative_path(&run.entries[*index].path, &run.directory)) + .collect(); + let sources: Vec<(&str, &str)> = collected + .iter() + .zip(&relative) + .map(|(index, path)| { + ( + path.as_str(), + final_texts + .get(index) + .copied() + .unwrap_or(run.entries[*index].original.as_str()), + ) + }) + .collect(); + let suppressions: Vec> = native(py, || { + reject_inline_suppressions(&sources).map_err(|error| error.to_string()) + })? + .into_iter() + .map(|finding| Finding::Suppression(collected[finding.file], finding)) + .collect(); + let mut violations: Vec> = violations; + violations.extend(violation_records(py, &run.entries, suppressions)?); + let final_contents: HashMap = collected + .iter() + .map(|index| { + let entry: &Entry = &run.entries[*index]; + let text: &str = final_texts + .get(index) + .copied() + .unwrap_or(entry.original.as_str()); + (entry.path.clone(), text.to_owned()) + }) + .collect(); Ok(FormatOutcome { files_checked, violations, changes, - rule_fixes: rule_fixes.try_iter()?.collect::>()?, + rule_fixes, declined, final_contents, }) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs new file mode 100644 index 0000000000..fc19a7338b --- /dev/null +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs @@ -0,0 +1,45 @@ +//! The per-file native work of a format run: header lint and header formatting. + +use sqlbuild_rules::dsl_headers::constants::RULE_HEADER_PARSE; +use sqlbuild_rules::dsl_headers::main::format_headers::format_headers; +use sqlbuild_rules::dsl_headers::main::scan_headers::scan_headers; +use sqlbuild_rules::dsl_headers::models::{HeaderRulesConfig, HeaderViolation}; + +use crate::bindings::_helpers::rules::dsl_headers::header_violations; +use crate::bindings::_helpers::rules::native_header_parser::NativeHeaderParser; + +/// Every header rule violation of `text`. +pub(crate) fn lint_header_text( + text: &str, + config: HeaderRulesConfig, +) -> Result, String> { + let characters: Vec = text.chars().collect(); + header_violations(&characters, &scan_headers(&characters, false), config) +} + +/// The header-parse faults among `violations`. +pub(crate) fn parse_faults(violations: &[HeaderViolation]) -> Vec { + violations + .iter() + .filter(|violation| violation.code == RULE_HEADER_PARSE) + .cloned() + .collect() +} + +/// `text` with header formatting fixes applied, and the faults relocation reported. +pub(crate) fn format_header_text( + text: &str, + config: HeaderRulesConfig, +) -> Result<(String, Vec), String> { + let characters: Vec = text.chars().collect(); + let (formatted, faults) = format_headers(&characters, config, &NativeHeaderParser)?; + Ok((formatted.into_iter().collect(), faults)) +} + +/// `operation` over every item, in item order; the first failure in item order is returned. +pub(crate) fn each_file( + items: &[T], + operation: impl Fn(&T) -> Result + Sync, +) -> Result, String> { + items.iter().map(operation).collect() +} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/rules/lint_text.rs b/crates/sqlbuild-python/src/bindings/_helpers/rules/lint_text.rs index 8f447e874b..a2584b361a 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/rules/lint_text.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/rules/lint_text.rs @@ -17,7 +17,7 @@ use sqlbuild_rules::lint_text::main::reject_inline_suppressions::reject_inline_s use sqlbuild_rules::lint_text::main::restore_interpolation::restore_interpolation; use sqlbuild_rules::lint_text::main::scan_fixture_null_candidates::scan_fixture_null_candidates; use sqlbuild_rules::lint_text::models::{ - FormatFile, FormattedBodies, InterpolationSite, Neutralized, SuppressionFinding, + FormatFault, FormatFile, FormattedBodies, InterpolationSite, Neutralized, SuppressionFinding, }; use crate::bindings::_helpers::boundary::panics::{compiler_guard, value_error}; @@ -156,7 +156,10 @@ pub(crate) fn reject_lint_suppressions<'py>( } /// The parts of `path` below `project_dir`, or `None` outside the project. -fn relative_parts(path: &std::path::Path, project_dir: &std::path::Path) -> Option> { +pub(crate) fn relative_parts( + path: &std::path::Path, + project_dir: &std::path::Path, +) -> Option> { match path.strip_prefix(project_dir) { Ok(relative) => Some( relative @@ -168,6 +171,42 @@ fn relative_parts(path: &std::path::Path, project_dir: &std::path::Path) -> Opti } } +/// The `format-unsafe` fault of a body the formatter declined in the file at `file_path`. +pub(crate) fn unsafe_fault<'py>( + file_path: &Bound<'py, PyAny>, + fault: &FormatFault, +) -> PyResult> { + let py: Python<'py> = file_path.py(); + let severity: Bound<'py, PyAny> = enum_member(py, RecordId::LintSeverity, "FAULT")?; + new_record_with( + py, + RecordId::LintViolation, + &[ + ("file_path", file_path.clone()), + ("line", fault.line.into_pyobject(py)?.into_any()), + ("column", 1_usize.into_pyobject(py)?.into_any()), + ("code", PyString::new(py, FORMAT_UNSAFE_CODE).into_any()), + ( + "message", + PyString::new( + py, + &format!( + "File left unchanged because its SQL body could not be formatted: {}", + fault.reason + ), + ) + .into_any(), + ), + ("severity", severity), + ("engine", PyString::new(py, NATIVE_ENGINE).into_any()), + ( + "remediation", + PyString::new(py, FORMAT_UNSAFE_REMEDIATION).into_any(), + ), + ], + ) +} + /// Format the SQL bodies of `files`; return changed texts by path and declined-body faults. #[pyfunction] pub(crate) fn format_lint_sql_bodies<'py>( @@ -196,36 +235,10 @@ pub(crate) fn format_lint_sql_bodies<'py>( for (index, text) in result.formatted { formatted.set_item(&files[index].0, text)?; } - let fault: Bound<'py, PyAny> = enum_member(py, RecordId::LintSeverity, "FAULT")?; let faults: Vec> = result .faults - .into_iter() - .map(|item| { - new_record_with(py, RecordId::LintViolation, &[ - ("file_path", files[item.file].0.clone()), - ("line", item.line.into_pyobject(py)?.into_any()), - ("column", 1_usize.into_pyobject(py)?.into_any()), - ("code", PyString::new(py, FORMAT_UNSAFE_CODE).into_any()), - ( - "message", - PyString::new( - py, - &format!( - "File left unchanged because its SQL body could not be formatted: {}", - item.reason - ), - ) - .into_any(), - ), - ("severity", fault.clone()), - ("engine", PyString::new(py, NATIVE_ENGINE).into_any()), - ( - "remediation", - PyString::new(py, FORMAT_UNSAFE_REMEDIATION).into_any(), - ), - ], - ) - }) + .iter() + .map(|item| unsafe_fault(&files[item.file].0, item)) .collect::>()?; Ok((formatted, faults)) } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/rules/native_header_parser.rs b/crates/sqlbuild-python/src/bindings/_helpers/rules/native_header_parser.rs index 8e73fc1c8a..64879da3a3 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/rules/native_header_parser.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/rules/native_header_parser.rs @@ -7,7 +7,7 @@ use sqlbuild_rules::dsl_headers::main::inner_header_text::inner_header_text; use sqlbuild_rules::dsl_headers::models::{HeaderSpan, ParseFailure, ParseOutcome}; use sqlbuild_rules::dsl_headers::types::HeaderParser; use sqlbuild_sqltext::compiler::main::header_word_value::header_word_value; -use sqlbuild_sqltext::compiler::main::model_header_parsing::parse_batch; +use sqlbuild_sqltext::compiler::main::model_header_single_parsing::parse_one; use sqlbuild_sqltext::compiler::models::{AuthoredValue, HeaderWord}; use crate::bindings::_helpers::boundary::panics::value_error; @@ -45,8 +45,7 @@ impl HeaderParser for NativeHeaderParser { type Error = String; fn parse(&self, kind: &str, inner: &str) -> ParseOutcome { - let headers: Vec = vec![inner.to_owned()]; - let parsed = match catch_compiler_panic(|| parse_batch(&headers)) { + let parsed = match catch_compiler_panic(|| Ok(parse_one(inner))) { Ok(parsed) => parsed, Err(message) => { return ParseOutcome::Raised( @@ -58,7 +57,7 @@ impl HeaderParser for NativeHeaderParser { ); } }; - let (value, _, error) = parsed.into_iter().next().unwrap_or((None, None, None)); + let (value, _, error) = parsed; if let Some(value) = value { return ParseOutcome::Parsed(value); } diff --git a/crates/sqlbuild-rules/src/dsl_headers/_helpers/scan.rs b/crates/sqlbuild-rules/src/dsl_headers/_helpers/scan.rs index 16c9a0567e..e87e7c25da 100644 --- a/crates/sqlbuild-rules/src/dsl_headers/_helpers/scan.rs +++ b/crates/sqlbuild-rules/src/dsl_headers/_helpers/scan.rs @@ -54,10 +54,14 @@ fn leading_kind<'k>(text: &[char], index: usize, kinds: &[&'k str]) -> Option<(& start += 1; } kinds.iter().find_map(|kind| { - let keyword: Vec = kind.chars().collect(); - let end: usize = start + keyword.len(); - (text[start..].starts_with(&keyword) && !text.get(end).is_some_and(|next| is_word(*next))) - .then_some((*kind, start)) + let mut end: usize = start; + for expected in kind.chars() { + if text.get(end) != Some(&expected) { + return None; + } + end += 1; + } + (!text.get(end).is_some_and(|next| is_word(*next))).then_some((*kind, start)) }) } diff --git a/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs b/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs index 8c41ac461f..c016eccf54 100644 --- a/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs +++ b/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs @@ -8,8 +8,8 @@ use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::sql_lint::_helpers::formatter_syntax::{cte_macro_indices, protect_syntax}; use crate::sql_lint::_helpers::printed_layout::{PrintedSql, WrapOptions, wrap_lines}; use crate::sql_lint::_helpers::token_layout::{ - AuthoredSql, KeywordRecase, comments_in, neutralize_comments, print_authored_tokens, - verify_token_invariant, + AuthoredSql, KeywordRecase, OracleLayout, comments_in, neutralize_comments, oracle_layout, + print_authored_tokens, tokenize, verify_token_invariant, }; use crate::sql_lint::constants::{ CAST_TYPE_SEPARATOR_KEYWORD, CLOSE_PARENTHESIS, LINT_API_VERSION, OPEN_PARENTHESIS, @@ -91,8 +91,8 @@ fn try_format_sql(request: FormatRequest) -> Result { dialect: &dialect, wrap: wrap.as_ref(), }; - let (formatted, oracle) = format_layout(&original, &layout, &parse_options)?; - if relayout(&formatted, &oracle, &layout)? != formatted { + let (formatted, formatted_tokens, oracle) = format_layout(&original, &layout, &parse_options)?; + if relayout((&formatted, &formatted_tokens), oracle.as_ref(), &layout)? != formatted { return Err(NOT_IDEMPOTENT_FAILURE.to_string()); } let changed = formatted != original; @@ -105,18 +105,21 @@ fn try_format_sql(request: FormatRequest) -> Result { }) } +/// The printed output of [`format_layout`], its tokens, and the oracle it was printed against. +type FormattedLayout = (String, Vec, Option); + /// Print the tokens of `sql` with parse-tree whitespace; return the output and its oracle. fn format_layout( sql: &str, layout: &Layout<'_>, parse_options: &ParseOptions, -) -> Result<(String, String), String> { +) -> Result { let dialect = layout.dialect; let tokens = dialect .tokenize(sql) .map_err(|error| format!("native formatter tokenization failed: {error}"))?; if tokens.is_empty() { - return Ok((sql.to_string(), sql.to_string())); + return Ok((sql.to_string(), tokens, None)); } let comments = comments_in(sql, &tokens); let neutral = neutralize_comments(sql, &comments); @@ -125,30 +128,39 @@ fn format_layout( .parse_with_options(&validation_sql.sql, parse_options) .map_err(|error| format!("native formatter could not parse SQL: {error}"))?; let semantic_tokens = without_statement_terminators(&tokens); - let oracle = layout_oracle(&neutral, &semantic_tokens, dialect, parse_options)?; + let oracle = oracle_layout( + &layout_oracle(&neutral, &semantic_tokens, dialect, parse_options)?, + dialect, + )?; let authored = AuthoredSql { sql, tokens: &tokens, comments: &comments, }; let (printed, recases) = layout.print(&authored, &oracle)?; - verify_token_invariant(sql, &printed, dialect, &recases)?; - Ok((printed, oracle)) + let printed_tokens = tokenize(&printed, dialect)?; + verify_token_invariant( + (sql, &tokens), + (&printed, &printed_tokens), + dialect, + &recases, + )?; + Ok((printed, printed_tokens, Some(oracle))) } /// Re-print formatted SQL against its oracle; unchanged tokens imply an unchanged layout. -fn relayout(formatted: &str, oracle: &str, layout: &Layout<'_>) -> Result { - let tokens = layout - .dialect - .tokenize(formatted) - .map_err(|error| format!("native formatter tokenization failed: {error}"))?; - if tokens.is_empty() { +fn relayout( + (formatted, tokens): (&str, &[Token]), + oracle: Option<&OracleLayout>, + layout: &Layout<'_>, +) -> Result { + let Some(oracle) = oracle.filter(|_| !tokens.is_empty()) else { return Ok(formatted.to_string()); - } - let comments = comments_in(formatted, &tokens); + }; + let comments = comments_in(formatted, tokens); let authored = AuthoredSql { sql: formatted, - tokens: &tokens, + tokens, comments: &comments, }; Ok(layout.print(&authored, oracle)?.0) @@ -164,7 +176,7 @@ impl Layout<'_> { fn print( &self, authored: &AuthoredSql<'_>, - oracle: &str, + oracle: &OracleLayout, ) -> Result<(String, BTreeMap), String> { let (printed, recases) = print_authored_tokens(authored, oracle, self.dialect)?; let no_widths: HashMap = HashMap::new(); diff --git a/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs b/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs index eac207cbb8..80d196c96e 100644 --- a/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs +++ b/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs @@ -157,10 +157,27 @@ pub(crate) struct AuthoredSql<'a> { pub(crate) comments: &'a [Comment], } +/// The lexemes and characters of a layout oracle, read once for every print against it. +pub(crate) struct OracleLayout { + generated: Vec, + characters: Vec, +} + +/// Tokenize `oracle` for printing against it. +pub(crate) fn oracle_layout(oracle: &str, dialect: &Dialect) -> Result { + let oracle_tokens = dialect + .tokenize(oracle) + .map_err(|error| error.to_string())?; + Ok(OracleLayout { + generated: lexemes(oracle, &oracle_tokens, None)?, + characters: oracle.chars().collect(), + }) +} + /// Print every authored token in order, taking only whitespace and keyword case from `oracle`. pub(crate) fn print_authored_tokens( source: &AuthoredSql<'_>, - oracle: &str, + oracle: &OracleLayout, dialect: &Dialect, ) -> Result<(String, BTreeMap), String> { let AuthoredSql { @@ -169,23 +186,21 @@ pub(crate) fn print_authored_tokens( comments, } = *source; let authored = lexemes(sql, tokens, Some(dialect))?; - let oracle_tokens = dialect - .tokenize(oracle) - .map_err(|error| error.to_string())?; - let generated = lexemes(oracle, &oracle_tokens, None)?; - let alignment = align(&authored, &generated)?; - let oracle_characters: Vec = oracle.chars().collect(); - let separators = separators(&authored, &generated, &alignment, &oracle_characters)?; + let generated = &oracle.generated; + let alignment = align(&authored, generated)?; + let separators = separators(&authored, generated, &alignment, &oracle.characters)?; let attached = attach_comments(tokens, comments); let mut output = String::with_capacity(sql.len() + sql.len() / 4); let string_breaks = string_breaks(sql, tokens); let mut recases: BTreeMap = BTreeMap::new(); for (index, lexeme) in authored.iter().enumerate() { let forced_break = string_breaks.contains(&index) && !separators[index].contains('\n'); - let separator = &if forced_break { - format!("\n{}", current_indentation(&output)) + let forced: String; + let separator: &str = if forced_break { + forced = format!("\n{}", current_indentation(&output)); + &forced } else { - separators[index].clone() + &separators[index] }; let leading = &attached[index].leading; if leading.is_empty() { @@ -246,20 +261,18 @@ pub(crate) fn print_authored_tokens( /// Refuse output that changes anything but layout, fold-set case, and recorded keyword recases. pub(crate) fn verify_token_invariant( - before: &str, - after: &str, + (before, before_tokens): (&str, &[Token]), + (after, after_tokens): (&str, &[Token]), dialect: &Dialect, recases: &BTreeMap, ) -> Result<(), String> { - let before_tokens = tokenize(before, dialect)?; - let after_tokens = tokenize(after, dialect)?; - let before_canonical = canonical_tokens(before, &before_tokens, dialect)?; - let after_canonical = canonical_tokens(after, &after_tokens, dialect)?; + let before_canonical = canonical_tokens(before, before_tokens, dialect)?; + let after_canonical = canonical_tokens(after, after_tokens, dialect)?; if before_canonical.len() != after_canonical.len() { return Err(TOKEN_PRESERVATION_FAILURE.to_string()); } - let before_texts = token_texts(before, &before_tokens)?; - let after_texts = token_texts(after, &after_tokens)?; + let before_texts = token_texts(before, before_tokens)?; + let after_texts = token_texts(after, after_tokens)?; for (index, (left, right)) in before_canonical.iter().zip(&after_canonical).enumerate() { if left != right && !recases @@ -269,10 +282,10 @@ pub(crate) fn verify_token_invariant( return Err(TOKEN_PRESERVATION_FAILURE.to_string()); } } - if comment_positions(before, &before_tokens) != comment_positions(after, &after_tokens) { + if comment_positions(before, before_tokens) != comment_positions(after, after_tokens) { return Err(COMMENT_ATTACHMENT_FAILURE.to_string()); } - if !string_breaks(before, &before_tokens).is_subset(&string_breaks(after, &after_tokens)) { + if !string_breaks(before, before_tokens).is_subset(&string_breaks(after, after_tokens)) { return Err(STRING_BREAK_FAILURE.to_string()); } Ok(()) @@ -288,7 +301,7 @@ impl KeywordRecase { } } -fn tokenize(sql: &str, dialect: &Dialect) -> Result, String> { +pub(crate) fn tokenize(sql: &str, dialect: &Dialect) -> Result, String> { dialect.tokenize(sql).map_err(|error| error.to_string()) } diff --git a/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs b/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs index 3d98f99548..af36b480a1 100644 --- a/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs +++ b/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs @@ -2,7 +2,7 @@ use serde_json::{Value, json}; use std::collections::BTreeMap; -use crate::sql_lint::_helpers::token_layout::{KeywordRecase, verify_token_invariant}; +use crate::sql_lint::_helpers::token_layout::{KeywordRecase, tokenize, verify_token_invariant}; use crate::sql_lint::main::formatter::format_json; use crate::sql_lint::tests::test_types; @@ -394,7 +394,14 @@ fn given_formatted_candidates_when_checking_token_invariant_then_only_layout_may ) }) .collect(); - let actual = verify_token_invariant(test_case.before, test_case.after, &dialect, &recases); + let before_tokens = tokenize(test_case.before, &dialect).expect("before tokenizes"); + let after_tokens = tokenize(test_case.after, &dialect).expect("after tokenizes"); + let actual = verify_token_invariant( + (test_case.before, &before_tokens), + (test_case.after, &after_tokens), + &dialect, + &recases, + ); assert_eq!( actual.err().as_deref(), test_case.expected_error, diff --git a/fensu.toml b/fensu.toml index c401ef48db..cc5bec7318 100644 --- a/fensu.toml +++ b/fensu.toml @@ -2338,7 +2338,7 @@ pending_budgets = [ "crates/sqlbuild-python/src/bindings/_helpers/fingerprints/storage_io.rs=12", "crates/sqlbuild-python/src/bindings/_helpers/format_command/command.rs=2", "crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs=1", - "crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs=7", + "crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs=6", "crates/sqlbuild-python/src/bindings/_helpers/format_command/selection.rs=4", "crates/sqlbuild-python/src/bindings/_helpers/freshness/freshness_observe.rs=1", "crates/sqlbuild-python/src/bindings/_helpers/freshness/observation.rs=3", From 147ab07d953f074c7497d73635caac430485a7d0 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Tue, 20 Oct 2026 21:17:26 +0100 Subject: [PATCH 2/6] perf(format): format files in parallel on dedicated worker threads Header lint, header formatting, body preparation and splicing, rewrite checks, suppression scanning and file writes now run across a format worker pool with 16 MiB stacks. Results are collected in file order and the first failure in file order is reported, so output, ordering and exit codes do not depend on scheduling. Python work (fixture contracts, Rule fixes, description lookups and report records) stays on the calling thread. --- .../bindings/_helpers/format_command/files.rs | 62 +++++++++++++------ .../bindings/_helpers/format_command/run.rs | 5 +- .../_helpers/format_command/stages.rs | 33 +++++++++- .../src/lint_text/main/format_sql_bodies.rs | 25 ++++++-- .../main/reject_inline_suppressions.rs | 15 ++++- 5 files changed, 106 insertions(+), 34 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs index 9fcfa8a121..2928f1dca6 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs @@ -8,6 +8,9 @@ use std::time::{SystemTime, UNIX_EPOCH}; use pyo3::exceptions::PyOSError; use pyo3::prelude::{Bound, PyAny, PyModule, PyModuleMethods, PyResult, Python}; use pyo3::{IntoPyObject, pyfunction, wrap_pyfunction}; +use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; + +use crate::bindings::_helpers::format_command::stages::on_format_workers; const LINE_FEED: &str = "\n"; const CARRIAGE_RETURN_LINE_FEED: &str = "\r\n"; @@ -60,12 +63,17 @@ fn write_replacing(path: &Path, temporary: &Path, contents: &str) -> std::io::Re fs::rename(temporary, path) } -/// Replace a text file atomically, keeping its permissions. -fn write_atomically(path: &Path, contents: &str) -> PyResult<()> { +/// Replace a text file atomically, keeping its permissions; the error names the path. +fn replace_file(path: &Path, contents: &str) -> Result<(), String> { let temporary: PathBuf = temporary_path(path); let written: std::io::Result<()> = write_replacing(path, &temporary, contents); let _ = fs::remove_file(&temporary); - written.map_err(|error| PyOSError::new_err(format!("{}: {error}", path.display()))) + written.map_err(|error| format!("{}: {error}", path.display())) +} + +/// Replace a text file atomically, keeping its permissions. +fn write_atomically(path: &Path, contents: &str) -> PyResult<()> { + replace_file(path, contents).map_err(PyOSError::new_err) } /// `write_atomically(*, path, contents)`: replace a text file atomically, keeping its mode. @@ -79,27 +87,41 @@ pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_function(wrap_pyfunction!(write_file_atomically, module)?) } -/// The changed files of `writes`, in their order, written in place when `write` is set. +/// The changed files of `writes`, in their order, written in place across the worker threads +/// when `write` is set; a failed write raises the first failure in order. pub(crate) fn persist_changes<'py>( py: Python<'py>, writes: &[PendingWrite<'_>], write: bool, ) -> PyResult>> { - let mut changes: Vec> = Vec::new(); - for pending in writes { - let after: String = with_newline_style(pending.contents, pending.newline); - let before: String = with_newline_style(pending.authored, pending.newline); - if after == before { - continue; - } - if write { - write_atomically(pending.path, &after)?; - } - changes.push(FormatChange { - path: pending.path.into_pyobject(py)?.into_any(), - before, - after, - }); + let changed: Vec<(&Path, String, String)> = writes + .iter() + .filter_map(|pending| { + let after: String = with_newline_style(pending.contents, pending.newline); + let before: String = with_newline_style(pending.authored, pending.newline); + (after != before).then_some((pending.path, before, after)) + }) + .collect(); + if write { + py.detach(|| { + on_format_workers(|| { + let written: Vec> = changed + .par_iter() + .map(|(path, _, after)| replace_file(path, after)) + .collect(); + written.into_iter().collect::>() + }) + }) + .map_err(PyOSError::new_err)?; } - Ok(changes) + changed + .into_iter() + .map(|(path, before, after)| { + Ok(FormatChange { + path: path.into_pyobject(py)?.into_any(), + before, + after, + }) + }) + .collect() } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs index 12c0cc3cb4..b58e4d5714 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs @@ -24,7 +24,7 @@ use crate::bindings::_helpers::format_command::files::{ FormatChange, PendingWrite, persist_changes, }; use crate::bindings::_helpers::format_command::stages::{ - each_file, format_header_text, lint_header_text, parse_faults, + each_file, format_header_text, lint_header_text, on_format_workers, parse_faults, }; use crate::bindings::_helpers::records::construct::error_class; use crate::bindings::_helpers::records::fields::field as native_field; @@ -132,7 +132,8 @@ fn native( py: Python<'_>, operation: impl FnOnce() -> Result + Send, ) -> PyResult { - py.compiler_detach(operation).map_err(value_error) + py.compiler_detach(|| on_format_workers(operation)) + .map_err(value_error) } impl<'py> Run<'_, '_, 'py> { diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs index fc19a7338b..7d263fb0ce 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/stages.rs @@ -1,5 +1,9 @@ //! The per-file native work of a format run: header lint and header formatting. +use std::sync::OnceLock; + +use rayon::ThreadPool; +use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; use sqlbuild_rules::dsl_headers::constants::RULE_HEADER_PARSE; use sqlbuild_rules::dsl_headers::main::format_headers::format_headers; use sqlbuild_rules::dsl_headers::main::scan_headers::scan_headers; @@ -8,6 +12,28 @@ use sqlbuild_rules::dsl_headers::models::{HeaderRulesConfig, HeaderViolation}; use crate::bindings::_helpers::rules::dsl_headers::header_violations; use crate::bindings::_helpers::rules::native_header_parser::NativeHeaderParser; +/// Parsers recurse deeply on nested SQL, so format workers get the stack the main thread has. +const FORMAT_WORKER_STACK_BYTES: usize = 16 * 1024 * 1024; + +static FORMAT_WORKERS: OnceLock> = OnceLock::new(); + +/// Run `operation` on the format worker threads, which every parallel pass inside it uses. +pub(crate) fn on_format_workers( + operation: impl FnOnce() -> Result + Send, +) -> Result { + FORMAT_WORKERS + .get_or_init(|| { + rayon::ThreadPoolBuilder::new() + .thread_name(|index| format!("sqlbuild-format-{index}")) + .stack_size(FORMAT_WORKER_STACK_BYTES) + .build() + .map_err(|error| error.to_string()) + }) + .as_ref() + .map_err(Clone::clone)? + .install(operation) +} + /// Every header rule violation of `text`. pub(crate) fn lint_header_text( text: &str, @@ -36,10 +62,11 @@ pub(crate) fn format_header_text( Ok((formatted.into_iter().collect(), faults)) } -/// `operation` over every item, in item order; the first failure in item order is returned. +/// `operation` over every item on the worker threads; results and the first error keep item order. pub(crate) fn each_file( items: &[T], - operation: impl Fn(&T) -> Result + Sync, + operation: impl Fn(&T) -> Result + Sync + Send, ) -> Result, String> { - items.iter().map(operation).collect() + let results: Vec> = items.par_iter().map(operation).collect(); + results.into_iter().collect() } diff --git a/crates/sqlbuild-rules/src/lint_text/main/format_sql_bodies.rs b/crates/sqlbuild-rules/src/lint_text/main/format_sql_bodies.rs index f823ee3ce7..6bf08424e0 100644 --- a/crates/sqlbuild-rules/src/lint_text/main/format_sql_bodies.rs +++ b/crates/sqlbuild-rules/src/lint_text/main/format_sql_bodies.rs @@ -1,8 +1,12 @@ //! `format_sql_bodies`: SQLBuild-owned safe formatting of every SQL body in files. -use crate::lint_text::_helpers::format_bodies::{RequestTable, prepare_file, splice_file}; +use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator}; + +use crate::lint_text::_helpers::format_bodies::{ + PendingBody, RequestTable, prepare_file, splice_file, +}; use crate::lint_text::errors::LintTextError; -use crate::lint_text::models::{FormatFile, FormattedBodies, PreparedBody}; +use crate::lint_text::models::{FormatFault, FormatFile, FormattedBodies, PreparedBody}; use crate::sql_lint::main::format_requests::format_requests; use crate::sql_lint::models::FormatResponse; @@ -12,20 +16,29 @@ pub fn format_sql_bodies( dialect: &str, line_width: usize, ) -> Result { + let pending: Vec, LintTextError>> = files + .par_iter() + .map(|file| prepare_file(file, dialect)) + .collect(); let mut table: RequestTable = RequestTable::default(); let mut prepared: Vec> = Vec::with_capacity(files.len()); - for file in files { + for file_bodies in pending { let mut bodies: Vec = Vec::new(); - for (mut body, sql, widths) in prepare_file(file, dialect)? { + for (mut body, sql, widths) in file_bodies? { body.request = table.insert(sql, widths, (dialect, line_width)); bodies.push(body); } prepared.push(bodies); } let responses: Vec = format_requests(table.requests); + let spliced: Vec<(Option, Vec)> = files + .par_iter() + .zip(prepared.par_iter()) + .enumerate() + .map(|(index, (file, bodies))| splice_file(index, &file.contents, bodies, &responses)) + .collect(); let mut result: FormattedBodies = FormattedBodies::default(); - for (index, (file, bodies)) in files.iter().zip(prepared).enumerate() { - let (formatted, faults) = splice_file(index, &file.contents, &bodies, &responses); + for (index, (formatted, faults)) in spliced.into_iter().enumerate() { result.formatted.extend(formatted.map(|text| (index, text))); result.faults.extend(faults); } diff --git a/crates/sqlbuild-rules/src/lint_text/main/reject_inline_suppressions.rs b/crates/sqlbuild-rules/src/lint_text/main/reject_inline_suppressions.rs index 23cf26ff3f..9cb72a4222 100644 --- a/crates/sqlbuild-rules/src/lint_text/main/reject_inline_suppressions.rs +++ b/crates/sqlbuild-rules/src/lint_text/main/reject_inline_suppressions.rs @@ -1,5 +1,7 @@ //! `reject_inline_suppressions`: an error for every `sqb: ignore` directive in a SQL comment. +use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator}; + use crate::lint_text::_helpers::lines::{LineSpan, line_spans}; use crate::lint_text::_helpers::suppressions::directive_findings; use crate::lint_text::errors::LintTextError; @@ -9,10 +11,17 @@ use crate::lint_text::models::SuppressionFinding; pub fn reject_inline_suppressions( files: &[(&str, &str)], ) -> Result, LintTextError> { + let per_file: Vec, LintTextError>> = files + .par_iter() + .enumerate() + .map(|(file, (path, contents))| { + let lines: Vec> = line_spans(contents); + directive_findings(file, path, (contents, &lines)) + }) + .collect(); let mut findings: Vec = Vec::new(); - for (file, (path, contents)) in files.iter().enumerate() { - let lines: Vec> = line_spans(contents); - findings.extend(directive_findings(file, path, (contents, &lines))?); + for file_findings in per_file { + findings.extend(file_findings?); } Ok(findings) } From a09a0684c73334fb2c89ccf37632c806553e59a0 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Tue, 20 Oct 2026 22:05:27 +0100 Subject: [PATCH 3/6] perf(format): skip files a previous run found clean sqb format now remembers every file it found already formatted with nothing to report, in target/cache/compiler-native-v1/format-results.bin. Entries are keyed by the file's project-relative path, its exact text, the effective format and rule threshold settings and dialect, and the SQLBuild build, so an edit, a settings change or another build formats the file again; a damaged store opens empty. --fix never uses the cache, SQL test files are not skipped while fixture-null formatting reads model contracts, and the new --no-cache flag formats every file. --- Makefile | 1 + .../sqlbuild-cli/src/argv/command_table.json | 12 ++ crates/sqlbuild-cli/src/dispatch/constants.rs | 3 +- .../_helpers/compile_driver/stage_work.rs | 2 + .../bindings/_helpers/format_command/cache.rs | 84 ++++++++ .../_helpers/format_command/command.rs | 4 + .../bindings/_helpers/format_command/files.rs | 17 +- .../bindings/_helpers/format_command/mod.rs | 1 + .../_helpers/format_command/registration.rs | 9 +- .../bindings/_helpers/format_command/run.rs | 153 +++++++++++++-- .../sqlbuild/references/docs/cli/format.md | 16 ++ src/sqlbuild/_native.pyi | 2 + .../cli/commands/main/format/_test_types.py | 20 ++ .../cli/commands/main/format/helpers.py | 47 +++++ .../commands/main/format/test_format_cache.py | 127 +++++++++++++ .../sqlbuild/cli/commands/main/_test_types.py | 37 ++++ .../src/sqlbuild/cli/commands/main/helpers.py | 83 ++++++++ .../cli/commands/main/test_format_cache.py | 179 ++++++++++++++++++ website/src/content/docs/docs/cli/format.mdx | 15 ++ 19 files changed, 791 insertions(+), 21 deletions(-) create mode 100644 crates/sqlbuild-python/src/bindings/_helpers/format_command/cache.rs create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/format/test_format_cache.py create mode 100644 tests/integration/src/sqlbuild/cli/commands/main/test_format_cache.py diff --git a/Makefile b/Makefile index 677f6f4f16..1845611f69 100644 --- a/Makefile +++ b/Makefile @@ -161,6 +161,7 @@ E2E_DUCKDB_CLI_PATHS := \ tests/e2e/src/sqlbuild/cli/commands/main/databricks \ tests/e2e/src/sqlbuild/cli/commands/main/dbt \ tests/e2e/src/sqlbuild/cli/commands/main/debug \ + tests/e2e/src/sqlbuild/cli/commands/main/format/test_format_cache.py \ tests/e2e/src/sqlbuild/cli/commands/main/format/test_format_example_projects.py \ tests/e2e/src/sqlbuild/cli/commands/main/format/test_rule_fixes.py \ tests/e2e/src/sqlbuild/cli/commands/main/format/test_token_preservation.py \ diff --git a/crates/sqlbuild-cli/src/argv/command_table.json b/crates/sqlbuild-cli/src/argv/command_table.json index fd2f44b5d8..d4ec6cb684 100644 --- a/crates/sqlbuild-cli/src/argv/command_table.json +++ b/crates/sqlbuild-cli/src/argv/command_table.json @@ -2385,6 +2385,18 @@ "help": "only remove redundant typed-null SQL test fixture columns", "group": 1 }, + { + "kind": "store_true", + "options": [ + "--no-cache" + ], + "dest": "no_cache", + "nargs": 0, + "const": true, + "default": false, + "help": "Bypass the format result cache for this invocation", + "group": 1 + }, { "kind": "store", "dest": "format_paths", diff --git a/crates/sqlbuild-cli/src/dispatch/constants.rs b/crates/sqlbuild-cli/src/dispatch/constants.rs index e1240666b5..b32def4996 100644 --- a/crates/sqlbuild-cli/src/dispatch/constants.rs +++ b/crates/sqlbuild-cli/src/dispatch/constants.rs @@ -629,7 +629,7 @@ pub(crate) const SKILLS_ROUTE: Route = routed( }, ); -const FORMAT_ARGUMENTS: [Field; 9] = [ +const FORMAT_ARGUMENTS: [Field; 10] = [ PROJECT_DIR, field("select", Source::FormatSelect), EXCLUDE, @@ -637,6 +637,7 @@ const FORMAT_ARGUMENTS: [Field; 9] = [ attr("diff", "format_diff"), attr("fixtures_only", "format_fixtures_only"), attr("fix", "format_fix"), + same("no_cache"), JSON_OUTPUT, NO_COLOR, ]; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/stage_work.rs b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/stage_work.rs index cc89abc6c2..88d543f7fa 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/stage_work.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/compile_driver/stage_work.rs @@ -23,6 +23,8 @@ pub(crate) const LINEAGE_GRAPH_BUILDS: &str = "lineage_graph_builds"; pub(crate) const PROJECT_GRAPHS: &str = "build_project_graph"; pub(crate) const FORMAT_LINT_BATCHES: &str = "format_lint_batches"; pub(crate) const FORMAT_LINT_FILES: &str = "format_lint_files"; +pub(crate) const FORMAT_CACHE_HITS: &str = "format_cache_hits"; +pub(crate) const FORMAT_CACHE_MISSES: &str = "format_cache_misses"; pub(crate) const SQL_TEST_ARTIFACT_PLANS: &str = "plan_compiled_sql_test_artifacts"; pub(crate) const SQL_TEST_MODEL_CHAINS: &str = "sql_test_model_chain_names_by_key"; pub(crate) const BACKGROUND_SQL_TEST_PLANS: &str = "background_sql_test_plans"; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/cache.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/cache.rs new file mode 100644 index 0000000000..a4c6ae3c1f --- /dev/null +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/cache.rs @@ -0,0 +1,84 @@ +//! Files already formatted and fault-free, kept in the shared native store to skip next time. + +use std::path::{Path, PathBuf}; + +use pyo3::exceptions::PyOSError; +use pyo3::prelude::{PyResult, Python}; +use sqlbuild_cache::digest::main::content_digest::content_digest; +use sqlbuild_cache::digest::main::hex_digest::hex_digest; +use sqlbuild_cache::digest::types::ContentDigest; +use sqlbuild_cache::store::models::NativeStore; + +use crate::bindings::_helpers::cache::native_store::{ + current_code_identity, open_store, save_store, +}; +use crate::bindings::_helpers::compile_command::constants::TARGET_DIRECTORY_NAME; +use crate::bindings::_helpers::frontier::constants::{ + CACHE_DIRECTORY_NAME, COMPILER_CACHE_DIRECTORY_NAME, NATIVE_CACHE_NAMESPACE_SUFFIX, +}; + +/// The native store kind holding clean format results. +const FORMAT_RESULT_STORE_KIND: &str = "format-results"; +/// The store file below the compile cache folder. +pub(crate) const FORMAT_RESULT_STORE_FILE_NAME: &str = "format-results.bin"; +/// Bumped whenever what a clean result means changes; older stores open empty. +const FORMAT_RESULT_STORE_VERSION: &str = "format-result-store-v1"; +/// The one value stored: formatting left the file unchanged and reported nothing about it. +const CLEAN: &[u8] = b"clean"; + +/// One run's view of the stored clean results, keyed by file, text and effective settings. +pub(crate) struct FormatCache { + store: NativeStore, + path: PathBuf, + settings: String, +} + +impl FormatCache { + /// The project's store for this SQLBuild build, or `None` when it cannot be read. + pub(crate) fn open( + py: Python<'_>, + project_dir: &Path, + settings: String, + ) -> PyResult> { + let path: PathBuf = project_dir + .join(TARGET_DIRECTORY_NAME) + .join(CACHE_DIRECTORY_NAME) + .join(format!( + "{COMPILER_CACHE_DIRECTORY_NAME}{NATIVE_CACHE_NAMESPACE_SUFFIX}" + )) + .join(FORMAT_RESULT_STORE_FILE_NAME); + let environment: String = hex_digest(&content_digest(&[ + FORMAT_RESULT_STORE_VERSION, + ¤t_code_identity(py)?, + ])); + match open_store(py, &path, FORMAT_RESULT_STORE_KIND, &environment) { + Ok(store) => Ok(Some(Self { + store, + path, + settings, + })), + Err(error) if error.is_instance_of::(py) => Ok(None), + Err(error) => Err(error), + } + } + + /// The key of one file: its project-relative path, its exact text and the run's settings. + pub(crate) fn key(&self, relative_path: &str, contents: &str) -> ContentDigest { + content_digest(&[relative_path, contents, &self.settings]) + } + + /// Whether a previous run found the file under `key` clean. + pub(crate) fn is_clean(&mut self, key: &ContentDigest) -> bool { + self.store.get(key) == Some(CLEAN) + } + + /// Remember that the file under `key` formatted cleanly. + pub(crate) fn record_clean(&mut self, key: ContentDigest) { + self.store.put(key, CLEAN.to_vec()); + } + + /// Save newly recorded results; a store that cannot be written is left as it was. + pub(crate) fn save(&self, py: Python<'_>) { + let _ = save_store(py, &self.store, &self.path, &[]); + } +} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/command.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/command.rs index 46a9cc1923..93945309cb 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/command.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/command.rs @@ -41,6 +41,7 @@ struct FormatFlags { fix: bool, json_output: bool, use_color: bool, + no_cache: bool, } /// The adapter, selected files and discovered inputs a run formats with. @@ -227,6 +228,7 @@ fn run_command( fixtures_only: flags.fixtures_only, write: !(flags.check || flags.diff), fix: flags.fix, + cache: !(flags.no_cache || flags.fix), }; let (outcome, status, elapsed) = format_with_status(py, &request, flags.use_color)?; let root: PathBuf = project_dir.extract()?; @@ -279,6 +281,7 @@ pub(crate) fn run_format_command<'py>( "fix", "json_output", "no_color", + "no_cache", ], ), )?; @@ -296,6 +299,7 @@ pub(crate) fn run_format_command<'py>( fix: arguments.flag("fix")?, json_output: arguments.flag("json_output")?, use_color, + no_cache: arguments.flag("no_cache")?, }; let select: Vec = string_tuple(&arguments.value("select")?)?; let exclude: Vec = string_tuple(&arguments.value("exclude")?)?; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs index 2928f1dca6..2233453d5d 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs @@ -18,12 +18,15 @@ const CARRIAGE_RETURN_LINE_FEED: &str = "\r\n"; /// One file the formatter would change: its path and its text before and after. pub(crate) struct FormatChange<'py> { pub(crate) path: Bound<'py, PyAny>, + /// The run's index of the changed file. + pub(crate) entry: usize, pub(crate) before: String, pub(crate) after: String, } /// One file a format run would leave in its final text. pub(crate) struct PendingWrite<'t> { + pub(crate) entry: usize, pub(crate) path: &'t Path, /// The text read, or empty for a file the run did not collect. pub(crate) authored: &'t str, @@ -87,19 +90,18 @@ pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_function(wrap_pyfunction!(write_file_atomically, module)?) } -/// The changed files of `writes`, in their order, written in place across the worker threads -/// when `write` is set; a failed write raises the first failure in order. +/// The changed files of `writes` in order, written in parallel when `write` is set. pub(crate) fn persist_changes<'py>( py: Python<'py>, writes: &[PendingWrite<'_>], write: bool, ) -> PyResult>> { - let changed: Vec<(&Path, String, String)> = writes + let changed: Vec<(&PendingWrite<'_>, String, String)> = writes .iter() .filter_map(|pending| { let after: String = with_newline_style(pending.contents, pending.newline); let before: String = with_newline_style(pending.authored, pending.newline); - (after != before).then_some((pending.path, before, after)) + (after != before).then_some((pending, before, after)) }) .collect(); if write { @@ -107,7 +109,7 @@ pub(crate) fn persist_changes<'py>( on_format_workers(|| { let written: Vec> = changed .par_iter() - .map(|(path, _, after)| replace_file(path, after)) + .map(|(pending, _, after)| replace_file(pending.path, after)) .collect(); written.into_iter().collect::>() }) @@ -116,9 +118,10 @@ pub(crate) fn persist_changes<'py>( } changed .into_iter() - .map(|(path, before, after)| { + .map(|(pending, before, after)| { Ok(FormatChange { - path: path.into_pyobject(py)?.into_any(), + path: pending.path.into_pyobject(py)?.into_any(), + entry: pending.entry, before, after, }) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs index 0a1e237715..8bfb1b8f28 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod cache; pub(crate) mod command; pub(crate) mod config; pub(crate) mod files; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/registration.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/registration.rs index 1b00100a44..28114d91b5 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/registration.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/registration.rs @@ -1,10 +1,15 @@ //! Register the native `sqb format` command and its atomic file writer. -use pyo3::prelude::{Bound, PyModule, PyResult}; +use pyo3::prelude::{Bound, PyModule, PyModuleMethods, PyResult}; +use crate::bindings::_helpers::format_command::cache::FORMAT_RESULT_STORE_FILE_NAME; use crate::bindings::_helpers::format_command::{command, files}; pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { command::register(module)?; - files::register(module) + files::register(module)?; + module.add( + "FORMAT_RESULT_STORE_FILE_NAME", + FORMAT_RESULT_STORE_FILE_NAME, + ) } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs index b58e4d5714..f6b68e23ce 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/run.rs @@ -1,11 +1,13 @@ //! Format every authored SQL file and collect the violations that remain. use std::collections::{BTreeSet, HashMap}; -use std::path::PathBuf; +use std::fs; +use std::path::{Path, PathBuf}; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, Python}; use pyo3::types::{PyBool, PyDict, PyDictMethods}; use pyo3::{IntoPyObject, IntoPyObjectExt}; +use sqlbuild_cache::digest::types::ContentDigest; use sqlbuild_core::format_command::main::collect_lint_files::collect_lint_files; use sqlbuild_rules::dsl_headers::models::{HeaderRulesConfig, HeaderViolation}; use sqlbuild_rules::lint_text::main::format_sql_bodies::format_sql_bodies; @@ -17,9 +19,10 @@ use sqlbuild_rules::lint_text::models::{ use crate::bindings::_helpers::boundary::panics::value_error; use crate::bindings::_helpers::boundary::python_calls::keywords; use crate::bindings::_helpers::compile_driver::stage_work::{ - FORMAT_LINT_BATCHES, FORMAT_LINT_FILES, count_stage, + FORMAT_CACHE_HITS, FORMAT_CACHE_MISSES, FORMAT_LINT_BATCHES, FORMAT_LINT_FILES, count_stage, }; use crate::bindings::_helpers::discovery::description_inputs::externally_described_models; +use crate::bindings::_helpers::format_command::cache::FormatCache; use crate::bindings::_helpers::format_command::files::{ FormatChange, PendingWrite, persist_changes, }; @@ -51,6 +54,8 @@ pub(crate) struct FormatRequest<'a, 'py> { pub(crate) fixtures_only: bool, pub(crate) write: bool, pub(crate) fix: bool, + /// Whether files a previous run found clean are skipped, and clean files remembered. + pub(crate) cache: bool, } /// The outcome of one format run, with Python violations and Rule fix reports. @@ -61,7 +66,7 @@ pub(crate) struct FormatOutcome<'py> { pub(crate) rule_fixes: Vec>, /// Paths whose formatting was declined, so their applied fixes were not kept. pub(crate) declined: BTreeSet, - /// The final text of every collected file. + /// The final text of every collected file a violation may quote. pub(crate) final_contents: HashMap, } @@ -501,6 +506,91 @@ fn lint_final_contents<'py>( Ok(findings) } +/// The entries `findings` are about; Python findings come only from uncached Rule fixes. +fn dirty_entries<'f>(findings: &'f [Finding<'_>]) -> impl Iterator + 'f { + findings.iter().filter_map(|finding| match finding { + Finding::Header(index, _) | Finding::Unsafe(index, _) | Finding::Suppression(index, _) => { + Some(*index) + } + Finding::Python(_) => None, + }) +} + +/// The effective settings that shape formatting, part of every cache key. +fn cache_settings(request: &FormatRequest<'_, '_>) -> PyResult { + let config: &Bound<'_, PyAny> = request.config; + let mut settings: Vec = Vec::new(); + for name in [ + "line_width", + "max_description_lines", + "max_literal_length", + "max_ranking_order_by", + ] { + settings.push(native_field(config, name)?.extract::()?.to_string()); + } + settings.push(native_field(config, "dialect")?.extract()?); + settings.push(request.fixtures_only.to_string()); + Ok(settings.join("\0")) +} + +/// SQL test files fixture-null formatting rewrites from model contracts, never skipped. +fn fixture_paths( + directory: &Path, + fixture_inputs: Option<&(Bound<'_, PyAny>, Bound<'_, PyAny>)>, +) -> PyResult> { + let Some((discovered, _)) = fixture_inputs else { + return Ok(BTreeSet::new()); + }; + let mut paths: BTreeSet = BTreeSet::new(); + for file in native_field(discovered, "test_files")?.try_iter()? { + let relative: PathBuf = native_field(&file?, "relative_path")?.extract()?; + let path: PathBuf = directory.join(relative); + let _ = paths.insert(fs::canonicalize(&path).unwrap_or_else(|_| path.clone())); + let _ = paths.insert(path); + } + Ok(paths) +} + +/// Collected files not cached as clean, each with its cache key, or none when not skippable. +fn uncached_entries( + py: Python<'_>, + (cache, uncacheable): (&mut FormatCache, &BTreeSet), + directory: &Path, + collected: Vec<(PathBuf, String)>, +) -> PyResult<(Vec, Vec>)> { + let shared: &FormatCache = cache; + let keys: Vec> = native(py, || { + each_file(&collected, |(path, contents)| { + let skippable: bool = uncacheable.is_empty() + || !(uncacheable.contains(path) + || fs::canonicalize(path) + .is_ok_and(|resolved| uncacheable.contains(&resolved))); + Ok(skippable.then(|| shared.key(&project_relative_path(path, directory), contents))) + }) + })?; + let mut entries: Vec = Vec::new(); + let mut kept: Vec> = Vec::new(); + let (mut hits, mut misses): (u64, u64) = (0, 0); + for ((path, original), key) in collected.into_iter().zip(keys) { + if let Some(key) = key.as_ref() { + if cache.is_clean(key) { + hits += 1; + continue; + } + misses += 1; + } + entries.push(Entry { + path, + original, + collected: true, + }); + kept.push(key); + } + count_stage(py, FORMAT_CACHE_HITS, hits)?; + count_stage(py, FORMAT_CACHE_MISSES, misses)?; + Ok((entries, kept)) +} + /// Format the project's authored SQL files and report what remains. pub(crate) fn run_format<'py>(request: &FormatRequest<'_, 'py>) -> PyResult> { let py: Python<'py> = request.project_dir.py(); @@ -521,14 +611,30 @@ pub(crate) fn run_format<'py>(request: &FormatRequest<'_, 'py>) -> PyResult = collect_lint_files(&directory, request.selected) .map_err(|error| pyo3::exceptions::PyOSError::new_err(error.to_string()))?; let files_checked: usize = collected.len(); - let entries: Vec = collected - .into_iter() - .map(|(path, original)| Entry { - path, - original, - collected: true, - }) - .collect(); + let mut cache: Option = if request.cache { + FormatCache::open(py, &directory, cache_settings(request)?)? + } else { + None + }; + let (entries, keys): (Vec, Vec>) = match cache.as_mut() { + Some(cache) => { + let uncacheable: BTreeSet = + fixture_paths(&directory, fixture_inputs.as_ref())?; + uncached_entries(py, (cache, &uncacheable), &directory, collected)? + } + None => { + let entries: Vec = collected + .into_iter() + .map(|(path, original)| Entry { + path, + original, + collected: true, + }) + .collect(); + let keys: Vec> = vec![None; entries.len()]; + (entries, keys) + } + }; let config: HeaderRulesConfig = rules_config(request.config)?; let original_lints: Vec> = native(py, || { each_file(&entries, |entry| lint_header_text(&entry.original, config)) @@ -582,6 +688,7 @@ pub(crate) fn run_format<'py>(request: &FormatRequest<'_, 'py>) -> PyResult(request: &FormatRequest<'_, 'py>) -> PyResult = dirty_entries(&findings).collect(); + let python_findings: bool = findings + .iter() + .any(|finding| matches!(finding, Finding::Python(_))); let violations: Vec> = drop_resolved_description_faults( request.project_dir, violation_records(py, &run.entries, findings)?, @@ -630,10 +741,30 @@ pub(crate) fn run_format<'py>(request: &FormatRequest<'_, 'py>) -> PyResult = changes + .iter() + .map(|change| change.entry) + .chain(findings_seen.iter().copied()) + .chain(dirty_entries(&suppressions)) + .collect(); + for (index, key) in keys.iter().enumerate() { + if let Some(key) = key.filter(|_| !dirty.contains(&index)) { + cache.record_clean(key); + } + } + cache.save(py); + } + let reported: BTreeSet = findings_seen + .iter() + .copied() + .chain(dirty_entries(&suppressions)) + .collect(); let mut violations: Vec> = violations; violations.extend(violation_records(py, &run.entries, suppressions)?); let final_contents: HashMap = collected .iter() + .filter(|index| python_findings || reported.contains(index)) .map(|index| { let entry: &Entry = &run.entries[*index]; let text: &str = final_texts diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md index 1662f04eb3..7696692cbe 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md @@ -8,6 +8,7 @@ Online: https://sqlbuild.com/docs/cli/format/ ## Contents +- Skipping unchanged files - Line width and descriptions - What formatting preserves - Rule fixes @@ -25,6 +26,7 @@ sqb format [flags] | `--diff` | Print the proposed diff without writing | | `--fix` | Also apply compiler-verified, meaning-preserving Rule fixes before layout formatting | | `--json` | Print structured results | +| `--no-cache` | Format every file again instead of skipping files a previous run found clean | | `--select`, `-s` | Format selected models | | `--select-file` | Read selectors from a file | | `--exclude` | Exclude models from the selected scope | @@ -38,6 +40,20 @@ sqb format --diff --select tag:marts Formatting owns presentation and source rewriting. Compiler-integrated diagnostics belong to configured [Rules](../concepts/rules.md), which never mutate source. +Files are formatted in parallel; the report, the diff and the exit code are the same as a +one-file-at-a-time run. + +## Skipping unchanged files + +`sqb format` remembers every file it found already formatted and free of faults and warnings, in +`target/cache/compiler-native-v1/format-results.bin`. A later run skips such a file while its text, +its path, the `[format]` and rule threshold settings, the adapter dialect and the SQLBuild build are +all unchanged, so `sqb format --check` on an unchanged project costs little more than reading the +files. A file that needed changes or reported anything is always formatted again, and a damaged or +unreadable cache is ignored. `--fix` never uses the cache, and SQL test files are not skipped +while typed-null fixture formatting reads their models' contracts. Pass `--no-cache` to format +every file. + ## Line width and descriptions Configure the maximum physical line width in `sqlbuild_project.toml` (the default is `100`): diff --git a/src/sqlbuild/_native.pyi b/src/sqlbuild/_native.pyi index 991ee32b6f..8becb121ff 100644 --- a/src/sqlbuild/_native.pyi +++ b/src/sqlbuild/_native.pyi @@ -5344,6 +5344,7 @@ class SqlTestScanCache: def save(self) -> None: ... SQL_TEST_SCAN_STORE_FILE_NAME: str +FORMAT_RESULT_STORE_FILE_NAME: str def content_digest(parts: list[str]) -> str: ... def compiled_code_identity() -> str: ... @@ -5796,6 +5797,7 @@ def run_format_command( fix: bool = False, json_output: bool = False, no_color: bool = False, + no_cache: bool = False, ) -> int: ... def format_parameterized_test_label( *, diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/format/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/format/_test_types.py index 4cbe2169ba..8213b07daf 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/format/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/format/_test_types.py @@ -107,3 +107,23 @@ class TokenPreservationBuildTestCase: expected_sql: str expected_columns: list[str] expected_rows: list[tuple[object, ...]] + + +@dataclass(frozen=True) +class FormatParallelDeterminismTestCase: + """One format mode whose output must not depend on how many worker threads run it.""" + + description: str + arguments: tuple[str, ...] + expected_returncode: int + + +@dataclass(frozen=True) +class FormatCacheEndToEndTestCase: + """A generated project checked cold, warm, after an edit and after damaging the cache.""" + + description: str + model_count: int + edited_model: str + expected_warm_summary: str + expected_edited_summary: str diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/format/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/format/helpers.py index c1f2e55cd6..5d6165828a 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/format/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/format/helpers.py @@ -9,6 +9,7 @@ import time from pathlib import Path +from scripts.format_performance._helpers.model_project import write_model_format_project from sqlbuild._native import compute_query_hash _FORMAT_ELAPSED_PATTERN: re.Pattern[str] = re.compile(r"Formatting SQL OK \(([0-9.]+)s;") @@ -131,3 +132,49 @@ def run_sqb(*, project_dir: Path, arguments: tuple[str, ...]) -> subprocess.Comp text=True, timeout=60, ) + + +FORMAT_CACHE_UNFORMATTED_ORDERS: str = ( + 'MODEL (description "Orders placed by customers.", materialized table);\n' + "select order_id, customer_id from __source('shop', 'orders')\n" +) + + +def write_mixed_format_project(*, project_dir: Path, model_count: int) -> None: + """Unformatted models plus files that fault, use CRLF line endings or carry a directive.""" + + write_model_format_project(project_dir=project_dir, model_count=model_count) + models_dir: Path = project_dir / "models" + path: Path + for path in sorted(models_dir.glob("*.sql")): + _ = path.write_text( + 'MODEL (description "Generated order model.", materialized view);\n' + + path.read_text(encoding="utf-8"), + encoding="utf-8", + ) + _ = (models_dir / "undescribed_orders.sql").write_text( + "MODEL (materialized view);\nselect 1 as order_id\n", encoding="utf-8" + ) + _ = (models_dir / "broken_header.sql").write_text( + 'MODEL (description "Broken.", materialized ((;\nselect 1 as order_id\n', + encoding="utf-8", + ) + _ = (models_dir / "unsafe_body.sql").write_text( + 'MODEL (description "Unsafe body.");\nselect (((( from where 1\n', encoding="utf-8" + ) + _ = (models_dir / "windows_orders.sql").write_bytes( + b'MODEL (description "Windows line endings.");\r\nselect 1 as order_id\r\n' + ) + _ = (models_dir / "suppressed_orders.sql").write_text( + 'MODEL (description "Suppressed.");\n-- sqb: ignore some-rule\nselect 1 as order_id\n', + encoding="utf-8", + ) + + +def model_tree(*, project_dir: Path) -> dict[str, bytes]: + """The bytes of every model file, by project-relative path.""" + + return { + str(path.relative_to(project_dir)): path.read_bytes() + for path in sorted((project_dir / "models").rglob("*.sql")) + } diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/format/test_format_cache.py b/tests/e2e/src/sqlbuild/cli/commands/main/format/test_format_cache.py new file mode 100644 index 0000000000..e9f6a094b1 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/format/test_format_cache.py @@ -0,0 +1,127 @@ +"""End-to-end coverage for parallel format determinism and the format result cache.""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from scripts.format_performance._helpers.model_project import write_model_format_project +from sqlbuild import _native as native_module +from tests.e2e.src.sqlbuild.cli.commands.main.format._test_types import ( + FormatCacheEndToEndTestCase, + FormatParallelDeterminismTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.format.helpers import ( + FORMAT_CACHE_UNFORMATTED_ORDERS, + model_tree, + write_mixed_format_project, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import run_sqb + +_SERIAL: dict[str, str] = {"RAYON_NUM_THREADS": "1"} +_PARALLEL: dict[str, str] = {"RAYON_NUM_THREADS": "4"} + + +@pytest.mark.parametrize( + "test_case", + [ + FormatParallelDeterminismTestCase( + description="check report", arguments=("--check",), expected_returncode=1 + ), + FormatParallelDeterminismTestCase( + description="json report", arguments=("--check", "--json"), expected_returncode=1 + ), + FormatParallelDeterminismTestCase( + description="diff", arguments=("--diff",), expected_returncode=1 + ), + FormatParallelDeterminismTestCase( + description="written files", arguments=(), expected_returncode=1 + ), + ], + ids=lambda case: case.description, +) +def test_given_mixed_project_when_formatting_serially_and_in_parallel_then_results_match( + test_case: FormatParallelDeterminismTestCase, + tmp_path: Path, +) -> None: + serial_dir: Path = tmp_path / "serial" + write_mixed_format_project(project_dir=serial_dir, model_count=60) + parallel_dir: Path = tmp_path / "parallel" + _ = shutil.copytree(serial_dir, parallel_dir) + command: tuple[str, ...] = ("--no-color", "format", "--no-cache", *test_case.arguments) + + serial: subprocess.CompletedProcess[str] = run_sqb( + project_dir=serial_dir, command=command, env=_SERIAL + ) + parallel: subprocess.CompletedProcess[str] = run_sqb( + project_dir=parallel_dir, command=command, env=_PARALLEL + ) + + assert serial.returncode == test_case.expected_returncode, serial.stderr + assert parallel.returncode == test_case.expected_returncode, parallel.stderr + assert parallel.stdout.replace(str(parallel_dir), "") == serial.stdout.replace( + str(serial_dir), "" + ) + assert model_tree(project_dir=parallel_dir) == model_tree(project_dir=serial_dir) + + +@pytest.mark.parametrize( + "test_case", + [ + FormatCacheEndToEndTestCase( + description="generated orders project", + model_count=40, + edited_model="orders_00007.sql", + expected_warm_summary="40 files checked, 0 changed", + expected_edited_summary="40 files checked, 1 changed", + ) + ], + ids=lambda case: case.description, +) +def test_given_formatted_project_when_checking_warm_edited_and_damaged_then_results_stay_exact( + test_case: FormatCacheEndToEndTestCase, + tmp_path: Path, +) -> None: + project_dir: Path = tmp_path / "project" + write_model_format_project(project_dir=project_dir, model_count=test_case.model_count) + store: Path = ( + native_module.compiler_cache_directory(project_dir) + / native_module.FORMAT_RESULT_STORE_FILE_NAME + ) + check: tuple[str, ...] = ("--no-color", "format", "--check") + + formatted: subprocess.CompletedProcess[str] = run_sqb( + project_dir=project_dir, command=("--no-color", "format") + ) + cold: subprocess.CompletedProcess[str] = run_sqb(project_dir=project_dir, command=check) + stored: bytes = store.read_bytes() + warm: subprocess.CompletedProcess[str] = run_sqb(project_dir=project_dir, command=check) + _ = (project_dir / "models" / test_case.edited_model).write_text( + FORMAT_CACHE_UNFORMATTED_ORDERS, encoding="utf-8" + ) + edited: subprocess.CompletedProcess[str] = run_sqb(project_dir=project_dir, command=check) + _ = store.write_bytes(b"not a native store") + damaged: subprocess.CompletedProcess[str] = run_sqb(project_dir=project_dir, command=check) + uncached: subprocess.CompletedProcess[str] = run_sqb( + project_dir=project_dir, command=(*check, "--no-cache") + ) + + assert formatted.returncode == 0, formatted.stderr + assert cold.returncode == 0, cold.stderr + assert stored + assert warm.returncode == 0, warm.stderr + assert warm.stdout == cold.stdout + assert test_case.expected_warm_summary in warm.stderr + assert edited.returncode == 1, edited.stderr + assert test_case.edited_model in edited.stdout + assert test_case.expected_edited_summary in edited.stderr + assert damaged.returncode == 1, damaged.stderr + assert damaged.stdout == edited.stdout + assert uncached.stdout == edited.stdout + + +if __name__ == "__main__": + pytest.main([__file__, "-vv"]) diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 9d10264b4c..0fd8156436 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -4,6 +4,8 @@ from dataclasses import dataclass, field from pathlib import Path +import pytest + from sqlbuild._native import CursorOverrides @@ -1208,3 +1210,38 @@ class TypedConfigReadsTestCase: description: str unread_keys: str expected_same_reads: bool + + +@dataclass(frozen=True) +class FormatCacheReuseTestCase: + """A second `format --check` of an unchanged project and the work its cache saves.""" + + description: str + arguments: tuple[str, ...] + expected_hits: int + expected_misses: int + expected_formatted_files: int + + +@dataclass(frozen=True) +class FormatCacheInvalidationTestCase: + """A change after a clean `format --check` and the files the next check formats again.""" + + description: str + change: Callable[[Path, pytest.MonkeyPatch], None] + expected_hits: int + expected_misses: int + expected_exit_code: int + expected_recheck_hits: int + expected_output_fragment: str = "" + + +@dataclass(frozen=True) +class FormatCacheFaultTestCase: + """A file whose remaining fault keeps it out of the format result cache.""" + + description: str + removed_text: str + expected_code: str + expected_hits: int + expected_misses: int diff --git a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py index 74bc48c9e9..6ac9989863 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py @@ -32,6 +32,7 @@ from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( RepeatedJsonParseTestCase, ) +from tests.unit.src.sqlbuild.rule_engine._helpers.run.helpers import rebuild_identity STAGE_CAPTURE_SUFFIX: str = ".json" @@ -1008,3 +1009,85 @@ def typed_config_reads(*, project_dir: Path, extra_header: str) -> int: exit_code: int = main(["--project-dir", str(project_dir), "plan", "--json", "--no-cache"]) assert exit_code == 0 return native_module.compile_stage_work_info(reset=True).get("typed_config_reads", 0) + + +FORMAT_CACHE_UNFORMATTED_MODELS: dict[str, str] = { + "orders.sql": ( + 'MODEL (description "Orders placed by customers.", materialized table);\n' + "select order_id, customer_id from __source('shop', 'orders')\n" + ), + "customers.sql": ( + 'MODEL (description "Customers who placed orders.", materialized table);\n' + "select customer_id, min(order_id) as first_order_id from __ref('orders') " + "group by customer_id\n" + ), + "products.sql": ( + 'MODEL (description "Products in the catalog.", materialized view);\n' + "select 1 as product_id, 'widget' as product_name\n" + ), +} + + +def write_formatted_cache_project(*, project_dir: Path) -> None: + """Write three small models and format them once without the format result cache.""" + + (project_dir / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + models_dir: Path = project_dir / "models" + models_dir.mkdir() + name: str + sql: str + for name, sql in FORMAT_CACHE_UNFORMATTED_MODELS.items(): + (models_dir / name).write_text(sql, encoding="utf-8") + assert main(["--project-dir", str(project_dir), "format", "--no-cache"]) == 0 + + +def format_result_store_path(*, project_dir: Path) -> Path: + """The project's format result store file.""" + + return ( + native_module.compiler_cache_directory(project_dir) + / native_module.FORMAT_RESULT_STORE_FILE_NAME + ) + + +def check_format_with_work( + *, project_dir: Path, arguments: tuple[str, ...] = () +) -> tuple[int, dict[str, int]]: + """Run `format --check` in process and return its exit code and native stage work.""" + + _ = native_module.compile_stage_work_info(reset=True) + exit_code: int = main(["--project-dir", str(project_dir), "format", "--check", *arguments]) + return exit_code, native_module.compile_stage_work_info() + + +def edit_cached_orders_model(project_dir: Path, _monkeypatch: pytest.MonkeyPatch) -> None: + """Make one cached model unformatted again.""" + + (project_dir / "models" / "orders.sql").write_text( + FORMAT_CACHE_UNFORMATTED_MODELS["orders.sql"], encoding="utf-8" + ) + + +def widen_format_line_width(project_dir: Path, _monkeypatch: pytest.MonkeyPatch) -> None: + """Change the `[format]` line width, which every cache key includes.""" + + config: Path = project_dir / "sqlbuild_project.toml" + _ = config.write_text( + config.read_text(encoding="utf-8") + "\n[format]\nline_width = 120\n", encoding="utf-8" + ) + + +def damage_format_result_store(project_dir: Path, _monkeypatch: pytest.MonkeyPatch) -> None: + """Overwrite the end of the stored format results so the store fails its checksum.""" + + path: Path = format_result_store_path(project_dir=project_dir) + assert path.is_file() + _ = path.write_bytes(path.read_bytes()[:-7] + b"damaged") + + +def rebuild_formatter(_project_dir: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Pretend another SQLBuild build formats the project.""" + + rebuild_identity(monkeypatch=monkeypatch) diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format_cache.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format_cache.py new file mode 100644 index 0000000000..aedf6b9359 --- /dev/null +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format_cache.py @@ -0,0 +1,179 @@ +"""Integration coverage for the format result cache: clean files are skipped until they change.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from _pytest.capture import CaptureResult + +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + FormatCacheFaultTestCase, + FormatCacheInvalidationTestCase, + FormatCacheReuseTestCase, +) +from tests.integration.src.sqlbuild.cli.commands.main.helpers import ( + check_format_with_work, + damage_format_result_store, + edit_cached_orders_model, + rebuild_formatter, + widen_format_line_width, + write_formatted_cache_project, +) + + +@pytest.mark.parametrize( + "test_case", + [ + FormatCacheReuseTestCase( + description="check skips every file a previous check found clean", + arguments=(), + expected_hits=3, + expected_misses=0, + expected_formatted_files=0, + ), + FormatCacheReuseTestCase( + description="diff reuses the same clean results", + arguments=("--diff",), + expected_hits=3, + expected_misses=0, + expected_formatted_files=0, + ), + FormatCacheReuseTestCase( + description="no-cache formats every file again", + arguments=("--no-cache",), + expected_hits=0, + expected_misses=0, + expected_formatted_files=3, + ), + ], + ids=lambda case: case.description, +) +def test_given_clean_checked_project_when_checking_again_then_cached_files_are_not_formatted( + test_case: FormatCacheReuseTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + write_formatted_cache_project(project_dir=tmp_path) + _ = capsys.readouterr() + cold_exit, cold_work = check_format_with_work(project_dir=tmp_path) + cold: CaptureResult[str] = capsys.readouterr() + + warm_exit, warm_work = check_format_with_work( + project_dir=tmp_path, arguments=test_case.arguments + ) + warm: CaptureResult[str] = capsys.readouterr() + + assert cold_exit == 0 + assert cold_work.get("format_cache_misses", 0) == 3 + assert warm_exit == 0 + assert warm_work.get("format_cache_hits", 0) == test_case.expected_hits + assert warm_work.get("format_cache_misses", 0) == test_case.expected_misses + assert warm_work.get("format_lint_files", 0) == test_case.expected_formatted_files + assert warm.out == cold.out + assert "3 files checked, 0 changed" in warm.err + + +@pytest.mark.parametrize( + "test_case", + [ + FormatCacheInvalidationTestCase( + description="an edited file is formatted again and reported", + change=edit_cached_orders_model, + expected_hits=2, + expected_misses=1, + expected_exit_code=1, + expected_recheck_hits=2, + expected_output_fragment="orders.sql", + ), + FormatCacheInvalidationTestCase( + description="changed format settings format every file again", + change=widen_format_line_width, + expected_hits=0, + expected_misses=3, + expected_exit_code=0, + expected_recheck_hits=3, + ), + FormatCacheInvalidationTestCase( + description="another formatter build formats every file again", + change=rebuild_formatter, + expected_hits=0, + expected_misses=3, + expected_exit_code=0, + expected_recheck_hits=3, + ), + FormatCacheInvalidationTestCase( + description="a damaged store is ignored", + change=damage_format_result_store, + expected_hits=0, + expected_misses=3, + expected_exit_code=0, + expected_recheck_hits=3, + ), + ], + ids=lambda case: case.description, +) +def test_given_cached_clean_project_when_inputs_change_then_affected_files_are_formatted_again( + test_case: FormatCacheInvalidationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +) -> None: + write_formatted_cache_project(project_dir=tmp_path) + assert check_format_with_work(project_dir=tmp_path)[0] == 0 + _ = capsys.readouterr() + test_case.change(tmp_path, monkeypatch) + + exit_code, work = check_format_with_work(project_dir=tmp_path) + captured: CaptureResult[str] = capsys.readouterr() + rechecked_exit, rechecked_work = check_format_with_work(project_dir=tmp_path) + + assert exit_code == test_case.expected_exit_code + assert work.get("format_cache_hits", 0) == test_case.expected_hits + assert work.get("format_cache_misses", 0) == test_case.expected_misses + assert test_case.expected_output_fragment in captured.out + assert rechecked_exit == test_case.expected_exit_code + assert rechecked_work.get("format_cache_hits", 0) == test_case.expected_recheck_hits + + +@pytest.mark.parametrize( + "test_case", + [ + FormatCacheFaultTestCase( + description="a missing description is reported on every check", + removed_text='description "Products in the catalog.", ', + expected_code="description-present", + expected_hits=2, + expected_misses=1, + ) + ], + ids=lambda case: case.description, +) +def test_given_file_with_fault_when_checking_twice_then_fault_is_reported_both_times( + test_case: FormatCacheFaultTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + write_formatted_cache_project(project_dir=tmp_path) + products: Path = tmp_path / "models" / "products.sql" + _ = products.write_text( + products.read_text(encoding="utf-8").replace(test_case.removed_text, ""), + encoding="utf-8", + ) + _ = capsys.readouterr() + + first_exit, first_work = check_format_with_work(project_dir=tmp_path) + first: CaptureResult[str] = capsys.readouterr() + second_exit, second_work = check_format_with_work(project_dir=tmp_path) + second: CaptureResult[str] = capsys.readouterr() + + assert first_exit == second_exit == 1 + assert test_case.expected_code in first.out + assert second.out == first.out + assert first_work.get("format_cache_misses", 0) == 3 + assert second_work.get("format_cache_hits", 0) == test_case.expected_hits + assert second_work.get("format_cache_misses", 0) == test_case.expected_misses + + +if __name__ == "__main__": + pytest.main([__file__, "-vv"]) diff --git a/website/src/content/docs/docs/cli/format.mdx b/website/src/content/docs/docs/cli/format.mdx index 0cef3bee65..8b6d2077f9 100644 --- a/website/src/content/docs/docs/cli/format.mdx +++ b/website/src/content/docs/docs/cli/format.mdx @@ -17,6 +17,7 @@ sqb format [flags] | `--diff` | Print the proposed diff without writing | | `--fix` | Also apply compiler-verified, meaning-preserving Rule fixes before layout formatting | | `--json` | Print structured results | +| `--no-cache` | Format every file again instead of skipping files a previous run found clean | | `--select`, `-s` | Format selected models | | `--select-file` | Read selectors from a file | | `--exclude` | Exclude models from the selected scope | @@ -30,6 +31,20 @@ sqb format --diff --select tag:marts Formatting owns presentation and source rewriting. Compiler-integrated diagnostics belong to configured [Rules](/docs/concepts/rules), which never mutate source. +Files are formatted in parallel; the report, the diff and the exit code are the same as a +one-file-at-a-time run. + +## Skipping unchanged files + +`sqb format` remembers every file it found already formatted and free of faults and warnings, in +`target/cache/compiler-native-v1/format-results.bin`. A later run skips such a file while its text, +its path, the `[format]` and rule threshold settings, the adapter dialect and the SQLBuild build are +all unchanged, so `sqb format --check` on an unchanged project costs little more than reading the +files. A file that needed changes or reported anything is always formatted again, and a damaged or +unreadable cache is ignored. `--fix` never uses the cache, and SQL test files are not skipped +while typed-null fixture formatting reads their models' contracts. Pass `--no-cache` to format +every file. + ## Line width and descriptions Configure the maximum physical line width in `sqlbuild_project.toml` (the default is `100`): From 5c3121f08ff660e94b6ea11c2bae20c7a0168506 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Tue, 20 Oct 2026 22:58:34 +0100 Subject: [PATCH 4/6] fix(format): write formatted files one by one and stop at the first failure Formatting stays parallel, but files are replaced in path order again and a failed write leaves every later file as authored. The --no-cache help text now matches the other format flags. --- .../sqlbuild-cli/src/argv/command_table.json | 2 +- .../bindings/_helpers/format_command/files.rs | 63 ++++++----------- .../sqlbuild/references/docs/cli/format.md | 2 +- .../sqlbuild/cli/commands/main/_test_types.py | 11 +++ .../cli/commands/main/test_format_writes.py | 69 +++++++++++++++++++ website/src/content/docs/docs/cli/format.mdx | 2 +- 6 files changed, 104 insertions(+), 45 deletions(-) create mode 100644 tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py diff --git a/crates/sqlbuild-cli/src/argv/command_table.json b/crates/sqlbuild-cli/src/argv/command_table.json index d4ec6cb684..bd81c7e6ef 100644 --- a/crates/sqlbuild-cli/src/argv/command_table.json +++ b/crates/sqlbuild-cli/src/argv/command_table.json @@ -2394,7 +2394,7 @@ "nargs": 0, "const": true, "default": false, - "help": "Bypass the format result cache for this invocation", + "help": "format every file instead of skipping files a previous run found clean", "group": 1 }, { diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs index 2233453d5d..2381ef8ac9 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs @@ -8,9 +8,6 @@ use std::time::{SystemTime, UNIX_EPOCH}; use pyo3::exceptions::PyOSError; use pyo3::prelude::{Bound, PyAny, PyModule, PyModuleMethods, PyResult, Python}; use pyo3::{IntoPyObject, pyfunction, wrap_pyfunction}; -use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; - -use crate::bindings::_helpers::format_command::stages::on_format_workers; const LINE_FEED: &str = "\n"; const CARRIAGE_RETURN_LINE_FEED: &str = "\r\n"; @@ -66,17 +63,12 @@ fn write_replacing(path: &Path, temporary: &Path, contents: &str) -> std::io::Re fs::rename(temporary, path) } -/// Replace a text file atomically, keeping its permissions; the error names the path. -fn replace_file(path: &Path, contents: &str) -> Result<(), String> { +/// Replace a text file atomically, keeping its permissions. +fn write_atomically(path: &Path, contents: &str) -> PyResult<()> { let temporary: PathBuf = temporary_path(path); let written: std::io::Result<()> = write_replacing(path, &temporary, contents); let _ = fs::remove_file(&temporary); - written.map_err(|error| format!("{}: {error}", path.display())) -} - -/// Replace a text file atomically, keeping its permissions. -fn write_atomically(path: &Path, contents: &str) -> PyResult<()> { - replace_file(path, contents).map_err(PyOSError::new_err) + written.map_err(|error| PyOSError::new_err(format!("{}: {error}", path.display()))) } /// `write_atomically(*, path, contents)`: replace a text file atomically, keeping its mode. @@ -90,41 +82,28 @@ pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_function(wrap_pyfunction!(write_file_atomically, module)?) } -/// The changed files of `writes` in order, written in parallel when `write` is set. +/// The changed files of `writes` in order, written one by one when `write` is set. pub(crate) fn persist_changes<'py>( py: Python<'py>, writes: &[PendingWrite<'_>], write: bool, ) -> PyResult>> { - let changed: Vec<(&PendingWrite<'_>, String, String)> = writes - .iter() - .filter_map(|pending| { - let after: String = with_newline_style(pending.contents, pending.newline); - let before: String = with_newline_style(pending.authored, pending.newline); - (after != before).then_some((pending, before, after)) - }) - .collect(); - if write { - py.detach(|| { - on_format_workers(|| { - let written: Vec> = changed - .par_iter() - .map(|(pending, _, after)| replace_file(pending.path, after)) - .collect(); - written.into_iter().collect::>() - }) - }) - .map_err(PyOSError::new_err)?; + let mut changes: Vec> = Vec::new(); + for pending in writes { + let after: String = with_newline_style(pending.contents, pending.newline); + let before: String = with_newline_style(pending.authored, pending.newline); + if after == before { + continue; + } + if write { + write_atomically(pending.path, &after)?; + } + changes.push(FormatChange { + path: pending.path.into_pyobject(py)?.into_any(), + entry: pending.entry, + before, + after, + }); } - changed - .into_iter() - .map(|(pending, before, after)| { - Ok(FormatChange { - path: pending.path.into_pyobject(py)?.into_any(), - entry: pending.entry, - before, - after, - }) - }) - .collect() + Ok(changes) } diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md index 7696692cbe..8d0fca7d69 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/format.md @@ -26,7 +26,7 @@ sqb format [flags] | `--diff` | Print the proposed diff without writing | | `--fix` | Also apply compiler-verified, meaning-preserving Rule fixes before layout formatting | | `--json` | Print structured results | -| `--no-cache` | Format every file again instead of skipping files a previous run found clean | +| `--no-cache` | Format every file instead of skipping files a previous run found clean | | `--select`, `-s` | Format selected models | | `--select-file` | Read selectors from a file | | `--exclude` | Exclude models from the selected scope | diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 0fd8156436..2d04cde928 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -1245,3 +1245,14 @@ class FormatCacheFaultTestCase: expected_code: str expected_hits: int expected_misses: int + + +@dataclass(frozen=True) +class FormatWriteFailureTestCase: + """A write that fails part-way through a format run and the files left as authored.""" + + description: str + locked_directory: str + later_directory: str + expected_error: type[BaseException] + expected_status: str diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py new file mode 100644 index 0000000000..d4979ec2c9 --- /dev/null +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py @@ -0,0 +1,69 @@ +"""Integration coverage for writing formatted files back in path order.""" + +from __future__ import annotations + +import os +import re +import stat +import sys +from pathlib import Path + +import pytest + +from sqlbuild._native import run_cli as main +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + FormatWriteFailureTestCase, +) + +_UNFORMATTED_SQL: str = ( + 'MODEL (description "Orders placed by customers.", materialized table);\n' + "select order_id, customer_id from __source('shop', 'orders')\n" +) + + +@pytest.mark.skipif( + sys.platform == "win32" or os.geteuid() == 0, + reason="needs a directory whose permissions the current user cannot override", +) +@pytest.mark.parametrize( + "test_case", + [ + FormatWriteFailureTestCase( + description="a failed write stops before later files are replaced", + locked_directory="a_locked", + later_directory="b_open", + expected_error=OSError, + expected_status="Formatting SQL ERROR", + ) + ], + ids=lambda case: case.description, +) +def test_given_unwritable_file_when_formatting_then_later_files_are_left_as_authored( + test_case: FormatWriteFailureTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + locked: Path = tmp_path / "models" / test_case.locked_directory + later: Path = tmp_path / "models" / test_case.later_directory + locked.mkdir(parents=True) + later.mkdir() + (locked / "orders.sql").write_text(_UNFORMATTED_SQL, encoding="utf-8") + later_file: Path = later / "orders_copy.sql" + later_file.write_text(_UNFORMATTED_SQL, encoding="utf-8") + locked.chmod(stat.S_IRUSR | stat.S_IXUSR) + try: + with pytest.raises(test_case.expected_error, match=re.escape(str(locked / "orders.sql"))): + _ = main(["--project-dir", str(tmp_path), "format", "--no-cache"]) + finally: + locked.chmod(stat.S_IRWXU) + + assert test_case.expected_status in capsys.readouterr().err + assert later_file.read_text(encoding="utf-8") == _UNFORMATTED_SQL + assert (locked / "orders.sql").read_text(encoding="utf-8") == _UNFORMATTED_SQL + + +if __name__ == "__main__": + pytest.main([__file__, "-vv"]) diff --git a/website/src/content/docs/docs/cli/format.mdx b/website/src/content/docs/docs/cli/format.mdx index 8b6d2077f9..acfacb35da 100644 --- a/website/src/content/docs/docs/cli/format.mdx +++ b/website/src/content/docs/docs/cli/format.mdx @@ -17,7 +17,7 @@ sqb format [flags] | `--diff` | Print the proposed diff without writing | | `--fix` | Also apply compiler-verified, meaning-preserving Rule fixes before layout formatting | | `--json` | Print structured results | -| `--no-cache` | Format every file again instead of skipping files a previous run found clean | +| `--no-cache` | Format every file instead of skipping files a previous run found clean | | `--select`, `-s` | Format selected models | | `--select-file` | Read selectors from a file | | `--exclude` | Exclude models from the selected scope | From 53e64583e6edbccc1e68f3a5b3ad428525ae5f42 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:26:02 +0100 Subject: [PATCH 5/6] perf(format): stage formatted files in parallel and replace them in path order Replacement files are written and synced beside their targets in parallel, then renamed in path order. The earliest file whose staging or rename fails stops the run: it and every later file keep their text, and every unused replacement file is removed. --- .../bindings/_helpers/format_command/files.rs | 88 ++++++++++++++----- .../sqlbuild/cli/commands/main/_test_types.py | 5 +- .../cli/commands/main/test_format_writes.py | 35 +++++--- 3 files changed, 92 insertions(+), 36 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs index 2381ef8ac9..19e5d730ae 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/format_command/files.rs @@ -8,6 +8,9 @@ use std::time::{SystemTime, UNIX_EPOCH}; use pyo3::exceptions::PyOSError; use pyo3::prelude::{Bound, PyAny, PyModule, PyModuleMethods, PyResult, Python}; use pyo3::{IntoPyObject, pyfunction, wrap_pyfunction}; +use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator}; + +use crate::bindings::_helpers::format_command::stages::on_format_workers; const LINE_FEED: &str = "\n"; const CARRIAGE_RETURN_LINE_FEED: &str = "\r\n"; @@ -51,7 +54,8 @@ fn temporary_path(path: &Path) -> PathBuf { path.with_file_name(format!(".{name}.{}{nanos:x}", std::process::id())) } -fn write_replacing(path: &Path, temporary: &Path, contents: &str) -> std::io::Result<()> { +/// Write and sync `contents` to a new `temporary` beside `path`, with its permissions. +fn stage_replacement(path: &Path, temporary: &Path, contents: &str) -> std::io::Result<()> { let permissions: fs::Permissions = fs::metadata(path)?.permissions(); let mut handle: File = OpenOptions::new() .write(true) @@ -59,16 +63,45 @@ fn write_replacing(path: &Path, temporary: &Path, contents: &str) -> std::io::Re .open(temporary)?; handle.set_permissions(permissions)?; handle.write_all(contents.as_bytes())?; - handle.sync_all()?; - fs::rename(temporary, path) + handle.sync_all() +} + +fn write_failure(path: &Path, error: &std::io::Error) -> String { + format!("{}: {error}", path.display()) } /// Replace a text file atomically, keeping its permissions. fn write_atomically(path: &Path, contents: &str) -> PyResult<()> { let temporary: PathBuf = temporary_path(path); - let written: std::io::Result<()> = write_replacing(path, &temporary, contents); + let written: std::io::Result<()> = + stage_replacement(path, &temporary, contents).and_then(|()| fs::rename(&temporary, path)); let _ = fs::remove_file(&temporary); - written.map_err(|error| PyOSError::new_err(format!("{}: {error}", path.display()))) + written.map_err(|error| PyOSError::new_err(write_failure(path, &error))) +} + +/// Replace `files` in order after staging all in parallel; the earliest failure stops the rest. +fn replace_in_order(files: &[(&Path, &str)]) -> Result<(), String> { + let temporaries: Vec = files.iter().map(|(path, _)| temporary_path(path)).collect(); + let staged: Vec> = on_format_workers(|| { + Ok(files + .par_iter() + .zip(temporaries.par_iter()) + .map(|((path, contents), temporary)| stage_replacement(path, temporary, contents)) + .collect()) + })?; + let mut outcome: Result<(), String> = Ok(()); + let mut renamed: usize = 0; + for (((path, _), temporary), staging) in files.iter().zip(&temporaries).zip(staged) { + if let Err(error) = staging.and_then(|()| fs::rename(temporary, path)) { + outcome = Err(write_failure(path, &error)); + break; + } + renamed += 1; + } + for temporary in &temporaries[renamed..] { + let _ = fs::remove_file(temporary); + } + outcome } /// `write_atomically(*, path, contents)`: replace a text file atomically, keeping its mode. @@ -82,28 +115,37 @@ pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_function(wrap_pyfunction!(write_file_atomically, module)?) } -/// The changed files of `writes` in order, written one by one when `write` is set. +/// The changed files of `writes` in order, replaced in that order when `write` is set. pub(crate) fn persist_changes<'py>( py: Python<'py>, writes: &[PendingWrite<'_>], write: bool, ) -> PyResult>> { - let mut changes: Vec> = Vec::new(); - for pending in writes { - let after: String = with_newline_style(pending.contents, pending.newline); - let before: String = with_newline_style(pending.authored, pending.newline); - if after == before { - continue; - } - if write { - write_atomically(pending.path, &after)?; - } - changes.push(FormatChange { - path: pending.path.into_pyobject(py)?.into_any(), - entry: pending.entry, - before, - after, - }); + let changed: Vec<(&PendingWrite<'_>, String, String)> = writes + .iter() + .filter_map(|pending| { + let after: String = with_newline_style(pending.contents, pending.newline); + let before: String = with_newline_style(pending.authored, pending.newline); + (after != before).then_some((pending, before, after)) + }) + .collect(); + if write { + let files: Vec<(&Path, &str)> = changed + .iter() + .map(|(pending, _, after)| (pending.path, after.as_str())) + .collect(); + py.detach(|| replace_in_order(&files)) + .map_err(PyOSError::new_err)?; } - Ok(changes) + changed + .into_iter() + .map(|(pending, before, after)| { + Ok(FormatChange { + path: pending.path.into_pyobject(py)?.into_any(), + entry: pending.entry, + before, + after, + }) + }) + .collect() } diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 2d04cde928..2499250938 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -1249,10 +1249,11 @@ class FormatCacheFaultTestCase: @dataclass(frozen=True) class FormatWriteFailureTestCase: - """A write that fails part-way through a format run and the files left as authored.""" + """A write that fails part-way through a format run and which files it replaced.""" description: str + directories: tuple[str, ...] locked_directory: str - later_directory: str + expected_replaced: tuple[bool, ...] expected_error: type[BaseException] expected_status: str diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py index d4979ec2c9..8e9562a0be 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format_writes.py @@ -29,12 +29,21 @@ "test_case", [ FormatWriteFailureTestCase( - description="a failed write stops before later files are replaced", + description="a failed first write leaves every later file as authored", + directories=("a_locked", "b_open"), locked_directory="a_locked", - later_directory="b_open", + expected_replaced=(False, False), expected_error=OSError, expected_status="Formatting SQL ERROR", - ) + ), + FormatWriteFailureTestCase( + description="files before a failed write are replaced and later ones are not", + directories=("a_open", "b_locked", "c_open", "d_open"), + locked_directory="b_locked", + expected_replaced=(True, False, False, False), + expected_error=OSError, + expected_status="Formatting SQL ERROR", + ), ], ids=lambda case: case.description, ) @@ -46,13 +55,14 @@ def test_given_unwritable_file_when_formatting_then_later_files_are_left_as_auth (tmp_path / "sqlbuild_project.toml").write_text( 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" ) + files: list[Path] = [] + directory: str + for directory in test_case.directories: + (tmp_path / "models" / directory).mkdir(parents=True) + model: Path = tmp_path / "models" / directory / "orders.sql" + model.write_text(_UNFORMATTED_SQL, encoding="utf-8") + files.append(model) locked: Path = tmp_path / "models" / test_case.locked_directory - later: Path = tmp_path / "models" / test_case.later_directory - locked.mkdir(parents=True) - later.mkdir() - (locked / "orders.sql").write_text(_UNFORMATTED_SQL, encoding="utf-8") - later_file: Path = later / "orders_copy.sql" - later_file.write_text(_UNFORMATTED_SQL, encoding="utf-8") locked.chmod(stat.S_IRUSR | stat.S_IXUSR) try: with pytest.raises(test_case.expected_error, match=re.escape(str(locked / "orders.sql"))): @@ -61,8 +71,11 @@ def test_given_unwritable_file_when_formatting_then_later_files_are_left_as_auth locked.chmod(stat.S_IRWXU) assert test_case.expected_status in capsys.readouterr().err - assert later_file.read_text(encoding="utf-8") == _UNFORMATTED_SQL - assert (locked / "orders.sql").read_text(encoding="utf-8") == _UNFORMATTED_SQL + assert ( + tuple(path.read_text(encoding="utf-8") != _UNFORMATTED_SQL for path in files) + == test_case.expected_replaced + ) + assert [path.name for path in (tmp_path / "models").rglob(".*")] == [] if __name__ == "__main__": From fac2bb3ff61f78a18a485c2b7e30b7e73d7a5b9a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:58:12 +0100 Subject: [PATCH 6/6] test: count only the spinner threads a status line starts itself --- tests/unit/src/sqlbuild/diagnostics/helpers.py | 13 +++++++++++++ tests/unit/src/sqlbuild/diagnostics/test_main.py | 10 +++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/unit/src/sqlbuild/diagnostics/helpers.py b/tests/unit/src/sqlbuild/diagnostics/helpers.py index fe27b8b3d4..78a1e4a747 100644 --- a/tests/unit/src/sqlbuild/diagnostics/helpers.py +++ b/tests/unit/src/sqlbuild/diagnostics/helpers.py @@ -35,3 +35,16 @@ def spinner_threads() -> list[threading.Thread]: """The live progress spinner threads.""" return list(filter(lambda thread: thread.name == SPINNER_THREAD_NAME, threading.enumerate())) + + +def spinner_threads_started_after(earlier: list[threading.Thread]) -> list[threading.Thread]: + """The live spinner threads not in `earlier`, ignoring other tests' lingering threads.""" + + return list(filter(lambda thread: thread not in earlier, spinner_threads())) + + +def stopped_after_join(threads: list[threading.Thread], timeout_seconds: float) -> bool: + """Whether every thread in `threads` has finished within `timeout_seconds` each.""" + + list(map(lambda thread: thread.join(timeout=timeout_seconds), threads)) + return not any(map(lambda thread: thread.is_alive(), threads)) diff --git a/tests/unit/src/sqlbuild/diagnostics/test_main.py b/tests/unit/src/sqlbuild/diagnostics/test_main.py index 31f796adec..223953e34a 100644 --- a/tests/unit/src/sqlbuild/diagnostics/test_main.py +++ b/tests/unit/src/sqlbuild/diagnostics/test_main.py @@ -25,6 +25,8 @@ HostRecordingLogHandler, log_vendor_records, spinner_threads, + spinner_threads_started_after, + stopped_after_join, ) from tests.unit.src.sqlbuild.presentation.classes.helpers import RecordingStream @@ -161,18 +163,20 @@ def test_given_many_records_during_status_line_when_logging_then_one_spinner_thr stream: RecordingStream = RecordingStream(events=[], tty=True) monkeypatch.setattr(sys, "stderr", stream) reporter: TransientStatusReporter = TransientStatusReporter(stream=stream, use_color=False) + earlier: list[threading.Thread] = spinner_threads() seen: list[threading.Thread] = [] reporter.start("Preparing clone plan...") try: with configure_diagnostics(debug=True): for index in range(test_case.record_count): logging.getLogger("vendor.client").warning("retrying request %d", index) - seen.extend(spinner_threads()) + seen.extend(spinner_threads_started_after(earlier)) finally: reporter.close() - assert len({id(thread) for thread in seen}) == test_case.expected_spinner_threads - assert spinner_threads() == [] + started: list[threading.Thread] = list({id(thread): thread for thread in seen}.values()) + assert len(started) == test_case.expected_spinner_threads + assert stopped_after_join(started, timeout_seconds=5.0) assert stream.getvalue().count("retrying request") == test_case.record_count