From 2b1117aac4be9f5a699a09812d149e487313c6b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:11:28 +0000 Subject: [PATCH 1/4] test: red suites for `ddl` as a multi-statement SQL batch Integration and CLI e2e coverage for the behavior #956 asks for: a `[[table]]`'s `ddl` runs as a whole SQL batch, and what it produced is settled against SQLite's catalog rather than by reading the text. All eleven fail today, and two of them expose the reason this matters: `Connection::execute` prepares only the first statement of the string and silently discards the tail, so `CREATE INDEX` never runs and a batch whose second statement SQLite cannot even parse still exits 0. skip-changelog: red tests only; the behavior lands in the next commit --- packages/rust/tests/ddl_batch.rs | 248 +++++++++++++++++++++++++++ packages/rust/tests/ddl_batch_e2e.rs | 162 +++++++++++++++++ 2 files changed, 410 insertions(+) create mode 100644 packages/rust/tests/ddl_batch.rs create mode 100644 packages/rust/tests/ddl_batch_e2e.rs diff --git a/packages/rust/tests/ddl_batch.rs b/packages/rust/tests/ddl_batch.rs new file mode 100644 index 000000000..fa26da06c --- /dev/null +++ b/packages/rust/tests/ddl_batch.rs @@ -0,0 +1,248 @@ +//! Integration tests for `ddl` as a multi-statement SQL batch. +//! +//! A `[[table]]`'s `ddl` is handed to SQLite whole: any number of statements, +//! of any kind. dirsql never reads the text — what the batch produced is +//! settled afterwards against SQLite's own catalog. That is what makes +//! indexes, FTS5 virtual tables and triggers expressible in config. + +use dirsql::{DirSQL, Row, Table, Value}; +use std::collections::HashMap; +use std::fs; +use std::path::Path; +use tempfile::TempDir; + +const BASE: &str = "CREATE TABLE notes (path TEXT, body TEXT)"; + +/// A `notes` table plus an external-content FTS5 index kept current by an +/// insert and a delete trigger — the shape the config unlocks. +const FTS_DDL: &str = "\ +CREATE TABLE notes (path TEXT, body TEXT); +CREATE VIRTUAL TABLE notes_fts USING fts5(body, content='notes', content_rowid='rowid'); +CREATE TRIGGER notes_ai AFTER INSERT ON notes BEGIN + INSERT INTO notes_fts(rowid, body) VALUES (new.rowid, new.body); +END; +CREATE TRIGGER notes_ad AFTER DELETE ON notes BEGIN + INSERT INTO notes_fts(notes_fts, rowid, body) VALUES ('delete', old.rowid, old.body); +END;"; + +/// A `notes` table whose rows are each markdown file's name and contents. +fn notes_table(ddl: &str) -> Table { + Table::new("notes", ddl, "*.md", |path| { + let body = fs::read_to_string(path).unwrap(); + vec![HashMap::from([ + ( + "path".into(), + Value::Text( + Path::new(path) + .file_name() + .unwrap() + .to_string_lossy() + .into_owned(), + ), + ), + ("body".into(), Value::Text(body.trim().to_string())), + ])] as Vec + }) +} + +fn seed(root: &Path) { + fs::write(root.join("a.md"), "hello world").unwrap(); +} + +fn build(root: &Path, ddl: &str) -> dirsql::Result { + DirSQL::builder().root(root).table(notes_table(ddl)).build() +} + +fn build_persist(root: &Path, ddl: &str) -> dirsql::Result { + DirSQL::builder() + .root(root) + .table(notes_table(ddl)) + .persist(None::<&Path>) + .build() +} + +/// The error message from a build that must fail. `DirSQL` is not `Debug`, +/// so `expect_err` is unavailable. +fn build_error(result: dirsql::Result, what: &str) -> String { + match result { + Ok(_) => panic!("{what}"), + Err(err) => err.to_string(), + } +} + +fn index_names(db: &DirSQL) -> Vec { + db.query("SELECT name FROM pragma_index_list('notes')") + .unwrap() + .into_iter() + .map(|row| row["name"].clone()) + .collect() +} + +#[test] +fn a_multi_statement_batch_runs_every_statement() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let db = build( + root.path(), + &format!("{BASE};\nCREATE INDEX notes_path ON notes(path);"), + ) + .expect("a multi-statement `ddl` must load"); + + let rows = db.query("SELECT body FROM notes").unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0]["body"], Value::Text("hello world".into())); + + let indexes = index_names(&db); + assert!( + indexes.contains(&Value::Text("notes_path".into())), + "the batch's CREATE INDEX must have run, got {indexes:?}" + ); +} + +/// The unlock the issue is really about: dirsql writes user rows with plain +/// `INSERT`/`DELETE`, so triggers declared in `ddl` keep an FTS5 index current +/// through the initial load *and* through a later file deletion. +#[test] +fn an_fts5_index_declared_in_ddl_tracks_inserts_and_deletes() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let db = build_persist(root.path(), FTS_DDL).expect("an FTS5 batch must load"); + let hits = db + .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") + .unwrap(); + assert_eq!(hits.len(), 1, "the insert trigger must have indexed the row"); + assert_eq!(hits[0]["body"], Value::Text("hello world".into())); + drop(db); + + fs::remove_file(root.path().join("a.md")).unwrap(); + let db = build_persist(root.path(), FTS_DDL).expect("a warm rebuild must load"); + let hits = db + .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") + .unwrap(); + assert!( + hits.is_empty(), + "the delete trigger must have dropped the vanished file's row from the index, got {hits:?}" + ); +} + +#[test] +fn a_batch_sqlite_rejects_fails_the_build_and_rolls_back() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let message = build_error( + build_persist(root.path(), &format!("{BASE};\nCREATE TABLE oops (")), + "a batch SQLite rejects must fail the build", + ); + assert!( + message.contains("table 'notes'"), + "the error must carry the config-entry prefix `table 'notes'`, got {message:?}" + ); + assert!( + message.contains("incomplete input"), + "SQLite's own error text must come through raw, got {message:?}" + ); + + let cache = rusqlite::Connection::open(root.path().join(".dirsql").join("cache.db")).unwrap(); + let created: i64 = cache + .query_row( + "SELECT COUNT(*) FROM pragma_table_list WHERE schema = 'main' AND name = 'notes'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!( + created, 0, + "the failed batch's earlier statements must roll back, leaving no `notes` table" + ); +} + +/// A batch may create virtual tables freely — but the *declared* table is the +/// per-file row table dirsql inserts into, so it may not itself be one. +#[test] +fn a_declared_name_that_is_a_virtual_table_is_a_load_error() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let message = build_error( + build(root.path(), "CREATE VIRTUAL TABLE notes USING fts5(path, body)"), + "a dirsql table must be a real per-file row table", + ); + assert!( + message.contains("table 'notes'") && message.contains("virtual"), + "the error must name the entry and say the table is virtual, got {message:?}" + ); +} + +#[test] +fn the_not_created_error_names_what_the_batch_did_create() { + let root = TempDir::new().unwrap(); + seed(root.path()); + + let table = Table::new( + "messages", + "CREATE TABLE notes (path TEXT);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(path);", + "*.md", + |_| Vec::::new(), + ); + let message = build_error( + DirSQL::builder().root(root.path()).table(table).build(), + "a `name` the batch never creates must fail to load", + ); + assert!( + message.contains("table 'messages'"), + "the error must carry the config-entry prefix, got {message:?}" + ); + assert!( + message.contains("notes") && message.contains("notes_fts"), + "the error must list what the batch did create, got {message:?}" + ); +} + +/// One invalidation lane: the persist hash covers the whole batch, so adding a +/// statement forces a full sweep and rebuild rather than a warm skip. +#[test] +fn editing_the_ddl_batch_rebuilds_the_cache() { + let root = TempDir::new().unwrap(); + seed(root.path()); + drop(build_persist(root.path(), BASE).expect("first build")); + + let db = build_persist( + root.path(), + &format!("{BASE};\nCREATE INDEX notes_path ON notes(path);"), + ) + .expect("an edited batch must rebuild"); + + let indexes = index_names(&db); + assert!( + indexes.contains(&Value::Text("notes_path".into())), + "a ddl edit must invalidate the cache and re-run the whole batch, got {indexes:?}" + ); +} + +/// The sweep that precedes a rebuild has to cope with whatever the previous +/// batch created — virtual tables and the shadow tables behind them included. +#[test] +fn a_rebuild_sweeps_a_cache_holding_virtual_tables() { + let root = TempDir::new().unwrap(); + seed(root.path()); + drop(build_persist(root.path(), FTS_DDL).expect("first build")); + + let db = build_persist( + root.path(), + &format!("{FTS_DDL}\nCREATE INDEX notes_path ON notes(path);"), + ) + .expect("sweeping a cache holding a virtual table and its shadows must succeed"); + + let hits = db + .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") + .unwrap(); + assert_eq!( + hits.len(), + 1, + "the rebuilt index must hold the re-ingested row, got {hits:?}" + ); +} diff --git a/packages/rust/tests/ddl_batch_e2e.rs b/packages/rust/tests/ddl_batch_e2e.rs new file mode 100644 index 000000000..0e09797e6 --- /dev/null +++ b/packages/rust/tests/ddl_batch_e2e.rs @@ -0,0 +1,162 @@ +//! End-to-end tests for `ddl` as a multi-statement SQL batch. +//! +//! Spawns the real compiled `dirsql` binary against a real temp directory and +//! a real `.dirsql.toml`. Nothing is mocked (real process, real filesystem, +//! real SQLite, real `on-file` command spawn). +//! +//! Gated behind `--features cli` (the `dirsql` bin needs it) and Unix (the +//! fixture shells out to `cat`); the Rust CI test job runs on Linux. + +#![cfg(all(feature = "cli", unix))] + +use std::fs; +use std::process::Output; + +use assert_cmd::prelude::*; +use serde_json::Value; +use tempfile::TempDir; + +/// A tempdir holding one JSON file of rows and a `.dirsql.toml` with `config`. +fn fixture(config: &str) -> TempDir { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("data")).unwrap(); + fs::write( + root.path().join("data/a.json"), + r#"[{"id": "one", "body": "hello world"}, {"id": "two", "body": "goodbye moon"}]"#, + ) + .unwrap(); + fs::write(root.path().join(".dirsql.toml"), config).unwrap(); + root +} + +fn query(root: &TempDir, sql: &str) -> Output { + std::process::Command::cargo_bin("dirsql") + .expect("`dirsql` binary must be built with --features cli") + .arg("query") + .arg(sql) + .arg("--config") + .arg(root.path().join(".dirsql.toml")) + .current_dir(root.path()) + .output() + .expect("spawning `dirsql query` failed") +} + +fn rows(out: &Output) -> Vec { + assert!( + out.status.success(), + "query must succeed, stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + serde_json::from_slice(&out.stdout).expect("stdout must be JSON") +} + +/// A batch declaring the row table, a B-tree index over it, an FTS5 index and +/// the trigger that fills it — every statement in one `ddl` key. +const BATCH_CONFIG: &str = r#" +[[table]] +name = "records" +glob = "data/*.json" +on-file = "cat {path}" +ddl = ''' +CREATE TABLE records (id TEXT, body TEXT); +CREATE INDEX records_id ON records(id); +CREATE VIRTUAL TABLE records_fts USING fts5(body, content='records', content_rowid='rowid'); +CREATE TRIGGER records_ai AFTER INSERT ON records BEGIN + INSERT INTO records_fts(rowid, body) VALUES (new.rowid, new.body); +END; +''' +"#; + +#[test] +fn a_multi_statement_batch_indexes_and_queries_through_the_cli() { + let root = fixture(BATCH_CONFIG); + + let out = query(&root, "SELECT id FROM records ORDER BY id"); + let parsed = rows(&out); + let ids: Vec<&str> = parsed + .iter() + .map(|r| r["id"].as_str().unwrap()) + .collect(); + assert_eq!(ids, ["one", "two"]); + + let out = query(&root, "SELECT name FROM pragma_index_list('records')"); + let parsed = rows(&out); + let names: Vec = parsed + .iter() + .map(|r| r["name"].as_str().unwrap().to_string()) + .collect(); + assert!( + names.iter().any(|n| n == "records_id"), + "the batch's CREATE INDEX must have run, got {names:?}" + ); +} + +#[test] +fn keyword_search_over_an_fts5_index_declared_in_ddl() { + let root = fixture(BATCH_CONFIG); + + let out = query( + &root, + "SELECT body FROM records_fts WHERE records_fts MATCH 'hello'", + ); + let parsed = rows(&out); + let bodies: Vec<&str> = parsed + .iter() + .map(|r| r["body"].as_str().unwrap()) + .collect(); + assert_eq!( + bodies, ["hello world"], + "the insert trigger must have filled the FTS5 index" + ); +} + +#[test] +fn a_batch_sqlite_rejects_exits_nonzero_naming_the_table() { + let root = fixture( + r#" +[[table]] +name = "records" +glob = "data/*.json" +on-file = "cat {path}" +ddl = ''' +CREATE TABLE records (id TEXT, body TEXT); +CREATE TABLE oops ( +''' +"#, + ); + + let out = query(&root, "SELECT id FROM records"); + assert!( + !out.status.success(), + "a batch SQLite rejects must exit non-zero" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("table 'records'") && stderr.contains("incomplete input"), + "stderr must prefix SQLite's raw error with the config entry, got {stderr:?}" + ); +} + +#[test] +fn a_declared_name_that_is_a_virtual_table_exits_nonzero() { + let root = fixture( + r#" +[[table]] +name = "records" +glob = "data/*.json" +on-file = "cat {path}" +ddl = "CREATE VIRTUAL TABLE records USING fts5(id, body)" +"#, + ); + + let out = query(&root, "SELECT id FROM records"); + assert!( + !out.status.success(), + "a declared table that is virtual must exit non-zero" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("table 'records'") && stderr.contains("virtual"), + "stderr must name the entry and say the table is virtual, got {stderr:?}" + ); +} From 9191ced6e960be0f079fd21957473441f5cd2455 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:32:27 +0000 Subject: [PATCH 2/4] feat: run `ddl` as a whole SQL batch, validated against the catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ddl` was one statement, string-sniffed for `CREATE VIRTUAL TABLE` and `WITHOUT ROWID` — and `Connection::execute` prepared only the first statement of whatever was handed to it, so a second one was silently discarded. Indexes, FTS5 tables and triggers were inexpressible in config, and a batch whose tail SQLite could not parse still loaded. It is now a wide-open batch: `execute_batch` inside one transaction, so a rejected statement leaves nothing behind and surfaces SQLite's own message under the entry that caused it. What the batch produced is settled against `pragma_table_list`, never by reading DDL text — no row for `name` fails the load naming what the batch did create, a virtual `name` fails, and `wr=1` keeps the existing WITHOUT ROWID warning. Both string sniffs and the now unconstructible `DbError::DdlParse` are deleted. The cache sweep takes its drop order from the catalog's type, virtuals first, so a `vec0` table's shadow tables can no longer be dropped out from under it. The persist hash already covered the whole `ddl` string, so editing any part of a batch remains the one invalidation lane. Closes #956 --- PARITY.md | 13 + docs/howto/define-tables.md | 44 +++ docs/howto/load-extension.md | 8 +- docs/howto/search-by-meaning.md | 10 +- docs/reference/config.md | 92 +++++- .../rust/changelog.d/2026-08-20-ddl-batch.md | 9 + .../rust/migrations.d/2026-08-20-ddl-batch.md | 88 ++++++ packages/rust/src/db.rs | 270 ++++++++++++------ packages/rust/src/lib.rs | 206 +++++++++++-- packages/rust/src/persist.rs | 54 +++- packages/rust/tests/ddl_batch.rs | 11 +- packages/rust/tests/ddl_batch_e2e.rs | 13 +- packages/rust/tests/sdk.rs | 18 +- 13 files changed, 684 insertions(+), 152 deletions(-) create mode 100644 packages/rust/changelog.d/2026-08-20-ddl-batch.md create mode 100644 packages/rust/migrations.d/2026-08-20-ddl-batch.md diff --git a/PARITY.md b/PARITY.md index 45189b85b..ee33ba3a1 100644 --- a/PARITY.md +++ b/PARITY.md @@ -140,6 +140,15 @@ This lives entirely in the shared Rust core (`create_table` runs DDL verbatim; `query` returns vanilla rows), so `PRAGMA table_info` and `SELECT *` report the same user-only columns across all three SDKs. +**Multi-statement `ddl` batch (#956) — parity by construction, no drift.** A +`[[table]]`'s `ddl` runs as a whole SQL batch (`execute_batch`, one +transaction), so indexes, FTS5/vec0 virtual tables and triggers are declarable +in config; what the batch produced is settled against SQLite's catalog +(`pragma_table_list`), not by reading DDL text. The `ddl` value is an opaque +string every SDK already forwards unchanged, so no binding signature moves and +all three gain the batch, the catalog validation and the new load errors +together. + **Internal tables unreachable through `query()` (#378, epic #358) — parity by construction, no drift.** The internal bookkeeping tables (`_dirsql_internal_rows`, `_dirsql_files`, `_dirsql_meta`) are denied on the `query()` path by a SQLite @@ -411,6 +420,9 @@ incl. #313). | `bytes`/`Vec`/`Buffer` → BLOB (list/array of ints does NOT) | Y (#465) | Y | Y (#343: `Buffer`/`Uint8Array` in, `Buffer` out) | | Invalid SQL raises | Y | Y | Y | | Invalid DDL raises | Y | Y | Y | +| Multi-statement `ddl` batch (index / FTS5 vtab / triggers) runs whole (#956) | core | Y (`ddl_batch.rs`, `ddl_batch_e2e.rs`) | core | +| `ddl` batch SQLite rejects: rolled back, error prefixed with the entry (#956) | core | Y (`ddl_batch.rs`, `sdk.rs`) | core | +| Declared `name` resolving to a virtual table errors (#956) | core | Y (`ddl_batch.rs`) | core | | Query rejects writes | Y | Y | Y | | Write-rejection edge matrix (leading comments, mixed case, CTE/whitespace allowed) | core | Y (`readonly_query.rs`) | core | | Internal `_dirsql_*` columns hidden from `SELECT *` | Y | Y | Y | @@ -473,6 +485,7 @@ incl. #313). | `[[table]]` missing `ddl` errors | Y | core | Y | | `[[table]]` missing `name` errors | Y | Y | Y | | `[[table]]` `name` absent from the catalog after `ddl` errors | Y | Y | Y | +| `[[table]]` `ddl` batch: whole batch runs, catalog settles the result (#956) | core | Y | core | | Config `persist` / `persist_path` resolution | core | Y (`from_config.rs`) | core | ### Persistence diff --git a/docs/howto/define-tables.md b/docs/howto/define-tables.md index 603d038ee..ae0294e9c 100644 --- a/docs/howto/define-tables.md +++ b/docs/howto/define-tables.md @@ -65,6 +65,50 @@ Add one `[[table]]` entry per table — each with its own `glob`, `ddl`, and table — each table is an independent view. See [`[[table]]`](../reference/config.md#table) for the remaining key, `strict`. +## Add indexes and full-text search + +`ddl` is a SQL batch, not a single statement — SQLite runs the whole thing. So +a table can arrive with its own index, and with an FTS5 index kept current by +two triggers: + +```toml +[[table]] +name = "posts" +glob = "posts/**/*.md" +on-file = "python3 extract.py {path}" +ddl = ''' +CREATE TABLE posts (title TEXT, slug TEXT, body TEXT); +CREATE INDEX posts_slug ON posts(slug); + +CREATE VIRTUAL TABLE posts_fts + USING fts5(body, content='posts', content_rowid='rowid'); +CREATE TRIGGER posts_ai AFTER INSERT ON posts BEGIN + INSERT INTO posts_fts(rowid, body) VALUES (new.rowid, new.body); +END; +CREATE TRIGGER posts_ad AFTER DELETE ON posts BEGIN + INSERT INTO posts_fts(posts_fts, rowid, body) + VALUES ('delete', old.rowid, old.body); +END; +''' +``` + +```bash +dirsql query "SELECT slug FROM posts JOIN posts_fts ON posts.rowid = posts_fts.rowid + WHERE posts_fts MATCH 'recursion'" -c ./.dirsql.toml +``` + +```json +[{"slug":"again"}] +``` + +dirsql writes rows with plain `INSERT` and `DELETE`, so those triggers hold +through the initial load and every [watcher](./react-to-changes.md) event — you +maintain nothing. `name` still has to be the row table (`posts`); the virtual +table lives beside it under its own name. The batch runs once, when the table +is created, and editing any part of it rebuilds a +[persistent cache](./persist.md) from scratch. Full rules: +[Batch `ddl`](../reference/config.md#batch-ddl). + ## Going further - The parser mechanics — placeholders, stdout protocol, per-file failure diff --git a/docs/howto/load-extension.md b/docs/howto/load-extension.md index 5a9ea7296..f569553a7 100644 --- a/docs/howto/load-extension.md +++ b/docs/howto/load-extension.md @@ -76,9 +76,11 @@ interpreter to resolve package names with ## Notes -- Extensions add **functions**. An extension-backed virtual table cannot be - declared as a `[[table]]` — `dirsql` tables are per-file row tables - ([reference](../reference/config.md#dirsql-extension)). +- Extensions add **functions**, and virtual tables a table's `ddl` batch can + create. A `[[table]]`'s own `name` may not be a virtual table — that one is + the per-file row table `dirsql` inserts into — so declare the virtual table + alongside it in the same batch + ([reference](../reference/config.md#batch-ddl)). - Loading happens before any table DDL runs, and the SQL `load_extension()` function is never exposed to queries ([reference](../reference/config.md#dirsql-extension)). diff --git a/docs/howto/search-by-meaning.md b/docs/howto/search-by-meaning.md index f692cff3e..1d5e15885 100644 --- a/docs/howto/search-by-meaning.md +++ b/docs/howto/search-by-meaning.md @@ -95,10 +95,12 @@ LIMIT 10 ::: tip Top-k is `LIMIT k` If you know `sqlite-vec` you may reach for its `MATCH … AND k = 10` idiom. -That syntax belongs to `sqlite-vec`'s `vec0` virtual table, which `dirsql` -does not use — `dirsql` tables are per-file row tables. For plain -expressions, `sqlite-vec`'s own documented pattern is exactly what this -guide uses: `ORDER BY vec_distance_cosine(...) LIMIT k`. +That syntax belongs to `sqlite-vec`'s `vec0` virtual table, which the table +above does not use: a `[[table]]`'s own `name` is always a per-file row table. +For plain expressions, `sqlite-vec`'s own documented pattern is exactly what +this guide uses: `ORDER BY vec_distance_cosine(...) LIMIT k`. To get the `vec0` +idiom instead, declare the `vec0` table alongside the row table in the same +[`ddl` batch](../reference/config.md#batch-ddl) and fill it from a trigger. ::: ## Repeat runs are cheap diff --git a/docs/reference/config.md b/docs/reference/config.md index 407be9efa..0f1f58c1c 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -87,10 +87,11 @@ name you installed: `path = "sqlite-vec-linux-x64"` (matching your platform), not `path = "sqlite-vec"`, whose meta-package ships no loadable. -Extensions add **functions** callable in queries and in a table's DDL. An -extension-backed **virtual table** cannot be declared as a `[[table]]` — -`dirsql` tables are per-file row tables, so a `CREATE VIRTUAL TABLE` DDL is -rejected; call the extension's functions in queries instead. +Extensions add **functions** callable in queries and in a table's DDL, and +**virtual tables** a table's [`ddl` batch](#batch-ddl) can create. What a +`[[table]]`'s own `name` may not be is a virtual table: that one is the +per-file row table `dirsql` inserts into. Create the virtual table alongside +it, under its own name. ## `[[dirsql.function]]` @@ -167,7 +168,7 @@ what its required `on-file` command emits — dirsql injects nothing (see | Key | Required | Description | |---|---|---| | `name` | yes | The table's SQL name — the name you query it by. Declared, never derived from `ddl`: dirsql does not read the DDL text. The `ddl` must create a table by this name; if it doesn't, loading fails. | -| `ddl` | yes | A SQLite `CREATE TABLE` statement, run verbatim. Only the columns declared here are kept; keys the `on-file` command emits that are not declared are dropped. | +| `ddl` | yes | A SQL batch, run verbatim — any number of statements. It must create a table called `name`; that table holds the file rows, and only the columns it declares are kept (keys the `on-file` command emits that are not declared are dropped). The rest of the batch is yours: indexes, virtual tables, triggers. See [Batch `ddl`](#batch-ddl). | | `glob` | yes | Glob pattern matched against root-relative paths. Every table whose glob matches a file receives that file's rows — a file can populate multiple tables. A `{name}` segment is rewritten to `*` (it matches one path segment but captures nothing). | | `on-file` | **yes** | A command run once per matched file; its stdout (a JSON array of row objects) becomes the file's rows. Must be non-empty. A `[[table]]` with no `on-file` is a load error (see [parse errors](#parse-errors)). See [Command hooks](./hooks.md#on-file). | | `strict` | no (default `false`) | When `true`, rows whose keys do not exactly match the declared columns are rejected with an error: extra keys error, and every declared column must be supplied by the `on-file` output. When `false`, extra keys are dropped and missing columns become `NULL`. | @@ -194,6 +195,72 @@ on-file = "uv run python extract_papers.py {path}" strict = true ``` +### Batch `ddl` + +`ddl` is handed to SQLite whole, so a table declaration is not limited to one +statement: + +```toml +[[table]] +name = "messages" +glob = "sessions/*/messages/*.json" +on-file = "jq -c '.' {path}" +ddl = ''' +CREATE TABLE messages (session TEXT, idx INT, role TEXT, text TEXT); +CREATE INDEX messages_session ON messages(session); + +CREATE VIRTUAL TABLE messages_fts + USING fts5(text, content='messages', content_rowid='rowid'); +CREATE TRIGGER messages_ai AFTER INSERT ON messages BEGIN + INSERT INTO messages_fts(rowid, text) VALUES (new.rowid, new.text); +END; +CREATE TRIGGER messages_ad AFTER DELETE ON messages BEGIN + INSERT INTO messages_fts(messages_fts, rowid, text) + VALUES ('delete', old.rowid, old.text); +END; +''' +``` + +```bash +dirsql query "SELECT text FROM messages_fts WHERE messages_fts MATCH 'deploy'" \ + -c ./.dirsql.toml +``` + +Those two triggers are all a keyword index needs. dirsql writes file rows with +plain `INSERT` and `DELETE` — an update is a delete and an insert in one +transaction, and there is no `UPDATE` path on user rows — so triggers you +declare here stay current through the initial load and every +[watcher](../howto/react-to-changes.md) event. The same shape with a `vec0` +virtual table and an [`embed()`](#dirsql-function) call in the trigger gives +you stored vectors. + +**dirsql never reads the batch.** After it runs, SQLite's own catalog +(`pragma_table_list`) settles what it produced: + +- No table called `name` → a load error that lists what the batch *did* + create, so a typo is obvious. +- `name` is a **virtual** table → a load error. The declared table is the one + dirsql inserts file rows into, so it has to be a real row table. Create the + virtual table alongside it, under its own name. +- `name` is **`WITHOUT ROWID`** → a warning. Internal row bookkeeping is keyed + on rowid, so these will be rejected in a future release. + +The whole batch runs in one transaction: if any statement fails, none of them +took effect, and the error is SQLite's own, prefixed with the config entry — +`table 'messages': SQLite error: near "(": syntax error`. Context, never +interpretation. + +Two consequences of `ddl` running **once, when the table is created**: + +- **Rows the batch inserts itself are not file-tracked.** No file owns them, so + they survive file deletions — and they vanish on any rebuild. +- **Editing `ddl` at all rebuilds a + [persistent cache](../howto/persist.md).** The config hash covers the entire + batch, so a new index, a different FTS5 + tokenizer or a changed embedding model id forces a full sweep and re-ingest. + That is the only invalidation lane: dirsql tracks no ownership of what the + batch made. + ### `on-file` row mapping The command prints a JSON array of objects; each object becomes one row. @@ -247,13 +314,24 @@ SDKs raise/reject) when: - A `[[table]]` entry omits `name`, `ddl`, or `glob` (or `name` is empty/whitespace). - A `[[table]]` entry's `ddl` runs but creates no table by its `name`. The - error carries the entry's name and points at the fix: + error carries the entry's name, lists what the batch did create, and points + at the fix: - > `table 'messages': its `ddl` ran but created no table called 'messages'. Set `name` to the table the `ddl` creates.` + > `table 'messages': its `ddl` ran but created no table called 'messages' (it created: mesages, mesages_fts). Set `name` to the table the `ddl` creates.` dirsql asks SQLite's catalog rather than interpreting the DDL, so quoted (`CREATE TABLE "messages"`), schema-qualified (`main.messages`) and `IF NOT EXISTS` forms all match a plain `name = "messages"`. +- A `[[table]]` entry's `name` names a **virtual** table. The declared table + holds the file rows, so it must be a real row table: + + > `table 'messages': its `ddl` created a virtual table called 'messages'. The declared table holds the file rows, so it must be a real row table; create the virtual table alongside it, under its own name.` + +- A `[[table]]` entry's `ddl` is rejected by SQLite. Nothing the batch did + takes effect, and SQLite's own message is passed through under the entry's + name: + + > `table 'messages': SQLite error: near "(": syntax error` - A `[[table]]` entry omits `on-file` (or it is empty/whitespace). The error names the offending glob and points at the fix: diff --git a/packages/rust/changelog.d/2026-08-20-ddl-batch.md b/packages/rust/changelog.d/2026-08-20-ddl-batch.md new file mode 100644 index 000000000..02ccad0d9 --- /dev/null +++ b/packages/rust/changelog.d/2026-08-20-ddl-batch.md @@ -0,0 +1,9 @@ +**Changed** + +- **A `[[table]]`'s `ddl` is now a wide-open multi-statement SQL batch.** It runs through `execute_batch` inside one transaction, so a table can arrive with its own indexes, virtual tables and triggers — `CREATE INDEX` for B-trees, an external-content FTS5 vtab plus two triggers for keyword search with `bm25()`/`snippet()`, a `vec0` vtab plus an `embed()` trigger for stored vectors. dirsql writes user rows with plain `INSERT`/`DELETE` (an upsert is a delete and an insert in one transaction, and there is no `UPDATE` path on user rows), so those triggers stay current through the initial load and every watcher event, with no bookkeeping on dirsql's side. The two string sniffs the single-statement rule needed — `is_virtual_table_ddl` and `is_without_rowid_ddl` — are gone: what the batch produced is settled against SQLite's catalog (`pragma_table_list`) instead. A declared `name` that ends up a virtual table is a load error; one that is `WITHOUT ROWID` keeps its existing warning. (#956) + +**Fixed** + +- **A `ddl` holding more than one statement no longer runs only its first.** `Connection::execute` prepares one statement and discards the rest of the string, so a second statement was silently dropped — a `CREATE INDEX` that never ran, and a batch whose tail SQLite could not even parse still loading successfully. The whole batch now runs, and a statement SQLite rejects fails the load with SQLite's own message under the config entry that caused it (`table 'messages': SQLite error: near "(": syntax error`), leaving nothing behind. (#956) + +- **The persistent-cache sweep now drops virtual tables before plain ones.** Drop order came from `sqlite_master`'s creation order, which happened to be safe; it is now derived from `pragma_table_list`'s type. Dropping a shadow table before the virtual table that owns it poisons the vtab's own drop even with `IF EXISTS` (probed against sqlite-vec 0.1.9), which a `ddl` batch creating a `vec0` table could now hit. (#956) diff --git a/packages/rust/migrations.d/2026-08-20-ddl-batch.md b/packages/rust/migrations.d/2026-08-20-ddl-batch.md new file mode 100644 index 000000000..193602d19 --- /dev/null +++ b/packages/rust/migrations.d/2026-08-20-ddl-batch.md @@ -0,0 +1,88 @@ +### `ddl` becomes a multi-statement SQL batch, validated against the catalog (#956) + +#### Summary + +A `[[table]]`'s `ddl` had to be exactly one statement, and dirsql string-sniffed +it for `CREATE VIRTUAL TABLE` and `WITHOUT ROWID`. It is now a whole SQL batch, +run through `execute_batch` inside one transaction, and what it produced is +settled afterwards against SQLite's catalog (`pragma_table_list`) rather than by +reading the DDL text. `Db::create_table` keeps its `(name, ddl)` signature; +`db::is_virtual_table_ddl`, `db::is_without_rowid_ddl` and `DbError::DdlParse` +are deleted. `DirSqlError` gains `TableIsVirtual` and `TableDdl`, and +`TableNotCreated` gains a `created` field carrying the before/after catalog diff. + +#### Required changes + +| Before | After | Fix | +| ------ | ----- | --- | +| `Err(DirSqlError::TableNotCreated { name })` as an exhaustive pattern | Does not compile — the variant gained `created: Vec` | Match `TableNotCreated { name, .. }`, or bind `created` to read what the batch did create. | +| `Err(DirSqlError::Core(_))` for a `ddl` SQLite rejects | No longer produced for that case | Match `DirSqlError::TableDdl { name, source }`; `source` is the same `DbError` as before, and `Display` prefixes it with `table '': `. | +| `DbError::DdlParse` constructed or matched | Does not compile | Nothing produces it any more. A rejected batch is `DbError::Sqlite`, reached through `DirSqlError::TableDdl`. | +| `ddl = "CREATE VIRTUAL TABLE vss USING vec0(...)"` rejected before running | The batch runs; the declared table being virtual is caught after, as `DirSqlError::TableIsVirtual` | Keep `name` on a real row table and declare the virtual table alongside it in the same batch. | + +#### Deprecations removed + +- `DbError::DdlParse` — removed outright, with `db::is_virtual_table_ddl` and + `db::is_without_rowid_ddl`, the two whitespace-normalizing string sniffs + behind it. + +#### Behavior changes without code changes + +- **Every statement in `ddl` now runs.** `Connection::execute` prepared only the + first and discarded the tail, so a config with `CREATE TABLE t (...); CREATE + INDEX ...` silently created the table without the index, and one whose second + statement SQLite could not parse still loaded. Both now behave as written: the + index is created, and the unparseable batch is a load error. +- **A batch SQLite rejects leaves nothing behind**, and its error carries the + config entry: `table 'messages': SQLite error: near "(": syntax error`. + Previously the first statement of a partly-invalid `ddl` took effect. +- **A `[[table]]` whose `name` resolves to a virtual table is a load error** + raised after the batch runs, not a string match before it. A batch is now free + to create as many virtual tables as it likes under other names. +- **`WITHOUT ROWID` is detected from the catalog's `wr` flag** rather than from + the DDL text. Same warning, same "will be rejected in a future release" + wording; a `WITHOUT ROWID` *shadow* table belonging to some other virtual + table no longer triggers it. +- **The persistent-cache sweep drops virtual tables first**, derived from + `pragma_table_list`'s type instead of `sqlite_master`'s creation order, and + leaves shadow tables to the virtual table that owns them. +- **Editing any part of `ddl` still invalidates a persistent cache** — the + config hash already covered the whole string, and now that the whole string + runs, that is the single invalidation lane for indexes, tokenizers and + embedding model ids alike. Rows a batch inserts itself are not file-tracked: + they survive file deletions and vanish on a rebuild. + +#### Verification + +```bash +# .dirsql.toml +# [[table]] +# name = "records" +# glob = "data/*.json" +# on-file = "cat {path}" +# ddl = ''' +# CREATE TABLE records (id TEXT, body TEXT); +# CREATE INDEX records_id ON records(id); +# CREATE VIRTUAL TABLE records_fts +# USING fts5(body, content='records', content_rowid='rowid'); +# CREATE TRIGGER records_ai AFTER INSERT ON records BEGIN +# INSERT INTO records_fts(rowid, body) VALUES (new.rowid, new.body); +# END; +# ''' + +dirsql query "SELECT name FROM pragma_index_list('records')" -c ./.dirsql.toml +# -> [{"name":"records_id"}] (before: [], the CREATE INDEX was dropped) + +dirsql query "SELECT body FROM records_fts WHERE records_fts MATCH 'hello'" \ + -c ./.dirsql.toml +# -> [{"body":"hello world"}] + +# Truncate the batch's last statement: +dirsql query "SELECT id FROM records" -c ./.dirsql.toml +# -> error: table 'records': SQLite error: incomplete input +# (before: exit 0 — the tail was silently ignored) + +# Point `name` at a virtual table: +dirsql query "SELECT id FROM records" -c ./.dirsql.toml +# -> error: table 'records': its `ddl` created a virtual table called 'records'. +``` diff --git a/packages/rust/src/db.rs b/packages/rust/src/db.rs index 3d459cfd0..addb5cb60 100644 --- a/packages/rust/src/db.rs +++ b/packages/rust/src/db.rs @@ -24,9 +24,6 @@ pub enum DbError { #[error("Schema mismatch: {0}")] SchemaMismatch(String), - #[error("DDL parse error: {0}")] - DdlParse(String), - #[error("invalid identifier: {0:?} (must match [A-Za-z_][A-Za-z0-9_]*)")] InvalidIdentifier(String), @@ -400,41 +397,26 @@ impl Db { Ok(()) } - /// Create a table from a user-provided DDL statement, executed - /// **verbatim**: dirsql injects no tracking columns — row ownership lives - /// entirely in `_dirsql_internal_rows`, so a table's schema is exactly - /// the DDL the user wrote. + /// Run a table's user-provided DDL **batch**, executed **verbatim**: + /// dirsql injects no tracking columns — row ownership lives entirely in + /// `_dirsql_internal_rows`, so a table's schema is exactly the DDL the + /// user wrote. + /// + /// The batch may hold any number of statements, of any kind: the row table + /// plus whatever indexes, virtual tables and triggers go with it. It runs + /// inside one transaction, so a statement SQLite rejects leaves nothing + /// behind, and SQLite's own error is returned untouched. /// /// `name` is the table's declared name, validated as a safe unquoted SQL - /// identifier before the DDL reaches SQLite: it is spliced into + /// identifier before the batch reaches SQLite: it is spliced into /// `format!()`-built INSERT/DELETE SQL downstream, so a poisoned name must - /// never get that far. Whether the DDL actually creates a table by that - /// name is settled afterwards against SQLite's catalog, not by reading the - /// DDL text. + /// never get that far. What the batch actually created is settled + /// afterwards against SQLite's catalog, not by reading the DDL text. pub fn create_table(&self, name: &str, ddl: &str) -> Result<()> { - // A dirsql table is a per-file row table; an extension-backed virtual - // table is not one, so reject `CREATE VIRTUAL TABLE` with a clear - // message. Load the extension and use its functions in queries instead. - if is_virtual_table_ddl(ddl) { - return Err(DbError::DdlParse( - "CREATE VIRTUAL TABLE is not supported as a dirsql table \ - (dirsql tables are per-file row tables); load the extension \ - and call its functions in queries instead" - .to_string(), - )); - } validate_identifier(name)?; - // A `WITHOUT ROWID` table has no rowid, so `last_insert_rowid()` cannot - // identify the inserted row and the `_dirsql_internal_rows.rowid_ref` - // mapping would be meaningless. Warn; the table is still created. - if is_without_rowid_ddl(ddl) { - eprintln!( - "dirsql: table `{name}` is declared WITHOUT ROWID; internal row \ - bookkeeping relies on rowid and WITHOUT ROWID tables will be \ - rejected in a future release" - ); - } - self.conn.execute(ddl, [])?; + let tx = self.conn.unchecked_transaction()?; + self.conn.execute_batch(ddl)?; + tx.commit()?; Ok(()) } @@ -900,21 +882,58 @@ impl rusqlite::types::ToSql for Value { } } -/// True if `ddl` is a `CREATE VIRTUAL TABLE` statement. dirsql tables are -/// per-file row tables, structurally incompatible with an extension-backed -/// virtual table, so those are rejected with a clear error. -fn is_virtual_table_ddl(ddl: &str) -> bool { - let normalized = ddl.split_whitespace().collect::>().join(" "); - normalized.to_uppercase().contains("CREATE VIRTUAL TABLE") +/// What SQLite's catalog says about one table in `main`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct CatalogEntry { + /// `pragma_table_list`'s `type`: `table`, `virtual`, `view` or `shadow`. + pub kind: String, + /// Whether the table is declared `WITHOUT ROWID`. + pub without_rowid: bool, +} + +/// The catalog rows a table's `ddl` batch could be responsible for: real and +/// virtual tables in `main`, never the shadow tables a virtual table keeps +/// behind it, SQLite's own `sqlite_*` tables, or dirsql's `_dirsql_*` +/// bookkeeping. +/// +/// Virtual tables come first, and that order is load-bearing for the cache +/// sweep: dropping a shadow table before its virtual table poisons the virtual +/// table's own drop even with `IF EXISTS` (probed against sqlite-vec 0.1.9). +/// `sqlite_master`'s creation order happens to get this right today; asking +/// the catalog for the type makes it guaranteed. +const USER_TABLES_SQL: &str = "SELECT name FROM pragma_table_list \ + WHERE schema = 'main' AND type IN ('table', 'virtual') \ + AND name NOT LIKE '_dirsql_%' AND name NOT LIKE 'sqlite_%' \ + ORDER BY type = 'virtual' DESC, name"; + +/// Every user table on `conn`, in an order that is safe to drop in. +pub(crate) fn user_table_names(conn: &Connection) -> rusqlite::Result> { + let mut stmt = conn.prepare(USER_TABLES_SQL)?; + let names = stmt + .query_map([], |row| row.get::<_, String>(0))? + .collect::>>()?; + Ok(names) } -/// True if `ddl` declares a `WITHOUT ROWID` table. Such tables have no rowid, -/// so `last_insert_rowid()` cannot identify an inserted row and the -/// `_dirsql_internal_rows.rowid_ref` mapping is meaningless. Whitespace is -/// normalized so `WITHOUT ROWID` and newline-separated forms are detected. -fn is_without_rowid_ddl(ddl: &str) -> bool { - let normalized = ddl.split_whitespace().collect::>().join(" "); - normalized.to_uppercase().contains("WITHOUT ROWID") +/// What `conn`'s catalog holds under `name`, or `None` if it holds nothing. +/// `pragma_table_list` is the catalog API for exactly this question, so no +/// part of dirsql has to interpret DDL text to answer it. +pub(crate) fn table_catalog_entry( + conn: &Connection, + name: &str, +) -> rusqlite::Result> { + use rusqlite::OptionalExtension; + conn.query_row( + "SELECT type, wr FROM pragma_table_list WHERE schema = 'main' AND name = ?1", + rusqlite::params![name], + |row| { + Ok(CatalogEntry { + kind: row.get(0)?, + without_rowid: row.get::<_, i64>(1)? != 0, + }) + }, + ) + .optional() } impl From for Value { @@ -1351,8 +1370,8 @@ mod tests { let err = DbError::SchemaMismatch("test error".to_string()); assert!(err.to_string().contains("Schema mismatch")); - let err = DbError::DdlParse("bad ddl".to_string()); - assert!(err.to_string().contains("DDL parse error")); + let err = DbError::InvalidIdentifier("a b".to_string()); + assert!(err.to_string().contains("invalid identifier")); } #[test] @@ -1551,25 +1570,6 @@ mod tests { assert!(matches!(err, DbError::Sqlite(_)), "got: {err}"); } - #[test] - fn create_table_rejects_virtual_table_with_clear_error() { - let db = Db::new().unwrap(); - let err = db - .create_table( - "vss", - "CREATE VIRTUAL TABLE vss USING vec0(embedding float[4])", - ) - .unwrap_err(); - // Must be a clear "not supported" message, NOT the generic `DdlParse` - // echo (which trivially contains "virtual table" because it echoes - // the DDL back). - let msg = err.to_string().to_lowercase(); - assert!( - msg.contains("virtual table") && msg.contains("not supported"), - "expected a clear 'virtual table not supported' error, not a generic DDL-parse echo, got: {err}" - ); - } - #[test] fn open_sets_wal_journal_mode_and_normal_synchronous() { let dir = tempfile::TempDir::new().unwrap(); @@ -1588,18 +1588,6 @@ mod tests { assert_eq!(synchronous, 1, "Db::open must set synchronous=NORMAL (1)"); } - #[test] - fn is_virtual_table_ddl_detects_variants() { - assert!(is_virtual_table_ddl("CREATE VIRTUAL TABLE x USING vec0(a)")); - assert!(is_virtual_table_ddl( - "create virtual table x using fts5(a)" - )); - assert!(!is_virtual_table_ddl("CREATE TABLE x (a TEXT)")); - assert!(!is_virtual_table_ddl( - "CREATE TABLE IF NOT EXISTS x (a TEXT)" - )); - } - /// Read the raw mapping rows for a table, ordered for stable assertions. fn mapping_rows(db: &Db, table: &str) -> Vec<(String, i64, i64)> { let mut stmt = db @@ -1821,7 +1809,65 @@ mod tests { } #[test] - fn create_table_allows_without_rowid_and_warns() { + fn create_table_runs_every_statement_in_the_batch() { + let db = Db::new().unwrap(); + db.create_table( + "notes", + "CREATE TABLE notes (path TEXT);\n\ + CREATE INDEX notes_path ON notes(path);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(path);", + ) + .unwrap(); + + assert_eq!(user_table_names(&db.conn).unwrap(), ["notes_fts", "notes"]); + let index: i64 = db + .conn + .query_row( + "SELECT COUNT(*) FROM pragma_index_list('notes') WHERE name = 'notes_path'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(index, 1, "the batch's CREATE INDEX must have run"); + } + + #[test] + fn create_table_rolls_back_a_batch_sqlite_rejects() { + let db = Db::new().unwrap(); + let err = db + .create_table( + "notes", + "CREATE TABLE notes (path TEXT); CREATE TABLE oops (", + ) + .unwrap_err(); + + assert!(matches!(err, DbError::Sqlite(_)), "got: {err}"); + assert!( + err.to_string().contains("incomplete input"), + "SQLite's own error must come through untouched, got: {err}" + ); + assert!( + user_table_names(&db.conn).unwrap().is_empty(), + "a rejected batch must leave none of its earlier statements behind" + ); + } + + #[test] + fn create_table_validates_the_name_before_running_the_batch() { + let db = Db::new().unwrap(); + let err = db + .create_table("a b", "CREATE TABLE \"a b\" (path TEXT)") + .unwrap_err(); + + assert!(matches!(err, DbError::InvalidIdentifier(_)), "got: {err}"); + assert!( + user_table_names(&db.conn).unwrap().is_empty(), + "a poisoned name must be rejected before SQLite sees the batch" + ); + } + + #[test] + fn create_table_still_runs_a_without_rowid_ddl() { let db = Db::new().unwrap(); db.create_table("t", "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID") .unwrap(); @@ -1830,14 +1876,62 @@ mod tests { } #[test] - fn is_without_rowid_ddl_detects_variants() { - assert!(is_without_rowid_ddl( - "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID" - )); - assert!(is_without_rowid_ddl( - "create table t (id text primary key)\n without rowid" - )); - assert!(!is_without_rowid_ddl("CREATE TABLE t (id TEXT)")); + fn user_table_names_lists_virtual_tables_before_real_ones() { + let db = Db::new().unwrap(); + // Created real-table-first, so `sqlite_master`'s creation order would + // put `notes` ahead of the virtual table it belongs to. + db.conn + .execute_batch( + "CREATE TABLE notes (body TEXT);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(body);\n\ + CREATE TABLE archive (body TEXT);", + ) + .unwrap(); + + assert_eq!( + user_table_names(&db.conn).unwrap(), + ["notes_fts", "archive", "notes"] + ); + } + + #[test] + fn user_table_names_omits_shadow_and_bookkeeping_tables() { + let db = Db::new().unwrap(); + db.conn + .execute_batch("CREATE VIRTUAL TABLE notes_fts USING fts5(body);") + .unwrap(); + + let names = user_table_names(&db.conn).unwrap(); + assert_eq!( + names, + ["notes_fts"], + "FTS5's shadow tables and `_dirsql_internal_rows` must not be listed" + ); + } + + #[test] + fn table_catalog_entry_reports_kind_and_without_rowid() { + let db = Db::new().unwrap(); + db.conn + .execute_batch( + "CREATE TABLE plain (id TEXT);\n\ + CREATE TABLE keyed (id TEXT PRIMARY KEY) WITHOUT ROWID;\n\ + CREATE VIRTUAL TABLE virt USING fts5(body);", + ) + .unwrap(); + + let plain = table_catalog_entry(&db.conn, "plain").unwrap().unwrap(); + assert_eq!(plain.kind, "table"); + assert!(!plain.without_rowid); + + let keyed = table_catalog_entry(&db.conn, "keyed").unwrap().unwrap(); + assert_eq!(keyed.kind, "table"); + assert!(keyed.without_rowid); + + let virt = table_catalog_entry(&db.conn, "virt").unwrap().unwrap(); + assert_eq!(virt.kind, "virtual"); + + assert_eq!(table_catalog_entry(&db.conn, "absent").unwrap(), None); } #[test] diff --git a/packages/rust/src/lib.rs b/packages/rust/src/lib.rs index 97de62302..4ee6a8840 100644 --- a/packages/rust/src/lib.rs +++ b/packages/rust/src/lib.rs @@ -128,10 +128,25 @@ pub enum DirSqlError { Ddl(String), #[error( - "table '{name}': its `ddl` ran but created no table called '{name}'. \ - Set `name` to the table the `ddl` creates." + "table '{name}': its `ddl` ran but created no table called '{name}' \ + (it created: {}). Set `name` to the table the `ddl` creates.", + if .created.is_empty() { "nothing".to_string() } else { .created.join(", ") } )] - TableNotCreated { name: String }, + TableNotCreated { name: String, created: Vec }, + + #[error( + "table '{name}': its `ddl` created a virtual table called '{name}'. \ + The declared table holds the file rows, so it must be a real row \ + table; create the virtual table alongside it, under its own name." + )] + TableIsVirtual { name: String }, + + #[error("table '{name}': {source}")] + TableDdl { + name: String, + #[source] + source: DbError, + }, #[error("failed to load extension '{}': {source}", .path.display())] Extension { @@ -848,17 +863,28 @@ impl DirSQL { for table in tables { let table_name = table.name.clone(); - // When the cache already holds this table from a prior run, - // skip CREATE TABLE: the schema is preserved verbatim across - // runs (the glob_config_hash captures the DDL). - if !table_exists(&db, &table_name)? { - db.create_table(&table_name, &table.ddl)?; - // Ask SQLite whether the DDL actually produced this name - // rather than reading the DDL text. No row means the declared - // `name` names nothing -- a load-time failure, before any file - // is ingested. - if !table_exists(&db, &table_name)? { - return Err(DirSqlError::TableNotCreated { name: table_name }); + // A real table under this name is the cache already holding it + // from a prior run: skip the batch, since the schema is preserved + // verbatim across runs (the glob_config_hash captures the whole + // `ddl`). Anything else under the name is not a warm cache — run + // the batch and let SQLite object to the collision itself. + let warm = catalog_entry(&db, &table_name)?.is_some_and(|entry| entry.kind == "table"); + if !warm { + let before = db::user_table_names(db.conn()).map_err(DirSqlError::sqlite)?; + db.create_table(&table_name, &table.ddl).map_err(|source| { + DirSqlError::TableDdl { + name: table_name.clone(), + source, + } + })?; + let created = db::user_table_names(db.conn()) + .map_err(DirSqlError::sqlite)? + .into_iter() + .filter(|name| !before.contains(name)) + .collect(); + let entry = catalog_entry(&db, &table_name)?; + if let Some(warning) = classify_declared_table(&table_name, entry, created)? { + eprintln!("{warning}"); } } // Reject a `{name}` glob placeholder whose name is also a declared @@ -1564,20 +1590,52 @@ fn reconcile_scan( Ok((to_parse, trusted, deleted)) } -/// Whether `main` holds a table called `name`, asked of SQLite's own catalog. -/// `pragma_table_list` is the catalog API for exactly this question, so no -/// part of dirsql has to interpret DDL text to answer it. -fn table_exists(db: &Db, name: &str) -> Result { - let count: i64 = db - .conn() - .query_row( - "SELECT COUNT(*) FROM pragma_table_list \ - WHERE schema = 'main' AND type = 'table' AND name = ?1", - rusqlite::params![name], - |row| row.get(0), - ) - .map_err(DirSqlError::sqlite)?; - Ok(count > 0) +/// What SQLite's catalog holds under `name`, if anything. +fn catalog_entry(db: &Db, name: &str) -> Result> { + db::table_catalog_entry(db.conn(), name).map_err(DirSqlError::sqlite) +} + +/// Settle what a table's `ddl` batch produced for its declared `name`, given +/// SQLite's catalog and the tables the batch added. `created` is a before/after +/// catalog diff, so a `name` that never appeared can say what did. +/// +/// `Ok(None)` accepts the table, `Ok(Some(warning))` accepts it with a line for +/// stderr, and `Err` fails the load before any file is ingested. Pure: the +/// caller reads the catalog and prints the warning. +fn classify_declared_table( + name: &str, + entry: Option, + mut created: Vec, +) -> Result> { + let Some(entry) = entry else { + // The catalog hands these back in drop-safe order (virtuals first); + // sort so the message reads as a list rather than as an artifact of + // how the sweep needs them. + created.sort(); + return Err(DirSqlError::TableNotCreated { + name: name.to_string(), + created, + }); + }; + // A dirsql table is the per-file row table rows are inserted into; an + // extension-backed virtual table is not one. The batch may still create as + // many as it likes alongside it. + if entry.kind == "virtual" { + return Err(DirSqlError::TableIsVirtual { + name: name.to_string(), + }); + } + // A `WITHOUT ROWID` table has no rowid, so `last_insert_rowid()` cannot + // identify the inserted row and the `_dirsql_internal_rows.rowid_ref` + // mapping would be meaningless. Warn; the table is still usable. + if entry.without_rowid { + return Ok(Some(format!( + "dirsql: table `{name}` is declared WITHOUT ROWID; internal row \ + bookkeeping relies on rowid and WITHOUT ROWID tables will be \ + rejected in a future release" + ))); + } + Ok(None) } fn map_db_error(e: DbError) -> DirSqlError { @@ -3328,6 +3386,98 @@ mod internal_tests { assert!(msg.contains("on-file")); } + /// A `name` the batch never created: the error has to say what it *did* + /// create, or a typo gives the reader nothing to compare against. + #[test] + fn classify_declared_table_rejects_a_name_the_batch_never_created() { + let err = classify_declared_table( + "messages", + None, + vec!["mesages_fts".to_string(), "mesages".to_string()], + ) + .expect_err("a missing catalog entry must fail the load"); + + let msg = err.to_string(); + assert!( + matches!(err, DirSqlError::TableNotCreated { .. }), + "got: {msg}" + ); + assert!(msg.contains("table 'messages'"), "got: {msg}"); + assert!( + msg.contains("it created: mesages, mesages_fts"), + "the list must read sorted, not in the catalog's drop order, got: {msg}" + ); + } + + #[test] + fn table_not_created_says_nothing_when_the_batch_created_nothing() { + let err = DirSqlError::TableNotCreated { + name: "messages".to_string(), + created: Vec::new(), + }; + assert!( + err.to_string().contains("it created: nothing"), + "got: {err}" + ); + } + + #[test] + fn classify_declared_table_rejects_a_virtual_declared_table() { + let entry = db::CatalogEntry { + kind: "virtual".to_string(), + without_rowid: false, + }; + let err = classify_declared_table("messages", Some(entry), Vec::new()) + .expect_err("a virtual declared table must fail the load"); + + let msg = err.to_string(); + assert!( + matches!(err, DirSqlError::TableIsVirtual { .. }), + "got: {msg}" + ); + assert!( + msg.contains("table 'messages'") && msg.contains("virtual"), + "got: {msg}" + ); + } + + #[test] + fn classify_declared_table_warns_about_without_rowid() { + let entry = db::CatalogEntry { + kind: "table".to_string(), + without_rowid: true, + }; + let warning = classify_declared_table("messages", Some(entry), Vec::new()) + .expect("a WITHOUT ROWID table is a warning, not a failure") + .expect("it must produce a warning"); + + assert!(warning.contains("messages"), "got: {warning}"); + assert!(warning.contains("WITHOUT ROWID"), "got: {warning}"); + } + + #[test] + fn classify_declared_table_accepts_a_plain_row_table() { + let entry = db::CatalogEntry { + kind: "table".to_string(), + without_rowid: false, + }; + assert_eq!( + classify_declared_table("messages", Some(entry), Vec::new()).unwrap(), + None + ); + } + + #[test] + fn table_ddl_error_prefixes_sqlites_own_message_with_the_entry() { + let err = DirSqlError::TableDdl { + name: "messages".to_string(), + source: DbError::SchemaMismatch("near \"(\": syntax error".to_string()), + }; + let msg = err.to_string(); + assert!(msg.starts_with("table 'messages': "), "got: {msg}"); + assert!(msg.contains("near \"(\": syntax error"), "got: {msg}"); + } + #[test] fn build_tables_from_config_creates_on_file_tables() { let cfg = config::load_config_str(concat!( diff --git a/packages/rust/src/persist.rs b/packages/rust/src/persist.rs index 737a99fd1..ac1c408bc 100644 --- a/packages/rust/src/persist.rs +++ b/packages/rust/src/persist.rs @@ -414,15 +414,14 @@ pub fn delete_file(conn: &Connection, rel_path: &str, table_name: &str) -> rusql /// Drop every user-defined table from a cache database. Used when the /// reconcile detects an incompatible meta state and we need to wipe the /// cache before re-ingesting from scratch. +/// +/// A table's `ddl` is a whole SQL batch, so the cache may hold virtual tables +/// and the shadow tables behind them. `db::user_table_names` orders the drops +/// virtuals-first (and leaves shadows to the virtual table that owns them), +/// which is what keeps a vec0 vtab droppable; indexes and triggers go with +/// their table. pub fn drop_user_tables(conn: &Connection) -> rusqlite::Result<()> { - let mut stmt = - conn.prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE '_dirsql_%' AND name NOT LIKE 'sqlite_%'")?; - let names: Vec = stmt - .query_map([], |row| row.get::<_, String>(0))? - .filter_map(|r| r.ok()) - .collect(); - drop(stmt); - for name in names { + for name in crate::db::user_table_names(conn)? { conn.execute(&format!("DROP TABLE IF EXISTS \"{name}\""), [])?; } conn.execute("DELETE FROM _dirsql_files", [])?; @@ -607,6 +606,45 @@ mod tests { assert!(read_cached_files(&conn).unwrap().is_empty()); } + /// A `ddl` batch may leave a virtual table and its shadow tables in the + /// cache. The sweep must clear all of it: the shadows go with the virtual + /// table, and dropping one out from under the other is what poisons the + /// vtab's own drop. + #[test] + fn drop_user_tables_clears_virtual_tables_and_their_shadows() { + let conn = Connection::open_in_memory().unwrap(); + create_sidecar_tables(&conn).unwrap(); + conn.execute( + "CREATE TABLE _dirsql_internal_rows ( + table_name TEXT NOT NULL, file_path TEXT NOT NULL, + row_index INTEGER NOT NULL, rowid_ref INTEGER NOT NULL)", + [], + ) + .unwrap(); + conn.execute_batch( + "CREATE TABLE notes (body TEXT);\n\ + CREATE VIRTUAL TABLE notes_fts USING fts5(body, content='notes');", + ) + .unwrap(); + + drop_user_tables(&conn).unwrap(); + + let left: Vec = conn + .prepare( + "SELECT name FROM sqlite_master WHERE type = 'table' \ + AND name NOT LIKE '_dirsql_%' AND name NOT LIKE 'sqlite_%' ORDER BY name", + ) + .unwrap() + .query_map([], |row| row.get::<_, String>(0)) + .unwrap() + .collect::>>() + .unwrap(); + assert!( + left.is_empty(), + "the sweep must leave no user table or shadow table behind, got {left:?}" + ); + } + #[test] fn drop_user_tables_clears_user_data_and_files_index() { let conn = Connection::open_in_memory().unwrap(); diff --git a/packages/rust/tests/ddl_batch.rs b/packages/rust/tests/ddl_batch.rs index fa26da06c..b1890080f 100644 --- a/packages/rust/tests/ddl_batch.rs +++ b/packages/rust/tests/ddl_batch.rs @@ -112,7 +112,11 @@ fn an_fts5_index_declared_in_ddl_tracks_inserts_and_deletes() { let hits = db .query("SELECT body FROM notes_fts WHERE notes_fts MATCH 'hello'") .unwrap(); - assert_eq!(hits.len(), 1, "the insert trigger must have indexed the row"); + assert_eq!( + hits.len(), + 1, + "the insert trigger must have indexed the row" + ); assert_eq!(hits[0]["body"], Value::Text("hello world".into())); drop(db); @@ -167,7 +171,10 @@ fn a_declared_name_that_is_a_virtual_table_is_a_load_error() { seed(root.path()); let message = build_error( - build(root.path(), "CREATE VIRTUAL TABLE notes USING fts5(path, body)"), + build( + root.path(), + "CREATE VIRTUAL TABLE notes USING fts5(path, body)", + ), "a dirsql table must be a real per-file row table", ); assert!( diff --git a/packages/rust/tests/ddl_batch_e2e.rs b/packages/rust/tests/ddl_batch_e2e.rs index 0e09797e6..44d89eacb 100644 --- a/packages/rust/tests/ddl_batch_e2e.rs +++ b/packages/rust/tests/ddl_batch_e2e.rs @@ -73,10 +73,7 @@ fn a_multi_statement_batch_indexes_and_queries_through_the_cli() { let out = query(&root, "SELECT id FROM records ORDER BY id"); let parsed = rows(&out); - let ids: Vec<&str> = parsed - .iter() - .map(|r| r["id"].as_str().unwrap()) - .collect(); + let ids: Vec<&str> = parsed.iter().map(|r| r["id"].as_str().unwrap()).collect(); assert_eq!(ids, ["one", "two"]); let out = query(&root, "SELECT name FROM pragma_index_list('records')"); @@ -100,12 +97,10 @@ fn keyword_search_over_an_fts5_index_declared_in_ddl() { "SELECT body FROM records_fts WHERE records_fts MATCH 'hello'", ); let parsed = rows(&out); - let bodies: Vec<&str> = parsed - .iter() - .map(|r| r["body"].as_str().unwrap()) - .collect(); + let bodies: Vec<&str> = parsed.iter().map(|r| r["body"].as_str().unwrap()).collect(); assert_eq!( - bodies, ["hello world"], + bodies, + ["hello world"], "the insert trigger must have filled the FTS5 index" ); } diff --git a/packages/rust/tests/sdk.rs b/packages/rust/tests/sdk.rs index daea35784..0e9908e22 100644 --- a/packages/rust/tests/sdk.rs +++ b/packages/rust/tests/sdk.rs @@ -786,8 +786,20 @@ fn wait_file_events_after_watch_errors() { fn ddl_sqlite_rejects_errors() { let root = TempDir::new().unwrap(); let table = Table::new("t", "THIS IS NOT A CREATE TABLE", "*.txt", |_| vec![]); - let result = DirSQL::new(root.path(), vec![table]); - assert!(matches!(result, Err(dirsql::DirSqlError::Core(_)))); + let message = match DirSQL::new(root.path(), vec![table]) { + Ok(_) => panic!("a `ddl` SQLite rejects must fail the build"), + Err(err) => { + assert!( + matches!(err, dirsql::DirSqlError::TableDdl { .. }), + "got: {err}" + ); + err.to_string() + } + }; + assert!( + message.starts_with("table 't': "), + "SQLite's error must carry the config entry that caused it, got {message:?}" + ); } /// A `ddl` that runs but creates something else is caught against SQLite's @@ -804,7 +816,7 @@ fn declared_name_absent_from_the_catalog_errors() { let result = DirSQL::new(root.path(), vec![table]); assert!(matches!( result, - Err(dirsql::DirSqlError::TableNotCreated { ref name }) if name == "messages" + Err(dirsql::DirSqlError::TableNotCreated { ref name, .. }) if name == "messages" )); } From 4a0f0aa55a6d9fcb3be9ee4955839d9e2d66eb48 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:43:53 +0000 Subject: [PATCH 3/4] e2e attestation for claude/tackle-956-xzen74 --- .../python/e2e-attestations/claude-tackle-956-xzen74.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 packages/python/e2e-attestations/claude-tackle-956-xzen74.json diff --git a/packages/python/e2e-attestations/claude-tackle-956-xzen74.json b/packages/python/e2e-attestations/claude-tackle-956-xzen74.json new file mode 100644 index 000000000..a31398207 --- /dev/null +++ b/packages/python/e2e-attestations/claude-tackle-956-xzen74.json @@ -0,0 +1,7 @@ +{ + "command": "uv run python -m pytest tests/e2e/ -x -q", + "ran_at": 1787258633, + "exit_code": 0, + "commit": "9191ced6e960be0f079fd21957473441f5cd2455", + "branch": "claude/tackle-956-xzen74" +} From 82de0409754d10cd4c232b64193638d8974e229f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 20:47:34 +0000 Subject: [PATCH 4/4] e2e attestation for claude/tackle-956-xzen74 --- packages/ts/e2e-attestations/claude-tackle-956-xzen74.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 packages/ts/e2e-attestations/claude-tackle-956-xzen74.json diff --git a/packages/ts/e2e-attestations/claude-tackle-956-xzen74.json b/packages/ts/e2e-attestations/claude-tackle-956-xzen74.json new file mode 100644 index 000000000..5f5c314db --- /dev/null +++ b/packages/ts/e2e-attestations/claude-tackle-956-xzen74.json @@ -0,0 +1,7 @@ +{ + "command": "pnpm test:e2e", + "ran_at": 1787258854, + "exit_code": 0, + "commit": "4a0f0aa55a6d9fcb3be9ee4955839d9e2d66eb48", + "branch": "claude/tackle-956-xzen74" +}