Skip to content
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

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

Closed
wants to merge 2 commits into from

Conversation

tpoisseau
Copy link
Contributor

@tpoisseau tpoisseau commented Aug 5, 2024

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.

@nodejs-github-bot 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 tpoisseau force-pushed the feat-sqlite-iterate branch 2 times, most recently from 16e00d0 to 762e0dc Compare August 5, 2024 08:55
@RedYetiDev RedYetiDev added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 5, 2024
@himself65 himself65 requested a review from cjihrig August 9, 2024 06:40
Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test in tests/parallel

lib/sqlite.js Outdated Show resolved Hide resolved
@tpoisseau tpoisseau force-pushed the feat-sqlite-iterate branch 2 times, most recently from 406165d to 63a780e Compare August 14, 2024 07:02
@tpoisseau tpoisseau marked this pull request as ready for review August 14, 2024 09:05
@tpoisseau
Copy link
Contributor Author

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,

Copy link

codecov bot commented Aug 14, 2024

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:

lib/sqlite.js Outdated Show resolved Hide resolved
lib/sqlite.js Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
tpoisseau added a commit to tpoisseau/node that referenced this pull request Aug 24, 2024
tpoisseau added a commit to tpoisseau/node that referenced this pull request Aug 25, 2024
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
tpoisseau added a commit to tpoisseau/node that referenced this pull request Aug 25, 2024
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
tpoisseau added a commit to tpoisseau/node that referenced this pull request Aug 26, 2024
src/env_properties.h Outdated Show resolved Hide resolved
tpoisseau added a commit to tpoisseau/node that referenced this pull request Aug 26, 2024
Refs: nodejs#54213 (comment)

Co-authored-by: Michaël Zasso <targos@protonmail.com>
src/node_sqlite.cc Outdated Show resolved Hide resolved
tpoisseau added a commit to tpoisseau/node that referenced this pull request Aug 28, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Nov 22, 2024

https://github.com/nodejs/build/issues/55929

Thanks. By the way, that link seems to 404.

@targos
Copy link
Member

targos commented Nov 22, 2024

Correct link: #55929

@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 22, 2024
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
PR-URL: nodejs#54213
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request Nov 26, 2024
PR-URL: #54213
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 added a commit to aduh95/node that referenced this pull request Nov 28, 2024
Notable changes:

assert:
  * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630
doc,lib,src,test:
  * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890
sqlite:
  * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213

PR-URL: #25
aduh95 added a commit to aduh95/node that referenced this pull request Nov 28, 2024
Notable changes:

assert:
  * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630
doc,lib,src,test:
  * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890
sqlite:
  * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213

PR-URL: #26
aduh95 added a commit to aduh95/node that referenced this pull request Nov 28, 2024
Notable changes:

assert:
  * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630
doc,lib,src,test:
  * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890
sqlite:
  * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213

PR-URL: #27
aduh95 added a commit to aduh95/node that referenced this pull request Nov 28, 2024
Notable changes:

assert:
  * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630
doc,lib,src,test:
  * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890
sqlite:
  * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213

PR-URL: #28
aduh95 added a commit to aduh95/node that referenced this pull request Nov 30, 2024
Notable changes:

assert:
  * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630
doc,lib,src,test:
  * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890
sqlite:
  * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213

PR-URL:
aduh95 added a commit to aduh95/node that referenced this pull request Nov 30, 2024
Notable changes:

assert:
  * (SEMVER-MINOR) add partialDeepStrictEqual (Giovanni Bucci) nodejs#54630
doc,lib,src,test:
  * (SEMVER-MINOR) unflag sqlite module (Colin Ihrig) nodejs#55890
sqlite:
  * (SEMVER-MINOR) add `StatementSync.prototype.iterate` method (tpoisseau) nodejs#54213

PR-URL: #30
github-actions bot pushed a commit that referenced this pull request Dec 3, 2024
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: TODO
github-actions bot pushed a commit that referenced this pull request Dec 3, 2024
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
targos pushed a commit that referenced this pull request Dec 3, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants