Skip to content

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

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

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 24 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.

@araujogui araujogui 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.

LGTM

@trivikr trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 23, 2026
@trivikr
trivikr requested a review from Renegade334 September 23, 2026 15:01
panva pushed a commit that referenced this pull request Sep 25, 2026
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
PR-URL: #66195
Fixes: #66193
Reviewed-By: Guilherme Araújo <arauujogui@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@panva

panva commented Sep 25, 2026

Copy link
Copy Markdown
Member

Landed in 8bf65d4

@panva panva closed this Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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

4 participants