sqlite: add aggregate function #56600

geeksilva97 · 2025-01-14T19:05:19Z

Closes #56511

inspired by https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#aggregatename-options---this

src/env_properties.h

src/node_sqlite.cc

cjihrig · 2025-01-20T17:30:32Z

Also, tests and docs are needed.

cjihrig · 2025-03-11T15:21:06Z

@geeksilva97 are you still planning to work on this?

geeksilva97 · 2025-03-11T15:44:17Z

@geeksilva97 are you still planning to work on this?

Hi @cjihrig . Yes, I plan to continue this. The last few weeks have been tough though. If nobody beats me I plan to get this done by April.

codecov · 2025-03-19T14:27:09Z

Codecov Report

Attention: Patch coverage is 80.48780% with 48 lines in your changes missing coverage. Please review.

Project coverage is 90.22%. Comparing base (9aa57bf) to head (484d68d).
Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	80.48%	25 Missing and 23 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56600      +/-   ##
==========================================
- Coverage   90.24%   90.22%   -0.02%     
==========================================
  Files         630      630              
  Lines      184990   185424     +434     
  Branches    36216    36347     +131     
==========================================
+ Hits       166948   167307     +359     
- Misses      11003    11016      +13     
- Partials     7039     7101      +62

Files with missing lines	Coverage Δ
src/node_sqlite.h	`63.63% <ø> (ø)`
src/node_sqlite.cc	`80.47% <80.48%> (-0.05%)`	⬇️

... and 48 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.

geeksilva97 · 2025-03-19T14:33:58Z

I got an implementation that works. I need to add tests and clean up the code.

doc/api/sqlite.md

src/node_sqlite.cc

geeksilva97 · 2025-04-07T13:09:25Z

It needs re-approval because I accepted Jasmes' suggestions.

