node icon indicating copy to clipboard operation
node copied to clipboard

feat(sqlite): add `StatementSync.prototype.iterate` method

Open tpoisseau opened this issue 1 year ago • 4 comments

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:

  • [x] 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())
  • [x] 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 ?
  • [x] 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.
  • [x] 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.

tpoisseau avatar Aug 05 '24 08:08 tpoisseau

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,

tpoisseau avatar Aug 14 '24 09:08 tpoisseau

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 :warning:
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%) :arrow_down:

... and 92 files with indirect coverage changes

---- 🚨 Try these New Features:

codecov[bot] avatar Aug 14 '24 10:08 codecov[bot]

diff --git a/lib/net.js b/lib/net.js ดัชนี eda1e24cd3b8d7..be21c566610286 100644 --- ก/lib/net.js +++ บ/ลิ้ง/เน็ต.เจเอส @@ -35,8 +35,6 @@ คงที่ { การแบ่งหมายเลข คุณสมบัติการกำหนดวัตถุ วัตถุชุดต้นแบบของ

  • เร้กเอ็กซ์พี,
  • RegExpPrototypeExec, เครื่องหมาย, สัญลักษณ์ AsyncDispose สัญลักษณ์การกำจัด @@ -145,8 +143,6 @@ const { kTimeout } = require('internal/timers'); ค่า DEFAULT_IPV4_ADDR = '0.0.0.0'; ค่า DEFAULT_IPV6_ADDR = '::';

-const HOST_REGEXP = ใหม่ RegExp('^[a-zA-Z0-9-:%.]+$');

const noop = () => {};

const kPerfHooksNetConnectContext = สัญลักษณ์('kPerfHooksNetConnectContext'); @@ -2024,10 +2020,6 @@ Server.prototype.listen = ฟังก์ชัน(...args) { toNumber(args.length > 2 && args[2]); // (พอร์ต, โฮสต์, แบ็กล็อก)

ตัวเลือก = ตัวเลือก._handle || ตัวเลือก.handle || ตัวเลือก;

  • if (typeof options.host === 'string' && RegExpPrototypeExec(HOST_REGEXP, options.host) === null) {
  • โยน ERR_INVALID_ARG_VALUE ใหม่('โฮสต์', ตัวเลือก.โฮสต์);
  • const flags = getFlags(ตัวเลือก ipv6Only); // รีเฟรช id เพื่อทำให้การเรียกครั้งก่อนไม่ถูกต้อง นี้._listeningId++; ความแตกต่าง --git a/test/parallel/test-net-server-listen-options.js b/test/parallel/test-net-server-listen-options.js ดัชนี cec081430cf504..7e306af8ab082f 100644 --- a/test/parallel/test-net-server-listen-options.js +++ b/test/parallel/test-net-server-listen-options.js @@ -15,10 +15,6 @@ ฟังก์ชัน close() { this.close(); } // ทดสอบการฟัง({port}) net.createServer().ฟัง({ พอร์ต: 0 }) .on('กำลังฟัง', common.mustCall(ปิด));
  • // ทดสอบการฟัง (โฮสต์, พอร์ต}) บน ipv4
  • net.createServer().listen({ โฮสต์: '127.0.0.1', พอร์ต: '3000' }).on('listening', common.mustCall(ปิด));
  • // ทดสอบการฟัง (โฮสต์, พอร์ต}) บน ipv6
  • net.createServer().listen({ โฮสต์: '::', พอร์ต: '3001' }).on('listening', common.mustCall(ปิด));

// ทดสอบการฟัง (พอร์ต, cb) และการฟัง ({ พอร์ต }, cb) @@ -70,13 +66,6 @@ const listenOnPort = [ ชื่อ: 'TypeError', ข้อความ: /^อาร์กิวเมนต์ 'options' ต้องมีคุณสมบัติ "port" หรือ "path". ได้รับ .+$/, -

  • } ไม่เช่นนั้นถ้า typeof options.host === 'string' && !options.host.match(/^[a-zA-Z0-9-:%.]+$/)) {
  • ยืนยันการโยน(fn,
  • รหัส: 'ERR_INVALID_ARG_VALUE'
  • ชื่อ: 'TypeError',
  • ข้อความ: /^อาร์กิวเมนต์ 'โฮสต์' ไม่ถูกต้อง. ได้รับ .+$/,
  • } อื่น { ยืนยันการโยน(fn, - @@ -102,5 +91,4 @@ const listenOnPort = [ shouldFailToListen({ โฮสต์: 'localhost:3000' }); shouldFailToListen({ โฮสต์: { พอร์ต: 3000 } }); shouldFailToListen({ พิเศษ: true });
  • shouldFailToListen({ โฮสต์: '[::]', พอร์ต: 3000 });

Gguyzaza avatar Aug 28 '24 08:08 Gguyzaza

diff --git a/lib/net.js b/lib/net.js ดัชนี eda1e24cd3b8d7..be21c566610286 100644 --- ก/lib/net.js +++ บ/ลิ้ง/เน็ต.เจเอส @@ -35,8 +35,6 @@ คงที่ { การแบ่งหมายเลข คุณสมบัติการกำหนดวัตถุ วัตถุชุดต้นแบบของ

This user has been blocked.

Trott avatar Aug 28 '24 15:08 Trott

@tpoisseau are you still interested in pursuing this? If so, could you rebase to resolve conflicts, and remove any merge commits, as our automation does not handle those very well.

cjihrig avatar Nov 20 '24 17:11 cjihrig

Yes, I would like to continue this PR so it can be merged.

I tried to rebase my branch, seems it's not great ^^'. I'll restore to previous state and retry.

tpoisseau avatar Nov 21 '24 08:11 tpoisseau

Ok, I found a simple way to "squash" all my commits:

git reset --soft upstream/main
git add .
git commit
git push --force-with-lease

Interactive rebase was way to hard in this context (merge commits, lot of commits)

tpoisseau avatar Nov 21 '24 08:11 tpoisseau

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

nodejs-github-bot avatar Nov 21 '24 16:11 nodejs-github-bot

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

nodejs-github-bot avatar Nov 21 '24 18:11 nodejs-github-bot

coverage-windows is broken because of a Visual Studio update. See https://github.com/nodejs/build/issues/55929

targos avatar Nov 22 '24 07:11 targos

Landed in bc701e90f3b8

targos avatar Nov 22 '24 09:11 targos

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

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

cjihrig avatar Nov 22 '24 14:11 cjihrig

Correct link: https://github.com/nodejs/node/issues/55929

targos avatar Nov 22 '24 14:11 targos