feat(sqlite): add `StatementSync.prototype.iterate` method by tpoisseau · Pull Request #54213 · nodejs/node · GitHub

Hello,

I wanted an iterate method for StatementSync. An SQL query can return lot of rows, for memory efficiency it seems important we can fetch rows on demand, instead collect all in an array.

I'm not a C/C++ developer, so this was really challenging to create an object with a next callback with v8. I tried to obtain Iterator.prototype so the results extends Iterator. I did not found how to do it with v8.

I'm sure my code is not ready to be merged:

memory leak.
- follow similar algorithm than All
- variables for next and return callback are wrapped by v8::External
  added to iterable_iterator JS Object, accessed from context (.This())
no unit tests
- I did not found unit tests for sqlite module, I can do some in JS, but in C++...
- Maybe they are not mandatory for experimental modules ?
solution to have an iterable iterator from iterate() is a bit hacky. in js sqlite module I decorate the method to return Iterator.from(<result from cpp iterate>).
- If someone can help me to extend Iterator from v8 it could be great, else I'll keep the decorator.
documentation

I'm hoping to get some help here to finalize this PR, so that we all get a quality iterate method.

Manual test:

// % ./node --experimental-sqlite
sqlite = require('node:sqlite');
db = new sqlite.DatabaseSync(':memory:');
stmt = db.prepare(`WITH cte(a) AS (VALUES (1), (2), (3)) SELECT * FROM cte`);
iterator = stmt.iterate();
// > Object [Iterator] {}
iterator.map(({a}) => a).map(x => x*x).toArray();
// > [ 1, 4, 9 ]

So it's properly integrated with JS iterator protocol, it's an iterable iterator, it's integrated with IteratoHelpers.

add test in tests/parallel

Tests are added, I did not found better approach than Iterator.from, (SafeArrayIterator is not working for this case).
So I consider the PR is ready for a complete review.

Best regards,

Codecov Report

Attention: Patch coverage is 65.32258% with 43 lines in your changes missing coverage. Please review.

Project coverage is 87.99%. Comparing base (f270462) to head (3f3b946).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	65.32%	30 Missing and 13 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54213      +/-   ##
==========================================
- Coverage   88.49%   87.99%   -0.51%     
==========================================
  Files         653      653              
  Lines      187728   187852     +124     
  Branches    36182    35885     -297     
==========================================
- Hits       166136   165292     -844     
- Misses      14814    15729     +915     
- Partials     6778     6831      +53

Files with missing lines	Coverage Δ
src/node_sqlite.h	70.00% <ø> (ø)
src/node_sqlite.cc	81.17% <65.32%> (-2.65%)	⬇️

... and 92 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) #54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) #55604 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) #55890 net: * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) #56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) #55545 sqlite: * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) #54213 PR-URL: #56119

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) #54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) #55604 doc: * add LJHarb to collaborators (Jordan Harband) #56132 net: * (SEMVER-MINOR) add BlockList.isBlockList(value) (James M Snell) #56078 * (SEMVER-MINOR) support blockList in net.connect (theanarkh) #56075 * (SEMVER-MINOR) support blockList in net.Server (theanarkh) #56079 * (SEMVER-MINOR) add SocketAddress.parse (James M Snell) #56076 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) #55545 sqlite: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) #55890 * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) #54213 PR-URL: #56119

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) nodejs#55604 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890 net: * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) nodejs#56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) nodejs#55545 sqlite: * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213 PR-URL: #31

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) #54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) #55604 doc: * add LJHarb to collaborators (Jordan Harband) #56132 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) #55890 net: * (SEMVER-MINOR) support blocklist in net.connect (theanarkh) #56075 * (SEMVER-MINOR) add SocketAddress.parse (James M Snell) #56076 * (SEMVER-MINOR) add net.BlockList.isBlockList(value) (James M Snell) #56078 * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) #56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) #55545 sqlite: * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) #54213 PR-URL: #56119

