Skip to content

sqlite: run generator return() on cursor refilter - #66195

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:sqlite-virtual-table-cursor-2
Open

trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:sqlite-virtual-table-cursor-2

Conversation

@trivikr

@trivikr trivikr commented Sep 22, 2026

Copy link
Copy Markdown
Member

Fixes: #66193

SQLite re-invokes xFilter on a cursor it already used, as it does for the inner table of a correlated subquery or join, abandoning the previous iterator mid-loop. Call its return() method so generator finally blocks still run.

Refactor the xClose cleanup into CloseIterator() so both paths share it; on refilter, a throwing cleanup is surfaced as the query error.


Assisted-by: opencode

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot 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 Sep 22, 2026
@nodejs-github-bot

This comment was marked as outdated.

SQLite re-invokes xFilter on a cursor it already used, as it does
for the inner table of a correlated subquery or join, abandoning the
previous iterator mid-loop. Call its return() method so generator
`finally` blocks still run.

Refactor the xClose cleanup intoCloseIterator() so both paths share it;
on refilter, a throwing cleanup is surfaced as the query error.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: opencode
@trivikr
trivikr force-pushed the sqlite-virtual-table-cursor-2 branch from 9b01002 to cefe77f Compare September 22, 2026 05:59
@nodejs-github-bot

This comment was marked as outdated.

@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.28%. Comparing base (dd9f4da) to head (cefe77f).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 88.46% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66195      +/-   ##
==========================================
- Coverage   90.29%   90.28%   -0.02%     
==========================================
  Files         790      790              
  Lines      272508   272515       +7     
  Branches    52028    52037       +9     
==========================================
- Hits       246071   246040      -31     
- Misses      16905    16949      +44     
+ Partials     9532     9526       -6     
Files with missing lines Coverage Δ
src/node_sqlite.h 87.27% <ø> (ø)
src/node_sqlite.cc 82.03% <88.46%> (-0.03%) ⬇️

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@trivikr
trivikr requested a review from araujogui September 22, 2026 13:48
@trivikr

trivikr commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

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

This CI is successful.
GitHub checks show old CI which was cancelled, as a force-push was needed.

If this PR is approved with no changes requested, I'll land it manually.

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++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sqlite: re-filtering a virtual-table cursor skips generator cleanup

2 participants