nodejs-github-bot · 2025-04-07T13:10:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/56600
✔  Done loading data for nodejs/node/pull/56600
----------------------------------- PR info ------------------------------------
Title      sqlite: add aggregate function (#56600)
Author     Edy Silva <[email protected]> (@geeksilva97)
Branch     geeksilva97:sqlite-aggregate-function -> nodejs:main
Labels     c++, author ready, needs-ci, commit-queue-squash, sqlite
Commits    3
 - sqlite,doc,test: add aggregate function
 - apply review suggestions
 - apply review suggestion
Committers 2
 - Edy Silva <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56600
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56600
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - sqlite,doc,test: add aggregate function
   ⚠  - apply review suggestions
   ⚠  - apply review suggestion
   ℹ  This PR was created on Tue, 14 Jan 2025 19:05:19 GMT
   ✔  Approvals: 2
   ✔  - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/56600#pullrequestreview-2744917456
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56600#pullrequestreview-2745190851
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-04-06T17:31:02Z: https://ci.nodejs.org/job/node-test-pull-request/66092/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - apply review suggestion
- Querying data for job/node-test-pull-request/66092/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/14310034479

test/parallel/test-sqlite-aggregate-function.mjs

src/node_sqlite.cc

nodejs-github-bot · 2025-04-07T16:48:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66108/

nodejs-github-bot · 2025-04-08T01:38:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66122/

nodejs-github-bot · 2025-04-08T13:14:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66131/

nodejs-github-bot · 2025-04-08T21:01:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66147/

nodejs-github-bot · 2025-04-09T02:04:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66152/

nodejs-github-bot · 2025-04-09T02:49:44Z

Landed in 437c6aa

targos · 2025-04-09T12:53:58Z

src/node_sqlite.cc

+    bool lossless;
+    int64_t as_int = value.As<BigInt>()->Int64Value(&lossless);
+    if (!lossless) {
+      sqlite3_result_error(ctx, "BigInt value is too large for SQLite", -1);


Today we got a Coverity report for this line:

*** CID 519229: Memory - corruptions (BAD_FREE) /src/node_sqlite.cc: 189 in node::sqlite::JSValueToSQLiteResult(v8::Isolate *, sqlite3_context *, v8::Local<v8::Value>)() 183 ArrayBufferViewContents<uint8_t> buf(value); 184 sqlite3_result_blob(ctx, buf.data(), buf.length(), SQLITE_TRANSIENT); 185 } else if (value->IsBigInt()) { 186 bool lossless; 187 int64_t as_int = [value.As](http://value.as/)<BigInt>()->Int64Value(&lossless); 188 if (!lossless) { >>> CID 519229: Memory - corruptions (BAD_FREE) >>> "sqlite3_result_error" frees array ""BigInt value is too large for SQLite"". 189 sqlite3_result_error(ctx, "BigInt value is too large for SQLite", -1); 190 return; 191 } 192 sqlite3_result_int64(ctx, as_int); 193 } else if (value->IsPromise()) { 194 sqlite3_result_error(

Is it something I can check locally?

It's weird that it was not complaining before. It was inline in the code; I just moved it to a method. Maybe adding inline would fix it?

It might have complained before already. I just noticed the notification in my emails.

Some details from the coverity interface:

It's possibly a false-positive. If you can confirm that, I will mark it as such.

I can't confirm with absolute certainty that this is a false positive, but I also can't find anything (online examples, etc.) to support what coverity is reporting.

This paragraph from the docs makes me think this is not an issue:

The sqlite3_result_error() and sqlite3_result_error16() functions cause the implemented SQL function to throw an exception. SQLite uses the string pointed to by the 2nd parameter of sqlite3_result_error() or sqlite3_result_error16() as the text of an error message. SQLite interprets the error message string from sqlite3_result_error() as UTF-8. SQLite interprets the string from sqlite3_result_error16() as UTF-16 using the same byte-order determination rules as sqlite3_bind_text16(). If the third parameter to sqlite3_result_error() or sqlite3_result_error16() is negative then SQLite takes as the error message all text up through the first zero character. If the third parameter to sqlite3_result_error() or sqlite3_result_error16() is non-negative then SQLite takes that many bytes (not characters) from the 2nd parameter as the error message. The sqlite3_result_error() and sqlite3_result_error16() routines make a private copy of the error message text before they return. Hence, the calling function can deallocate or modify the text after they return without harm. The sqlite3_result_error_code() function changes the error code returned by SQLite as a result of an error in a function. By default, the error code is SQLITE_ERROR. A subsequent call to sqlite3_result_error() or sqlite3_result_error16() resets the error code to SQLITE_ERROR.

the calling function can deallocate or modify the text after they return without harm
If we can do that, it's certainly not freed by sqlite. Marking as a false-positive, thanks!

PR-URL: nodejs#56600 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

src/node_sqlite.cc

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 Jan 14, 2025

targos reviewed Jan 20, 2025

View reviewed changes

src/env_properties.h Outdated Show resolved Hide resolved

jasnell reviewed Jan 20, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

cjihrig reviewed Jan 20, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

src/node_sqlite.cc Outdated Show resolved Hide resolved

geeksilva97 added the wip Issues and PRs that are still a work in progress. label Mar 15, 2025

geeksilva97 force-pushed the sqlite-aggregate-function branch 7 times, most recently from beaf983 to 70dff34 Compare March 19, 2025 13:00

geeksilva97 marked this pull request as ready for review March 19, 2025 13:01

geeksilva97 force-pushed the sqlite-aggregate-function branch from 70dff34 to f4e4869 Compare March 19, 2025 13:17

geeksilva97 force-pushed the sqlite-aggregate-function branch 3 times, most recently from cd3f0aa to 22da3ad Compare March 20, 2025 19:56

jasnell reviewed Mar 20, 2025

View reviewed changes

doc/api/sqlite.md Outdated Show resolved Hide resolved

jasnell reviewed Mar 20, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

cjihrig reviewed Mar 20, 2025

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

src/node_sqlite.cc Show resolved Hide resolved

geeksilva97 changed the title ~~sqlite: add aggregate function~~ [wip] sqlite: add aggregate function Mar 20, 2025

geeksilva97 force-pushed the sqlite-aggregate-function branch from 22da3ad to 9badd9b Compare March 20, 2025 20:59

geeksilva97 mentioned this pull request Mar 21, 2025

sqlite,src: refactor sqlite value conversion #57571

Merged

geeksilva97 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 7, 2025

geeksilva97 requested review from jasnell and cjihrig April 7, 2025 13:09

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 7, 2025

cjihrig reviewed Apr 7, 2025

View reviewed changes

test/parallel/test-sqlite-aggregate-function.mjs Outdated Show resolved Hide resolved

test/parallel/test-sqlite-aggregate-function.mjs Outdated Show resolved Hide resolved

src/node_sqlite.cc Show resolved Hide resolved

fix typos

484d68d

cjihrig approved these changes Apr 7, 2025

View reviewed changes

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2025

geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 8, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 8, 2025

cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 9, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025

nodejs-github-bot merged commit 437c6aa into nodejs:main Apr 9, 2025
63 checks passed

cjihrig mentioned this pull request Apr 9, 2025

node:sqlite: DatabaseSync.function wrapper for sqlite3_create_function_v2 should allow use of xStep and xFinal parameters #56510

Closed

targos reviewed Apr 9, 2025

View reviewed changes

JonasBa pushed a commit to JonasBa/node that referenced this pull request Apr 11, 2025

sqlite,doc,test: add aggregate function

50001ec

PR-URL: nodejs#56600 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

jakecastelli reviewed Apr 13, 2025

View reviewed changes

src/node_sqlite.cc Show resolved Hide resolved

cjihrig mentioned this pull request Apr 13, 2025

sqlite: add location method #57860

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqlite: add aggregate function #56600

sqlite: add aggregate function #56600

geeksilva97 commented Jan 14, 2025 •

edited

Loading

cjihrig commented Jan 20, 2025

cjihrig commented Mar 11, 2025

geeksilva97 commented Mar 11, 2025

codecov bot commented Mar 19, 2025 •

edited

Loading

geeksilva97 commented Mar 19, 2025

geeksilva97 commented Apr 7, 2025 •

edited

Loading

nodejs-github-bot commented Apr 7, 2025

nodejs-github-bot commented Apr 7, 2025

nodejs-github-bot commented Apr 8, 2025

nodejs-github-bot commented Apr 8, 2025

nodejs-github-bot commented Apr 8, 2025

nodejs-github-bot commented Apr 9, 2025

nodejs-github-bot commented Apr 9, 2025

targos Apr 9, 2025

geeksilva97 Apr 9, 2025

geeksilva97 Apr 9, 2025 •

edited

Loading

targos Apr 9, 2025

targos Apr 9, 2025

targos Apr 9, 2025 •

edited

Loading

cjihrig Apr 9, 2025

targos Apr 9, 2025

sqlite: add aggregate function #56600

sqlite: add aggregate function #56600

Conversation

geeksilva97 commented Jan 14, 2025 • edited Loading

cjihrig commented Jan 20, 2025

cjihrig commented Mar 11, 2025

geeksilva97 commented Mar 11, 2025

codecov bot commented Mar 19, 2025 • edited Loading

Codecov Report

geeksilva97 commented Mar 19, 2025

geeksilva97 commented Apr 7, 2025 • edited Loading

nodejs-github-bot commented Apr 7, 2025

nodejs-github-bot commented Apr 7, 2025

nodejs-github-bot commented Apr 8, 2025

nodejs-github-bot commented Apr 8, 2025

nodejs-github-bot commented Apr 8, 2025

nodejs-github-bot commented Apr 9, 2025

nodejs-github-bot commented Apr 9, 2025

targos Apr 9, 2025

Choose a reason for hiding this comment

geeksilva97 Apr 9, 2025

Choose a reason for hiding this comment

geeksilva97 Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

targos Apr 9, 2025

Choose a reason for hiding this comment

targos Apr 9, 2025

Choose a reason for hiding this comment

targos Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

cjihrig Apr 9, 2025

Choose a reason for hiding this comment

targos Apr 9, 2025

Choose a reason for hiding this comment

geeksilva97 commented Jan 14, 2025 •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

geeksilva97 commented Apr 7, 2025 •

edited

Loading

geeksilva97 Apr 9, 2025 •

edited

Loading

targos Apr 9, 2025 •

edited

Loading