PR-URL: #54213 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) nodejs#55604 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890 net: * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) nodejs#56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) nodejs#55545 sqlite: * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213 PR-URL: #32

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) nodejs#55604 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890 net: * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) nodejs#56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) nodejs#55545 sqlite: * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213 PR-URL: #33

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) #54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) #55604 crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 dgram: * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 * move typescript support to active development (Marco Ippolito) #55536 * add LJHarb to collaborators (Jordan Harband) #56132 * (SEMVER-MINOR) add report version and history section (Chengzhong Wu) #55697 * (SEMVER-MINOR) sort --report-exclude alphabetically (Rafael Gonzaga) #55697 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) #55890 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) #55282 * (SEMVER-MINOR) simplify `findPackageJSON` implementation (Antoine du Hamel) #55412 * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412 net: * (SEMVER-MINOR) support blocklist in net.connect (theanarkh) #56075 * (SEMVER-MINOR) add SocketAddress.parse (James M Snell) #56076 * (SEMVER-MINOR) add net.BlockList.isBlockList(value) (James M Snell) #56078 * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) #56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) #55545 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) #54213 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) #55697 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) #55589 PR-URL: #56329

PR-URL: #54213 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) #54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) #55604 crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 dgram: * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 * move typescript support to active development (Marco Ippolito) #55536 * add LJHarb to collaborators (Jordan Harband) #56132 * (SEMVER-MINOR) add report version and history section (Chengzhong Wu) #56130 * (SEMVER-MINOR) sort --report-exclude alphabetically (Rafael Gonzaga) #55788 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) #55890 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) #55282 * (SEMVER-MINOR) simplify `findPackageJSON` implementation (Antoine du Hamel) #55412 * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412 net: * (SEMVER-MINOR) support blocklist in net.connect (theanarkh) #56075 * (SEMVER-MINOR) add SocketAddress.parse (James M Snell) #56076 * (SEMVER-MINOR) add net.BlockList.isBlockList(value) (James M Snell) #56078 * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) #56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) #55545 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) #54213 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) #55697 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) #55589 PR-URL: #56329

Notable changes: assert: * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) #54630 cli: * (SEMVER-MINOR) implement --trace-env and --trace-env-[js|native]-stack (Joyee Cheung) #55604 crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 dgram: * (SEMVER-MINOR) support blocklist in udp (theanarkh) #56087 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 * move typescript support to active development (Marco Ippolito) #55536 * add LJHarb to collaborators (Jordan Harband) #56132 * (SEMVER-MINOR) add report version and history section (Chengzhong Wu) #56130 * (SEMVER-MINOR) sort --report-exclude alphabetically (Rafael Gonzaga) #55788 doc,lib,src,test: * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) #55890 module: * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) #55282 net: * (SEMVER-MINOR) support blocklist in net.connect (theanarkh) #56075 * (SEMVER-MINOR) add SocketAddress.parse (James M Snell) #56076 * (SEMVER-MINOR) add net.BlockList.isBlockList(value) (James M Snell) #56078 * (SEMVER-MINOR) support blocklist for net.Server (theanarkh) #56079 process: * (SEMVER-MINOR) deprecate `features.{ipv6,uv}` and `features.tls_*` (René) #55545 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) #54213 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) #55697 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) #55589 PR-URL: #56329

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 5, 2024

tpoisseau force-pushed the feat-sqlite-iterate branch 2 times, most recently from 16e00d0 to 762e0dc Compare August 5, 2024 08:55

avivkeller added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 5, 2024

tpoisseau force-pushed the feat-sqlite-iterate branch from 762e0dc to a73d026 Compare August 7, 2024 18:14

himself65 requested a review from cjihrig August 9, 2024 06:40

himself65 requested changes Aug 9, 2024

View reviewed changes

Comment thread lib/sqlite.js Outdated

tpoisseau force-pushed the feat-sqlite-iterate branch 2 times, most recently from 406165d to 63a780e Compare August 14, 2024 07:02

tpoisseau marked this pull request as ready for review August 14, 2024 09:05

cjihrig reviewed Aug 15, 2024

View reviewed changes

Comment thread lib/sqlite.js Outdated

