diff --git a/Cargo.lock b/Cargo.lock index 0aa28f386..b868532be 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2451,6 +2451,7 @@ version = "1.0.149" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86" dependencies = [ + "indexmap", "itoa", "memchr", "serde", diff --git a/Cargo.toml b/Cargo.toml index 503e9e635..928e1feef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,9 @@ repository = "https://github.com/thekevinscott/dirsql" [workspace.dependencies] dirsql = { path = "packages/rust" } serde = { version = "1", features = ["derive"] } -serde_json = "1" +# `preserve_order`: a serialized row must keep the projection order the SELECT +# list asked for, which the default BTreeMap-backed object discards. +serde_json = { version = "1", features = ["preserve_order"] } regex = "1" thiserror = "2" tokio = { version = "1", features = ["rt", "sync"] } diff --git a/PARITY.md b/PARITY.md index db8676940..98df17d03 100644 --- a/PARITY.md +++ b/PARITY.md @@ -88,6 +88,7 @@ core, so all three SDKs change at once. **No drift.** |----------------------------|------------------------------------------------|------------------------------------------------------|---------------------------------------------------------| | Constructor | `DirSQL(root=None, *, tables=None, ignore=None, no_ignore=False, config=None, persist=False, persist_path=None, extensions=None)` | `DirSQL::builder().root(..).tables(..).ignore(..).config(..).persist(Option).extensions(..).build()` (also `DirSQL::new`/`with_ignore` shortcuts) | `new DirSQL(configPath)` or `new DirSQL({ root?, tables?, ignore?, config?, persist?, persistPath?, extensions?, noIgnore? })` + `await db.ready` | | Query (read-only; rejects non-SELECT) | `db.query(sql) -> list[dict]` | `db.query(sql) -> Result>` | `await db.query(sql) -> Record[]` (runs on libuv threadpool) | +| Query carrying the projection order | — (drift, #1173) | `db.query_ordered(sql) -> Result` (`columns` + `rows`) | — (drift, #1174) | | Start watcher | `db._start_watcher()` | `db.start_watching()` | `await db.startWatcher()` (runs on libuv threadpool) | | Poll events | `db._poll_events(ms)` | `db.poll_events(duration)` | `await db.pollEvents(ms)` (runs on libuv threadpool) | | Watch (channel/stream) | `async for event in db.watch()` (via `_async.py`) | `db.watch() -> WatchStream` (channel) | `for await (const ev of db.watch())` | diff --git a/packages/python/e2e-attestations/claude-1170-projection-order.json b/packages/python/e2e-attestations/claude-1170-projection-order.json new file mode 100644 index 000000000..5e809570e --- /dev/null +++ b/packages/python/e2e-attestations/claude-1170-projection-order.json @@ -0,0 +1,7 @@ +{ + "command": "just test-e2e", + "ran_at": 1790181270, + "exit_code": 0, + "commit": "010d9772867fcb1e1b56ce4a77eb8b0223b0870a", + "branch": "claude/1170-projection-order" +} diff --git a/packages/rust/changelog.d/2026-09-23-projection-column-order.md b/packages/rust/changelog.d/2026-09-23-projection-column-order.md new file mode 100644 index 000000000..ee2d6b23e --- /dev/null +++ b/packages/rust/changelog.d/2026-09-23-projection-column-order.md @@ -0,0 +1,6 @@ +**Fixed** Columns now render in the order the SELECT list names them, in both +the table and JSON renderings. `SELECT path, ctime FROM './'` printed +`ctime, path` because the row maps are unordered and every renderer re-derived +its columns alphabetically. `Db::query_ordered` / `DirSQL::query_ordered` +return the projection order alongside the rows, and the CLI serializes each row +against it. diff --git a/packages/rust/migrations.d/2026-09-23-projection-column-order.md b/packages/rust/migrations.d/2026-09-23-projection-column-order.md new file mode 100644 index 000000000..a223f86b6 --- /dev/null +++ b/packages/rust/migrations.d/2026-09-23-projection-column-order.md @@ -0,0 +1,35 @@ +### Result columns follow the projection order + +**Summary** + +Query results are rendered in the order the SELECT list names the columns, +where they were previously alphabetical. This affects the Rust core's CLI +(`--format table` and `--format json`) and the HTTP `POST /query` body. No +signature changed; `DirSQL::query` still returns `Vec`, and the new +`DirSQL::query_ordered` is additive. + +**Required changes** + +_None._ + +**Deprecations removed** + +_None._ + +**Behavior changes without code changes** + +| Surface | Before | After | +|---|---|---| +| `dirsql "SELECT path, ctime FROM './'" --format table` | header `ctime path` | header `path ctime` | +| `dirsql "SELECT path, ctime FROM './'" --format json` | `{"ctime":…,"path":…}` | `{"path":…,"ctime":…}` | +| `POST /query` response rows | keys sorted alphabetically | keys in projection order | + +A consumer that relied on alphabetical key order (e.g. comparing serialized +JSON byte-for-byte) must key off the column names instead. + +**Verification** + +```bash +dirsql "SELECT path, ctime FROM './' LIMIT 2" --format json +# [{"path":"a.md","ctime":1790181167},…] -- path first, as written +``` diff --git a/packages/rust/src/cli/execute.rs b/packages/rust/src/cli/execute.rs index 8291a8d97..2cb2cbe27 100644 --- a/packages/rust/src/cli/execute.rs +++ b/packages/rust/src/cli/execute.rs @@ -15,7 +15,7 @@ use serde::Deserialize; use serde_json::Value; use super::AppState; -use super::serialize::rows_to_json; +use super::serialize::result_to_json; use crate::{DirSQL, DirSqlError}; /// Why a query failed, classified independently of transport. The HTTP @@ -66,10 +66,14 @@ pub async fn execute_query( let db = require_ready(state)?; - let join = run_bounded(timeout, tokio::task::spawn_blocking(move || db.query(&sql))).await?; + let join = run_bounded( + timeout, + tokio::task::spawn_blocking(move || db.query_ordered(&sql)), + ) + .await?; match join { - Ok(Ok(rows)) => Ok(Value::Array(rows_to_json(&rows))), + Ok(Ok(result)) => Ok(Value::Array(result_to_json(&result))), Ok(Err(err)) => Err(classify_query_error(err)), Err(join_err) => Err(QueryFailure::Internal(join_err.to_string())), } diff --git a/packages/rust/src/cli/serialize.rs b/packages/rust/src/cli/serialize.rs index 0fe632c35..5ca8cbc5c 100644 --- a/packages/rust/src/cli/serialize.rs +++ b/packages/rust/src/cli/serialize.rs @@ -2,13 +2,29 @@ use serde_json::{Map, Value}; -use crate::Row; use crate::db::Value as CellValue; use crate::differ::RowEvent; use crate::row_event_flat::flatten_row_event; +use crate::{QueryResult, Row}; + +/// Serialize a query result, each object keyed in projection order. Row events +/// carry no projection, so they keep going through [`row_to_json`]. +pub(super) fn result_to_json(result: &QueryResult) -> Vec { + result + .rows + .iter() + .map(|row| ordered_row_to_json(&result.columns, row)) + .collect() +} -pub(super) fn rows_to_json(rows: &[Row]) -> Vec { - rows.iter().map(row_to_json).collect() +fn ordered_row_to_json(columns: &[String], row: &Row) -> Value { + let mut map = Map::with_capacity(columns.len()); + for column in columns { + if let Some(value) = row.get(column) { + map.insert(column.clone(), cell_to_json(value)); + } + } + Value::Object(map) } pub(super) fn row_to_json(row: &Row) -> Value { @@ -75,6 +91,35 @@ mod tests { assert_eq!(json.get("count").and_then(Value::as_i64), Some(3)); } + #[test] + fn a_result_serializes_its_rows_in_projection_order() { + let mut row: Row = HashMap::new(); + row.insert("title".into(), CellValue::Text("Hello".into())); + row.insert("count".into(), CellValue::Integer(3)); + let result = QueryResult { + columns: vec!["count".into(), "title".into()], + rows: vec![row], + }; + + let json = result_to_json(&result); + + assert_eq!(json[0].to_string(), r#"{"count":3,"title":"Hello"}"#); + } + + #[test] + fn a_column_no_row_key_matches_is_left_out() { + let mut row: Row = HashMap::new(); + row.insert("title".into(), CellValue::Text("Hello".into())); + let result = QueryResult { + columns: vec!["title".into(), "title".into()], + rows: vec![row], + }; + + let json = result_to_json(&result); + + assert_eq!(json[0].to_string(), r#"{"title":"Hello"}"#); + } + #[test] fn insert_event_emits_expected_shape() { let mut row: Row = HashMap::new(); diff --git a/packages/rust/src/cli/table.rs b/packages/rust/src/cli/table.rs index 06d7cd5d3..e9e9f8a43 100644 --- a/packages/rust/src/cli/table.rs +++ b/packages/rust/src/cli/table.rs @@ -11,7 +11,7 @@ //! swallowing the table; laying the table out to the terminal's width, and //! paging a long result, are both out of scope. -use std::collections::BTreeSet; +use std::collections::HashSet; use serde_json::Value; @@ -53,16 +53,22 @@ pub(super) fn render(rows: &[Value]) -> String { out } -/// Every column any row carries. Rows are JSON objects, so the keys arrive -/// sorted and stay in step with the JSON rendering of the same result. +/// Every column any row carries, first seen first. Rows are JSON objects +/// serialized in projection order, so that is the order the SELECT list asked +/// for -- and the same order the JSON rendering of this result prints. fn columns(rows: &[Value]) -> Vec { - let mut names = BTreeSet::new(); + let mut seen = HashSet::new(); + let mut names = Vec::new(); for row in rows { if let Value::Object(map) = row { - names.extend(map.keys().cloned()); + for key in map.keys() { + if seen.insert(key.clone()) { + names.push(key.clone()); + } + } } } - names.into_iter().collect() + names } /// One row's cells, in column order. A row missing a column renders it as @@ -242,6 +248,25 @@ mod tests { } } + #[test] + fn the_header_keeps_the_order_the_row_carries() { + // Rows arrive serialized in projection order; re-sorting them here is + // what made `SELECT size, basename` print `basename size`. + let table = rendered([row([ + ("size", Value::from(6)), + ("basename", Value::from("a.md")), + ])]); + + assert!(table.starts_with("size basename\n"), "{table:?}"); + } + + #[test] + fn a_later_rows_new_columns_come_after_the_earlier_ones() { + let table = rendered([row([("b", Value::from(1))]), row([("a", Value::from(2))])]); + + assert!(table.starts_with("b a\n"), "{table:?}"); + } + #[test] fn every_column_any_row_carries_is_shown() { // A ragged result must not silently drop the columns only some rows diff --git a/packages/rust/src/db.rs b/packages/rust/src/db.rs index ba5e463bb..c3ed2d900 100644 --- a/packages/rust/src/db.rs +++ b/packages/rust/src/db.rs @@ -244,6 +244,13 @@ pub fn ensure_internal_rows_table(conn: &Connection) -> rusqlite::Result<()> { ) } +/// One query's rows plus the column order the SELECT list asked for. +#[derive(Debug, Clone, PartialEq)] +pub struct QueryResult { + pub columns: Vec, + pub rows: Vec>, +} + pub struct Db { conn: Connection, /// The directory a path-table's glob is resolved against. `None` disables @@ -714,6 +721,14 @@ impl Db { /// internal writes (`insert_row`, delete-by-file, persist), which never go /// through `query()`, are unaffected. pub fn query(&self, sql: &str) -> Result>> { + self.query_ordered(sql).map(|result| result.rows) + } + + /// [`query`](Self::query), plus the projection order. A row is a + /// `HashMap`, so the order the SELECT list asked for survives only in + /// [`QueryResult::columns`]; a renderer that re-derives its columns from + /// row keys prints them alphabetically instead. + pub fn query_ordered(&self, sql: &str) -> Result { // Bracket the whole statement, prepare included: a worker-backed // function called on every row makes this the slowest thing dirsql // does, and the guard is what erases the counter before the caller @@ -757,7 +772,10 @@ impl Db { for row in rows { results.push(row?); } - Ok(results) + Ok(QueryResult { + columns: column_names, + rows: results, + }) } /// Prepare `sql` with the internal-table / ATTACH authorizer installed for @@ -1026,6 +1044,40 @@ mod tests { assert!(!rows[0].contains_key("_dirsql_row_index")); } + #[test] + fn query_ordered_reports_the_select_list_order() { + let db = Db::new().unwrap(); + db.create_table("docs", "CREATE TABLE docs (title TEXT, draft INTEGER)") + .unwrap(); + + let result = db.query_ordered("SELECT draft, title FROM docs").unwrap(); + + assert_eq!( + result.columns, + vec!["draft".to_string(), "title".to_string()] + ); + } + + #[test] + fn query_ordered_carries_the_same_rows_query_returns() { + let db = Db::new().unwrap(); + db.create_table("docs", "CREATE TABLE docs (title TEXT, draft INTEGER)") + .unwrap(); + let row = HashMap::from([ + ("title".into(), Value::Text("Hello".into())), + ("draft".into(), Value::Integer(0)), + ]); + db.insert_row("docs", &row, "docs/hello.md", 0).unwrap(); + + let result = db.query_ordered("SELECT title, draft FROM docs").unwrap(); + + assert_eq!( + result.rows, + db.query("SELECT title, draft FROM docs").unwrap() + ); + assert_eq!(result.rows[0]["title"], Value::Text("Hello".into())); + } + #[test] fn pragma_table_info_matches_user_ddl_exactly() { let db = Db::new().unwrap(); diff --git a/packages/rust/src/lib.rs b/packages/rust/src/lib.rs index 35426a939..2bd2645b9 100644 --- a/packages/rust/src/lib.rs +++ b/packages/rust/src/lib.rs @@ -77,7 +77,7 @@ use std::time::Duration; use thiserror::Error; pub use crate::config::ExtensionSpec as Extension; -pub use crate::db::{DbError, Value}; +pub use crate::db::{DbError, QueryResult, Value}; pub use crate::differ::RowEvent; #[doc(hidden)] pub use crate::row_event_flat::{FlatRowEvent, flatten_row_event}; @@ -404,8 +404,15 @@ impl DirSQL { /// keeps the ephemeral index consistent with the on-disk files that back /// it: mutations only happen through the watcher/indexer pipeline. pub fn query(&self, sql: &str) -> Result> { + self.query_ordered(sql).map(|result| result.rows) + } + + /// [`query`](Self::query), plus the projection order in + /// [`QueryResult::columns`]. A [`Row`] is a `HashMap`, so a caller that + /// renders columns in the order the user asked for needs this instead. + pub fn query_ordered(&self, sql: &str) -> Result { let db = self.inner.db.lock().map_err(DirSqlError::lock)?; - db.query(sql).map_err(map_db_error) + db.query_ordered(sql).map_err(map_db_error) } /// Lazily create the filesystem watcher. Idempotent; subsequent calls are diff --git a/packages/rust/tests/column_order.rs b/packages/rust/tests/column_order.rs new file mode 100644 index 000000000..7144ad79b --- /dev/null +++ b/packages/rust/tests/column_order.rs @@ -0,0 +1,75 @@ +//! Integration tests for column order: a result carries the order the SELECT +//! list asked for, not the alphabetical order a `HashMap`/`BTreeMap` falls +//! into. Exercised through the `/query` transport, where the serialized key +//! order is observable. +//! +//! Gated behind `--features cli` — the server lives in `src/cli/`. + +#![cfg(feature = "cli")] + +use std::fs; + +use dirsql::DirSQL; +use dirsql::cli::{ServerConfig, ServerHandle, serve}; +use tempfile::TempDir; + +fn fixture() -> TempDir { + let root = TempDir::new().unwrap(); + fs::write(root.path().join("a.md"), "alpha\n").unwrap(); + root +} + +async fn spawn(db: DirSQL) -> ServerHandle { + serve(ServerConfig::ephemeral(), db) + .await + .expect("server should bind on an ephemeral port") +} + +async fn query_body(handle: &ServerHandle, sql: &str) -> String { + reqwest::Client::new() + .post(format!("http://{}/query", handle.local_addr())) + .json(&serde_json::json!({ "sql": sql })) + .send() + .await + .expect("POST /query failed") + .text() + .await + .expect("response body must be text") +} + +fn key_positions(body: &str, keys: [&str; 2]) -> [usize; 2] { + keys.map(|key| { + body.find(&format!("\"{key}\"")) + .unwrap_or_else(|| panic!("`{key}` missing from response: {body}")) + }) +} + +#[tokio::test] +async fn serialized_rows_follow_the_projection_order() { + let root = fixture(); + let db = DirSQL::new(root.path(), vec![]).unwrap(); + let handle = spawn(db).await; + + let body = query_body(&handle, "SELECT size, basename FROM './'").await; + let [size, basename] = key_positions(&body, ["size", "basename"]); + + assert!( + size < basename, + "expected `size` before `basename`, got: {body}" + ); +} + +#[tokio::test] +async fn reversing_the_select_list_reverses_the_serialized_order() { + let root = fixture(); + let db = DirSQL::new(root.path(), vec![]).unwrap(); + let handle = spawn(db).await; + + let body = query_body(&handle, "SELECT basename, size FROM './'").await; + let [size, basename] = key_positions(&body, ["size", "basename"]); + + assert!( + basename < size, + "expected `basename` before `size`, got: {body}" + ); +} diff --git a/packages/rust/tests/column_order_e2e.rs b/packages/rust/tests/column_order_e2e.rs new file mode 100644 index 000000000..2215690b1 --- /dev/null +++ b/packages/rust/tests/column_order_e2e.rs @@ -0,0 +1,81 @@ +//! End-to-end tests for column order: the real `dirsql` binary must print +//! columns in the order the SELECT list names them, in both renderings. +//! +//! Nothing is mocked (real process, real filesystem, real SQLite). +//! +//! Gated behind `--features cli`: the `dirsql` bin target is +//! `required-features = ["cli"]`. + +#![cfg(feature = "cli")] + +use std::process::Output; + +use assert_cmd::prelude::*; +use tempfile::TempDir; + +fn fixture() -> TempDir { + let root = TempDir::new().unwrap(); + std::fs::write(root.path().join("a.md"), "alpha\n").unwrap(); + root +} + +fn query(root: &TempDir, sql: &str, format: &str) -> Output { + std::process::Command::cargo_bin("dirsql") + .expect("binary must exist") + .arg(sql) + .args(["--format", format]) + .current_dir(root.path()) + .output() + .expect("spawning `dirsql` failed") +} + +fn stdout(out: &Output) -> String { + assert!( + out.status.success(), + "expected success, stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8(out.stdout.clone()).expect("stdout must be UTF-8") +} + +fn header(out: &Output) -> Vec { + stdout(out) + .lines() + .next() + .expect("table must have a header line") + .split_whitespace() + .map(str::to_string) + .collect() +} + +#[test] +fn the_table_header_follows_the_select_list() { + let root = fixture(); + let out = query(&root, "SELECT size, basename FROM './'", "table"); + + assert_eq!(header(&out), vec!["size", "basename"]); +} + +#[test] +fn reversing_the_select_list_reverses_the_table_header() { + let root = fixture(); + let out = query(&root, "SELECT basename, size FROM './'", "table"); + + assert_eq!(header(&out), vec!["basename", "size"]); +} + +#[test] +fn json_keys_follow_the_select_list() { + let root = fixture(); + let out = query(&root, "SELECT size, basename FROM './'", "json"); + let text = stdout(&out); + + let size = text.find("\"size\"").expect("`size` must be present"); + let basename = text + .find("\"basename\"") + .expect("`basename` must be present"); + assert!( + size < basename, + "expected `size` before `basename`, got: {text}" + ); +} diff --git a/packages/ts/e2e-attestations/claude-1170-projection-order.json b/packages/ts/e2e-attestations/claude-1170-projection-order.json new file mode 100644 index 000000000..971bb5791 --- /dev/null +++ b/packages/ts/e2e-attestations/claude-1170-projection-order.json @@ -0,0 +1,7 @@ +{ + "command": "pnpm test:e2e", + "ran_at": 1790181338, + "exit_code": 0, + "commit": "512caf0eacef881005d91779876567ca79e5d5ba", + "branch": "claude/1170-projection-order" +}