FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: refactor prepared statement iterator by cjihrig · Pull Request #57569 · nodejs/node · GitHub

/ node Public

sqlite: refactor prepared statement iterator - #57569

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
cjihrig:iterator
Apr 3, 2025
ConversationCommits (1)ChecksFiles changed
Merged

sqlite: refactor prepared statement iterator#57569
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
cjihrig:iterator

Conversation

cjihrig commented Mar 20, 2025

Copy link
Copy Markdown
Contributor

This commit refactors the StatementSync iterator implementation in two primary ways:

  • The iterator internal state is no longer exposed to JavaScript.
  • The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493

sqlite: refactor prepared statement iterator
928e4a2 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: nodejs#57493

nodejs-github-bot commented Mar 20, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

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 Mar 20, 2025
cjihrig mentioned this pull request Mar 20, 2025
Merged
jasnell reviewed Mar 20, 2025
View reviewed changes
Comment thread src/node_sqlite.cc
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsFinalized(), "statement has been finalized");
Isolate* isolate = env->isolate();
LocalVector<Name> keys(isolate, {env->done_string(), env->value_string()});

jasnell Mar 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit... LocalVector here might be overkill when a simple array would do. Non-blocking tho.

codecov Bot commented Mar 20, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 84.21053% with 15 lines in your changes missing coverage. Please review.

Project coverage is 90.25%. Comparing base (5a7b7d2) to head (928e4a2).
Report is 586 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 86.02% 2 Missing and 11 partials ⚠️
src/node_sqlite.h 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57569      +/-   ##
==========================================
+ Coverage   90.24%   90.25%   +0.01%     
==========================================
  Files         630      630              
  Lines      185013   184989      -24     
  Branches    36241    36233       -8     
==========================================
+ Hits       166966   166971       +5     
+ Misses      10998    10973      -25     
+ Partials     7049     7045       -4
Files with missing lines Coverage Δ
src/node_sqlite.h 63.63% <0.00%> (-6.37%) ⬇️
src/node_sqlite.cc 80.79% <86.02%> (+2.27%) ⬆️

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

tpoisseau commented Mar 21, 2025

Copy link
Copy Markdown
Contributor

🙏 This implementation is better than my original one

gurgunday reviewed Mar 21, 2025
View reviewed changes

gurgunday left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm!

nodejs-github-bot commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

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

cjihrig commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

I'm not sure if anyone else has permission to resume the CI, but I don't.

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

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

gurgunday added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 2, 2025

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

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

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. labels Apr 3, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2025
nodejs-github-bot merged commit 771b682 into nodejs:main Apr 3, 2025

nodejs-github-bot commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

Landed in 771b682

cjihrig deleted the iterator branch April 3, 2025 14:16
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
sqlite: refactor prepared statement iterator
51557d1 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
sqlite: refactor prepared statement iterator
26fa594 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
sqlite: refactor prepared statement iterator
c4a2997 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
sqlite: refactor prepared statement iterator
1885b11 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
sqlite: refactor prepared statement iterator
3a805ad 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
sqlite: refactor prepared statement iterator
e18b5df 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
sqlite: refactor prepared statement iterator
7261f49 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
github-actions Bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
sqlite: refactor prepared statement iterator
06dcb31 
This commit refactors the StatementSync iterator implementation
in two primary ways:

- The iterator internal state is no longer exposed to JavaScript.
- The iterator prevents the prepared statement from being GC'ed.

Fixes: #57493
PR-URL: #57569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
marco-ippolito added dont-land-on-v20.x sqlite Issues and PRs related to the SQLite subsystem. labels Jun 5, 2025
ghost mentioned this pull request Jun 8, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gurgunday gurgunday left review comments
jasnell jasnell approved these changes
himself65 himself65 approved these changes
geeksilva97 geeksilva97 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sqlite: StatementSync#iterate fails with Error: statement has been finalized

8 participants

Back | FazBrowse Home | New Git URL