fix(ext/node): implement DatabaseSync.aggregate()
#31461
Conversation
WalkthroughAdds V8-backed SQLite aggregate and window-function support. Introduces Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
ext/node/ops/sqlite/database.rs (1)
1537-1544: Window function cleanup logic in xfinal.When
is_final=trueandis_window=true, the code drops the value and returns early without calling the result function. This is correct for window functions wherexvaluehas already emitted intermediate results andxfinalis just cleanup. However, consider adding a brief comment explaining this behavior.if !is_final { agg.is_window = true; } else if agg.is_window { + // For window functions, xfinal is only called for cleanup. + // Intermediate results were already emitted via xvalue. if let Some(value_ptr) = agg.value.take() { let _ = v8::Global::from_raw(tc_scope, value_ptr); } return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ext/node/ops/sqlite/database.rs(3 hunks)ext/node/ops/sqlite/validators.rs(1 hunks)tests/unit_node/sqlite_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/node/ops/sqlite/validators.rsext/node/ops/sqlite/database.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit_node/sqlite_test.ts
🧬 Code graph analysis (1)
ext/node/ops/sqlite/database.rs (1)
ext/node/ops/sqlite/validators.rs (1)
name_str(78-89)
⏰ 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). (10)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release macos-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: build libs
🔇 Additional comments (13)
ext/node/ops/sqlite/validators.rs (1)
78-89: LGTM!The
name_strvalidator follows the established pattern of other validators in this file and provides consistent error messaging for the "name" argument validation.tests/unit_node/sqlite_test.ts (3)
496-557: Good test coverage for input validation.The input validation tests comprehensively cover all the options fields (
start,step,useBigIntArguments,varargs,directOnly) with proper error code and message verification.
706-754: Good coverage for start option variations and non-primitive accumulator values.Tests properly verify that
startworks as both a value and a function, and that the accumulator can hold complex JS values like arrays.
813-839: Good test for window function inverse requirement.This test correctly verifies that attempting to use an aggregate as a window function without providing
inverseresults in the expected SQLite error.ext/node/ops/sqlite/database.rs (9)
174-338: Option parsing implementation looks correct.The
from_valuemethod follows the same pattern asApplyChangesetOptions::from_valuein this file, with proper validation for each field type.
1232-1259: argc calculation logic is correct.The code correctly handles the varargs case (-1) and properly calculates argc by taking the max of step and inverse function lengths minus 1 (to exclude the accumulator argument).
1344-1349: AggregateData struct relies on zero-initialization.SQLite's
sqlite3_aggregate_contextzero-initializes memory on first allocation, which meansinitializedwill befalseandvaluewill be a null pointer (treated asNone) initially. This is the expected behavior.
1384-1400: Start function error handling looks correct.When the start function throws, the code properly sets
ignore_next_sqlite_error, callssqlite_result_error, and returnsNone. The caller at line 1431-1434 checks forNoneand callsrethrow().
1462-1469: Correct handling of Global handle to avoid double-free.The code at line 1465 uses
std::mem::forget(global)to prevent dropping the Global handle after creating a Local from it, since the Global is still owned byagg.value. This is the correct pattern.
1626-1647: LGTM!The destroy callback properly reclaims the
Box<CustomAggregate>and drops all storedv8::Globalhandles. The per-invocationAggregateData.valueis cleaned up separately inxfinal.
1390-1396: Consider edge case: repeated start failures in xfinal.If the
startfunction throws an error duringxstep, theaggremains uninitialized. When SQLite subsequently callsxfinalfor cleanup,get_aggregate_datawill attempt to callstartagain, which may throw again. While memory is still properly managed (SQLite handles AggregateData, xdestroy handles CustomAggregate), this results in potentially repeated error throwing.This matches Node.js behavior, so it's acceptable, but worth being aware of.
1586-1624: LGTM!The C callback wrappers are clean thin wrappers around the base implementation functions, properly marked
unsafe extern "C"with appropriate safety comments.
1217-1341: Overall aggregate implementation looks solid.The implementation correctly:
- Validates options and extracts v8 function handles
- Calculates argc based on varargs and function lengths
- Registers the aggregate with SQLite using
sqlite3_create_window_function- Conditionally provides xvalue/xinverse callbacks only when inverse is defined
- Properly manages v8::Global handles lifecycle
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit_node/sqlite_test.ts (1)
496-839: Comprehensive test coverage for DatabaseSync.aggregate() looks solid.The test suite thoroughly covers validation, varargs behavior, directOnly flag, error propagation, and window function scenarios. All assertions appear correct, and the tests properly exercise the new aggregate API.
One minor note on lines 559-576: The varargs test's step function ignores the accumulator and just returns
var1 + var2 + var3each time. This works for testing that varargs are accepted, but doesn't demonstrate actual accumulation. Consider adding a comment clarifying this is intentionally testing varargs support rather than aggregation logic, or using a step that actually accumulates:step: (acc, _value, var1, var2, var3) => acc + var1 + var2 + var3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_node/sqlite_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit_node/sqlite_test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ext/node/ops/sqlite/database.rs (2)
192-214: Minor: Store the object reference once to reduce repetition.After the
is_object()check passes, you convertvaluetov8::Objectmultiple times via.try_from(value).unwrap(). Consider storing it once:if !value.is_object() { return Err(Error::InvalidArgType( "The \"options\" argument must be an object.", )); } + let obj = v8::Local::<v8::Object>::try_from(value).unwrap();Then use
objthroughout instead of repeating the conversion. This is purely stylistic.
1245-1258: Inconsistent handling of zero-length functions.If
step_fn_lengthis 0 (function with no declared params),argcbecomes -1, which SQLite interprets as varargs. Meanwhile, forinverse_fn_length, you explicitly guard withcmp::max(..., 0).This asymmetry may cause unexpected varargs behavior when the step function happens to have 0 declared parameters. Consider applying consistent handling:
- argc = step_fn_length - 1; + argc = cmp::max(step_fn_length - 1, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/ops/sqlite/database.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/node/ops/sqlite/database.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). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (2)
ext/node/ops/sqlite/database.rs (2)
1503-1580: LGTM on window function finalization logic.The ownership handling for
v8::Globalvalues and the window function cleanup semantics (early return onis_final && is_window) look correct.
1622-1643: LGTM on cleanup.The
custom_aggregate_xdestroycorrectly reclaims theBox<CustomAggregate>and releases allv8::Globalhandles.
Tango992
force-pushed
the
fix-node-sqlite-aggregate-function
branch
from
7825a66 to
848c8dc
Compare
Tango992
force-pushed
the
fix-node-sqlite-aggregate-function
branch
from
848c8dc to
159b469
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/node/ops/sqlite/database.rs (1)
174-338: AggregateFunctionOption parsing/validation looks solid; only minor cleanup opportunityThe option parsing covers required fields and validates all boolean flags with clear error messages, and the
start/step/result/inversehandling matches the intended JS API. If you want to trim repetition, you could bindvalueonce as an object (e.g.let obj = v8::Local::<v8::Object>::try_from(value).unwrap();) and reuseobjfor all subsequentgetcalls instead of repeatedly callingtry_from(value).unwrap(), but this is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/ops/sqlite/database.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/node/ops/sqlite/database.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). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (3)
ext/node/ops/sqlite/database.rs (3)
28-28: New sqlite3_create_window_function FFI import matches later usageThe direct import is consistent with other SQLite FFI usage and cleanly supports the new aggregate/window-function registration below; no changes needed.
1218-1341: DatabaseSync::aggregate correctly wires options into sqlite3_create_window_functionArg-count computation (using
step.length - 1andinverse.length - 1with a 0 lower bound) and thevarargs => argc = -1behavior look correct, and the text-representation flags (UTF8, deterministic, directOnly) mirror the scalarfunction()path. The V8 globals for context/start/step/inverse/result are all owned viaCustomAggregateand released incustom_aggregate_xdestroy, so I don’t see lifetime or registration issues here.
1344-1663: AggregateData / CustomAggregate callbacks handle context, errors, and GC safelyThe aggregate helpers and callbacks are carefully structured:
sqlite3_aggregate_contextandsqlite3_user_dataare null-checked,get_aggregate_datainitializes state once viastart(function or value),custom_aggregate_step_baseandcustom_aggregate_value_baseconsistently convert arguments/results and setignore_next_sqlite_errorwhen JS code fails, and theis_windowflag ensuresresultisn’t re-run when SQLite invokesxFinalafter window use. TheGlobal::from_raw/into_rawusage avoids leaks and double-frees under normal completion, andcustom_aggregate_xdestroymirrors the existing scalar UDF cleanup pattern. I don’t see correctness or safety problems in this block.
|
I skimmed through it and I think it's better this waits until @littledivy can review it next week. |
2 participants
Implementation is based on https://github.com/nodejs/node/blob/v24.2.0/src/node_sqlite.cc
It passes all tests from https://github.com/nodejs/node/blob/v24.2.0/test/parallel/test-sqlite-aggregate-function.mjs except for two test cases where it uses
mock()fromnode:test, which we don't yet implement. The tests onsqlite_test.tsare copied from the node compat test cases.