Skip to content

Commit 7ed3ad4

Browse files
committed
chore(doc): update aggregate functions section with implementation details and lessons learned
1 parent a96afb5 commit 7ed3ad4

File tree

1 file changed

+8
-48
lines changed

1 file changed

+8
-48
lines changed

CLAUDE.md

Lines changed: 8 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -286,59 +286,19 @@ db.close();
286286

287287
**Always verify staged changes before committing** by running `git diff --cached` or `git status -v`.
288288

289-
### Aggregate Functions and V8 HandleScope
289+
### Aggregate Functions Implementation
290290

291-
**Problem**: When implementing SQLite aggregate functions, we encountered "Invalid argument" errors that were caused by V8 HandleScope lifetime issues.
291+
**Status**: ✅ Fully implemented and tested (21 tests passing)
292292

293-
**Root Cause**: SQLite aggregate callbacks (`xStep`, `xFinal`) are called from SQLite's context, not directly from JavaScript. Creating a HandleScope in helper methods like `GetStartValue()` or `SqliteValueToJS()` caused the scope to be destroyed before the JavaScript values were used, resulting in values becoming `<the_hole_value>`.
293+
Aggregate functions presented unique challenges due to N-API constraints in SQLite callback contexts. The implementation is now complete with full type support and comprehensive error handling.
294294

295-
**Solution**:
295+
**Key Implementation Details**:
296296

297-
1. Don't create HandleScope in methods that return JavaScript values - let the caller manage the scope
298-
2. Store aggregate values as raw C++ data instead of JavaScript objects to avoid cross-context issues
299-
3. Use placement new for proper C++ object initialization in SQLite-allocated memory
297+
1. **N-API Constraints**: Cannot use `Napi::Reference` from SQLite callbacks - must use immediate value conversion
298+
2. **Memory Management**: Store only POD types in SQLite's aggregate context, use JSON serialization for complex objects
299+
3. **Error Handling**: Proper early returns after errors (fixed missing return bug from Node.js source)
300300

301-
**Key Code Pattern**:
302-
303-
```cpp
304-
// DON'T do this:
305-
Napi::Value GetStartValue() {
306-
Napi::HandleScope scope(env_); // This scope will be destroyed before value is used!
307-
return Napi::Number::New(env_, 0);
308-
}
309-
310-
// DO this instead:
311-
Napi::Value GetStartValue() {
312-
// No HandleScope - let the caller manage it
313-
return Napi::Number::New(env_, 0);
314-
}
315-
```
316-
317-
### Aggregate Function Argument Count
318-
319-
**Problem**: SQLite determines the number of arguments for aggregate functions based on the JavaScript function's `length` property.
320-
321-
**Key Behavior**:
322-
323-
- For a step function `(acc) => acc + 1`, length is 1, so SQLite expects 0 SQL arguments
324-
- For a step function `(acc, value) => acc + value`, length is 2, so SQLite expects 1 SQL argument
325-
- The first parameter is always the accumulator, additional parameters map to SQL arguments
326-
327-
**Example**:
328-
329-
```javascript
330-
// This expects my_count() with no arguments
331-
db.aggregate("my_count", {
332-
start: 0,
333-
step: (acc) => acc + 1,
334-
});
335-
336-
// This expects my_sum(value) with one argument
337-
db.aggregate("my_sum", {
338-
start: 0,
339-
step: (acc, value) => acc + value,
340-
});
341-
```
301+
For detailed implementation history and lessons learned, see: `doc/archive/aggregate-function-implementation-summary.md`
342302

343303
### Async Cleanup Anti-Patterns
344304

0 commit comments

Comments
 (0)