feat: Add DELETE statement support with WHERE clause filtering #57
Conversation
- Add Delete variant to Statement enum with table and optional WHERE clause - Implement DELETE FROM table [WHERE condition] parsing in SQL parser - Add DELETE execution logic in executor with row filtering - Support both targeted deletes (with WHERE) and full table deletes - Add warning for DELETE without WHERE clause to prevent accidental data loss - Return count of deleted rows in ExecutionResult::Deleted - Add comprehensive unit tests for DELETE functionality - Update planner to handle DELETE statements Resolves aviralgarg05#45
Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
Warning Rate limit exceeded@sshekhar563 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds end-to-end DELETE support: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Parser
participant Planner
participant Executor
participant Storage
Client->>Parser: "DELETE FROM table [WHERE ...]"
Parser-->>Client: Statement::Delete{table, where_clause}
Client->>Planner: plan(Statement::Delete)
Planner-->>Client: Plan::Delete{table, has_where}
Client->>Executor: execute(Plan::Delete)
Executor->>Storage: scan rows (table prefix)
alt WHERE present
Storage-->>Executor: stream rows
Executor->>Executor: evaluate WHERE per row
alt match
Executor->>Storage: delete row
Executor-->>Executor: increment counter
else no match
Executor-->>Executor: continue
end
else no WHERE
loop for each row
Executor->>Storage: delete row
Executor-->>Executor: increment counter
end
end
Executor-->>Client: ExecutionResult::Deleted{table, rows}
Note right of Executor: logs via log::debug / log::info / log::warn
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
nexum_core/src/executor/mod.rsnexum_core/src/sql/parser.rsnexum_core/src/sql/planner.rsnexum_core/src/sql/types.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
Focus on Rust best practices, memory safety, error handling, and idiomatic code patterns.
Files:
nexum_core/src/sql/planner.rsnexum_core/src/sql/types.rsnexum_core/src/sql/parser.rsnexum_core/src/executor/mod.rs
🧬 Code graph analysis (1)
nexum_core/src/executor/mod.rs (6)
nexum_core/src/executor/filter.rs (1)
new(53-55)nexum_core/src/bridge/mod.rs (3)
new(10-12)new(70-82)new(119-134)nexum_core/src/storage/engine.rs (3)
new(10-13)memory(15-21)delete(36-39)nexum_core/src/catalog/mod.rs (1)
new(18-20)nexum_core/src/sql/parser.rs (1)
row(49-51)nexum_core/src/sql/types.rs (1)
data_type(23-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Rust benchmarks
- GitHub Check: Python tests
🔇 Additional comments (5)
nexum_core/src/sql/planner.rs (1)
43-46: LGTM!The DELETE statement mapping correctly extracts the table name and derives
has_wherefrom the presence of the WHERE clause. Implementation follows existing patterns.nexum_core/src/sql/types.rs (1)
70-73: LGTM!The
Statement::Deletevariant is well-designed and consistent with the existingSelectstatement pattern, using an optionalwhere_clauseto support both targeted and full-table deletions.nexum_core/src/sql/parser.rs (2)
64-81: LGTM!The DELETE parsing logic correctly extracts the table name and optional WHERE clause. The implementation follows the same pattern as SELECT statement parsing and includes appropriate error handling.
239-265: LGTM!The parsing tests provide good coverage for both DELETE scenarios (with and without WHERE clause), verifying table extraction and WHERE clause handling.
nexum_core/src/executor/mod.rs (1)
249-252: LGTM!The
ExecutionResult::Deletedvariant follows the established pattern and provides appropriate metadata about the deletion operation.
nexum_core/src/executor/mod.rs
Outdated
| Statement::Delete { | ||
| table, | ||
| where_clause, | ||
| } => { | ||
| let schema = self.catalog.get_table(&table)?.ok_or_else(|| { | ||
| StorageError::ReadError(format!("Table {} not found", table)) | ||
| })?; | ||
|
|
||
| let prefix = Self::table_data_prefix(&table); | ||
| let all_rows = self.storage.scan_prefix(&prefix)?; | ||
|
|
||
| let mut deleted_count = 0; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider memory efficiency for large tables.
The implementation loads all rows into memory via scan_prefix before filtering and deletion. For very large tables, this could be memory-intensive. However, this matches the existing pattern used in SELECT operations (line 100), so it's consistent with the current architecture.
If the storage engine supports streaming or iterative scanning in the future, consider refactoring to process rows incrementally.
🤖 Prompt for AI Agents
In nexum_core/src/executor/mod.rs around lines 165 to 177, the DELETE branch
currently calls self.storage.scan_prefix which loads all rows into memory;
change the implementation to iterate over rows incrementally instead of
materializing them all — use the storage engine's streaming/iterator API (e.g.,
scan_prefix_iter or a paginated scan) or implement chunked pagination to process
and delete rows one batch at a time, updating deleted_count per-row and
performing deletions as you go to avoid high memory use for large tables.
| #[test] | ||
| fn test_delete_with_where_clause() { | ||
| let storage = StorageEngine::memory().unwrap(); | ||
| let executor = Executor::new(storage); | ||
|
|
||
| // Create table | ||
| let create = Statement::CreateTable { | ||
| name: "test_delete".to_string(), | ||
| columns: vec![ | ||
| Column { | ||
| name: "id".to_string(), | ||
| data_type: DataType::Integer, | ||
| }, | ||
| Column { | ||
| name: "name".to_string(), | ||
| data_type: DataType::Text, | ||
| }, | ||
| ], | ||
| }; | ||
| executor.execute(create).unwrap(); | ||
|
|
||
| // Insert rows | ||
| let insert = Statement::Insert { | ||
| table: "test_delete".to_string(), | ||
| columns: vec!["id".to_string(), "name".to_string()], | ||
| values: vec![ | ||
| vec![Value::Integer(1), Value::Text("Alice".to_string())], | ||
| vec![Value::Integer(2), Value::Text("Bob".to_string())], | ||
| vec![Value::Integer(3), Value::Text("Charlie".to_string())], | ||
| ], | ||
| }; | ||
| executor.execute(insert).unwrap(); | ||
|
|
||
| // Delete with WHERE clause | ||
| use sqlparser::dialect::GenericDialect; | ||
| use sqlparser::parser::Parser as SqlParser; | ||
| let dialect = GenericDialect {}; | ||
| let ast = SqlParser::parse_sql(&dialect, "SELECT * FROM t WHERE id = 2").unwrap(); | ||
| let where_expr = if let sqlparser::ast::Statement::Query(query) = &ast[0] { | ||
| if let sqlparser::ast::SetExpr::Select(select) = &*query.body { | ||
| select.selection.clone().map(Box::new) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let delete = Statement::Delete { | ||
| table: "test_delete".to_string(), | ||
| where_clause: where_expr, | ||
| }; | ||
|
|
||
| let result = executor.execute(delete).unwrap(); | ||
| match result { | ||
| ExecutionResult::Deleted { table, rows } => { | ||
| assert_eq!(table, "test_delete"); | ||
| assert_eq!(rows, 1); | ||
| } | ||
| _ => panic!("Expected Deleted result"), | ||
| } | ||
|
|
||
| // Verify only 2 rows remain | ||
| let select = Statement::Select { | ||
| table: "test_delete".to_string(), | ||
| columns: vec!["*".to_string()], | ||
| where_clause: None, | ||
| order_by: None, | ||
| limit: None, | ||
| }; | ||
| let result = executor.execute(select).unwrap(); | ||
| match result { | ||
| ExecutionResult::Selected { rows, .. } => { | ||
| assert_eq!(rows.len(), 2); | ||
| } | ||
| _ => panic!("Expected Selected result"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_delete_all_rows() { | ||
| let storage = StorageEngine::memory().unwrap(); | ||
| let executor = Executor::new(storage); | ||
|
|
||
| // Create table | ||
| let create = Statement::CreateTable { | ||
| name: "test_delete_all".to_string(), | ||
| columns: vec![ | ||
| Column { | ||
| name: "id".to_string(), | ||
| data_type: DataType::Integer, | ||
| }, | ||
| ], | ||
| }; | ||
| executor.execute(create).unwrap(); | ||
|
|
||
| // Insert rows | ||
| let insert = Statement::Insert { | ||
| table: "test_delete_all".to_string(), | ||
| columns: vec!["id".to_string()], | ||
| values: vec![ | ||
| vec![Value::Integer(1)], | ||
| vec![Value::Integer(2)], | ||
| ], | ||
| }; | ||
| executor.execute(insert).unwrap(); | ||
|
|
||
| // Delete all (no WHERE clause) | ||
| let delete = Statement::Delete { | ||
| table: "test_delete_all".to_string(), | ||
| where_clause: None, | ||
| }; | ||
|
|
||
| let result = executor.execute(delete).unwrap(); | ||
| match result { | ||
| ExecutionResult::Deleted { rows, .. } => { | ||
| assert_eq!(rows, 2); | ||
| } | ||
| _ => panic!("Expected Deleted result"), | ||
| } | ||
|
|
||
| // Verify no rows remain | ||
| let select = Statement::Select { | ||
| table: "test_delete_all".to_string(), | ||
| columns: vec!["*".to_string()], | ||
| where_clause: None, | ||
| order_by: None, | ||
| limit: None, | ||
| }; | ||
| let result = executor.execute(select).unwrap(); | ||
| match result { | ||
| ExecutionResult::Selected { rows, .. } => { | ||
| assert_eq!(rows.len(), 0); | ||
| } | ||
| _ => panic!("Expected Selected result"), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests are comprehensive with good coverage.
Both test cases thoroughly verify DELETE functionality:
test_delete_with_where_clause: Tests targeted deletion and verifies remaining rowstest_delete_all_rows: Tests full table deletion
However, lines 350-363 use a workaround to construct WHERE expressions by parsing a SELECT statement and extracting its WHERE clause. Consider creating a test helper function to simplify WHERE expression construction for future tests.
💡 Optional: Helper function for test WHERE expressions
#[cfg(test)]
fn create_where_clause(sql_condition: &str) -> Option<Box<sqlparser::ast::Expr>> {
use sqlparser::dialect::GenericDialect;
use sqlparser::parser::Parser as SqlParser;
let dialect = GenericDialect {};
let sql = format!("SELECT * FROM t WHERE {}", sql_condition);
let ast = SqlParser::parse_sql(&dialect, &sql).unwrap();
if let sqlparser::ast::Statement::Query(query) = &ast[0] {
if let sqlparser::ast::SetExpr::Select(select) = &*query.body {
return select.selection.clone().map(Box::new);
}
}
None
}This would simplify line 365 to:
let delete = Statement::Delete {
table: "test_delete".to_string(),
where_clause: create_where_clause("id = 2"),
};🤖 Prompt for AI Agents
nexum_core/src/executor/mod.rs lines ~350-363: the test builds a WHERE
expression by parsing a SELECT and extracting its selection inline, which is
repetitive and hard to reuse; add a #[cfg(test)] helper function (e.g.,
create_where_clause) that accepts a condition string, parses "SELECT * FROM t
WHERE {cond}" using GenericDialect and sqlparser::parser::Parser, extracts and
returns the selection as Option<Box<sqlparser::ast::Expr>>, then replace the
inline parsing block in test_delete_with_where_clause with a call to that helper
to produce where_clause.
| Delete { | ||
| table: String, | ||
| has_where: bool, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of Plan::Delete and has_where field
# Search for Plan::Delete pattern matching or field access
rg -nP --type=rust -C3 'Plan::Delete\s*\{'
# Search for has_where field access
rg -nP --type=rust -C3 '\.has_where'Repository: aviralgarg05/NexumDB
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Search for Plan::Delete handling in executor
rg -nP --type=rust -C5 'Plan::Delete'
# Also search for Statement::Delete in executor
rg -nP --type=rust -C5 'Statement::Delete' nexum_core/src/executor/Repository: aviralgarg05/NexumDB
Length of output: 3016
🏁 Script executed:
#!/bin/bash
# Find the execute method signature
rg -nP --type=rust 'fn execute\(' nexum_core/src/executor/
# Search for any usage of Plan enum (to see if it's used at all)
rg -nP --type=rust 'Plan::' nexum_core/src/executor/
# Check the Plan enum definition for all variants
rg -nP --type=rust -A 20 'enum Plan' nexum_core/src/Repository: aviralgarg05/NexumDB
Length of output: 1193
🏁 Script executed:
#!/bin/bash
# Search for any usage of plan() method
rg -nP --type=rust 'plan\('
# Search for Plan:: usage anywhere
rg -nP --type=rust 'Plan::'
# Check if plan() method result is captured or used
rg -nP --type=rust -B2 -A2 'fn plan\('Repository: aviralgarg05/NexumDB
Length of output: 856
🏁 Script executed:
#!/bin/bash
# Comprehensive search for calls to plan() or Planner::plan
rg -nP --type=rust 'Planner::plan|\.plan\('
# Also check for imports of Planner or plan
rg -nP --type=rust 'use.*Planner|use.*plan'
# Check all files that import from planner module
rg -nP --type=rust "from.*planner|planner::"Repository: aviralgarg05/NexumDB
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Final check: search for any .plan( in the entire codebase including tests
rg -nP --type=rust 'plan\s*\(' --glob='**/*.rs'
# Check test files specifically
find . -name '*test*.rs' -o -name 'tests' -type d | head -20Repository: aviralgarg05/NexumDB
Length of output: 197
🏁 Script executed:
#!/bin/bash
# Check integration tests for plan() calls
cat ./tests/integration_test.rs | grep -n "plan\|Planner" || echo "No matches in integration_test.rs"
# Check test directory
ls -la ./tests/ 2>/dev/null || echo "No tests directory at root"
# Check if there are other test files
find . -name '*.rs' -path '*/tests/*' -o -name '*test.rs' | head -10Repository: aviralgarg05/NexumDB
Length of output: 365
Remove the unused has_where field from Plan::Delete.
The has_where field is never read after being set. The executor operates directly on Statement::Delete and checks where_clause.is_some() itself (lines 165-168 in executor/mod.rs), making this field redundant. Additionally, the entire Plan enum and plan() method appear to be unused infrastructure—no callers invoke Planner::plan() anywhere in the codebase.
🤖 Prompt for AI Agents
In nexum_core/src/sql/planner.rs around lines 17 to 20, remove the unused
has_where field from the Plan::Delete variant because the executor reads the
WHERE directly from Statement::Delete; update the Plan::Delete definition to
only include table: String, then search and update any code that constructs or
pattern-matches on Plan::Delete to stop setting or expecting has_where (remove
that argument and any dead branches), run cargo build/tests to ensure no
remaining references, and leave the rest of Planner::plan() unchanged even if
currently unused.
- Replace unwrap_or(false) with explicit error handling for WHERE clause evaluation - Use proper logging (log crate) instead of println! statements - Add log dependency to Cargo.toml - Improve error messages for WHERE clause evaluation failures - Use appropriate log levels (info, warn, debug) for different message types This addresses the code review feedback about error handling, logging practices, and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nexum_core/src/executor/mod.rs (1)
328-405: Comprehensive test coverage for DELETE with WHERE.The test correctly verifies:
- Row deletion with WHERE clause filtering
- Deleted row count (1 row)
- Remaining rows after deletion (2 rows)
Note: Lines 362-374 still use the inline WHERE expression construction workaround that was mentioned in previous reviews as a potential helper function candidate. Since this was marked as optional/nitpick and the test is functionally correct, it can be addressed in future refactoring if needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nexum_core/Cargo.tomlnexum_core/src/executor/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
Focus on Rust best practices, memory safety, error handling, and idiomatic code patterns.
Files:
nexum_core/src/executor/mod.rs
🧬 Code graph analysis (1)
nexum_core/src/executor/mod.rs (1)
nexum_core/src/storage/engine.rs (3)
new(10-13)memory(15-21)delete(36-39)
🔇 Additional comments (5)
nexum_core/src/executor/mod.rs (5)
36-36: Good use of structured logging for cache initialization.The logging changes properly use
log::info!for successful cache initialization andlog::warn!for failures. This addresses the previous review feedback and enables runtime control of log output.Also applies to: 39-39
88-88: Excellent logging granularity for query execution.The use of
log::debug!for cache hits, filtering, sorting, limiting, and query duration provides appropriate visibility for debugging without cluttering production logs. This is well-aligned with the log crate's level system.Also applies to: 118-118, 149-149, 154-154, 219-219
165-215: DELETE implementation successfully addresses previous review feedback.The implementation now includes:
- Explicit error handling (lines 185-198): The match expression properly handles
Ok(true),Ok(false), andErrcases, propagating evaluation errors instead of silently ignoring them withunwrap_or(false).- Structured logging (line 203): Uses
log::warn!instead ofprintln!for the DELETE-without-WHERE warning.- Correct logic: Table validation, row filtering, deletion, and counting all work correctly.
260-263: LGTM! ExecutionResult::Deleted variant is well-designed.The new
Deletedvariant follows the same structure as other variants (e.g.,Inserted) and provides the necessary information (table name and deleted row count) for DELETE operation results.
407-464: LGTM! Comprehensive test for DELETE without WHERE.This test thoroughly validates the DELETE-all-rows behavior:
- Correctly executes DELETE without WHERE clause
- Verifies deleted row count (2 rows)
- Confirms the table is empty after deletion
- Exercises the warning log code path (line 203)
| serde = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| regex = "1.10" | ||
| log = "0.4" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the repository structure
echo "=== Repository structure ==="
ls -la | head -20
echo -e "\n=== Check for Cargo.toml files ==="
find . -name "Cargo.toml" -type f | head -20
echo -e "\n=== Workspace (root) Cargo.toml content ==="
if [ -f "Cargo.toml" ]; then
cat -n Cargo.toml
fi
echo -e "\n=== nexum_core/Cargo.toml content ==="
if [ -f "nexum_core/Cargo.toml" ]; then
cat -n nexum_core/Cargo.toml
fiRepository: aviralgarg05/NexumDB
Length of output: 2803
Move log and regex to workspace-level dependency management.
Lines 14-15 specify regex = "1.10" and log = "0.4" directly, while other dependencies (lines 7-13) use { workspace = true }. Define both in the workspace's Cargo.toml [workspace.dependencies] section and reference them here with { workspace = true } for consistency and centralized version management.
🤖 Prompt for AI Agents
In nexum_core/Cargo.toml around line 15, the dependency entry log = "0.4" (and
similarly regex = "1.10" at line 14) are pinned locally instead of using
workspace-managed versions; remove these explicit version entries here, add both
log = "0.4" and regex = "1.10" to the top-level Cargo.toml under
[workspace.dependencies], and then change the entries in nexum_core/Cargo.toml
to reference each with { workspace = true } so they follow the workspace-level
dependency management.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
nexum_core/src/executor/mod.rs (1)
186-187: Misleading comment: processing is not actually incremental.The comment on line 186 claims to "Process rows incrementally to avoid loading all into memory," but
scan_prefixon line 187 loads all rows at once into memory. The processing still iterates over an in-memory collection, not a streaming iterator.🔎 Proposed fix
Either update the comment to reflect the actual behavior:
- // Process rows incrementally to avoid loading all into memory + // Scan all rows matching the table prefix let all_rows = self.storage.scan_prefix(&prefix)?;Or, if the storage engine supports streaming/iterative scanning, implement true incremental processing as noted in a past review comment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nexum_core/src/executor/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
Focus on Rust best practices, memory safety, error handling, and idiomatic code patterns.
Files:
nexum_core/src/executor/mod.rs
🧬 Code graph analysis (1)
nexum_core/src/executor/mod.rs (4)
nexum_core/src/executor/filter.rs (1)
new(53-55)nexum_core/src/storage/engine.rs (3)
new(10-13)memory(15-21)delete(36-39)nexum_core/src/sql/parser.rs (1)
row(49-51)nexum_core/src/sql/types.rs (1)
data_type(23-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Python tests
- GitHub Check: Rust benchmarks
🔇 Additional comments (4)
nexum_core/src/executor/mod.rs (4)
93-93: LGTM: Consistent debug logging.The conversion from
println!tolog::debug!for operational metrics (cache hits, filtering, sorting, limiting, and query duration) is appropriate and follows Rust best practices. Debug level is suitable for these diagnostic messages.Also applies to: 123-123, 154-154, 159-159, 224-224
208-208: LGTM: Appropriate warning for DELETE without WHERE.The warning log correctly alerts operators when a DELETE without a WHERE clause is executed, which could be unintentional. The
warn!level is appropriate for this potentially destructive operation.
299-302: LGTM: ExecutionResult::Deleted variant follows existing patterns.The new
Deletedvariant is consistent with theInsertedvariant structure (table name and row count), making the API predictable and easy to use.
446-503: LGTM: Comprehensive test coverage for DELETE without WHERE.The
test_delete_all_rowstest thoroughly verifies DELETE behavior when no WHERE clause is provided: inserts 2 rows, deletes all, and confirms the table is empty. Combined withtest_delete_with_where_clause, the DELETE functionality has solid test coverage.
| log::info!("Semantic cache enabled"); | ||
| println!("Semantic cache enabled with file: {}", cache_file); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent logging approach: both log and println! used.
Line 40 uses log::info! while line 41 uses println! for essentially the same information. This is inconsistent and redundant. Consider including the cache file path in the log message and removing the println! call.
🔎 Proposed fix
- log::info!("Semantic cache enabled");
- println!("Semantic cache enabled with file: {}", cache_file);
+ log::info!("Semantic cache enabled with file: {}", cache_file);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log::info!("Semantic cache enabled"); | |
| println!("Semantic cache enabled with file: {}", cache_file); | |
| log::info!("Semantic cache enabled with file: {}", cache_file); |
🤖 Prompt for AI Agents
In nexum_core/src/executor/mod.rs around lines 40 to 41, there's inconsistent
logging: log::info! is used on line 40 while println! is used on line 41 for the
same event; replace the println! with a single log::info! that includes the
cache_file path so all output uses the logging framework (remove the println!
call and augment the existing log::info! to include the file path).
| use sqlparser::dialect::GenericDialect; | ||
| use sqlparser::parser::Parser as SqlParser; | ||
| let dialect = GenericDialect {}; | ||
| let ast = SqlParser::parse_sql(&dialect, "SELECT * FROM t WHERE id = 2").unwrap(); | ||
| let where_expr = if let sqlparser::ast::Statement::Query(query) = &ast[0] { | ||
| if let sqlparser::ast::SetExpr::Select(select) = &*query.body { | ||
| select.selection.clone().map(Box::new) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Consider test helper for WHERE clause construction.
Lines 401-413 construct a WHERE expression by parsing a SELECT statement and extracting its selection clause. This approach is verbose and was noted in a past review. While functional, a test helper function could improve readability and reusability across DELETE tests.
The past review (lines 367-503) suggested a create_where_clause helper function that would simplify this to:
let where_clause = create_where_clause("id = 2");This is a low-priority refinement for test maintainability.
🤖 Prompt for AI Agents
In nexum_core/src/executor/mod.rs around lines 401 to 413 the test builds a
WHERE expression by parsing a full SELECT SQL string which is verbose; add a
small test helper (e.g. create_where_clause(s: &str) -> Option<Box<Expr>> used
only in tests) that parses a predicate string and returns the boxed selection
expression, then replace the current parsing block with a single call like let
where_clause = create_where_clause("id = 2"); to improve readability and
reusability across DELETE tests.
- 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.
Fix formatting issues flagged by CI
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nexum_core/src/executor/mod.rs (1)
40-41: Logging inconsistency remains: both log and println! used.Line 40 uses
log::info!while line 41 usesprintln!for the same event. This exact issue was flagged in a previous review but wasn't fully addressed. The println! call should be removed and the cache file path should be included in the log message.🔎 Proposed fix
- log::info!("Semantic cache enabled"); - println!("Semantic cache enabled with file: {}", cache_file); + log::info!("Semantic cache enabled with file: {}", cache_file);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nexum_core/src/executor/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
Focus on Rust best practices, memory safety, error handling, and idiomatic code patterns.
Files:
nexum_core/src/executor/mod.rs
🧬 Code graph analysis (1)
nexum_core/src/executor/mod.rs (1)
nexum_core/src/storage/engine.rs (3)
new(10-13)memory(15-21)delete(36-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Rust benchmarks
- GitHub Check: Python tests
🔇 Additional comments (4)
nexum_core/src/executor/mod.rs (4)
93-93: LGTM: Logging improvements are well-implemented.The replacement of
println!statements with appropriatelog::debug!calls provides better operational visibility and control over logging output. The log levels are appropriate for the types of information being logged (cache hits, filtering, sorting, limiting, and query duration).Also applies to: 123-123, 154-154, 159-159, 237-237
170-233: Well-implemented DELETE with robust error handling.The two-phase deletion approach (collect keys, then delete) effectively addresses previous concerns about partial deletion when WHERE evaluation fails. Key strengths:
- Phase 1 (lines 190-207): Evaluates all rows and collects matching keys. If any evaluation fails, returns an error with the clear message "No rows were deleted."
- Phase 2 (lines 209-213): Only executes if Phase 1 succeeds, ensuring atomic-like behavior despite the lack of transaction support.
- Explicit error handling (lines 200-204): Replaces
unwrap_or(false)with proper match statement, providing visibility into evaluation failures.- Safety warning (line 221): Uses
log::warn!appropriately for DELETE without WHERE.The memory usage pattern (loading all rows via
scan_prefix) is consistent with the existing SELECT implementation and was noted in a previous review as acceptable.
312-315: LGTM: ExecutionResult::Deleted variant is well-designed.The new variant follows the established pattern of other execution results, providing the table name and deletion count. This is consistent with
ExecutionResult::Inserted.
459-516: LGTM: Complete test coverage for DELETE without WHERE clause.This test validates the full-table deletion scenario:
- Creates table and inserts test data
- Executes DELETE without WHERE clause
- Verifies correct deletion count (2 rows)
- Confirms table is empty via SELECT (0 rows)
The test properly exercises the warning path (line 221) where DELETE without WHERE is executed.
|
@aviralgarg05 Thanks for the feedback. I’ve resolved the issues pointed out in the review and updated the changes in the PR. Looking forward to your re-review. |
2 participants
Summary
Implements DELETE statement support for NexumDB SQL engine, addressing issue #45. This adds the ability to remove rows from tables using standard SQL DELETE syntax with optional WHERE clause filtering.
Changes Made
Core Implementation
Deletevariant toStatementenum with table name and optional WHERE clauseDELETE FROM table [WHERE condition]statementsDeletevariant to execution plan with metadataKey Features
DELETE FROM users WHERE id = 1DELETE FROM users(with safety warning)ExecutionResult::DeletedTechnical Details
ExpressionEvaluatorfor WHERE clause filtering (supports comparisons, logical operators, LIKE, IN, BETWEEN)delete()method for efficient row removalTesting
Example Usage