From c4cf5de5e60c53910bb4cf667bd302f78c60f2a1 Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 11 Sep 2026 15:35:51 +0200 Subject: [PATCH 1/3] fix: rowid/_rowid_/oid resolve in a SELECT projection, not just WHERE (#708) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scope::resolve gains a rowid pseudo-column, consulted only after a declared column of the same name fails to match (so shadowing and INTEGER PRIMARY KEY's existing alias case are unaffected), and never consulted for WITHOUT ROWID tables. Bare rowid/_rowid_/oid now works in a result-list projection, in ORDER BY, and combined with an alias and WHERE in the same statement — the last of which required compile_sorted_scan's pass 1 to materialize the pseudo-column into the sorted record so pass 2's post-ORDER-BY pseudo cursor has something to read back, instead of re-issuing Rowid against a cursor that doesn't support it. spend: roughly matched the small-medium estimate; the ORDER BY+alias combination surfaced a real pseudo-cursor bug beyond the original WIP, adding one extra iteration to close. --- CHANGELOG.md | 15 ++ Cargo.toml | 4 + src/codegen.rs | 16 +- src/codegen/expr.rs | 2 +- src/codegen/expr/cond.rs | 22 +++ src/codegen/expr/value.rs | 6 +- src/codegen/select.rs | 2 +- src/codegen/select/limit_scan.rs | 38 ++++ src/codegen/select/order_by.rs | 14 ++ src/codegen/select/projection.rs | 18 +- tests/unit/rowid_projection_test.rs | 275 ++++++++++++++++++++++++++++ 11 files changed, 402 insertions(+), 10 deletions(-) create mode 100644 tests/unit/rowid_projection_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a35d73e..1c10fe20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep **Versioning policy:** one minor version per completed plan phase — the version number tells the plan's story, sub-steps stay inside a phase. V1 (READ CORE) = 0.1.0 through 0.4.0. *(History note: internal iterations briefly numbered 0.4.0–0.6.0 were renumbered into the phase scheme on 14 Aug 2026, before any tag or publication of those versions existed.)* +## [Unreleased] + +### Fixed + +- `rowid`/`_rowid_`/`oid` resolved in a `WHERE` clause but not in a + result-list projection (`Scope::resolve` had no pseudo-column + awareness, unlike the `UPDATE`/`DELETE` seek path's + `is_rowid_reference`). Now resolves as a pseudo-column on rowid + tables — a declared column of that name still shadows it, `INTEGER + PRIMARY KEY` remains the existing alias case, and `WITHOUT ROWID` + tables reject it with the same unknown-column error the oracle gives. + Also fixes the same reference combined with `ORDER BY` and an alias + in one statement, where the post-sort pseudo cursor previously tried + to re-issue `Rowid` against itself (#708). + ## [0.18.10] - 2026-08-31 ### Fixed diff --git a/Cargo.toml b/Cargo.toml index 99457437..edda6891 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -140,6 +140,10 @@ path = "tests/unit/codegen_expr_test.rs" name = "codegen_select" path = "tests/unit/codegen_select_test.rs" +[[test]] +name = "rowid_projection" +path = "tests/unit/rowid_projection_test.rs" + [[test]] name = "codegen_insert" path = "tests/unit/codegen_insert_test.rs" diff --git a/src/codegen.rs b/src/codegen.rs index 177185c8..e4807e48 100644 --- a/src/codegen.rs +++ b/src/codegen.rs @@ -561,7 +561,9 @@ impl Scope { .ok_or_else(|| select::CodegenError::UnknownColumn { name: name.to_string(), })?; - let idx = expr::column_index(&binding.schema, name).unwrap_or(0); + let idx = expr::column_index(&binding.schema, name) + .or_else(|| expr::rowid_pseudo_column_index(&binding.schema, name)) + .unwrap_or(0); Ok((binding.cursor, idx, &binding.schema, binding.forced_null)) } @@ -593,16 +595,18 @@ impl Scope { .ok_or_else(|| select::CodegenError::UnknownColumn { name: format!("{table}.{name}"), })?; - expr::column_index(&binding.schema, name).ok_or_else(|| { - select::CodegenError::UnknownColumn { + expr::column_index(&binding.schema, name) + .or_else(|| expr::rowid_pseudo_column_index(&binding.schema, name)) + .ok_or_else(|| select::CodegenError::UnknownColumn { name: format!("{table}.{name}"), - } - })?; + })?; return Ok(idx); } let mut found: Option = None; for (i, binding) in self.tables.iter().enumerate() { - if expr::column_index(&binding.schema, name).is_some() { + if expr::column_index(&binding.schema, name).is_some() + || expr::rowid_pseudo_column_index(&binding.schema, name).is_some() + { if found.is_some() { return Err(select::CodegenError::AmbiguousColumn { name: name.to_string(), diff --git a/src/codegen/expr.rs b/src/codegen/expr.rs index 14e95ee1..35541b34 100644 --- a/src/codegen/expr.rs +++ b/src/codegen/expr.rs @@ -24,7 +24,7 @@ mod cond; mod value; -pub(crate) use cond::{column_index, compile_cond, ensure_label}; +pub(crate) use cond::{column_index, compile_cond, ensure_label, rowid_pseudo_column_index}; pub(crate) use value::{ collation_of, compile_value, emit_column_read, expr_affinity, expr_collation, is_aggregate_call, }; diff --git a/src/codegen/expr/cond.rs b/src/codegen/expr/cond.rs index a439442c..510bc17e 100644 --- a/src/codegen/expr/cond.rs +++ b/src/codegen/expr/cond.rs @@ -23,6 +23,28 @@ pub(crate) fn column_index(schema: &TableSchema, name: &str) -> Option { .position(|c| c.eq_ignore_ascii_case(name)) } +/// #708: `rowid`/`_rowid_`/`oid` as a pseudo-column on a rowid table — +/// resolves to a sentinel index one past `schema.columns`'s end, which +/// [`super::value::emit_column_read`] recognizes and reads via +/// `Opcode::Rowid` off the live cursor, same as a genuine +/// `rowid_alias` hit. Only consulted when [`column_index`] has already +/// failed, so a declared column of that name always wins (shadowing). +/// `WITHOUT ROWID` tables have no rowid at all, so this always misses +/// for them, and callers fall through to the ordinary "unknown column" +/// error — matching the oracle. +pub(crate) fn rowid_pseudo_column_index(schema: &TableSchema, name: &str) -> Option { + if schema.without_rowid { + return None; + } + if name.eq_ignore_ascii_case("rowid") + || name.eq_ignore_ascii_case("_rowid_") + || name.eq_ignore_ascii_case("oid") + { + return Some(schema.columns.len()); + } + None +} + /// #581: a rough, static (no `ANALYZE` data needed) cost class for an /// expression, used only to order `AND`/`OR` operands cheapest-first so /// short-circuit evaluation skips the pricier side more often — never diff --git a/src/codegen/expr/value.rs b/src/codegen/expr/value.rs index 75a779a5..f2e333bf 100644 --- a/src/codegen/expr/value.rs +++ b/src/codegen/expr/value.rs @@ -27,7 +27,11 @@ pub(crate) fn emit_column_read( idx: usize, dest: i32, ) -> Result<(), CodegenError> { - if schema.rowid_alias == Some(idx) { + if schema.rowid_alias == Some(idx) || idx == schema.columns.len() { + // `idx == schema.columns.len()` is #708's `rowid`/`_rowid_`/`oid` + // pseudo-column sentinel (see `expr::rowid_pseudo_column_index`) + // — one past the last declared column, so it never collides + // with a real index. em.emit(Instruction::new(Opcode::Rowid, cursor, dest, 0)); return Ok(()); } diff --git a/src/codegen/select.rs b/src/codegen/select.rs index 70279ca5..ebb38b04 100644 --- a/src/codegen/select.rs +++ b/src/codegen/select.rs @@ -17,7 +17,7 @@ use crate::codegen::expr::{ collation_of, column_index, compile_cond, compile_value, emit_column_read, expr_affinity, - expr_collation, is_aggregate_call, + expr_collation, is_aggregate_call, rowid_pseudo_column_index, }; use crate::codegen::{ p4_coll_seq, CondTargets, Emitter, Label, RegAlloc, Scope, TableBinding, Target, diff --git a/src/codegen/select/limit_scan.rs b/src/codegen/select/limit_scan.rs index 720d5b73..9bad78be 100644 --- a/src/codegen/select/limit_scan.rs +++ b/src/codegen/select/limit_scan.rs @@ -948,6 +948,32 @@ where Ok(()) } +/// #708: does `select` project a bare `rowid`/`_rowid_`/`oid` +/// pseudo-column reference (not a declared column, and not buried in a +/// compound expression — same narrow scope as the existing +/// `rowid_alias` special case in `projection.rs`)? If so, +/// `compile_sorted_scan`'s pass 1 must materialize it into the sorted +/// record — it has no field there otherwise, since pass 1's +/// schema-column block only ever iterates `schema.columns`. +fn select_projects_rowid_pseudo_column(select: &Select, schema: &TableSchema) -> bool { + select.columns.iter().any(|col| match col { + ResultColumn::Expr { + expr: + Expr { + kind: + ExprKind::Column { + name, + table: None, + catalog: None, + }, + .. + }, + .. + } => crate::codegen::expr::rowid_pseudo_column_index(schema, name).is_some(), + _ => false, + }) +} + #[allow(clippy::too_many_arguments)] pub(super) fn compile_sorted_scan( em: &mut Emitter, @@ -1075,6 +1101,18 @@ where catalog, )?; + // #708: materialize the bare rowid pseudo-column, if projected, + // immediately after the schema-column block — landing at record + // field index `schema.columns.len()`, the same sentinel + // `expr::rowid_pseudo_column_index` hands out, so pass 2's + // `projection.rs` special case can read it back with a plain + // `Column` op against the post-sort pseudo cursor instead of + // re-issuing `Rowid` (which only a real table cursor supports). + if !schema.without_rowid && select_projects_rowid_pseudo_column(select, schema) { + let r = reg.alloc(); + em.emit(Instruction::new(Opcode::Rowid, cursors.table, r, 0)); + } + // Compute every genuine-expression sort key into its own register, // appended after the schema-column block. A key's final register // need not be the highest one its expression allocates (e.g. `CASE` diff --git a/src/codegen/select/order_by.rs b/src/codegen/select/order_by.rs index c906bfd1..4807a734 100644 --- a/src/codegen/select/order_by.rs +++ b/src/codegen/select/order_by.rs @@ -146,6 +146,17 @@ pub(super) fn order_by_target_for_expr( schema: &TableSchema, ) -> Result { match &expr.kind { + // #708: `rowid`/`_rowid_`/`oid` has no schema column index to + // give `OrderByTarget::Column` (its "index" is the sentinel + // `emit_column_read` recognizes against a *live* cursor, which + // this target's consumers don't all have) — routing it through + // `Expr` instead reuses ordinary expression compilation + // (`Scope::resolve` already knows the pseudo-column). + ExprKind::Column { + table: None, name, .. + } if rowid_pseudo_column_index(schema, name).is_some() => { + Ok(OrderByTarget::Expr(expr.clone())) + } ExprKind::Column { table: None, name, .. } => column_index(schema, name) @@ -189,6 +200,9 @@ pub(super) fn resolve_order_by_target( { return order_by_target_for_expr(&entry.expr, schema); } + if rowid_pseudo_column_index(schema, name).is_some() { + return Ok(OrderByTarget::Expr(expr.clone())); + } column_index(schema, name) .map(OrderByTarget::Column) .ok_or_else(|| CodegenError::UnknownColumn { name: name.clone() }) diff --git a/src/codegen/select/projection.rs b/src/codegen/select/projection.rs index 7db24e7a..82436b08 100644 --- a/src/codegen/select/projection.rs +++ b/src/codegen/select/projection.rs @@ -119,6 +119,15 @@ pub(super) fn compile_row_values( // through to `compile_value`, matching this crate's // existing register-reuse limitations for compound // result-column expressions. + // + // #708's bare `rowid`/`_rowid_`/`oid` pseudo-column + // (not a declared column at all) needs the identical + // treatment: `compile_sorted_scan`'s pass 1 materializes + // it into the sorted record right after the schema + // columns block, at position `schema.columns.len()` — + // matching `rowid_pseudo_column_index`'s sentinel — so + // pass 2 reads it back with a plain `Column` op instead + // of re-issuing `Rowid` against the pseudo cursor. if let ExprKind::Column { name, table: None, @@ -128,7 +137,14 @@ pub(super) fn compile_row_values( let pseudo_rowid_idx = pseudo .then(|| column_index(schema, name)) .flatten() - .filter(|idx| schema.rowid_alias == Some(*idx)); + .filter(|idx| schema.rowid_alias == Some(*idx)) + .or_else(|| { + pseudo + .then(|| { + crate::codegen::expr::rowid_pseudo_column_index(schema, name) + }) + .flatten() + }); if let Some(idx) = pseudo_rowid_idx { let r = reg.alloc(); em.emit(Instruction::new( diff --git a/tests/unit/rowid_projection_test.rs b/tests/unit/rowid_projection_test.rs new file mode 100644 index 00000000..c08a0b89 --- /dev/null +++ b/tests/unit/rowid_projection_test.rs @@ -0,0 +1,275 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +//! #708: `rowid`/`_rowid_`/`oid` were resolvable in a `WHERE` clause +//! (`is_rowid_reference`'s seek fast path) but not in a projection — +//! `Scope::resolve` had no pseudo-column awareness. Oracle-diffed +//! against the pinned 3.53.4 `sqlite3`, reusing +//! `tests/unit/codegen_select_test.rs`'s scratch-db-plus-oracle +//! pattern, but through the real `oracle::pinned_oracle()` (never the +//! system `/usr/bin/sqlite3`). + +#[path = "../corpus/oracle.rs"] +#[allow(dead_code)] +mod oracle; + +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::rc::Rc; + +use oracle::pinned_oracle; +use sqlite_rs::codegen::compile_select; +use sqlite_rs::header::DatabaseHeader; +use sqlite_rs::parser::{parse_select, ParseOutcome}; +use sqlite_rs::record::Value; +use sqlite_rs::schema::TableSchema; +use sqlite_rs::vdbe::execute_with_db; +use sqlite_rs::vfs::{UnixVfs, Vfs, VfsPageSource}; + +fn scratch_db(label: &str, oracle: &Path, setup_sql: &str) -> PathBuf { + let path = std::env::temp_dir().join(format!( + "sqlite_rs_rowid_projection_test_{}_{}.db", + std::process::id(), + label + )); + std::fs::remove_file(&path).ok(); + let status = Command::new(oracle) + .arg(&path) + .arg(setup_sql) + .status() + .expect("creating scratch fixture db"); + assert!(status.success()); + path +} + +fn our_rows(path: &Path, schema: &TableSchema, sql: &str) -> Result>, String> { + let select = match parse_select(sql) { + ParseOutcome::Accepted(s) => *s, + other => return Err(format!("parser rejected {sql:?}: {other:?}")), + }; + let program = compile_select(&select, schema).map_err(|e| format!("{e:?}"))?; + let vfs = UnixVfs; + let file = vfs.open_read(path).unwrap(); + let mut header_buf = [0u8; 100]; + file.read_at(&mut header_buf, 0).unwrap(); + let header = DatabaseHeader::parse(&header_buf).unwrap(); + let source = VfsPageSource::open(&vfs, path, header.page_size).unwrap(); + execute_with_db(&program, Rc::new(source), header).map_err(|e| format!("{e:?}")) +} + +fn oracle_rows(oracle: &Path, db: &Path, sql: &str) -> Vec> { + let output = Command::new(oracle) + .arg("-readonly") + .arg("-separator") + .arg("\u{1f}") + .arg(db) + .arg(sql) + .output() + .expect("invoking sqlite3 oracle"); + String::from_utf8_lossy(&output.stdout) + .lines() + .map(|l| l.split('\u{1f}').map(str::to_string).collect()) + .collect() +} + +fn value_to_oracle_text(v: &Value) -> String { + match v { + Value::Null => String::new(), + Value::Integer(i) => i.to_string(), + Value::Real(r) => { + if r.fract() == 0.0 { + format!("{r:.1}") + } else { + r.to_string() + } + } + Value::Text(s) => s.to_string(), + Value::Blob(_) => "".to_string(), + } +} + +fn assert_matches_oracle(oracle: &Path, db: &Path, schema: &TableSchema, sql: &str) { + let ours = our_rows(db, schema, sql).unwrap_or_else(|e| panic!("compiling {sql:?}: {e}")); + let ours_text: Vec> = ours + .iter() + .map(|row| row.iter().map(value_to_oracle_text).collect()) + .collect(); + let expected = oracle_rows(oracle, db, sql); + assert_eq!(ours_text, expected, "mismatch for {sql:?}"); +} + +fn rowid_table_schema() -> TableSchema { + TableSchema { + unresolved_autoindex: false, + name: "t".to_string(), + root_page: 2, + columns: vec!["a".to_string(), "v".to_string()], + column_types: vec!["INTEGER".to_string(), "TEXT".to_string()], + column_collations: vec![], + without_rowid: false, + strict: false, + is_virtual: false, + sql: String::new(), + indexes: vec![], + rowid_alias: None, + } + .with_computed_rowid_alias() +} + +/// The core defect: `rowid` in a projection alongside real columns. +/// Also exercises gaps left by a delete (#708's acceptance criteria). +#[test] +fn rowid_selectable_in_projection_including_after_deletes() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "basic", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); \ + INSERT INTO t VALUES (1, 'aa'), (2, 'bb'), (3, 'cc'), (4, 'dd'); \ + DELETE FROM t WHERE a = 2;", + ); + let schema = rowid_table_schema(); + assert_matches_oracle(&oracle, &db, &schema, "SELECT rowid, a, v FROM t"); + assert_matches_oracle(&oracle, &db, &schema, "SELECT _rowid_, a FROM t"); + assert_matches_oracle(&oracle, &db, &schema, "SELECT oid FROM t"); +} + +/// `rowid` in a projection, WHERE and ORDER BY together, plus an +/// alias, in the same statement. +#[test] +fn rowid_works_in_where_order_by_and_alias_together() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "combo", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); \ + INSERT INTO t VALUES (1, 'aa'), (2, 'bb'), (3, 'cc');", + ); + let schema = rowid_table_schema(); + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT rowid AS rid, a FROM t WHERE rowid > 1 ORDER BY rowid DESC", + ); +} + +/// Shadowing: a declared column named `rowid` wins over the +/// pseudo-column. +#[test] +fn declared_rowid_column_shadows_the_pseudo_column() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "shadow", + &oracle, + "CREATE TABLE t(rowid TEXT, v INTEGER); \ + INSERT INTO t VALUES ('x', 1), ('y', 2);", + ); + let schema = TableSchema { + unresolved_autoindex: false, + name: "t".to_string(), + root_page: 2, + columns: vec!["rowid".to_string(), "v".to_string()], + column_types: vec!["TEXT".to_string(), "INTEGER".to_string()], + column_collations: vec![], + without_rowid: false, + strict: false, + is_virtual: false, + sql: String::new(), + indexes: vec![], + rowid_alias: None, + } + .with_computed_rowid_alias(); + assert_matches_oracle(&oracle, &db, &schema, "SELECT rowid, v FROM t"); +} + +/// `INTEGER PRIMARY KEY` tables: `rowid` and the alias column must +/// agree. +#[test] +fn integer_primary_key_alias_and_rowid_agree() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "ipk", + &oracle, + "CREATE TABLE t(id INTEGER PRIMARY KEY, v TEXT); \ + INSERT INTO t VALUES (5, 'aa'), (9, 'bb');", + ); + let schema = TableSchema { + unresolved_autoindex: false, + name: "t".to_string(), + root_page: 2, + columns: vec!["id".to_string(), "v".to_string()], + column_types: vec!["INTEGER".to_string(), "TEXT".to_string()], + column_collations: vec![], + without_rowid: false, + strict: false, + is_virtual: false, + sql: "CREATE TABLE t(id INTEGER PRIMARY KEY, v TEXT)".to_string(), + indexes: vec![], + rowid_alias: None, + } + .with_computed_rowid_alias(); + assert_matches_oracle(&oracle, &db, &schema, "SELECT rowid, id, v FROM t"); +} + +/// `WITHOUT ROWID` tables have no rowid at all — must be rejected the +/// same way the oracle rejects it (an unknown-column error), not +/// silently produce a number. +#[test] +fn without_rowid_table_rejects_rowid_reference() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "without_rowid", + &oracle, + "CREATE TABLE t(a INTEGER PRIMARY KEY, v TEXT) WITHOUT ROWID; \ + INSERT INTO t VALUES (1, 'aa');", + ); + let oracle_out = Command::new(&oracle) + .arg("-readonly") + .arg(&db) + .arg("SELECT rowid FROM t") + .output() + .expect("invoking sqlite3 oracle"); + assert!( + !oracle_out.status.success(), + "expected the oracle itself to reject rowid on a WITHOUT ROWID table" + ); + + let schema = TableSchema { + unresolved_autoindex: false, + name: "t".to_string(), + root_page: 2, + columns: vec!["a".to_string(), "v".to_string()], + column_types: vec!["INTEGER".to_string(), "TEXT".to_string()], + column_collations: vec![], + without_rowid: true, + strict: false, + is_virtual: false, + sql: "CREATE TABLE t(a INTEGER PRIMARY KEY, v TEXT) WITHOUT ROWID".to_string(), + indexes: vec![], + rowid_alias: None, + } + .with_computed_rowid_alias(); + let err = our_rows(&db, &schema, "SELECT rowid FROM t") + .expect_err("rowid on a WITHOUT ROWID table must be rejected"); + assert!( + err.contains("UnknownColumn"), + "expected an unknown-column error, got: {err}" + ); +} From 6ea401cf663e756f8a07a82ded70e1d224ef27db Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 11 Sep 2026 17:46:41 +0200 Subject: [PATCH 2/3] fix: GROUP BY rowid compiles and matches the oracle (#708) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #718 taught Scope::resolve the rowid/_rowid_/oid pseudo-column and fixed the post-sort pseudo cursor for ORDER BY (materialize once in compile_sorted_scan's pass 1, read back with Column in pass 2), but never extended that treatment to the hash-aggregate path: `GROUP BY rowid` regressed from a clean "unknown column" compile-time rejection on main to a runtime "Rowid: cursor slot 2 is a pseudo cursor" error. The live GROUP BY path is compile_grouped_scan's sort-then-group strategy (src/codegen/select/aggregate.rs) — the six-opcode HashAgg family in hash.rs/vdbe/hash_agg.rs is unwired from dispatch (#631) and never reached, so it only needed a call-site signature update to keep building. Root cause: order_by_target_for_expr already routes a bare rowid reference to OrderByTarget::Expr, and pass 1 already materializes it into the sort record as a GROUP BY sort key — but pass 2 recomputed the group's current-row key via compile_value against the pass-2 pseudo cursor instead of reading the field pass 1 had already written, and flush_group's synthetic per-group record had no field for it at all (so even a fixed key comparison would still have projected NULL for `SELECT rowid`). Fix, reusing #718's materialize-then-read-with-Column mechanism rather than inventing a new one: - compile_grouped_scan's pass 1 materializes the bare rowid pseudo-column once (real Opcode::Rowid against the live table cursor) when GROUP BY, a result column, or HAVING references it. - Pass 2 reads that field back with Opcode::Column for the group-key comparison, and snapshots it (same "arbitrary row" shape as snapshot_regs) for flush_group. - flush_group gains an optional rowid_reg parameter: when present, its synthetic per-group record gets a real "__rowid" field, and a new substitute_rowid_pseudo_column rewrites bare rowid/_rowid_/oid references (respecting shadowing) to it before the existing substitute_aggregates rewrite, so ordinary compile_row_values/ compile_cond resolve it with no further special-casing. WITHOUT ROWID and GROUP BY + ORDER BY (already rejected regardless of rowid) are unaffected. Found and left alone as pre-existing, out of scope: a table with a real declared column literally named "rowid" already projected NULL instead of its value in any GROUP BY/ORDER BY pseudo-cursor re-projection before this ticket (a #718 shadowing gap in projection.rs's compile_row_values, confirmed present on the parent branch with an unrelated reproduction) — not touched here since it's outside "GROUP BY rowid" and pre-dates this ticket. Tests: tests/unit/rowid_projection_test.rs gains 5 oracle-diffed cases (base GROUP BY rowid incl. gaps and all three spellings, HAVING with mixed spellings, a control GROUP BY on a real column, GROUP BY + ORDER BY's clean Unsupported rejection, and WITHOUT ROWID's clean UnknownColumn rejection). spend: roughly matched a small follow-up estimate; one extra pass to find and fill flush_group's missing rowid snapshot read after the initial group-key fix left SELECT rowid still projecting NULL. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 7 +- src/codegen/select/aggregate.rs | 140 ++++++++++++++++++++ src/codegen/select/aggregate/accum.rs | 116 +++++++++++++++-- src/codegen/select/aggregate/hash.rs | 1 + tests/unit/rowid_projection_test.rs | 180 ++++++++++++++++++++++++++ 5 files changed, 431 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c10fe20..a51e0f3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,12 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep tables reject it with the same unknown-column error the oracle gives. Also fixes the same reference combined with `ORDER BY` and an alias in one statement, where the post-sort pseudo cursor previously tried - to re-issue `Rowid` against itself (#708). + to re-issue `Rowid` against itself. `GROUP BY rowid` (and `_rowid_`/ + `oid`) had the identical pseudo-cursor problem in + `compile_grouped_scan`'s sort-then-group pass 2 — both the group-key + comparison and a bare `rowid` in the result list or `HAVING` now read + back a materialized field instead of re-issuing `Rowid` against a + cursor that can't answer it (#708). ## [0.18.10] - 2026-08-31 diff --git a/src/codegen/select/aggregate.rs b/src/codegen/select/aggregate.rs index 605b3690..43908c5f 100644 --- a/src/codegen/select/aggregate.rs +++ b/src/codegen/select/aggregate.rs @@ -431,6 +431,7 @@ where schema, catalog, &snapshot_regs, + None, &agg_slots, limit.as_ref(), end_label, @@ -682,6 +683,72 @@ fn walk_expr_for_column_refs( } } +/// #708 follow-up: is `expr` exactly a bare, unshadowed +/// `rowid`/`_rowid_`/`oid` reference — the narrow shape +/// [`order_by_target_for_expr`] already routes to `OrderByTarget::Expr` +/// since it has no real schema column index of its own. `schema` is the +/// original table schema (not [`compact_schema`]'s narrowed one), so a +/// declared column named `rowid` correctly wins (shadowing) via +/// `column_index` before `rowid_pseudo_column_index` is even consulted. +fn is_bare_rowid_pseudo_column(expr: &Expr, schema: &TableSchema) -> bool { + matches!( + &expr.kind, + ExprKind::Column { + name, + table: None, + catalog: None, + } if crate::codegen::expr::column_index(schema, name).is_none() + && crate::codegen::expr::rowid_pseudo_column_index(schema, name).is_some() + ) +} + +/// Same bare-reference check as [`is_bare_rowid_pseudo_column`], but +/// walking through `Paren`/`Collate`/`Unary`/`Binary` wrappers (matching +/// `accum::substitute_rowid_pseudo_column`'s own traversal) to answer +/// whether `expr` references the pseudo-column *anywhere*, e.g. `HAVING +/// rowid > 1`'s `rowid` is nested one level inside a `Binary`. +fn expr_references_rowid_pseudo_column(expr: &Expr, schema: &TableSchema) -> bool { + if is_bare_rowid_pseudo_column(expr, schema) { + return true; + } + match &expr.kind { + ExprKind::Paren(inner) + | ExprKind::Collate { expr: inner, .. } + | ExprKind::Unary { expr: inner, .. } => expr_references_rowid_pseudo_column(inner, schema), + ExprKind::Binary { lhs, rhs, .. } => { + expr_references_rowid_pseudo_column(lhs, schema) + || expr_references_rowid_pseudo_column(rhs, schema) + } + _ => false, + } +} + +/// #708 follow-up: does [`compile_grouped_scan`]'s pass 1 need to +/// materialize the bare rowid pseudo-column into the sort record? Needed +/// whenever `GROUP BY`, a result column, or `HAVING` references it — +/// otherwise there is no field for it anywhere in the compacted record +/// (unlike a real `schema` column, `needed_order` never contains it). +/// `WITHOUT ROWID` never needs it: there is no rowid to materialize, and +/// [`crate::codegen::expr::rowid_pseudo_column_index`] already reports +/// unknown-column for it, same as main. +fn grouped_scan_needs_rowid_field(select: &Select, schema: &TableSchema) -> bool { + if schema.without_rowid { + return false; + } + select + .group_by + .iter() + .any(|e| expr_references_rowid_pseudo_column(e, schema)) + || select.columns.iter().any(|col| match col { + ResultColumn::Expr { expr, .. } => expr_references_rowid_pseudo_column(expr, schema), + ResultColumn::Star | ResultColumn::TableStar { .. } => false, + }) + || select + .having + .as_ref() + .is_some_and(|h| expr_references_rowid_pseudo_column(h, schema)) +} + /// #239: `GROUP BY` / `HAVING`. Strategy mirrors real SQLite's /// sort-then-group `select.c` shape rather than a hash table, since the /// `Sorter*` opcode family this compiler already has for `ORDER BY` @@ -778,6 +845,11 @@ where if let Some(outer) = outer_scope { pseudo_scope = pseudo_scope.with_outer(outer.clone()); } + // #708 follow-up: whether a bare rowid/`_rowid_`/`oid` pseudo-column + // reference anywhere in `GROUP BY`/`select.columns`/`HAVING` needs a + // materialized field in the sort record — see + // `grouped_scan_needs_rowid_field`'s doc for why. + let needs_rowid_field = grouped_scan_needs_rowid_field(select, schema); // Pass 1: buffer every WHERE-matching row's needed column values // (#665: only the columns `needed_order` names, not the full row), @@ -810,6 +882,21 @@ where } let first = compile_row_values_compact(em, reg, schema, &needed_order, cursors.table)?; + // #708 follow-up: materialize the bare rowid pseudo-column right + // after the compacted schema-column block — `cursors.table` is a + // real table cursor here, so `Opcode::Rowid` is valid (unlike pass + // 2's `cursors.pseudo`, which can't answer it at all). Its field + // position (`needed_order.len()`, i.e. right after `first`) is + // reused below by both a rowid `GROUP BY` key and, in pass 2, by the + // group's snapshot/key re-reads — the same materialize-once, + // read-back-with-`Column` shape `compile_sorted_scan` established + // for `ORDER BY rowid` (#718). + let rowid_field_index = needs_rowid_field.then(|| { + let r = reg.alloc(); + em.emit(Instruction::new(Opcode::Rowid, cursors.table, r, 0)); + usize::try_from(r.saturating_sub(first)).unwrap_or(0) + }); + let mut sort_keys = Vec::with_capacity(group_targets.len()); for (expr, target) in select.group_by.iter().zip(&group_targets) { let index = match target { @@ -817,6 +904,12 @@ where // `select.group_by` references (`columns_needed_for_projection`), // so `idx` always has a compacted position. OrderByTarget::Column(idx) => compact_of.get(*idx).copied().flatten().unwrap_or(0), + OrderByTarget::Expr(e) if is_bare_rowid_pseudo_column(e, schema) => { + // Already materialized above — reuse it rather than + // recompiling (which would emit a second, redundant + // `Rowid` read against the same real cursor). + rowid_field_index.unwrap_or(0) + } OrderByTarget::Expr(e) => { let r = compile_value(em, reg, &table_scope, e)?; usize::try_from(r.saturating_sub(first)).unwrap_or(0) @@ -892,6 +985,15 @@ where for &r in &snapshot_regs { em.emit(Instruction::new(Opcode::Null, 0, r, 0)); } + // #708 follow-up: the bare rowid pseudo-column's own persistent + // "arbitrary row" snapshot register, alongside `snapshot_regs` — + // kept separate rather than folded into that array since it has no + // corresponding `schema.columns`/`compact_of` entry to zip against. + let rowid_snapshot_reg = rowid_field_index.map(|_| { + let r = reg.alloc(); + em.emit(Instruction::new(Opcode::Null, 0, r, 0)); + r + }); // Aggregate-context slots (`Vm::agg_contexts`) are a disjoint table // from the register file, addressed by their own small integer // space — a bare 0-based counter here, not `reg.alloc()`. @@ -945,6 +1047,26 @@ where read_pseudo_column(em, &pseudo_schema, cursors.pseudo, compact_idx, r)?; Ok(r) } + OrderByTarget::Expr(_) if is_bare_rowid_pseudo_column(expr, schema) => { + // #708 follow-up: `compile_value(&pseudo_scope, ...)` + // would resolve this against the pass-2 pseudo cursor, + // which can't answer `Opcode::Rowid` at all (that's the + // regression this ticket fixes) — read the field pass 1 + // already materialized instead, same as + // `read_pseudo_column` does for `OrderByTarget::Column` + // just above. + let r = reg.alloc(); + let idx = rowid_field_index.unwrap_or(0); + em.emit(Instruction::new( + Opcode::Column, + cursors.pseudo, + i32::try_from(idx).map_err(|_| CodegenError::Unsupported { + reason: format!("column index {idx} does not fit in a P2 operand"), + })?, + r, + )); + Ok(r) + } OrderByTarget::Expr(_) => compile_value(em, reg, &pseudo_scope, expr), }) .collect::>()?; @@ -995,6 +1117,7 @@ where schema, catalog, &snapshot_regs, + rowid_snapshot_reg, &agg_slots, limit.as_ref(), end_label, @@ -1034,6 +1157,20 @@ where read_pseudo_column(em, &pseudo_schema, cursors.pseudo, compact_idx, dest)?; } } + // #708 follow-up: the bare rowid pseudo-column's own "arbitrary row" + // snapshot, same shape as `snapshot_regs`'s loop just above — read + // straight off pass 1's materialized field (`rowid_field_index`), + // never recomputed via `Opcode::Rowid` against `cursors.pseudo`. + if let (Some(idx), Some(dest)) = (rowid_field_index, rowid_snapshot_reg) { + em.emit(Instruction::new( + Opcode::Column, + cursors.pseudo, + i32::try_from(idx).map_err(|_| CodegenError::Unsupported { + reason: format!("column index {idx} does not fit in a P2 operand"), + })?, + dest, + )); + } let after_accumulate = em.new_label(); let goto_after_accumulate = em.emit(Instruction::new(Opcode::Goto, 0, 0, 0)); em.patch_p2(goto_after_accumulate, after_accumulate); @@ -1070,6 +1207,7 @@ where schema, catalog, &snapshot_regs, + rowid_snapshot_reg, &agg_slots, limit.as_ref(), end_label, @@ -1331,6 +1469,7 @@ where schema, catalog, &snapshot_regs, + None, &agg_slots, limit.as_ref(), end_label, @@ -1374,6 +1513,7 @@ where schema, catalog, &snapshot_regs, + None, &agg_slots, limit.as_ref(), end_label, diff --git a/src/codegen/select/aggregate/accum.rs b/src/codegen/select/aggregate/accum.rs index 9fbea571..b07c136b 100644 --- a/src/codegen/select/aggregate/accum.rs +++ b/src/codegen/select/aggregate/accum.rs @@ -202,6 +202,65 @@ pub(in crate::codegen::select) fn substitute_aggregates( } } +/// #708 follow-up (GROUP BY rowid): rewrites a bare, unshadowed +/// `rowid`/`_rowid_`/`oid` reference into a `Column` reference to +/// [`flush_group`]'s synthetic `__rowid` field — the same idea as +/// [`substitute_aggregates`]'s aggregate-call rewrite, just for the +/// pseudo-column [`compile_grouped_scan`] materialized into the sort +/// record instead. `schema` is the *original* table schema (not the +/// synthetic one), so `column_index` sees any real declared `rowid` +/// column and correctly leaves it alone (shadowing). +pub(in crate::codegen::select) fn substitute_rowid_pseudo_column( + expr: &Expr, + schema: &TableSchema, +) -> Expr { + if let ExprKind::Column { + name, + table: None, + catalog: None, + } = &expr.kind + { + if crate::codegen::expr::column_index(schema, name).is_none() + && crate::codegen::expr::rowid_pseudo_column_index(schema, name).is_some() + { + return Expr { + kind: ExprKind::Column { + table: None, + catalog: None, + name: "__rowid".to_string(), + }, + span: expr.span, + }; + } + } + let kind = match &expr.kind { + ExprKind::Paren(inner) => { + ExprKind::Paren(Box::new(substitute_rowid_pseudo_column(inner, schema))) + } + ExprKind::Collate { + expr: inner, + collation, + } => ExprKind::Collate { + expr: Box::new(substitute_rowid_pseudo_column(inner, schema)), + collation: collation.clone(), + }, + ExprKind::Unary { op, expr: inner } => ExprKind::Unary { + op: *op, + expr: Box::new(substitute_rowid_pseudo_column(inner, schema)), + }, + ExprKind::Binary { op, lhs, rhs } => ExprKind::Binary { + op: *op, + lhs: Box::new(substitute_rowid_pseudo_column(lhs, schema)), + rhs: Box::new(substitute_rowid_pseudo_column(rhs, schema)), + }, + other => other.clone(), + }; + Expr { + kind, + span: expr.span, + } +} + /// Pseudo-cursor-safe single-column read: like `emit_column_read`, but /// aware that `cursor` re-reads an already-materialized record (so the /// rowid-alias column is an ordinary field within it, not something @@ -339,10 +398,13 @@ pub(in crate::codegen::select) fn emit_agg_step( /// Finalizes and emits one grouped output row via `sink`, applying /// `HAVING`/`LIMIT`/`OFFSET` exactly as the ungrouped scans do. Builds a /// synthetic record — the group's snapshot column values (from the last -/// row seen) followed by each aggregate's finalized value — and opens a -/// fresh pseudo cursor over it, so `select.columns`/`having` (with -/// aggregate calls rewritten to reference the synthetic record's -/// trailing fields via [`substitute_aggregates`]) compile through the +/// row seen), the group's bare rowid pseudo-column value (#708 follow-up, +/// when `rowid_reg` is given), then each aggregate's finalized value — +/// and opens a fresh pseudo cursor over it, so `select.columns`/`having` +/// (with aggregate calls rewritten to reference the synthetic record's +/// trailing fields via [`substitute_aggregates`], and a bare +/// `rowid`/`_rowid_`/`oid` reference rewritten to the synthetic `__rowid` +/// field via [`substitute_rowid_pseudo_column`]) compile through the /// ordinary `compile_row_values`/`compile_cond` machinery unchanged. #[allow(clippy::too_many_arguments)] pub(in crate::codegen::select) fn flush_group( @@ -352,6 +414,7 @@ pub(in crate::codegen::select) fn flush_group( schema: &TableSchema, catalog: &[TableSchema], snapshot_regs: &[i32], + rowid_reg: Option, agg_slots: &[AggSlot], limit: Option<&LimitState>, end_label: Label, @@ -363,8 +426,14 @@ where let synthetic_names: Vec = (0..agg_slots.len()).map(|i| format!("__agg{i}")).collect(); let mut synthetic_columns = schema.columns.clone(); + if rowid_reg.is_some() { + synthetic_columns.push("__rowid".to_string()); + } synthetic_columns.extend(synthetic_names.iter().cloned()); let mut synthetic_types = schema.column_types.clone(); + if rowid_reg.is_some() { + synthetic_types.push(String::new()); + } synthetic_types.extend(synthetic_names.iter().map(|_| String::new())); let synthetic_schema = TableSchema { unresolved_autoindex: false, @@ -381,17 +450,28 @@ where rowid_alias: None, }; - // Allocate one fresh, contiguous register per snapshot/aggregate + // Allocate one fresh, contiguous register per snapshot/rowid/aggregate // field up front — `reg.alloc()` bump-allocates sequentially, so as // long as nothing else allocates in between, `dests` is guaranteed // contiguous for `MakeRecord`. - let synthetic_count = snapshot_regs.len().saturating_add(agg_slots.len()); + let rowid_width = usize::from(rowid_reg.is_some()); + let synthetic_count = snapshot_regs + .len() + .saturating_add(rowid_width) + .saturating_add(agg_slots.len()); let dests: Vec = (0..synthetic_count).map(|_| reg.alloc()).collect(); let synthetic_first = dests.first().copied().unwrap_or_else(|| reg.alloc()); for (&snap, &dest) in snapshot_regs.iter().zip(&dests) { em.emit(Instruction::new(Opcode::Copy, snap, dest, 0)); } - let agg_dests = dests.get(snapshot_regs.len()..).unwrap_or(&[]); + if let Some(rreg) = rowid_reg { + if let Some(&dest) = dests.get(snapshot_regs.len()) { + em.emit(Instruction::new(Opcode::Copy, rreg, dest, 0)); + } + } + let agg_dests = dests + .get(snapshot_regs.len().saturating_add(rowid_width)..) + .unwrap_or(&[]); for (agg, &dest) in agg_slots.iter().zip(agg_dests) { // `avg()`'s sum/count division now happens inside // `crate::vdbe::aggregate::finalize` — `AggFinal` just reads @@ -423,7 +503,12 @@ where let flush_scope = Scope::single(&synthetic_schema, flush_cursor).with_catalog(catalog.to_vec()); let skip_label = em.new_label(); if let Some(having) = &select.having { - let rewritten = substitute_aggregates(having, agg_slots, &synthetic_names); + let having = if rowid_reg.is_some() { + substitute_rowid_pseudo_column(having, schema) + } else { + having.clone() + }; + let rewritten = substitute_aggregates(&having, agg_slots, &synthetic_names); compile_cond( em, reg, @@ -443,10 +528,17 @@ where .columns .iter() .map(|col| match col { - ResultColumn::Expr { expr, alias } => ResultColumn::Expr { - expr: substitute_aggregates(expr, agg_slots, &synthetic_names), - alias: alias.clone(), - }, + ResultColumn::Expr { expr, alias } => { + let expr = if rowid_reg.is_some() { + substitute_rowid_pseudo_column(expr, schema) + } else { + expr.clone() + }; + ResultColumn::Expr { + expr: substitute_aggregates(&expr, agg_slots, &synthetic_names), + alias: alias.clone(), + } + } other => other.clone(), }) .collect(); diff --git a/src/codegen/select/aggregate/hash.rs b/src/codegen/select/aggregate/hash.rs index a0acd7ad..e26a807e 100644 --- a/src/codegen/select/aggregate/hash.rs +++ b/src/codegen/select/aggregate/hash.rs @@ -257,6 +257,7 @@ where schema, catalog, &snapshot_regs, + None, &agg_slots, limit.as_ref(), end_label, diff --git a/tests/unit/rowid_projection_test.rs b/tests/unit/rowid_projection_test.rs index c08a0b89..5d883b0d 100644 --- a/tests/unit/rowid_projection_test.rs +++ b/tests/unit/rowid_projection_test.rs @@ -273,3 +273,183 @@ fn without_rowid_table_rejects_rowid_reference() { "expected an unknown-column error, got: {err}" ); } + +/// #708 follow-up: `GROUP BY rowid` regressed relative to main (a +/// runtime "pseudo cursor" internals error) because `compile_grouped_scan` +/// never got the materialize-then-read-back-with-`Column` treatment +/// `compile_sorted_scan` (#718) established for `ORDER BY rowid`. Covers +/// the base case, gaps left by a delete, and every spelling. +#[test] +fn group_by_rowid_matches_oracle() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "group_by_rowid", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); \ + INSERT INTO t VALUES (1, 'aa'), (2, 'bb'), (3, 'cc'), (4, 'dd'); \ + DELETE FROM t WHERE a = 2;", + ); + let schema = rowid_table_schema(); + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT rowid, count(*) FROM t GROUP BY rowid", + ); + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT _rowid_, count(*) FROM t GROUP BY _rowid_", + ); + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT oid, count(*) FROM t GROUP BY oid", + ); +} + +/// #708 follow-up: `GROUP BY rowid` combined with `HAVING` on the same +/// pseudo-column — both the group key comparison and the `HAVING` +/// re-projection go through the same pass-2 pseudo cursor, so both need +/// the fix. +#[test] +fn group_by_rowid_with_having_matches_oracle() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "group_by_rowid_having", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); \ + INSERT INTO t VALUES (1, 'aa'), (2, 'bb'), (3, 'cc'), (4, 'dd'); \ + DELETE FROM t WHERE a = 2;", + ); + let schema = rowid_table_schema(); + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT rowid, count(*) FROM t GROUP BY rowid HAVING rowid > 1", + ); + // A mix of spellings between the `GROUP BY` key and `HAVING` is + // legal in SQLite (all three are the same pseudo-column). + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT rowid, count(*) FROM t GROUP BY rowid HAVING _rowid_ > 1", + ); +} + +/// #708 follow-up: `GROUP BY` combined with `ORDER BY` is rejected +/// outright regardless of what's being grouped by (see +/// `compile_select_scan`'s check, predating this ticket) — `rowid` +/// must hit that same clean compile-time rejection, not the "pseudo +/// cursor" runtime error this ticket fixes elsewhere, and not a panic. +#[test] +fn group_by_rowid_with_order_by_is_cleanly_unsupported() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let schema = rowid_table_schema(); + let db = scratch_db( + "group_by_rowid_order_by", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); INSERT INTO t VALUES (1, 'aa'), (2, 'bb');", + ); + let err = our_rows( + &db, + &schema, + "SELECT rowid, count(*) FROM t GROUP BY rowid ORDER BY rowid", + ) + .expect_err("GROUP BY combined with ORDER BY is not yet supported, rowid or not"); + assert!( + err.contains("Unsupported"), + "expected a clean Unsupported rejection, not a runtime error, got: {err}" + ); + assert!( + !err.contains("pseudo cursor") && !err.contains("CursorTypeMismatch"), + "must not surface the internals error this ticket fixes, got: {err}" + ); +} + +/// Control: `GROUP BY` on an ordinary declared column, with no rowid +/// involved at all, must not regress from this ticket's changes to +/// `compile_grouped_scan`'s pass 1/pass 2. +#[test] +fn group_by_real_column_still_matches_oracle() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "group_by_real_column", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); \ + INSERT INTO t VALUES (1, 'aa'), (2, 'aa'), (3, 'bb');", + ); + let schema = rowid_table_schema(); + assert_matches_oracle( + &oracle, + &db, + &schema, + "SELECT v, count(*) FROM t GROUP BY v", + ); +} + +/// `WITHOUT ROWID` tables have no rowid to `GROUP BY` at all — must be +/// rejected the same way a bare `SELECT rowid` already is (#708), not +/// silently produce a number and not hit the pseudo-cursor internals +/// error. +#[test] +fn group_by_rowid_on_without_rowid_table_rejects_cleanly() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned 3.53.4 sqlite3 oracle on this machine"); + return; + }; + let db = scratch_db( + "group_by_without_rowid", + &oracle, + "CREATE TABLE t(a INTEGER PRIMARY KEY, v TEXT) WITHOUT ROWID; \ + INSERT INTO t VALUES (1, 'aa'), (2, 'bb');", + ); + let oracle_out = Command::new(&oracle) + .arg("-readonly") + .arg(&db) + .arg("SELECT rowid, count(*) FROM t GROUP BY rowid") + .output() + .expect("invoking sqlite3 oracle"); + assert!( + !oracle_out.status.success(), + "expected the oracle itself to reject rowid on a WITHOUT ROWID table" + ); + + let schema = TableSchema { + unresolved_autoindex: false, + name: "t".to_string(), + root_page: 2, + columns: vec!["a".to_string(), "v".to_string()], + column_types: vec!["INTEGER".to_string(), "TEXT".to_string()], + column_collations: vec![], + without_rowid: true, + strict: false, + is_virtual: false, + sql: "CREATE TABLE t(a INTEGER PRIMARY KEY, v TEXT) WITHOUT ROWID".to_string(), + indexes: vec![], + rowid_alias: None, + } + .with_computed_rowid_alias(); + let err = our_rows(&db, &schema, "SELECT rowid, count(*) FROM t GROUP BY rowid") + .expect_err("GROUP BY rowid on a WITHOUT ROWID table must be rejected"); + assert!( + err.contains("UnknownColumn"), + "expected an unknown-column error, got: {err}" + ); +} From 9814f208881ae4ce4be504cdacaabae51b11de01 Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 11 Sep 2026 18:03:49 +0200 Subject: [PATCH 3/3] fix: rowid pseudo-column in an implicit group, and declared-column shadowing (#708) Two silent wrong answers left by this branch's earlier commits, both in the pseudo-cursor family the GROUP BY fix addressed, and both measured against the pinned 3.53.4 oracle rather than inferred: - `SELECT rowid, count(*) FROM t` (an aggregate with no `GROUP BY`) took `try_compile_direct_agg_scan`'s fast path. That path's synthetic per-group record carries one field per *declared* column and nothing else, so the projection's `rowid_pseudo_column_index` sentinel read one past the end of the record and produced an empty value where the oracle returns the rowid. The fast path now declines whenever the statement references the bare pseudo-column, deferring to `compile_grouped_scan`'s implicit-whole-table-group path, which already materializes the field and reads it back with `Column`. - A table with a declared column named `rowid` projected the hidden rowid instead of that column's value once an `ORDER BY` put the read behind a pseudo cursor: `projection.rs` filtered the declared index down to the rowid-alias case and then fell through to the pseudo-column sentinel. SQLite's rule is that a declared column of that name wins, so the sentinel is now only reachable when no such column exists. This one was correct on `origin/main`, i.e. a regression this branch introduced rather than a pre-existing gap. Both are covered by new oracle-diffed tests, each with controls for the cases that were already correct (a plain scan, an aggregate that never mentions the pseudo-column) so a future change cannot quietly swap which path serves them. spend: small, on top of the GROUP BY follow-up's medium --- CHANGELOG.md | 11 +++- src/codegen/select/aggregate.rs | 12 +++++ src/codegen/select/projection.rs | 14 ++++- tests/unit/rowid_projection_test.rs | 82 +++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a51e0f3d..5bc3750b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,16 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep `compile_grouped_scan`'s sort-then-group pass 2 — both the group-key comparison and a bare `rowid` in the result list or `HAVING` now read back a materialized field instead of re-issuing `Rowid` against a - cursor that can't answer it (#708). + cursor that can't answer it (#708). Two silent wrong answers in the + same family are fixed alongside it: an aggregate with no `GROUP BY` + (`SELECT rowid, count(*) FROM t`) took a fast path whose synthetic + per-group record has no slot for the pseudo-column and so projected an + empty value instead of the rowid — that path now defers to + `compile_grouped_scan`; and a table with a *declared* column named + `rowid` projected the hidden rowid instead of the column's own value + once an `ORDER BY` put the read behind a pseudo cursor, because the + projection fell through to the pseudo-column sentinel even though a + declared column of that name shadows it (#708). ## [0.18.10] - 2026-08-31 diff --git a/src/codegen/select/aggregate.rs b/src/codegen/select/aggregate.rs index 43908c5f..298b6df8 100644 --- a/src/codegen/select/aggregate.rs +++ b/src/codegen/select/aggregate.rs @@ -321,6 +321,18 @@ where if aggs.iter().any(|(_, _, _, distinct)| *distinct) { return Ok(false); } + // #708 follow-up: this fast path's synthetic per-group record holds + // one field per *declared* schema column and nothing else, so it has + // no slot for the bare `rowid`/`_rowid_`/`oid` pseudo-column — yet + // the projection still asks for `rowid_pseudo_column_index`'s + // sentinel (one past the last column), which reads off the end of + // the record and projects an empty value instead of the rowid. + // Decline and let `compile_grouped_scan`'s implicit-whole-table-group + // path handle it: it materializes the field (see + // `grouped_scan_needs_rowid_field`) and reads it back with `Column`. + if grouped_scan_needs_rowid_field(select, schema) { + return Ok(false); + } let table_scope = Scope::single(schema, cursors.table).with_catalog(catalog.to_vec()); // #322: hoist any uncorrelated WHERE-clause subquery once, up diff --git a/src/codegen/select/projection.rs b/src/codegen/select/projection.rs index 82436b08..5ef772e1 100644 --- a/src/codegen/select/projection.rs +++ b/src/codegen/select/projection.rs @@ -134,11 +134,23 @@ pub(super) fn compile_row_values( catalog: None, } = &expr.kind { + let declared_idx = column_index(schema, name); let pseudo_rowid_idx = pseudo - .then(|| column_index(schema, name)) + .then_some(declared_idx) .flatten() .filter(|idx| schema.rowid_alias == Some(*idx)) .or_else(|| { + // SQLite's shadowing rule: a *declared* + // column named `rowid`/`_rowid_`/`oid` wins + // over the pseudo-column, so the sentinel is + // only reachable when no such column exists. + // Without this guard a declared, non-alias + // `rowid` column read back through the + // post-`ORDER BY` pseudo cursor projected the + // hidden rowid instead of its own value. + if declared_idx.is_some() { + return None; + } pseudo .then(|| { crate::codegen::expr::rowid_pseudo_column_index(schema, name) diff --git a/tests/unit/rowid_projection_test.rs b/tests/unit/rowid_projection_test.rs index 5d883b0d..b562d03b 100644 --- a/tests/unit/rowid_projection_test.rs +++ b/tests/unit/rowid_projection_test.rs @@ -453,3 +453,85 @@ fn group_by_rowid_on_without_rowid_table_rejects_cleanly() { "expected an unknown-column error, got: {err}" ); } + +/// #708 follow-up: the implicit whole-table group (an aggregate with no +/// `GROUP BY`) took `try_compile_direct_agg_scan`'s fast path, whose +/// synthetic per-group record has one field per declared column and no +/// slot for the rowid pseudo-column — so `SELECT rowid, count(*)` read +/// off the end of the record and projected an empty value where the +/// oracle returns the rowid. Silent wrong answer, not an error. +#[test] +fn bare_rowid_alongside_an_aggregate_with_no_group_by() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned sqlite3 oracle found"); + return; + }; + let db = scratch_db( + "implicit_group_rowid", + &oracle, + "CREATE TABLE t(a INTEGER, v TEXT); \ + INSERT INTO t VALUES (10, 'aa'), (20, 'bb'), (30, 'cc');", + ); + let schema = rowid_table_schema(); + for sql in [ + "SELECT rowid, count(*) FROM t", + "SELECT _rowid_, count(*) FROM t", + "SELECT oid, count(*) FROM t", + "SELECT rowid, count(*) FROM t WHERE v > 'aa'", + "SELECT rowid, count(*) FROM t HAVING count(*) > 0", + "SELECT max(rowid) FROM t", + // Control: the fast path must still serve an aggregate that + // never mentions the pseudo-column. + "SELECT count(*) FROM t", + "SELECT count(*), max(a) FROM t", + ] { + assert_matches_oracle(&oracle, &db, &schema, sql); + } +} + +/// #708 follow-up: SQLite's shadowing rule says a *declared* column +/// named `rowid` wins over the pseudo-column. That held on a plain scan +/// but broke once the row was read back through the post-`ORDER BY` +/// pseudo cursor, which projected the hidden rowid instead of the +/// declared column's own value — correct on `origin/main`, so a +/// regression this stack introduced. +#[test] +fn a_declared_rowid_column_still_wins_after_order_by() { + let Some(oracle) = pinned_oracle() else { + eprintln!("skipping: no pinned sqlite3 oracle found"); + return; + }; + let db = scratch_db( + "declared_rowid_order_by", + &oracle, + "CREATE TABLE t(rowid TEXT, v TEXT); \ + INSERT INTO t VALUES ('r1', 'bb'), ('r2', 'aa');", + ); + let schema = TableSchema { + unresolved_autoindex: false, + name: "t".to_string(), + root_page: 2, + columns: vec!["rowid".to_string(), "v".to_string()], + column_types: vec!["TEXT".to_string(), "TEXT".to_string()], + column_collations: vec![], + without_rowid: false, + strict: false, + is_virtual: false, + sql: "CREATE TABLE t(rowid TEXT, v TEXT)".to_string(), + indexes: vec![], + rowid_alias: None, + } + .with_computed_rowid_alias(); + for sql in [ + // The regression: a sort puts the read behind a pseudo cursor. + "SELECT rowid, v FROM t ORDER BY v", + "SELECT rowid, v FROM t ORDER BY v DESC", + "SELECT rowid FROM t ORDER BY rowid", + "SELECT t.rowid FROM t ORDER BY v", + // Controls: these were already correct and must stay so. + "SELECT rowid, v FROM t", + "SELECT rowid, v FROM t WHERE rowid = 'r1'", + ] { + assert_matches_oracle(&oracle, &db, &schema, sql); + } +}