Skip to content

Commit 6b9b66a

Browse files
committed
fix(sql): evaluate sequence-backed DEFAULTs on every engine
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). This is 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 (parse_column_type_str_full). UUID-class defaults on the document engine now materialize; the sql_suite "declared doc" behavior is unchanged. 2. Evaluation: one shared per-row expander (`expand_row_defaults`) now serves INSERT and UPSERT on the document family, the columnar batch encoder (rows.rs), and the kv converter — replacing three copies of the pure-evaluator swallow. Sequence accessors advance the CP-side registry through the ConvertContext; unknown sequences raise loudly naming them, never NULL. Mechanics: - SequenceRegistry threaded SharedState -> QueryContext -> ConvertContext (both planning construction sites). - SqlPlan::KvInsert carries key_column + sequence_defaults; the kv planner skips sequence defaults (pure evaluator cannot run them) and the converter fills key slot + value map per row (named pk columns are mirrored into the value map so scans read them back). - nodedb_value_to_sql relocated into value/convert for shared use. Verified (wire `sequence_default_all_engines`, 6 tests, plus typed suite): strict 1,2,3; document nextval 1,2; document uuid fills; document UPSERT 1,2; kv key 1,2 (mirrored, readable); kv unknown-sequence raises. Regression sentinels green (kv_column_defaults, dml_returning_kv/insert, not-null gate). fmt clean; clippy -D warnings clean; nodedb-sql 864 + nodedb-types 688. 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.
1 parent bb76352 commit 6b9b66a

25 files changed

Lines changed: 589 additions & 39 deletions

File tree