tniessen reviewed Aug 23, 2024

View reviewed changes

Comment thread lib/sqlite.js Outdated

cjihrig reviewed Aug 23, 2024

View reviewed changes

Comment thread src/node_sqlite.cc Outdated

Comment thread src/node_sqlite.cc Outdated

tpoisseau force-pushed the feat-sqlite-iterate branch from fc82998 to a413954 Compare August 25, 2024 12:22

targos reviewed Aug 25, 2024

View reviewed changes

Comment thread src/node_sqlite.cc Outdated

jasnell reviewed Aug 25, 2024

View reviewed changes

Comment thread src/node_sqlite.cc Outdated

targos reviewed Aug 26, 2024

View reviewed changes

Comment thread src/node_sqlite.cc Outdated

Comment thread src/node_sqlite.cc Outdated

targos reviewed Aug 26, 2024

View reviewed changes

Comment thread src/env_properties.h Outdated

jasnell reviewed Aug 27, 2024

View reviewed changes

Comment thread src/node_sqlite.cc Outdated

jasnell reviewed Aug 27, 2024

View reviewed changes

Comment thread src/node_sqlite.cc Outdated

github-actions Bot mentioned this pull request Nov 30, 2024

2024-12-01, Version 23.4.0 (Current), @aduh95 aduh95/node#30

Closed

nodejs-github-bot mentioned this pull request Dec 3, 2024

2024-12-10, Version 23.4.0 (Current), @targos #56119

Merged

github-actions Bot mentioned this pull request Dec 6, 2024

2024-12-11, Version 23.4.0 (Current), @aduh95 aduh95/node#31

Closed

aduh95 pushed a commit that referenced this pull request Dec 10, 2024

sqlite: add StatementSync.prototype.iterate method

30628eb

PR-URL: #54213 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

github-actions Bot mentioned this pull request Dec 11, 2024

2024-12-12, Version 23.4.0 (Current) aduh95/node#32

Closed

github-actions Bot mentioned this pull request Dec 11, 2024

2024-12-12, Version 23.4.0 (Current) aduh95/node#33

Closed

ruyadorno mentioned this pull request Dec 20, 2024

2025-01-07, Version 22.13.0 'Jod' (LTS) #56329

Merged

ruyadorno pushed a commit that referenced this pull request Jan 5, 2025

sqlite: add StatementSync.prototype.iterate method

b4041e5

PR-URL: #54213 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

feat(sqlite): add `StatementSync.prototype.iterate` method by tpoisseau · Pull Request #54213 · nodejs/node · GitHub

Uh oh!

Uh oh!

feat(sqlite): add StatementSync.prototype.iterate method - #54213

feat(sqlite): add StatementSync.prototype.iterate method#54213
tpoisseau wants to merge 2 commits into
nodejs:mainfrom
tpoisseau:feat-sqlite-iterate

tpoisseau commented Aug 5, 2024 •
edited
Loading

Uh oh!

Uh oh!

himself65 left a comment

Uh oh!

Uh oh!

tpoisseau commented Aug 14, 2024

Uh oh!

codecov Bot commented Aug 14, 2024 •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

feat(sqlite): add `StatementSync.prototype.iterate` method by tpoisseau · Pull Request #54213 · nodejs/node · GitHub

Uh oh!

Uh oh!

feat(sqlite): add StatementSync.prototype.iterate method - #54213

feat(sqlite): add StatementSync.prototype.iterate method#54213tpoisseau wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom tpoisseau:feat-sqlite-iteratetpoisseau/node:feat-sqlite-iterateCopy head branch name to clipboard

Conversation

tpoisseau commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himself65 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tpoisseau commented Aug 14, 2024

Uh oh!

codecov Bot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

feat(sqlite): add StatementSync.prototype.iterate method#54213
tpoisseau wants to merge 2 commits into
nodejs:mainfrom
tpoisseau:feat-sqlite-iterate

tpoisseau commented Aug 5, 2024 •
edited
Loading

codecov Bot commented Aug 14, 2024 •
edited
Loading