fix(dml): support subqueries in UPDATE SET clause

[mjm918]

Summary

• Adds support for scalar subqueries in UPDATE SET clause (e.g., UPDATE orders SET total = (SELECT SUM(quantity * unit_price) FROM order_items WHERE order_id = 1))
• Pre-computes scalar subqueries before the main UPDATE loop to avoid repeated execution
• Handles both TableScan and SecondaryIndexScan plan sources for subqueries
• Adds manual aggregate computation for expression-based aggregates that the executor can't handle natively

Test plan

• Run cargo test e_commerce_order_flow - the failing test that motivated this fix
• Run full test suite - cargo test
• Run cargo clippy - no warnings

Fixes #23

🤖 Generated with Claude Code

[claude]

Pull Request Review: Subquery Support in UPDATE SET Clause

This is a solid implementation that successfully enables scalar subqueries in UPDATE statements. However, I've identified several critical allocation issues that violate TurDB's zero-allocation principle.

CRITICAL: Allocation Violations

1. Massive Materialization (update.rs:521, 660)
Both TableScan and SecondaryIndexScan paths materialize ALL rows into Vec<Vec>. For a table with 1M rows, this allocates millions of heap objects. This must be replaced with streaming aggregate computation.

2. Multiple .clone() Calls (convert.rs:441, 451; update.rs:272)
Cloning OwnedValue violates zero-copy principle. Each clone allocates heap memory for Text/Blob/Jsonb variants. Should use references with appropriate lifetimes or arena allocation.

3. Unnecessary Vec Allocations (update.rs:510, 515, 577, 585, 660, 661)
Should use SmallVec for bounded collections like column_types and column_map (typically <20 items).

Code Quality Issues

4. Code Duplication: Binary Operators
Identical binary operator handling in convert.rs:472-519 and update.rs:288-324. Should extract to shared helper.

5. Duplicated Root Page Reading (update.rs:483, 489)
Two nearly identical helper functions should be consolidated.

6. Inefficient O(n²) Lookup (update.rs:746)
Linear scan with .iter().any() for duplicate detection. Should use HashMap for O(1) lookups.

7. Missing Error Context
Per ERRORS.md, errors need operation, resource, and failure reason. Many errors are too vague.

8. Potential Panic (update.rs:584)
.unwrap() on try_into() can panic. Should use ? with proper error context.

9. Unnecessary String Allocations (update.rs:510, 654)
to_lowercase() allocates unnecessarily in column_map construction.

10. Missing Subquery Validation
No check that subquery returns exactly one row/column (scalar requirement).

What's Good

1. Pre-computation strategy - executing subqueries once is correct
2. Pointer-based keying with std::ptr::from_ref is clever
3. Expression-based aggregate handling shows good understanding
4. Comprehensive expression traversal
5. Test coverage with realistic scenario

Action Items

Must Fix (Blocking):

• Eliminate materialized_rows allocation - stream aggregates instead
• Remove all .clone() calls
• Fix O(n²) subquery lookup

Should Fix:

• Extract duplicated binary operator logic
• Add scalar subquery validation
• Replace .unwrap() with proper error context
• Use SmallVec for bounded collections

Overall: functionally correct but needs allocation fixes to meet zero-allocation goals. Won't scale to large tables on embedded devices in current form.

[claude]

Code Review: PR #40 - Support Subqueries in UPDATE SET Clause

Summary

This PR adds support for scalar subqueries in UPDATE SET clauses (e.g., UPDATE orders SET total = (SELECT SUM(...) FROM ...)). The implementation includes streaming aggregates to avoid materializing all rows, proper error handling, and eliminates duplicate helper functions.

✅ Positive Aspects

1. Good adherence to error handling rules: Proper use of eyre, wrap_err_with, and detailed error contexts throughout
2. Streaming aggregation: Smart optimization using StreamingAggregateState to avoid materializing rows for expression-based aggregates
3. Test coverage: The e_commerce_order_flow test validates the core functionality
4. Code cleanup: Removed duplicate inner read_root_page/read_index_root_page functions
5. Pre-computation strategy: Subqueries are evaluated once before the UPDATE loop, avoiding repeated execution

⚠️ Critical Issues (MUST FIX)

1. ALLOCATION VIOLATIONS - Zero-Allocation Rule Broken

Multiple allocations detected in hot paths:

src/database/dml/update.rs:

• Line 706: Vec::new() for row keys - materializing all row IDs for SecondaryIndexScan path
• Line 716: let mut keys = Vec::new() - allocates for every scan range
• Line 787: Vec::with_capacity(row_keys.len()) - materializes all rows even for non-aggregate case

