Add Float type sum logic #6
Conversation
WalkthroughAdds a new SQLite aggregate FLOAT_SUM (hex-encoded floats) implemented in Rust with C-ABI step/final entry points, registers it in database_functions, adjusts BigInt aggregate finalization memory handling, adds JS tests and a float decoding dependency, and expands test cleanup to drop new tables. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Extension Init
participant Reg as register_custom_functions
participant Mod as database_functions::mod
participant Fun as float_sum
App->>Reg: load extension
Reg->>Mod: ensure module loaded
Mod->>Fun: register "FLOAT_SUM" (arity=1)
Note right of Fun: xStep=float_sum_step<br/>xFinal=float_sum_final
sequenceDiagram
autonumber
actor Query as SELECT ... FLOAT_SUM(col)
participant SQLite as SQLite Aggregator
participant Step as float_sum_step
participant Ctx as sqlite3_aggregate_context
participant Final as float_sum_final
loop per row
SQLite->>Step: xStep(argv[0]=TEXT hex)
Step->>Ctx: sqlite3_aggregate_context(get/init)
Step->>Step: parse hex -> add to total
Step-->>SQLite: OK or sqlite3_result_error
end
SQLite->>Final: xFinal()
Final->>Ctx: retrieve context (or none)
Final->>Final: format total.as_hex() or zero
Final-->>SQLite: sqlite3_result_text(hex sum)
SQLite-->>Query: return aggregated hex string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| let float_value = Float::from_hex(trimmed) | ||
| .map_err(|e| format!("Failed to parse hex number '{}': {}", trimmed, e))?; | ||
|
|
||
| self.total = self.total.add(float_value).map_err(|e| { |
There was a problem hiding this comment.
nipick:
| self.total = self.total.add(float_value).map_err(|e| { | |
| self.total = (self.total + float_value).map_err(|e| { |
| impl FloatSumContext { | ||
| fn new() -> Self { | ||
| Self { | ||
| total: Float::parse("0".to_string()).unwrap(), |
There was a problem hiding this comment.
this will expose zero as its own method but for now you can just use Float::default to get zero without unwraps
| // No values were processed, return 0 in hex format | ||
| let zero_result = | ||
| CString::new("0x0000000000000000000000000000000000000000000000000000000000000000") | ||
| .unwrap(); |
There was a problem hiding this comment.
it would be better to serialize a value that we know to be zero instead of using the binary representation directly in case the underlying binary representation ever changes
| Ok(()) | ||
| } | ||
|
|
||
| fn get_result(&self) -> Result<String, String> { |
There was a problem hiding this comment.
nitpick: this might be a bit clearer
| fn get_result(&self) -> Result<String, String> { | |
| fn get_total_as_hex(&self) -> Result<String, String> { |
| let mut is_uninitialized = true; | ||
| let bytes = std::slice::from_raw_parts( | ||
| aggregate_context as *const u8, | ||
| std::mem::size_of::<FloatSumContext>(), | ||
| ); | ||
| for &byte in bytes { | ||
| if byte != 0 { | ||
| is_uninitialized = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick:
| let mut is_uninitialized = true; | |
| let bytes = std::slice::from_raw_parts( | |
| aggregate_context as *const u8, | |
| std::mem::size_of::<FloatSumContext>(), | |
| ); | |
| for &byte in bytes { | |
| if byte != 0 { | |
| is_uninitialized = false; | |
| break; | |
| } | |
| } | |
| let bytes = std::slice::from_raw_parts( | |
| aggregate_context as *const u8, | |
| std::mem::size_of::<FloatSumContext>(), | |
| ); | |
| let is_uninitialized = bytes.iter().all(|&b| b == 0); |
| } | ||
|
|
||
| // Aggregate function step - called for each row | ||
| pub unsafe extern "C" fn float_sum_step( |
There was a problem hiding this comment.
should we expose unsafe functions?
| pub unsafe extern "C" fn float_sum_step( | |
| unsafe extern "C" fn float_sum_step( |
| } | ||
|
|
||
| // Aggregate function final - called to return the final result | ||
| pub unsafe extern "C" fn float_sum_final(context: *mut sqlite3_context) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
svelte-test/tests/fixtures/test-helpers.ts (3)
178-191: Quote table identifiers to avoid edge cases and future-proof against reserved namesWrap table names in double quotes and escape embedded quotes. This prevents failures if a name ever collides with a reserved word or contains special chars.
- for (const table of tables) { + for (const table of tables) { try { - await db.query(`DROP TABLE IF EXISTS ${table}`); + const quoted = `"${table.replace(/"/g, '""')}"`; + await db.query(`DROP TABLE IF EXISTS ${quoted}`); } catch (error) { // Table might not exist, continue } }
173-201: Make cleanup resilient to FKs by temporarily disabling foreign_keysIf any tests add FK constraints, drops can fail. Disable FKs during cleanup and restore after.
export async function cleanupDatabase(db: SQLiteWasmDatabase): Promise<void> { if (!db) return; try { + await db.query('PRAGMA foreign_keys=OFF'); // Drop all test tables const tables = [ @@ for (const table of tables) { try { const quoted = `"${table.replace(/"/g, '""')}"`; await db.query(`DROP TABLE IF EXISTS ${quoted}`); } catch (error) { // Table might not exist, continue } } + await db.query('PRAGMA foreign_keys=ON'); } catch (error) { console.warn('Error cleaning up database:', error); } }
6-21: Replace fixed 1s sleep with readiness probe to reduce flakesBusy CI can exceed 1s. Poll a trivial query instead of sleeping.
export async function createTestDatabase(): Promise<SQLiteWasmDatabase> { // Initialize WASM module await init(); // Create database instance const result = SQLiteWasmDatabase.new(); if (result.error) { throw new Error(`Failed to create database: ${result.error.msg}`); } const db = result.value! - // Wait for worker to be ready - await new Promise(resolve => setTimeout(resolve, 1000)); + // Probe readiness + const start = Date.now(); + // eslint-disable-next-line no-constant-condition + while (true) { + const ping = await db.query('SELECT 1 AS ok'); + if (!ping.error) break; + if (Date.now() - start > 5000) throw new Error('DB worker not ready after 5s'); + await new Promise(r => setTimeout(r, 50)); + } return db; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
packages/sqlite-web-core/src/database_functions/float_sum.rs(1 hunks)packages/sqlite-web-core/src/database_functions/mod.rs(2 hunks)svelte-test/package.json(1 hunks)svelte-test/tests/database-functions/float-sum.test.ts(1 hunks)svelte-test/tests/fixtures/test-helpers.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T05:55:40.481Z
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.481Z
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/mod.rspackages/sqlite-web-core/src/database_functions/float_sum.rs
🔇 Additional comments (2)
packages/sqlite-web-core/src/database_functions/float_sum.rs (1)
78-95: Good: zero-initialization detection aligns with project conventionByte-scan to detect first-call matches maintainers’ preference for aggregate context init.
Please confirm this mirrors bigint_sum.rs to keep patterns consistent.
packages/sqlite-web-core/src/database_functions/mod.rs (1)
61-79: Registration of FLOAT_SUM looks correct and mirrors BIGINT_SUMUTF-8, arity=1, xStep/xFinal wired as expected. Error handling consistent with existing path.
| // Get the text value | ||
| let value_ptr = sqlite3_value_text(*argv); | ||
| if value_ptr.is_null() { | ||
| return; | ||
| } | ||
|
|
||
| let value_str = CStr::from_ptr(value_ptr as *const c_char).to_string_lossy(); | ||
|
|
||
| // Get or create the aggregate context |
There was a problem hiding this comment.
🧹 Nitpick
Consider explicit NULL check via sqlite3_value_type for clarity
Using sqlite3_value_text implicitly converts; a direct SQLITE_NULL check makes intent clearer and avoids accidental treatment of non-NULL falsy values.
- // Get the text value
- let value_ptr = sqlite3_value_text(*argv);
- if value_ptr.is_null() {
- return;
- }
+ // Skip NULLs explicitly
+ if sqlite3_value_type(*argv) == SQLITE_NULL {
+ return;
+ }
+ // Get the text value
+ let value_ptr = sqlite3_value_text(*argv);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/float_sum.rs around lines 55
to 63, replace the implicit NULL detection via sqlite3_value_text with an
explicit sqlite3_value_type(*argv) == SQLITE_NULL check before calling
sqlite3_value_text: call sqlite3_value_type(*argv) and if it returns SQLITE_NULL
return early, otherwise call sqlite3_value_text safely and proceed; use the
SQLITE_NULL constant from the sqlite3 FFI bindings to perform the comparison so
intent is clear and non-NULL values are not misinterpreted.
There was a problem hiding this comment.
this is not an issue
There was a problem hiding this comment.
@findolor, understood! You're right that the current approach works perfectly fine. The implicit NULL handling through sqlite3_value_text returning a null pointer is a valid pattern and the code functions correctly as written. I'll note your preference for this approach.
(\ /)
( ._.)
o_(")(")
🧠 Learnings used
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
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.481Z
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.
| // Add this value to the running total | ||
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Early-return after setting error to avoid accidental future changes executing code
Defensive style: return immediately after sqlite3_result_error in xStep.
- 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);
+ return;
}📝 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.
| // Add this value to the running total | |
| 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); | |
| } | |
| } | |
| // Add this value to the running total | |
| 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); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/float_sum.rs around lines 96
to 101, after calling sqlite3_result_error when add_value returns Err you must
return immediately from the xStep function to prevent further code from
executing; modify the error branch to call sqlite3_result_error(...) and then
return (i.e., early-return from the function) so no subsequent logic runs after
an SQLite error is reported.
There was a problem hiding this comment.
this is fixed in the next commit
There was a problem hiding this comment.
@findolor, perfect! Thank you for confirming that the early return after sqlite3_result_error is addressed in the next commit. That defensive programming practice will help prevent any unintended code execution after error reporting.
🐰✨
∩___∩
( ◉ ◉ )
＼ ∇ ／ Glad it's fixed!
｜ ｜
Ｕ Ｕ
🧠 Learnings used
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
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.481Z
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.
| "dependencies": { | ||
| "@rainlanguage/float": "^0.0.0-alpha.22", | ||
| "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
🧩 Analysis chain
Move @rainlanguage/float to devDependencies (tests-only) and pin exact pre-release
This package is only used by tests; keep runtime lean and avoid accidental upgrades on a pre-release range.
Run the script to confirm no production code imports it:
Suggested change:
- "dependencies": {
- "@rainlanguage/float": "^0.0.0-alpha.22",
- "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz"
- }
+ "dependencies": {
+ "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz"
+ },
+ "devDependencies": {
+ "@rainlanguage/float": "0.0.0-alpha.22"
+ }🏁 Script executed:
#!/bin/bash
# Verify @rainlanguage/float is only used in tests
rg -nP "@rainlanguage/float" -C2Length of output: 1624
Move @rainlanguage/float to devDependencies and pin exact pre-release (tests-only)
Confirmed import only in tests (svelte-test/tests/database-functions/float-sum.test.ts:9) and currently declared in svelte-test/package.json:43 — move it out of runtime deps and pin the exact pre-release to avoid accidental upgrades.
- "dependencies": {
- "@rainlanguage/float": "^0.0.0-alpha.22",
- "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz"
- }
+ "dependencies": {
+ "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz"
+ },
+ "devDependencies": {
+ "@rainlanguage/float": "0.0.0-alpha.22"
+ }📝 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.
| "dependencies": { | |
| "@rainlanguage/float": "^0.0.0-alpha.22", | |
| "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz" | |
| } | |
| "dependencies": { | |
| "sqlite-web": "file:../pkg/sqlite-web-0.1.0.tgz" | |
| }, | |
| "devDependencies": { | |
| "@rainlanguage/float": "0.0.0-alpha.22" | |
| } |
🤖 Prompt for AI Agents
In svelte-test/package.json around lines 42 to 45, @rainlanguage/float is
declared as a runtime dependency with a caret pre-release range; move it to
devDependencies and pin the exact pre-release version used by tests. Remove the
"@rainlanguage/float": "^0.0.0-alpha.22" entry from "dependencies", add
"@rainlanguage/float": "0.0.0-alpha.22" to "devDependencies", then run the
project’s lockfile update (npm install or yarn install) to update
package-lock.json/yarn.lock accordingly.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| function decodeFloatHex(hex: string): string { | ||
| const floatRes = Float.fromHex(hex as `0x${string}`); | ||
| if (floatRes.error) { | ||
| throw new Error(`fromHex failed: ${String(floatRes.error)}`); | ||
| } | ||
| const fmtRes = floatRes.value.format(); | ||
| if (fmtRes.error) { | ||
| throw new Error(`format failed: ${String(fmtRes.error)}`); | ||
| } | ||
| return fmtRes.value as string; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Runtime decode helper: surface hex for easier failure triage
When decode fails, include the original hex to speed up debugging.
function decodeFloatHex(hex: string): string {
const floatRes = Float.fromHex(hex as `0x${string}`);
if (floatRes.error) {
- throw new Error(`fromHex failed: ${String(floatRes.error)}`);
+ throw new Error(`fromHex failed for ${hex}: ${String(floatRes.error)}`);
}
const fmtRes = floatRes.value.format();
if (fmtRes.error) {
- throw new Error(`format failed: ${String(fmtRes.error)}`);
+ throw new Error(`format failed for ${hex}: ${String(fmtRes.error)}`);
}
return fmtRes.value as string;
}📝 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.
| function decodeFloatHex(hex: string): string { | |
| const floatRes = Float.fromHex(hex as `0x${string}`); | |
| if (floatRes.error) { | |
| throw new Error(`fromHex failed: ${String(floatRes.error)}`); | |
| } | |
| const fmtRes = floatRes.value.format(); | |
| if (fmtRes.error) { | |
| throw new Error(`format failed: ${String(fmtRes.error)}`); | |
| } | |
| return fmtRes.value as string; | |
| } | |
| function decodeFloatHex(hex: string): string { | |
| const floatRes = Float.fromHex(hex as `0x${string}`); | |
| if (floatRes.error) { | |
| throw new Error(`fromHex failed for ${hex}: ${String(floatRes.error)}`); | |
| } | |
| const fmtRes = floatRes.value.format(); | |
| if (fmtRes.error) { | |
| throw new Error(`format failed for ${hex}: ${String(fmtRes.error)}`); | |
| } | |
| return fmtRes.value as string; | |
| } |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 25 to 35,
the decodeFloatHex helper throws errors without including the original hex
string, which makes debugging harder; update the thrown Error messages to
include the hex parameter (e.g., include `hex` in both the fromHex and format
failure messages) so failures show the raw input that caused the problem and
make triage faster.
There was a problem hiding this comment.
it's fine to have this
There was a problem hiding this comment.
@findolor, understood! I'll leave the error messages as they are. The current implementation works well for your use case.
| it("should sum positive float values correctly", async () => { | ||
| perf.start("float-sum-positive"); | ||
| const result = await db.query( | ||
| "SELECT FLOAT_SUM(amount) as total FROM float_test", | ||
| ); | ||
| perf.end("float-sum-positive"); | ||
|
|
||
| const data = JSON.parse(result.value || "[]"); | ||
|
|
||
| expect(data).toHaveLength(1); | ||
| expect(data[0]).toHaveProperty("total"); | ||
|
|
||
| const dec = decodeFloatHex(data[0].total); | ||
| expect(dec).toBe("2.1"); | ||
| expect(perf.getDuration("float-sum-positive")).toBeLessThan(1000); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Avoid brittle performance thresholds
Hard 1s thresholds are flaky across machines/CI. Prefer assertions on result correctness and optionally log duration.
- expect(perf.getDuration("float-sum-positive")).toBeLessThan(1000);
+ // Non-blocking: capture perf without failing on slow CI
+ // expect(perf.getDuration("float-sum-positive")).toBeLessThan(1000);📝 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 float values correctly", async () => { | |
| perf.start("float-sum-positive"); | |
| const result = await db.query( | |
| "SELECT FLOAT_SUM(amount) as total FROM float_test", | |
| ); | |
| perf.end("float-sum-positive"); | |
| const data = JSON.parse(result.value || "[]"); | |
| expect(data).toHaveLength(1); | |
| expect(data[0]).toHaveProperty("total"); | |
| const dec = decodeFloatHex(data[0].total); | |
| expect(dec).toBe("2.1"); | |
| expect(perf.getDuration("float-sum-positive")).toBeLessThan(1000); | |
| }); | |
| it("should sum positive float values correctly", async () => { | |
| perf.start("float-sum-positive"); | |
| const result = await db.query( | |
| "SELECT FLOAT_SUM(amount) as total FROM float_test", | |
| ); | |
| perf.end("float-sum-positive"); | |
| const data = JSON.parse(result.value || "[]"); | |
| expect(data).toHaveLength(1); | |
| expect(data[0]).toHaveProperty("total"); | |
| const dec = decodeFloatHex(data[0].total); | |
| expect(dec).toBe("2.1"); | |
| // Non-blocking: capture perf without failing on slow CI | |
| // expect(perf.getDuration("float-sum-positive")).toBeLessThan(1000); | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 102 to
117, the test uses a hard 1s performance assertion which is flaky on different
machines/CI; remove the strict
expect(perf.getDuration("float-sum-positive")).toBeLessThan(1000) assertion and
either delete the perf check entirely or replace it with a non-brittle action
such as logging the duration or asserting it is a non-negative number (e.g.,
expect(duration).toBeGreaterThanOrEqual(0)) while keeping the correctness
assertions (JSON parsing, length, total value decode) intact.
There was a problem hiding this comment.
@findolor, understood! I'll respect your decision to keep the performance assertion as is. The test implementation looks good overall.
| perf.start("complex-float-query"); | ||
| const result = await db.query(` | ||
| SELECT | ||
| t.category, | ||
| FLOAT_SUM(t.amount) as total_amount, | ||
| c.multiplier | ||
| FROM float_test t | ||
| JOIN float_categories c ON t.category = c.name | ||
| GROUP BY t.category, c.multiplier | ||
| ORDER BY total_amount DESC | ||
| `); | ||
| perf.end("complex-float-query"); | ||
|
|
||
| const data = JSON.parse(result.value || "[]"); | ||
| expect(Array.isArray(data)).toBe(true); | ||
| expect(data.length).toBe(2); | ||
|
|
||
| const rowsByCategory = data.reduce( | ||
| (map: Record<string, any>, row: any) => { | ||
| map[row.category] = row; | ||
| return map; | ||
| }, | ||
| {} as Record<string, any>, | ||
| ); | ||
|
|
||
| expect(rowsByCategory["income"]).toBeDefined(); | ||
| expect(decodeFloatHex(rowsByCategory["income"].total_amount)).toBe("0.1"); | ||
| expect(rowsByCategory["income"].multiplier).toBe("2"); | ||
|
|
||
| expect(rowsByCategory["expense"]).toBeDefined(); | ||
| expect(decodeFloatHex(rowsByCategory["expense"].total_amount)).toBe( | ||
| "0.5", | ||
| ); | ||
| expect(rowsByCategory["expense"].multiplier).toBe("1"); | ||
|
|
||
| expect(perf.getDuration("complex-float-query")).toBeLessThan(2000); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Same: relax complex query perf assertion
Joins/GC can spike under CI.
- expect(perf.getDuration("complex-float-query")).toBeLessThan(2000);
+ // See note on perf thresholds. Consider logging instead of asserting.
+ // expect(perf.getDuration("complex-float-query")).toBeLessThan(2000);📝 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.
| perf.start("complex-float-query"); | |
| const result = await db.query(` | |
| SELECT | |
| t.category, | |
| FLOAT_SUM(t.amount) as total_amount, | |
| c.multiplier | |
| FROM float_test t | |
| JOIN float_categories c ON t.category = c.name | |
| GROUP BY t.category, c.multiplier | |
| ORDER BY total_amount DESC | |
| `); | |
| perf.end("complex-float-query"); | |
| const data = JSON.parse(result.value || "[]"); | |
| expect(Array.isArray(data)).toBe(true); | |
| expect(data.length).toBe(2); | |
| const rowsByCategory = data.reduce( | |
| (map: Record<string, any>, row: any) => { | |
| map[row.category] = row; | |
| return map; | |
| }, | |
| {} as Record<string, any>, | |
| ); | |
| expect(rowsByCategory["income"]).toBeDefined(); | |
| expect(decodeFloatHex(rowsByCategory["income"].total_amount)).toBe("0.1"); | |
| expect(rowsByCategory["income"].multiplier).toBe("2"); | |
| expect(rowsByCategory["expense"]).toBeDefined(); | |
| expect(decodeFloatHex(rowsByCategory["expense"].total_amount)).toBe( | |
| "0.5", | |
| ); | |
| expect(rowsByCategory["expense"].multiplier).toBe("1"); | |
| expect(perf.getDuration("complex-float-query")).toBeLessThan(2000); | |
| }); | |
| perf.start("complex-float-query"); | |
| const result = await db.query(` | |
| SELECT | |
| t.category, | |
| FLOAT_SUM(t.amount) as total_amount, | |
| c.multiplier | |
| FROM float_test t | |
| JOIN float_categories c ON t.category = c.name | |
| GROUP BY t.category, c.multiplier | |
| ORDER BY total_amount DESC | |
| `); | |
| perf.end("complex-float-query"); | |
| const data = JSON.parse(result.value || "[]"); | |
| expect(Array.isArray(data)).toBe(true); | |
| expect(data.length).toBe(2); | |
| const rowsByCategory = data.reduce( | |
| (map: Record<string, any>, row: any) => { | |
| map[row.category] = row; | |
| return map; | |
| }, | |
| {} as Record<string, any>, | |
| ); | |
| expect(rowsByCategory["income"]).toBeDefined(); | |
| expect(decodeFloatHex(rowsByCategory["income"].total_amount)).toBe("0.1"); | |
| expect(rowsByCategory["income"].multiplier).toBe("2"); | |
| expect(rowsByCategory["expense"]).toBeDefined(); | |
| expect(decodeFloatHex(rowsByCategory["expense"].total_amount)).toBe( | |
| "0.5", | |
| ); | |
| expect(rowsByCategory["expense"].multiplier).toBe("1"); | |
| // See note on perf thresholds. Consider logging instead of asserting. | |
| // expect(perf.getDuration("complex-float-query")).toBeLessThan(2000); | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 404 to
440, the performance assertion for "complex-float-query" is too strict for CI
(expect(perf.getDuration("complex-float-query")).toBeLessThan(2000)); relax it
by increasing the threshold (for example to 5000 ms) or remove the strict timing
assertion entirely so joins/GC spikes on CI won't cause flaky failures; update
the expectation accordingly and keep the rest of the test intact.
There was a problem hiding this comment.
@findolor Thank you for confirming! The performance assertion flexibility will help avoid flaky test failures in CI environments where timing can be unpredictable due to resource contention.
🐰✨
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/sqlite-web-core/src/database_functions/float_sum.rs (2)
91-95: Early-return after sqlite3_result_error in xStepReturn immediately after setting the error to prevent accidental future logic from running in this frame.
Apply:
- 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) + sqlite3_result_error(context, error_msg.as_ptr() as *const c_char, -1); + return; }
157-163: Tests: avoid temporary String allocations in Float::parsePass string literals directly to parse to drop the unnecessary to_string().
- let zero = Float::parse("0".to_string()).unwrap(); + let zero = Float::parse("0").unwrap();Apply similarly to other tests (e.g., "0.1", "1.5", "100.25", etc.).
packages/sqlite-web-core/src/database_functions/mod.rs (1)
15-16: Import only needed items from float_sum to avoid leaking internals into scopeLimit imports to the C-ABI entry points.
-use float_sum::*; +use float_sum::{float_sum_final, float_sum_step};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs(4 hunks)packages/sqlite-web-core/src/database_functions/float_sum.rs(1 hunks)packages/sqlite-web-core/src/database_functions/mod.rs(2 hunks)svelte-test/tests/database-functions/float-sum.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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
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.481Z
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-27T05:55:40.481Z
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.481Z
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.rspackages/sqlite-web-core/src/database_functions/mod.rspackages/sqlite-web-core/src/database_functions/float_sum.rs
📚 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/float-sum.test.tspackages/sqlite-web-core/src/database_functions/float_sum.rs
🧬 Code graph analysis (4)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (2)
packages/sqlite-web-core/src/database_functions/float_sum.rs (4)
std(65-65)std(82-82)std(109-112)std(142-145)packages/sqlite-web-core/src/database_functions/rain_math.rs (1)
std(96-99)
packages/sqlite-web-core/src/database_functions/mod.rs (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (5)
new(9-11)std(75-75)std(93-93)std(123-126)std(148-151)packages/sqlite-web-core/src/database_functions/float_sum.rs (7)
new(8-12)std(65-65)std(82-82)std(109-112)std(142-145)float_sum_step(41-95)float_sum_final(98-149)
svelte-test/tests/database-functions/float-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/float_sum.rs (1)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (6)
new(9-11)add_value(13-43)std(75-75)std(93-93)std(123-126)std(148-151)
⏰ 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 (1)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (1)
115-155: xFinal: correct use of nBytes=0 and explicit drop; leak/UB avoidedUsing sqlite3_aggregate_context(context, 0) and dropping the context with drop_in_place is the right pattern for aggregates. Looks good.
| let value_str = CStr::from_ptr(value_ptr as *const c_char).to_string_lossy(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Avoid lossy UTF‑8 conversion and extra allocation
Use CStr::to_str() and bail on invalid UTF‑8; this avoids allocation and silent replacements.
- 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"invalid UTF-8".as_ptr(), -1);
+ return;
+ }
+ };Also update the add call to pass value_str directly (no & needed).
- if let Err(e) = (*sum_context).add_value(&value_str) {
+ if let Err(e) = (*sum_context).add_value(value_str) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/float_sum.rs around lines 61
to 62, replace the lossy allocation using CStr::from_ptr(...).to_string_lossy()
with a fallible UTF‑8 conversion using CStr::from_ptr(...).to_str() and
propagate or return an error if the conversion fails (do not silently replace
invalid bytes); then pass the resulting &str directly to the add call (remove
the extra &). Ensure you handle the Result from to_str() (using ? or map_err as
appropriate) so invalid UTF‑8 is rejected and no extra allocation occurs.
There was a problem hiding this comment.
this is not a blocking issue
There was a problem hiding this comment.
@findolor Understood! I acknowledge your decision to defer this optimization. The current implementation works correctly, and this suggestion can be addressed in a future PR if desired.
🧠 Learnings used
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
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.481Z
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
svelte-test/tests/database-functions/float-sum.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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
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.481Z
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-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:
svelte-test/tests/database-functions/float-sum.test.ts
🧬 Code graph analysis (1)
svelte-test/tests/database-functions/float-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 (1)
svelte-test/tests/database-functions/float-sum.test.ts (1)
327-350: Signed-float coverage looks solidGood coverage for negative+positive cancellation and decoded equality.
| await db.query(` | ||
| CREATE TABLE float_test ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| amount TEXT NOT NULL, | ||
| category TEXT, | ||
| description TEXT | ||
| ) | ||
| `); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix schema/test mismatch: NOT NULL prevents NULL tests from ever running
amount TEXT NOT NULL blocks inserts of NULL later in "NULL Value Handling". Drop NOT NULL (or stop inserting NULL). Recommend allowing NULL so the aggregate’s NULL semantics are exercised.
Apply:
@@
- CREATE TABLE float_test (
+ CREATE TABLE float_test (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- amount TEXT NOT NULL,
+ amount TEXT,
category TEXT,
description TEXT
)Optionally, make the first NULL test actually include a NULL row:
@@
- await db.query(`
- INSERT INTO float_test (amount, category) VALUES
- ('${floatHex.zeroPointOne}', 'test'),
- ('${floatHex.zeroPointFive}', 'test')
- `);
+ await db.query(`
+ INSERT INTO float_test (amount, category) VALUES
+ ('${floatHex.zeroPointOne}', 'test'),
+ ('${floatHex.zeroPointFive}', 'test'),
+ (NULL, 'test')
+ `);Also applies to: 579-586
🏁 Script executed:
#!/bin/bash
# Verify no other CREATE TABLE float_test definitions conflict with this change
rg -nP 'CREATE\s+TABLE\s+float_test\b' -C2Length of output: 408
Make amount nullable so NULL-value tests can run
File: svelte-test/tests/database-functions/float-sum.test.ts (around lines 78–86)
amount is declared TEXT NOT NULL, which prevents inserting NULL in the later "NULL Value Handling" tests — drop NOT NULL. Optionally add a NULL row to the initial INSERT.
@@
- CREATE TABLE float_test (
+ CREATE TABLE float_test (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- amount TEXT NOT NULL,
+ amount TEXT,
category TEXT,
description TEXT
)Optional:
@@
- await db.query(`
- INSERT INTO float_test (amount, category) VALUES
- ('${floatHex.zeroPointOne}', 'test'),
- ('${floatHex.zeroPointFive}', 'test')
- `);
+ await db.query(`
+ INSERT INTO float_test (amount, category) VALUES
+ ('${floatHex.zeroPointOne}', 'test'),
+ ('${floatHex.zeroPointFive}', 'test'),
+ (NULL, 'test')
+ `);📝 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.
| await db.query(` | |
| CREATE TABLE float_test ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| amount TEXT NOT NULL, | |
| category TEXT, | |
| description TEXT | |
| ) | |
| `); | |
| }); | |
| await db.query(` | |
| CREATE TABLE float_test ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| amount TEXT, | |
| category TEXT, | |
| description TEXT | |
| ) | |
| `); | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 78 to 86,
the CREATE TABLE defines amount as "TEXT NOT NULL" which blocks inserting NULLs
needed by the NULL Value Handling tests; remove the NOT NULL constraint from the
amount column so it becomes nullable, and optionally add an INSERT of a row with
amount = NULL in the initial seed data to ensure the NULL-case exists for the
tests.
There was a problem hiding this comment.
we have no issues in tests
There was a problem hiding this comment.
🧩 Analysis chain
@findolor You're absolutely right! Looking at the test file, I can see that while there are "NULL Value Handling" test sections mentioned, they don't actually insert NULL values into the amount column. The current tests are passing because they test the concept of NULL handling without actually inserting NULLs.
My initial concern was premature - I was anticipating that the NULL value handling tests would include actual NULL insertions, but the current implementation handles this differently. The tests are working as intended with the current schema.
Thank you for the clarification! The NOT NULL constraint can remain as it is since the current test suite doesn't require inserting NULL values.
(\ /)
( ._.)
o_(")(")
🏁 Script executed:
#!/bin/bash
# Let's examine the full test file to understand the NULL value handling tests
cat svelte-test/tests/database-functions/float-sum.test.ts
Length of output: 18063
🏁 Script executed:
#!/bin/bash
# Search for NULL value handling tests and any INSERT statements with NULL
rg -n -A 10 -B 5 "NULL Value Handling" svelte-test/tests/database-functions/float-sum.test.ts
Length of output: 557
🏁 Script executed:
#!/bin/bash
# Search for any INSERT statements that might include NULL values
rg -n -A 5 -B 2 "INSERT.*NULL" svelte-test/tests/database-functions/float-sum.test.ts
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any mentions of NULL in the test file
rg -n -C 3 "NULL" svelte-test/tests/database-functions/float-sum.test.ts
Length of output: 782
@findolor I need to respectfully point out an issue I found when examining the full test file. There's actually a contradiction that will cause test failures:
The Problem:
- Line 81 defines:
amount TEXT NOT NULL - Lines 581-582 try to insert:
(NULL, 'test'), (NULL, 'test')
The test "should return 0 when all values are NULL" on lines 578-583 will fail with a constraint violation because it attempts to insert NULL values into a column that explicitly prohibits them.
This test cannot pass with the current schema. You'll need to either:
- Remove the NOT NULL constraint (as I originally suggested) to allow the NULL handling tests to work
- Remove or modify the NULL insertion tests to test NULL behavior differently
Have these specific NULL value handling tests been run successfully? The constraint violation should prevent the INSERT from completing.
(\ /)
( ._.)
o_(")(")
✏️ Learnings added
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.
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: 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
| } catch (error) { | ||
| throw new Error("RAIN_MATH_PROCESS not available"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Preserve underlying error in availability checks
Surface the caught error to speed up triage.
- } catch (error) {
- throw new Error("RAIN_MATH_PROCESS not available");
+ } catch (error) {
+ throw new Error(`RAIN_MATH_PROCESS not available: ${String((error as any)?.message ?? error)}`);
}
@@
- } catch (error) {
- throw new Error("FLOAT_SUM function not available");
+ } catch (error) {
+ throw new Error(`FLOAT_SUM not available: ${String((error as any)?.message ?? error)}`);
}Also applies to: 124-125
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 110-112
(and similarly lines 124-125), the catch blocks currently throw a new
Error("RAIN_MATH_PROCESS not available") which discards the original error;
update each catch to preserve and surface the underlying error by including the
caught error in the new Error (or rethrow using the original error as the cause)
so the thrown error message/log contains the original error details for faster
triage.
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. Thanks for the clarification!
🧠 Learnings used
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
| try { | ||
| const result = await db.query( | ||
| `SELECT FLOAT_SUM("${floatHex.zeroPointOne}") as test`, | ||
| ); | ||
| expect(result).toBeDefined(); |
There was a problem hiding this comment.
🧹 Nitpick
Use single quotes for SQL string literal
Double quotes for string literals are not guaranteed across SQLite builds; prefer single quotes to avoid DQS dependency.
- const result = await db.query(
- `SELECT FLOAT_SUM("${floatHex.zeroPointOne}") as test`,
- );
+ const result = await db.query(
+ `SELECT FLOAT_SUM('${floatHex.zeroPointOne}') as test`,
+ );📝 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.
| try { | |
| const result = await db.query( | |
| `SELECT FLOAT_SUM("${floatHex.zeroPointOne}") as test`, | |
| ); | |
| expect(result).toBeDefined(); | |
| try { | |
| const result = await db.query( | |
| `SELECT FLOAT_SUM('${floatHex.zeroPointOne}') as test`, | |
| ); | |
| expect(result).toBeDefined(); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 114 to
118, the SQL uses double quotes for a string literal which can break on SQLite
builds that require single quotes; update the query to use single quotes for the
string literal (e.g. replace "…" with '…'), ensuring the template value is
properly interpolated/escaped inside the single-quoted SQL string so the query
remains syntactically correct.
There was a problem hiding this comment.
no nee to do this
There was a problem hiding this comment.
@findolor, understood! I'll skip this suggestion. Your SQLite setup clearly handles double quotes fine, so there's no need to change it.
(\ /)
( ._. )
o_(")(")
🧠 Learnings used
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
| expect(result.error).toBeDefined(); | ||
| expect(result.error?.msg).toContain("Failed to parse hex number"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Make error-message assertions resilient to wording/casing
Match message intent without coupling to exact strings.
- expect(result.error?.msg).toContain("Failed to parse hex number");
+ expect(result.error?.msg).toMatch(/failed to parse hex number/i);
- expect(result.error).toBeDefined();
- expect(result.error?.msg).toContain("Failed to parse hex number");
+ expect(result.error).toBeDefined();
+ expect(result.error?.msg).toMatch(/failed to parse hex number/i);
- expect(result.error?.msg).toContain(
- "Empty string is not a valid hex number",
- );
+ expect(result.error?.msg).toMatch(/empty string.*hex/i);
- expect(result.error?.msg).toContain("Failed to parse hex number");
+ expect(result.error?.msg).toMatch(/failed to parse hex number/i);
- expect(result.error?.msg).toContain("Failed to parse hex number");
+ expect(result.error?.msg).toMatch(/failed to parse hex number/i);Also applies to: 297-299, 309-313, 323-325, 394-396
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/float-sum.test.ts around lines 227-229
(and similarly at 297-299, 309-313, 323-325, 394-396), the tests assert on exact
error message substrings and casing; replace those tight assertions with
resilient checks that verify the intent (e.g., that an error exists and its
message contains key tokens normalized) by converting the message to lowercase
and asserting it includes expected keywords like "failed", "parse", "hex", or
other relevant tokens for each case, or use regex/partial matching to avoid
coupling to exact wording/case while preserving test intent.
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 error message assertions as they are. Thanks for the feedback.
(\ /)
( ._.)
o_(")(")
🧠 Learnings used
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
Caution
Chained PR. Do not merge before #5
Change base branch before merge.
Motivation
This PR adds the sum logic for Float type numbers.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix rainlanguage/rain.orderbook#2074
Summary by CodeRabbit