Remove unused RAIN_MATH database function #15
Conversation
WalkthroughRemoved the rain_math SQLite function and its registration/tests; retained BIGINT_SUM and FLOAT_SUM aggregates. Added imports: I256 and FromStr to bigint_sum, and rain_math_float::Float to float_sum. Updated tests to stop referencing RAIN_MATH_PROCESS and to validate BIGINT_SUM error handling and FLOAT_SUM presence. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SQLite
participant Extension as sqlite-web-core
participant BIGINT_SUM
participant FLOAT_SUM
Client->>SQLite: Load extension
SQLite->>Extension: register_custom_functions()
Note right of Extension #efefef: RAIN_MATH_PROCESS removed
Extension->>SQLite: register BIGINT_SUM (aggregate)
Extension->>SQLite: register FLOAT_SUM (aggregate)
Client->>SQLite: SELECT BIGINT_SUM(amount)
SQLite->>BIGINT_SUM: xStep called per row
BIGINT_SUM-->>SQLite: xFinal -> result or error
SQLite-->>Client: Return result or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 (6)
💤 Files with no reviewable changes (2)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
📚 Learning: 2025-08-27T05:52:51.073ZApplied to files:
📚 Learning: 2025-09-17T08:04:44.062ZApplied to files:
📚 Learning: 2025-09-19T07:06:20.173ZApplied to files:
📚 Learning: 2025-09-19T07:06:37.309ZApplied to files:
📚 Learning: 2025-09-17T17:44:54.855ZApplied to files:
📚 Learning: 2025-09-17T18:08:06.145ZApplied to files:
🧬 Code graph analysis (1)packages/sqlite-web-core/src/database_functions/bigint_sum.rs (1)
⏰ 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)
Comment |
findolor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (1)
25-35: Optional: support negative hex (e.g., "-0xFF")Currently hex handling excludes signed hex. If desired, extend parsing to handle a leading '-' before 0x/0X.
Apply this diff inside
add_valuewhere the hex/decimal branch is chosen:- let num = if trimmed.starts_with("0x") || trimmed.starts_with("0X") { + let num = if let Some(rest) = trimmed.strip_prefix("-0x").or_else(|| trimmed.strip_prefix("-0X")) { + if rest.is_empty() { + return Err("Incomplete hex number: missing digits after -0x".to_string()); + } + let v = I256::from_hex_str(rest) + .map_err(|e| format!("Failed to parse hex number '{}': {}", trimmed, e))?; + v.checked_neg() + .ok_or_else(|| format!("Integer overflow when negating {}", v))? + } else 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))? };Would you like this behavior? If not in scope for this PR, we can backlog it.
svelte-test/tests/database-functions/bigint-sum.test.ts (1)
197-205: Numeric HAVING with text results — minor note
BIGINT_SUMreturns text; SQLite coerces to numeric for> '0'. This is fine for sign checks; if you later depend on ordering magnitude beyond 1e15, consider explicit casts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
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(0 hunks)svelte-test/tests/database-functions/bigint-sum.test.ts(1 hunks)svelte-test/tests/integration/error-handling.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/sqlite-web-core/src/database_functions/rain_math.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.165Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
📚 Learning: 2025-09-17T06:55:20.165Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.165Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
Applied to files:
packages/sqlite-web-core/src/database_functions/bigint_sum.rspackages/sqlite-web-core/src/database_functions/mod.rssvelte-test/tests/database-functions/bigint-sum.test.ts
📚 Learning: 2025-08-27T05:52:51.073Z
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.073Z
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.rs
📚 Learning: 2025-09-17T08:04:44.039Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#6
File: svelte-test/tests/database-functions/float-sum.test.ts:78-86
Timestamp: 2025-09-17T08:04:44.039Z
Learning: In svelte-test/tests/database-functions/float-sum.test.ts, the schema defines `amount TEXT NOT NULL` but the NULL value handling tests attempt to insert NULL values, creating a constraint violation that prevents the tests from passing.
Applied to files:
svelte-test/tests/integration/error-handling.test.tssvelte-test/tests/database-functions/bigint-sum.test.ts
⏰ 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 (7)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (3)
2-2: BringFromStrinto scope — LGTMThis unblocks
I256::from_strusage in parsing paths.
158-392: Test coverage breadth — LGTMHex/decimal, overflow, edge cases, and a real-world scenario are well covered.
107-112: Confirmed — sqlite3_result_error() from xStep aborts the statement per SQLite docsSQLite's C API documents sqlite3_result_error() as signaling/“throwing” an error and the aggregate/window-function example shows it being called from xStep, so calling it here will abort evaluation. Verify this on our WASM build (run a small repro where xStep calls sqlite3_result_error and confirm the statement is aborted).
Location: packages/sqlite-web-core/src/database_functions/bigint_sum.rs (lines 107–112).packages/sqlite-web-core/src/database_functions/mod.rs (2)
13-56: RAIN_MATH removal path — LGTMRegistration now cleanly exposes only BIGINT_SUM and FLOAT_SUM.
22-22: Mark functions deterministic for better planningAdd SQLITE_DETERMINISTIC to the function flags to improve SQLite's query planning. rg returned no matches for SQLITE_DETERMINISTIC in the repo — verify the binding exists or import/use the numeric flag.
Applies to: packages/sqlite-web-core/src/database_functions/mod.rs (lines 22, 42)
- SQLITE_UTF8, + SQLITE_UTF8 | SQLITE_DETERMINISTIC,svelte-test/tests/database-functions/bigint-sum.test.ts (1)
56-66: Function-availability probe — LGTMUsing
SELECT BIGINT_SUM("123")as a smoke test is simple and reliable across SQLite builds that may not exposePRAGMA function_list.svelte-test/tests/integration/error-handling.test.ts (1)
407-474: Robust invalid-input coverage — LGTMGood spread of invalid strings; post-error recovery assertion is valuable.
| @@ -1,43 +1,17 @@ | |||
| use alloy::primitives::{I256, U256}; | |||
| use rain_math_float::Float; | |||
| use alloy::primitives::I256; | |||
There was a problem hiding this comment.
🧹 Nitpick
Avoid cross-module aliasing; import I256 where it’s used
I256 isn’t used in this module; it’s only relied upon by bigint_sum.rs via super::*. Import it directly in the child module and drop this import to reduce coupling and silence the unused-import lint.
Apply this diff here:
-use alloy::primitives::I256;And add in packages/sqlite-web-core/src/database_functions/bigint_sum.rs near the top:
use super::*;
+use alloy::primitives::I256;
use std::str::FromStr;If you keep the parent import on purpose for visibility, confirm CI isn’t treating the unused import as a warning/error.
📝 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.
| use alloy::primitives::I256; |
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/mod.rs around line 1, remove
the unused cross-module import use alloy::primitives::I256; because I256 is not
used in this parent module; instead add a direct import in
packages/sqlite-web-core/src/database_functions/bigint_sum.rs near the top
(e.g., use alloy::primitives::I256;) so the child module declares its own
dependency and eliminates coupling; if you intentionally left the parent import
for visibility, confirm CI does not treat unused imports as warnings/errors and
keep it only after verifying CI settings.
| for (const value of invalidValues) { | ||
| const sanitizedValue = value.replace(/'/g, "''"); | ||
|
|
||
| await db.query('DELETE FROM custom_function_test'); | ||
| await db.query(` | ||
| INSERT INTO custom_function_test (amount) VALUES ('${sanitizedValue}') | ||
| `); | ||
|
|
||
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM custom_function_test'); | ||
|
|
||
| expect(result.error).toBeDefined(); | ||
|
|
||
| const errorMessage = | ||
| result.error?.msg || result.error?.readableMsg || String(result.error); | ||
|
|
||
| expect(errorMessage.toLowerCase()).toMatch(/failed|invalid|empty|incomplete/); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Prefer parameter binding over string interpolation (tests)
Even in tests, replacing manual quote-doubling with bound parameters avoids edge cases and improves readability.
Example:
await db.query(
'INSERT INTO custom_function_test (amount) VALUES (?1)',
['not_a_number']
);Does db.query support bound parameters in this test harness? If yes, I can refactor the tests.
There was a problem hiding this comment.
this is not possible
There was a problem hiding this comment.
@findolor, understood! I acknowledge that parameter binding isn't available in your test harness. The manual quote escaping with value.replace(/'/g, "''") is the appropriate solution given that constraint.
Thanks for clarifying this limitation - the current string interpolation approach is perfectly valid when parameter binding isn't supported by the test infrastructure.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-web#15
File: svelte-test/tests/integration/error-handling.test.ts:418-434
Timestamp: 2025-09-17T17:44:54.832Z
Learning: In the sqlite-web test harness, parameter binding is not available for the db.query() method, so string interpolation with manual SQL escaping using value.replace(/'/g, "''") is the necessary approach for handling special characters in test data.
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.
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/sqlite-web#6
File: svelte-test/tests/database-functions/float-sum.test.ts:78-86
Timestamp: 2025-09-17T08:04:44.062Z
Learning: In svelte-test/tests/database-functions/float-sum.test.ts, the schema defines `amount TEXT NOT NULL` but the NULL value handling tests attempt to insert NULL values, creating a constraint violation that prevents the tests from passing.
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
Summary
https://chatgpt.com/codex/tasks/task_e_68ca6d3e774083339bf7bfe57af190bc
Summary by CodeRabbit