src/database/convert.rs:

• Lines 43, 441, 451: .clone() on OwnedValue parameters and subquery results

Recommendation:

• For the streaming aggregate path (lines 522-540), excellent work! This is zero-copy.
• For the non-aggregate SecondaryIndexScan path (lines 696-820), you're materializing ALL rows into Vec<Vec<OwnedValue>>. This violates zero-allocation principles.
• Consider using an iterator-based approach or arena allocation from the arena parameter already available.

2. Code Duplication - DRY Violation

The index scanning logic appears 6 times with slight variations:

• Lines 629-691: Streaming aggregate with PrefixScan/RangeScan/FullScan
• Lines 724-773: Row key extraction with PrefixScan/RangeScan/FullScan

Issues:

• process_index_cursor (line 612) and extract_row_key (line 718) are nearly identical closures defined separately
• The three scan range match arms (PrefixScan, RangeScan, FullScan) are duplicated twice with only minor differences

Recommendation:
Extract a shared function to handle all scan patterns with a callback.

3. Integer Overflow Risk

StreamingAggregateState (lines 384-385, 391-392):

self.sum_int += i;  // No overflow check
self.sum_float += f;  // Potential precision loss accumulation

Recommendation:

• Use checked_add or saturating_add for integer sums
• Document the overflow behavior or add explicit handling

4. Potential Correctness Issue - Mixed Int/Float Comparison

Lines 427, 445:

if (i as f64) < f {  // Lossy conversion for large i64 values
    OwnedValue::Int(i)
} else {
    OwnedValue::Float(f)
}

Issue: Converting i64 to f64 loses precision for values outside ±2^53. This could produce incorrect MIN/MAX results.

5. Performance - Unnecessary Materialization in SecondaryIndexScan

The non-aggregate SecondaryIndexScan path (lines 706-820):

1. Opens index storage and reads all matching row keys into Vec
2. Drops index storage guard
3. Opens table storage
4. Fetches each row by key and materializes into another Vec
5. Creates MaterializedRowSource
6. Builds and executes the subquery plan

Issues:

• The row_keys vector persists in memory even after we're done with the index
• We could stream directly from index→table→aggregate without materializing

⚠️ Minor Issues (SHOULD FIX)

6. Inconsistent Error Messages

Line 54:

.ok_or_else(|| eyre::eyre!("scalar subquery result not found in pre-computed results"))

Missing context: which subquery? Include the subquery pointer or a description.

7. Documentation Missing

The new functions lack doc comments:

• execute_scalar_subquery_for_update (line 468)
• collect_scalar_subqueries_from_expr (line 168)
• find_expression_aggregate (line 230)
• eval_binary_op (line 267)
• eval_expr_for_record_streaming (line 302)

Per STYLE.md, these should have brief /// comments explaining their purpose.

8. HashMap Creation in Hot Loop

Lines 508-513, 579-584:

let column_info: HashMap<String, (usize, DataType)> = table_def
    .columns()
    .iter()
    .enumerate()
    .map(|(i, c)| (c.name().to_lowercase(), (i, c.data_type())))
    .collect();

This creates a HashMap every time a subquery is executed. Consider pre-computing at planning stage.

💡 Optimization Opportunities

9. Pointer-as-Key Anti-Pattern

Lines 49, 857:

let key = std::ptr::from_ref(*subq) as usize;
scalar_subquery_results.iter().find(|(k, _)| *k == key)

Issues:

• Fragile: relies on subquery AST node addresses staying stable
• O(n) lookup in a Vec instead of O(1) with a proper HashMap
• Could fail if AST is copied/moved

Recommendation:
Use a proper HashMap<usize, OwnedValue> for ScalarSubqueryResults.

📋 Testing Concerns

10. Test Coverage Gaps

The e_commerce_order_flow test is good, but consider adding:

• Edge cases:

  • Subquery returning NULL
  • Subquery returning zero rows (should be NULL)
  • Subquery returning multiple rows (should error)
  • Nested subqueries
  • Subquery with aggregate over zero rows

• Error cases:

  • Subquery referencing non-existent table
  • Type mismatch between subquery result and target column

Per TESTING.md, each test should verify a specific bug it would catch.

🎯 Action Items

Critical (blocking merge):

1. Fix allocation violations in SecondaryIndexScan non-aggregate path (lines 706-820)
2. Eliminate code duplication in index scanning (extract shared function)
3. Fix integer overflow risk in StreamingAggregateState
4. Fix mixed int/float comparison correctness issue

Important (should fix before merge):
5. Add missing documentation for new public(crate) functions
6. Use HashMap for scalar_subquery_results instead of Vec with pointer keys
7. Add edge case tests (NULL, zero rows, multiple rows)

Nice-to-have (can defer):
8. Pre-compute column_info HashMap at planning stage
9. Optimize clone operations on critical paths
10. Add performance regression test

🏆 Overall Assessment

Code Quality: Good error handling and streaming optimization, but allocation violations need fixing

Correctness: Mostly correct, but int/float overflow and comparison issues need addressing

Performance: Good streaming path, but SecondaryIndexScan path materializes unnecessarily

Maintainability: Code duplication is a concern; needs refactoring

Recommendation: Request changes - fix critical issues before merge.

[claude]

Code Review - PR #40: Support Subqueries in UPDATE SET Clause

Summary

This PR successfully adds scalar subquery support to UPDATE SET clauses. The implementation pre-computes subquery results to avoid repeated execution, which is a solid architectural choice. However, there are several allocation issues and code quality concerns that violate TurDB's zero-allocation principles.

---

Critical Issues - Zero Allocation Violations ⚠️

1. Excessive Cloning in Hot Path (src/database/dml/update.rs)

Line 1255, 1421, 1789, 1789, 1898-1906, 1921-1922:

row_values[*col_idx] = val.clone();  // Line 1255
old_toast_values.push((*col_idx, row_values[*col_idx].clone()));  // Line 1421
processed_rows.push((key.clone(), updated_values.clone()));  // Line 1789

Problem: The UPDATE execution path performs multiple clones per row being updated. This violates the zero-allocation principle for CRUD operations.

Impact: For bulk updates, this creates O(n*m) allocations where n=rows and m=columns.

Suggested Fix: Consider using arena allocation (bumpalo) for temporary values during UPDATE execution, or restructure to work with references where possible.

---

2. HashMap Allocation in convert.rs (Line 450)

Line 450 in eval_expr_with_params_and_subqueries:

.cloned()

Problem: Cloning OwnedValue for every subquery lookup.

Suggested Fix: Since scalar_subquery_results is read-only during evaluation, consider returning a reference or using Cow<'_, OwnedValue> to avoid unnecessary clones.

---

3. Streaming Aggregate Implementation Creates Duplicate Code

Lines 285-511 (update.rs):
The eval_binary_op and binary operator handling in eval_expr_with_params_and_subqueries (convert.rs:470-520) are duplicated.

Problem:

• Code duplication between update.rs and convert.rs
• Both implement the same arithmetic operations on OwnedValue

Suggested Refactoring: Extract binary operation evaluation into a shared utility function in a common module (e.g., src/types/ops.rs or src/sql/eval.rs).

---

Code Quality Issues

4. File Length Approaching Limit

src/database/dml/update.rs: Now at ~2700 lines (estimated after this PR)

Problem: Approaching the 1800-line hard limit from STYLE.md.

Suggested Fix: Split into submodules:

• update/mod.rs - Main UPDATE logic
• update/subquery.rs - Scalar subquery execution (execute_scalar_subquery_for_update)
• update/streaming_agg.rs - Streaming aggregate implementation
• update/cascade.rs - Foreign key cascade logic

---

5. Missing Edge Case Tests

The PR adds comprehensive functionality but lacks tests for:

Missing test cases:

• ❌ Subquery returning NULL
• ❌ Subquery returning no rows (empty result)
• ❌ Subquery with multiple aggregate functions (should fail or return specific column)
• ❌ Nested subqueries in UPDATE SET
• ❌ Subquery with complex expressions (BinaryOp involving subqueries)
• ❌ UPDATE with both parameters AND subqueries

Required (per TESTING.md):

#[test]
fn update_with_null_subquery_result() {
    // Test: UPDATE t SET col = (SELECT val FROM empty_table)
    // Expected: col should be set to NULL
}

#[test]
fn update_with_subquery_and_parameters() {
    // Test: UPDATE t SET col = (SELECT x FROM y WHERE id = ?) + ?
    // Expected: Both parameter and subquery work together
}

---

6. Potential Performance Issue - Redundant Storage Access

Lines 653-665 (update.rs):

let index_storage_arc = file_manager.index_data(...)?;
let index_storage = index_storage_arc.read();
let index_root = read_index_root_page(&index_storage)?;
let index_reader = BTreeReader::new(&index_storage, index_root)?;

let table_storage_arc = file_manager.table_data(...)?;
let table_storage = table_storage_arc.read();
let table_root = read_root_page(&table_storage)?;
let table_reader = BTreeReader::new(&table_storage, table_root)?;

Problem: This pattern is repeated 3+ times in execute_scalar_subquery_for_update. Each repetition acquires read locks and creates new BTreeReaders.

Suggested Fix: Extract to a helper function that returns both readers, reducing code duplication and making lock acquisition patterns clearer.

---

Architectural Concerns

7. Pointer-Based HashMap Keys

Lines 48-54, 871-875:

let key = std::ptr::from_ref(*subq) as usize;
scalar_subquery_results.insert(key, result);

Concern: Using raw pointer addresses as HashMap keys is fragile:

• Assumes subquery AST nodes have stable addresses
• No guarantee across different invocations
• Difficult to debug when keys mismatch

This approach works but is non-obvious. Consider:

• Adding a safety comment explaining lifetime assumptions
• Or using a sequential ID assigned during planning phase

---

8. Float Comparison Logic

Lines 367-406 (compare_int_float):

Good: This implementation correctly handles precision issues when comparing i64 to f64.

Concern: This is complex logic buried in update.rs. Should be:

1. Moved to a shared utility module (e.g., src/types/cmp.rs)
2. Thoroughly tested with edge cases:
   • MAX_EXACT boundaries (2^53)
   • NaN, Infinity
   • Values near i64::MAX and i64::MIN

---

Positive Aspects ✅

1. Pre-computation Strategy: Executing subqueries once before the UPDATE loop is the correct approach
2. Error Handling: Good use of wrap_err_with with context throughout
3. Streaming Aggregates: Clever optimization to avoid full executor overhead for simple aggregates
4. Type Handling: Proper mixed Int/Float arithmetic in binary operators
5. Test Coverage: The e_commerce_order_flow test provides good real-world validation

---

Performance Considerations

9. Subquery Cache Not Shared Across Statements

Line 1016:

let mut scalar_subquery_results: ScalarSubqueryResults = ScalarSubqueryResults::new();

Observation: Each UPDATE statement creates a fresh cache. For prepared statements executed repeatedly, the same subquery is re-executed every time.

Potential Optimization (Future): For prepared statements with stable subqueries (no correlated references), consider caching results across executions. Not required for this PR, but worth noting.

---

10. Index Scan Memory Usage

Lines 684-748: The SecondaryIndexScan path for streaming aggregates doesn't bound memory usage if the scan is large.

Risk: A full index scan with aggregation could process millions of rows without a memory budget check.

Suggested Fix: Add periodic memory budget checks in the cursor loop, or set a configurable scan limit.

---

Documentation

11. Missing Module Documentation

Lines 178-838: The new functions lack module-level documentation explaining the subquery execution strategy.

Required (per STYLE.md): Each file should have 80-100 lines of module documentation at the top.

Suggested Addition:

//! # Scalar Subquery Support for UPDATE
//!
//! This module implements scalar subquery evaluation in UPDATE SET clauses.
//!
//! ## Strategy
//!
//! 1. Collect all scalar subqueries from SET expressions
//! 2. Pre-compute results before UPDATE loop (avoids N*M executions)
//! 3. Store results in HashMap keyed by subquery AST node address
//! 4. Substitute results during expression evaluation
//!
//! ## Performance Optimizations
//!
//! - Streaming aggregates: For simple aggregates like SUM(expr), we bypass
//!   the full executor and stream through BTree cursors directly
//! - Zero-copy: Results stored in arena allocations where possible
//!
//! ... (continue to 80-100 lines)

---

Recommendations

Blockers (Must Fix):

1. Reduce allocations in hot path (clones on lines 1255, 1421, 1789, etc.)
2. Add missing edge case tests (NULL results, empty results, nested subqueries)
3. Split update.rs into submodules (approaching line limit)

High Priority:

4. Extract duplicated binary op code into shared utility
5. Add module documentation explaining the subquery strategy
6. Extract compare_int_float to shared module with tests

Nice to Have:

7. Add safety comment explaining pointer-based HashMap keys
8. Consider memory budget checks in large index scans
9. Extract storage access pattern into helper function

---

Conclusion

This PR implements valuable functionality and demonstrates good understanding of the codebase. The streaming aggregate optimization is particularly clever. However, the implementation introduces allocation hotspots that violate TurDB's core zero-allocation principle for CRUD operations.

Recommendation: Request changes to address allocation issues and add missing tests before merge.

Estimated Effort: ~2-3 hours to fix allocation issues and add tests.

---

Great work on tackling a complex feature! The approach is sound; it just needs refinement to meet TurDB's strict performance requirements.

[claude]

Pull Request Review: Subquery Support in UPDATE SET Clause

Overall Assessment

This PR successfully implements scalar subquery support in UPDATE SET clauses with a streaming aggregate optimization. The code is functional and tests pass. However, there are critical zero-allocation violations and several optimization opportunities that need to be addressed per TurDB's strict memory rules.

