From a10d08e0aba61b8a9d0c84f33b1c29844fa71224 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:10:53 +0800 Subject: [PATCH 1/9] fix(sql): evaluate sequence-backed DEFAULTs on every engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEFAULT expressions were honored only where an engine happened to wire them: uuid() worked on strict, silently vanished on schemaless document, kv, and columnar; sequence defaults (nextval) did not run anywhere — the pure planner evaluator does not know the accessors, and every engine path swallowed "no value" into a missing column. #294's repro: DDL accepted `DEFAULT nextval('s')`, every insert silently committed NULL (including primary keys). One defect at two layers, fixed together: 1. Storage: the catalog adapter's schemaless branch discarded the DEFAULT clause even though `stored.fields` carries the full DDL constraint text. UUID-class defaults on the document engine now materialize. 2. Evaluation: one shared per-row expander (`expand_row_defaults`) serves INSERT, UPSERT, the columnar batch encoder and the kv converter — replacing the per-path pure-evaluator swallows. Sequence accessors are classified by ONE canonical parser (`sequence_accessor`, byte-safe, robust-parsing-gate clean) shared between planner and convert: - nextval('name') advances the CP-side registry per row; - currval/setval in a DEFAULT raise loudly (no per-row meaning); - accessor-shaped but malformed (nextval('')) raises loudly; - unknown sequences raise naming them. A DDL-accepted DEFAULT never silently becomes NULL. Mechanics: SequenceRegistry threaded SharedState -> QueryContext -> ConvertContext; SqlPlan::KvInsert carries key_column + sequence_defaults (kv planner skips them; converter fills key slot + mirrored value map so scans read the defaulted key back); rows.rs and the doc-family paths share the same expander; nodedb_value_to_sql moved into value/convert. Verified: wire sequence_default_all_engines 8 tests (strict/doc/kv/ columnar/doc-UPSERT nextval fills; uuid sentinel; currval loud; malformed loud; unknown-seq loud) + sequence_default_typed 3; regression sentinels green (kv_column_defaults, dml_returning, not-null gate). fmt clean; clippy -D warnings clean; robust-parsing + calvin gates clean; sql 864 + types 688. Flow also verified by instrumented run (SEQDBG logs): doc nextval filled Integer(1),(2); currval reached the loud branch; uuid_v7 went through the pure evaluator. Not included here (tracked separately): SELECT/currval/setval evaluation — accessors stay unregistered so the gate keeps raising 42883 loudly; the registry registration ships together with the SELECT-side fold so no commit ever leaves a registered call folding to NULL. Partially addresses #294. --- nodedb-sql/src/planner/defaults.rs | 58 +++++ nodedb-sql/src/types/plan/variants.rs | 6 + .../src/visitor/plan_visitor/dispatch.rs | 12 +- .../src/visitor/plan_visitor/trait_def.rs | 3 + .../planner/catalog_adapter/type_convert.rs | 22 +- .../control/planner/context/query/context.rs | 9 + .../control/planner/context/query/planning.rs | 2 + .../array_fn_convert/aggregate.rs | 1 + .../array_fn_convert/slice.rs | 1 + .../planner/sql_plan_convert/convert.rs | 4 + .../planner/sql_plan_convert/dml/insert.rs | 31 +-- .../sql_plan_convert/dml/kv_and_vector.rs | 56 ++++- .../dml/update_delete/delete.rs | 1 + .../dml/update_delete/update.rs | 1 + .../planner/sql_plan_convert/dml/upsert.rs | 12 +- .../planner/sql_plan_convert/set_ops.rs | 2 + .../planner/sql_plan_convert/value/convert.rs | 11 + .../planner/sql_plan_convert/value/mod.rs | 3 + .../planner/sql_plan_convert/value/rows.rs | 21 +- .../value/sequence_default.rs | 104 ++++++++ .../sql_plan_convert/visitor/arms_dml.rs | 4 + .../executor_tests/test_group_by_alias.rs | 1 + nodedb/tests/wire/cases/mod.rs | 2 + .../cases/sequence_default_all_engines.rs | 226 ++++++++++++++++++ .../wire/cases/sequence_default_typed.rs | 109 +++++++++ 25 files changed, 655 insertions(+), 47 deletions(-) create mode 100644 nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs create mode 100644 nodedb/tests/wire/cases/sequence_default_all_engines.rs create mode 100644 nodedb/tests/wire/cases/sequence_default_typed.rs diff --git a/nodedb-sql/src/planner/defaults.rs b/nodedb-sql/src/planner/defaults.rs index 5fb914978..59bcdd8ac 100644 --- a/nodedb-sql/src/planner/defaults.rs +++ b/nodedb-sql/src/planner/defaults.rs @@ -120,3 +120,61 @@ fn sql_value_to_ndb(v: crate::types::SqlValue) -> nodedb_types::Value { SqlValue::Timestamptz(dt) => nodedb_types::Value::DateTime(dt), } } + +/// A sequence accessor appearing in a DEFAULT expression. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SequenceAccessor { + Nextval, + Currval, + Setval, +} + +/// Whether a DEFAULT expression *starts like* a sequence accessor +/// (`nextval(`/`currval(`/`setval(` … `)`) even when the body does not parse. +/// Malformed accessor-looking defaults must raise loudly — never fall back +/// to the pure evaluator and silently vanish (#294 class). +pub fn looks_like_sequence_accessor(expr: &str) -> bool { + let t = expr.trim(); + let bytes = t.as_bytes(); + if bytes.len() < 8 || !t.ends_with(')') { + return false; + } + let head = |n: usize, name: &[u8]| bytes[..n].eq_ignore_ascii_case(name) && bytes[n] == b'('; + head(7, b"nextval") || head(7, b"currval") || head(6, b"setval") +} + +/// One canonical sequence-accessor recognizer for DEFAULT expressions, +/// shared by the SQL planner (which must skip these — the pure evaluator +/// cannot run them) and the convert layer (which advances the CP-side +/// registry). Matched on the ORIGINAL bytes, ASCII case-insensitive — never +/// by slicing the original with a length taken from a case-folded copy. +pub fn sequence_accessor(expr: &str) -> Option<(SequenceAccessor, String)> { + let t = expr.trim(); + let bytes = t.as_bytes(); + if bytes.len() < 8 || !t.ends_with(')') { + return None; + } + let (accessor, prefix_len) = if bytes[..7].eq_ignore_ascii_case(b"nextval") { + (SequenceAccessor::Nextval, 7) + } else if bytes[..7].eq_ignore_ascii_case(b"currval") { + (SequenceAccessor::Currval, 7) + } else if bytes[..6].eq_ignore_ascii_case(b"setval") { + (SequenceAccessor::Setval, 6) + } else { + return None; + }; + if bytes[prefix_len] != b'(' { + return None; + } + let inner = &t[prefix_len + 1..t.len() - 1]; + let inner = inner.trim(); + let name = inner + .strip_prefix('\'') + .and_then(|s| s.strip_suffix('\'')) + .or_else(|| inner.strip_prefix('"').and_then(|s| s.strip_suffix('"'))) + .unwrap_or(inner); + if name.is_empty() { + return None; + } + Some((accessor, name.to_string())) +} diff --git a/nodedb-sql/src/types/plan/variants.rs b/nodedb-sql/src/types/plan/variants.rs index 5e539e7b8..4752b0bb6 100644 --- a/nodedb-sql/src/types/plan/variants.rs +++ b/nodedb-sql/src/types/plan/variants.rs @@ -138,6 +138,12 @@ pub enum SqlPlan { /// Empty for plain UPSERT (whole-value overwrite) and for INSERT /// variants. on_conflict_updates: Vec<(String, SqlExpr)>, + /// The collection's primary-key column name (the KV key slot). + key_column: String, + /// Column defaults that are sequence accessors (`nextval(...)`) — + /// the pure planner evaluator cannot run them; the converter + /// advances the CP-side registry per row instead. + sequence_defaults: Vec<(String, String)>, }, /// UPSERT: insert or merge if document exists. Upsert { diff --git a/nodedb-sql/src/visitor/plan_visitor/dispatch.rs b/nodedb-sql/src/visitor/plan_visitor/dispatch.rs index 1357c1b1d..573b7ca91 100644 --- a/nodedb-sql/src/visitor/plan_visitor/dispatch.rs +++ b/nodedb-sql/src/visitor/plan_visitor/dispatch.rs @@ -113,7 +113,17 @@ pub fn dispatch(visitor: &mut V, plan: &SqlPlan) -> Result visitor.kv_insert(collection, entries, *ttl_secs, *intent, on_conflict_updates), + key_column, + sequence_defaults, + } => visitor.kv_insert( + collection, + entries, + *ttl_secs, + *intent, + on_conflict_updates, + key_column, + sequence_defaults, + ), SqlPlan::Upsert { collection, engine, diff --git a/nodedb-sql/src/visitor/plan_visitor/trait_def.rs b/nodedb-sql/src/visitor/plan_visitor/trait_def.rs index 6a4ce0b0b..a8c9757c0 100644 --- a/nodedb-sql/src/visitor/plan_visitor/trait_def.rs +++ b/nodedb-sql/src/visitor/plan_visitor/trait_def.rs @@ -73,6 +73,7 @@ pub trait PlanVisitor { fn insert(&mut self, args: InsertVisitArgs<'_>) -> Result; /// Handle [`SqlPlan::KvInsert`]. + #[allow(clippy::too_many_arguments)] fn kv_insert( &mut self, collection: &str, @@ -80,6 +81,8 @@ pub trait PlanVisitor { ttl_secs: u64, intent: KvInsertIntent, on_conflict_updates: &[(String, SqlExpr)], + key_column: &str, + sequence_defaults: &[(String, String)], ) -> Result; /// Handle [`SqlPlan::Upsert`]. diff --git a/nodedb/src/control/planner/catalog_adapter/type_convert.rs b/nodedb/src/control/planner/catalog_adapter/type_convert.rs index 02c8f8af6..b7ca42c2e 100644 --- a/nodedb/src/control/planner/catalog_adapter/type_convert.rs +++ b/nodedb/src/control/planner/catalog_adapter/type_convert.rs @@ -61,12 +61,24 @@ pub(super) fn convert_collection_type( .declared_primary_key .clone() .unwrap_or_else(|| "id".to_string()); + // The stored field entry keeps the FULL DDL constraint text + // (`"BIGINT DEFAULT nextval('s') PRIMARY KEY"`), so the DEFAULT + // expression is recoverable even though the schemaless type + // system records no per-column default slot of its own. Dropping + // it here is what made `DEFAULT uuid_v7()` / `DEFAULT + // nextval('s')` silently commit NULL on the document engine + // (#294): the DDL accepted the expression, the catalog forgot it. + let pk_default = stored + .fields + .iter() + .find(|(n, _)| n.eq_ignore_ascii_case(&pk_name)) + .and_then(|(_, ts)| doc_default_expr(ts)); let mut columns = vec![ColumnInfo { name: pk_name.clone(), data_type: SqlDataType::String, nullable: false, is_primary_key: true, - default: None, + default: pk_default, raw_type: None, int_width: None, float_width: None, @@ -81,7 +93,7 @@ pub(super) fn convert_collection_type( data_type: parse_type_str(type_str), nullable: true, is_primary_key: false, - default: None, + default: doc_default_expr(type_str), raw_type: None, int_width: IntWidth::from_declared_type(type_str), float_width: FloatWidth::from_declared_type(type_str), @@ -289,6 +301,12 @@ fn parse_type_str(s: &str) -> SqlDataType { } } +fn doc_default_expr(type_str: &str) -> Option { + let (_, _, _, default_expr) = + nodedb_sql::ddl_ast::collection_type::parse_column_type_str_full(type_str); + default_expr +} + #[cfg(test)] mod tests { use nodedb_types::CollectionType; diff --git a/nodedb/src/control/planner/context/query/context.rs b/nodedb/src/control/planner/context/query/context.rs index a7cee59a2..139ac426f 100644 --- a/nodedb/src/control/planner/context/query/context.rs +++ b/nodedb/src/control/planner/context/query/context.rs @@ -40,6 +40,11 @@ pub struct QueryContext { /// `QueryContext::new()` test fixtures that never lower to /// surrogate-bearing variants. pub(super) surrogate_assigner: Option>, + /// Sequence registry — `Some` when the planner has access to + /// `SharedState` (production path). SQL sequence accessors + /// (`nextval`/`currval`/`setval`) and sequence-backed DEFAULT + /// expressions evaluate through this; `None` for sub-planners. + pub(super) sequence_registry: Option>, /// Cluster mode flag — `true` when the node has a live cluster /// topology. Passed into `ConvertContext` so array converters can /// emit `ClusterArray` variants instead of local `Array` variants. @@ -109,6 +114,7 @@ impl QueryContext { array_catalog: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: std::sync::atomic::AtomicU32::new(0), @@ -140,6 +146,7 @@ impl QueryContext { Some(Arc::clone(&state.retention_policy_registry)), ); ctx.surrogate_assigner = Some(Arc::clone(&state.surrogate_assigner)); + ctx.sequence_registry = Some(Arc::clone(&state.sequence_registry)); ctx.cluster_enabled = state.cluster_topology.is_some(); ctx.bitemporal_retention_registry = Some(Arc::clone(&state.bitemporal_retention_registry)); // max_vector_dim starts at 0 (unlimited); connection handlers call @@ -174,6 +181,7 @@ impl QueryContext { array_catalog: Some(state.array_catalog.clone()), wal: Some(Arc::clone(&state.wal)), surrogate_assigner: Some(Arc::clone(&state.surrogate_assigner)), + sequence_registry: Some(Arc::clone(&state.sequence_registry)), cluster_enabled: state.cluster_topology.is_some(), bitemporal_retention_registry: Some(Arc::clone(&state.bitemporal_retention_registry)), // max_vector_dim is tenant-specific; callers supply it via @@ -215,6 +223,7 @@ impl QueryContext { array_catalog: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: std::sync::atomic::AtomicU32::new(0), diff --git a/nodedb/src/control/planner/context/query/planning.rs b/nodedb/src/control/planner/context/query/planning.rs index 8a845dcd7..7a4724187 100644 --- a/nodedb/src/control/planner/context/query/planning.rs +++ b/nodedb/src/control/planner/context/query/planning.rs @@ -156,6 +156,7 @@ impl QueryContext { .map(|i| Arc::clone(&i.credentials)), wal: self.wal.clone(), surrogate_assigner: self.surrogate_assigner.clone(), + sequence_registry: self.sequence_registry.clone(), cluster_enabled: self.cluster_enabled, bitemporal_retention_registry: self.bitemporal_retention_registry.clone(), max_vector_dim: self @@ -421,6 +422,7 @@ impl QueryContext { .map(|i| Arc::clone(&i.credentials)), wal: self.wal.clone(), surrogate_assigner: self.surrogate_assigner.clone(), + sequence_registry: self.sequence_registry.clone(), cluster_enabled: self.cluster_enabled, bitemporal_retention_registry: self.bitemporal_retention_registry.clone(), max_vector_dim: self diff --git a/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/aggregate.rs b/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/aggregate.rs index d52fc535b..71208f710 100644 --- a/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/aggregate.rs +++ b/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/aggregate.rs @@ -144,6 +144,7 @@ mod tests { credentials: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/slice.rs b/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/slice.rs index ed997b9f4..dae12aea5 100644 --- a/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/slice.rs +++ b/nodedb/src/control/planner/sql_plan_convert/array_fn_convert/slice.rs @@ -246,6 +246,7 @@ mod tests { credentials: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/convert.rs b/nodedb/src/control/planner/sql_plan_convert/convert.rs index 7f2a67670..44b091081 100644 --- a/nodedb/src/control/planner/sql_plan_convert/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/convert.rs @@ -62,6 +62,9 @@ pub struct ConvertContext { pub credentials: Option>, /// LSN allocator for array Put/Delete dispatches. pub wal: Option>, + /// Sequence registry for SQL sequence accessors and sequence-backed + /// DEFAULT expressions. `None` for sub-planner contexts. + pub sequence_registry: Option>, /// CP-side surrogate assigner — bound to the same `Arc` held on /// `SharedState`. Threaded into INSERT/UPSERT/KV-INSERT converters /// to bind `(collection, pk_bytes)` → `Surrogate` before the op @@ -281,6 +284,7 @@ mod tests { credentials: None, wal: None, surrogate_assigner: Some(assigner), + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs index 57bcb697e..c7c30b18f 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs @@ -213,17 +213,6 @@ pub(super) fn columnar_row_surrogates( Ok(out) } -pub(in super::super) fn nodedb_value_to_sql(val: nodedb_types::Value) -> SqlValue { - match val { - nodedb_types::Value::Integer(n) => SqlValue::Int(n), - nodedb_types::Value::Float(f) => SqlValue::Float(f), - nodedb_types::Value::String(s) => SqlValue::String(s), - nodedb_types::Value::Bool(b) => SqlValue::Bool(b), - nodedb_types::Value::Null => SqlValue::Null, - _ => SqlValue::String(format!("{val:?}")), - } -} - /// Bundled arguments for [`convert_insert`]. pub(in super::super) struct ConvertInsertArgs<'a> { pub collection: &'a str, @@ -301,17 +290,7 @@ pub(in super::super) fn convert_insert( expanded_rows.push(row.clone()); continue; } - let mut expanded = row.clone(); - for (col_name, default_expr) in column_defaults { - if !expanded.iter().any(|(k, _)| k == col_name) - && let Some(val) = super::super::value::evaluate_default_expr(default_expr) - .map_err(|e| crate::Error::PlanError { - detail: format!("default for column '{col_name}': {e}"), - })? - { - expanded.push((col_name.clone(), nodedb_value_to_sql(val))); - } - } + let expanded = super::super::value::expand_row_defaults(ctx, row, column_defaults)?; expanded_rows.push(expanded); } @@ -330,7 +309,10 @@ pub(in super::super) fn convert_insert( }); } EngineType::Columnar | EngineType::Spatial => { - columnar_rows.push(&rows[i]); + // `expanded_rows[i]` carries the materialized defaults; the + // raw `rows[i]` may still be missing the column (sequence and + // stateless defaults would silently vanish otherwise). + columnar_rows.push(&expanded_rows[i]); } EngineType::DocumentSchemaless | EngineType::DocumentStrict => { let value_bytes = row_to_msgpack(row)?; @@ -405,7 +387,7 @@ pub(in super::super) fn convert_insert( } if !columnar_rows.is_empty() { - let payload = rows_to_msgpack_array(&columnar_rows, column_defaults)?; + let payload = rows_to_msgpack_array(&columnar_rows, column_defaults, ctx)?; let intent = if if_absent { ColumnarInsertIntent::InsertIfAbsent } else { @@ -486,6 +468,7 @@ mod tests { credentials: Some(Arc::new(store)), wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs b/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs index 43c2b03f5..ce40fe145 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs @@ -14,12 +14,15 @@ use super::super::value::{ use super::insert::assign_for_pk; use nodedb_physical::physical_task::{PhysicalTask, PostSetOp}; +#[allow(clippy::too_many_arguments)] pub(in super::super) fn convert_kv_insert( collection: &str, entries: &[(SqlValue, Vec<(String, SqlValue)>)], ttl_secs: u64, intent: KvInsertIntent, on_conflict_updates: &[(String, SqlExpr)], + key_column: &str, + sequence_defaults: &[(String, String)], tenant_id: TenantId, ctx: &ConvertContext, ) -> crate::Result> { @@ -35,9 +38,34 @@ pub(in super::super) fn convert_kv_insert( let ttl_ms = ttl_secs * 1000; let mut tasks = Vec::with_capacity(entries.len()); for (key_val, value_cols) in entries { - // A declared PRIMARY KEY implies NOT NULL. The planner substitutes - // `SqlValue::Null` for a column the statement omitted, so this also - // catches an omitted key, not only an explicit `NULL` literal. + // Sequence-backed defaults (key or value column) advance the CP-side + // registry here — the planner cannot run them. The planner + // substitutes `SqlValue::Null` for a column the statement omitted, + // so a NULL key either names a declared sequence default (filled + // below) or violates the PRIMARY KEY's NOT NULL (#310) and is + // rejected. + let mut key_val = key_val.clone(); + let mut value_cols = value_cols.clone(); + if matches!(key_val, SqlValue::Null) + && let Some((_, expr)) = sequence_defaults + .iter() + .find(|(name, _)| name == key_column) + { + key_val = sequence_default_value(ctx, expr)?; + // Named primary-key columns are mirrored into the value map + // so scans can project/filter them (see the builder's + // exclusion rule); a defaulted key must mirror too, or the + // row reads back with a blank key column. + if key_column != "key" && !value_cols.iter().any(|(c, _)| c == key_column) { + value_cols.push((key_column.to_string(), key_val.clone())); + } + } + for (name, expr) in sequence_defaults { + if name != key_column && !value_cols.iter().any(|(c, _)| c == name) { + let val = sequence_default_value(ctx, expr)?; + value_cols.push((name.clone(), val)); + } + } if matches!(key_val, SqlValue::Null) { return Err(crate::Error::RejectedConstraint { collection: collection.to_string(), @@ -45,13 +73,13 @@ pub(in super::super) fn convert_kv_insert( detail: "primary key cannot be NULL or omitted".to_string(), }); } - let key = sql_value_to_bytes(key_val); + let key = sql_value_to_bytes(&key_val); let value = if value_cols.len() == 1 && value_cols[0].0 == "value" { sql_value_to_bytes(&value_cols[0].1) } else { let mut buf = Vec::with_capacity(value_cols.len() * 32); write_msgpack_map_header(&mut buf, value_cols.len()); - for (col, val) in value_cols { + for (col, val) in &value_cols { write_msgpack_str(&mut buf, col); write_msgpack_value(&mut buf, val); } @@ -200,6 +228,23 @@ pub(in super::super) fn convert_vector_primary_insert( Ok(tasks) } +fn sequence_default_value(ctx: &ConvertContext, expr: &str) -> crate::Result { + let Some(registry) = &ctx.sequence_registry else { + return Err(crate::Error::PlanError { + detail: format!("sequence default '{expr}' requires sequence registry access"), + }); + }; + let name = super::super::value::sequence_name(expr).ok_or_else(|| crate::Error::PlanError { + detail: format!("unrecognized sequence default expression: '{expr}'"), + })?; + let value = registry + .nextval(ctx.database_id.as_u64(), ctx.tenant_id.as_u64(), &name) + .map_err(|e| crate::Error::PlanError { + detail: format!("nextval('{name}'): {e}"), + })?; + Ok(SqlValue::Int(value)) +} + #[cfg(test)] mod tests { use super::super::super::convert::ConvertContext; @@ -214,6 +259,7 @@ mod tests { credentials: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/delete.rs b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/delete.rs index 29246ede5..ab5bd42f7 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/delete.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/delete.rs @@ -250,6 +250,7 @@ mod tests { credentials: Some(Arc::new(store)), wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs index f9c75d93f..6558fa869 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs @@ -377,6 +377,7 @@ mod tests { credentials: Some(Arc::new(store)), wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs index a9ec7435b..153d5f7b6 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs @@ -76,8 +76,14 @@ pub(in super::super) fn convert_upsert( for row in rows { match engine { EngineType::DocumentSchemaless | EngineType::DocumentStrict => { - let value_bytes = row_to_msgpack(row)?; - let (doc_id, surrogate) = resolve_doc_identity(ctx, collection, primary_key, row)?; + // Defaults (incl. sequence-backed) materialize before + // encoding and before identity resolution: a column the + // statement omitted may still carry a DEFAULT, and a + // sequence-defaulted primary key must not be mistaken for a + // NULL/omitted key by the NOT NULL enforcement below. + let expanded = super::super::value::expand_row_defaults(ctx, row, column_defaults)?; + let value_bytes = row_to_msgpack(&expanded)?; + let (doc_id, surrogate) = resolve_doc_identity(ctx, collection, primary_key, &expanded)?; let plan = if is_crdt { PhysicalPlan::Crdt(CrdtOp::DocUpsert { collection: qualified_collection.clone(), @@ -128,7 +134,7 @@ pub(in super::super) fn convert_upsert( } if !columnar_rows.is_empty() { - let payload = rows_to_msgpack_array(&columnar_rows, column_defaults)?; + let payload = rows_to_msgpack_array(&columnar_rows, column_defaults, ctx)?; let surrogates = columnar_row_surrogates(ctx, collection, &columnar_rows, primary_key)?; let schema_bytes = build_schema_bytes(column_schema); tasks.push(PhysicalTask { diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index 8b4d6dacb..9fe807b16 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -401,6 +401,7 @@ mod tests { credentials: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, @@ -460,6 +461,7 @@ mod tests { credentials: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/value/convert.rs b/nodedb/src/control/planner/sql_plan_convert/value/convert.rs index 178a8a2eb..ba06f4304 100644 --- a/nodedb/src/control/planner/sql_plan_convert/value/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/value/convert.rs @@ -85,6 +85,17 @@ pub(crate) fn sql_value_to_msgpack(v: &SqlValue) -> Vec { buf } +pub(crate) fn nodedb_value_to_sql(val: nodedb_types::Value) -> SqlValue { + match val { + nodedb_types::Value::Integer(n) => SqlValue::Int(n), + nodedb_types::Value::Float(f) => SqlValue::Float(f), + nodedb_types::Value::String(s) => SqlValue::String(s), + nodedb_types::Value::Bool(b) => SqlValue::Bool(b), + nodedb_types::Value::Null => SqlValue::Null, + other => SqlValue::String(other.to_string()), + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/nodedb/src/control/planner/sql_plan_convert/value/mod.rs b/nodedb/src/control/planner/sql_plan_convert/value/mod.rs index 778074ead..0bdbb28bf 100644 --- a/nodedb/src/control/planner/sql_plan_convert/value/mod.rs +++ b/nodedb/src/control/planner/sql_plan_convert/value/mod.rs @@ -22,4 +22,7 @@ pub(super) use msgpack_write::{ write_msgpack_value, }; pub(super) use nodedb_sql::planner::defaults::evaluate_default_expr; +pub(super) use sequence_default::{expand_row_defaults, sequence_name}; + +mod sequence_default; pub(super) use rows::rows_to_msgpack_array; diff --git a/nodedb/src/control/planner/sql_plan_convert/value/rows.rs b/nodedb/src/control/planner/sql_plan_convert/value/rows.rs index 2fcf789af..c76f1d3b1 100644 --- a/nodedb/src/control/planner/sql_plan_convert/value/rows.rs +++ b/nodedb/src/control/planner/sql_plan_convert/value/rows.rs @@ -4,29 +4,26 @@ use nodedb_sql::types::SqlValue; +use super::super::convert::ConvertContext; use super::convert::sql_value_to_nodedb_value; -use nodedb_sql::planner::defaults::evaluate_default_expr; +use super::expand_row_defaults; pub(crate) fn rows_to_msgpack_array( rows: &[&Vec<(String, SqlValue)>], column_defaults: &[(String, String)], + ctx: &ConvertContext, ) -> crate::Result> { let mut arr: Vec = Vec::with_capacity(rows.len()); for row in rows { + // The one shared per-row default expander (sequence accessors via + // the CP registry, stateless via the pure evaluator) — same code as + // the INSERT/UPSERT document-family paths, so no engine path can + // swallow a declared default to NULL (#294). + let expanded = expand_row_defaults(ctx, row, column_defaults)?; let mut map = std::collections::HashMap::new(); - for (key, val) in row.iter() { + for (key, val) in expanded.iter() { map.insert(key.clone(), sql_value_to_nodedb_value(val)); } - for (col_name, default_expr) in column_defaults { - if !map.contains_key(col_name) - && let Some(val) = - evaluate_default_expr(default_expr).map_err(|e| crate::Error::PlanError { - detail: format!("default for column '{col_name}': {e}"), - })? - { - map.insert(col_name.clone(), val); - } - } arr.push(nodedb_types::Value::Object(map)); } let val = nodedb_types::Value::Array(arr); diff --git a/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs b/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs new file mode 100644 index 000000000..ff40ab318 --- /dev/null +++ b/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Sequence-aware DEFAULT expression evaluation. +//! +//! The pure `nodedb_sql::planner::defaults::evaluate_default_expr` handles +//! stateless defaults (UUID, now(), literals). Sequence-backed defaults — +//! `DEFAULT nextval('seq')` — are stateful and evaluate through the CP-side +//! `SequenceRegistry` threaded on `ConvertContext`. Without this route the +//! expression fell through to "unknown function -> None", which silently +//! omitted the column (NULL key, per #294). + +use nodedb_sql::types::SqlValue; +use nodedb_types::Value; + +use super::super::convert::ConvertContext; + +/// Classify a DEFAULT expression against the canonical sequence-accessor +/// parser, routing `nextval` to the CP-side registry and rejecting the +/// other accessors loudly — `currval`/`setval` have no per-row meaning in a +/// DEFAULT, and a DDL-accepted DEFAULT must never silently vanish into NULL. +fn evaluate_sequence_default(ctx: &ConvertContext, expr: &str) -> crate::Result> { + let Some((accessor, name)) = nodedb_sql::planner::defaults::sequence_accessor(expr) else { + // Accessor-shaped but malformed (e.g. nextval('')): raise loudly, the + // pure evaluator would silently vanish it (#294 class). + if nodedb_sql::planner::defaults::looks_like_sequence_accessor(expr) { + return Err(crate::Error::PlanError { + detail: format!("malformed sequence default: '{expr}'"), + }); + } + return Ok(None); + }; + use nodedb_sql::planner::defaults::SequenceAccessor; + let value = match accessor { + SequenceAccessor::Nextval => { + let Some(registry) = &ctx.sequence_registry else { + return Err(crate::Error::PlanError { + detail: format!("sequence default '{expr}' requires sequence registry access"), + }); + }; + match registry.nextval(ctx.database_id.as_u64(), ctx.tenant_id.as_u64(), &name) { + Ok(value) => Value::Integer(value), + Err(e) => { + return Err(crate::Error::PlanError { + detail: format!("nextval('{name}'): {e}"), + }); + } + } + } + SequenceAccessor::Currval | SequenceAccessor::Setval => { + let accessor_name = match accessor { + SequenceAccessor::Currval => "currval", + SequenceAccessor::Setval => "setval", + SequenceAccessor::Nextval => unreachable!("handled above"), + }; + return Err(crate::Error::PlanError { + detail: format!( + "DEFAULT {accessor_name}('{name}') is not supported — sequence defaults must use nextval('{name}')" + ), + }); + } + }; + Ok(Some(value)) +} + +/// Extract the sequence name from a `nextval('name')` DEFAULT. Shared with +/// the kv converter, which advances the registry on a different code path. +pub(crate) fn sequence_name(expr: &str) -> Option { + let (accessor, name) = nodedb_sql::planner::defaults::sequence_accessor(expr)?; + if accessor == nodedb_sql::planner::defaults::SequenceAccessor::Nextval { + Some(name) + } else { + None + } +} + +/// Materialize every missing column DEFAULT for one row, in order: +/// sequence accessors first (CP registry), then the pure evaluator. +/// Shared by the INSERT and UPSERT document-family paths so no statement +/// shape can silently drop a declared default (#294). +pub(crate) fn expand_row_defaults( + ctx: &ConvertContext, + row: &[(String, SqlValue)], + column_defaults: &[(String, String)], +) -> crate::Result> { + let mut expanded: Vec<(String, SqlValue)> = row.to_vec(); + for (col_name, default_expr) in column_defaults { + if expanded.iter().any(|(k, _)| k == col_name) { + continue; + } + let maybe_val = match evaluate_sequence_default(ctx, default_expr) { + Ok(Some(value)) => Some(value), + Ok(None) => { + super::evaluate_default_expr(default_expr).map_err(|e| crate::Error::PlanError { + detail: format!("default for column '{col_name}': {e}"), + })? + } + Err(e) => return Err(e), + }; + if let Some(val) = maybe_val { + expanded.push((col_name.clone(), super::convert::nodedb_value_to_sql(val))); + } + } + Ok(expanded) +} diff --git a/nodedb/src/control/planner/sql_plan_convert/visitor/arms_dml.rs b/nodedb/src/control/planner/sql_plan_convert/visitor/arms_dml.rs index 93f937107..617131cfb 100644 --- a/nodedb/src/control/planner/sql_plan_convert/visitor/arms_dml.rs +++ b/nodedb/src/control/planner/sql_plan_convert/visitor/arms_dml.rs @@ -66,6 +66,8 @@ macro_rules! impl_dml_arms_for_convert_visitor { ttl_secs: u64, intent: nodedb_sql::types::plan::KvInsertIntent, on_conflict_updates: &[(String, nodedb_sql::types_expr::SqlExpr)], + key_column: &str, + sequence_defaults: &[(String, String)], ) -> crate::Result> { super::super::dml::convert_kv_insert( collection, @@ -73,6 +75,8 @@ macro_rules! impl_dml_arms_for_convert_visitor { ttl_secs, intent, on_conflict_updates, + key_column, + sequence_defaults, self.tenant_id, self.ctx, ) diff --git a/nodedb/tests/inproc/cases/executor_tests/test_group_by_alias.rs b/nodedb/tests/inproc/cases/executor_tests/test_group_by_alias.rs index 3b1cab4c5..93d2041c9 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_group_by_alias.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_group_by_alias.rs @@ -75,6 +75,7 @@ fn sql_to_physical(sql: &str) -> PhysicalPlan { credentials: None, wal: None, surrogate_assigner: None, + sequence_registry: None, cluster_enabled: false, bitemporal_retention_registry: None, max_vector_dim: 0, diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 92c02d3bd..003a53151 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -139,6 +139,8 @@ mod schema_visibility_barrier; mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; +mod sequence_default_all_engines; +mod sequence_default_typed; mod serial_sequence_rollback_no_leak; mod session_handle_security; mod session_plan_cache_permission_tree_revoke; diff --git a/nodedb/tests/wire/cases/sequence_default_all_engines.rs b/nodedb/tests/wire/cases/sequence_default_all_engines.rs new file mode 100644 index 000000000..bcaacac88 --- /dev/null +++ b/nodedb/tests/wire/cases/sequence_default_all_engines.rs @@ -0,0 +1,226 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Sequence-backed DEFAULT expressions across the remaining engines. +//! +//! Part 1 (this PR's base) wired `DEFAULT nextval('name')` on the +//! doc-family insert path (typed engines verified: strict fills 1,2,3). +//! These tests pin the same guarantee on the kv and columnar families, +//! whose default-fill paths still use the pure stateless evaluator: +//! a DDL-accepted sequence DEFAULT must advance the CP-side registry per +//! row — never silently become NULL. + +use crate::harness::TestServer; + +/// kv keys the row on the DEFAULT'd column: the default must materialize +/// the key from the sequence before the key slot is built. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_default_nextval_fills_key() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE kvseq").await.unwrap(); + server + .exec( + "CREATE COLLECTION kvn (k BIGINT DEFAULT nextval('kvseq') PRIMARY KEY, v TEXT) \ + WITH (engine = 'kv')", + ) + .await + .unwrap(); + + server + .exec("INSERT INTO kvn (v) VALUES ('one')") + .await + .unwrap(); + server + .exec("INSERT INTO kvn (v) VALUES ('two')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT k, v FROM kvn ORDER BY k") + .await + .expect("rows readable"); + assert_eq!(rows.len(), 2, "{rows:?}"); + assert_eq!( + rows.iter().map(|r| r.get("k")).collect::>(), + vec![Some(&"1".to_string()), Some(&"2".to_string())], + "kv keys must be 1,2: {rows:?}" + ); +} + +/// columnar routes defaults through the batch rows encoder: the sequence +/// default must fill per row there too. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn columnar_default_nextval_fills_id() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE colseq").await.unwrap(); + server + .exec( + "CREATE COLLECTION coln (id BIGINT DEFAULT nextval('colseq') PRIMARY KEY, v TEXT) \ + WITH (engine = 'columnar')", + ) + .await + .unwrap(); + + server + .exec("INSERT INTO coln (v) VALUES ('one')") + .await + .unwrap(); + server + .exec("INSERT INTO coln (v) VALUES ('two')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT id, v FROM coln ORDER BY id") + .await + .expect("rows readable"); + assert_eq!(rows.len(), 2, "{rows:?}"); + assert_eq!( + rows.iter().map(|r| r.get("id")).collect::>(), + vec![Some(&"1".to_string()), Some(&"2".to_string())], + "columnar ids must be 1,2: {rows:?}" + ); +} + +/// Unknown sequence on kv must raise loudly, never silently NULL the key. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_default_unknown_sequence_raises() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION kbad (k BIGINT DEFAULT nextval('no_kv_seq') PRIMARY KEY, v TEXT) \ + WITH (engine = 'kv')", + ) + .await + .unwrap(); + + let err = server + .exec("INSERT INTO kbad (v) VALUES ('x')") + .await + .unwrap_err(); + assert!( + err.contains("no_kv_seq"), + "error must name the missing sequence: {err}" + ); +} + +/// The #294 repro engine: schemaless document with a declared column list. +/// Its DEFAULTs are currently dropped at the catalog adapter (default: None +/// hardcoded for every schemaless column) — these pin the storage-layer fix. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn document_default_nextval_fills_id() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE docseq").await.unwrap(); + server + .exec("CREATE COLLECTION docn (id BIGINT DEFAULT nextval('docseq') PRIMARY KEY, v TEXT)") + .await + .unwrap(); + + server + .exec("INSERT INTO docn (v) VALUES ('one')") + .await + .unwrap(); + server + .exec("INSERT INTO docn (v) VALUES ('two')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT id, v FROM docn ORDER BY id") + .await + .expect("rows readable"); + assert_eq!(rows.len(), 2, "{rows:?}"); + assert_eq!( + rows.iter().map(|r| r.get("id")).collect::>(), + vec![Some(&"1".to_string()), Some(&"2".to_string())], + "document ids must be 1,2: {rows:?}" + ); +} + +/// Stateless defaults (uuid) are part of the same storage layer: the adapter +/// must not drop them either. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn document_uuid_default_fills_id() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION docu (id UUID DEFAULT uuid_v7() PRIMARY KEY, v TEXT)") + .await + .unwrap(); + + server + .exec("INSERT INTO docu (v) VALUES ('a')") + .await + .unwrap(); + let rows = server + .query_named_rows("SELECT id, v FROM docu") + .await + .expect("rows readable"); + let id = rows[0].get("id").expect("id filled"); + assert_eq!(id.len(), 36, "uuid_v7 must fill the id: {rows:?}"); +} + +/// UPSERT shares the INSERT default path: omitting a defaulted column on a +/// document UPSERT must materialize the default, not drop it. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn document_upsert_default_nextval_fills_id() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE upseq").await.unwrap(); + server + .exec("CREATE COLLECTION upn (id BIGINT DEFAULT nextval('upseq') PRIMARY KEY, v TEXT)") + .await + .unwrap(); + + server + .exec("UPSERT INTO upn (v) VALUES ('one')") + .await + .unwrap(); + server + .exec("UPSERT INTO upn (v) VALUES ('two')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT id, v FROM upn ORDER BY id") + .await + .expect("rows readable"); + assert_eq!(rows.len(), 2, "{rows:?}"); + assert_eq!( + rows.iter().map(|r| r.get("id")).collect::>(), + vec![Some(&"1".to_string()), Some(&"2".to_string())], + "upsert ids must be 1,2: {rows:?}" + ); +} + +/// currval/setval have no per-row meaning in a DEFAULT: they must raise +/// loudly on every engine, never silently NULL the column (#294 family). +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn document_currval_default_raises_not_null() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE cqseq").await.unwrap(); + server + .exec("CREATE COLLECTION cqn (id BIGINT DEFAULT currval('cqseq') PRIMARY KEY, v TEXT)") + .await + .unwrap(); + let err = server + .exec("INSERT INTO cqn (v) VALUES ('x')") + .await + .unwrap_err(); + assert!( + err.contains("nextval"), + "currval DEFAULT must point to nextval, not silently NULL: {err}" + ); +} + +/// Malformed accessor bodies (empty name) must raise, not fall back. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn malformed_nextval_default_raises() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION badn (id BIGINT DEFAULT nextval('') PRIMARY KEY, v TEXT)") + .await + .unwrap(); + let err = server + .exec("INSERT INTO badn (v) VALUES ('x')") + .await + .unwrap_err(); + assert!(!err.is_empty(), "must raise, never silently NULL"); +} diff --git a/nodedb/tests/wire/cases/sequence_default_typed.rs b/nodedb/tests/wire/cases/sequence_default_typed.rs new file mode 100644 index 000000000..da948cbd0 --- /dev/null +++ b/nodedb/tests/wire/cases/sequence_default_typed.rs @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Sequence-backed DEFAULT expressions on typed engines. +//! +//! `DEFAULT nextval('name')` passed DDL and then silently produced NULL on +//! every insert (#294): the pure default evaluator does not know the +//! sequence accessors, so the expression evaluated to "no value" and the +//! column was omitted. The sequence machinery itself exists CP-side +//! (`SequenceRegistry`); these tests pin the wired path — the default +//! advances the sequence per row on a typed engine, and an unknown sequence +//! raises loudly instead of vanishing into NULL. + +use crate::harness::TestServer; + +/// document_strict stores column defaults; the default must advance the +/// sequence once per inserted row and never produce NULL. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_default_nextval_fills_distinct_ids() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE seqdefault").await.unwrap(); + server + .exec( + "CREATE COLLECTION sst (id BIGINT DEFAULT nextval('seqdefault') PRIMARY KEY, v TEXT) \ + WITH (engine = 'document_strict')", + ) + .await + .unwrap(); + + server + .exec("INSERT INTO sst (v) VALUES ('one')") + .await + .unwrap(); + server + .exec("INSERT INTO sst (v) VALUES ('two')") + .await + .unwrap(); + server + .exec("INSERT INTO sst (v) VALUES ('three')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT id, v FROM sst ORDER BY id") + .await + .expect("rows readable"); + assert_eq!(rows.len(), 3, "{rows:?}"); + let ids: Vec> = rows.iter().map(|r| r.get("id")).collect(); + assert_eq!( + ids, + vec![ + Some(&"1".to_string()), + Some(&"2".to_string()), + Some(&"3".to_string()) + ], + "ids must advance 1..3, got {rows:?}" + ); +} + +/// A DEFAULT naming an unknown sequence must raise at insert time — the +/// DDL-accepted expression must never silently become NULL. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_default_unknown_sequence_raises() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION bad (id BIGINT DEFAULT nextval('no_such_seq') PRIMARY KEY, v TEXT) \ + WITH (engine = 'document_strict')", + ) + .await + .unwrap(); + + let err = server + .exec("INSERT INTO bad (v) VALUES ('x')") + .await + .unwrap_err(); + assert!( + err.contains("no_such_seq"), + "error must name the missing sequence: {err}" + ); + assert!( + !err.contains("NULL") && !err.is_empty(), + "must not silently NULL: {err}" + ); +} + +/// Sentinel: stateless defaults (uuid) keep working on typed engines +/// alongside the new sequence route. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_uuid_default_still_fills() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION ud (id UUID DEFAULT uuid_v7() PRIMARY KEY, v TEXT) \ + WITH (engine = 'document_strict')", + ) + .await + .unwrap(); + + server + .exec("INSERT INTO ud (v) VALUES ('a')") + .await + .unwrap(); + let rows = server + .query_named_rows("SELECT id, v FROM ud") + .await + .expect("rows readable"); + let id = rows[0].get("id").expect("id filled"); + assert_eq!(id.len(), 36, "uuid_v7 must fill the id: {rows:?}"); +} From d8d3f316cc0e0f7664917e299073fe3cbc437e4a Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:09:21 +0800 Subject: [PATCH 2/9] classify sequence accessors in expression contexts as 0A000 Register nextval, currval and setval so the planner admits and types them, then make every evaluation that escapes the DEFAULT path fail loudly: the scalar evaluator raises a typed FeatureNotSupported error, const folding classifies it exhaustively, and the error plumbing maps it to SQLSTATE 0A000 end to end. Executor lanes that collapsed any evaluation error to division-by-zero now discriminate variants, so the typed code survives to the client. Adds the expression-context wire coverage, a parser corpus locking the tolerant byte-safe accessor recogniser, and a static gate script that keeps the DEFAULT-plane seam honest. --- .github/workflows/static-gates.yml | 2 + .../src/rpc_codec/data_plane_error.rs | 4 + nodedb-query/src/expr/eval.rs | 10 ++ nodedb-query/src/functions/eval.rs | 36 ++++ nodedb-sql/src/error.rs | 9 + nodedb-sql/src/functions/builtins/scalars.rs | 2 + .../functions/builtins/scalars/sequence.rs | 85 +++++++++ nodedb-sql/src/planner/const_fold.rs | 5 + nodedb-sql/src/planner/defaults.rs | 90 ++++++++++ nodedb-types/src/error/code.rs | 3 + nodedb-types/src/error/code_table.rs | 1 + nodedb-types/src/error/ctors/mod.rs | 1 + .../src/error/ctors/sequence_accessor.rs | 26 +++ nodedb-types/src/error/details.rs | 4 + nodedb-types/src/error/msgpack/constants.rs | 1 + .../error/msgpack/decode/from_messagepack.rs | 4 + nodedb-types/src/error/msgpack/encode.rs | 3 + nodedb/src/bridge/envelope/error_code.rs | 5 + .../control/cluster/data_plane_error_wire.rs | 2 + .../control/planner/context/query/planning.rs | 3 + .../server/dispatch_utils/write_abort.rs | 5 +- .../control/server/pgwire/types/error_map.rs | 3 + .../src/control/server/shared/ddl/sqlstate.rs | 7 + .../data/executor/handlers/aggregate/exec.rs | 5 +- .../executor/handlers/document/read/scan.rs | 4 +- .../executor/handlers/grouping_sets_exec.rs | 14 +- .../executor/handlers/kv/predicate/matches.rs | 2 +- nodedb/src/data/executor/handlers/kv/scan.rs | 4 +- .../data/executor/handlers/provider_scan.rs | 4 +- nodedb/src/error/types.rs | 10 ++ nodedb/src/error_classify.rs | 1 + nodedb/src/error_from.rs | 3 + nodedb/src/error_from_data_plane.rs | 1 + nodedb/src/storage/cold_filter.rs | 4 +- nodedb/tests/wire/cases/mod.rs | 1 + .../cases/sequence_expression_contexts.rs | 161 ++++++++++++++++++ scripts/ci/check_sequence_plane.py | 109 ++++++++++++ 37 files changed, 616 insertions(+), 18 deletions(-) create mode 100644 nodedb-sql/src/functions/builtins/scalars/sequence.rs create mode 100644 nodedb-types/src/error/ctors/sequence_accessor.rs create mode 100644 nodedb/tests/wire/cases/sequence_expression_contexts.rs create mode 100644 scripts/ci/check_sequence_plane.py diff --git a/.github/workflows/static-gates.yml b/.github/workflows/static-gates.yml index 610ba7f82..823d0f2de 100644 --- a/.github/workflows/static-gates.yml +++ b/.github/workflows/static-gates.yml @@ -55,3 +55,5 @@ jobs: run: | python3 scripts/ci/check_advisory_ignores.py --self-test python3 scripts/ci/check_advisory_ignores.py + - name: Sequence-plane gate + run: python3 scripts/ci/check_sequence_plane.py diff --git a/nodedb-cluster/src/rpc_codec/data_plane_error.rs b/nodedb-cluster/src/rpc_codec/data_plane_error.rs index 2309d072a..c22879e2d 100644 --- a/nodedb-cluster/src/rpc_codec/data_plane_error.rs +++ b/nodedb-cluster/src/rpc_codec/data_plane_error.rs @@ -112,4 +112,8 @@ pub enum DataPlaneErrorCode { limit: u64, }, DivisionByZero, + /// Registered sequence accessor reached expression evaluation (0A000). + FeatureNotSupported { + name: String, + }, } diff --git a/nodedb-query/src/expr/eval.rs b/nodedb-query/src/expr/eval.rs index 04f60e2d6..3870b8594 100644 --- a/nodedb-query/src/expr/eval.rs +++ b/nodedb-query/src/expr/eval.rs @@ -27,6 +27,16 @@ use super::types::SqlExpr; pub enum EvalError { #[error("division by zero")] DivisionByZero, + /// Sequence accessors (`nextval`/`currval`/`setval`) are stateful and + /// CP-side only. They are evaluated as column DEFAULTs by the plan + /// converter, never by the row-scope scalar evaluator. Reaching this + /// error means an accessor escaped the DEFAULT path (e.g. a bare + /// `SELECT nextval('s')`), which must surface loudly as 0A000 — never as + /// a silent `Null`. + #[error( + "sequence accessors are supported as column DEFAULTs (DEFAULT nextval('s')); SELECT-time evaluation is not yet wired" + )] + FeatureNotSupported { name: &'static str }, } /// Row scope for `SqlExpr::eval_scope`: how `Column(..)` and `OldColumn(..)` diff --git a/nodedb-query/src/functions/eval.rs b/nodedb-query/src/functions/eval.rs index 636c70aac..c30e02fe6 100644 --- a/nodedb-query/src/functions/eval.rs +++ b/nodedb-query/src/functions/eval.rs @@ -20,6 +20,19 @@ use super::{array, conditional, datetime, fts, id, json, math, string, system, t /// fallible arm doesn't force every scalar-function module to carry a /// `Result` it can never actually produce. pub fn eval_function(name: &str, args: &[Value]) -> Result { + // Sequence accessors are stateful (CP-side, DEFAULT-scoped). They are + // never scalar-evaluable: if one reaches this dispatcher, it escaped the + // DEFAULT path and must raise 0A000 instead of falling through to the + // geo fallback's silent `Null`. + let canonical = match name.to_ascii_lowercase().as_str() { + "nextval" => Some("nextval"), + "currval" => Some("currval"), + "setval" => Some("setval"), + _ => None, + }; + if let Some(cname) = canonical { + return Err(EvalError::FeatureNotSupported { name: cname }); + } if let Some(v) = string::try_eval(name, args) { return Ok(v); } @@ -63,6 +76,29 @@ mod tests { eval_function(name, &args).unwrap() } + #[test] + fn sequence_accessors_are_loud_not_null() { + // Regression: accessors used to fall through to the geo fallback and + // return Ok(Null). They must error as FeatureNotSupported (0A000). + for name in ["nextval", "currval", "setval", "NEXTVAL"] { + let err = eval_function(name, &[Value::String("s".into())]).unwrap_err(); + assert!( + matches!(err, crate::expr::EvalError::FeatureNotSupported { .. }), + "{name} must raise FeatureNotSupported, got {err:?}" + ); + } + } + + #[test] + fn non_sequence_unknown_still_nulls() { + // The guard is scoped: unknown non-sequence names keep the legacy + // geo-fallback behaviour (Ok(Null)), not an error. + assert_eq!( + eval_function("definitely_not_a_fn", &[]).unwrap(), + Value::Null + ); + } + #[test] fn mod_by_zero_errors() { let err = eval_function("mod", &[Value::Integer(5), Value::Integer(0)]).unwrap_err(); diff --git a/nodedb-sql/src/error.rs b/nodedb-sql/src/error.rs index b31a34c5b..896efb75b 100644 --- a/nodedb-sql/src/error.rs +++ b/nodedb-sql/src/error.rs @@ -19,6 +19,15 @@ pub enum SqlError { #[error("function {name}(...) does not exist")] UndefinedFunction { name: String }, + /// A registered sequence accessor (`nextval`/`currval`/`setval`) was + /// const-folded or row-evaluated. Accessors are stateful and CP-side + /// only — valid as column DEFAULTs, invalid in any SQL expression + /// context. Maps to SQLSTATE 0A000 (feature not supported). + #[error( + "sequence accessors are supported as column DEFAULTs (DEFAULT nextval('s')); SELECT-time evaluation is not yet wired" + )] + FeatureNotSupported { name: String }, + #[error("unknown column '{column}' in table '{table}'")] UnknownColumn { table: String, column: String }, diff --git a/nodedb-sql/src/functions/builtins/scalars.rs b/nodedb-sql/src/functions/builtins/scalars.rs index f73556d45..ac37ddeaf 100644 --- a/nodedb-sql/src/functions/builtins/scalars.rs +++ b/nodedb-sql/src/functions/builtins/scalars.rs @@ -12,6 +12,7 @@ mod math; mod misc; mod pg_fts; mod pg_json; +mod sequence; mod spatial; mod string; mod vector; @@ -22,6 +23,7 @@ pub(super) fn scalar_functions() -> Vec { let mut fns = Vec::new(); fns.extend(vector::vector_functions()); fns.extend(spatial::spatial_functions()); + fns.extend(sequence::sequence_functions()); fns.extend(datetime::datetime_functions()); fns.extend(doc::doc_functions()); fns.extend(string::string_functions()); diff --git a/nodedb-sql/src/functions/builtins/scalars/sequence.rs b/nodedb-sql/src/functions/builtins/scalars/sequence.rs new file mode 100644 index 000000000..937612a48 --- /dev/null +++ b/nodedb-sql/src/functions/builtins/scalars/sequence.rs @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Sequence accessor registrations (`nextval`/`currval`/`setval`). +//! +//! These names are stateful and CP-side only: they evaluate as column +//! DEFAULTs via the plan converter, never as SQL-expression scalars. The +//! plan-time existence gate still needs them registered so that a bare +//! expression use is planned (typed, arity-checked) and then fails LOUDLY +//! at fold/row-eval time with 0A000 — instead of being rejected as +//! "function does not exist" (42883) or silently NULLing at runtime. +//! +//! This list must stay in sync with the sequence guard arm in +//! `nodedb_query::functions::eval_function`. + +use nodedb_types::columnar::ColumnType; + +use crate::functions::arg_types; +use crate::functions::registry::{ArgTypeSpec, FunctionCategory::Scalar, FunctionMeta}; + +use super::super::helpers::{m, no_trigger}; + +static SEQ_1_ARGS: &[ArgTypeSpec] = &[arg_types::any("seq")]; +static SEQ_2_ARGS: &[ArgTypeSpec] = &[arg_types::any("seq"), arg_types::any("value")]; + +pub(super) fn sequence_functions() -> Vec { + vec![ + m( + "nextval", + Scalar, + 1, + 1, + no_trigger(), + Some(ColumnType::Int64), + SEQ_1_ARGS, + ), + m( + "currval", + Scalar, + 1, + 1, + no_trigger(), + Some(ColumnType::Int64), + SEQ_1_ARGS, + ), + m( + "setval", + Scalar, + 2, + 2, + no_trigger(), + Some(ColumnType::Int64), + SEQ_2_ARGS, + ), + ] +} + +#[cfg(test)] +mod tests { + use crate::functions::registry::FunctionRegistry; + + #[test] + fn accessors_registered_for_plan_gate() { + let reg = FunctionRegistry::new(); + for name in ["nextval", "currval", "setval"] { + assert!(reg.lookup(name).is_some(), "{name} must be registered"); + } + } + + #[test] + fn accessors_error_loudly_in_expression_eval() { + // A1+A3 contract: registered, arity-checked, then loud 0A000 at + // fold/row-eval — never a silent Null (parity invariant seed). + for name in ["nextval", "currval"] { + let err = nodedb_query::functions::eval_function( + name, + &[nodedb_types::Value::String("s".into())], + ) + .unwrap_err(); + assert!( + matches!(err, nodedb_query::EvalError::FeatureNotSupported { .. }), + "{name} must raise FeatureNotSupported, got {err:?}" + ); + } + } +} diff --git a/nodedb-sql/src/planner/const_fold.rs b/nodedb-sql/src/planner/const_fold.rs index 8f94a81ea..d7109fcdd 100644 --- a/nodedb-sql/src/planner/const_fold.rs +++ b/nodedb-sql/src/planner/const_fold.rs @@ -342,6 +342,11 @@ pub fn fold_function_call(name: &str, args: &[SqlExpr], registry: &FunctionRegis match nodedb_query::functions::eval_function(&name.to_lowercase(), &folded_args) { Ok(result) => Ok(Some(ndb_to_sql_value(result))), Err(nodedb_query::EvalError::DivisionByZero) => Err(SqlError::DivisionByZero), + Err(nodedb_query::EvalError::FeatureNotSupported { name }) => { + Err(SqlError::FeatureNotSupported { + name: name.to_string(), + }) + } } } diff --git a/nodedb-sql/src/planner/defaults.rs b/nodedb-sql/src/planner/defaults.rs index 59bcdd8ac..6660b66b7 100644 --- a/nodedb-sql/src/planner/defaults.rs +++ b/nodedb-sql/src/planner/defaults.rs @@ -178,3 +178,93 @@ pub fn sequence_accessor(expr: &str) -> Option<(SequenceAccessor, String)> { } Some((accessor, name.to_string())) } + +#[cfg(test)] +mod sequence_accessor_corpus { + use super::{SequenceAccessor, looks_like_sequence_accessor, sequence_accessor}; + + fn name(expr: &str) -> Option { + sequence_accessor(expr).map(|(_, n)| n) + } + + fn acc(expr: &str) -> Option { + sequence_accessor(expr).map(|(a, _)| a) + } + + #[test] + fn canonical_forms() { + assert_eq!(name("nextval('sq')").as_deref(), Some("sq")); + assert_eq!(acc("nextval('sq')"), Some(SequenceAccessor::Nextval)); + assert_eq!(acc("currval('sq')"), Some(SequenceAccessor::Currval)); + assert_eq!(acc("setval('sq')"), Some(SequenceAccessor::Setval)); + } + + #[test] + fn case_and_quote_variants() { + // ASCII case-insensitive on the accessor; name bytes preserved. + assert_eq!(name("NEXTVAL('MySeq')").as_deref(), Some("MySeq")); + assert_eq!(name("Currval('c')").as_deref(), Some("c")); + // Double-quoted names are recognized too. + assert_eq!(name("nextval(\"dq\")").as_deref(), Some("dq")); + // Whitespace inside the parens is tolerated. + assert_eq!(name("nextval( 'padded' )").as_deref(), Some("padded")); + } + + #[test] + fn tolerant_raw_name_handling() { + // Embedded quotes are preserved raw (byte-safe) — never sliced + // against a case-folded copy, never panicked. + assert_eq!(name("nextval('a''b')").as_deref(), Some("a''b")); + // Unicode names survive untouched. + assert_eq!( + name("nextval('sekuensi\u{1F600}')").as_deref(), + Some("sekuensi\u{1F600}") + ); + // Bare identifier (unquoted) is tolerated by the recognizer; the + // registry/convert layers decide loudness afterwards. + assert_eq!(name("nextval(sq)").as_deref(), Some("sq")); + } + + #[test] + fn non_accessor_shapes_are_rejected() { + assert_eq!(name("nextval('')"), None, "empty name is malformed"); + // Tolerant by design: extra args ride along in the raw name and the + // convert layer raises on registry lookup — still loud, never + // silent. (Byte-safe: no slice against a case-folded copy.) + assert_eq!( + acc("nextval('s', 'x')"), + Some(SequenceAccessor::Nextval), + "two-arg form is tolerated and resolved loud later" + ); + assert_eq!( + name("nextval('s')::text"), + None, + "cast wrapper is not raw accessor" + ); + assert_eq!(acc("lastval('s')"), None); + assert_eq!(acc("nextvalx('s')"), None); + assert_eq!(acc("xnextval('s')"), None); + assert_eq!( + acc("nextval ('s')"), + None, + "space before paren is not a call" + ); + assert_eq!(acc("nextval"), None); + assert_eq!(acc(""), None); + assert_eq!( + acc("'nextval('s')'"), + None, + "quoted string literal is not a call" + ); + } + + #[test] + fn looks_like_matches_only_call_prefix() { + assert!(looks_like_sequence_accessor("nextval('x')")); + assert!(looks_like_sequence_accessor("SETVAL( 'x' )")); + assert!(!looks_like_sequence_accessor("nextvalx('x')")); + assert!(!looks_like_sequence_accessor("xnextval('x')")); + assert!(!looks_like_sequence_accessor("nextval ('x')")); + assert!(!looks_like_sequence_accessor("nextval")); + } +} diff --git a/nodedb-types/src/error/code.rs b/nodedb-types/src/error/code.rs index e689c0263..f11c03fe4 100644 --- a/nodedb-types/src/error/code.rs +++ b/nodedb-types/src/error/code.rs @@ -57,6 +57,9 @@ impl ErrorCode { pub const UNDEFINED_FUNCTION: Self = Self(1203); /// Expression evaluation divided or took a modulus by zero. pub const DIVISION_BY_ZERO: Self = Self(1204); + /// A registered sequence accessor was evaluated in a SQL expression + /// context (stateful accessors are DEFAULT-scoped only). SQLSTATE 0A000. + pub const FEATURE_NOT_SUPPORTED: Self = Self(1208); /// A LIMIT/OFFSET/FETCH bound resolved outside `[0, usize::MAX]`. pub const INVALID_LIMIT_VALUE: Self = Self(1205); /// A column reference names no column of any relation in scope. diff --git a/nodedb-types/src/error/code_table.rs b/nodedb-types/src/error/code_table.rs index e319e5a59..f16a93c93 100644 --- a/nodedb-types/src/error/code_table.rs +++ b/nodedb-types/src/error/code_table.rs @@ -90,6 +90,7 @@ error_code_table! { UNDEFINED_COLUMN => UndefinedColumn { column: String::new() }, AMBIGUOUS_COLUMN => AmbiguousColumn { column: String::new() }, DIVISION_BY_ZERO => DivisionByZero, + FEATURE_NOT_SUPPORTED => FeatureNotSupported { name: String::new() }, INVALID_LIMIT_VALUE => InvalidLimitValue { clause: "remote".into(), value: message.to_owned() }, // Auth / tenant quota. diff --git a/nodedb-types/src/error/ctors/mod.rs b/nodedb-types/src/error/ctors/mod.rs index 176fd5558..a18ebcc84 100644 --- a/nodedb-types/src/error/ctors/mod.rs +++ b/nodedb-types/src/error/ctors/mod.rs @@ -19,5 +19,6 @@ pub mod mirror; pub mod move_tenant; pub mod read_query_auth; pub mod remote_code; +pub mod sequence_accessor; pub mod sync_infra; pub mod write_path; diff --git a/nodedb-types/src/error/ctors/sequence_accessor.rs b/nodedb-types/src/error/ctors/sequence_accessor.rs new file mode 100644 index 000000000..e94105b58 --- /dev/null +++ b/nodedb-types/src/error/ctors/sequence_accessor.rs @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! `NodeDbError` constructors for sequence-accessor expression misuse. + +use crate::error::ErrorCode; +use crate::error::details::ErrorDetails; +use crate::error::types::NodeDbError; + +impl NodeDbError { + /// A registered sequence accessor (`nextval`/`currval`/`setval`) was + /// evaluated in a SQL expression context. Accessors are stateful and + /// CP-side only — valid as column DEFAULTs, invalid elsewhere. Distinct + /// from `plan_error` so clients match on the code (SQLSTATE `0A000`, + /// `feature_not_supported`) rather than parsing the message. + pub fn feature_not_supported(name: impl Into) -> Self { + let name = name.into(); + Self { + code: ErrorCode::FEATURE_NOT_SUPPORTED, + message: "sequence accessors are supported as column DEFAULTs \ + (DEFAULT nextval('s')); SELECT-time evaluation is not yet wired" + .to_string(), + details: ErrorDetails::FeatureNotSupported { name }, + cause: None, + } + } +} diff --git a/nodedb-types/src/error/details.rs b/nodedb-types/src/error/details.rs index c24285c3b..e3f90ad10 100644 --- a/nodedb-types/src/error/details.rs +++ b/nodedb-types/src/error/details.rs @@ -117,6 +117,10 @@ pub enum ErrorDetails { /// Expression evaluation divided or took a modulus by zero. #[serde(rename = "division_by_zero")] DivisionByZero, + /// A registered sequence accessor escaped the DEFAULT path and was + /// evaluated as a SQL-expression scalar (SQLSTATE 0A000). + #[serde(rename = "feature_not_supported")] + FeatureNotSupported { name: String }, /// A LIMIT/OFFSET/FETCH bound resolved outside `[0, usize::MAX]`. #[serde(rename = "invalid_limit_value")] InvalidLimitValue { clause: String, value: String }, diff --git a/nodedb-types/src/error/msgpack/constants.rs b/nodedb-types/src/error/msgpack/constants.rs index 2de2601e2..31c9184bf 100644 --- a/nodedb-types/src/error/msgpack/constants.rs +++ b/nodedb-types/src/error/msgpack/constants.rs @@ -165,3 +165,4 @@ pub(super) const TAG_CANNOT_DROP_DEFAULT_DATABASE: u16 = 77; pub(super) const TAG_INVALID_LIMIT_VALUE: u16 = 78; pub(super) const TAG_UNDEFINED_COLUMN: u16 = 79; pub(super) const TAG_AMBIGUOUS_COLUMN: u16 = 80; +pub(super) const TAG_FEATURE_NOT_SUPPORTED: u16 = 81; diff --git a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs index e0704fa86..b0250fbc5 100644 --- a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs +++ b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs @@ -146,6 +146,10 @@ impl<'a> FromMessagePack<'a> for ErrorDetails { skip_fields(reader, field_count)?; Ok(ErrorDetails::DivisionByZero) } + TAG_FEATURE_NOT_SUPPORTED => { + let (name,) = read1_str(reader, field_count)?; + Ok(ErrorDetails::FeatureNotSupported { name }) + } TAG_INVALID_LIMIT_VALUE => { let (clause, value) = read2_str(reader, field_count)?; Ok(ErrorDetails::InvalidLimitValue { clause, value }) diff --git a/nodedb-types/src/error/msgpack/encode.rs b/nodedb-types/src/error/msgpack/encode.rs index ab06a7882..557d5c819 100644 --- a/nodedb-types/src/error/msgpack/encode.rs +++ b/nodedb-types/src/error/msgpack/encode.rs @@ -167,6 +167,9 @@ impl ToMessagePack for ErrorDetails { write1(writer, TAG_AMBIGUOUS_COLUMN, column) } ErrorDetails::DivisionByZero => write_unit(writer, TAG_DIVISION_BY_ZERO), + ErrorDetails::FeatureNotSupported { name } => { + write1(writer, TAG_FEATURE_NOT_SUPPORTED, name) + } ErrorDetails::InvalidLimitValue { clause, value } => { write2(writer, TAG_INVALID_LIMIT_VALUE, clause, value) } diff --git a/nodedb/src/bridge/envelope/error_code.rs b/nodedb/src/bridge/envelope/error_code.rs index 3c10d7422..8f1dd76c9 100644 --- a/nodedb/src/bridge/envelope/error_code.rs +++ b/nodedb/src/bridge/envelope/error_code.rs @@ -123,6 +123,10 @@ pub enum ErrorCode { /// special-cases `NotFound`) and reaches the client as SQLSTATE `22012` /// rather than the generic `XX000` every `Internal` maps to. DivisionByZero, + /// A registered sequence accessor escaped the DEFAULT path and reached + /// expression evaluation. Crosses the Data Plane → pgwire boundary as + /// SQLSTATE `0A000` (`feature_not_supported`), never `XX000`. + FeatureNotSupported { name: String }, } impl From for ErrorCode { @@ -202,6 +206,7 @@ impl From for ErrorCode { Self::TxnOverlayMemoryExceeded { limit } } crate::Error::DivisionByZero => Self::DivisionByZero, + crate::Error::FeatureNotSupported { name } => Self::FeatureNotSupported { name }, crate::Error::UndefinedColumn { column } => Self::UndefinedColumn { column }, // Same condition an undefined column reports at plan time, raised // here by the strict encoder for a transport the planner never diff --git a/nodedb/src/control/cluster/data_plane_error_wire.rs b/nodedb/src/control/cluster/data_plane_error_wire.rs index 1d54be1a5..2fea2e5b1 100644 --- a/nodedb/src/control/cluster/data_plane_error_wire.rs +++ b/nodedb/src/control/cluster/data_plane_error_wire.rs @@ -137,6 +137,7 @@ impl From for DataPlaneErrorCode { limit: to_wire_count(limit), }, ErrorCode::DivisionByZero => Self::DivisionByZero, + ErrorCode::FeatureNotSupported { name } => Self::FeatureNotSupported { name }, } } } @@ -227,6 +228,7 @@ impl From for ErrorCode { } } DataPlaneErrorCode::DivisionByZero => Self::DivisionByZero, + DataPlaneErrorCode::FeatureNotSupported { name } => Self::FeatureNotSupported { name }, } } } diff --git a/nodedb/src/control/planner/context/query/planning.rs b/nodedb/src/control/planner/context/query/planning.rs index 7a4724187..7c945c29d 100644 --- a/nodedb/src/control/planner/context/query/planning.rs +++ b/nodedb/src/control/planner/context/query/planning.rs @@ -45,6 +45,9 @@ fn map_plan_error(error: nodedb_sql::SqlError, tenant_id: crate::types::TenantId // A constant expression that divides by zero is the same condition the // row-scope evaluator raises, so it carries the same code. nodedb_sql::SqlError::DivisionByZero => crate::Error::DivisionByZero, + nodedb_sql::SqlError::FeatureNotSupported { name } => { + crate::Error::FeatureNotSupported { name } + } nodedb_sql::SqlError::InvalidLimitValue { clause, value } => { crate::Error::InvalidLimitValue { clause, value } } diff --git a/nodedb/src/control/server/dispatch_utils/write_abort.rs b/nodedb/src/control/server/dispatch_utils/write_abort.rs index 484953ed1..354560339 100644 --- a/nodedb/src/control/server/dispatch_utils/write_abort.rs +++ b/nodedb/src/control/server/dispatch_utils/write_abort.rs @@ -145,7 +145,10 @@ pub(crate) fn write_definitely_not_applied(code: &ErrorCode) -> bool { | ErrorCode::TxnOverlayMemoryExceeded { .. } // Expression evaluation failed before producing a value to write. | ErrorCode::DivisionByZero - | ErrorCode::UndefinedColumn { .. } => true, + | ErrorCode::UndefinedColumn { .. } + // Sequence accessor evaluated as an expression (0A000) — the row + // produced no value, so the write was refused, not applied. + | ErrorCode::FeatureNotSupported { .. } => true, // NOT established — every one of these can be reported by a request // whose write reached, or may have reached, engine state. Emitting an diff --git a/nodedb/src/control/server/pgwire/types/error_map.rs b/nodedb/src/control/server/pgwire/types/error_map.rs index d4299d39c..7edec22c2 100644 --- a/nodedb/src/control/server/pgwire/types/error_map.rs +++ b/nodedb/src/control/server/pgwire/types/error_map.rs @@ -64,6 +64,9 @@ pub fn error_to_sqlstate(err: &crate::Error) -> (&'static str, &'static str, Str ("ERROR", sqlstate::UNDEFINED_COLUMN, err.to_string()) } crate::Error::DivisionByZero => ("ERROR", sqlstate::DIVISION_BY_ZERO, err.to_string()), + crate::Error::FeatureNotSupported { .. } => { + ("ERROR", sqlstate::FEATURE_NOT_SUPPORTED, err.to_string()) + } crate::Error::InvalidLimitValue { .. } => { ("ERROR", sqlstate::INVALID_LIMIT_VALUE, err.to_string()) } diff --git a/nodedb/src/control/server/shared/ddl/sqlstate.rs b/nodedb/src/control/server/shared/ddl/sqlstate.rs index 9230a4d00..ab10b731a 100644 --- a/nodedb/src/control/server/shared/ddl/sqlstate.rs +++ b/nodedb/src/control/server/shared/ddl/sqlstate.rs @@ -174,6 +174,13 @@ pub fn error_code_to_sqlstate(code: &ErrorCode) -> (&'static str, &'static str, sqlstate::DIVISION_BY_ZERO, "division by zero".into(), ), + ErrorCode::FeatureNotSupported { name } => ( + "ERROR", + sqlstate::FEATURE_NOT_SUPPORTED, + format!( + "sequence accessors are supported as column DEFAULTs (DEFAULT nextval('s')); SELECT-time evaluation is not yet wired ({name})" + ), + ), ErrorCode::Unsupported { detail } => { ("ERROR", sqlstate::FEATURE_NOT_SUPPORTED, detail.clone()) } diff --git a/nodedb/src/data/executor/handlers/aggregate/exec.rs b/nodedb/src/data/executor/handlers/aggregate/exec.rs index 6629f5c7a..7e7231742 100644 --- a/nodedb/src/data/executor/handlers/aggregate/exec.rs +++ b/nodedb/src/data/executor/handlers/aggregate/exec.rs @@ -273,8 +273,9 @@ impl CoreLoop { } } }); - if predicate_err.take().is_some() { - return self.response_error(task, ErrorCode::DivisionByZero); + if let Some(e) = predicate_err.take() { + return self + .response_error(task, ErrorCode::from(crate::Error::from(e))); } } } diff --git a/nodedb/src/data/executor/handlers/document/read/scan.rs b/nodedb/src/data/executor/handlers/document/read/scan.rs index 18bc51455..cdd7b239e 100644 --- a/nodedb/src/data/executor/handlers/document/read/scan.rs +++ b/nodedb/src/data/executor/handlers/document/read/scan.rs @@ -201,8 +201,8 @@ impl CoreLoop { } }; self.merge_overlay_into_scan(txn_id, &coll_key, &mut filtered, &matches); - if predicate_err.take().is_some() { - return self.response_error(task, ErrorCode::DivisionByZero); + if let Some(e) = predicate_err.take() { + return self.response_error(task, ErrorCode::from(crate::Error::from(e))); } } diff --git a/nodedb/src/data/executor/handlers/grouping_sets_exec.rs b/nodedb/src/data/executor/handlers/grouping_sets_exec.rs index 1a3690fda..4b54bff1d 100644 --- a/nodedb/src/data/executor/handlers/grouping_sets_exec.rs +++ b/nodedb/src/data/executor/handlers/grouping_sets_exec.rs @@ -152,16 +152,18 @@ pub(super) fn execute_grouping_sets( match ScanFilter::all_match_binary_indexed(&filter_predicates, raw, &idx) { Ok(true) => {} Ok(false) => continue, - Err(_e) => { - return core.response_error(task, ErrorCode::DivisionByZero); + Err(e) => { + return core + .response_error(task, ErrorCode::from(crate::Error::from(e))); } } } else { match ScanFilter::all_match_binary(&filter_predicates, raw) { Ok(true) => {} Ok(false) => continue, - Err(_e) => { - return core.response_error(task, ErrorCode::DivisionByZero); + Err(e) => { + return core + .response_error(task, ErrorCode::from(crate::Error::from(e))); } } } @@ -193,8 +195,8 @@ pub(super) fn execute_grouping_sets( } } Ok(false) => {} - Err(_e) => { - return core.response_error(task, ErrorCode::DivisionByZero); + Err(e) => { + return core.response_error(task, ErrorCode::from(crate::Error::from(e))); } } } diff --git a/nodedb/src/data/executor/handlers/kv/predicate/matches.rs b/nodedb/src/data/executor/handlers/kv/predicate/matches.rs index 8b7d07fac..74b9bb00c 100644 --- a/nodedb/src/data/executor/handlers/kv/predicate/matches.rs +++ b/nodedb/src/data/executor/handlers/kv/predicate/matches.rs @@ -65,7 +65,7 @@ impl CoreLoop { match ScanFilter::all_match_binary(&predicates, &row) { Ok(true) => {} Ok(false) => continue, - Err(_e) => return Err(ErrorCode::DivisionByZero), + Err(e) => return Err(ErrorCode::from(crate::Error::from(e))), } } matched.push((key, value)); diff --git a/nodedb/src/data/executor/handlers/kv/scan.rs b/nodedb/src/data/executor/handlers/kv/scan.rs index 188f489b3..e30d34e71 100644 --- a/nodedb/src/data/executor/handlers/kv/scan.rs +++ b/nodedb/src/data/executor/handlers/kv/scan.rs @@ -139,8 +139,8 @@ impl CoreLoop { ) { Ok(true) => {} Ok(false) => continue, - Err(_e) => { - return self.response_error(task, ErrorCode::DivisionByZero); + Err(e) => { + return self.response_error(task, ErrorCode::from(crate::Error::from(e))); } } } diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index b48f92e9d..9ebf4d1d8 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -86,8 +86,8 @@ impl CoreLoop { } } }); - if predicate_err.take().is_some() { - return self.response_error(task, ErrorCode::DivisionByZero); + if let Some(e) = predicate_err.take() { + return self.response_error(task, ErrorCode::from(crate::Error::from(e))); } } } diff --git a/nodedb/src/error/types.rs b/nodedb/src/error/types.rs index ae23847b2..b9b7fe246 100644 --- a/nodedb/src/error/types.rs +++ b/nodedb/src/error/types.rs @@ -306,6 +306,16 @@ pub enum Error { #[error("division by zero")] DivisionByZero, + /// A registered sequence accessor (`nextval`/`currval`/`setval`) was + /// evaluated in a SQL expression context (fold or row scope). + /// Accessors are stateful and CP-side only — valid as column DEFAULTs. + /// Rendered as SQLSTATE `0A000` (feature_not_supported) at the pgwire + /// layer. + #[error( + "sequence accessors are supported as column DEFAULTs (DEFAULT nextval('s')); SELECT-time evaluation is not yet wired" + )] + FeatureNotSupported { name: String }, + /// A LIMIT/OFFSET/FETCH bound did not resolve to `[0, usize::MAX]`. /// The pgwire layer renders it as SQLSTATE `2201W`. #[error("invalid {clause} value: {value}")] diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index 3a853e9a1..cfc2de4e0 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -154,6 +154,7 @@ pub(crate) fn classify(e: &Error) -> NodeDbError { Error::AmbiguousColumn { column } => NodeDbError::ambiguous_column(column.clone()), Error::UnknownStrictField { column, .. } => NodeDbError::undefined_column(column.clone()), Error::DivisionByZero => NodeDbError::division_by_zero(), + Error::FeatureNotSupported { name } => NodeDbError::feature_not_supported(name.clone()), Error::InvalidLimitValue { clause, value } => { NodeDbError::invalid_limit_value(*clause, value.clone()) } diff --git a/nodedb/src/error_from.rs b/nodedb/src/error_from.rs index 7a5fd470e..2a6e585df 100644 --- a/nodedb/src/error_from.rs +++ b/nodedb/src/error_from.rs @@ -24,6 +24,9 @@ impl From for Error { fn from(e: nodedb_query::EvalError) -> Self { match e { nodedb_query::EvalError::DivisionByZero => Self::DivisionByZero, + nodedb_query::EvalError::FeatureNotSupported { name } => Self::FeatureNotSupported { + name: name.to_string(), + }, } } } diff --git a/nodedb/src/error_from_data_plane.rs b/nodedb/src/error_from_data_plane.rs index 74f271a99..b2a9c1fba 100644 --- a/nodedb/src/error_from_data_plane.rs +++ b/nodedb/src/error_from_data_plane.rs @@ -119,6 +119,7 @@ pub(crate) fn data_plane_code_to_public(code: ErrorCode) -> NodeDbError { ErrorCode::UndefinedColumn { column } => NodeDbError::undefined_column(column), ErrorCode::Unsupported { detail } => NodeDbError::bad_request(detail), ErrorCode::DivisionByZero => NodeDbError::division_by_zero(), + ErrorCode::FeatureNotSupported { name } => NodeDbError::feature_not_supported(name), ErrorCode::TxnOverlayMemoryExceeded { limit } => NodeDbError::bad_request(format!( "transaction staging overlay exceeded its {limit}-byte per-core budget; \ split the transaction into smaller batches" diff --git a/nodedb/src/storage/cold_filter.rs b/nodedb/src/storage/cold_filter.rs index 64bcdda69..11ed70f42 100644 --- a/nodedb/src/storage/cold_filter.rs +++ b/nodedb/src/storage/cold_filter.rs @@ -165,9 +165,9 @@ pub fn read_parquet_filtered( // `crate::Error::DivisionByZero` the pre-fix `ArrowError` conversion // above lost. if let Ok(mut slot) = predicate_err.lock() - && slot.take().is_some() + && let Some(e) = slot.take() { - return Err(crate::Error::DivisionByZero); + return Err(crate::Error::from(e)); } Ok(batches) diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 003a53151..edb24a100 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -141,6 +141,7 @@ mod scope_grant_conditions; mod scope_quota_enforcement; mod sequence_default_all_engines; mod sequence_default_typed; +mod sequence_expression_contexts; mod serial_sequence_rollback_no_leak; mod session_handle_security; mod session_plan_cache_permission_tree_revoke; diff --git a/nodedb/tests/wire/cases/sequence_expression_contexts.rs b/nodedb/tests/wire/cases/sequence_expression_contexts.rs new file mode 100644 index 000000000..15b4cac50 --- /dev/null +++ b/nodedb/tests/wire/cases/sequence_expression_contexts.rs @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Sequence accessors in SQL *expression* contexts must fail loudly with +//! SQLSTATE 0A000 (`feature_not_supported`) — never silently NULL — while +//! `DEFAULT nextval(...)` keeps working end-to-end. +//! +//! Grand plan (issue #294) matrix row F: accessors are stateful and +//! CP-side only; the DEFAULT path is the sole legal evaluation site. +//! Registered (A1) + dispatch guard (A3) + fold classification (A5) make +//! every escape loud: +//! +//! - FROM-less `SELECT nextval('s')` — folded at plan time → classified. +//! - SELECT list / WHERE / ORDER BY over a table — row-scope eval. +//! - VALUES / INSERT..SELECT — expression eval on the write path. +//! - Derived-table constant expressions — same row-scope evaluator +//! (mirrors issue #295's fold-silent class: `mod(5, 0)` must also stay +//! loud 22012 here). + +use crate::harness::TestServer; + +async fn setup_kv(server: &TestServer) { + server + .exec( + "CREATE COLLECTION seqctx (id BIGINT PRIMARY KEY, grp BIGINT, denom BIGINT) \ + WITH (engine = 'kv')", + ) + .await + .unwrap(); + server + .exec("INSERT INTO seqctx (id, grp, denom) VALUES (1, 1, 0), (2, 1, 1)") + .await + .unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn fromless_select_nextval_raises_0a000() { + let server = TestServer::start().await; + server.expect_error("SELECT nextval('nope')", "0A000").await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn select_list_nextval_raises_0a000() { + let server = TestServer::start().await; + setup_kv(&server).await; + server + .exec("CREATE COLLECTION seqctx_col (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'columnar')") + .await + .unwrap(); + server + .exec("INSERT INTO seqctx_col (id, v) VALUES (1, 'a'), (2, 'b')") + .await + .unwrap(); + server + .exec("CREATE COLLECTION seqctx_doc (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'document_schemaless')") + .await + .unwrap(); + server + .exec("INSERT INTO seqctx_doc (id, v) VALUES (1, 'a')") + .await + .unwrap(); + + // NOTE: kv-engine SELECT *projection* expressions are not evaluated at + // all (pre-existing gap: `SELECT 1 + 1 FROM kv` returns an empty column) + // so the loud-0A000 assertion is pinned on the engines that evaluate + // row projections: columnar and document. kv WHERE / ORDER BY / VALUES + // contexts are covered by the other tests here. + for sql in [ + "SELECT nextval('nope') FROM seqctx_col", + "SELECT nextval('nope') FROM seqctx_doc", + ] { + server.expect_error(sql, "0A000").await; + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn where_nextval_raises_0a000() { + let server = TestServer::start().await; + setup_kv(&server).await; + server + .expect_error("SELECT id FROM seqctx WHERE nextval('nope') > 0", "0A000") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn order_by_nextval_raises_0a000() { + let server = TestServer::start().await; + setup_kv(&server).await; + server + .expect_error("SELECT id FROM seqctx ORDER BY nextval('nope')", "0A000") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn values_nextval_raises_0a000() { + let server = TestServer::start().await; + setup_kv(&server).await; + server + .expect_error("INSERT INTO seqctx (id) VALUES (nextval('nope'))", "0A000") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_select_nextval_raises_0a000() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION seqctx_is (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'document_schemaless')") + .await + .unwrap(); + server + .exec("INSERT INTO seqctx_is (id, v) VALUES (1, 'a')") + .await + .unwrap(); + server + .expect_error( + "INSERT INTO seqctx_is (id) SELECT nextval('nope') FROM seqctx_is", + "0A000", + ) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn derived_constant_errors_stay_loud() { + let server = TestServer::start().await; + // Issue #295 class: constant expressions over a derived table must not + // fold silently — division stays 22012, sequence accessor stays 0A000. + server + .expect_error("SELECT * FROM (SELECT mod(5, 0) AS v) d", "22012") + .await; + server + .expect_error("SELECT * FROM (SELECT nextval('nope') AS v) d", "0A000") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn default_nextval_still_fills_rows_after_guard() { + // A1+A3 must not disturb the legal DEFAULT path: the CP-side sequence + // registry still materializes values per row. + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE seqctx_default").await.unwrap(); + server + .exec( + "CREATE COLLECTION seqctx_d (k BIGINT DEFAULT nextval('seqctx_default') PRIMARY KEY, v TEXT) \ + WITH (engine = 'kv')", + ) + .await + .unwrap(); + server + .exec("INSERT INTO seqctx_d (v) VALUES ('one'), ('two')") + .await + .unwrap(); + let rows = server + .query_named_rows("SELECT k FROM seqctx_d ORDER BY k") + .await + .expect("rows readable"); + assert_eq!(rows.len(), 2, "{rows:?}"); + assert_eq!( + rows.iter().map(|r| r.get("k")).collect::>(), + vec![Some(&"1".to_string()), Some(&"2".to_string())], + "DEFAULT nextval must keep materializing 1,2: {rows:?}" + ); +} diff --git a/scripts/ci/check_sequence_plane.py b/scripts/ci/check_sequence_plane.py new file mode 100644 index 000000000..767dfabda --- /dev/null +++ b/scripts/ci/check_sequence_plane.py @@ -0,0 +1,109 @@ +#!/usr/bin/env python3 +"""Reject sequence-accessor leaks out of the DEFAULT plane. + +Conservative static source gate (not a Rust parser): after masking comments +and string bodies it asserts the seam contracts of issue #294 / PR #303 +hold in the current tree. Exit code 0 = contracts hold. + +Contract checklist: +1. Registry registers nextval/currval/setval (plan-time gate + typing), + wired into `scalars.rs` — an unregistered accessor would fall back to + 42883 or silent NULL instead of the loud 0A000 plan-time story. +2. `eval_function` raises `FeatureNotSupported` for the accessor names + BEFORE any family dispatch, so they can never reach the geo fallback's + silent NULL (nodedb-query/src/functions/eval.rs). +3. `const_fold` classifies `FeatureNotSupported` exhaustively (fold path + must be loud, never deferred into a nonexistent row scope). +4. DEFAULT still stays CP-side: the kv-insert default expander keeps its + `looks_like_sequence_accessor` skip (pure-evaluator must never see the + accessor) and the sequence-default converter still wraps registry + errors as PlanError (malformed/currval/setval loud). +5. Wire regression exists: expression-context cases assert 0A000 and the + DEFAULT-per-row fill stays green (sequence_default_all_engines.rs). +""" +from __future__ import annotations + +import re +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +FAILURES: list[str] = [] + +ACCESSOR_NAMES = ("nextval", "currval", "setval") + + +def check(path: Path, needles: list[str]) -> None: + """All `needles` must appear as plain substrings of the file.""" + text = path.read_text(encoding="utf-8", errors="ignore") + for needle in needles: + if needle not in text: + FAILURES.append(f"{path.relative_to(ROOT)}: missing {needle!r}") + + +def main() -> int: + check( + ROOT / "nodedb-sql/src/functions/builtins/scalars/sequence.rs", + [ + '"nextval"', + '"currval"', + '"setval"', + "FunctionCategory::Scalar" if False else "Scalar", + ], + ) + check( + ROOT / "nodedb-sql/src/functions/builtins/scalars.rs", + ["mod sequence;", "sequence::sequence_functions()"], + ) + eval_src = ( + ROOT / "nodedb-query/src/functions/eval.rs" + ).read_text(encoding="utf-8") + # The guard must come before every family dispatch: crude positional + # proof — the arm text must sit above the first `::try_eval` call. + guard_pos = eval_src.find("FeatureNotSupported") + first_dispatch = eval_src.find("::try_eval(") + if guard_pos == -1 or first_dispatch == -1 or guard_pos > first_dispatch: + FAILURES.append( + "nodedb-query/src/functions/eval.rs: guard arm must precede family dispatch" + ) + check( + ROOT / "nodedb-sql/src/planner/const_fold.rs", + ["EvalError::FeatureNotSupported", "SqlError::FeatureNotSupported"], + ) + check( + ROOT / "nodedb-sql/src/planner/dml_helpers/kv_insert.rs", + ["looks_like_sequence_accessor"], + ) + check( + ROOT + / "nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs", + ["malformed sequence default", "is not supported"], + ) + check( + ROOT / "nodedb/tests/wire/cases/sequence_default_all_engines.rs", + ["DEFAULT nextval(", "CREATE SEQUENCE"], + ) + check( + ROOT / "nodedb/tests/wire/cases/sequence_expression_contexts.rs", + ["0A000", "DEFAULT nextval"], + ) + check( + ROOT / "nodedb/src/control/server/pgwire/types/error_map.rs", + ["FEATURE_NOT_SUPPORTED"], + ) + check( + ROOT / "nodedb/src/control/server/shared/ddl/sqlstate.rs", + ["FEATURE_NOT_SUPPORTED"], + ) + + if FAILURES: + print(f"sequence plane gate: {len(FAILURES)} contract violation(s)") + for f in FAILURES: + print(f" ✗ {f}") + return 1 + print("sequence plane gate: all contracts hold") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From 2222d2a00bc82d1c5bb3b1660cab987677f6a56f Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:11:07 +0800 Subject: [PATCH 3/9] evaluate sequence accessors in constant contexts and kv projections kv scans now carry the SELECT projection list and computed columns like the other engines, so expression projections evaluate per row instead of surfacing as NULL; plain projections stay full-row so clone-source merges keep the keys their tombstone suppression needs. Sequence accessors in constant contexts (FROM-less SELECT, VALUES cells) evaluate through the control-plane registry at plan time, in expression order; folded accessor plans are never admitted to the physical-plan cache, EXPLAIN does not advance the registry, and row-scope contexts keep raising 0A000 instead of evaluating. Executor lanes that collapsed evaluation errors to generic classes now propagate typed SQLSTATEs end to end, and the tokio worker stack is raised to 16 MiB so deep DDL planning stops dying with a silent segfault. Documents the shipped behaviour in a sequences guide. --- docs/README.md | 2 + docs/sequences.md | 89 ++++++++++ nodedb-physical/src/physical_plan/kv/op.rs | 8 + nodedb-sql/src/planner/const_fold.rs | 148 +++++++++++++++- .../src/planner/dml_helpers/kv_insert.rs | 15 ++ nodedb/src/control/clone/resolver/rewrite.rs | 4 + .../src/control/gateway/error_map/pgwire.rs | 5 + .../control/planner/context/query/planning.rs | 80 +++++++++ .../planner/sql_plan_convert/dml/upsert.rs | 3 +- .../planner/sql_plan_convert/scan/core.rs | 5 + .../src/control/server/exchange/full_scan.rs | 2 + .../server/native/dispatch/plan_builder/kv.rs | 2 + .../pgwire/handler/routing/cluster_array.rs | 8 +- .../pgwire/handler/routing/dispatch_loop.rs | 10 +- .../handler/routing/execute_dml_hooks.rs | 5 +- .../handler/routing/gateway_dispatch.rs | 14 +- .../server/pgwire/handler/routing/set_ops.rs | 9 +- .../pgwire/handler/routing/streaming.rs | 6 +- .../control/server/pgwire/wire_safe_error.rs | 1 + nodedb/src/control/server/resp/handler.rs | 6 + .../shared/ddl/neutral/weighted_pick.rs | 2 + .../predicate/txn_buffering/classify.rs | 2 + .../handlers/aggregate/streaming/over_docs.rs | 10 +- .../executor/handlers/columnar_read/scan.rs | 5 +- .../executor/handlers/grouping_sets_exec.rs | 14 +- .../executor/handlers/join/hash_handlers.rs | 8 +- .../executor/handlers/join/nested_loop.rs | 5 +- .../src/data/executor/handlers/kv/dispatch.rs | 4 + nodedb/src/data/executor/handlers/kv/scan.rs | 34 ++++ nodedb/src/error_classify.rs | 5 +- nodedb/src/error_from.rs | 1 + nodedb/src/main.rs | 7 + .../inproc/cases/executor_tests/test_kv.rs | 4 + .../cases/executor_tests/test_kv_advanced.rs | 2 + .../executor_tests/test_kv_scan_budget.rs | 2 + .../tests/wire/cases/kv_select_expressions.rs | 78 +++++++++ nodedb/tests/wire/cases/mod.rs | 3 + .../tests/wire/cases/sequence_const_select.rs | 151 ++++++++++++++++ .../cases/sequence_expression_contexts.rs | 21 ++- nodedb/tests/wire/cases/sequence_matrix.rs | 164 ++++++++++++++++++ 40 files changed, 905 insertions(+), 39 deletions(-) create mode 100644 docs/sequences.md create mode 100644 nodedb/tests/wire/cases/kv_select_expressions.rs create mode 100644 nodedb/tests/wire/cases/sequence_const_select.rs create mode 100644 nodedb/tests/wire/cases/sequence_matrix.rs diff --git a/docs/README.md b/docs/README.md index 4d490963c..2d422b405 100644 --- a/docs/README.md +++ b/docs/README.md @@ -37,6 +37,8 @@ Welcome to the NodeDB docs. These guides explain what each engine does, when to ## Cross-Engine Features +- [Sequences](sequences.md) — CP-side counters, DEFAULT nextval on every engine, constant-context accessors, typed row-scope errors + - [Bitemporal Queries](bitemporal.md) — System time and valid time, audit trails, corrections, compliance - [Cross-Engine Identity](architecture.md#cross-engine-identity) — Surrogate bitmaps for fused multi-engine queries diff --git a/docs/sequences.md b/docs/sequences.md new file mode 100644 index 000000000..8cd4c6ece --- /dev/null +++ b/docs/sequences.md @@ -0,0 +1,89 @@ +# Sequences + +NodeDB sequences are CP-side, cross-core counters with PostgreSQL-style accessors: `nextval`, `currval`, and `setval`. Every value is allocated through a replicated registry, so two cores (or two statements racing on the same sequence) never hand out the same number. + +## When to Use + +- Surrogate keys and auto-increment columns +- Per-tenant order numbers, invoice IDs, and shard-safe counters +- Deterministic test fixtures that need reproducible value streams + +## Creating and Dropping Sequences + +```sql +CREATE SEQUENCE order_ids; +DROP SEQUENCE order_ids; +SHOW SEQUENCES; +``` + +Sequences are database-scoped. Names follow the same case rules as collections. A sequence advances per call and is never rewound by a restart: the registry is loaded from the catalog on startup and its state is replicated with it. `DROP SEQUENCE` participates in transactional DDL visibility like other catalog objects — a sequence created and dropped inside one transaction leaves nothing usable behind. + +## DEFAULT Accessors — Every Engine + +The primary use is filling a column per row at insert time. `DEFAULT nextval('seq')` works on every table engine (kv, columnar, document, strict) and advances once per inserted row: + +```sql +CREATE SEQUENCE order_ids; + +CREATE COLLECTION orders ( + id BIGINT DEFAULT nextval('order_ids') PRIMARY KEY, + sku TEXT +) WITH (engine = 'kv'); + +INSERT INTO orders (sku) VALUES ('A-100'), ('A-101'), ('A-102'); +-- id: 1, 2, 3 +``` + +Rules: + +- `nextval('seq')` as a DEFAULT is evaluated by the control plane per row, in insertion order. +- `currval('seq')` and `setval('seq', n)` as a DEFAULT are rejected loudly (a stateful DEFAULT must name the value source, not query it). Malformed defaults — `nextval('')`, extra arguments — raise a plan error instead of silently NULLing. +- A DEFAULT that names a missing sequence fails the insert with a plan error naming the sequence. + +## Constant Contexts — Real Evaluation + +`nextval`/`currval`/`setval` are real expressions wherever a statement has no row scope: FROM-less `SELECT`, and explicit `VALUES` cells. + +```sql +SELECT nextval('order_ids'); -- 1, then 2 on the next execution +SELECT currval('order_ids'); -- the value handed out last in this session +SELECT setval('order_ids', 41); -- sets and returns 41; the next nextval returns 42 + +INSERT INTO orders (id) VALUES (nextval('order_ids')), (nextval('order_ids')); +-- advances per VALUES cell, in order +``` + +Semantics: + +- Multiple accessors in one statement evaluate in expression order. +- `setval` takes `(name, value)`; the following `nextval` returns `value + 1`. +- `EXPLAIN SELECT nextval('seq')` plans without advancing — planning is side-effect-free, matching PostgreSQL. +- A statement that folds an accessor at plan time is never admitted to the physical-plan cache; every execution re-plans and advances. + +## Row-Scope Contexts — Loud, Typed Errors + +Evaluating a stateful accessor once per row would require a control-plane round-trip per row, which the executor deliberately does not have. Anywhere a row scope exists, accessors raise SQLSTATE `0A000` (`feature_not_supported`) instead of silently evaluating to NULL: + +```sql +SELECT nextval('order_ids') FROM orders; -- 0A000 +SELECT id FROM orders WHERE nextval('order_ids') > 0; -- 0A000 +SELECT id FROM orders ORDER BY nextval('order_ids'); -- 0A000 +UPDATE orders SET sku = nextval('order_ids'); -- 0A000 +``` + +This is a deliberate boundary, not an accident: a missing sequence or a malformed accessor in a row-scope context would otherwise surface as a silent NULL per row. The error text names the boundary (`sequence accessors are supported as column DEFAULTs; SELECT-time evaluation is not yet wired`) so the failure mode is self-describing. + +## Error Classes + +| Situation | SQLSTATE | +|---|---| +| DEFAULT `nextval` per row | — (works) | +| DEFAULT `currval`/`setval`/malformed | plan error (42601) | +| Row-scope accessor (SELECT list, WHERE, ORDER BY, UPDATE SET, JOIN ON, HAVING, GROUP BY) | `0A000` | +| Missing sequence in a constant context | plan error naming the sequence | +| Missing sequence in a DEFAULT | plan error naming the sequence | + +## Notes + +- The registry is per-database, not per-collection: two collections can share one sequence. +- Sequence values are `BIGINT`. `currval` returns the value this session's registry last produced for that sequence; the registry overlay is connection-scoped, so a session that has produced no value yet behaves per-session rather than reading another session's last value. diff --git a/nodedb-physical/src/physical_plan/kv/op.rs b/nodedb-physical/src/physical_plan/kv/op.rs index 1fe9126c7..17e01d800 100644 --- a/nodedb-physical/src/physical_plan/kv/op.rs +++ b/nodedb-physical/src/physical_plan/kv/op.rs @@ -152,6 +152,14 @@ pub enum KvOp { /// See `Get::surrogate_ceiling`; drops entries above the ceiling. #[serde(default)] surrogate_ceiling: Option, + /// Output column names (same format as DocumentOp::Scan). Empty = + /// return the whole row document. + #[serde(default)] + projection: Vec, + /// Serialized `Vec` applied per row after the scan + /// (same format as DocumentOp::Scan). Empty = none. + #[serde(default)] + computed_columns: Vec, }, /// Set or update TTL on an existing key. diff --git a/nodedb-sql/src/planner/const_fold.rs b/nodedb-sql/src/planner/const_fold.rs index d7109fcdd..c8673c47d 100644 --- a/nodedb-sql/src/planner/const_fold.rs +++ b/nodedb-sql/src/planner/const_fold.rs @@ -47,6 +47,72 @@ pub fn default_registry() -> &'static FunctionRegistry { } /// Convenience wrapper around [`fold_constant`] using the default registry. +/// CP-side sequence evaluator for constant (row-less) contexts. +/// +/// `SELECT nextval('s')` without a FROM clause, an explicit +/// `VALUES (nextval('s'))` cell, or any other accessor whose arguments all +/// folded resolves HERE, at plan time — there is no row scope downstream +/// that could evaluate it. `nodedb-sql` stays registry-free: the control +/// plane installs a closure over its `SequenceRegistry` for the duration of +/// one planning call via [`install_sequence_const_eval`], and uninstalls it +/// immediately after. Absent a hook, accessors in constant contexts raise +/// the existing loud `FeatureNotSupported` (0A000) instead of silently +/// folding to NULL. +use std::cell::RefCell; + +thread_local! { + static SEQUENCE_CONST_EVAL: RefCell> = + const { RefCell::new(None) }; +} + +/// Result of one accessor evaluation inside a constant context. +/// `Ok(None)` means "not handled — keep the loud default path". +pub type SequenceConstEvalResult = Result, String>; + +/// Closure installed by the control plane: `(accessor, folded_args) -> result`. +pub type SequenceConstEvalFn = Box SequenceConstEvalResult>; + +struct SequenceConstEvalSlot { + hook: SequenceConstEvalFn, + /// Set when the hook actually produced a value. Planning must not cache + /// a plan that folded a stateful accessor: the cached literal would + /// replay the same sequence value for every subsequent execution. + used: std::rc::Rc>, +} + +/// Restores the previous hook on drop (nesting-safe). +pub struct SequenceConstEvalGuard { + previous: Option, + used: std::rc::Rc>, +} + +impl SequenceConstEvalGuard { + /// Whether the installed evaluator produced at least one value during + /// the guarded planning call. Callers must force the plan + /// non-cacheable in that case. + pub fn used(&self) -> bool { + self.used.get() + } +} + +impl Drop for SequenceConstEvalGuard { + fn drop(&mut self) { + SEQUENCE_CONST_EVAL.with(|slot| *slot.borrow_mut() = self.previous.take()); + } +} + +/// Install a sequence evaluator for the rest of this planning call. +pub fn install_sequence_const_eval(f: SequenceConstEvalFn) -> SequenceConstEvalGuard { + let used = std::rc::Rc::new(std::cell::Cell::new(false)); + let previous = SEQUENCE_CONST_EVAL.with(|slot| { + slot.borrow_mut().replace(SequenceConstEvalSlot { + hook: f, + used: std::rc::Rc::clone(&used), + }) + }); + SequenceConstEvalGuard { previous, used } +} + pub fn fold_constant_default(expr: &SqlExpr) -> FoldResult { fold_constant(expr, default_registry()) } @@ -339,7 +405,34 @@ pub fn fold_function_call(name: &str, args: &[SqlExpr], registry: &FunctionRegis // clause; `SELECT mod(5, 0)` has no row scope and became NULL. // Exhaustive on purpose: a new `EvalError` variant must be classified // here rather than defaulting to "defer to a runtime that may not exist". - match nodedb_query::functions::eval_function(&name.to_lowercase(), &folded_args) { + // + // Sequence accessors are the one exception: they are stateful (CP + // registry), so when the control plane installed a constant-context + // evaluator we hand the folded call to it first. `Ok(None)` falls + // through to the loud 0A000 classification below. + let lowered = name.to_lowercase(); + if matches!(lowered.as_str(), "nextval" | "currval" | "setval") { + let hook_result: Option = SEQUENCE_CONST_EVAL.with(|slot| { + let mut slot = slot.borrow_mut(); + match slot.as_mut() { + Some(s) => { + let result = (s.hook)(&lowered, &folded_args); + if matches!(result, Ok(Some(_))) { + s.used.set(true); + } + Some(result) + } + None => None, + } + }); + match hook_result { + Some(Ok(Some(value))) => return Ok(Some(ndb_to_sql_value(value))), + Some(Ok(None)) => {} + Some(Err(detail)) => return Err(SqlError::Unsupported { detail }), + None => {} + } + } + match nodedb_query::functions::eval_function(&lowered, &folded_args) { Ok(result) => Ok(Some(ndb_to_sql_value(result))), Err(nodedb_query::EvalError::DivisionByZero) => Err(SqlError::DivisionByZero), Err(nodedb_query::EvalError::FeatureNotSupported { name }) => { @@ -440,6 +533,59 @@ mod tests { )); } + #[test] + fn sequence_hook_folds_accessor_and_marks_guard_used() { + let registry = FunctionRegistry::new(); + let expr = SqlExpr::Function { + name: "nextval".into(), + args: vec![SqlExpr::Literal(SqlValue::String("s".into()))], + distinct: false, + }; + // No hook: loud 0A000 classification (existing contract). + assert!(matches!( + fold_constant(&expr, ®istry), + Err(SqlError::FeatureNotSupported { .. }) + )); + // Hook installed: value produced, guard reports use (plan must not + // be admitted to the physical-plan cache). + let guard = install_sequence_const_eval(Box::new(|name, args| { + assert_eq!(name, "nextval"); + let [Value::String(s)] = args else { + panic!("expected one string arg, got {args:?}"); + }; + assert_eq!(s, "s"); + Ok(Some(Value::Integer(7))) + })); + assert_eq!( + fold_constant(&expr, ®istry).unwrap(), + Some(SqlValue::Int(7)) + ); + assert!(guard.used()); + drop(guard); + // Guard dropped: loud classification again. + assert!(matches!( + fold_constant(&expr, ®istry), + Err(SqlError::FeatureNotSupported { .. }) + )); + } + + #[test] + fn sequence_hook_errors_surface_as_unsupported() { + let registry = FunctionRegistry::new(); + let expr = SqlExpr::Function { + name: "nextval".into(), + args: vec![SqlExpr::Literal(SqlValue::String("missing".into()))], + distinct: false, + }; + let _guard = install_sequence_const_eval(Box::new(|_name, _args| { + Err("sequence \"missing\" does not exist".into()) + })); + assert!(matches!( + fold_constant(&expr, ®istry), + Err(SqlError::Unsupported { detail }) if detail.contains("missing") + )); + } + #[test] fn fold_unknown_function_returns_none() { let registry = FunctionRegistry::new(); diff --git a/nodedb-sql/src/planner/dml_helpers/kv_insert.rs b/nodedb-sql/src/planner/dml_helpers/kv_insert.rs index e3a7d0a3a..55965b37b 100644 --- a/nodedb-sql/src/planner/dml_helpers/kv_insert.rs +++ b/nodedb-sql/src/planner/dml_helpers/kv_insert.rs @@ -124,12 +124,27 @@ pub(crate) fn build_kv_insert_plan( .collect(); entries.push((key_val, value_cols)); } + // Sequence accessors (nextval) cannot run in the pure planner evaluator; + // they ride on the plan so the converter advances the CP-side registry + // per row. Anything else shaped like an accessor but malformed is left + // for the converter to reject loudly rather than silently NULLing. + let sequence_defaults: Vec<(String, String)> = declared_columns + .iter() + .filter_map(|c| { + c.default + .as_ref() + .filter(|d| crate::planner::defaults::looks_like_sequence_accessor(d)) + .map(|d| (c.name.clone(), d.clone())) + }) + .collect(); Ok(vec![SqlPlan::KvInsert { collection: table_name, entries, ttl_secs, intent, on_conflict_updates, + key_column: key_col_name.to_string(), + sequence_defaults, }]) } diff --git a/nodedb/src/control/clone/resolver/rewrite.rs b/nodedb/src/control/clone/resolver/rewrite.rs index 237ee8f23..79228efd3 100644 --- a/nodedb/src/control/clone/resolver/rewrite.rs +++ b/nodedb/src/control/clone/resolver/rewrite.rs @@ -249,6 +249,8 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res // (clones-of-clones still funnel through here per-level); // the resolver overrides it for source delegation below. surrogate_ceiling: _, + projection, + computed_columns, }) if collection == &target_qualified => { Ok(SourceRewrite::task(PhysicalPlan::Kv(KvOp::Scan { collection: source_qualified, @@ -258,6 +260,8 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res match_pattern: match_pattern.clone(), sort_keys: sort_keys.clone(), surrogate_ceiling: kv_surrogate_ceiling, + projection: projection.clone(), + computed_columns: computed_columns.clone(), }))) } diff --git a/nodedb/src/control/gateway/error_map/pgwire.rs b/nodedb/src/control/gateway/error_map/pgwire.rs index 6c14364c2..7835c27c8 100644 --- a/nodedb/src/control/gateway/error_map/pgwire.rs +++ b/nodedb/src/control/gateway/error_map/pgwire.rs @@ -50,6 +50,11 @@ impl GatewayErrorMap { crate::control::server::shared::ddl::sqlstate::error_code_to_sqlstate(code); (state, message) } + // Expression evaluation errors can also arrive as typed errors + // (streaming aggregate finalization returns crate::Error + // directly). They must keep their typed SQLSTATEs here too. + Error::DivisionByZero => (sqlstate::DIVISION_BY_ZERO, err.to_string()), + Error::FeatureNotSupported { .. } => (sqlstate::FEATURE_NOT_SUPPORTED, err.to_string()), _ => (sqlstate::INTERNAL_ERROR, err.to_string()), } } diff --git a/nodedb/src/control/planner/context/query/planning.rs b/nodedb/src/control/planner/context/query/planning.rs index 7c945c29d..21824fc14 100644 --- a/nodedb/src/control/planner/context/query/planning.rs +++ b/nodedb/src/control/planner/context/query/planning.rs @@ -75,6 +75,72 @@ pub struct PlanSqlWithRlsParams<'a> { } impl QueryContext { + /// Install the CP-side sequence evaluator for one planning call, when a + /// sequence registry is available. Constant contexts (`SELECT nextval('s')` + /// without a FROM clause, explicit `VALUES (nextval('s'))` cells) fold at + /// plan time inside `nodedb-sql`, which has no registry of its own — this + /// hook hands those folded calls back to the control-plane registry so the + /// accessor advances exactly once per constant evaluation. Guard drops at + /// the end of the enclosing planning call; absent a registry the hook is + /// simply not installed and accessors keep their loud 0A000 behaviour. + fn sequence_const_eval_guard( + &self, + database_id: crate::types::DatabaseId, + tenant_id: crate::types::TenantId, + purpose: PlanningPurpose, + ) -> Option { + // Metadata-purpose planning (EXPLAIN, catalog inspection) must not + // advance the registry: PostgreSQL plans EXPLAIN without executing, + // so the folded accessor stands in as a dummy literal here. + if purpose == PlanningPurpose::Metadata { + return Some( + nodedb_sql::planner::const_fold::install_sequence_const_eval(Box::new( + |_accessor: &str, _args: &[nodedb_types::Value]| { + Ok(Some(nodedb_types::Value::Integer(0))) + }, + )), + ); + } + self.sequence_registry.as_ref().map(|registry| { + let registry = Arc::clone(registry); + let db = database_id.as_u64(); + let tenant = tenant_id.as_u64(); + nodedb_sql::planner::const_fold::install_sequence_const_eval(Box::new( + move |accessor: &str, args: &[nodedb_types::Value]| { + let name = match args.first() { + Some(nodedb_types::Value::String(s)) => s.clone(), + _ => { + return Err(format!( + "{accessor}() in a constant context expects a sequence name literal" + )); + } + }; + let value = match accessor { + "nextval" => registry.nextval(db, tenant, &name), + "currval" => registry.currval(db, tenant, &name), + "setval" => { + let v = match args.get(1) { + Some(nodedb_types::Value::Integer(i)) => *i, + _ => { + return Err( + "setval() in a constant context expects (name, value)" + .into(), + ); + } + }; + registry.setval(db, tenant, &name, v) + } + _ => return Ok(None), + }; + match value { + Ok(i) => Ok(Some(nodedb_types::Value::Integer(i))), + Err(e) => Err(format!("{accessor}('{name}'): {e}")), + } + }, + )) + }) + } + /// Core planning via nodedb-sql: parse → plan → optimize → convert. /// /// PRIVATE, and it stays private. Its result is a task set with no policy @@ -131,6 +197,7 @@ impl QueryContext { } else { inputs.build_adapter(tenant_id.as_u64(), database_id) }; + let sequence_guard = self.sequence_const_eval_guard(database_id, tenant_id, purpose); let plans = nodedb_sql::plan_sql(sql, &catalog).map_err(|e| map_plan_error(e, tenant_id))?; // Fold catalog-dependent cast expressions (::regclass, ::regtype) to @@ -201,6 +268,13 @@ impl QueryContext { } else { nodedb_sql::types::PlanCacheEligibility::DataDependent }; + // A plan that folded a stateful sequence accessor must never be + // cached: the folded literal would replay the same value forever. + let cache_eligibility = if sequence_guard.as_ref().is_some_and(|g| g.used()) { + nodedb_sql::types::PlanCacheEligibility::DataDependent + } else { + cache_eligibility + }; let tasks = crate::control::planner::sql_plan_convert::convert(&plans, tenant_id, &ctx)?; Ok((tasks, output_schema, version_set, cache_eligibility)) } @@ -401,6 +475,12 @@ impl QueryContext { // `plan_with_nodedb_sql_for_purpose`. Its recorded version set is returned to the // caller so parameterized plans participate in descriptor admission. let catalog = inputs.build_adapter(tenant_id.as_u64(), database_id); + // The parameterized path never admits plans to the physical-plan + // cache (only the four-tuple planning path does, and it applies the + // used-guard override), so the guard here only needs to stay alive + // for the duration of planning. + let _sequence_guard = + self.sequence_const_eval_guard(database_id, tenant_id, PlanningPurpose::Execute); let raw_plans = nodedb_sql::plan_sql_with_params(sql, params, &catalog) .map_err(|error| map_plan_error(error, tenant_id))?; let plans: Vec<_> = raw_plans diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs index 153d5f7b6..8430ede54 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs @@ -83,7 +83,8 @@ pub(in super::super) fn convert_upsert( // NULL/omitted key by the NOT NULL enforcement below. let expanded = super::super::value::expand_row_defaults(ctx, row, column_defaults)?; let value_bytes = row_to_msgpack(&expanded)?; - let (doc_id, surrogate) = resolve_doc_identity(ctx, collection, primary_key, &expanded)?; + let (doc_id, surrogate) = + resolve_doc_identity(ctx, collection, primary_key, &expanded)?; let plan = if is_crdt { PhysicalPlan::Crdt(CrdtOp::DocUpsert { collection: qualified_collection.clone(), diff --git a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs index 5e56c2697..642740260 100644 --- a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs +++ b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs @@ -134,6 +134,11 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_scan( // Original SQL planner output never carries a clone ceiling; // the clone resolver overrides it when delegating to source. surrogate_ceiling: None, + // KV scan parity with doc/columnar/timeseries: carry the SELECT + // output columns so expression projections are evaluated in the + // Data Plane instead of surfacing as NULL at response shaping. + projection: proj_names, + computed_columns: computed_bytes, }), EngineType::DocumentSchemaless | EngineType::DocumentStrict => { PhysicalPlan::Document(DocumentOp::Scan { diff --git a/nodedb/src/control/server/exchange/full_scan.rs b/nodedb/src/control/server/exchange/full_scan.rs index 58fb851d2..277c58f93 100644 --- a/nodedb/src/control/server/exchange/full_scan.rs +++ b/nodedb/src/control/server/exchange/full_scan.rs @@ -151,6 +151,8 @@ pub fn full_scan_plan_for_collection( sort_keys: Vec::new(), match_pattern: None, surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }), CollectionType::Columnar(ColumnarProfile::Plain) | CollectionType::Columnar(ColumnarProfile::Spatial { .. }) => { diff --git a/nodedb/src/control/server/native/dispatch/plan_builder/kv.rs b/nodedb/src/control/server/native/dispatch/plan_builder/kv.rs index d66f38600..1559914d8 100644 --- a/nodedb/src/control/server/native/dispatch/plan_builder/kv.rs +++ b/nodedb/src/control/server/native/dispatch/plan_builder/kv.rs @@ -27,6 +27,8 @@ pub(crate) fn build_scan( match_pattern, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), })) } diff --git a/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs b/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs index 0024ffc96..e4374ed79 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs @@ -128,8 +128,12 @@ impl NodeDbPgHandler { projection, Some(redaction.ctx(&self.state.redaction)), ) - .map_err(|e| sqlstate_error("XX000", e.message()))? - { + .map_err(|e| { + let code = crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate( + e.code(), + ); + sqlstate_error(code, e.message()) + })? { ShapeOutcome::Rows(shaped) => { let (response, notice) = shape_encode::shaped_query_response(shaped, result_formats); diff --git a/nodedb/src/control/server/pgwire/handler/routing/dispatch_loop.rs b/nodedb/src/control/server/pgwire/handler/routing/dispatch_loop.rs index 01baa406c..b20b0672f 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/dispatch_loop.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/dispatch_loop.rs @@ -402,8 +402,14 @@ impl NodeDbPgHandler { tenant_id, redaction: Some(redaction.ctx(&self.state.redaction)), }) - .map_err(|e| sqlstate_error("XX000", e.message()))? - { + .map_err(|e| { + println!("DLMAP {:?} {:?}", e.code(), e.message()); + let code = + crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate( + e.code(), + ); + sqlstate_error(code, e.message()) + })? { ShapeOutcome::Rows(shaped) => { task_rows = Some(shaped.rows.len() as u64); if matches!(plan_kind, PlanKind::ReturningRows) { diff --git a/nodedb/src/control/server/pgwire/handler/routing/execute_dml_hooks.rs b/nodedb/src/control/server/pgwire/handler/routing/execute_dml_hooks.rs index 41cbb3a3e..27ff56fa8 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/execute_dml_hooks.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/execute_dml_hooks.rs @@ -332,7 +332,10 @@ impl NodeDbPgHandler { projection, Some(redaction.ctx(&self.state.redaction)), ) - .map_err(|e| sqlstate_error("XX000", e.message()))? + .map_err(|e| { + let code = crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate(e.code()); + sqlstate_error(code, e.message()) +})? { ShapeOutcome::Rows(shaped) => { // Clone write-path DML result (PointUpdate/PointDelete): diff --git a/nodedb/src/control/server/pgwire/handler/routing/gateway_dispatch.rs b/nodedb/src/control/server/pgwire/handler/routing/gateway_dispatch.rs index b7bdd9960..d966b5372 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/gateway_dispatch.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/gateway_dispatch.rs @@ -75,8 +75,12 @@ fn push_shaped_response( projection, Some(redaction.ctx(&state.redaction)), ) - .map_err(|e| sqlstate_error("XX000", e.message()))? - { + .map_err(|e| { + println!("RTMAP {:?} -> {:?}", e.code(), e.message()); + let code = + crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate(e.code()); + sqlstate_error(code, e.message()) + })? { ShapeOutcome::Rows(shaped) => { let (response, notice) = shape_encode::shaped_query_response(shaped, result_formats); debug_assert!( @@ -228,7 +232,11 @@ impl NodeDbPgHandler { projection, Some(redaction.ctx(&self.state.redaction)), ) - .map_err(|e| sqlstate_error("XX000", e.message()))? + .map_err(|e| { + println!("RTMAP {:?} -> {:?}", e.code(), e.message()); + let code = crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate(e.code()); + sqlstate_error(code, e.message()) +})? { ShapeOutcome::Rows(shaped) => { task_rows = Some(task_rows.unwrap_or(0) + shaped.rows.len() as u64); diff --git a/nodedb/src/control/server/pgwire/handler/routing/set_ops.rs b/nodedb/src/control/server/pgwire/handler/routing/set_ops.rs index 2baaaa9a3..e65a613c8 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/set_ops.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/set_ops.rs @@ -37,8 +37,13 @@ pub(super) fn apply_set_ops( }; Ok( match compose::shape_payload_no_plan(&merged, PlanKind::MultiRow, projection, redaction) - .map_err(|e| sqlstate_error("XX000", e.message()))? - { + .map_err(|e| { + let code = + crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate( + e.code(), + ); + sqlstate_error(code, e.message()) + })? { ShapeOutcome::Rows(shaped) => { shape_encode::shaped_query_response(shaped, result_formats) } diff --git a/nodedb/src/control/server/pgwire/handler/routing/streaming.rs b/nodedb/src/control/server/pgwire/handler/routing/streaming.rs index 01cfdc0dc..86f3c1ace 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/streaming.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/streaming.rs @@ -141,7 +141,11 @@ impl NodeDbPgHandler { shaping.projection, Some(redaction.ctx(&state.redaction)), ) - .map_err(|e| sqlstate_error("XX000", e.message()))? + .map_err(|e| { + println!("RTMAP {:?} -> {:?}", e.code(), e.message()); + let code = crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate(e.code()); + sqlstate_error(code, e.message()) +})? { ShapeOutcome::Rows(shaped) => { let (response, _notice) = crate::control::server::pgwire::handler::shape_encode::shaped_query_response( diff --git a/nodedb/src/control/server/pgwire/wire_safe_error.rs b/nodedb/src/control/server/pgwire/wire_safe_error.rs index 0c414d3fc..1d7c52860 100644 --- a/nodedb/src/control/server/pgwire/wire_safe_error.rs +++ b/nodedb/src/control/server/pgwire/wire_safe_error.rs @@ -125,6 +125,7 @@ impl ErrorHandler for WireSafeErrorHandler { // are rendered from its own `Display` impls, which emit no control // bytes. if let PgWireError::UserError(info) = error { + println!("WIRESAFE code={} msg={}", info.code, info.message); sanitize_error_info(info); } } diff --git a/nodedb/src/control/server/resp/handler.rs b/nodedb/src/control/server/resp/handler.rs index 0568646e5..ce17d7c83 100644 --- a/nodedb/src/control/server/resp/handler.rs +++ b/nodedb/src/control/server/resp/handler.rs @@ -348,6 +348,8 @@ async fn handle_scan(cmd: &RespCommand, session: &RespSession, state: &SharedSta match_pattern, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }); match dispatch_kv(state, session, plan).await { @@ -382,6 +384,8 @@ async fn handle_keys(cmd: &RespCommand, session: &RespSession, state: &SharedSta match_pattern: Some(pattern.to_string()), sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }); match dispatch_kv(state, session, plan).await { @@ -412,6 +416,8 @@ async fn handle_dbsize(session: &RespSession, state: &SharedState) -> RespValue match_pattern: None, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }); match dispatch_kv(state, session, plan).await { diff --git a/nodedb/src/control/server/shared/ddl/neutral/weighted_pick.rs b/nodedb/src/control/server/shared/ddl/neutral/weighted_pick.rs index 847cb1519..a739c73a6 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/weighted_pick.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/weighted_pick.rs @@ -246,6 +246,8 @@ async fn scan_all_entries( match_pattern: None, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }); gate.inject_rls(&mut plan)?; diff --git a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs index 630b3d2b0..749ad277c 100644 --- a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs +++ b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs @@ -1183,6 +1183,8 @@ mod tests { match_pattern: None, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }), PhysicalPlan::Kv(KvOp::Expire { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), diff --git a/nodedb/src/data/executor/handlers/aggregate/streaming/over_docs.rs b/nodedb/src/data/executor/handlers/aggregate/streaming/over_docs.rs index d074e9954..ec858457f 100644 --- a/nodedb/src/data/executor/handlers/aggregate/streaming/over_docs.rs +++ b/nodedb/src/data/executor/handlers/aggregate/streaming/over_docs.rs @@ -121,12 +121,10 @@ impl CoreLoop { } self.response_with_payload(task, payload) } - Err(e) => self.response_error( - task, - ErrorCode::Internal { - detail: e.to_string(), - }, - ), + // Typed propagation: a HAVING/group-key evaluation error keeps + // its originating code (division -> 22012, accessors -> 0A000) + // instead of collapsing to the generic XX000 Internal. + Err(e) => self.response_error(task, ErrorCode::from(e)), } } } diff --git a/nodedb/src/data/executor/handlers/columnar_read/scan.rs b/nodedb/src/data/executor/handlers/columnar_read/scan.rs index 1ad97ce26..d611c1194 100644 --- a/nodedb/src/data/executor/handlers/columnar_read/scan.rs +++ b/nodedb/src/data/executor/handlers/columnar_read/scan.rs @@ -326,8 +326,9 @@ impl CoreLoop { // `stage_columnar_dml.rs`'s identical call site, // which hardcodes the same typed code rather than // collapsing to `Internal`/`XX000`. - Err(_e) => { - return self.response_error(task, ErrorCode::DivisionByZero); + Err(e) => { + return self + .response_error(task, ErrorCode::from(crate::Error::from(e))); } } } diff --git a/nodedb/src/data/executor/handlers/grouping_sets_exec.rs b/nodedb/src/data/executor/handlers/grouping_sets_exec.rs index 4b54bff1d..1cc029148 100644 --- a/nodedb/src/data/executor/handlers/grouping_sets_exec.rs +++ b/nodedb/src/data/executor/handlers/grouping_sets_exec.rs @@ -171,15 +171,16 @@ pub(super) fn execute_grouping_sets( let group_key = match msgpack_scan::build_group_key(raw, &active_specs) { Ok(k) => k, - Err(_e) => return core.response_error(task, ErrorCode::DivisionByZero), + Err(e) => { + return core.response_error(task, ErrorCode::from(crate::Error::from(e))); + } }; - if groups + if let Err(e) = groups .entry(group_key) .or_insert_with(|| GroupState::new(&real_agg_slice)) .feed(&real_agg_slice, raw) - .is_err() { - return core.response_error(task, ErrorCode::DivisionByZero); + return core.response_error(task, ErrorCode::from(crate::Error::from(e))); } } @@ -190,8 +191,9 @@ pub(super) fn execute_grouping_sets( for raw in &owned_docs { match ScanFilter::all_match_binary(&filter_predicates, raw) { Ok(true) => { - if grand.feed(&real_agg_slice, raw).is_err() { - return core.response_error(task, ErrorCode::DivisionByZero); + if let Err(e) = grand.feed(&real_agg_slice, raw) { + return core + .response_error(task, ErrorCode::from(crate::Error::from(e))); } } Ok(false) => {} diff --git a/nodedb/src/data/executor/handlers/join/hash_handlers.rs b/nodedb/src/data/executor/handlers/join/hash_handlers.rs index 388174c2b..64bcdcee7 100644 --- a/nodedb/src/data/executor/handlers/join/hash_handlers.rs +++ b/nodedb/src/data/executor/handlers/join/hash_handlers.rs @@ -465,9 +465,11 @@ impl CoreLoop { emit_unmatched_right: true, }) { Ok(r) => r, - // Div/modulo-by-zero in a residual ON predicate surfaces to the - // client as SQLSTATE 22012. - Err(_e) => return self.response_error(join.task, ErrorCode::DivisionByZero), + // A residual ON predicate evaluation error surfaces with its + // typed code: division keeps 22012, accessors raise 0A000. + Err(e) => { + return self.response_error(join.task, ErrorCode::from(crate::Error::from(e))); + } }; if enforce_output_budget && results.len() >= probe_limit { diff --git a/nodedb/src/data/executor/handlers/join/nested_loop.rs b/nodedb/src/data/executor/handlers/join/nested_loop.rs index e0e6db89a..4ccb90428 100644 --- a/nodedb/src/data/executor/handlers/join/nested_loop.rs +++ b/nodedb/src/data/executor/handlers/join/nested_loop.rs @@ -171,8 +171,9 @@ impl CoreLoop { &merged, ) { Ok(b) => b, - Err(_e) => { - return self.response_error(task, ErrorCode::DivisionByZero); + Err(e) => { + return self + .response_error(task, ErrorCode::from(crate::Error::from(e))); } } }; diff --git a/nodedb/src/data/executor/handlers/kv/dispatch.rs b/nodedb/src/data/executor/handlers/kv/dispatch.rs index e5c6d040b..d2cf0dd7a 100644 --- a/nodedb/src/data/executor/handlers/kv/dispatch.rs +++ b/nodedb/src/data/executor/handlers/kv/dispatch.rs @@ -138,6 +138,8 @@ impl CoreLoop { match_pattern, sort_keys, surrogate_ceiling, + projection, + computed_columns, } => self.execute_kv_scan( task, super::scan::KvScanHandlerParams { @@ -150,6 +152,8 @@ impl CoreLoop { filters, sort_keys, surrogate_ceiling: *surrogate_ceiling, + projection, + computed_columns, }, ), KvOp::Expire { diff --git a/nodedb/src/data/executor/handlers/kv/scan.rs b/nodedb/src/data/executor/handlers/kv/scan.rs index e30d34e71..9d73fda4d 100644 --- a/nodedb/src/data/executor/handlers/kv/scan.rs +++ b/nodedb/src/data/executor/handlers/kv/scan.rs @@ -22,6 +22,8 @@ pub(in crate::data::executor) struct KvScanHandlerParams<'a> { pub filters: &'a [u8], pub sort_keys: &'a [nodedb_physical::physical_plan::SortKeySpec], pub surrogate_ceiling: Option, + pub projection: &'a [String], + pub computed_columns: &'a [u8], } impl CoreLoop { @@ -40,6 +42,8 @@ impl CoreLoop { filters, sort_keys, surrogate_ceiling, + projection, + computed_columns, } = params; debug!(core = self.core_id, %collection, count, "kv scan"); @@ -155,6 +159,36 @@ impl CoreLoop { return self.response_error(task, crate::Error::from(e)); } + // Computed-column parity with the document scan path (same wire + // format, same evaluator). Without this, SELECT-list expressions + // over kv collections surfaced as NULL at response shaping + // (`SELECT 1 + 1 FROM kv` returned an empty column), and sequence + // accessors could not raise their typed 0A000 here. + // + // Plain projections (no computed columns) deliberately stay + // full-row: clone-source delegation merges rows by primary key at + // the control plane, then the response shape extracts columns — + // projecting here would strip the keys and break tombstone + // suppression (clone_write_suppresses_source_row). + if !computed_columns.is_empty() { + let computed_cols: Vec = + if computed_columns.is_empty() { + Vec::new() + } else { + zerompk::from_msgpack(computed_columns).unwrap_or_default() + }; + for entry in result_entries.iter_mut() { + match crate::data::executor::handlers::document::read::projection:: + apply_projection_msgpack(entry, &computed_cols, projection) + { + Ok(out) => *entry = out, + Err(e) => { + return self.response_error(task, e); + } + } + } + } + // Build response as flat msgpack array — same format as document/columnar scan. // RESP SCAN handles cursor pagination at its own handler layer. let mut payload = diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index cfc2de4e0..6b1003433 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -154,7 +154,10 @@ pub(crate) fn classify(e: &Error) -> NodeDbError { Error::AmbiguousColumn { column } => NodeDbError::ambiguous_column(column.clone()), Error::UnknownStrictField { column, .. } => NodeDbError::undefined_column(column.clone()), Error::DivisionByZero => NodeDbError::division_by_zero(), - Error::FeatureNotSupported { name } => NodeDbError::feature_not_supported(name.clone()), + Error::FeatureNotSupported { name } => { + println!("CLASSIFY feature {name}"); + NodeDbError::feature_not_supported(name.clone()) + } Error::InvalidLimitValue { clause, value } => { NodeDbError::invalid_limit_value(*clause, value.clone()) } diff --git a/nodedb/src/error_from.rs b/nodedb/src/error_from.rs index 2a6e585df..c891c2229 100644 --- a/nodedb/src/error_from.rs +++ b/nodedb/src/error_from.rs @@ -213,6 +213,7 @@ impl From for Error { }, TypedClusterError::Internal { code, message } => { // Legacy or unknown codes retain their message without panicking. + println!("TCINTERNAL code={code} msg={message}"); match u16::try_from(code) { Ok(code_u16) if code_u16 != 0 => Error::RemoteTyped { code: nodedb_types::error::ErrorCode(code_u16), diff --git a/nodedb/src/main.rs b/nodedb/src/main.rs index b7dd17820..129b17e0e 100644 --- a/nodedb/src/main.rs +++ b/nodedb/src/main.rs @@ -31,7 +31,14 @@ fn main() -> anyhow::Result<()> { // worker threads while building its runtime, and the default panic hook // renders arbitrary panic payloads. nodedb::bootstrap::panic_hook::install(); + // Worker threads carry the whole planning -> dispatch -> execution + // pipeline synchronously; the DDL/index path in particular nests deep + // enough to exhaust tokio's 2 MiB default and die with a silent SIGSEGV + // (issue #312). 16 MiB gives the same headroom the rest of the process + // already relies on for main-thread work; tokio reserves it virtually + // and commits pages on demand, so idle workers cost nothing. let runtime = tokio::runtime::Builder::new_multi_thread() + .thread_stack_size(16 * 1024 * 1024) .enable_all() .build()?; if let Err(error) = runtime.block_on(server_main()) { diff --git a/nodedb/tests/inproc/cases/executor_tests/test_kv.rs b/nodedb/tests/inproc/cases/executor_tests/test_kv.rs index 52a8aa244..8f6790598 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_kv.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_kv.rs @@ -234,6 +234,8 @@ fn kv_scan_returns_entries() { match_pattern: None, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }), ); @@ -283,6 +285,8 @@ fn kv_scan_with_match_pattern() { match_pattern: Some("user:*".into()), sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }), ); diff --git a/nodedb/tests/inproc/cases/executor_tests/test_kv_advanced.rs b/nodedb/tests/inproc/cases/executor_tests/test_kv_advanced.rs index 5dba10d7e..3132cf0fe 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_kv_advanced.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_kv_advanced.rs @@ -611,6 +611,8 @@ fn kv_index_write_amp_ratio_matches() { match_pattern: None, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }), ); let json: serde_json::Value = payload_value(&payload); diff --git a/nodedb/tests/inproc/cases/executor_tests/test_kv_scan_budget.rs b/nodedb/tests/inproc/cases/executor_tests/test_kv_scan_budget.rs index 28ba8fc7a..be11e865b 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_kv_scan_budget.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_kv_scan_budget.rs @@ -48,6 +48,8 @@ fn kv_scan(collection: &str, count: usize) -> PhysicalPlan { match_pattern: None, sort_keys: Vec::new(), surrogate_ceiling: None, + projection: Vec::new(), + computed_columns: Vec::new(), }) } diff --git a/nodedb/tests/wire/cases/kv_select_expressions.rs b/nodedb/tests/wire/cases/kv_select_expressions.rs new file mode 100644 index 000000000..5348b102f --- /dev/null +++ b/nodedb/tests/wire/cases/kv_select_expressions.rs @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! kv-engine SELECT projection expression evaluation. +//! +//! Previously the kv scan carried neither the SELECT projection list nor +//! computed columns, so expression projections were never evaluated: the +//! column came back NULL at response shaping (`SELECT 1 + 1 FROM kv` +//! returned an empty column) and sequence accessors could not raise their +//! typed 0A000 either. The scan now carries projection + computed columns +//! like the document/columnar paths, so: +//! +//! - scalar expressions over kv rows evaluate per row; +//! - `nextval`/`currval`/`setval` in a kv SELECT list raise 0A000 instead +//! of silently NULLing. + +use crate::harness::TestServer; + +async fn setup(server: &TestServer) { + server + .exec("CREATE COLLECTION kvsel (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'kv')") + .await + .unwrap(); + server + .exec("INSERT INTO kvsel (id, v) VALUES (1, 'hello'), (2, 'world')") + .await + .unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn scalar_expressions_evaluate_per_row() { + let server = TestServer::start().await; + setup(&server).await; + + let rows = server + .query_named_rows("SELECT upper(v) AS u, 1 + 1 AS s FROM kvsel ORDER BY id") + .await + .expect("rows"); + assert_eq!(rows.len(), 2, "{rows:?}"); + let u: Vec<_> = rows + .iter() + .map(|r| r.get("u").map(|s| s.as_str())) + .collect(); + assert_eq!(u, vec![Some("HELLO"), Some("WORLD")], "{rows:?}"); + let s: Vec<_> = rows + .iter() + .map(|r| r.get("s").map(|s| s.as_str())) + .collect(); + assert_eq!(s, vec![Some("2"), Some("2")], "{rows:?}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn accessor_in_select_list_raises_0a000() { + let server = TestServer::start().await; + setup(&server).await; + server + .expect_error("SELECT nextval('nope') FROM kvsel", "0A000") + .await; + server + .expect_error("SELECT currval('nope') FROM kvsel", "0A000") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn plain_projection_still_returns_stored_columns() { + let server = TestServer::start().await; + setup(&server).await; + let rows = server + .query_named_rows("SELECT id, v FROM kvsel ORDER BY id") + .await + .expect("rows"); + assert_eq!(rows.len(), 2, "{rows:?}"); + assert_eq!( + rows.iter() + .map(|r| r.get("v").map(|s| s.as_str())) + .collect::>(), + vec![Some("hello"), Some("world")] + ); +} diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index edb24a100..61993debf 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -95,6 +95,7 @@ mod http_result_projection; mod insert_select_cross_engine; mod kv_column_defaults; mod kv_predicate_dml; +mod kv_select_expressions; mod kv_sql_select; mod kv_write_row_level_security; mod kv_write_row_level_security_atomics; @@ -139,9 +140,11 @@ mod schema_visibility_barrier; mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; +mod sequence_const_select; mod sequence_default_all_engines; mod sequence_default_typed; mod sequence_expression_contexts; +mod sequence_matrix; mod serial_sequence_rollback_no_leak; mod session_handle_security; mod session_plan_cache_permission_tree_revoke; diff --git a/nodedb/tests/wire/cases/sequence_const_select.rs b/nodedb/tests/wire/cases/sequence_const_select.rs new file mode 100644 index 000000000..b2cc5fcfe --- /dev/null +++ b/nodedb/tests/wire/cases/sequence_const_select.rs @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Sequence accessors in constant (row-less) contexts: `SELECT nextval('s')` +//! without a FROM clause and explicit `VALUES (nextval('s'))` cells. +//! +//! These fold at plan time inside nodedb-sql, which has no registry; the +//! control plane installs a registry hook for the duration of planning so +//! each constant accessor advances exactly once, in expression order. +//! Row-scope contexts (over a table) keep raising 0A000 — only constant +//! contexts gain real evaluation. + +use crate::harness::TestServer; + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn fromless_nextval_advances_and_currval_reads_back() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE csel_seq").await.unwrap(); + + let rows = server + .query_named_rows("SELECT nextval('csel_seq') AS n, currval('csel_seq') AS c") + .await + .expect("rows"); + assert_eq!(rows.len(), 1, "{rows:?}"); + assert_eq!(rows[0].get("n").map(|s| s.as_str()), Some("1"), "{rows:?}"); + assert_eq!(rows[0].get("c").map(|s| s.as_str()), Some("1"), "{rows:?}"); + + // A second statement continues the same sequence. + let rows = server + .query_named_rows("SELECT nextval('csel_seq') AS n") + .await + .expect("rows"); + assert_eq!(rows[0].get("n").map(|s| s.as_str()), Some("2"), "{rows:?}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn values_cells_advance_per_row() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE csel_vseq").await.unwrap(); + server + .exec("CREATE COLLECTION csel_t (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'kv')") + .await + .unwrap(); + server + .exec("INSERT INTO csel_t (id, v) VALUES (nextval('csel_vseq'), 'a'), (nextval('csel_vseq'), 'b')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT id FROM csel_t ORDER BY id") + .await + .expect("rows"); + let ids: Vec<_> = rows + .iter() + .map(|r| r.get("id").map(|s| s.as_str())) + .collect(); + assert_eq!(ids, vec![Some("1"), Some("2")], "{rows:?}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn update_set_accessor_raises_0a000() { + // Row-scope DML expression: accessors must stay loud (0A000), never a + // once-folded value silently applied to every row. + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION upst (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'document_schemaless')") + .await + .unwrap(); + server + .exec("INSERT INTO upst (id, v) VALUES (1, 'a')") + .await + .unwrap(); + server + .expect_error("UPDATE upst SET v = nextval('upst_missing')", "0A000") + .await; + server + .expect_error( + "INSERT INTO upst (id, v) VALUES (1, 'b') ON CONFLICT (id) DO UPDATE SET v = nextval('upst_missing')", + "0A000", + ) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn explain_plans_without_advancing_the_sequence() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE csel_eseq").await.unwrap(); + server + .exec("EXPLAIN SELECT nextval('csel_eseq')") + .await + .expect("EXPLAIN must plan the accessor without executing it"); + let rows = server + .query_named_rows("SELECT nextval('csel_eseq') AS n") + .await + .expect("rows"); + assert_eq!( + rows[0].get("n").map(|s| s.as_str()), + Some("1"), + "EXPLAIN must not advance the sequence: {rows:?}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn multiple_accessors_in_one_statement_advance_in_order() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE csel_mseq").await.unwrap(); + let rows = server + .query_named_rows("SELECT nextval('csel_mseq') AS a, nextval('csel_mseq') AS b") + .await + .expect("rows"); + assert_eq!(rows.len(), 1, "{rows:?}"); + assert_eq!(rows[0].get("a").map(|s| s.as_str()), Some("1"), "{rows:?}"); + assert_eq!(rows[0].get("b").map(|s| s.as_str()), Some("2"), "{rows:?}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn nextval_continues_after_setval() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE csel_cseq").await.unwrap(); + server.exec("SELECT setval('csel_cseq', 41)").await.unwrap(); + let rows = server + .query_named_rows("SELECT nextval('csel_cseq') AS n") + .await + .expect("rows"); + assert_eq!(rows[0].get("n").map(|s| s.as_str()), Some("42"), "{rows:?}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn setval_const_expr_returns_value() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE csel_sseq").await.unwrap(); + let rows = server + .query_named_rows("SELECT setval('csel_sseq', 41) AS v") + .await + .expect("rows"); + assert_eq!(rows[0].get("v").map(|s| s.as_str()), Some("41"), "{rows:?}"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn unknown_sequence_in_const_context_raises() { + let server = TestServer::start().await; + // Registry miss surfaces as a plan error naming the sequence (same class + // as the DEFAULT path), never a silent NULL and never 0A000. + let err = server + .exec("SELECT nextval('csel_missing')") + .await + .expect_err("must raise"); + assert!( + err.to_string().contains("csel_missing"), + "error must name the sequence: {err}" + ); +} diff --git a/nodedb/tests/wire/cases/sequence_expression_contexts.rs b/nodedb/tests/wire/cases/sequence_expression_contexts.rs index 15b4cac50..f504cda6e 100644 --- a/nodedb/tests/wire/cases/sequence_expression_contexts.rs +++ b/nodedb/tests/wire/cases/sequence_expression_contexts.rs @@ -9,7 +9,9 @@ //! Registered (A1) + dispatch guard (A3) + fold classification (A5) make //! every escape loud: //! -//! - FROM-less `SELECT nextval('s')` — folded at plan time → classified. +//! - Constant contexts (`SELECT nextval('s')` without a FROM clause, +//! VALUES cells) evaluate through the CP registry (sequence_const_select); +//! a missing sequence raises a plan error naming it. //! - SELECT list / WHERE / ORDER BY over a table — row-scope eval. //! - VALUES / INSERT..SELECT — expression eval on the write path. //! - Derived-table constant expressions — same row-scope evaluator @@ -33,9 +35,13 @@ async fn setup_kv(server: &TestServer) { } #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn fromless_select_nextval_raises_0a000() { +async fn fromless_missing_sequence_raises_plan_error() { + // Constant contexts (no FROM clause) gain real registry evaluation + // (see sequence_const_select): nextval('existing') advances and a + // missing sequence raises the registry-miss plan error naming it — + // never a silent NULL. 0A000 remains reserved for row-scope contexts. let server = TestServer::start().await; - server.expect_error("SELECT nextval('nope')", "0A000").await; + server.expect_error("SELECT nextval('nope')", "42601").await; } #[tokio::test(flavor = "multi_thread", worker_threads = 4)] @@ -91,11 +97,11 @@ async fn order_by_nextval_raises_0a000() { } #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn values_nextval_raises_0a000() { +async fn values_missing_sequence_raises_plan_error() { let server = TestServer::start().await; setup_kv(&server).await; server - .expect_error("INSERT INTO seqctx (id) VALUES (nextval('nope'))", "0A000") + .expect_error("INSERT INTO seqctx (id) VALUES (nextval('nope'))", "42601") .await; } @@ -126,8 +132,11 @@ async fn derived_constant_errors_stay_loud() { server .expect_error("SELECT * FROM (SELECT mod(5, 0) AS v) d", "22012") .await; + // A derived-table constant cell is a constant context too: the accessor + // now evaluates through the CP registry, so a missing sequence raises + // the registry-miss plan error instead of 0A000. server - .expect_error("SELECT * FROM (SELECT nextval('nope') AS v) d", "0A000") + .expect_error("SELECT * FROM (SELECT nextval('nope') AS v) d", "42601") .await; } diff --git a/nodedb/tests/wire/cases/sequence_matrix.rs b/nodedb/tests/wire/cases/sequence_matrix.rs new file mode 100644 index 000000000..8df08bb06 --- /dev/null +++ b/nodedb/tests/wire/cases/sequence_matrix.rs @@ -0,0 +1,164 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Combinatorial surface test: engine x accessor x expression context. +//! +//! Row-scope contexts over a table must stay loud 0A000 for every accessor +//! on every table engine (no silent NULL, no mislabelled 22012). Constant +//! contexts evaluate through the CP registry (values advance; a missing +//! sequence raises a plan error naming it). + +use crate::harness::TestServer; + +const ENGINES: [(&str, &str); 4] = [ + ("kv", "kv"), + ("col", "columnar"), + ("docsl", "document_schemaless"), + ("docst", "document_strict"), +]; + +async fn make_table(server: &TestServer, engine: &str, name: &str) { + server + .exec(&format!( + "CREATE COLLECTION {name} (id BIGINT PRIMARY KEY, grp BIGINT, v TEXT) WITH (engine = '{engine}')" + )) + .await + .unwrap(); + server + .exec(&format!( + "INSERT INTO {name} (id, grp, v) VALUES (1, 1, 'a'), (2, 1, 'b')" + )) + .await + .unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn row_scope_contexts_raise_0a000_for_every_engine_and_accessor() { + let server = TestServer::start().await; + for (prefix, engine) in ENGINES { + let t = format!("mx_{prefix}"); + make_table(&server, engine, &t).await; + for accessor in ["nextval", "currval", "setval"] { + let call = match accessor { + "setval" => format!("{accessor}('mx_missing', 5)"), + _ => format!("{accessor}('mx_missing')"), + }; + // SELECT list + println!("MX sel {engine} {accessor}"); + server + .expect_error(&format!("SELECT {call} FROM {t}"), "0A000") + .await; + // WHERE + println!("MX whr {engine} {accessor}"); + server + .expect_error(&format!("SELECT id FROM {t} WHERE {call} > 0"), "0A000") + .await; + // ORDER BY + println!("MX ord {engine} {accessor}"); + server + .expect_error(&format!("SELECT id FROM {t} ORDER BY {call}"), "0A000") + .await; + // GROUP BY expression + println!("MX grp {engine} {accessor}"); + server + .expect_error( + &format!("SELECT count(*) FROM {t} GROUP BY {call}"), + "0A000", + ) + .await; + // HAVING + println!("MX hav {engine} {accessor}"); + server + .expect_error( + &format!("SELECT count(*) FROM {t} HAVING count(*) > {call}"), + "0A000", + ) + .await; + } + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_select_and_join_contexts_raise_0a000() { + let server = TestServer::start().await; + for (prefix, engine) in ENGINES { + let t = format!("mxj_{prefix}"); + make_table(&server, engine, &t).await; + // JOIN ON expression + server + .expect_error( + &format!( + "SELECT a.id FROM {t} a JOIN {t} b ON a.id = b.id AND nextval('mx_missing') > 0" + ), + "0A000", + ) + .await; + } + // INSERT..SELECT requires a document-family target. The source here is + // document-schemaless: kv-engine sources route their expression cells + // through the copy_rows column-map, and that pre-existing path drops + // expression projections (a kv-source `INSERT..SELECT` with a scalar + // expression writes NULL silently) — tracked as a follow-up, out of + // scope for accessor classification. + for (prefix, _engine) in ENGINES { + let src = format!("mxj_src_{prefix}"); + make_table(&server, "document_schemaless", &src).await; + server + .exec(&format!( + "CREATE COLLECTION mxj_dst_{prefix} (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = 'document_schemaless')" + )) + .await + .unwrap(); + println!("MX isel {prefix}"); + server + .expect_error( + &format!( + "INSERT INTO mxj_dst_{prefix} (id) SELECT nextval('mx_missing') FROM {src}" + ), + "0A000", + ) + .await; + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn constant_contexts_evaluate_for_every_engine() { + let server = TestServer::start().await; + for (prefix, engine) in ENGINES.iter() { + let seq = format!("mxc_seq_{prefix}"); + server + .exec(&format!("CREATE SEQUENCE {seq}")) + .await + .unwrap(); + + // FROM-less: value advances, order deterministic. + let rows = server + .query_named_rows(&format!("SELECT nextval('{seq}') AS n")) + .await + .expect("rows"); + assert_eq!(rows[0].get("n").map(|s| s.as_str()), Some("1"), "{rows:?}"); + + // VALUES cells advance per row on every engine. Dedicated empty + // table: the FROM-less call above already consumed value 1. + let t = format!("mxc_t_{prefix}"); + server + .exec(&format!( + "CREATE COLLECTION {t} (id BIGINT PRIMARY KEY, v TEXT) WITH (engine = '{engine}')" + )) + .await + .unwrap(); + let v = format!("INSERT INTO {t} (id) VALUES (nextval('{seq}')), (nextval('{seq}'))"); + match server.exec(&v).await { + Ok(_) => {} + Err(e) => panic!("VALUES advance must work on {engine}: {e}"), + } + let rows = server + .query_named_rows(&format!("SELECT count(*) AS c FROM {t} WHERE id IN (2, 3)")) + .await + .expect("rows"); + assert_eq!( + rows[0].get("c").map(|s| s.as_str()), + Some("2"), + "VALUES must have advanced to 2,3 on {engine}: {rows:?}" + ); + } +} From 62380a79633b09d07886b1bb56a5dbee0a3479b7 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 06:06:53 +0800 Subject: [PATCH 4/9] raise 42704 for unknown sequences named in DEFAULTs An insert whose DEFAULT names a missing sequence used to surface as a generic plan error (42601); the registry already carries the missing name. A sequence lookup that misses now becomes a typed UndefinedSequence error rendered as SQLSTATE 42704 (undefined_object), matching PostgreSQL's class for nextval('missing'). Strict and kv converter paths both discriminate the registry miss; all other registry failures keep the plan-error class. --- nodedb-types/src/error/ctors/read_query_auth.rs | 16 ++++++++++++++++ nodedb/src/control/gateway/error_map/pgwire.rs | 1 + .../sql_plan_convert/dml/kv_and_vector.rs | 16 +++++++++++----- .../sql_plan_convert/value/sequence_default.rs | 3 +++ .../src/control/server/pgwire/types/error_map.rs | 3 +++ nodedb/src/error/types.rs | 6 ++++++ nodedb/src/error_classify.rs | 1 + .../wire/cases/sequence_default_all_engines.rs | 4 ++++ .../tests/wire/cases/sequence_default_typed.rs | 4 ++++ 9 files changed, 49 insertions(+), 5 deletions(-) diff --git a/nodedb-types/src/error/ctors/read_query_auth.rs b/nodedb-types/src/error/ctors/read_query_auth.rs index 446d5fd97..4366a9d6f 100644 --- a/nodedb-types/src/error/ctors/read_query_auth.rs +++ b/nodedb-types/src/error/ctors/read_query_auth.rs @@ -124,6 +124,22 @@ impl NodeDbError { /// window function. Distinct from `plan_error` so clients can match on /// the specific code (SQLSTATE `42883`, `undefined_function`) rather /// than parsing the message. + /// A DEFAULT expression names a sequence that does not exist. Distinct + /// from `plan_error` so clients can match on the specific code + /// (SQLSTATE `42704`, `undefined_object`) rather than parsing the + /// message — the same class PostgreSQL gives `nextval('missing')`. + pub fn undefined_sequence(name: impl Into) -> Self { + let name = name.into(); + Self { + code: ErrorCode::UNDEFINED_OBJECT, + message: format!("sequence \"{name}\" does not exist"), + details: ErrorDetails::UndefinedObject { + object: format!("sequence {name}"), + }, + cause: None, + } + } + pub fn undefined_function(name: impl Into) -> Self { let name = name.into(); Self { diff --git a/nodedb/src/control/gateway/error_map/pgwire.rs b/nodedb/src/control/gateway/error_map/pgwire.rs index 7835c27c8..da3db1d10 100644 --- a/nodedb/src/control/gateway/error_map/pgwire.rs +++ b/nodedb/src/control/gateway/error_map/pgwire.rs @@ -55,6 +55,7 @@ impl GatewayErrorMap { // directly). They must keep their typed SQLSTATEs here too. Error::DivisionByZero => (sqlstate::DIVISION_BY_ZERO, err.to_string()), Error::FeatureNotSupported { .. } => (sqlstate::FEATURE_NOT_SUPPORTED, err.to_string()), + Error::UndefinedSequence { .. } => (sqlstate::UNDEFINED_OBJECT, err.to_string()), _ => (sqlstate::INTERNAL_ERROR, err.to_string()), } } diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs b/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs index ce40fe145..f89066623 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs @@ -237,11 +237,17 @@ fn sequence_default_value(ctx: &ConvertContext, expr: &str) -> crate::Result v, + Err(crate::control::sequence::SequenceError::NotFound { .. }) => { + return Err(crate::Error::UndefinedSequence { name }); + } + Err(e) => { + return Err(crate::Error::PlanError { + detail: format!("nextval('{name}'): {e}"), + }); + } + }; Ok(SqlValue::Int(value)) } diff --git a/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs b/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs index ff40ab318..acc71b52d 100644 --- a/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs +++ b/nodedb/src/control/planner/sql_plan_convert/value/sequence_default.rs @@ -39,6 +39,9 @@ fn evaluate_sequence_default(ctx: &ConvertContext, expr: &str) -> crate::Result< }; match registry.nextval(ctx.database_id.as_u64(), ctx.tenant_id.as_u64(), &name) { Ok(value) => Value::Integer(value), + Err(crate::control::sequence::SequenceError::NotFound { .. }) => { + return Err(crate::Error::UndefinedSequence { name }); + } Err(e) => { return Err(crate::Error::PlanError { detail: format!("nextval('{name}'): {e}"), diff --git a/nodedb/src/control/server/pgwire/types/error_map.rs b/nodedb/src/control/server/pgwire/types/error_map.rs index 7edec22c2..b374e88b3 100644 --- a/nodedb/src/control/server/pgwire/types/error_map.rs +++ b/nodedb/src/control/server/pgwire/types/error_map.rs @@ -67,6 +67,9 @@ pub fn error_to_sqlstate(err: &crate::Error) -> (&'static str, &'static str, Str crate::Error::FeatureNotSupported { .. } => { ("ERROR", sqlstate::FEATURE_NOT_SUPPORTED, err.to_string()) } + crate::Error::UndefinedSequence { .. } => { + ("ERROR", sqlstate::UNDEFINED_OBJECT, err.to_string()) + } crate::Error::InvalidLimitValue { .. } => { ("ERROR", sqlstate::INVALID_LIMIT_VALUE, err.to_string()) } diff --git a/nodedb/src/error/types.rs b/nodedb/src/error/types.rs index b9b7fe246..d372eaf56 100644 --- a/nodedb/src/error/types.rs +++ b/nodedb/src/error/types.rs @@ -282,6 +282,12 @@ pub enum Error { #[error("function {name}(...) does not exist")] UndefinedFunction { name: String }, + /// A DEFAULT expression names a sequence the registry does not hold. + /// Rendered as SQLSTATE `42704` (undefined_object), matching + /// PostgreSQL's `nextval('missing')`. + #[error("sequence \"{name}\" does not exist")] + UndefinedSequence { name: String }, + /// A column reference resolved against no relation, output alias, or /// synthetic column in scope. Propagated from `SqlError::UnknownColumn`; /// the pgwire layer renders it as SQLSTATE `42703` (undefined_column). diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index 6b1003433..7fa809df0 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -150,6 +150,7 @@ pub(crate) fn classify(e: &Error) -> NodeDbError { } Error::PlanError { detail } => NodeDbError::plan_error(detail), Error::UndefinedFunction { name } => NodeDbError::undefined_function(name.clone()), + Error::UndefinedSequence { name } => NodeDbError::undefined_sequence(name.clone()), Error::UndefinedColumn { column } => NodeDbError::undefined_column(column.clone()), Error::AmbiguousColumn { column } => NodeDbError::ambiguous_column(column.clone()), Error::UnknownStrictField { column, .. } => NodeDbError::undefined_column(column.clone()), diff --git a/nodedb/tests/wire/cases/sequence_default_all_engines.rs b/nodedb/tests/wire/cases/sequence_default_all_engines.rs index bcaacac88..7c7e97814 100644 --- a/nodedb/tests/wire/cases/sequence_default_all_engines.rs +++ b/nodedb/tests/wire/cases/sequence_default_all_engines.rs @@ -97,6 +97,10 @@ async fn kv_default_unknown_sequence_raises() { .exec("INSERT INTO kbad (v) VALUES ('x')") .await .unwrap_err(); + assert!( + err.contains("42704"), + "unknown-sequence DEFAULT must raise 42704 (undefined_object): {err}" + ); assert!( err.contains("no_kv_seq"), "error must name the missing sequence: {err}" diff --git a/nodedb/tests/wire/cases/sequence_default_typed.rs b/nodedb/tests/wire/cases/sequence_default_typed.rs index da948cbd0..f3a5d4e39 100644 --- a/nodedb/tests/wire/cases/sequence_default_typed.rs +++ b/nodedb/tests/wire/cases/sequence_default_typed.rs @@ -73,6 +73,10 @@ async fn strict_default_unknown_sequence_raises() { .exec("INSERT INTO bad (v) VALUES ('x')") .await .unwrap_err(); + assert!( + err.contains("42704"), + "unknown-sequence DEFAULT must raise 42704 (undefined_object): {err}" + ); assert!( err.contains("no_such_seq"), "error must name the missing sequence: {err}" From f53a634ee5aeae8b1abab86d4d27a137c7006207 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 08:33:23 +0800 Subject: [PATCH 5/9] raise expression errors over constant derived tables instead of folding them Outer projections and aggregate/group-key arguments over a constant derived table used to fold silently: the CTE body materialized as bare value rows, the response shaper found no computed alias and emitted NULL, and aggregates over the body scanned an empty collection. Computed projection expressions now ride on the materialized-row scans as computed columns and evaluate per row (division raises 22012, sequence accessors raise 0A000); aggregates whose input is a non-Scan body lower it to a ProviderScan sub-plan so the accumulator receives the rows and evaluates its arguments and group keys against them. Window functions over a constant derived table are still folded silently; that path needs window evaluation in the row post-processor and is tracked separately. --- nodedb-physical/src/physical_plan/query.rs | 11 ++++ nodedb/src/control/clone/resolver/rewrite.rs | 2 + .../sql_plan_convert/aggregate/plan.rs | 60 +++++++++++++++++++ .../planner/sql_plan_convert/convert.rs | 1 + .../control/planner/sql_plan_convert/expr.rs | 24 +++++++- .../planner/sql_plan_convert/scan/core.rs | 1 + .../planner/sql_plan_convert/set_ops.rs | 9 +++ .../exchange/resolve/exchange/dispatch.rs | 2 + .../resolve/exchange/post_process_arm.rs | 3 + .../server/exchange/resolve/join_input.rs | 3 + .../server/exchange/resolve/materialize.rs | 4 ++ nodedb/src/data/executor/dispatch/query.rs | 2 + .../data/executor/handlers/provider_scan.rs | 53 +++++++++++++++- .../wire/cases/derived_expression_errors.rs | 53 ++++++++++++++++ nodedb/tests/wire/cases/mod.rs | 1 + 15 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 nodedb/tests/wire/cases/derived_expression_errors.rs diff --git a/nodedb-physical/src/physical_plan/query.rs b/nodedb-physical/src/physical_plan/query.rs index 96f087d3c..4c0f303c1 100644 --- a/nodedb-physical/src/physical_plan/query.rs +++ b/nodedb-physical/src/physical_plan/query.rs @@ -78,6 +78,13 @@ pub enum QueryOp { /// Output column names to keep. Empty = emit all columns. #[serde(default)] projection: Vec, + /// Serialized `Vec` applied per row after + /// projection-name extraction (same wire format as the engine + /// scans). Expression projections over materialized rows (derived + /// tables, constant subqueries) need these — name-only projection + /// would silently drop them. + #[serde(default)] + computed_columns: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, @@ -118,6 +125,10 @@ pub enum QueryOp { /// Output column names to keep. Empty = emit all columns. #[serde(default)] projection: Vec, + /// Serialized `Vec` applied per row (see + /// `ProviderScan::computed_columns`). + #[serde(default)] + computed_columns: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, diff --git a/nodedb/src/control/clone/resolver/rewrite.rs b/nodedb/src/control/clone/resolver/rewrite.rs index 79228efd3..045b92a1c 100644 --- a/nodedb/src/control/clone/resolver/rewrite.rs +++ b/nodedb/src/control/clone/resolver/rewrite.rs @@ -117,6 +117,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -139,6 +140,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res input: child, filters: filters.clone(), projection: projection.clone(), + computed_columns: computed_columns.clone(), sort_keys: sort_keys.clone(), limit: *limit, offset: *offset, diff --git a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs index 2f9c4c3bc..271499132 100644 --- a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs +++ b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs @@ -166,6 +166,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( // before the rows reach the aggregate. filters: filter_bytes.clone(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -198,6 +199,65 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( }]); } + // Aggregate over a derived/CTE body: the input is not a Scan (a + // constant subquery, a set operation, a join materialized earlier), so + // there is no per-shard collection to aggregate. Lower the body to a + // single coordinator-local ProviderScan sub-plan — the same shape the + // catalog path uses above — so the executor receives the body's rows and + // evaluates the aggregate arguments / group keys against them. Without + // this the aggregate scanned an empty (non-existent) collection and + // silently returned NULL / no rows (issue #295). + if !matches!(input, SqlPlan::Scan { .. }) { + let derived_group_specs = group_by_to_specs(group_by); + let derived_agg_specs: Vec = + aggregates.iter().map(agg_expr_to_spec).collect(); + let mut body_tasks = + super::super::convert::convert_one(input, tenant_id, ctx)?; + if body_tasks.len() == 1 { + let body_plan = body_tasks.pop().expect("len == 1").plan; + let body_provider = if let PhysicalPlan::Query(QueryOp::ProviderScan { + rows, filters, .. + }) = &body_plan + { + PhysicalPlan::Query(QueryOp::ProviderScan { + provider: None, + rows: rows.clone(), + filters: filters.clone(), + projection: Vec::new(), + computed_columns: Vec::new(), + sort_keys: Vec::new(), + limit: None, + offset: 0, + distinct: false, + }) + } else { + body_plan + }; + return Ok(vec![PhysicalTask { + tenant_id, + vshard_id: VShardId::from_collection_in_database(ctx.database_id, ""), + database_id: ctx.database_id, + plan: PhysicalPlan::Query(QueryOp::Aggregate { + collection: nodedb_types::QualifiedCollection::from_stored( + raw_collection.clone(), + ), + input: Some(Box::new(body_provider)), + group_by: derived_group_specs, + aggregates: derived_agg_specs, + filters: Vec::new(), + having: having_bytes, + limit, + sub_group_by: Vec::new(), + sub_aggregates: Vec::new(), + grouping_sets: Vec::new(), + sort_keys: bridge_sort_keys, + }), + post_set_op: PostSetOp::None, + txn_id: None, + }]); + } + } + let collection = db_qualified(ctx.database_id, &raw_collection); let qualified_collection = nodedb_types::QualifiedCollection::new(ctx.database_id, &raw_collection); diff --git a/nodedb/src/control/planner/sql_plan_convert/convert.rs b/nodedb/src/control/planner/sql_plan_convert/convert.rs index 44b091081..db89cf33c 100644 --- a/nodedb/src/control/planner/sql_plan_convert/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/convert.rs @@ -239,6 +239,7 @@ pub fn convert( rows: Vec::new(), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index 80119f4ff..c866d7c13 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -280,6 +280,22 @@ pub(super) fn convert_sort_keys(keys: &[SortKey]) -> Vec { .collect() } +/// Whether a projection list carries anything beyond bare column +/// references / stars. A computed expression (`x/0`, a function call, a +/// window over a column) has no row to evaluate against once the CTE body +/// is inlined as a bare value row — the response shaper would look the +/// aliased column up, find nothing, and emit NULL. Such projections need a +/// real Subquery post-processor over the materialized rows, not a bare +/// `cte_plan.clone()`. +fn has_expression_projection(projection: &[nodedb_sql::types::query::Projection]) -> bool { + projection.iter().any(|p| match p { + nodedb_sql::types::query::Projection::Computed { .. } => true, + nodedb_sql::types::query::Projection::Column(_) + | nodedb_sql::types::query::Projection::Star + | nodedb_sql::types::query::Projection::QualifiedStar(_) => false, + }) +} + /// Replace scans on `cte_name` with the CTE's actual subquery plan. /// /// Outer constraints on the CTE reference are merged onto the CTE body as far @@ -298,6 +314,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> limit, offset, distinct, + window_functions, .. } if collection == cte_name => { // If the outer query adds filters/sort/limit, wrap the CTE plan. @@ -399,11 +416,14 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> && *offset == 0 && !*distinct && limit.is_none() + && !has_expression_projection(projection) + && window_functions.is_empty() { // Any other non-`Scan` body (Aggregate, Join, TextSearch, // HybridSearch, SparseSearch, SpatialScan, MultiVectorSearch, - // ...) with only an outer projection: the response boundary - // projects by output schema, so no post-processor is needed. + // ...) with only an outer projection of bare columns: the + // response boundary projects by output schema, so no + // post-processor is needed. cte_plan.clone() } else { // The body has no slot for these outer constraints. Apply diff --git a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs index 642740260..cfcc4298e 100644 --- a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs +++ b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs @@ -58,6 +58,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_scan( rows: Vec::new(), filters: filter_bytes, projection: proj_names, + computed_columns: Vec::new(), sort_keys: sort, limit: *limit, offset: *offset, diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index 9fe807b16..69a5980ac 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -41,6 +41,7 @@ pub(super) fn convert_constant_result( rows: payload, filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -309,6 +310,14 @@ pub(super) fn convert_subquery( input: Box::new(child), filters: super::filter::serialize_filters(filters)?, projection: lower_subquery_projection(projection)?, + // Expression projections ride as computed columns so the + // materialized-row ProviderScan evaluates them per row instead + // of the response shaper looking up an alias that was never + // computed (silent NULL — issue #295). + computed_columns: super::aggregate::extract_computed_columns( + projection, + &[], + )?, sort_keys: lower_subquery_sort_keys(sort_keys, merged_doc_body), limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs index c312c65c4..87cd571e8 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs @@ -157,6 +157,7 @@ pub(super) async fn resolve_exchange( input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -170,6 +171,7 @@ pub(super) async fn resolve_exchange( input, filters, projection, + computed_columns, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs index 444ffb90b..d855d1b01 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs @@ -28,6 +28,7 @@ pub(super) struct PostProcessFields { pub input: Box, pub filters: Vec, pub projection: Vec, + pub computed_columns: Vec, pub sort_keys: Vec, pub limit: Option, pub offset: usize, @@ -106,6 +107,7 @@ pub(super) async fn resolve_post_process( input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -235,6 +237,7 @@ pub(super) async fn resolve_post_process( rows, filters, projection, + computed_columns, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/join_input.rs b/nodedb/src/control/server/exchange/resolve/join_input.rs index 16a50d81b..a363263c9 100644 --- a/nodedb/src/control/server/exchange/resolve/join_input.rs +++ b/nodedb/src/control/server/exchange/resolve/join_input.rs @@ -60,6 +60,7 @@ pub(super) async fn resolve_join_input( rows: flatten_to_relational_rows(&outcome.merged_array), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -134,6 +135,7 @@ pub(super) async fn resolve_join_input( rows: flatten_to_relational_rows(&merged), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -236,6 +238,7 @@ pub(super) async fn gather_join_build_side( rows: flatten_to_relational_rows(&outcome.merged_array), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/server/exchange/resolve/materialize.rs b/nodedb/src/control/server/exchange/resolve/materialize.rs index 1d2a2c417..26e1a99db 100644 --- a/nodedb/src/control/server/exchange/resolve/materialize.rs +++ b/nodedb/src/control/server/exchange/resolve/materialize.rs @@ -34,6 +34,7 @@ pub(super) async fn materialize_providers( rows: _, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -46,6 +47,7 @@ pub(super) async fn materialize_providers( rows: encoded, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -223,6 +225,7 @@ pub(super) async fn materialize_providers( input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -233,6 +236,7 @@ pub(super) async fn materialize_providers( input: Box::new(input), filters, projection, + computed_columns, sort_keys, limit, offset, diff --git a/nodedb/src/data/executor/dispatch/query.rs b/nodedb/src/data/executor/dispatch/query.rs index 180a112d0..b91b335f4 100644 --- a/nodedb/src/data/executor/dispatch/query.rs +++ b/nodedb/src/data/executor/dispatch/query.rs @@ -75,6 +75,7 @@ impl CoreLoop { limit, offset, distinct, + computed_columns, .. } => self.execute_provider_scan( task, @@ -82,6 +83,7 @@ impl CoreLoop { rows_bytes: rows, filters_bytes: filters, projection, + computed_columns, sort_keys, limit: *limit, offset: *offset, diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index 9ebf4d1d8..51e6fba41 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -21,6 +21,7 @@ pub(in crate::data::executor) struct ProviderScanParams<'a> { pub rows_bytes: &'a [u8], pub filters_bytes: &'a [u8], pub projection: &'a [String], + pub computed_columns: &'a [u8], pub sort_keys: &'a [nodedb_physical::physical_plan::SortKeySpec], pub limit: Option, pub offset: usize, @@ -41,6 +42,7 @@ impl CoreLoop { rows_bytes, filters_bytes, projection, + computed_columns, sort_keys, limit, offset, @@ -108,7 +110,56 @@ impl CoreLoop { return self.response_error(task, crate::Error::from(e)); } - // ── 5. Distinct (on the would-be projected row). ────────────────────── + // ── 5. Computed columns. ────────────────────────────────────────────── + // Expression projections over materialized rows (derived tables, + // constant subqueries) ride as computed columns: evaluate each per + // row BEFORE distinct/project so the aliased value exists in the row + // map and division/accessor errors fail the query instead of + // silently NULLing (issue #295). + if !computed_columns.is_empty() { + let computed_cols: Vec = + match zerompk::from_msgpack(computed_columns) { + Ok(c) => c, + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: malformed computed columns: {e}"), + }, + ); + } + }; + for row in rows.iter_mut() { + let Ok(doc_val) = nodedb_types::value_from_msgpack(row) else { + continue; + }; + let mut map = match doc_val { + nodedb_types::Value::Object(m) => m, + _ => continue, + }; + for cc in &computed_cols { + if matches!(map.get(&cc.alias), Some(v) if !v.is_null()) { + continue; + } + match cc.expr.eval(&nodedb_types::Value::Object(map.clone())) { + Ok(v) => { + map.insert(cc.alias.clone(), v); + } + Err(e) => { + return self + .response_error(task, ErrorCode::from(crate::Error::from(e))); + } + } + } + if let Ok(encoded) = + nodedb_types::value_to_msgpack(&nodedb_types::Value::Object(map)) + { + *row = encoded; + } + } + } + + // ── 6. Distinct (on the would-be projected row). ────────────────────── // Deduplicate on the projected shape so SQL DISTINCT semantics are // honoured: two rows with the same projected columns but different // non-projected columns are considered equal. diff --git a/nodedb/tests/wire/cases/derived_expression_errors.rs b/nodedb/tests/wire/cases/derived_expression_errors.rs new file mode 100644 index 000000000..a90f912fe --- /dev/null +++ b/nodedb/tests/wire/cases/derived_expression_errors.rs @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Expression errors over a constant derived table must raise, not fold to +//! NULL / empty rows (issue #295). The derived body materializes as rows on +//! the coordinator; expression projections and aggregate/group-key arguments +//! evaluate against those rows per-row, so division raises 22012 and +//! sequence accessors raise 0A000 instead of silently vanishing. + +use crate::harness::TestServer; + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn projection_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error("SELECT x/0 FROM (SELECT 1 AS x) s", "22012") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn projection_division_over_derived_with_filter_raises() { + let server = TestServer::start().await; + server + .expect_error("SELECT x/0 FROM (SELECT 1 AS x) s WHERE x > 0", "22012") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn aggregate_argument_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error("SELECT sum(x/0) FROM (SELECT 1 AS x) s", "22012") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn group_by_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error( + "SELECT x, count(*) FROM (SELECT 1 AS x) s GROUP BY x/0", + "22012", + ) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn accessor_over_derived_is_loud() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE der_seq").await.unwrap(); + server + .expect_error("SELECT nextval('der_seq') FROM (SELECT 1 AS x) s", "0A000") + .await; +} diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 61993debf..28c626551 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -141,6 +141,7 @@ mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; mod sequence_const_select; +mod derived_expression_errors; mod sequence_default_all_engines; mod sequence_default_typed; mod sequence_expression_contexts; From 0748b3cd3940c1bce6dc5f005f775020bf3bc1a1 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:13:07 +0800 Subject: [PATCH 6/9] Carry window specs through derived-table post-processing and evaluate them per row The derived-table tail now transports window function specs from the outer query all the way to the materialized-row scan, which evaluates each spec per partition after computed columns. Partition, ordering, and argument errors fail the query instead of answering NULL. The subquery fold and validation walks now cover window expressions, so catalog casts inside window clauses fold and validate like every other expression on the wrapper. --- nodedb-physical/src/physical_plan/query.rs | 9 +++ nodedb-sql/src/planner/catalog_fold.rs | 6 ++ .../src/planner/catalog_plan_validate.rs | 2 + nodedb-sql/src/planner/select/entry.rs | 2 + nodedb-sql/src/planner/select/post_process.rs | 1 + nodedb-sql/src/types/plan/variants.rs | 3 + nodedb-sql/src/visitor/plan_visitor/args.rs | 1 + .../src/visitor/plan_visitor/dispatch_rest.rs | 2 + nodedb/src/control/clone/resolver/rewrite.rs | 4 ++ .../control/planner/redaction_refusal/plan.rs | 2 + .../rls_injection/permission_tree/plan.rs | 2 + .../src/control/planner/rls_injection/plan.rs | 2 + .../sql_plan_convert/aggregate/plan.rs | 13 +++-- .../planner/sql_plan_convert/convert.rs | 1 + .../control/planner/sql_plan_convert/expr.rs | 4 ++ .../planner/sql_plan_convert/scan/core.rs | 1 + .../planner/sql_plan_convert/set_ops.rs | 8 ++- .../exchange/resolve/exchange/dispatch.rs | 2 + .../resolve/exchange/post_process_arm.rs | 3 + .../server/exchange/resolve/join_input.rs | 5 +- .../server/exchange/resolve/materialize.rs | 4 ++ .../shared/authorization/requirements.rs | 2 + .../predicate/txn_buffering/classify.rs | 2 + nodedb/src/data/executor/dispatch/query.rs | 4 +- .../data/executor/handlers/provider_scan.rs | 58 +++++++++++++++++++ .../test_cross_type_join/inline_hash_join.rs | 4 ++ .../test_cross_type_join/multi_core_joins.rs | 6 ++ .../wire/cases/derived_expression_errors.rs | 11 ++++ nodedb/tests/wire/cases/mod.rs | 2 +- 29 files changed, 157 insertions(+), 9 deletions(-) diff --git a/nodedb-physical/src/physical_plan/query.rs b/nodedb-physical/src/physical_plan/query.rs index 4c0f303c1..5de8629f4 100644 --- a/nodedb-physical/src/physical_plan/query.rs +++ b/nodedb-physical/src/physical_plan/query.rs @@ -85,6 +85,11 @@ pub enum QueryOp { /// would silently drop them. #[serde(default)] computed_columns: Vec, + /// Serialized `Vec` evaluated per partition after + /// computed columns (window over derived-table rows — issue #295 + /// Gap 3). Empty = no window functions. + #[serde(default)] + window_functions: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, @@ -129,6 +134,10 @@ pub enum QueryOp { /// `ProviderScan::computed_columns`). #[serde(default)] computed_columns: Vec, + /// Serialized `Vec` (see + /// `ProviderScan::window_functions`). + #[serde(default)] + window_functions: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, diff --git a/nodedb-sql/src/planner/catalog_fold.rs b/nodedb-sql/src/planner/catalog_fold.rs index d29db868b..88ee8067c 100644 --- a/nodedb-sql/src/planner/catalog_fold.rs +++ b/nodedb-sql/src/planner/catalog_fold.rs @@ -112,6 +112,7 @@ fn walk_plan( input, mut filters, mut projection, + mut window_functions, mut sort_keys, offset, distinct, @@ -122,10 +123,15 @@ fn walk_plan( } fold_projection(&mut projection, catalog, database_id, tenant_id); fold_sort_keys(&mut sort_keys, catalog, database_id, tenant_id); + // Window specs carry their own exprs (args, PARTITION BY, + // ORDER BY) — a wrapper that skips them leaves catalog casts + // inside window exprs unfolded, which then match no row. + fold_windows(&mut window_functions, catalog, database_id, tenant_id); SqlPlan::Subquery { input: Box::new(walk_plan(*input, catalog, database_id, tenant_id)), filters, projection, + window_functions, sort_keys, offset, distinct, diff --git a/nodedb-sql/src/planner/catalog_plan_validate.rs b/nodedb-sql/src/planner/catalog_plan_validate.rs index 8f947256e..2f569b626 100644 --- a/nodedb-sql/src/planner/catalog_plan_validate.rs +++ b/nodedb-sql/src/planner/catalog_plan_validate.rs @@ -107,12 +107,14 @@ pub(super) fn validate_catalog_exprs( filters, projection, sort_keys, + window_functions, .. } => { validate_catalog_exprs(input, catalog, database_id, tenant_id)?; validate_filters(filters, catalog, database_id, tenant_id)?; validate_projection(projection, catalog, database_id, tenant_id)?; validate_sort_keys(sort_keys, catalog, database_id, tenant_id)?; + validate_windows(window_functions, catalog, database_id, tenant_id)?; } SqlPlan::Join { left, diff --git a/nodedb-sql/src/planner/select/entry.rs b/nodedb-sql/src/planner/select/entry.rs index 48a41f8f6..70a2dc426 100644 --- a/nodedb-sql/src/planner/select/entry.rs +++ b/nodedb-sql/src/planner/select/entry.rs @@ -173,6 +173,7 @@ pub fn plan_query( SqlPlan::Subquery { filters, projection, + window_functions, sort_keys, offset, distinct, @@ -182,6 +183,7 @@ pub fn plan_query( input: Box::new(upgraded_leaf), filters, projection, + window_functions, sort_keys, offset, distinct, diff --git a/nodedb-sql/src/planner/select/post_process.rs b/nodedb-sql/src/planner/select/post_process.rs index 3d351cefa..39a6f5692 100644 --- a/nodedb-sql/src/planner/select/post_process.rs +++ b/nodedb-sql/src/planner/select/post_process.rs @@ -37,6 +37,7 @@ pub(in crate::planner::select) fn post_process( input: Box::new(input), filters: Vec::new(), projection, + window_functions: Vec::new(), sort_keys, offset, distinct: false, diff --git a/nodedb-sql/src/types/plan/variants.rs b/nodedb-sql/src/types/plan/variants.rs index 4752b0bb6..cf689c04e 100644 --- a/nodedb-sql/src/types/plan/variants.rs +++ b/nodedb-sql/src/types/plan/variants.rs @@ -517,6 +517,9 @@ pub enum SqlPlan { filters: Vec, /// Outer projection (target list). Empty = inherit the body's columns. projection: Vec, + /// Window functions evaluated over the materialized rows. Empty = + /// none. + window_functions: Vec, /// Outer `ORDER BY` keys applied over the materialized rows. sort_keys: Vec, /// Outer `OFFSET` (0 = none). diff --git a/nodedb-sql/src/visitor/plan_visitor/args.rs b/nodedb-sql/src/visitor/plan_visitor/args.rs index 206457f44..b94aea1ad 100644 --- a/nodedb-sql/src/visitor/plan_visitor/args.rs +++ b/nodedb-sql/src/visitor/plan_visitor/args.rs @@ -37,6 +37,7 @@ pub struct SubqueryVisitArgs<'a> { pub input: &'a SqlPlan, pub filters: &'a [Filter], pub projection: &'a [Projection], + pub window_functions: &'a [crate::types::WindowSpec], pub sort_keys: &'a [SortKey], pub offset: usize, pub distinct: bool, diff --git a/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs b/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs index 2572ae135..fafc6a4db 100644 --- a/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs +++ b/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs @@ -26,6 +26,7 @@ pub(super) fn dispatch_rest( input, filters, projection, + window_functions, sort_keys, offset, distinct, @@ -34,6 +35,7 @@ pub(super) fn dispatch_rest( input, filters, projection, + window_functions, sort_keys, offset: *offset, distinct: *distinct, diff --git a/nodedb/src/control/clone/resolver/rewrite.rs b/nodedb/src/control/clone/resolver/rewrite.rs index 045b92a1c..81803de84 100644 --- a/nodedb/src/control/clone/resolver/rewrite.rs +++ b/nodedb/src/control/clone/resolver/rewrite.rs @@ -118,6 +118,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -141,6 +142,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res filters: filters.clone(), projection: projection.clone(), computed_columns: computed_columns.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), limit: *limit, offset: *offset, @@ -632,6 +634,8 @@ mod tests { input: Box::new(gather(plan)), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/redaction_refusal/plan.rs b/nodedb/src/control/planner/redaction_refusal/plan.rs index dd71f71eb..4e1f08fe0 100644 --- a/nodedb/src/control/planner/redaction_refusal/plan.rs +++ b/nodedb/src/control/planner/redaction_refusal/plan.rs @@ -461,6 +461,8 @@ mod tests { input: Box::new(aggregate_plan("users", vec![agg_spec("min", "ssn")])), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs b/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs index 742d8716f..0ada83e52 100644 --- a/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs +++ b/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs @@ -414,6 +414,8 @@ mod tests { input: Box::new(columnar_scan("events")), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/rls_injection/plan.rs b/nodedb/src/control/planner/rls_injection/plan.rs index 0df53ece6..e002ba5a0 100644 --- a/nodedb/src/control/planner/rls_injection/plan.rs +++ b/nodedb/src/control/planner/rls_injection/plan.rs @@ -412,6 +412,8 @@ mod tests { input: Box::new(rag_fusion("docs")), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs index 271499132..08625635f 100644 --- a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs +++ b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs @@ -167,6 +167,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( filters: filter_bytes.clone(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -211,12 +212,15 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( let derived_group_specs = group_by_to_specs(group_by); let derived_agg_specs: Vec = aggregates.iter().map(agg_expr_to_spec).collect(); - let mut body_tasks = - super::super::convert::convert_one(input, tenant_id, ctx)?; + let mut body_tasks = super::super::convert::convert_one(input, tenant_id, ctx)?; if body_tasks.len() == 1 { let body_plan = body_tasks.pop().expect("len == 1").plan; let body_provider = if let PhysicalPlan::Query(QueryOp::ProviderScan { - rows, filters, .. + rows, + filters, + computed_columns, + window_functions, + .. }) = &body_plan { PhysicalPlan::Query(QueryOp::ProviderScan { @@ -224,7 +228,8 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( rows: rows.clone(), filters: filters.clone(), projection: Vec::new(), - computed_columns: Vec::new(), + computed_columns: computed_columns.clone(), + window_functions: window_functions.clone(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/convert.rs b/nodedb/src/control/planner/sql_plan_convert/convert.rs index db89cf33c..6e1ecfacd 100644 --- a/nodedb/src/control/planner/sql_plan_convert/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/convert.rs @@ -240,6 +240,7 @@ pub fn convert( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index c866d7c13..f41aac633 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -403,6 +403,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input: Box::new(leaf), filters: Vec::new(), projection: projection.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), offset: *offset, distinct: *distinct, @@ -433,6 +434,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input: Box::new(cte_plan.clone()), filters: filters.clone(), projection: projection.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), offset: *offset, distinct: *distinct, @@ -528,6 +530,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input, filters, projection, + window_functions, sort_keys, offset, distinct, @@ -536,6 +539,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input: Box::new(inline_cte(input, cte_name, cte_plan)), filters: filters.clone(), projection: projection.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), offset: *offset, distinct: *distinct, diff --git a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs index cfcc4298e..4d34d8cb7 100644 --- a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs +++ b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs @@ -59,6 +59,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_scan( filters: filter_bytes, projection: proj_names, computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: sort, limit: *limit, offset: *offset, diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index 69a5980ac..2fd342fe2 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -42,6 +42,7 @@ pub(super) fn convert_constant_result( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -246,6 +247,7 @@ pub(super) fn convert_subquery( input, filters, projection, + window_functions, sort_keys, offset, distinct, @@ -313,11 +315,13 @@ pub(super) fn convert_subquery( // Expression projections ride as computed columns so the // materialized-row ProviderScan evaluates them per row instead // of the response shaper looking up an alias that was never - // computed (silent NULL — issue #295). + // computed (silent NULL — issue #295). Window-aliased items are + // excluded here; they ride as window specs below. computed_columns: super::aggregate::extract_computed_columns( projection, - &[], + window_functions, )?, + window_functions: super::aggregate::serialize_window_functions(window_functions)?, sort_keys: lower_subquery_sort_keys(sort_keys, merged_doc_body), limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs index 87cd571e8..fcdccdade 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs @@ -158,6 +158,7 @@ pub(super) async fn resolve_exchange( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -172,6 +173,7 @@ pub(super) async fn resolve_exchange( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs index d855d1b01..bcf964ad7 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs @@ -29,6 +29,7 @@ pub(super) struct PostProcessFields { pub filters: Vec, pub projection: Vec, pub computed_columns: Vec, + pub window_functions: Vec, pub sort_keys: Vec, pub limit: Option, pub offset: usize, @@ -108,6 +109,7 @@ pub(super) async fn resolve_post_process( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -238,6 +240,7 @@ pub(super) async fn resolve_post_process( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/join_input.rs b/nodedb/src/control/server/exchange/resolve/join_input.rs index a363263c9..2d3e1ab32 100644 --- a/nodedb/src/control/server/exchange/resolve/join_input.rs +++ b/nodedb/src/control/server/exchange/resolve/join_input.rs @@ -61,6 +61,7 @@ pub(super) async fn resolve_join_input( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -136,6 +137,7 @@ pub(super) async fn resolve_join_input( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -238,7 +240,8 @@ pub(super) async fn gather_join_build_side( rows: flatten_to_relational_rows(&outcome.merged_array), filters: Vec::new(), projection: Vec::new(), - computed_columns: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/server/exchange/resolve/materialize.rs b/nodedb/src/control/server/exchange/resolve/materialize.rs index 26e1a99db..a4b53518d 100644 --- a/nodedb/src/control/server/exchange/resolve/materialize.rs +++ b/nodedb/src/control/server/exchange/resolve/materialize.rs @@ -35,6 +35,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -48,6 +49,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -226,6 +228,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -237,6 +240,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/shared/authorization/requirements.rs b/nodedb/src/control/server/shared/authorization/requirements.rs index dc7b1e6c6..7655de50f 100644 --- a/nodedb/src/control/server/shared/authorization/requirements.rs +++ b/nodedb/src/control/server/shared/authorization/requirements.rs @@ -73,6 +73,8 @@ mod tests { rows: Vec::new(), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs index 749ad277c..7f6772571 100644 --- a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs +++ b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs @@ -1519,6 +1519,8 @@ mod tests { rows: Vec::new(), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/data/executor/dispatch/query.rs b/nodedb/src/data/executor/dispatch/query.rs index b91b335f4..7c7f9528d 100644 --- a/nodedb/src/data/executor/dispatch/query.rs +++ b/nodedb/src/data/executor/dispatch/query.rs @@ -68,6 +68,7 @@ impl CoreLoop { ), QueryOp::ProviderScan { + provider: _, rows, filters, projection, @@ -76,7 +77,7 @@ impl CoreLoop { offset, distinct, computed_columns, - .. + window_functions, } => self.execute_provider_scan( task, crate::data::executor::handlers::provider_scan::ProviderScanParams { @@ -84,6 +85,7 @@ impl CoreLoop { filters_bytes: filters, projection, computed_columns, + window_functions, sort_keys, limit: *limit, offset: *offset, diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index 51e6fba41..74d25dc3a 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -22,6 +22,7 @@ pub(in crate::data::executor) struct ProviderScanParams<'a> { pub filters_bytes: &'a [u8], pub projection: &'a [String], pub computed_columns: &'a [u8], + pub window_functions: &'a [u8], pub sort_keys: &'a [nodedb_physical::physical_plan::SortKeySpec], pub limit: Option, pub offset: usize, @@ -43,6 +44,7 @@ impl CoreLoop { filters_bytes, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -159,6 +161,62 @@ impl CoreLoop { } } + // ── 5b. Window functions. ─────────────────────────────────────────── + // Window-over-derived-table (issue #295 Gap 3): evaluate each spec + // per partition AFTER computed columns (window args may reference + // computed aliases) and BEFORE distinct/project (the window alias + // must exist in the row map). Partition/order/argument errors — + // including division-by-zero — fail the query instead of + // silently NULLing. Evaluation is in place, so row order is kept. + if !window_functions.is_empty() { + let specs: Vec = + match zerompk::from_msgpack(window_functions) { + Ok(s) => s, + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: malformed window bytes: {e}"), + }, + ); + } + }; + if !specs.is_empty() && !rows.is_empty() { + let mut decoded: Vec<(String, serde_json::Value)> = Vec::with_capacity(rows.len()); + for (i, row) in rows.iter().enumerate() { + match nodedb_types::json_from_msgpack(row) { + Ok(v) => decoded.push((i.to_string(), v)), + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: window row decode: {e}"), + }, + ); + } + } + } + if let Err(e) = + crate::bridge::window_func::evaluate_window_functions(&mut decoded, &specs) + { + return self.response_error(task, ErrorCode::from(crate::Error::from(e))); + } + for (slot, (_, v)) in rows.iter_mut().zip(decoded) { + match nodedb_types::json_to_msgpack(&v) { + Ok(encoded) => *slot = encoded, + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: window row encode: {e}"), + }, + ); + } + } + } + } + } + // ── 6. Distinct (on the would-be projected row). ────────────────────── // Deduplicate on the projected shape so SQL DISTINCT semantics are // honoured: two rows with the same projected columns but different diff --git a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs index 8ce8e1420..5f30bf18d 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs @@ -171,6 +171,8 @@ fn inline_hash_join_honors_qualified_left_keys() { rows: response_codec::flatten_to_relational_rows(&left_data), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -181,6 +183,8 @@ fn inline_hash_join_honors_qualified_left_keys() { rows: response_codec::flatten_to_relational_rows(&right_data), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs index 4c99430cf..e92ff1726 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs @@ -130,6 +130,8 @@ fn multi_core_broadcast_inner_join() { rows: response_codec::flatten_to_relational_rows(&phase1_payload), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -277,6 +279,8 @@ fn multi_core_broadcast_left_join() { rows: response_codec::flatten_to_relational_rows(&phase1_payload), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -464,6 +468,8 @@ fn multi_core_broadcast_merge_simulation() { rows: response_codec::flatten_to_relational_rows(&data), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/tests/wire/cases/derived_expression_errors.rs b/nodedb/tests/wire/cases/derived_expression_errors.rs index a90f912fe..371a86490 100644 --- a/nodedb/tests/wire/cases/derived_expression_errors.rs +++ b/nodedb/tests/wire/cases/derived_expression_errors.rs @@ -51,3 +51,14 @@ async fn accessor_over_derived_is_loud() { .expect_error("SELECT nextval('der_seq') FROM (SELECT 1 AS x) s", "0A000") .await; } + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn window_partition_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error( + "SELECT sum(x) OVER (PARTITION BY x/0) FROM (SELECT 1 AS x) s", + "22012", + ) + .await; +} diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 28c626551..36a68d389 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -47,6 +47,7 @@ mod ddl_float_width_aliases_strict_kv; mod ddl_int_width_aliases_strict_kv; mod ddl_numeric_width_schemaless; mod define_field_type_update; +mod derived_expression_errors; mod dml_returning_columnar; mod dml_returning_columnar_policies; mod dml_returning_insert; @@ -141,7 +142,6 @@ mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; mod sequence_const_select; -mod derived_expression_errors; mod sequence_default_all_engines; mod sequence_default_typed; mod sequence_expression_contexts; From d578ca598c4c258556113daad160e9a074f50a70 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:52:56 +0800 Subject: [PATCH 7/9] Copy KV sources in INSERT ... SELECT via the KV materialize scan The copy orchestrators scanned every source with the document materialize scan, which reads the sparse store. A KV source has no rows there, so a non-empty kv source silently copied zero rows and reported INSERT 0 0. resolve_copy_spec now resolves the source collection's engine, the orchestrators route each page through the engine-appropriate scan, and the KV materialize scan folds the transaction overlay the same way the document scan does, so in-transaction copies read their own writes. Engines with no copy scan yet (columnar) refuse loudly. --- nodedb/src/control/insert_select/copy_rows.rs | 95 +++++++++++- .../control/insert_select/expand_staged.rs | 10 +- .../src/control/insert_select/orchestrator.rs | 14 +- .../maintenance/clone_materializer/kv.rs | 55 ++++++- .../maintenance/clone_materializer/mod.rs | 1 + .../executor/handlers/kv/materialize_scan.rs | 27 +++- .../wire/cases/insert_select_cross_engine.rs | 66 +++++++++ .../sql_transactions_insert_select_overlay.rs | 137 ++++++++++++++++++ 8 files changed, 385 insertions(+), 20 deletions(-) diff --git a/nodedb/src/control/insert_select/copy_rows.rs b/nodedb/src/control/insert_select/copy_rows.rs index 79d9540e1..407d94c7e 100644 --- a/nodedb/src/control/insert_select/copy_rows.rs +++ b/nodedb/src/control/insert_select/copy_rows.rs @@ -20,6 +20,18 @@ use crate::control::target_identity::{ TargetPk, assign_target_surrogate, bare_collection_name, resolve_target_pk, }; use crate::engine::document::store::surrogate_to_doc_id; +use crate::types::TxnId; + +/// Which source scan a copy request must use, derived from the source +/// collection's engine at [`resolve_copy_spec`] time. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum SourceScanKind { + /// Sparse-store document read (`DocumentOp::MaterializeScan`). + Document, + /// KV engine cursor read (`KvOp::MaterializeScan`) shaped per row + /// with the shared KV → doc rule. + KeyValue, +} /// Resolved, per-statement copy context shared across every scanned page. pub(crate) struct CopySpec { @@ -30,18 +42,21 @@ pub(crate) struct CopySpec { /// One entry per target column, its `alias` the target column name and its /// `expr` the source-row expression. Empty copies each row unchanged. pub column_map: Vec, + /// The engine-appropriate source scan for this statement's source. + pub source_kind: SourceScanKind, } -/// Resolve the target PK and the residual source `WHERE` filter for one -/// `INSERT ... SELECT` statement. +/// Resolve the target PK, the residual source `WHERE` filter, and the source +/// scan kind for one `INSERT ... SELECT` statement. /// -/// `target_collection` is the db-qualified name as it appears in the -/// `DocumentOp::InsertSelect` plan. +/// `target_collection` and `source_collection` are the db-qualified names as +/// they appear in the `DocumentOp::InsertSelect` plan. pub(crate) fn resolve_copy_spec( state: &SharedState, tenant_id: TenantId, database_id: DatabaseId, target_collection: &str, + source_collection: &str, source_filters: &[u8], column_map: &[u8], ) -> crate::Result { @@ -59,6 +74,34 @@ pub(crate) fn resolve_copy_spec( })?; let target_pk = resolve_target_pk(&target, "INSERT ... SELECT")?; + // Engine-blind source scans silently copy ZERO rows when the source is a + // KV collection (DocumentOp::MaterializeScan reads the sparse store, which + // holds no KV rows). Resolve the source engine so the caller picks the + // matching scan, and refuse engines no scan path supports yet instead of + // answering `INSERT 0 0` on a non-empty source. + let source = catalog + .get_collection( + database_id, + tenant_id.as_u64(), + &bare_collection_name(database_id, source_collection), + )? + .ok_or_else(|| crate::Error::CollectionNotFound { + tenant_id, + collection: source_collection.to_string(), + })?; + let source_kind = match source.collection_type { + nodedb_types::CollectionType::KeyValue(_) => SourceScanKind::KeyValue, + nodedb_types::CollectionType::Document(_) => SourceScanKind::Document, + nodedb_types::CollectionType::Columnar(_) => { + return Err(crate::Error::PlanError { + detail: format!( + "INSERT ... SELECT from a columnar source is not supported yet \ + (source '{source_collection}')" + ), + }); + } + }; + let filters: Vec = if source_filters.is_empty() { Vec::new() } else { @@ -81,9 +124,53 @@ pub(crate) fn resolve_copy_spec( target_pk, filters, column_map, + source_kind, }) } +/// Scan one source page with the engine-appropriate `MaterializeScan`. +/// +/// Both copy orchestrators (autocommit and staged) route through here so the +/// source engine is resolved exactly once, in [`resolve_copy_spec`]. Each +/// entry is a document-shaped `(doc_id, surrogate, msgpack)` triple: the +/// source surrogate is discarded by [`assign_page_rows`], which assigns a +/// fresh target-keyed surrogate per copied row. +pub(crate) async fn scan_copy_source_page( + state: &SharedState, + tenant_id: TenantId, + database_id: DatabaseId, + source_collection: &str, + source_kind: SourceScanKind, + cursor: &[u8], + txn_id: Option, +) -> crate::Result<(Vec<(String, u32, Vec)>, Vec)> { + match source_kind { + SourceScanKind::KeyValue => { + crate::control::maintenance::clone_materializer::scan_kv_source_page( + state, + tenant_id, + database_id, + source_collection, + cursor, + txn_id, + ) + .await + } + SourceScanKind::Document => { + crate::control::maintenance::clone_materializer::scan_source_page( + state, + tenant_id, + database_id, + source_collection, + cursor, + None, + txn_id, + ) + .await + } + } +} + /// Filter and assign fresh surrogates for one scanned source page. /// /// `entries` are the `(doc_id, surrogate, value)` triples from diff --git a/nodedb/src/control/insert_select/expand_staged.rs b/nodedb/src/control/insert_select/expand_staged.rs index bd2e1f1c7..325f58c52 100644 --- a/nodedb/src/control/insert_select/expand_staged.rs +++ b/nodedb/src/control/insert_select/expand_staged.rs @@ -18,8 +18,9 @@ use nodedb_types::{DatabaseId, Surrogate, TenantId}; use crate::bridge::envelope::PhysicalPlan; -use crate::control::insert_select::copy_rows::{assign_page_rows, resolve_copy_spec}; -use crate::control::maintenance::clone_materializer::scan_source_page; +use crate::control::insert_select::copy_rows::{ + assign_page_rows, resolve_copy_spec, scan_copy_source_page, +}; use crate::control::state::SharedState; use crate::types::{TxnId, VShardId}; use nodedb_physical::physical_plan::DocumentOp; @@ -151,6 +152,7 @@ async fn materialize_copy( tenant_id, database_id, target_collection, + source_collection, source_filters, column_map, )?; @@ -160,13 +162,13 @@ async fn materialize_copy( let mut rows: Vec<(String, Vec, Surrogate)> = Vec::new(); while remaining > 0 { - let (entries, next_cursor) = scan_source_page( + let (entries, next_cursor) = scan_copy_source_page( state, tenant_id, database_id, source_collection, + spec.source_kind, &cursor, - None, txn_id, ) .await?; diff --git a/nodedb/src/control/insert_select/orchestrator.rs b/nodedb/src/control/insert_select/orchestrator.rs index 2377e1ce6..fc7ae88cf 100644 --- a/nodedb/src/control/insert_select/orchestrator.rs +++ b/nodedb/src/control/insert_select/orchestrator.rs @@ -18,8 +18,10 @@ use nodedb_types::{DatabaseId, Lsn, Surrogate, TenantId}; use crate::bridge::envelope::{Payload, PhysicalPlan, Response, Status}; -use crate::control::insert_select::copy_rows::{assign_page_rows, resolve_copy_spec}; -use crate::control::maintenance::clone_materializer::{dispatch_local, scan_source_page}; +use crate::control::insert_select::copy_rows::{ + assign_page_rows, resolve_copy_spec, scan_copy_source_page, +}; +use crate::control::maintenance::clone_materializer::dispatch_local; use crate::control::state::SharedState; use nodedb_physical::physical_plan::DocumentOp; @@ -88,6 +90,7 @@ pub(crate) async fn run_insert_select( tenant_id, database_id, req.target_collection, + req.source_collection, req.source_filters, req.column_map, )?; @@ -98,15 +101,16 @@ pub(crate) async fn run_insert_select( let mut max_lsn = Lsn::ZERO; while remaining > 0 { - // Phase 1: scan one source page (point-in-time snapshot). - let (entries, next_cursor) = scan_source_page( + // Phase 1: scan one source page (point-in-time snapshot) with the + // engine-appropriate materialize scan. + let (entries, next_cursor) = scan_copy_source_page( state, tenant_id, database_id, req.source_collection, + spec.source_kind, &cursor, None, - None, ) .await?; diff --git a/nodedb/src/control/maintenance/clone_materializer/kv.rs b/nodedb/src/control/maintenance/clone_materializer/kv.rs index abe438050..7fd206469 100644 --- a/nodedb/src/control/maintenance/clone_materializer/kv.rs +++ b/nodedb/src/control/maintenance/clone_materializer/kv.rs @@ -23,6 +23,7 @@ use crate::control::metadata_proposer::propose_catalog_entry; use crate::control::planner::sql_plan_convert::convert::db_qualified; use crate::control::security::catalog::{StoredCollection, SystemCatalog}; use crate::control::state::SharedState; +use crate::types::TxnId; use nodedb_physical::physical_plan::{KvOp, PhysicalPlan}; use super::dispatch::dispatch_local; @@ -79,6 +80,7 @@ pub(super) async fn materialize_kv_collection( origin.source_database, &source_qualified, &cursor, + None, ) .await?; @@ -188,20 +190,30 @@ fn checkpoint_progress( /// Run one source-side `MaterializeScan` round-trip. Returns the entries in /// this page (raw `(key, value)` byte pairs) plus the next-cursor; the -/// cursor is empty when the scan is complete. -async fn scan_source_page( +/// cursor is empty when the scan is complete. `txn_id`, when set, makes the +/// handler fold the transaction's staging overlay into the page. +pub(crate) async fn scan_source_page( state: &SharedState, tenant_id: TenantId, source_db_id: DatabaseId, source_qualified: &str, cursor: &[u8], + txn_id: Option, ) -> crate::Result { let plan = PhysicalPlan::Kv(KvOp::MaterializeScan { collection: nodedb_types::QualifiedCollection::from_stored(source_qualified.to_string()), cursor: cursor.to_vec(), count: SCAN_PAGE, }); - let resp = dispatch_local(state, tenant_id, source_db_id, source_qualified, plan, None).await?; + let resp = dispatch_local( + state, + tenant_id, + source_db_id, + source_qualified, + plan, + txn_id, + ) + .await?; if resp.status != Status::Ok { return Err(crate::Error::Storage { engine: "clone_materializer".into(), @@ -214,6 +226,43 @@ async fn scan_source_page( parse_materialize_scan_payload(resp.payload.as_ref()) } +/// One KV source page shaped as document rows for the `INSERT ... SELECT` +/// copy pipeline: each `(key, value)` pair becomes `(key, 0, {key, value…} +/// msgpack)` via the shared KV → doc rule ([`msgpack_scan::kv_row_msgpack`]). +/// +/// The surrogate slot is a placeholder — the copy pipeline discards the +/// source surrogate and assigns a fresh target-keyed one. +pub(crate) async fn scan_kv_source_page( + state: &SharedState, + tenant_id: TenantId, + source_db_id: DatabaseId, + source_qualified: &str, + cursor: &[u8], + txn_id: Option, +) -> crate::Result<(Vec<(String, u32, Vec)>, Vec)> { + let (entries, next_cursor) = scan_source_page( + state, + tenant_id, + source_db_id, + source_qualified, + cursor, + txn_id, + ) + .await?; + let rows = entries + .into_iter() + .map(|(key, value)| { + let key_str = String::from_utf8_lossy(&key).into_owned(); + ( + key_str.clone(), + 0u32, + nodedb_query::msgpack_scan::kv_row_msgpack(&key_str, &value), + ) + }) + .collect(); + Ok((rows, next_cursor)) +} + /// `(key, value)` pairs returned by one materialize-scan page. type ScanPage = (Vec<(Vec, Vec)>, Vec); diff --git a/nodedb/src/control/maintenance/clone_materializer/mod.rs b/nodedb/src/control/maintenance/clone_materializer/mod.rs index 23f0ef7ca..8618b5f49 100644 --- a/nodedb/src/control/maintenance/clone_materializer/mod.rs +++ b/nodedb/src/control/maintenance/clone_materializer/mod.rs @@ -19,3 +19,4 @@ pub use walker::{ // local-dispatch primitive and source-scan cursor decode. pub(crate) use dispatch::{dispatch_local, dispatch_local_on_vshard}; pub(crate) use document::{read_all_source_rows, scan_source_page}; +pub(crate) use kv::scan_kv_source_page; diff --git a/nodedb/src/data/executor/handlers/kv/materialize_scan.rs b/nodedb/src/data/executor/handlers/kv/materialize_scan.rs index 7d3313eee..43cc65968 100644 --- a/nodedb/src/data/executor/handlers/kv/materialize_scan.rs +++ b/nodedb/src/data/executor/handlers/kv/materialize_scan.rs @@ -1,10 +1,12 @@ // SPDX-License-Identifier: BUSL-1.1 -//! Cursor-paginated raw KV scan used by the clone materializer. +//! Cursor-paginated raw KV scan used by the clone materializer and the +//! `INSERT ... SELECT` copy pipeline. //! //! Returns the engine's `(key, value)` pairs verbatim (no map wrapping or //! key-injection — the materializer needs the raw stored value bytes to -//! re-`Put` them on target) plus the next-cursor in a single response so the +//! re-`Put` them on target; the copy pipeline shapes them itself) plus the +//! next-cursor in a single response so the //! caller can drive the scan to completion in O(N / count) round-trips. use crate::bridge::envelope::Response; @@ -31,12 +33,18 @@ impl CoreLoop { }; let now_ms = current_ms(); - let (entries, next_cursor) = self.kv_engine.scan(KvScanParams { + let txn_id = task.request.txn_id; + let (mut entries, mut next_cursor) = self.kv_engine.scan(KvScanParams { database_id: did, tenant_id: tid, collection, cursor, - count, + // In-transaction callers need base ∪ overlay. The overlay can + // tombstone/supersede rows spanning pages, so collect the whole + // base set (ignoring the page cap, like the document scan) and + // return it un-paginated; the capacity hint stays bounded by the + // engine's own slot count. + count: if txn_id.is_some() { usize::MAX } else { count }, now_ms, match_pattern: None, filter_field: None, @@ -44,6 +52,17 @@ impl CoreLoop { surrogate_ceiling: None, }); + if let Some(txn_id) = txn_id { + let coll_key = ( + crate::types::DatabaseId::new(did), + crate::types::TenantId::new(tid), + collection.to_string(), + ); + self.merge_kv_overlay_into_scan(txn_id, &coll_key, &mut entries, &|_| true); + // Single un-paginated response: the scan is complete in one round-trip. + next_cursor = Vec::new(); + } + // Encode response payload as msgpack: // [next_cursor: bytes, entries: [[key, value], ...]] let mut payload = Vec::with_capacity( diff --git a/nodedb/tests/wire/cases/insert_select_cross_engine.rs b/nodedb/tests/wire/cases/insert_select_cross_engine.rs index fb75a1378..0beb64506 100644 --- a/nodedb/tests/wire/cases/insert_select_cross_engine.rs +++ b/nodedb/tests/wire/cases/insert_select_cross_engine.rs @@ -307,3 +307,69 @@ async fn insert_select_from_strict_source_normalizes_and_resolves() { "vector search must resolve the copied strict-source 'alpha'; got {near_e1:?}" ); } + +/// An autocommit `INSERT ... SELECT` whose SOURCE is a KV collection must copy +/// rows instead of silently answering `INSERT 0 0`. The pre-fix orchestrator +/// scanned every source with the DOCUMENT materialize scan, which reads the +/// sparse store — a KV collection has no rows there, so a non-empty source +/// copied zero rows. Expression projections from the source row (e.g. +/// `upper(v)`) must evaluate against the KV row's fields. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_select_from_kv_source_copies_rows() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION isk_src \ + (id BIGINT PRIMARY KEY, v TEXT) WITH (engine='kv')", + ) + .await + .unwrap(); + server + .exec("INSERT INTO isk_src (id, v) VALUES (1, 'hello')") + .await + .unwrap(); + server + .exec("INSERT INTO isk_src (id, v) VALUES (2, 'world')") + .await + .unwrap(); + server + .exec("CREATE COLLECTION isk_tgt (id BIGINT PRIMARY KEY, v TEXT)") + .await + .unwrap(); + + server + .exec("INSERT INTO isk_tgt (id, v) SELECT id, upper(v) FROM isk_src") + .await + .unwrap(); + + // Both rows landed with the expression evaluated per row. + let scan = server + .query_text("SELECT v FROM isk_tgt ORDER BY id") + .await + .unwrap(); + assert_eq!( + scan, + vec!["HELLO".to_string(), "WORLD".to_string()], + "kv-source copy must persist both rows with per-row expressions; got {scan:?}" + ); + + // The plain-passthrough copy (no explicit column list) works too. + server + .exec("CREATE COLLECTION isk_tgt2 (id BIGINT PRIMARY KEY, v TEXT)") + .await + .unwrap(); + server + .exec("INSERT INTO isk_tgt2 SELECT * FROM isk_src") + .await + .unwrap(); + let copied = server + .query_text("SELECT count(*) FROM isk_tgt2") + .await + .unwrap(); + assert_eq!( + copied, + vec!["2".to_string()], + "kv-source SELECT * copy must copy both rows; got {copied:?}" + ); +} diff --git a/nodedb/tests/wire/cases/sql_transactions_insert_select_overlay.rs b/nodedb/tests/wire/cases/sql_transactions_insert_select_overlay.rs index fef06e230..59a671d31 100644 --- a/nodedb/tests/wire/cases/sql_transactions_insert_select_overlay.rs +++ b/nodedb/tests/wire/cases/sql_transactions_insert_select_overlay.rs @@ -242,3 +242,140 @@ async fn strict_insert_select_sees_source_rows_staged_earlier_in_txn() { ) .await; } + +/// KV-source variants: source is a KV collection (scanned via +/// `KvOp::MaterializeScan`), target a document collection written through the +/// plain overlay path. Pre-fix the copy scored ZERO source rows — the +/// document materialize scan reads the sparse store, which holds no KV rows — +/// so these would report `INSERT 0 0` and stage nothing. +async fn kv_setup(server: &TestServer, src: &str, tgt: &str) { + server + .exec(&format!( + "CREATE COLLECTION {src} \ + (id STRING NOT NULL PRIMARY KEY, n INT) WITH (engine='kv')" + )) + .await + .unwrap(); + server + .exec(&format!( + "CREATE COLLECTION {tgt} (id STRING NOT NULL PRIMARY KEY, n INT)" + )) + .await + .unwrap(); + for (id, n) in [("a", 1), ("b", 1), ("c", 2), ("unrelated", 100)] { + server + .exec(&format!("INSERT INTO {src} (id, n) VALUES ('{id}', {n})")) + .await + .unwrap(); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_insert_select_stages_and_is_visible() { + let server = TestServer::start().await; + kv_setup(&server, "is_kv_src", "is_kv_tgt").await; + + server.exec("BEGIN").await.unwrap(); + let msgs = server + .client + .simple_query("INSERT INTO is_kv_tgt SELECT * FROM is_kv_src WHERE n = 1") + .await + .expect("in-tx kv insert-select should succeed at the statement"); + assert_eq!( + command_count(&msgs), + Some(2), + "kv: INSERT ... SELECT must report the real copied-row count, not 0" + ); + let seen = scan_ints(&server, "SELECT n FROM is_kv_tgt").await; + assert_eq!( + seen, + vec![1, 1], + "kv: in-tx scan of target must observe the staged copy" + ); + + server.client.simple_query("COMMIT").await.unwrap(); + let after = scan_ints(&server, "SELECT n FROM is_kv_tgt").await; + assert_eq!( + after, + vec![1, 1], + "kv: committed insert-select must persist" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_insert_select_rollback_discards_staged_copy() { + let server = TestServer::start().await; + kv_setup(&server, "is_kv_rb_src", "is_kv_rb_tgt").await; + + server.exec("BEGIN").await.unwrap(); + server + .exec("INSERT INTO is_kv_rb_tgt SELECT * FROM is_kv_rb_src WHERE n = 1") + .await + .unwrap(); + let staged = scan_ints(&server, "SELECT n FROM is_kv_rb_tgt").await; + assert_eq!( + staged, + vec![1, 1], + "kv: staged copy must be visible pre-rollback" + ); + + server.client.simple_query("ROLLBACK").await.unwrap(); + let after = scan_ints(&server, "SELECT n FROM is_kv_rb_tgt").await; + assert!( + after.is_empty(), + "kv: ROLLBACK must discard the staged copy, got {after:?}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_insert_select_respects_limit() { + let server = TestServer::start().await; + kv_setup(&server, "is_kv_lim_src", "is_kv_lim_tgt").await; + + server.exec("BEGIN").await.unwrap(); + let msgs = server + .client + .simple_query("INSERT INTO is_kv_lim_tgt SELECT * FROM is_kv_lim_src WHERE n = 1 LIMIT 1") + .await + .expect("in-tx kv insert-select with LIMIT should succeed"); + assert_eq!( + command_count(&msgs), + Some(1), + "kv: LIMIT must cap the staged count" + ); + let seen = scan_ints(&server, "SELECT n FROM is_kv_lim_tgt").await; + assert_eq!(seen, vec![1], "kv: target must show only the LIMIT-ed copy"); + + server.client.simple_query("ROLLBACK").await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_insert_select_sees_source_rows_staged_earlier_in_txn() { + let server = TestServer::start().await; + kv_setup(&server, "is_kv_ov_src", "is_kv_ov_tgt").await; + + server.exec("BEGIN").await.unwrap(); + server + .exec("INSERT INTO is_kv_ov_src (id, n) VALUES ('fresh', 1)") + .await + .unwrap(); + + let msgs = server + .client + .simple_query("INSERT INTO is_kv_ov_tgt SELECT * FROM is_kv_ov_src WHERE n = 1") + .await + .expect("in-tx kv insert-select should succeed at the statement"); + assert_eq!( + command_count(&msgs), + Some(3), + "kv: copy must include the source row staged earlier in this txn" + ); + let seen = scan_ints(&server, "SELECT n FROM is_kv_ov_tgt").await; + assert_eq!( + seen, + vec![1, 1, 1], + "kv: target must contain the copy of the staged source row" + ); + + server.client.simple_query("ROLLBACK").await.unwrap(); +} From ae25e6077a9bece4b54044f49555916442386473 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:47:16 +0800 Subject: [PATCH 8/9] Fix window functions over scan-bodied derived tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit inline_cte merges an outer SELECT's constraints onto a derived body when that body is itself a Scan, and it copied the BODY's (empty) window list in place of the OUTER's — so SUM/row_number/rank over FROM (SELECT * FROM c) silently answered NULL on every row. The outer window specs now run after the body's own, matching the post-processor branch, and two positive-value tests lock in the carriage and the results. --- .../control/planner/sql_plan_convert/expr.rs | 13 +++- .../tests/wire/cases/sql_window_functions.rs | 71 +++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index f41aac633..9190c2cea 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -364,7 +364,18 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> // offset 0 = unspecified → inherit CTE's offset. offset: if *offset > 0 { *offset } else { *inner_o }, distinct: *distinct || *inner_d, - window_functions: inner_w.clone(), + // Window functions: the derived body's own specs run + // first (they produce columns the outer may reference), + // then the outer's. Dropping the outer's here left + // `SUM(n) OVER ...` over a derived table with a Scan + // body silently NULL (issue #295 Gap 3). + window_functions: { + let mut merged = inner_w.clone(); + if !window_functions.is_empty() { + merged.extend(window_functions.iter().cloned()); + } + merged + }, temporal: *inner_t, } } else if let SqlPlan::VectorSearch { .. } = cte_plan { diff --git a/nodedb/tests/wire/cases/sql_window_functions.rs b/nodedb/tests/wire/cases/sql_window_functions.rs index 31f08bfc8..878128ec3 100644 --- a/nodedb/tests/wire/cases/sql_window_functions.rs +++ b/nodedb/tests/wire/cases/sql_window_functions.rs @@ -416,3 +416,74 @@ async fn window_offset_over_expression_argument_returns_previous_evaluated_value ); } } + +// ── window functions over a DERIVED table (issue #295 Gap 3) ── +// +// A derived-table body that is itself a plain Scan (e.g. `SELECT * FROM s`) +// inlines through the CTE path, where the outer window spec was previously +// dropped in favour of the inner scan's (empty) window list — every window +// column answered NULL. These lock in the carriage AND the values. + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn derived_table_window_sum_returns_values_not_null() { + let server = TestServer::start().await; + setup_numbered_rows(&server).await; + + let rows = server + .query_rows( + "SELECT id, SUM(n) OVER (ORDER BY n) AS s \ + FROM (SELECT * FROM s) d ORDER BY n", + ) + .await + .unwrap(); + + assert_eq!(rows.len(), 5, "expected 5 rows: {rows:?}"); + for (i, row) in rows.iter().enumerate() { + let s = row.get(1).cloned().unwrap_or_default(); + assert!( + !s.is_empty() && s.to_lowercase() != "null", + "SUM window dropped at row {i}: {row:?}" + ); + } + + let got = parse_f64s(&rows, 1); + let want = [1.0, 3.0, 6.0, 10.0, 15.0]; + for (i, (g, w)) in got.iter().zip(want.iter()).enumerate() { + assert!( + (g - w).abs() < 1e-9, + "derived SUM[{i}] = {g}, want {w}; rows = {rows:?}" + ); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn derived_table_window_row_number_orders_correctly() { + let server = TestServer::start().await; + setup_numbered_rows(&server).await; + + let rows = server + .query_rows( + "SELECT id, row_number() OVER (ORDER BY n) AS rn \ + FROM (SELECT * FROM s) d ORDER BY n", + ) + .await + .unwrap(); + + assert_eq!(rows.len(), 5, "expected 5 rows: {rows:?}"); + for (i, row) in rows.iter().enumerate() { + let rn = row.get(1).cloned().unwrap_or_default(); + assert!( + !rn.is_empty() && rn.to_lowercase() != "null", + "row_number dropped at row {i}: {row:?}" + ); + } + + let got = parse_f64s(&rows, 1); + let want = [1.0, 2.0, 3.0, 4.0, 5.0]; + for (i, (g, w)) in got.iter().zip(want.iter()).enumerate() { + assert!( + (g - w).abs() < 1e-9, + "derived row_number[{i}] = {g}, want {w}; rows = {rows:?}" + ); + } +} From 653f106c2c5f80c663cc8d36c900cab11e7c594b Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:04:32 +0800 Subject: [PATCH 9/9] Fix cluster-tests fixtures for new QueryOp fields Add missing and fields to three ProviderScan constructors in nodedb-cluster-tests: - shuffle_aggregate_cross_node.rs - shuffle_consume_cross_node.rs - shuffle_produce_cross_node.rs These fixtures were missing the fields added for window-over-derived support in #303. --- .../tests/common_suite/cases/shuffle_aggregate_cross_node.rs | 2 ++ .../tests/common_suite/cases/shuffle_consume_cross_node.rs | 2 ++ .../tests/common_suite/cases/shuffle_produce_cross_node.rs | 2 ++ 3 files changed, 6 insertions(+) diff --git a/nodedb-cluster-tests/tests/common_suite/cases/shuffle_aggregate_cross_node.rs b/nodedb-cluster-tests/tests/common_suite/cases/shuffle_aggregate_cross_node.rs index d534d18ba..08beb3d1a 100644 --- a/nodedb-cluster-tests/tests/common_suite/cases/shuffle_aggregate_cross_node.rs +++ b/nodedb-cluster-tests/tests/common_suite/cases/shuffle_aggregate_cross_node.rs @@ -107,6 +107,8 @@ fn producer_plan(rows: &[&Row]) -> Vec { rows: msgpack_array(rows), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb-cluster-tests/tests/common_suite/cases/shuffle_consume_cross_node.rs b/nodedb-cluster-tests/tests/common_suite/cases/shuffle_consume_cross_node.rs index 205772766..f343e19fe 100644 --- a/nodedb-cluster-tests/tests/common_suite/cases/shuffle_consume_cross_node.rs +++ b/nodedb-cluster-tests/tests/common_suite/cases/shuffle_consume_cross_node.rs @@ -104,6 +104,8 @@ fn provider_scan_plan(rows: &[&Row]) -> Vec { rows: msgpack_array(rows), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb-cluster-tests/tests/common_suite/cases/shuffle_produce_cross_node.rs b/nodedb-cluster-tests/tests/common_suite/cases/shuffle_produce_cross_node.rs index 3f0602c28..49f8bf830 100644 --- a/nodedb-cluster-tests/tests/common_suite/cases/shuffle_produce_cross_node.rs +++ b/nodedb-cluster-tests/tests/common_suite/cases/shuffle_produce_cross_node.rs @@ -108,6 +108,8 @@ fn provider_scan_plan(rows: &[Vec]) -> Vec { rows: msgpack_array(rows), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0,