Skip to content

Commit a208673

Browse files
committed
fix: Address CodeRabbit review feedback
- 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.
1 parent 17974b8 commit a208673

2 files changed

Lines changed: 25 additions & 13 deletions

File tree

nexum_core/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ thiserror = { workspace = true }
1212
serde = { workspace = true }
1313
serde_json = { workspace = true }
1414
regex = "1.10"
15+
log = "0.4"
1516

1617
[dev-dependencies]
1718
tempfile = "3.8"

nexum_core/src/executor/mod.rs

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,10 @@ impl Executor {
3333
match SemanticCache::new() {
3434
Ok(cache) => {
3535
self.cache = Some(cache);
36-
println!("Semantic cache enabled");
36+
log::info!("Semantic cache enabled");
3737
}
3838
Err(e) => {
39-
println!("Warning: Could not initialize semantic cache: {}", e);
39+
log::warn!("Could not initialize semantic cache: {}", e);
4040
}
4141
}
4242
self
@@ -85,7 +85,7 @@ impl Executor {
8585
let query_str = format!("SELECT {:?} FROM {}", columns, table);
8686

8787
if let Ok(Some(cached_result)) = cache.get(&query_str) {
88-
println!("Cache hit for query: {}", query_str);
88+
log::debug!("Cache hit for query: {}", query_str);
8989
let rows: Vec<Row> =
9090
serde_json::from_str(&cached_result).unwrap_or_else(|_| Vec::new());
9191
return Ok(ExecutionResult::Selected { columns, rows });
@@ -115,7 +115,7 @@ impl Executor {
115115
.unwrap_or(false)
116116
});
117117

118-
println!("Filtered {} rows using WHERE clause", rows.len());
118+
log::debug!("Filtered {} rows using WHERE clause", rows.len());
119119
}
120120

121121
if let Some(order_clauses) = order_by {
@@ -146,12 +146,12 @@ impl Executor {
146146
}
147147
}
148148

149-
println!("Sorted {} rows using ORDER BY", rows.len());
149+
log::debug!("Sorted {} rows using ORDER BY", rows.len());
150150
}
151151

152152
if let Some(limit_count) = limit {
153153
rows.truncate(limit_count);
154-
println!("Limited to {} rows using LIMIT", limit_count);
154+
log::debug!("Limited to {} rows using LIMIT", limit_count);
155155
}
156156

157157
if let Some(cache) = &self.cache {
@@ -171,26 +171,37 @@ impl Executor {
171171
})?;
172172

173173
let prefix = Self::table_data_prefix(&table);
174-
let all_rows = self.storage.scan_prefix(&prefix)?;
175-
176174
let mut deleted_count = 0;
177175

178176
if let Some(where_expr) = where_clause {
179177
let column_names: Vec<String> =
180178
schema.columns.iter().map(|c| c.name.clone()).collect();
181179
let evaluator = ExpressionEvaluator::new(column_names);
182180

181+
// Process rows incrementally to avoid loading all into memory
182+
let all_rows = self.storage.scan_prefix(&prefix)?;
183183
for (key, value) in &all_rows {
184184
if let Ok(row) = serde_json::from_slice::<Row>(value) {
185-
if evaluator.evaluate(&where_expr, &row.values).unwrap_or(false) {
186-
self.storage.delete(key)?;
187-
deleted_count += 1;
185+
match evaluator.evaluate(&where_expr, &row.values) {
186+
Ok(true) => {
187+
self.storage.delete(key)?;
188+
deleted_count += 1;
189+
}
190+
Ok(false) => {
191+
// Row doesn't match WHERE condition, skip
192+
}
193+
Err(e) => {
194+
return Err(StorageError::ReadError(format!(
195+
"WHERE clause evaluation failed: {}", e
196+
)));
197+
}
188198
}
189199
}
190200
}
191201
} else {
192202
// No WHERE clause - delete all rows
193-
println!("Warning: DELETE without WHERE clause will remove all rows from table '{}'", table);
203+
log::warn!("DELETE without WHERE clause will remove all rows from table '{}'", table);
204+
let all_rows = self.storage.scan_prefix(&prefix)?;
194205
for (key, _) in &all_rows {
195206
self.storage.delete(key)?;
196207
deleted_count += 1;
@@ -205,7 +216,7 @@ impl Executor {
205216
};
206217

207218
let duration = start.elapsed();
208-
println!("Query executed in {:?}", duration);
219+
log::debug!("Query executed in {:?}", duration);
209220

210221
result
211222
}

0 commit comments

Comments
 (0)