---

🚨 CRITICAL: Zero-Allocation Violations

1. Unnecessary .cloned() in subquery lookup (src/database/convert.rs:450)

// CURRENT - allocates
scalar_subquery_results
    .get(&key)
    .cloned()
    .ok_or_else(|| eyre::eyre\!("..."))

// SHOULD BE - zero-copy
scalar_subquery_results
    .get(&key)
    .ok_or_else(|| eyre::eyre\!("..."))
    .map(|v| v) // Returns &OwnedValue, caller decides when to clone

Impact: This allocates a new OwnedValue on every subquery evaluation. For UPDATE statements with subqueries in SET clauses, this means one allocation per updated row.

---

🔧 Code Duplication Issues

2. Binary operator evaluation duplicated

Locations:

• src/database/convert.rs:470-520 (in eval_expr_with_params_and_subqueries)
• src/database/dml/update.rs:327-359 (in eval_binary_op)

The same arithmetic operations (Plus, Minus, Multiply, Divide) are implemented twice with identical logic. Extract to a shared function.

---

🎯 Performance Considerations

3. HashMap allocations for column info (lines 600-605, 683-688)

Every scalar subquery execution creates a HashMap for column lookup. Consider:

• Linear search through a slice for small column counts
• Arena allocation for reuse
• SmallVec for stack allocation

4. Streaming aggregate state min/max tracking (lines 463-479)

Current pattern allocates unnecessarily:

self.min_int = Some(self.min_int.map(|m| m.min(*i)).unwrap_or(*i));

Use simpler pattern without unwrap_or allocation.

---

🐛 Potential Bugs

5. Pointer-as-HashMap-key stability (line 918)

Using AST node addresses as HashMap keys is fragile. Consider sequential IDs or content-based hashing instead.

6. CRITICAL: Streaming aggregate bypasses WHERE filters

Documentation states (line 188):

Streaming optimization bypasses WHERE filters (full table scan for aggregates)

Looking at lines 621-632, I don't see WHERE predicate evaluation. This means:
UPDATE orders SET total = (SELECT SUM(amount) FROM order_items WHERE order_id = 1)

Would sum ALL order_items, not just order_id=1. This is a correctness bug.

Recommendation: Disable streaming optimization if scan.filter is present.

---

✅ Positive Aspects

1. Good streaming optimization concept
2. Comprehensive testing (3 test cases)
3. Excellent module documentation
4. Proper error context with wrap_err_with
5. Safe SecondaryIndexScan handling

---

🎯 Action Items

Must fix before merge:

1. Remove .cloned() at line 450 (zero-allocation violation)
2. Fix WHERE clause handling in streaming aggregate (correctness bug)

Should fix:
3. Deduplicate binary operator evaluation
4. Add test for subquery WITH WHERE clause
5. Replace pointer-as-key with safer alternative

Nice to have:
6. Optimize HashMap allocations
7. Simplify min/max tracking

---

Test Coverage Gaps

Add tests for:

• Subquery with WHERE clause (verify correctness!)
• Subquery returning NULL
• Subquery with zero rows
• Nested expressions: UPDATE t SET x = (SELECT a + b FROM ...)

---

Great work on a complex feature! Address the allocation violations and WHERE clause bug, and this will be ready to merge.

🤖 Reviewed with Claude Code

[mjm918]

Third Code Review Round - Addressed

Changes in commit 79b5282:

Documentation:

• Added comprehensive module documentation (~35 lines) covering:
  • Scalar subquery support strategy
  • Streaming aggregate optimization details
  • Limitations (non-correlated only, WHERE filters bypassed in streaming path)

Safety:

• Added SAFETY comment explaining AST node address stability for HashMap keys
• Documents why using pointer addresses is safe within function scope

Tests:

• update_with_subquery_sum_computes_total_correctly - verifies SUM aggregate
• update_with_subquery_avg_computes_average_correctly - verifies AVG aggregate
• update_with_multiple_subqueries_in_set_clause - verifies MIN, MAX, SUM in same UPDATE

Previously Addressed (commits 7eafd8c, c1fdab9):

• Integer overflow protection with saturating_add
• compare_int_float for proper int/float comparison
• Changed ScalarSubqueryResults from SmallVec to HashMap for O(1) lookup
• Eliminated materialization in SecondaryIndexScan non-aggregate path

Deferred (nice-to-have):

• Extract binary operator logic to shared utility
• Extract compare_int_float to shared module

All critical items from the code review have been addressed. Ready for re-review.

[mjm918]

