Skip to content

sqlite: fix aggregate step function undefined handling #58780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions doc/api/sqlite.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ Registers a new aggregate function with the SQLite database. This method is a wr
function is initialized. When a {Function} is passed the identity will be its return value.
* `step` {Function} The function to call for each row in the aggregation. The
function receives the current state and the row value. The return value of
this function should be the new state.
this function should be the new state. Returning `undefined` preserves the
current state unchanged, while returning `null` explicitly sets the state to `null`.
* `result` {Function} The function to call to get the result of the
aggregation. The function receives the final state and should return the
result of the aggregation.
Expand All @@ -178,15 +179,16 @@ db.exec(`
('b', 5),
('c', 3),
('d', 8),
('e', 1);
('e', 1),
('f', -1);
`);

db.aggregate('sumint', {
db.aggregate('sum_positive', {
start: 0,
step: (acc, value) => acc + value,
step: (acc, value) => (value < 0 ? undefined : acc + value), // Skip negative values
});

db.prepare('SELECT sumint(y) as total FROM t3').get(); // { total: 21 }
db.prepare('SELECT sum_positive(y) as total FROM t3').get(); // { total: 21 }
```

```mjs
Expand All @@ -199,15 +201,16 @@ db.exec(`
('b', 5),
('c', 3),
('d', 8),
('e', 1);
('e', 1),
('f', -1);
`);

db.aggregate('sumint', {
db.aggregate('sum_positive', {
start: 0,
step: (acc, value) => acc + value,
step: (acc, value) => (value < 0 ? undefined : acc + value), // Skip negative values
});

db.prepare('SELECT sumint(y) as total FROM t3').get(); // { total: 21 }
db.prepare('SELECT sum_positive(y) as total FROM t3').get(); // { total: 21 }
```

### `database.close()`
Expand Down
4 changes: 3 additions & 1 deletion src/node_sqlite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,9 @@ class CustomAggregate {
return;
}

agg->value.Reset(isolate, ret);
if (!ret->IsUndefined()) {
agg->value.Reset(isolate, ret);
}
}

static inline void xValueBase(sqlite3_context* ctx, bool is_final) {
Expand Down
80 changes: 80 additions & 0 deletions test/parallel/test-sqlite-aggregate-function.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -413,3 +413,83 @@ test('throws an error when trying to use as windown function but didn\'t provide
message: 'sumint() may not be used as a window function'
});
});

describe('step function undefined handling', () => {
test('preserves accumulator when step returns undefined', (t) => {
const db = new DatabaseSync(':memory:');
t.after(() => db.close());

db.exec('CREATE TABLE numbers (n INTEGER)');
db.exec('INSERT INTO numbers VALUES (1), (2), (3), (4), (5)');

db.aggregate('sum_skip_three', {
start: 0,
step: (sum, n) => (n === 3 ? undefined : sum + n)
});

const result = db.prepare('SELECT sum_skip_three(n) as total FROM numbers').get();
t.assert.strictEqual(result.total, 12); // Should sum 1+2+4+5, skipping 3
});

test('distinguishes between undefined and null returns', (t) => {
const db = new DatabaseSync(':memory:');
t.after(() => db.close());

db.exec('CREATE TABLE test (val INTEGER)');
db.exec('INSERT INTO test VALUES (1), (2), (3)');

// Test that null is stored when explicitly returned
db.aggregate('explicit_null', {
start: 10,
step: (acc, val) => {
if (val === 3) return null; // Explicitly return null on last value
if (val === 2) return undefined; // Skip middle value
return acc + val;
}
});

const result = db.prepare('SELECT explicit_null(val) as result FROM test').get();
t.assert.strictEqual(result.result, null);

// Test that undefined preserves the accumulator
db.aggregate('preserve_on_undefined', {
start: 10,
step: (acc, val) => {
if (val === 2) return undefined; // Return undefined to preserve
return acc + val;
}
});

const result2 = db.prepare('SELECT preserve_on_undefined(val) as result FROM test').get();
t.assert.strictEqual(result2.result, 14); // Should preserve accumulator: 10+1+3=14
});

test('handles implicit undefined returns', (t) => {
const db = new DatabaseSync(':memory:');
t.after(() => db.close());

db.exec('CREATE TABLE test (val INTEGER)');
db.exec('INSERT INTO test VALUES (1), (2), (3)');

// Test step function with implicit undefined return
db.aggregate('implicit_undefined', {
start: 10,
step: (acc, val) => {
if (val === 2) return; // Implicit undefined
return acc * val;
}
});

const result = db.prepare('SELECT implicit_undefined(val) as result FROM test').get();
t.assert.strictEqual(result.result, 30); // Should calculate 10*1*3, skipping 2

// Test empty function body (always returns undefined)
db.aggregate('empty_step', {
start: 42,
step: (acc, val) => {} // Always returns undefined
});

const result2 = db.prepare('SELECT empty_step(val) as result FROM test').get();
t.assert.strictEqual(result2.result, 42); // Start value should be preserved
});
});
Loading