Skip to content

Commit f184562

Browse files
committed
fix: Implement two-phase deletion to prevent partial deletion risk
- Phase 1: Evaluate all rows and collect matching keys first - Phase 2: Delete all matching rows only if Phase 1 succeeds - If WHERE evaluation fails on any row, no rows are deleted - Improved error message to indicate no rows were deleted on failure - This ensures table consistency even when WHERE clause evaluation fails Addresses CodeRabbit review feedback about partial deletion risk leaving table in inconsistent state.
1 parent ef26ec3 commit f184562

1 file changed

Lines changed: 24 additions & 11 deletions

File tree

nexum_core/src/executor/mod.rs

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -176,47 +176,60 @@ impl Executor {
176176
})?;
177177

178178
let prefix = Self::table_data_prefix(&table);
179-
let mut deleted_count = 0;
180179

181180
if let Some(where_expr) = where_clause {
182181
let column_names: Vec<String> =
183182
schema.columns.iter().map(|c| c.name.clone()).collect();
184183
let evaluator = ExpressionEvaluator::new(column_names);
185184

186-
// Process rows incrementally to avoid loading all into memory
185+
// Two-phase deletion: first collect keys to delete, then delete them
186+
// This prevents partial deletion if WHERE evaluation fails
187187
let all_rows = self.storage.scan_prefix(&prefix)?;
188+
let mut keys_to_delete: Vec<Vec<u8>> = Vec::new();
189+
190+
// Phase 1: Evaluate all rows and collect matching keys
188191
for (key, value) in &all_rows {
189192
if let Ok(row) = serde_json::from_slice::<Row>(value) {
190193
match evaluator.evaluate(&where_expr, &row.values) {
191194
Ok(true) => {
192-
self.storage.delete(key)?;
193-
deleted_count += 1;
195+
keys_to_delete.push(key.clone());
194196
}
195197
Ok(false) => {
196198
// Row doesn't match WHERE condition, skip
197199
}
198200
Err(e) => {
199201
return Err(StorageError::ReadError(format!(
200-
"WHERE clause evaluation failed: {}", e
202+
"WHERE clause evaluation failed on row: {}. No rows were deleted.", e
201203
)));
202204
}
203205
}
204206
}
205207
}
208+
209+
// Phase 2: Delete all matching rows (only if Phase 1 succeeded)
210+
let deleted_count = keys_to_delete.len();
211+
for key in keys_to_delete {
212+
self.storage.delete(&key)?;
213+
}
214+
215+
Ok(ExecutionResult::Deleted {
216+
table,
217+
rows: deleted_count,
218+
})
206219
} else {
207220
// No WHERE clause - delete all rows
208221
log::warn!("DELETE without WHERE clause will remove all rows from table '{}'", table);
209222
let all_rows = self.storage.scan_prefix(&prefix)?;
223+
let deleted_count = all_rows.len();
210224
for (key, _) in &all_rows {
211225
self.storage.delete(key)?;
212-
deleted_count += 1;
213226
}
214-
}
215227

216-
Ok(ExecutionResult::Deleted {
217-
table,
218-
rows: deleted_count,
219-
})
228+
Ok(ExecutionResult::Deleted {
229+
table,
230+
rows: deleted_count,
231+
})
232+
}
220233
}
221234
};
222235

0 commit comments

Comments
 (0)