Fourth Code Review Round - Critical Bug Fix

CRITICAL BUG FIXED: Streaming Aggregate WHERE Filter Bypass

Issue: Streaming aggregate optimization was bypassing WHERE filters in subqueries.

Example of broken behavior:

UPDATE orders SET total = (SELECT SUM(amount) FROM order_items WHERE order_id = 1)

Would incorrectly sum ALL order_items instead of just order_id = 1.

Fix (commit 604220b):

• Added plan_has_filter() helper to detect FilterExec in query plan tree
• Streaming optimization now disabled when filters are present
• Falls back to full executor which correctly applies WHERE filters
• Check both plan_has_filter(subq_plan.root) AND scan.post_scan_filter

Test added: update_with_subquery_where_filter_applied_correctly

• Creates order_items with multiple order_ids
• Verifies SUM with WHERE filter returns correct subset (25.0 not 325.0)

Other Fixes:

• Simplified min/max tracking pattern using map_or
• Updated documentation to reflect correct behavior

On .cloned() allocation concern:

The .cloned() in convert.rs is called during precomputation (once per expression), not per-row. For the common case of aggregate subqueries (Int/Float results), cloning is O(1). Removing it would require changing the function signature to return references, which is a larger API refactor.

Remaining (nice-to-have):

• Extract binary operator logic to shared utility (code duplication)

All critical issues have been addressed.

[claude]

Pull Request Review: Scalar Subquery Support in UPDATE

Overview

This PR adds scalar subquery support to UPDATE SET clauses with streaming aggregate optimization. The implementation pre-computes subquery results to avoid repeated execution.

---

Code Quality and Best Practices

EXCELLENT:

• Comprehensive module documentation (80+ lines) following STYLE.md requirements
• Proper error handling with rich context using eyre
• Clear separation of concerns with helper functions
• Good test coverage with multiple real-world scenarios

NEEDS ATTENTION:

1. ALLOCATION VIOLATIONS - CRITICAL

Per MEMORY.md rules, the CRUD path must be zero-allocation. Found violations:

src/database/convert.rs:441,450

• Line 441: Ok(params[idx].clone()) - FORBIDDEN allocation on params access
• Line 450: .cloned() - FORBIDDEN allocation on HashMap lookup

Fix: Return references instead of clones. This requires refactoring eval_expr_with_params_and_subqueries to return &OwnedValue instead of OwnedValue.

2. Code Duplication - Major Issue

The binary operator evaluation logic appears THREE times:

• src/database/convert.rs:470-520 in eval_expr_with_params_and_subqueries
• src/database/dml/update.rs:328-361 in eval_binary_op

Fix: Extract to a single shared helper in src/types/owned_value.rs

3. Potential Performance Issue

src/database/dml/update.rs:619-624, 626-631

Allocates HashMap and Vec on every subquery execution. Pre-compute these once before the subquery loop (lines 927-950) and pass as references.

---

Potential Bugs

1. Filter Detection Incomplete

Line 633: let has_filter = plan_has_filter(subq_plan.root) || scan.post_scan_filter.is_some();
Line 714: let has_filter = plan_has_filter(subq_plan.root); - Missing post_scan_filter check

Fix: Use the same check in both TableScan and SecondaryIndexScan paths.

2. Error Handling - Silent Failure

Lines 658, 689: .unwrap_or(OwnedValue::Null) silently converts empty result to Null. If row.values is empty (malformed result), this hides a bug.

Fix: Add validation using ok_or_else to catch malformed results.

3. Missing Edge Case - WHERE Filter on Index Scans

Test update_with_subquery_where_filter_applied_correctly doesn't verify SecondaryIndexScan paths handle WHERE filters correctly.

Recommendation: Add a test that forces SecondaryIndexScan usage and verify filtering still works.

---

Performance Considerations

EXCELLENT:

• Streaming aggregate optimization avoids materializing full result sets
• Pre-computation of subqueries prevents N×M execution
• Direct BTree cursor iteration bypasses executor overhead

OPTIMIZATION OPPORTUNITIES:

1. Pre-allocate StreamingAggregateState outside subquery loop
2. Avoid repeated schema creation (lines 636, 648, 703)
3. Consider lock holding duration - write lock on file_manager is held during all subquery execution

---

Testing

STRONG:

• Multiple aggregate functions tested (SUM, AVG, MIN, MAX)
• Multiple subqueries in one UPDATE
• WHERE clause interaction tested

MISSING:

1. Negative tests: multi-row subquery, NULL results, empty tables, division by zero
2. Edge cases: overflow, mixed Int/Float precision, non-existent tables
3. Test for SecondaryIndexScan with WHERE filter