‎nodedb-sql/src/planner/dml_helpers/kv_insert.rs‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,10 @@ pub(crate) fn build_kv_insert_plan(
108108
// position in the statement's column list.
109109
let key_val = match row.iter().find(|(name, _)| name == key_col_name) {
110110
Some((_, value)) => value.clone(),
111-
None => SqlValue::String(String::new()),
111+
// NULL marks "key column absent from this row" — the converter
112+
// fills a sequence default there, or stores the legacy empty key
113+
// when none is declared.
114+
None => SqlValue::Null,
112115
};
113116
if let Some((_, value)) = row.iter().find(|(name, _)| name == "ttl") {
114117
match value {
@@ -124,12 +127,23 @@ pub(crate) fn build_kv_insert_plan(
124127
.collect();
125128
entries.push((key_val, value_cols));
126129
}
130+
let sequence_defaults: Vec<(String, String)> = declared_columns
131+
.iter()
132+
.filter_map(|c| {
133+
c.default
134+
.as_ref()
135+
.filter(|d| is_sequence_default(d))
136+
.map(|d| (c.name.clone(), d.clone()))
137+
})
138+
.collect();
127139
Ok(vec![SqlPlan::KvInsert {
128140
collection: table_name,
129141
entries,
130142
ttl_secs,
131143
intent,
132144
on_conflict_updates,
145+
key_column: key_col_name.to_string(),
146+
sequence_defaults,
133147
}])
134148
}
135149

@@ -162,6 +176,12 @@ fn materialize_declared_defaults(
162176
if row.iter().any(|(name, _)| name == &column.name) {
163177
continue;
164178
}
179+
// Sequence accessors cannot run in the pure planner evaluator; the
180+
// converter advances the CP-side registry instead (the column is
181+
// carried on the plan as a sequence default and stays absent here).
182+
if is_sequence_default(default_expr) {
183+
continue;
184+
}
165185
let evaluated =
166186
crate::planner::defaults::evaluate_default_expr(default_expr).map_err(|e| {
167187
SqlError::Parse {
@@ -211,6 +231,13 @@ fn nodedb_value_to_sql_value(column: &str, value: nodedb_types::Value) -> Result
211231
})
212232
}
213233

234+
/// Whether a DEFAULT expression is a sequence accessor the pure planner
235+
/// evaluator cannot run. Mirrors the convert-side recognizer.
236+
fn is_sequence_default(expr: &str) -> bool {
237+
let upper = expr.trim().to_uppercase();
238+
upper.starts_with("NEXTVAL(") || upper.starts_with("SETVAL(") || upper.starts_with("CURRVAL(")
239+
}
240+
214241
#[cfg(test)]
215242
mod kv_on_conflict_range_tests {
216243
use sqlparser::ast::{Expr, Value, ValueWithSpan};

‎nodedb-sql/src/types/plan/variants.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,12 @@ pub enum SqlPlan {
138138
/// Empty for plain UPSERT (whole-value overwrite) and for INSERT
139139
/// variants.
140140
on_conflict_updates: Vec<(String, SqlExpr)>,
141+
/// The collection's primary-key column name (the KV key slot).
142+
key_column: String,
143+
/// Column defaults that are sequence accessors (`nextval(...)`) —
144+
/// the pure planner evaluator cannot run them; the converter
145+
/// advances the CP-side registry per row instead.
146+
sequence_defaults: Vec<(String, String)>,
141147
},
142148
/// UPSERT: insert or merge if document exists.
143149
Upsert {

‎nodedb-sql/src/visitor/plan_visitor/dispatch.rs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,17 @@ pub fn dispatch<V: PlanVisitor>(visitor: &mut V, plan: &SqlPlan) -> Result<V::Ou
113113
ttl_secs,
114114
intent,
115115
on_conflict_updates,
116-
} => visitor.kv_insert(collection, entries, *ttl_secs, *intent, on_conflict_updates),
116+
key_column,
117+
sequence_defaults,
118+
} => visitor.kv_insert(
119+
collection,
120+
entries,
121+
*ttl_secs,
122+
*intent,
123+
on_conflict_updates,
124+
key_column,
125+
sequence_defaults,
126+
),
117127
SqlPlan::Upsert {
118128
collection,
119129
engine,

‎nodedb-sql/src/visitor/plan_visitor/trait_def.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,16 @@ pub trait PlanVisitor {
7373
fn insert(&mut self, args: InsertVisitArgs<'_>) -> Result<Self::Output, Self::Error>;
7474

7575
/// Handle [`SqlPlan::KvInsert`].
76+
#[allow(clippy::too_many_arguments)]
7677
fn kv_insert(
7778
&mut self,
7879
collection: &str,
7980
entries: &[(SqlValue, Vec<(String, SqlValue)>)],
8081
ttl_secs: u64,
8182
intent: KvInsertIntent,
8283
on_conflict_updates: &[(String, SqlExpr)],
84+
key_column: &str,
85+
sequence_defaults: &[(String, String)],
8386
) -> Result<Self::Output, Self::Error>;
8487

8588
/// Handle [`SqlPlan::Upsert`].

‎nodedb/src/control/planner/catalog_adapter/type_convert.rs‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,24 @@ pub(super) fn convert_collection_type(
6161
.declared_primary_key
6262
.clone()
6363
.unwrap_or_else(|| "id".to_string());
64+
// The stored field entry keeps the FULL DDL constraint text
65+
// (`"BIGINT DEFAULT nextval('s') PRIMARY KEY"`), so the DEFAULT
66+
// expression is recoverable even though the schemaless type
67+
// system records no per-column default slot of its own. Dropping
68+
// it here is what made `DEFAULT uuid_v7()` / `DEFAULT
69+
// nextval('s')` silently commit NULL on the document engine
70+
// (#294): the DDL accepted the expression, the catalog forgot it.
71+
let pk_default = stored
72+
.fields
73+
.iter()
74+
.find(|(n, _)| n.eq_ignore_ascii_case(&pk_name))
75+
.and_then(|(_, ts)| doc_default_expr(ts));
6476
let mut columns = vec![ColumnInfo {
6577
name: pk_name.clone(),
6678
data_type: SqlDataType::String,
6779
nullable: false,
6880
is_primary_key: true,
69-
default: None,
81+
default: pk_default,
7082
raw_type: None,
7183
int_width: None,
7284
float_width: None,
@@ -81,7 +93,7 @@ pub(super) fn convert_collection_type(
8193
data_type: parse_type_str(type_str),
8294
nullable: true,
8395
is_primary_key: false,
84-
default: None,
96+
default: doc_default_expr(type_str),
8597
raw_type: None,
8698
int_width: IntWidth::from_declared_type(type_str),
8799
float_width: FloatWidth::from_declared_type(type_str),
@@ -289,6 +301,12 @@ fn parse_type_str(s: &str) -> SqlDataType {
289301
}
290302
}
291303

304+
fn doc_default_expr(type_str: &str) -> Option<String> {
305+
let (_, _, _, default_expr) =
306+
nodedb_sql::ddl_ast::collection_type::parse_column_type_str_full(type_str);
307+
default_expr
308+
}
309+
292310
#[cfg(test)]
293311
mod tests {
294312
use nodedb_types::CollectionType;

‎nodedb/src/control/planner/context/query/context.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ pub struct QueryContext {
4040
/// `QueryContext::new()` test fixtures that never lower to
4141
/// surrogate-bearing variants.
4242
pub(super) surrogate_assigner: Option<Arc<crate::control::surrogate::SurrogateAssigner>>,
43+
/// Sequence registry — `Some` when the planner has access to
44+
/// `SharedState` (production path). SQL sequence accessors
45+
/// (`nextval`/`currval`/`setval`) and sequence-backed DEFAULT
46+
/// expressions evaluate through this; `None` for sub-planners.
47+
pub(super) sequence_registry: Option<Arc<crate::control::sequence::SequenceRegistry>>,
4348
/// Cluster mode flag — `true` when the node has a live cluster
4449
/// topology. Passed into `ConvertContext` so array converters can
4550
/// emit `ClusterArray` variants instead of local `Array` variants.
@@ -109,6 +114,7 @@ impl QueryContext {
109114
array_catalog: None,
110115
wal: None,
111116
surrogate_assigner: None,
117+
sequence_registry: None,
112118
cluster_enabled: false,
113119
bitemporal_retention_registry: None,
114120
max_vector_dim: std::sync::atomic::AtomicU32::new(0),
@@ -140,6 +146,7 @@ impl QueryContext {
140146
Some(Arc::clone(&state.retention_policy_registry)),
141147
);
142148
ctx.surrogate_assigner = Some(Arc::clone(&state.surrogate_assigner));
149+
ctx.sequence_registry = Some(Arc::clone(&state.sequence_registry));
143150
ctx.cluster_enabled = state.cluster_topology.is_some();
144151
ctx.bitemporal_retention_registry = Some(Arc::clone(&state.bitemporal_retention_registry));
145152
// max_vector_dim starts at 0 (unlimited); connection handlers call
@@ -174,6 +181,7 @@ impl QueryContext {
174181
array_catalog: Some(state.array_catalog.clone()),
175182
wal: Some(Arc::clone(&state.wal)),
176183
surrogate_assigner: Some(Arc::clone(&state.surrogate_assigner)),
184+
sequence_registry: Some(Arc::clone(&state.sequence_registry)),
177185
cluster_enabled: state.cluster_topology.is_some(),
178186
bitemporal_retention_registry: Some(Arc::clone(&state.bitemporal_retention_registry)),
179187
// max_vector_dim is tenant-specific; callers supply it via
@@ -215,6 +223,7 @@ impl QueryContext {
215223
array_catalog: None,
216224
wal: None,
217225
surrogate_assigner: None,
226+
sequence_registry: None,
218227
cluster_enabled: false,
219228
bitemporal_retention_registry: None,
220229
max_vector_dim: std::sync::atomic::AtomicU32::new(0),

‎nodedb/src/control/planner/context/query/planning.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ impl QueryContext {
147147
.map(|i| Arc::clone(&i.credentials)),
148148
wal: self.wal.clone(),
149149
surrogate_assigner: self.surrogate_assigner.clone(),
150+
sequence_registry: self.sequence_registry.clone(),
150151
cluster_enabled: self.cluster_enabled,
151152
bitemporal_retention_registry: self.bitemporal_retention_registry.clone(),
152153
max_vector_dim: self
@@ -412,6 +413,7 @@ impl QueryContext {
412413
.map(|i| Arc::clone(&i.credentials)),
413414
wal: self.wal.clone(),
414415
surrogate_assigner: self.surrogate_assigner.clone(),
416+
sequence_registry: self.sequence_registry.clone(),
415417
cluster_enabled: self.cluster_enabled,
416418
bitemporal_retention_registry: self.bitemporal_retention_registry.clone(),
417419
max_vector_dim: self

‎nodedb/src/control/planner/sql_plan_convert/array_fn_convert/aggregate.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ mod tests {
144144
credentials: None,
145145
wal: None,
146146
surrogate_assigner: None,
147+
sequence_registry: None,
147148
cluster_enabled,
148149
bitemporal_retention_registry: None,
149150
max_vector_dim: 0,

‎nodedb/src/control/planner/sql_plan_convert/array_fn_convert/slice.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ mod tests {
246246
credentials: None,
247247
wal: None,
248248
surrogate_assigner: None,
249+
sequence_registry: None,
249250
cluster_enabled,
250251
bitemporal_retention_registry: None,
251252
max_vector_dim: 0,

‎nodedb/src/control/planner/sql_plan_convert/convert.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ pub struct ConvertContext {
6262
pub credentials: Option<Arc<CredentialStore>>,
6363
/// LSN allocator for array Put/Delete dispatches.
6464
pub wal: Option<Arc<WalManager>>,
65+
/// Sequence registry for SQL sequence accessors and sequence-backed
66+
/// DEFAULT expressions. `None` for sub-planner contexts.
67+
pub sequence_registry: Option<Arc<crate::control::sequence::SequenceRegistry>>,
6568
/// CP-side surrogate assigner — bound to the same `Arc` held on
6669
/// `SharedState`. Threaded into INSERT/UPSERT/KV-INSERT converters
6770
/// to bind `(collection, pk_bytes)` → `Surrogate` before the op
@@ -281,6 +284,7 @@ mod tests {
281284
credentials: None,
282285
wal: None,
283286
surrogate_assigner: Some(assigner),
287+
sequence_registry: None,
284288
cluster_enabled: false,
285289
bitemporal_retention_registry: None,
286290
max_vector_dim: 0,

0 commit comments

Comments
 (0)