sqlite: fix aggregate step function undefined handling #58780

mceachen · 2025-06-21T19:23:29Z

First off: kudos to the team for node:sqlite, it's a great thing for node.js to have built-in! (I'm a maintainer of better-sqlite3)

I was working on a bridge facade from better-sqlite to node:sqlite and found an edge case in the aggregate function callback.

When an aggregate step function returns undefined (not NULL), we should preserve the accumulator value instead of resetting it to null. This allows step functions to conditionally skip processing certain values while maintaining the current aggregate state.

Before this fix, returning undefined would incorrectly set the accumulator to null, breaking the aggregation logic.

This patch makes the step function behavior match (presumably) expected semantics, where undefined return values preserve state and only explicit null returns store null values.

This patch includes a bunch of new test coverage for:

Preserving accumulator when step returns undefined
Distinguishing between undefined and null returns
Handling implicit undefined returns (empty function bodies)

Here are the failing tests without the patch applied to node_sqlite.cc:

✖ failing tests:

test at test/parallel/test-sqlite-aggregate-function.mjs:418:3
✖ preserves accumulator when step returns undefined (0.937672ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  null !== 12
  
      at assert.<computed> [as strictEqual] (node:internal/test_runner/test:320:18)
      at TestContext.<anonymous> (file:///home/mrm/src/node/test/parallel/test-sqlite-aggregate-function.mjs:431:14)
      at Test.runInAsyncScope (node:async_hooks:213:14)
      at Test.run (node:internal/test_runner/test:1062:25)
      at Test.start (node:internal/test_runner/test:959:17)
      at node:internal/test_runner/test:1464:71
      at node:internal/per_context/primordials:483:82
      at new Promise (<anonymous>)
      at new SafePromise (node:internal/per_context/primordials:451:29)
      at node:internal/per_context/primordials:483:9 {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: null,
    expected: 12,
    operator: 'strictEqual'
  }

test at test/parallel/test-sqlite-aggregate-function.mjs:434:3
✖ distinguishes between undefined and null returns (0.445361ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  null !== 14
  
      at assert.<computed> [as strictEqual] (node:internal/test_runner/test:320:18)
      at TestContext.<anonymous> (file:///home/mrm/src/node/test/parallel/test-sqlite-aggregate-function.mjs:464:14)
      at Test.runInAsyncScope (node:async_hooks:213:14)
      at Test.run (node:internal/test_runner/test:1062:25)
      at Suite.processPendingSubtests (node:internal/test_runner/test:752:18)
      at Test.postRun (node:internal/test_runner/test:1191:19)
      at Test.run (node:internal/test_runner/test:1119:12)
      at async Promise.all (index 0)
      at async Suite.run (node:internal/test_runner/test:1466:7)
      at async Test.processPendingSubtests (node:internal/test_runner/test:752:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: null,
    expected: 14,
    operator: 'strictEqual'
  }

test at test/parallel/test-sqlite-aggregate-function.mjs:467:3
✖ handles implicit undefined returns (0.392371ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  null !== 30
  
      at assert.<computed> [as strictEqual] (node:internal/test_runner/test:320:18)
      at TestContext.<anonymous> (file:///home/mrm/src/node/test/parallel/test-sqlite-aggregate-function.mjs:484:14)
      at Test.runInAsyncScope (node:async_hooks:213:14)
      at Test.run (node:internal/test_runner/test:1062:25)
      at Suite.processPendingSubtests (node:internal/test_runner/test:752:18)
      at Test.postRun (node:internal/test_runner/test:1191:19)
      at Test.run (node:internal/test_runner/test:1119:12)
      at async Suite.processPendingSubtests (node:internal/test_runner/test:752:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: null,
    expected: 30,
    operator: 'strictEqual'
  }

Cheers!

When an aggregate step function returns undefined, preserve the accumulator value instead of resetting it to null. This allows step functions to conditionally skip processing certain values while maintaining the current aggregate state. Before this fix, returning undefined would incorrectly set the accumulator to null, breaking the aggregation logic. Fixes the step function behavior to match expected semantics where undefined return values preserve state and only explicit null returns store null values.

nodejs-github-bot · 2025-06-21T19:23:35Z

Review requested:

@nodejs/sqlite

Fix trailing whitespace and arrow function formatting to pass Node.js JavaScript linting requirements.

mceachen · 2025-06-21T19:39:25Z

Here's a simple test harness to run to study the issue, if it helps:

// test-aggregate-undefined.js
// Run with: node --experimental-sqlite test-aggregate-undefined.js

// Test with node:sqlite (shows the bug)
const { DatabaseSync } = require("node:sqlite");

console.log("Testing aggregate function with undefined return value...\n");

// Create an in-memory database
const db = new DatabaseSync(":memory:");

// Create test data
db.exec("CREATE TABLE test (value INTEGER)");
db.exec("INSERT INTO test VALUES (1), (2), (3), (4), (5)");

// Register an aggregate function that returns undefined for value 3
db.aggregate("test_sum", {
  start: 0,
  step: (accumulator, value) => {
    console.log(`step called: accumulator=${accumulator}, value=${value}`);

    if (value === 3) {
      console.log(
        "  -> returning undefined (should keep accumulator unchanged)",
      );
      return undefined;
    }

    const result = accumulator + value;
    console.log(`  -> returning ${result}`);
    return result;
  },
});

// Execute the aggregate
console.log("\nRunning aggregate query...");
const result = db.prepare("SELECT test_sum(value) as sum FROM test").get();
console.log("\nFinal result:", result);

// Expected behavior (like better-sqlite3):
// - accumulator=0, value=1 -> returns 1
// - accumulator=1, value=2 -> returns 3
// - accumulator=3, value=3 -> returns undefined (keeps 3)
// - accumulator=3, value=4 -> returns 7
// - accumulator=7, value=5 -> returns 12
// Final result: { sum: 12 }

// Actual behavior with node:sqlite bug:
// - accumulator=0, value=1 -> returns 1
// - accumulator=1, value=2 -> returns 3
// - accumulator=3, value=3 -> returns undefined (stores undefined!)
// - accumulator=undefined, value=4 -> returns NaN
// - accumulator=NaN, value=5 -> returns NaN
// Final result: { sum: null }

db.close();

// If you have better-sqlite3 installed, you can compare:
console.log("\n---\nFor comparison with better-sqlite3 (if installed):");
try {
  const Database = require("better-sqlite3");
  const db2 = new Database(":memory:");

  db2.exec("CREATE TABLE test (value INTEGER)");
  db2.exec("INSERT INTO test VALUES (1), (2), (3), (4), (5)");

  db2.aggregate("test_sum", {
    start: 0,
    step: (accumulator, value) => {
      if (value === 3) return undefined;
      return accumulator + value;
    },
  });

  const result2 = db2.prepare("SELECT test_sum(value) as sum FROM test").get();
  console.log("better-sqlite3 result:", result2); // { sum: 12 }
  db2.close();
} catch (e) {
  console.log("(better-sqlite3 not installed for comparison)");
}

geeksilva97 · 2025-06-21T19:53:19Z

Thanks for that, @mceachen . We should also note this in the documentation.

geeksilva97 · 2025-06-21T19:54:11Z

Would a change like this be a semver-major, @jasnell @cjihrig ?

mceachen · 2025-06-21T20:12:32Z

Would a change like this be a semver-major, @jasnell @cjihrig ?

IMHO I felt this was more of a documentation and bugfix sort of thing -- semver-minor or even just semver-patch.

mceachen · 2025-06-21T20:13:24Z

Thanks for that, @mceachen . We should also note this in the documentation.

Would it be helpful for me to do that? Should I do that in another PR, or amend this one?

geeksilva97 · 2025-06-21T20:13:49Z

Thanks for that, @mceachen . We should also note this in the documentation.

Would it be helpful for me to do that? Should I do that in another PR, or amend this one?

Can do in this same PR.

…lues do

mceachen · 2025-06-21T20:28:33Z

$ node --experimental-sqlite
Welcome to Node.js v24.1.0.
Type ".help" for more information.
> const { DatabaseSync } = require('node:sqlite');
undefined
> (node:655187) ExperimentalWarning: SQLite is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

> const db = new DatabaseSync(':memory:');
undefined
> db.exec(`
|   CREATE TABLE t3(x, y);
|   INSERT INTO t3 VALUES ('a', 4),
|                         ('b', 5),
|                         ('c', 3),
|                         ('d', 8),
|                         ('e', 1),
|                         ('f', -1);
| `);
undefined
> db.aggregate('sum_positive', {
|   start: 0,
|   step: (acc, value) => value < 0 ? undefined : acc + value, // Skip negative values
| });
undefined
> 
> db.prepare('SELECT sum_positive(y) as total FROM t3').get(); // { total: 21 }
[Object: null prototype] { total: null }

mrm@speedy:~/src/node$ ./out/Release/node --experimental-sqlite
Welcome to Node.js v25.0.0-pre.
Type ".help" for more information.
> const { DatabaseSync } = require('node:sqlite');
undefined
> 
> const db = new DatabaseSync(':memory:');
undefined
> db.exec(`
|   CREATE TABLE t3(x, y);
|   INSERT INTO t3 VALUES ('a', 4),
|                         ('b', 5),
|                         ('c', 3),
|                         ('d', 8),
|                         ('e', 1),
|                         ('f', -1);
| `);
undefined
> 
> db.aggregate('sum_positive', {
|   start: 0,
|   step: (acc, value) => value < 0 ? undefined : acc + value, // Skip negative values
| });
undefined
> 
> db.prepare('SELECT sum_positive(y) as total FROM t3').get(); // { total: 21 }
[Object: null prototype] { total: 21 }

…inter happy

codecov · 2025-06-21T21:24:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.07%. Comparing base (c3b9868) to head (ff8bf07).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58780      +/-   ##
==========================================
- Coverage   90.13%   90.07%   -0.06%     
==========================================
  Files         640      640              
  Lines      188294   188289       -5     
  Branches    36923    36917       -6     
==========================================
- Hits       169712   169598     -114     
- Misses      11304    11405     +101     
- Partials     7278     7286       +8

Files with missing lines	Coverage Δ
src/node_sqlite.cc	`80.82% <100.00%> (+0.02%)`	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Jun 21, 2025

sqlite: fix linting issues in aggregate tests

1ad4c3d

Fix trailing whitespace and arrow function formatting to pass Node.js JavaScript linting requirements.

docs(sqlite): explain what null and undefined step function return va…

39277dc

…lues do

docs(sqlite): wrap the the fat arrow return with parens to make the l…

ff8bf07

…inter happy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

sqlite: fix aggregate step function undefined handling #58780

sqlite: fix aggregate step function undefined handling #58780

mceachen commented Jun 21, 2025

Uh oh!

nodejs-github-bot commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

geeksilva97 commented Jun 21, 2025

Uh oh!

geeksilva97 commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

geeksilva97 commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

codecov bot commented Jun 21, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

sqlite: fix aggregate step function undefined handling #58780

Are you sure you want to change the base?

sqlite: fix aggregate step function undefined handling #58780

Conversation

mceachen commented Jun 21, 2025

Uh oh!

nodejs-github-bot commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

geeksilva97 commented Jun 21, 2025

Uh oh!

geeksilva97 commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

geeksilva97 commented Jun 21, 2025

Uh oh!

mceachen commented Jun 21, 2025

Uh oh!

codecov bot commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

codecov bot commented Jun 21, 2025 •

edited

Loading