---

Summary

Category	Grade	Notes
Functionality	A	Solves the problem effectively
Zero-Alloc Compliance	C	CRITICAL: Multiple .clone() violations
Code Duplication	C+	Binary op logic duplicated 3x
Performance	A-	Excellent streaming optimization
Testing	B+	Good coverage, missing negative tests
Documentation	A	Excellent module docs

---

Required Changes Before Merge

1. CRITICAL: Eliminate .clone() at convert.rs:441,450 - refactor to return &OwnedValue
2. CRITICAL: Consolidate binary operator evaluation logic
3. Fix filter detection inconsistency (line 633 vs 714)
4. Pre-compute column_info/column_map once, not per subquery

Recommended Improvements

1. Add negative test cases (multi-row subquery, NULL results, empty tables)
2. Document pointer key safety in more detail
3. Add test for SecondaryIndexScan with WHERE filter
4. Consider reducing file_manager lock hold duration

---

Great work on this complex feature! The streaming optimization is particularly clever. Once the allocation violations are fixed, this will be a solid addition.

[claude]

PR Review: Subquery Support in UPDATE SET Clause

Summary

This PR adds scalar subquery support to UPDATE SET clauses with a streaming aggregate optimization. The implementation is solid, but there are allocation issues violating TurDB's zero-alloc CRUD principle.

---

🚨 Critical Issues

1. Allocation in CRUD Path (Zero-Copy Violation)

Location: src/database/dml/update.rs:1210

let row_key = index_btree.get_value(&handle)?.to_vec();

Problem: Heap allocation for every PK lookup in UPDATE path.

Fix: Use fixed array or SmallVec since row keys are 8 bytes:

let row_key_slice = index_btree.get_value(&handle)?;
let row_key: [u8; 8] = row_key_slice.try_into()
    .wrap_err("invalid row key length from PK index")?;

2. Multiple Heap Allocations in Hot Path

Locations:

• Line 1126: column_types.clone() - Clones entire column type vector
• Line 1206: Vec::new() - Allocates for every PK lookup
• Line 1232-1233: Multiple Vec allocations

Fix: Pre-allocate and reuse buffers with SmallVec

3. Unclear Error Context

Location: src/database/convert.rs:452

Error message lacks table/operation context. Add which UPDATE and why subquery result missing.

---

⚠️ Code Quality Issues

4. Duplicated Logic

Lines 277-347: find_expression_aggregate vs find_any_aggregate are nearly identical. Extract common logic into shared helper.

5. Magic Constant 53

Line 424: Add doc comment explaining this is f64 mantissa bits for exact integer representation.

6. HashMap Lookups in Hot Path

Line 375: String allocation + HashMap lookup for every expression eval. Consider Vec indexed by column ordinal.

---

📊 Performance Concerns

7. Streaming Disabled With Filters

Lines 646-667: Streaming optimization disabled if ANY filter present, even simple WHERE clauses. Consider filter pushdown.

---

✅ What's Good

1. Excellent module-level documentation (lines 86-120)
2. Safety comment explaining pointer-as-key (lines 1103-1106)
3. Comprehensive test coverage
4. Smart SmallVec usage (line 1094)
5. Clever streaming optimization avoiding materialization

---

📋 Checklist

• Fix .to_vec() on line 1210 (use fixed array)
• Pre-allocate/reuse index key buffers
• Remove column_types.clone() on line 1126
• Deduplicate aggregate finder functions
• Add error context
• Document magic constant 53

---

Overall Assessment

Functionality: ✅ Solves the problem correctly
Performance: ⚠️ Good optimization, but CRUD path allocations
Code Quality: ⚠️ Some duplication
TurDB Rules: ❌ Violates zero-alloc CRUD principle

