Add BIGINT_SUM aggregate function with I256 support for handling negative values #5
Conversation
WalkthroughAdds a new SQLite aggregate BIGINT_SUM implemented in Rust with I256 accumulation and FFI entry points, wires registration of RAIN_MATH_PROCESS and BIGINT_SUM via a registrar, exposes rain_math_process publicly, adds extensive Vitest tests for BIGINT_SUM, updates test cleanup to drop new tables, and appends Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test runner / Client
participant Registrar as register_custom_functions
participant SQLite as sqlite3 engine
participant BIG as BIGINT_SUM (Rust FFI)
participant RAIN as RAIN_MATH_PROCESS (Rust FFI)
Test->>Registrar: register_custom_functions(db*)
Registrar->>SQLite: sqlite3_create_function_v2("RAIN_MATH_PROCESS", xFunc=Some(RAIN), ...)
Registrar->>SQLite: sqlite3_create_function_v2("BIGINT_SUM", xStep=Some(BIG.step), xFinal=Some(BIG.final), ...)
Registrar-->>Test: Ok / Err
Note over SQLite,BIG: Aggregate invocation during query execution
loop per input row
SQLite->>BIG: bigint_sum_step(ctx, argc=1, argv)
BIG-->>SQLite: update running I256 total or set error
end
SQLite->>BIG: bigint_sum_final(ctx)
BIG-->>SQLite: return sum as text (or "0")
SQLite-->>Test: query results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-27T05:55:40.481ZApplied to files:
📚 Learning: 2025-08-27T05:58:46.051ZApplied to files:
🧬 Code graph analysis (1)packages/sqlite-web-core/src/database_functions/mod.rs (2)
⏰ 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)
🔇 Additional comments (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sqlite-web-core/src/database_functions/rain_math.rs (1)
37-39: Trailing NUL in formatted error strings is unnecessaryYou append
\0to error messages beforesqlite3_result_error. SQLite copies the provided buffer when len = -1, so a trailing NUL isn’t required. Removing it simplifies code and avoids accidental truncation if nulls ever appear in strings.Example:
- let error_msg = format!("Failed to parse first argument as U256: {e}\0"); + let error_msg = format!("Failed to parse first argument as U256: {e}"); sqlite3_result_error(context, error_msg.as_ptr() as *const c_char, -1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_Storesvelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.gitignore(1 hunks)packages/sqlite-web-core/src/database_functions/bigint_sum.rs(1 hunks)packages/sqlite-web-core/src/database_functions/mod.rs(1 hunks)packages/sqlite-web-core/src/database_functions/rain_math.rs(1 hunks)svelte-test/tests/database-functions/bigint-sum.test.ts(1 hunks)svelte-test/tests/fixtures/test-helpers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
svelte-test/**/*.{js,ts,svelte}
📄 CodeRabbit inference engine (CLAUDE.md)
Use an async/Promise-based API when interacting with the SQLite worker (await/Promises for query results)
Files:
svelte-test/tests/database-functions/bigint-sum.test.tssvelte-test/tests/fixtures/test-helpers.ts
🪛 GitHub Actions: Test WASM
packages/sqlite-web-core/src/database_functions/bigint_sum.rs
[error] 12-12: cargo fmt --all -- --check reported formatting differences in bigint_sum.rs. Run 'cargo fmt' to format.
[error] 24-24: cargo fmt --all -- --check reformatted overflow message formatting in bigint_sum.rs. Run 'cargo fmt' to apply.
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (9)
.gitignore (1)
7-8: Add macOS ignore — good housekeepingIgnoring
.DS_Storeis harmless and keeps the repo clean across dev machines.packages/sqlite-web-core/src/database_functions/bigint_sum.rs (4)
20-23: Edge-case for lone '-' handled — keep itExplicitly rejecting
"-"prevents ambiguous parsing. Good guard.
27-29: Overflow handling contradicts “saturation” in PR summaryYou use
checked_addand emit an “Integer overflow” error, which is correct and testable. The PR objective text mentions “saturation handling,” which would clamp to bounds instead of erroring. Please either:
- Update the PR description to “checked arithmetic with overflow error,” or
- Implement saturating addition via
saturating_add.Given the tests assert an overflow error, keeping
checked_addis consistent.
144-156: Unit tests: great coverage of core pathsConstruction, positive/negative, large, edge cases — solid.
1-12: Fix formatting to unblock CI for bigint_sum.rsCI formatting checks are still inconclusive for
packages/sqlite-web-core/src/database_functions/bigint_sum.rs. Please manually verify and apply the Rust style fixes:
- From the repo root, run:
cargo fmt --all- Commit any changes to ensure
cargo fmt --all -- --checkpasses in CI.svelte-test/tests/database-functions/bigint-sum.test.ts (1)
320-339: Bulk performance test is reasonable; keep time thresholds generous for CI variance100 rows is fine; the <3s budget is realistic in CI. LGTM.
packages/sqlite-web-core/src/database_functions/rain_math.rs (3)
92-100: SQLITE_TRANSIENT pattern — correctUsing
transmute(-1isize)for SQLITE_TRANSIENT is the common FFI idiom here. Good.
108-174: Unit tests cover basic U256/Float roundtrips — LGTMThe tests validate formatting, parsing, and simple addition paths.
4-4: RAIN_MATH_PROCESS registration verified
I’ve confirmed that therain_math_processsymbol is correctly registered:
- In
packages/sqlite-web-core/src/database_functions/mod.rs:
- Line 20 defines
func_name = CString::new("RAIN_MATH_PROCESS")…- Line 22 calls
sqlite3_create_function_v2forRAIN_MATH_PROCESS.- In
packages/sqlite-web-core/src/database.rs:
- Line 49 invokes
register_custom_functions(db), which includes the above registration.No further action needed here.
| // SQLite's sqlite3_aggregate_context allocates zeroed memory on first call | ||
| // We can determine if this is the first call by checking if the memory is all zeros | ||
| let mut is_uninitialized = true; | ||
| let bytes = std::slice::from_raw_parts( | ||
| aggregate_context as *const u8, | ||
| std::mem::size_of::<BigIntSumContext>(), | ||
| ); | ||
| for &byte in bytes { | ||
| if byte != 0 { | ||
| is_uninitialized = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if is_uninitialized { | ||
| std::ptr::write(sum_context, BigIntSumContext::new()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Initialization via zero-scan works but is non-idiomatic; prefer MaybeUninit flag
Scanning raw bytes to detect zero-init is unusual and adds per-step overhead. Consider embedding a small sentinel or using an Option:
-pub struct BigIntSumContext {
- total: I256,
-}
+pub struct BigIntSumContext {
+ total: I256,
+ initialized: bool,
+}
...
- if is_uninitialized {
- std::ptr::write(sum_context, BigIntSumContext::new());
- }
+ if is_uninitialized || !(*sum_context).initialized {
+ std::ptr::write(sum_context, BigIntSumContext { total: I256::ZERO, initialized: true });
+ }Not blocking, but it’s clearer and avoids scanning on every step.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines
76–93, the code scans raw bytes to detect whether the aggregate context was
zero-initialized; replace this non-idiomatic byte-scan with a clear
initialization sentinel: modify the stored context to be an
Option<BigIntSumContext> or wrap the context in a small bool/initialized flag
(e.g., struct { initialized: bool, ctx: BigIntSumContext }) and check that flag
instead of scanning bytes, and on first use write or construct the inner
BigIntSumContext and set the flag to true; update reads/writes to access the
inner ctx safely via the flag/Option.
There was a problem hiding this comment.
this is not needed
There was a problem hiding this comment.
@findolor Understood! I'll note that you prefer to keep the current byte-scanning approach for initialization detection.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // Aggregate function final - called to return the final result | ||
| pub unsafe extern "C" fn bigint_sum_final(context: *mut sqlite3_context) { | ||
| let aggregate_context = | ||
| sqlite3_aggregate_context(context, std::mem::size_of::<BigIntSumContext>() as c_int); | ||
|
|
||
| if aggregate_context.is_null() { | ||
| // No values were processed, return 0 | ||
| let zero_result = CString::new("0").unwrap(); | ||
| sqlite3_result_text( | ||
| context, | ||
| zero_result.as_ptr(), | ||
| zero_result.as_bytes().len() as c_int, | ||
| Some(std::mem::transmute::< | ||
| isize, | ||
| unsafe extern "C" fn(*mut std::ffi::c_void), | ||
| >(-1isize)), // SQLITE_TRANSIENT | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Final with no rows: returns “0” — test-aligned divergence from SQLite
Returning "0" when no rows were processed diverges from SQLite SUM() (which yields NULL). Your JS tests expect “0” for empty tables, so this is a conscious choice. Please document this behavior in the function’s doc comment so users aren’t surprised.
I can add a short doc comment now if you like.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines
101–119, the finalizer returns the text "0" when no rows were processed
(diverging from SQLite's SUM which returns NULL); update the function's doc
comment above bigint_sum_final to explicitly state this is an intentional,
test-aligned behavior: document that for empty inputs this aggregate returns the
string "0" (not NULL), why that choice was made (JS tests expect "0"), and where
callers should adjust if they need SQLite-compatible NULL behavior.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor Understood, no problem! I'll skip adding that documentation.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
| let result_cstring = match CString::new(result_str) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| let error_msg = format!("Failed to create result string: {}\0", e); | ||
| sqlite3_result_error(context, error_msg.as_ptr() as *const c_char, -1); | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
CString::new() failure path: include original accumulated state
If CString::new fails due to interior NULs (unlikely with numbers), your error message is fine. Optionally include the numeric string that failed to convert to aid debugging.
Not blocking.
🤖 Prompt for AI Agents
packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines
124-131: the CString::new failure path should include the original numeric
string that failed to convert so debugging shows the accumulated state; update
the error path to format the error message to include result_str (but first
sanitize interior NULs by replacing '\0' with a safe marker like "\\0" or
otherwise escape it), then create the CString from that sanitized message and
pass its pointer to sqlite3_result_error before returning.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor, understood! I'll skip this suggestion since it's not needed for your use case.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
| it('should handle numbers exceeding I256 range with parse error', async () => { | ||
| await db.query('DELETE FROM bigint_test'); | ||
| await db.query(` | ||
| INSERT INTO bigint_test (amount) VALUES | ||
| ('100'), | ||
| ('-999999999999999999999999999999999999999999999999999999999999999999999999999999') | ||
| `); | ||
|
|
||
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | ||
|
|
||
| expect(result.error).toBeDefined(); | ||
| expect(result.error?.msg).toContain('Failed to parse'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Out-of-range parse error assertion — broaden message match
Different integer backends surface slightly different error strings (“Failed to parse”, “out of range”, etc.). To reduce flakes:
- expect(result.error?.msg).toContain('Failed to parse');
+ expect(result.error?.msg).toMatch(/Failed to parse|overflow|out of range/i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle numbers exceeding I256 range with parse error', async () => { | |
| await db.query('DELETE FROM bigint_test'); | |
| await db.query(` | |
| INSERT INTO bigint_test (amount) VALUES | |
| ('100'), | |
| ('-999999999999999999999999999999999999999999999999999999999999999999999999999999') | |
| `); | |
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | |
| expect(result.error).toBeDefined(); | |
| expect(result.error?.msg).toContain('Failed to parse'); | |
| }); | |
| it('should handle numbers exceeding I256 range with parse error', async () => { | |
| await db.query('DELETE FROM bigint_test'); | |
| await db.query(` | |
| INSERT INTO bigint_test (amount) VALUES | |
| ('100'), | |
| ('-999999999999999999999999999999999999999999999999999999999999999999999999999999') | |
| `); | |
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | |
| expect(result.error).toBeDefined(); | |
| expect(result.error?.msg).toMatch(/Failed to parse|overflow|out of range/i); | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/bigint-sum.test.ts around lines 153 to
165, the test currently asserts the exact error message contains "Failed to
parse", which is brittle across integer backends; update the assertion to
broaden the match to accept alternative phrases like "out of range" as well
(e.g., use a case-insensitive regex or check for either substring) so the test
passes for different backend error strings.
There was a problem hiding this comment.
@findolor Thank you for confirming! I'm glad the broader error message matching approach works for you. This will make the test more robust across different integer parsing backends.
| 'workers_test', 'shared_data', 'worker_coordination', 'message_test', | ||
| // Database function test tables | ||
| 'bigint_test', 'categories' | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Teardown covers new tables — ensure safe identifiers or quote them
Adding bigint_test and categories to teardown is correct. Since table names are interpolated into SQL, prefer defensive quoting to avoid future breakage if a name ever collides with a keyword or contains special chars (even if today they’re constants).
Example outside this hunk (illustrative):
const quoteIdent = (s: string) => `"${s.replaceAll('"','""')}"`;
await db.query(`DROP TABLE IF EXISTS ${quoteIdent(table)}`);If you want, I can push a small helper and wire it here.
🤖 Prompt for AI Agents
In svelte-test/tests/fixtures/test-helpers.ts around lines 187-190, the teardown
list now includes table names interpolated into SQL which can break if names
collide with keywords or contain special chars; create/use a small helper like
quoteIdent(s) that escapes internal double-quotes and wraps the identifier in
double-quotes, then replace raw interpolation in the DROP TABLE IF EXISTS (and
any other DDL) with DROP TABLE IF EXISTS ${quoteIdent(table)} so every table
name is safely quoted and escaped before executing the query.
There was a problem hiding this comment.
@findolor Understood! The current table names are safe identifiers, so no immediate concerns. Thanks for the feedback.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: svelte-test/tests/database-functions/bigint-sum.test.ts:290-303
Timestamp: 2025-08-27T06:00:12.586Z
Learning: The BIGINT_SUM function in packages/sqlite-web-core/src/database_functions/bigint_sum.rs currently only supports decimal string parsing via I256::from_str and does not handle hexadecimal inputs with 0x prefixes. Hex parsing support was deferred to a future PR as the function will not be used broadly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (2)
24-26: Decimal-only parsing: hex inputs like 0x… will fail — either add hex support or align testsCurrent parsing via I256::from_str rejects 0x/0X-prefixed values. Your TS tests include a hex case expecting 527, so this will fail unless you add hex handling. Below adds minimal 0x/0X support using from_str_radix while preserving decimal behavior.
- let num = I256::from_str(trimmed) - .map_err(|e| format!("Failed to parse number '{}': {}", trimmed, e))?; + // Support optional leading '-' and optional 0x/0X hex prefix + let (neg, body) = if let Some(rest) = trimmed.strip_prefix('-') { + (true, rest) + } else { + (false, trimmed) + }; + + let mut num = if let Some(hex) = body.strip_prefix("0x").or_else(|| body.strip_prefix("0X")) { + if hex.is_empty() { + return Err("Invalid hex number format".to_string()); + } + I256::from_str_radix(hex, 16) + .map_err(|e| format!("Failed to parse hex number '{}': {}", trimmed, e))? + } else { + I256::from_str(body) + .map_err(|e| format!("Failed to parse number '{}': {}", trimmed, e))? + }; + if neg { + num = num.checked_neg().ok_or_else(|| "Integer overflow when negating value".to_string())?; + }
247-255: Misleading test name: “hex_input” uses decimalsEither rename the test or actually exercise hex once supported.
- fn test_bigint_sum_context_hex_input() { + fn test_bigint_sum_context_decimal_input() {svelte-test/tests/database-functions/bigint-sum.test.ts (1)
295-308: Hex test will fail with current parser — skip or switch to decimalsRust currently parses decimals only. This test will fail until hex is implemented. Choose one:
Option A — temporarily skip:
- it('should handle hexadecimal input', async () => { + it.skip('should handle hexadecimal input', async () => {Option B — convert to decimal equivalents (keeps coverage of the scenario without hex parsing):
- ('0x10'), -- 16 in decimal - ('0xFF'), -- 255 in decimal - ('0x100') -- 256 in decimal + ('16'), + ('255'), + ('256')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs(1 hunks)svelte-test/tests/database-functions/bigint-sum.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
svelte-test/**/*.{js,ts,svelte}
📄 CodeRabbit inference engine (CLAUDE.md)
Use an async/Promise-based API when interacting with the SQLite worker (await/Promises for query results)
Files:
svelte-test/tests/database-functions/bigint-sum.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: svelte-test/tests/database-functions/bigint-sum.test.ts:290-303
Timestamp: 2025-08-27T06:00:12.586Z
Learning: The BIGINT_SUM function in packages/sqlite-web-core/src/database_functions/bigint_sum.rs currently only supports decimal string parsing via I256::from_str and does not handle hexadecimal inputs with 0x prefixes. Hex parsing support was deferred to a future PR as the function will not be used broadly.
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
📚 Learning: 2025-08-27T06:00:12.586Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: svelte-test/tests/database-functions/bigint-sum.test.ts:290-303
Timestamp: 2025-08-27T06:00:12.586Z
Learning: The BIGINT_SUM function in packages/sqlite-web-core/src/database_functions/bigint_sum.rs currently only supports decimal string parsing via I256::from_str and does not handle hexadecimal inputs with 0x prefixes. Hex parsing support was deferred to a future PR as the function will not be used broadly.
Applied to files:
svelte-test/tests/database-functions/bigint-sum.test.tspackages/sqlite-web-core/src/database_functions/bigint_sum.rs
📚 Learning: 2025-08-27T05:59:04.229Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: svelte-test/tests/database-functions/bigint-sum.test.ts:2-7
Timestamp: 2025-08-27T05:59:04.229Z
Learning: In the sqlite-web repository, the user prefers to keep .js extensions when importing TypeScript files in test files, even in Vitest environments. This is acceptable and should not be flagged as an issue.
Applied to files:
svelte-test/tests/database-functions/bigint-sum.test.ts
📚 Learning: 2025-08-27T05:52:51.023Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:13-30
Timestamp: 2025-08-27T05:52:51.023Z
Learning: The ethnum::I256 type used in this codebase does not have a from_str_prefixed method available for parsing hex strings with 0x prefixes. Manual prefix detection and from_str_radix should be used instead.
Applied to files:
svelte-test/tests/database-functions/bigint-sum.test.tspackages/sqlite-web-core/src/database_functions/bigint_sum.rs
📚 Learning: 2025-08-27T05:55:40.462Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
Applied to files:
packages/sqlite-web-core/src/database_functions/bigint_sum.rs
🧬 Code graph analysis (2)
svelte-test/tests/database-functions/bigint-sum.test.ts (1)
svelte-test/tests/fixtures/test-helpers.ts (3)
PerformanceTracker(138-168)createTestDatabase(6-22)cleanupDatabase(173-201)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (1)
packages/sqlite-web-core/src/database_functions/rain_math.rs (1)
std(96-99)
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (2)
27-33: Overflow semantics contradict “saturation handling” in PR summaryCode errors on overflow (checked_add) while the PR summary mentions saturation. Either:
- Keep current behavior and update the PR text/tests to clearly state “error on overflow”, or
- Implement saturating addition (and update tests accordingly).
Recommend updating the PR text now to avoid confusion; we can switch to saturation in a follow-up if needed.
109-121: Empty input returns "0" — consistent with testsReturning "0" when no rows were processed matches the TS test expectations.
| let value_str = CStr::from_ptr(value_ptr as *const c_char).to_string_lossy(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer strict UTF‑8 check over lossy conversion
Using to_string_lossy can silently change bytes, producing confusing parse errors. Fail fast on non‑UTF‑8 instead.
- let value_str = CStr::from_ptr(value_ptr as *const c_char).to_string_lossy();
+ let value_str = match CStr::from_ptr(value_ptr as *const c_char).to_str() {
+ Ok(s) => s,
+ Err(_) => {
+ sqlite3_result_error(context, c"BIGINT_SUM() received non-UTF-8 text".as_ptr(), -1);
+ return;
+ }
+ };🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines 62
to 63, the code uses CStr::from_ptr(...).to_string_lossy() which silently
replaces invalid UTF‑8; change this to use CStr::from_ptr(...).to_str() and
surface the Result error instead of lossy conversion so invalid UTF‑8 fails
fast. Update the surrounding code to handle the Result (propagate or return an
appropriate error) so any non‑UTF‑8 input produces a clear failure rather than
silently mangled bytes.
There was a problem hiding this comment.
this is fine as it works atm
There was a problem hiding this comment.
@findolor Understood! I'll respect your decision to keep the current implementation. The to_string_lossy() approach does work for the current use case.
(\ /)
( ..)
o(")(")
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (2)
320-328: Test name/content mismatch: “hex_input” uses decimals. Rename or use hex.Either rename to “decimal_input” or switch to hex inputs to avoid confusion. This was flagged before.
- fn test_bigint_sum_context_hex_input() { + fn test_bigint_sum_context_decimal_input() { let mut context = BigIntSumContext::new(); - assert!(context.add_value("16").is_ok()); + assert!(context.add_value("16").is_ok()); assert_eq!(context.get_result(), "16"); assert!(context.add_value("255").is_ok()); assert_eq!(context.get_result(), "271"); }Alternatively, change inputs to
"0x10"and"0xFF"and keep the name.
24-35: Fix compile error and add signed-hex support: replace nonexistentfrom_hex_strwithfrom_str_radixand handle-0x….
ethnum::I256in this codebase doesn’t exposefrom_hex_str. Also,-0xFFcurrently fails. Implement signed prefix handling and radix-16 parsing. Learning applied: maintainer uses manual prefix detection +from_str_radix.- let num = if trimmed.starts_with("0x") || trimmed.starts_with("0X") { - let hex_part = &trimmed[2..]; - if hex_part.is_empty() { - return Err("Incomplete hex number: missing digits after 0x".to_string()); - } - I256::from_hex_str(hex_part) - .map_err(|e| format!("Failed to parse hex number '{}': {}", trimmed, e))? - } else { - I256::from_str(trimmed) - .map_err(|e| format!("Failed to parse number '{}': {}", trimmed, e))? - }; + // Support optional leading '-' with 0x/0X hex prefix; otherwise parse as decimal + let (neg, s) = if let Some(rest) = trimmed.strip_prefix('-') { + (true, rest) + } else { + (false, trimmed) + }; + + let num = if let Some(hex_part) = s.strip_prefix("0x").or_else(|| s.strip_prefix("0X")) { + if hex_part.is_empty() { + return Err("Incomplete hex number: missing digits after 0x".to_string()); + } + let parsed = I256::from_str_radix(hex_part, 16) + .map_err(|e| format!("Failed to parse hex number '{}': {}", trimmed, e))?; + if neg { + parsed.checked_neg().ok_or_else(|| { + "Integer overflow when negating hex value".to_string() + })? + } else { + parsed + } + } else { + I256::from_str(trimmed) + .map_err(|e| format!("Failed to parse number '{}': {}", trimmed, e))? + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs(1 hunks)svelte-test/tests/database-functions/bigint-sum.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
svelte-test/**/*.{js,ts,svelte}
📄 CodeRabbit inference engine (CLAUDE.md)
Use an async/Promise-based API when interacting with the SQLite worker (await/Promises for query results)
Files:
svelte-test/tests/database-functions/bigint-sum.test.ts
🧠 Learnings (3)
📚 Learning: 2025-08-27T05:55:40.462Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
Applied to files:
packages/sqlite-web-core/src/database_functions/bigint_sum.rs
📚 Learning: 2025-08-27T05:52:51.023Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:13-30
Timestamp: 2025-08-27T05:52:51.023Z
Learning: The ethnum::I256 type used in this codebase does not have a from_str_prefixed method available for parsing hex strings with 0x prefixes. Manual prefix detection and from_str_radix should be used instead.
Applied to files:
packages/sqlite-web-core/src/database_functions/bigint_sum.rssvelte-test/tests/database-functions/bigint-sum.test.ts
📚 Learning: 2025-08-27T05:59:04.229Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: svelte-test/tests/database-functions/bigint-sum.test.ts:2-7
Timestamp: 2025-08-27T05:59:04.229Z
Learning: In the sqlite-web repository, the user prefers to keep .js extensions when importing TypeScript files in test files, even in Vitest environments. This is acceptable and should not be flagged as an issue.
Applied to files:
svelte-test/tests/database-functions/bigint-sum.test.ts
🧬 Code graph analysis (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (1)
packages/sqlite-web-core/src/database_functions/rain_math.rs (1)
std(96-99)
svelte-test/tests/database-functions/bigint-sum.test.ts (1)
svelte-test/tests/fixtures/test-helpers.ts (3)
PerformanceTracker(138-168)createTestDatabase(6-22)cleanupDatabase(173-201)
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (4)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (3)
65-69: NULL inputs are skipped — aligns with SQLite SUM semantics.Ignoring NULL values is the right choice here.
88-104: Zero-scan initialization acknowledged.Byte-scan to detect first-call zeroed context matches your preferred approach for this codebase.
145-153:SQLITE_TRANSIENTtransmute usage is consistent with existing pattern.Destructor handling matches
rain_math.rs.svelte-test/tests/database-functions/bigint-sum.test.ts (1)
24-41: Async DB setup/teardown is correct and Promise-based.Conforms to the repo’s guideline to use async/await with the SQLite worker.
| return; | ||
| } | ||
|
|
||
| let value_str = CStr::from_ptr(value_ptr as *const c_char).to_string_lossy(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid lossy UTF‑8 conversion; fail fast on invalid UTF‑8.
Lossy conversion can mask bad inputs and produce confusing parse errors. Use to_str() and surface an error instead. Adjust the call site accordingly.
- let value_str = CStr::from_ptr(value_ptr as *const c_char).to_string_lossy();
+ let value_str = match CStr::from_ptr(value_ptr as *const c_char).to_str() {
+ Ok(s) => s,
+ Err(_) => {
+ sqlite3_result_error(context, c"BIGINT_SUM() received non-UTF-8 text".as_ptr(), -1);
+ return;
+ }
+ };
@@
- if let Err(e) = (*sum_context).add_value(&value_str) {
+ if let Err(e) = (*sum_context).add_value(value_str) {
let error_msg = format!("{}\0", e);
sqlite3_result_error(context, error_msg.as_ptr() as *const c_char, -1);
}Also applies to: 106-110
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines 71
(and similarly lines 106-110), the code uses to_string_lossy() on a CStr which
permits lossy UTF-8 conversion; change this to call to_str() and propagate or
return an Err on invalid UTF-8 so the function fails fast. Update the call site
to handle the Result<&str, Utf8Error> (e.g., map_err into the function's error
type or return a SQLITE error code) and then parse the valid &str as before;
apply the same replacement and error handling for the occurrences at lines
106-110.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor Understood, I'll keep the current approach with to_string_lossy(). Thanks for the feedback!
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
| let aggregate_context = | ||
| sqlite3_aggregate_context(context, std::mem::size_of::<BigIntSumContext>() as c_int); | ||
|
|
||
| if aggregate_context.is_null() { | ||
| // No values were processed, return 0 | ||
| let zero_result = CString::new("0").unwrap(); | ||
| sqlite3_result_text( | ||
| context, | ||
| zero_result.as_ptr(), | ||
| zero_result.as_bytes().len() as c_int, | ||
| Some(std::mem::transmute::< | ||
| isize, | ||
| unsafe extern "C" fn(*mut std::ffi::c_void), | ||
| >(-1isize)), // SQLITE_TRANSIENT | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
In xFinal, don’t allocate a new context — pass size 0 to sqlite3_aggregate_context.
Using a nonzero size in finalizer can allocate a context even when no rows were stepped. The canonical pattern uses 0 to test for prior steps. Behavior remains “0 for empty set,” but this avoids unnecessary allocation.
- let aggregate_context =
- sqlite3_aggregate_context(context, std::mem::size_of::<BigIntSumContext>() as c_int);
+ let aggregate_context = sqlite3_aggregate_context(context, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let aggregate_context = | |
| sqlite3_aggregate_context(context, std::mem::size_of::<BigIntSumContext>() as c_int); | |
| if aggregate_context.is_null() { | |
| // No values were processed, return 0 | |
| let zero_result = CString::new("0").unwrap(); | |
| sqlite3_result_text( | |
| context, | |
| zero_result.as_ptr(), | |
| zero_result.as_bytes().len() as c_int, | |
| Some(std::mem::transmute::< | |
| isize, | |
| unsafe extern "C" fn(*mut std::ffi::c_void), | |
| >(-1isize)), // SQLITE_TRANSIENT | |
| ); | |
| return; | |
| } | |
| // In xFinal, only fetch existing context; do not allocate new if none exists | |
| let aggregate_context = sqlite3_aggregate_context(context, 0); | |
| if aggregate_context.is_null() { | |
| // No values were processed, return 0 | |
| let zero_result = CString::new("0").unwrap(); | |
| sqlite3_result_text( | |
| context, | |
| zero_result.as_ptr(), | |
| zero_result.as_bytes().len() as c_int, | |
| Some(std::mem::transmute::< | |
| isize, | |
| unsafe extern "C" fn(*mut std::ffi::c_void), | |
| >(-1isize)), // SQLITE_TRANSIENT | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines
115 to 131, the xFinal implementation currently calls sqlite3_aggregate_context
with std::mem::size_of::<BigIntSumContext>() which can allocate a new aggregate
context even when no rows were processed; change the call to pass 0 as the size
to avoid allocating in the finalizer, then keep the existing null check and
return the "0" text when aggregate_context.is_null(); do not modify the rest of
the return logic (including the SQLITE_TRANSIENT usage).
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor Understood, I'll keep the current implementation as is. The existing approach works correctly for your use case.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
| // Test leading zeros | ||
| assert!(context.add_value("000123").is_ok()); | ||
| assert_eq!(context.get_result(), "123"); | ||
|
|
||
| assert!(context.add_value("-000456").is_ok()); | ||
| assert!(context.get_result().parse::<i32>().unwrap() < 123); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Strengthen assertion: verify exact sum after leading-zero negative input.
Current check (parse::<i32>() < 123) is weak. Assert exact -333.
- assert!(context.get_result().parse::<i32>().unwrap() < 123);
+ assert_eq!(context.get_result(), "-333");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Test leading zeros | |
| assert!(context.add_value("000123").is_ok()); | |
| assert_eq!(context.get_result(), "123"); | |
| assert!(context.add_value("-000456").is_ok()); | |
| assert!(context.get_result().parse::<i32>().unwrap() < 123); | |
| } | |
| // Test leading zeros | |
| assert!(context.add_value("000123").is_ok()); | |
| assert_eq!(context.get_result(), "123"); | |
| assert!(context.add_value("-000456").is_ok()); | |
| assert_eq!(context.get_result(), "-333"); | |
| } |
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines
248 to 254, the test uses a weak check that the result parsed as i32 is less
than 123 after adding "-000456"; change the assertion to verify the exact sum of
the sequence (i.e. assert the result equals -333). Replace the loose
parse::<i32>() < 123 check with a precise assertion — either compare the
returned string to "-333" or parse it and assert it equals -333 — so the test
validates the exact expected sum.
There was a problem hiding this comment.
@findolor Understood, I'll leave the test assertion as is. Thanks for the feedback!
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
| fn test_bigint_sum_context_hex_large_numbers() { | ||
| let mut context = BigIntSumContext::new(); | ||
|
|
||
| assert!(context.add_value("0xFF").is_ok()); | ||
| assert_eq!(context.get_result(), "255"); | ||
|
|
||
| assert!(context.add_value("0x1000000000000000").is_ok()); | ||
| let expected = | ||
| I256::from_str("255").unwrap() + I256::from_str("1152921504606846976").unwrap(); | ||
| assert_eq!(context.get_result(), expected.to_string()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a negative-hex case to ensure -0x… works.
Now that signed hex is supported, add coverage (e.g., 0x20 then -0x10 => 16).
Additional test (place near other hex tests):
#[wasm_bindgen_test]
fn test_bigint_sum_context_hex_negative() {
let mut context = BigIntSumContext::new();
assert!(context.add_value("0x20").is_ok());
assert!(context.add_value("-0x10").is_ok());
assert_eq!(context.get_result(), "16");
}🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/bigint_sum.rs around lines
268 to 278 add a test that verifies negative hex parsing: create a new
BigIntSumContext, call add_value("0x20") and add_value("-0x10") asserting both
return Ok, then assert get_result() == "16"; annotate the function with
#[wasm_bindgen_test] and place it near the other hex tests so negative-hex
coverage is included.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor, understood! I'll skip that test case suggestion.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.462Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
| it('should sum positive big integers correctly', async () => { | ||
| perf.start('bigint-sum-positive'); | ||
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | ||
| perf.end('bigint-sum-positive'); | ||
|
|
||
| const data = JSON.parse(result.value || '[]'); | ||
|
|
||
| expect(data).toHaveLength(1); | ||
| expect(data[0]).toHaveProperty('total'); | ||
|
|
||
| // Expected: 123456789012345678901234567890 + 987654321098765432109876543210 + 555555555555555555555555555555 | ||
| // = 1666666665666666666566666666655 | ||
| expect(data[0].total).toBe('1666666665666666666566666666655'); | ||
| expect(perf.getDuration('bigint-sum-positive')).toBeLessThan(1000); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Time-based assertion may flake in CI; relax threshold or make it diagnostic-only.
WASM + CI variance can exceed 1s. Suggest raising to 3–5s or converting to a logged metric without failing the test.
- expect(perf.getDuration('bigint-sum-positive')).toBeLessThan(1000);
+ expect(perf.getDuration('bigint-sum-positive')).toBeLessThan(5000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should sum positive big integers correctly', async () => { | |
| perf.start('bigint-sum-positive'); | |
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | |
| perf.end('bigint-sum-positive'); | |
| const data = JSON.parse(result.value || '[]'); | |
| expect(data).toHaveLength(1); | |
| expect(data[0]).toHaveProperty('total'); | |
| // Expected: 123456789012345678901234567890 + 987654321098765432109876543210 + 555555555555555555555555555555 | |
| // = 1666666665666666666566666666655 | |
| expect(data[0].total).toBe('1666666665666666666566666666655'); | |
| expect(perf.getDuration('bigint-sum-positive')).toBeLessThan(1000); | |
| }); | |
| it('should sum positive big integers correctly', async () => { | |
| perf.start('bigint-sum-positive'); | |
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | |
| perf.end('bigint-sum-positive'); | |
| const data = JSON.parse(result.value || '[]'); | |
| expect(data).toHaveLength(1); | |
| expect(data[0]).toHaveProperty('total'); | |
| // Expected: 123456789012345678901234567890 + 987654321098765432109876543210 + 555555555555555555555555555555 | |
| // = 1666666665666666666566666666655 | |
| expect(data[0].total).toBe('1666666665666666666566666666655'); | |
| expect(perf.getDuration('bigint-sum-positive')).toBeLessThan(5000); | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/bigint-sum.test.ts around lines 89 to
103 the time-based assertion uses a 1000ms threshold which can flake on CI;
increase the threshold to a safer 3000–5000ms range or remove the failing
assertion and instead log perf.getDuration('bigint-sum-positive') for
diagnostics only so CI timing variance won't cause test failures. Ensure the
change either updates the expect call to toBeLessThan(3000 or 5000) or replaces
it with a non-failing console.warn/log of the duration.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor Understood! I'll leave the performance assertion threshold as is. You know your CI environment best.
ฅ(•ﻌ•)ฅ
| it('should handle hexadecimal input', async () => { | ||
| await db.query(` | ||
| INSERT INTO bigint_test (amount) VALUES | ||
| ('0x10'), | ||
| ('0xFF'), | ||
| ('0x100') | ||
| `); | ||
|
|
||
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | ||
| const data = JSON.parse(result.value || '[]'); | ||
|
|
||
| expect(data[0].total).toBe('527'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hex tests cover core paths; consider adding a signed-hex case.
Add one case with -0x… to lock in behavior once Rust side handles signed hex.
Example (can be a new test):
+ it('should handle negative hexadecimal input', async () => {
+ await db.query(`
+ INSERT INTO bigint_test (amount) VALUES
+ ('0x20'),
+ ('-0x10')
+ `);
+ const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test');
+ const data = JSON.parse(result.value || '[]');
+ expect(data[0].total).toBe('16');
+ });Also applies to: 309-320, 322-334, 336-346
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/bigint-sum.test.ts around lines 295 to
307 (and also apply similar additions at 309-320, 322-334, 336-346), the test
suite covers hex inputs but lacks a signed-hex case; add an additional insert
row using a negative hex literal (e.g., '-0x10') and update the expected summed
total accordingly (or add a separate test that inserts both positive and
negative hex values and asserts the correct total) so the behavior is locked in
once the Rust side supports signed hex parsing.
There was a problem hiding this comment.
@findolor, great! Thanks for confirming that the suggestion is acceptable. The negative hexadecimal test case will help ensure robust handling once the Rust implementation supports signed hex parsing.
🐰
◕ ◕
( · )
︶︶︶
| perf.start('bulk-bigint-sum'); | ||
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | ||
| perf.end('bulk-bigint-sum'); | ||
|
|
||
| const data = JSON.parse(result.value || '[]'); | ||
| expect(data[0]).toHaveProperty('total'); | ||
| expect(data[0].total).toMatch(/^\d+$/); | ||
| expect(perf.getDuration('bulk-bigint-sum')).toBeLessThan(3000); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Performance thresholds are tight; consider bumping to reduce flakes.
Similar rationale for bulk and join tests; CI can spike beyond 2–3s.
- expect(perf.getDuration('bulk-bigint-sum')).toBeLessThan(3000);
+ expect(perf.getDuration('bulk-bigint-sum')).toBeLessThan(6000);
@@
- expect(perf.getDuration('complex-bigint-query')).toBeLessThan(2000);
+ expect(perf.getDuration('complex-bigint-query')).toBeLessThan(6000);Also applies to: 482-483
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/bigint-sum.test.ts around lines 418-426
(and also adjust the related assertions at lines 482-483), the performance
assertion uses a tight threshold of 3000ms which causes CI flakes; increase the
allowed duration to a more resilient value (e.g., 5000ms or another team-agreed
threshold) by replacing
expect(perf.getDuration('bulk-bigint-sum')).toBeLessThan(3000) with the higher
threshold, and make the same change for the corresponding bulk/join test
assertions at lines 482-483 so all slow-but-valid runs pass reliably.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor, understood! I'll skip the performance threshold adjustments as requested. The current timing assertions will remain as they are.
| // Import the individual function modules | ||
| mod bigint_sum; | ||
| mod rain_math; | ||
|
|
||
| // Re-export the functions | ||
| pub use bigint_sum::*; | ||
| pub use rain_math::*; | ||
|
|
Motivation
The existing SQLite implementation lacked support for summing large integers that could include negative values. The previous approach using U256 (unsigned 256-bit integers) couldn't properly handle negative numbers in aggregate calculations.
Solution
Added a new BIGINT_SUM aggregate function that uses I256 (signed 256-bit integers) to properly handle both positive and negative large integer values. Key changes include:
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Tests
Chores