feat: Add UPDATE statement support#59
Conversation
- Add Update variant to Statement enum with table, assignments, and where_clause - Parse UPDATE table SET col = val, ... WHERE ... statements - Support multiple column assignments in SET clause - Implement two-phase atomic update execution in executor - Reuse ExpressionEvaluator for WHERE clause filtering - Add type checking for column/value type mismatches - Add Updated variant to ExecutionResult - Add comprehensive unit tests for parser and executor Closes aviralgarg05#46
Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
|
Warning Rate limit exceeded
⌛ 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 selected for processing (1)
WalkthroughAdds end-to-end SQL UPDATE support: parser recognizes UPDATE and builds assignments/optional WHERE; planner emits Plan::Update; executor performs two-phase per-row WHERE evaluation, assignment validation, and atomic batch writes via new StorageEngine::batch_set; introduces ExecutionResult::Updated, tests, and a new cache fixture Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant Planner
participant Executor
participant Storage
Client->>Parser: "UPDATE table SET col=val, ... WHERE cond"
Parser-->>Planner: Statement::Update
Planner-->>Executor: Plan::Update
Executor->>Storage: read rows for table
Storage-->>Executor: rows
Executor->>Executor: Phase 1: evaluate WHERE per row\nvalidate assignments, detect duplicates
Executor->>Executor: Phase 2: build serialized updates (batch)
Executor->>Storage: batch_set(operations)
Storage-->>Executor: persisted
Executor-->>Client: ExecutionResult::Updated { table, rows }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nexum_core/src/executor/mod.rs (1)
279-283: StorageEngine::clone creates a new empty database, not a true clone.This
Cloneimplementation returns a fresh in-memory storage instance, discarding all existing data. If any code relies on cloning to preserve data, it will silently lose all stored rows.This is particularly dangerous because
Cloneis typically expected to produce a semantically equivalent copy.🔎 Suggested alternatives
Remove the
Cloneimpl and refactor code that needs shared access to useArc<StorageEngine>instead.Rename to a more explicit method if creating a new empty instance is intentional:
impl StorageEngine { pub fn new_empty_memory() -> Result<Self> { StorageEngine::memory() } }
- If true cloning is needed, implement data copying (though this may be expensive for large datasets).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
cache/test_cache.jsonnexum_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/types.rsnexum_core/src/executor/mod.rsnexum_core/src/sql/parser.rsnexum_core/src/sql/planner.rs
🧬 Code graph analysis (2)
nexum_core/src/executor/mod.rs (5)
nexum_core/src/executor/filter.rs (1)
new(53-55)nexum_core/src/catalog/mod.rs (1)
new(18-20)nexum_core/src/storage/engine.rs (2)
new(10-13)memory(15-21)nexum_core/src/sql/parser.rs (1)
assignments(72-79)nexum_core/src/sql/types.rs (1)
data_type(23-31)
nexum_core/src/sql/planner.rs (1)
nexum_core/src/sql/parser.rs (1)
assignments(72-79)
⏰ 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 (14)
cache/test_cache.json (1)
1-787: Test fixture data looks appropriate.This JSON file provides test cache entries for semantic query caching. The structure is valid and the metadata fields (
similarity_threshold,cache_size,format_version) are well-defined.Note that while this PR adds UPDATE statement support, these cache entries contain only SELECT queries. Consider adding UPDATE query cache entries if the semantic cache is intended to support UPDATE statements as well.
nexum_core/src/sql/types.rs (1)
70-74: Well-structured Update variant.The
Updatevariant follows the established patterns in this enum. UsingOption<Box<Expr>>forwhere_clauseis consistent with theSelectvariant, andVec<(String, Value)>for assignments is appropriate for representing column-value pairs.nexum_core/src/sql/planner.rs (2)
17-21: Plan::Update variant is well-designed.The variant captures essential planning metadata: target table, affected columns, and whether a WHERE clause is present.
44-52: Planning logic for UPDATE is correct.The implementation correctly extracts column names from assignments and determines WHERE clause presence for plan metadata.
nexum_core/src/sql/parser.rs (3)
64-88: UPDATE parsing implementation looks correct.The parsing logic properly extracts table name, assignments, and optional WHERE clause. The column name construction using
join(".")correctly handles compound identifiers.
266-285: Multi-column UPDATE test is comprehensive.The test validates multiple assignments are parsed correctly with their column names.
287-304: UPDATE without WHERE test covers important edge case.This ensures the parser correctly handles UPDATE statements that affect all rows.
nexum_core/src/executor/mod.rs (7)
165-198: UPDATE validation and type checking logic is sound.The implementation correctly:
- Validates table existence
- Builds column index mapping
- Performs type checking with appropriate Null handling
- Returns clear error messages for type mismatches
One minor consideration: the type checking allows
Nullfor any column regardless of whether the schema permits nulls. If nullable constraints are added later, this logic will need updating.
200-232: Two-phase collection with WHERE evaluation is well-implemented.The approach of collecting all qualifying rows before applying updates prevents issues with iterating over a changing dataset. The early return on WHERE evaluation failure ensures no partial updates occur during the collection phase.
298-301: ExecutionResult::Updated variant is appropriate.The variant provides the necessary feedback (table name and row count) for UPDATE operations.
366-459: Comprehensive test for UPDATE with WHERE clause.The test properly:
- Sets up table and data
- Constructs WHERE clause using sqlparser
- Executes UPDATE and verifies row count
- Validates the specific row was updated via SELECT
Good use of verification query to confirm the update took effect.
461-538: Multi-column UPDATE test validates assignment handling.The test ensures multiple assignments are applied correctly to the same row.
540-606: UPDATE all rows test covers the no-WHERE-clause scenario.Good coverage for the case where all rows should be updated, with proper verification that every row received the new value.
304-364: Existing end-to-end test remains valid.The original test continues to verify basic CRUD operations work correctly.
- Run cargo fmt to fix formatting issues - Add batch_set method to StorageEngine for true atomic updates - Use sled's batch operations to ensure UPDATE atomicity - Add test for batch_set functionality Addresses CodeRabbit feedback on atomicity concerns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nexum_core/src/sql/parser.rs (1)
251-269: Tests verify structure but not parsed values.The tests assert assignment keys but not the assigned values. Consider adding value assertions for completeness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
nexum_core/src/executor/mod.rsnexum_core/src/sql/parser.rsnexum_core/src/storage/engine.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/storage/engine.rsnexum_core/src/executor/mod.rsnexum_core/src/sql/parser.rs
🧬 Code graph analysis (2)
nexum_core/src/storage/engine.rs (2)
nexum_ai/optimizer.py (1)
get(62-72)nexum_core/src/bridge/mod.rs (1)
get(84-97)
nexum_core/src/executor/mod.rs (5)
nexum_core/src/storage/engine.rs (2)
new(10-13)memory(15-21)nexum_core/src/executor/filter.rs (1)
new(53-55)nexum_core/src/catalog/mod.rs (1)
new(18-20)nexum_core/src/sql/parser.rs (1)
assignments(72-84)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 (8)
nexum_core/src/storage/engine.rs (2)
41-49: LGTM - Clean atomic batch write implementation.The
batch_setmethod correctly uses sled'sBatchAPI and follows the same flush pattern asset(). This properly addresses the atomicity requirement for UPDATE operations.Minor consideration: You could add an early return for empty operations to avoid an unnecessary
apply_batchcall, but this is optional since sled handles empty batches gracefully.
119-135: Good test coverage for batch operations.The test verifies basic batch insertion and retrieval. Consider adding an edge case test for empty batch operations to ensure no errors occur.
nexum_core/src/sql/parser.rs (1)
64-93: LGTM - UPDATE parsing follows established patterns.The implementation correctly:
- Extracts table name
- Maps assignments using
convert_exprfor value conversion- Handles qualified column names via dot-joining
- Uses the same
where_clausepattern as SELECTnexum_core/src/executor/mod.rs (5)
191-201: Type checking logic is correct.The implementation properly:
- Allows
Nullvalues for any column type- Validates that non-null values match the expected column type
- Provides clear error messages on type mismatch
214-238: Good fail-fast behavior on WHERE evaluation errors.The implementation correctly aborts the entire operation if WHERE clause evaluation fails for any row, ensuring no partial updates occur. This maintains consistency.
309-312: LGTM - Consistent enum variant definition.The
Updatedvariant follows the same pattern asInserted, providing the table name and affected row count.
377-470: Comprehensive test with proper verification.Good test structure: creates table, inserts data, performs update with WHERE clause, and verifies both the row count and actual data changes. The approach of parsing a WHERE clause from SQL is pragmatic for testing.
551-617: Good coverage for UPDATE without WHERE clause.This test properly validates the "update all rows" behavior and verifies that all rows received the new value.
- Replace .unwrap() with proper error handling for serde_json::to_vec() - Fail early if any row serialization fails before applying batch - Maintain atomicity guarantee by preventing partial updates on serialization errors - Add descriptive error message for serialization failures Addresses CodeRabbit feedback on potential panic in batch operation.
aviralgarg05
left a comment
aviralgarg05
left a comment
There was a problem hiding this comment.
@sshekhar563 Mention the issue it solves
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nexum_core/src/executor/mod.rs (2)
492-606: Fix test syntax errors: duplicate and conflicting statements.The test contains duplicate variable declarations and conflicting code:
- Lines 499-500: Duplicate
namefield inStatement::CreateTable- Lines 516-517: Duplicate
tablefield inStatement::Insert- Lines 562-563: Conflicting statement types (
selectvsdelete)- Lines 587-602: test logic expects 3 rows and searches for updated row, but line 602 expects only 2 rows
These conflicts will cause compilation errors.
I recommend restructuring this test to either:
- Focus solely on UPDATE testing with consistent variable names (
test_updatethroughout), or- Separate UPDATE and DELETE tests into distinct test functions
As written, the test mixes UPDATE and DELETE logic in a way that cannot compile.
688-783: Fix test: duplicate and conflicting statements.Similar to the previous tests, this test contains conflicting code:
- Lines 695-705: CREATE TABLE for test_update_all (marked with ~)
- Lines 706-710: CREATE TABLE for test_delete_all (not marked with ~)
- Lines 716-723: INSERT for test_update_all
- Lines 743-746: INSERT for test_delete_all
- Lines 774-778: Verification logic for UPDATE (checking status = "completed")
- Line 779: Expects 0 rows (DELETE verification)
The test mixes UPDATE and DELETE verification logic, causing assertion failures and confusion.
This test should verify only UPDATE behavior with consistent variable names and assertions throughout.
📜 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/executor/mod.rsnexum_core/src/sql/types.rsnexum_core/src/sql/parser.rs
🧬 Code graph analysis (2)
nexum_core/src/sql/planner.rs (1)
nexum_core/src/sql/parser.rs (1)
assignments(72-84)
nexum_core/src/executor/mod.rs (5)
nexum_core/src/sql/parser.rs (1)
assignments(72-84)nexum_core/src/storage/engine.rs (2)
new(10-13)memory(15-21)nexum_core/src/executor/filter.rs (1)
new(53-55)nexum_core/src/catalog/mod.rs (2)
new(18-20)serde_json(76-76)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/executor/mod.rs (4)
250-277: LGTM: Solid assignment validation with type checking.The implementation correctly:
- Maps column names to indices with clear error messages for missing columns
- Validates type compatibility between new values and column types
- Allows
Nullas a universal placeholder (line 267-268)- Provides descriptive error messages for type mismatches
282-311: LGTM: Two-phase update with proper WHERE evaluation.Phase 1 correctly:
- Evaluates the WHERE clause per row before collecting updates
- Handles missing WHERE clause (update all rows) explicitly
- Provides clear error messages if WHERE evaluation fails
- Aborts the entire update operation on evaluation errors, preserving atomicity
This prevents partial updates if WHERE evaluation fails mid-scan.
313-328: LGTM: Atomic batch updates with proper error handling.The implementation correctly addresses previous review feedback:
- Serializes all rows first with proper error handling (lines 320-322), avoiding the previous
.unwrap()panic risk- Only applies the batch if all serializations succeed (line 327)
- Uses
storage.batch_set()for true atomicity via sled's batch API- Fails early on serialization errors to preserve atomicity guarantees
330-340: LGTM: Good user feedback for UPDATE without WHERE.Printing a message when updating all rows without a WHERE clause (lines 330-335) is helpful for preventing accidental mass updates. The
ExecutionResult::Updatedvariant properly reports the affected table and row count.nexum_core/src/sql/parser.rs (1)
297-316: Fix syntax error: missing closing brace.The test function
test_parse_update_multiple_columnsis missing its closing brace. This will cause a compilation error.🔎 Proposed fix
_ => panic!("Expected Update statement"), + } } #[test]Likely an incorrect or invalid review comment.
| #[test] | ||
| fn test_update_multiple_columns() { | ||
| fn test_delete_all_rows() { | ||
| let storage = StorageEngine::memory().unwrap(); | ||
| let executor = Executor::new(storage); | ||
|
|
||
| // Create table | ||
| let create = Statement::CreateTable { | ||
| name: "test_update_multi".to_string(), | ||
| columns: vec![ | ||
| Column { | ||
| name: "id".to_string(), | ||
| data_type: DataType::Integer, | ||
| }, | ||
| Column { | ||
| name: "name".to_string(), | ||
| data_type: DataType::Text, | ||
| }, | ||
| Column { | ||
| name: "age".to_string(), | ||
| data_type: DataType::Integer, | ||
| }, | ||
| ], | ||
| }; | ||
| executor.execute(create).unwrap(); | ||
|
|
||
| // Insert a row | ||
| let insert = Statement::Insert { | ||
| table: "test_update_multi".to_string(), | ||
| columns: vec!["id".to_string(), "name".to_string(), "age".to_string()], | ||
| values: vec![vec![ | ||
| Value::Integer(1), | ||
| Value::Text("Alice".to_string()), | ||
| Value::Integer(25), | ||
| ]], | ||
| }; | ||
| executor.execute(insert).unwrap(); | ||
|
|
||
| // Update multiple columns | ||
| let update = Statement::Update { | ||
| table: "test_update_multi".to_string(), | ||
| assignments: vec![ | ||
| ("name".to_string(), Value::Text("Alicia".to_string())), | ||
| ("age".to_string(), Value::Integer(26)), | ||
| ], | ||
| where_clause: None, | ||
| }; | ||
|
|
||
| let result = executor.execute(update).unwrap(); | ||
| match result { | ||
| ExecutionResult::Updated { rows, .. } => { | ||
| assert_eq!(rows, 1); | ||
| } | ||
| _ => panic!("Expected Updated result"), | ||
| } | ||
|
|
||
| // Verify the update | ||
| let select = Statement::Select { | ||
| table: "test_update_multi".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(), 1); | ||
| let row = &rows[0]; | ||
| if let Value::Text(name) = &row.values[1] { | ||
| assert_eq!(name, "Alicia"); | ||
| } | ||
| if let Value::Integer(age) = &row.values[2] { | ||
| assert_eq!(*age, 26); | ||
| } | ||
| } | ||
| _ => panic!("Expected Selected result"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix test: incomplete function definition.
The test function test_update_multiple_columns starts on line 609 but the function name on line 610 is test_delete_all_rows(), creating a naming mismatch. Additionally, the test body contains conflicting logic:
- Lines 616-631: CREATE TABLE for test_update_multi (marked as changed)
- Lines 706-710: CREATE TABLE for test_delete_all (not marked as changed)
This creates duplicate structure definitions and compilation errors.
The test needs to be cleaned up to have consistent naming and structure.
🤖 Prompt for AI Agents
In nexum_core/src/executor/mod.rs around lines 608 to 686, the test function
declaration is inconsistent (function opens as test_update_multiple_columns but
the next line declares test_delete_all_rows), causing a malformed/incomplete
function and duplicate test/table setup; rename the inner declaration to match
test_update_multiple_columns (or remove the extra duplicate fn line), remove any
duplicated CREATE TABLE/INSERT/SELECT blocks belonging to a different test
(test_delete_all), and ensure the test has a single, correctly named fn with
matching braces so the compile-time duplicate definitions and mismatched names
are resolved.
- Fix corrupted executor/mod.rs with proper test functions - Fix corrupted parser.rs with complete Update statement handling - Fix corrupted planner.rs with proper Plan enum variants - Fix corrupted types.rs with complete Statement enum - Ensure all closing braces and delimiters are properly matched - Separate DELETE and UPDATE tests properly
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nexum_core/src/executor/mod.rs`:
- Around line 252-279: Handle duplicate column assignments by deduplicating on
column index and keeping the last assignment: iterate over `assignments`,
resolve `col_idx` (as currently done), perform the existing type check against
`schema.columns[col_idx].data_type`, then insert/overwrite into a temporary map
keyed by `col_idx` (e.g., HashMap<usize, Value>) so later occurrences replace
earlier ones; after the loop, convert that map into `assignment_indices:
Vec<(usize, Value)>` (preserving whatever order you need for application)
instead of pushing duplicates directly into `assignment_indices`. This makes the
"last value wins" behavior explicit and avoids multiple entries for the same
column.
In `@nexum_core/src/sql/parser.rs`:
- Around line 267-285: The test test_parse_update_single_column is missing an
assertion for the parsed assigned value; update the test to assert that
assignments[0].1 equals the expected Value::Text("Bob".to_string()) (e.g.
assert_eq!(assignments[0].1, Value::Text("Bob".to_string()))), and import or
qualify the Value enum as needed so the variant resolves in the test scope.
📜 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/parser.rsnexum_core/src/sql/planner.rsnexum_core/src/executor/mod.rsnexum_core/src/sql/types.rs
🧬 Code graph analysis (1)
nexum_core/src/sql/planner.rs (1)
nexum_core/src/sql/parser.rs (1)
assignments(72-84)
⏰ 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 (13)
nexum_core/src/sql/types.rs (1)
74-78: LGTM!The
Updatevariant is now complete withtable,assignments, andwhere_clausefields. The structure is consistent with theDeletevariant, and usingBox<Expr>for the where clause is appropriate to keep the enum size manageable.nexum_core/src/sql/planner.rs (2)
21-25: LGTM!The
Plan::Updatevariant appropriately captures the table name, affected column names, and whether a WHERE clause is present. This is consistent with the existingPlan::Deletepattern.
55-63: LGTM!The match arm correctly transforms
Statement::UpdateintoPlan::Update, extracting column names from assignments and derivinghas_wherefrom the where clause presence.nexum_core/src/sql/parser.rs (3)
64-93: LGTM! Well-structured UPDATE parsing.The implementation correctly:
- Extracts the table name
- Builds assignment pairs by mapping column identifiers and converting expressions
- Constructs an optional WHERE clause from the selection
The pattern is consistent with the existing DELETE handling.
304-323: LGTM!Good test coverage for multi-column UPDATE parsing. The test verifies both column names are correctly extracted.
325-342: LGTM!The test appropriately validates UPDATE without a WHERE clause, confirming
where_clause.is_none().nexum_core/src/executor/mod.rs (7)
239-279: Solid implementation with proper validation and type checking.The UPDATE execution correctly:
- Validates table existence and retrieves schema
- Maps column names to indices with clear error messages
- Performs type checking allowing NULL values for any column type
281-313: LGTM! Two-phase update with proper WHERE evaluation.The implementation correctly:
- Evaluates WHERE clause per row with proper error propagation
- Updates all rows when no WHERE clause is present
- Collects updates before applying to ensure atomicity
315-330: LGTM! Proper atomicity with batch operations.The serialization errors are now handled with
map_errinstead ofunwrap, andbatch_setis used to apply all updates atomically. This addresses the atomicity and panic concerns from previous reviews.
427-430: LGTM!The new
Updatedvariant is consistent with the existingDeletedandInsertedvariants.
628-721: LGTM! Comprehensive test for UPDATE with WHERE clause.The test properly:
- Sets up table with multiple rows
- Applies UPDATE with WHERE filtering
- Verifies only the targeted row was updated
- Confirms unchanged rows remain intact
723-800: LGTM!Good coverage for multi-column UPDATE, verifying both columns are updated correctly.
802-868: LGTM!Properly tests UPDATE without WHERE clause, confirming all rows are updated.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add value assertion in test_parse_update_single_column for completeness - Add duplicate column assignment validation in UPDATE executor - Prevent same column from being assigned multiple times in single UPDATE
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@nexum_core/src/executor/mod.rs`:
- Around line 814-880: Add negative-path tests for type/column errors alongside
test_update_all_rows: create new tests (e.g., test_update_type_mismatch_error,
test_update_duplicate_assignment_error, test_update_unknown_column_error) that
use Executor::new and Statement::Update to trigger failures; for type mismatch,
create a table with an Integer column and attempt an Update assigning
Value::Text to that column, assert executor.execute(update) returns Err and the
error string contains "Type mismatch"; for duplicate assignment, build an Update
with the same column listed twice and assert Err contains "Duplicate
assignment"; for unknown column, update a non-existent column and assert Err
contains "Unknown column". Ensure tests reference the same structs/enums
(Statement::Update, Value, Column, DataType, ExecutionResult) so they exercise
the existing validation paths.
- Around line 317-322: The update loop inside the should_update block can panic
if a stored Row has fewer values than expected when indexing into row.values;
change the assignment loop that iterates over assignment_indices to use a safe
access (e.g., row.values.get_mut(*col_idx)) and handle the None case instead of
direct indexing — either skip the assignment and log/record an error, return a
Result/Error from the surrounding function, or otherwise surface the corruption
so updates.push((key.clone(), row)) is not reached with a partially
mutated/invalid row; update the surrounding function (the executor update
routine in mod.rs) to propagate or log the error consistently.
In `@nexum_core/src/sql/parser.rs`:
- Around line 305-324: Update the test_parse_update_multiple_columns unit test
to assert the parsed assignment values as well as their keys: after confirming
assignments[0].0 == "name" and assignments[1].0 == "age", add assertions that
assignments[0].1 equals Value::Text("Bob".to_string()) and assignments[1].1
equals Value::Integer(30). Ensure Value is in scope (same enum used by
Parser/Statement) and keep these assertions inside the Statement::Update match
arm.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nexum_core/src/executor/mod.rsnexum_core/src/sql/parser.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/parser.rsnexum_core/src/executor/mod.rs
⏰ 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 (9)
nexum_core/src/sql/parser.rs (3)
64-93: Well-structured UPDATE statement parsing.The implementation correctly:
- Extracts table name from the UPDATE statement
- Builds assignment pairs by joining nested identifiers (supporting qualified column names like
table.column)- Converts expression values via
convert_expr- Handles optional WHERE clause by cloning the selection expression
The pattern follows the existing DELETE handling approach consistently.
267-286: Good test coverage for single-column UPDATE parsing.The test now includes the value assertion (
assert_eq!(assignments[0].1, Value::Text("Bob".to_string()))), addressing the previous review feedback. The test comprehensively validates table name, assignment structure, and WHERE clause presence.
326-343: LGTM!The test correctly validates UPDATE parsing without a WHERE clause, checking that
where_clauseisNone.nexum_core/src/executor/mod.rs (6)
327-356: Well-implemented atomic batch update with proper error handling.The implementation correctly:
- Serializes all rows first, failing early if any serialization fails (preventing partial updates)
- Uses
batch_setfor atomic application of all updates- Logs a warning when UPDATE affects all rows (no WHERE clause)
This addresses the previous atomicity and panic concerns effectively.
252-262: Good implementation of duplicate column assignment detection.This correctly rejects SQL like
SET name = 'A', name = 'B'with a clear error message, preventing undefined behavior.
439-442: LGTM!The
Updatedvariant is consistent with the existingInsertedandDeletedvariants.
640-733: Comprehensive test for UPDATE with WHERE clause.The test correctly:
- Sets up a table with multiple rows
- Executes an UPDATE targeting a specific row
- Verifies only the expected row was updated
- Validates the new value was applied correctly
735-812: Good test for multi-column UPDATE.The test validates that multiple columns can be updated in a single statement and verifies both updated values.
814-880: Good test for UPDATE without WHERE clause.The test correctly verifies that all rows are updated when no WHERE clause is provided, matching the expected SQL semantics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add value assertions in test_parse_update_multiple_columns - Add bounds check for row.values access with proper error handling - Add error path tests for type mismatch, duplicate column, and unknown column - Prevent potential panic from corrupted row data
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nexum_core/src/sql/parser.rs`:
- Around line 328-345: The test test_parse_update_without_where checks structure
but not the assigned boolean value; update it to inspect the parsed assignment
from Parser::parse's Statement::Update branch by extracting assignments (e.g.,
assignments[0]) and asserting its value equals true (or the expected boolean) to
match other UPDATE parsing tests and ensure the AST stores the correct boolean
literal.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nexum_core/src/executor/mod.rsnexum_core/src/sql/parser.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.rsnexum_core/src/sql/parser.rs
🧬 Code graph analysis (1)
nexum_core/src/executor/mod.rs (4)
nexum_core/src/storage/engine.rs (2)
new(10-13)memory(15-21)nexum_core/src/executor/filter.rs (1)
new(53-55)nexum_core/src/sql/parser.rs (1)
assignments(72-84)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 (12)
nexum_core/src/sql/parser.rs (3)
64-93: LGTM! Clean UPDATE parsing implementation.The parsing logic correctly extracts table name, assignments, and optional WHERE clause. The use of
join(".")for compound identifiers properly handles both simple column names and qualified references.
267-286: LGTM! Comprehensive test with value assertion.The test properly validates the table name, assignment structure, parsed value, and WHERE clause presence.
305-326: LGTM! Multi-column UPDATE parsing is well-tested.Both column names and their corresponding values are properly asserted.
nexum_core/src/executor/mod.rs (9)
239-251: LGTM! Standard table lookup pattern.Consistent with existing DELETE/SELECT implementations.
252-262: LGTM! Proper duplicate column detection.The HashSet-based check efficiently catches duplicate assignments with a clear error message.
264-291: LGTM! Sound type checking with NULL handling.The implementation correctly allows NULL values for any column type while enforcing type compatibility for non-NULL values. Error messages are clear and include relevant context.
301-333: Solid two-phase update with proper bounds checking.The bounds check using
get_mut(line 320) properly guards against corrupted row data. WHERE clause evaluation failures correctly abort the entire operation.One observation: if
serde_json::from_slicefails (line 302), the row is silently skipped. This is consistent with DELETE behavior, but corrupted rows would remain unmodified without any indication. Consider logging a warning for failed deserializations in a future improvement.
335-350: LGTM! Proper atomic batch updates with fail-fast serialization.The implementation correctly serializes all rows first, ensuring that any serialization failure prevents partial updates. The
batch_setcall provides atomicity.
352-364: LGTM! Helpful warning for full-table updates.The warning log for UPDATE without WHERE clause is a good safety measure.
447-450: LGTM! Consistent ExecutionResult variant.The
Updatedvariant follows the same pattern asDeletedandInserted.
648-741: LGTM! Thorough UPDATE test with data verification.The test properly validates both the operation result and the actual data state after the update.
890-1011: LGTM! Comprehensive error path coverage.The error tests for type mismatch, duplicate column, and unknown column ensure robust validation. This addresses the previous review suggestion for error path testing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Assert column name 'active' and value Boolean(true) for consistency - Matches the pattern used in other UPDATE parsing tests
|
@aviralgarg05 I've fix the issue |
2 participants
Summary
Implements SQL UPDATE statement support to allow users to modify existing rows in tables.
Changes
UPDATE table SET col = val, ... WHERE ...syntaxUpdatevariant toStatementenum with table, assignments, and where_clause fieldsUpdatevariant toPlanenumFeatures
Testing
Added comprehensive unit tests for:
Example Usage