Recommendation: Approve after fixing critical allocation issues (#1 and #2). Others are improvements but not blockers.

Great work on the streaming aggregate optimization! 🎯

[claude]

PR Review: Support Subqueries in UPDATE SET Clause

Summary

This PR adds scalar subquery support to UPDATE SET clauses with a streaming aggregate optimization. The implementation is solid overall, but there are several allocation issues that violate TurDB's zero-allocation principle for CRUD operations.

---

Critical Issues - ALLOCATION VIOLATIONS

1. Line 930: columns.to_vec()

Unnecessary allocation. Use table_def.columns() directly instead of cloning.

2. Line 1211: SmallVec::from_slice()

Row keys are always [u8; 8]. Use fixed array instead of SmallVec.

3. Line 1249: .into_owned() on Cow

Forces allocation even when borrowed. Keep as Cow longer.

4. Lines 1224-1230: Nested Vec allocations

Each row update creates 5 heap allocations. Use arena or SmallVec.

5. Multiple .clone() in hot paths

Lines 1337, 1495, 1503 clone in the update loop - restructure to avoid.

---

Major Issues

Performance: Streaming Aggregate State

The compare_int_float function (lines 403-449) is well-implemented but expensive for mixed-type aggregations. Document the trade-off.

Code Duplication

eval_expr_for_record_streaming duplicates ~80% of eval_expr_with_row logic. Consider extracting common helper.

Pointer-as-Key Pattern

Using AST node address as HashMap key (lines 448, 939, 1117) is safe within scope but fragile. Consider sequential ID or hash instead.

---

Code Quality

ScalarSubqueryResults Type Change

HashMap allocates on creation vs SmallVec. Given most queries have 0-2 subqueries, consider reverting to SmallVec for better memory efficiency.

Function Complexity

execute_scalar_subquery_for_update is 318 lines. Consider splitting into table_scan and index_scan variants.

---

Positives

✅ Excellent documentation (lines 86-120)
✅ Good error messages with context
✅ Zero-copy cursor-based streaming for aggregates
✅ Comprehensive test coverage with proper expected values
✅ Pre-computation of subqueries (not per-row)

---

Verdict

Must Fix: Allocation violations (items 1-5 above). These could 10x memory usage for bulk updates.

Should Fix: SmallVec sizing rationale, edge case tests, complete arithmetic refactoring

Nice to Have: Extract expression evaluator, split large function, add correlated subquery validation

Recommendation: Request changes for allocations. Once fixed, excellent addition!

The streaming aggregate optimization is exactly right for an embedded database. Well done!

[claude]

Code Review: Scalar Subquery Support in UPDATE SET Clause

Summary

This PR adds scalar subquery support to UPDATE statements with a streaming aggregate optimization to minimize memory allocations. The implementation is well-documented and generally follows TurDB's coding standards. However, there are several allocation violations and potential improvements to address.

---

Critical Issues (Zero-Allocation Violations)

1. Hidden Allocation in StreamingAggregateState::update (update.rs:469-486)

While using map_or correctly, the repeated updates in a hot loop create Option churn. Consider tracking min/max with separate boolean flags instead of Option to eliminate wrapper allocation overhead in the hot path.

2. String Allocation (update.rs:374)

to_string() allocates when converting AST literals. Low priority if literals are rare in aggregate expressions.

3. Column Info HashMap (update.rs:624-629)

to_lowercase() allocates for each column name. Done once per subquery, acceptable but consider optimization if it becomes a bottleneck.

---

Major Issues

5. Missing Error Context in eval_expr_for_record_streaming (update.rs:368)

VIOLATION: unwrap_or() swallows errors. Per ERRORS.md, should propagate with proper context. Change return type to Result and use wrap_err_with.

6. Arithmetic Overflow Risk in compare_int_float (update.rs:427)

Casting f64 to i64 when f is outside i64 range causes undefined behavior. Add explicit bounds checking before cast.

7. Integer Overflow - GOOD (update.rs:472)

Already using saturating_add - correct per MEMORY.md ✅

---

Code Quality Issues

8. Duplicate Logic

find_expression_aggregate and find_any_aggregate have 90% identical code. Extract shared helper function.

9. Nested Match Arms

StreamingAggregateState::finalize Min/Max cases have deeply nested matches. Extract to helper methods.

10. plan_has_filter Inconsistency

Function checks FilterExec but misses inline filters. Handled at call site but function should be consistent.

---

Positive Highlights

1. ✅ Excellent use of Cow to avoid allocations in convert.rs
2. ✅ Proper use of SmallVec to eliminate heap allocations in PK lookup path
3. ✅ Comprehensive module documentation following STYLE.md
4. ✅ Good error context with wrap_err_with throughout most code
5. ✅ Smart use of saturating_add to prevent integer overflow
6. ✅ Test coverage for main use cases

---

Recommendations

High Priority

1. Fix error handling in eval_expr_for_record_streaming
2. Add bounds checking in compare_int_float f64→i64 cast
3. Eliminate Option churn in StreamingAggregateState hot path

Medium Priority

4. Deduplicate find_expression_aggregate and find_any_aggregate
5. Add missing edge case tests (NULL values, division by zero, NaN/Infinity)
6. Extract nested match logic from finalize methods

---

Verdict

This is solid work that adds important functionality while respecting TurDB's zero-allocation principles. The streaming optimization is clever and well-implemented. Address the critical error handling issues and the Option churn, and this will be ready to merge.

Great job on the documentation and test coverage! 🎉