Skip to content

sqlite: add setReturnArrays method to StatementSync #57542

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Apr 4, 2025
Merged

sqlite: add setReturnArrays method to StatementSync #57542

merged 15 commits into from
Apr 4, 2025

Conversation

gurgunday
Copy link
Member

Add a new StatementSync method setReturnArrays() that allows query results to be returned as arrays instead of objects. This is more efficient when column names are meaningless, auto-generated, or when working with hundreds of columns.

Fixes #57534

It's my first significant C++ PR, hopefully first of many :)

@nodejs-github-bot
Copy link
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 Mar 19, 2025
@gurgunday gurgunday changed the title sqlite: add setReturnArrays method to StatementSync for toggling arra… sqlite: add setReturnArrays method to StatementSync for toggling array row support Mar 19, 2025
@gurgunday gurgunday changed the title sqlite: add setReturnArrays method to StatementSync for toggling array row support sqlite: add setReturnArrays method to StatementSync Mar 19, 2025
geeksilva97
geeksilva97 reviewed Mar 19, 2025
View reviewed changes
@anonrig anonrig requested review from anonrig, cjihrig and jasnell March 19, 2025 01:30
@anonrig
Copy link
Member

anonrig commented Mar 19, 2025

Cc @nodejs/cpp-reviewers

@codecov Codecov
Copy link

codecov bot commented Mar 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.85859% with 14 lines in your changes missing coverage. Please review.

Project coverage is 90.25%. Comparing base (32e5e81) to head (5420ed5).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 85.85% 0 Missing and 14 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57542      +/-   ##
==========================================
+ Coverage   90.24%   90.25%   +0.01%     
==========================================
  Files         630      630              
  Lines      184990   185042      +52     
  Branches    36214    36232      +18     
==========================================
+ Hits       166941   167008      +67     
+ Misses      11002    10996       -6     
+ Partials     7047     7038       -9
Files with missing lines Coverage Δ
src/node_sqlite.h 63.63% <ø> (ø)
src/node_sqlite.cc 80.52% <85.85%> (+<0.01%) ⬆️

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

cjihrig
cjihrig reviewed Mar 19, 2025
View reviewed changes
@cjihrig
Copy link
Contributor

cjihrig commented Mar 19, 2025

Oh, and this needs documentation.

anonrig
anonrig reviewed Mar 19, 2025
View reviewed changes
@gurgunday
Copy link
Member Author

gurgunday commented Mar 19, 2025 •
edited
Loading

I'll add docs once the code is approved

@cjihrig
Copy link
Contributor

cjihrig commented Mar 20, 2025

I've opened #57569 to refactor the iterator implementation. I think we should land that PR before this one so that we can avoid exposing iterator internal state to JS.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 3, 2025

This needs a rebase, but we can finally move this forward.

@gurgunday
Copy link
Member Author

Yep, will rebase asap

cjihrig
cjihrig approved these changes Apr 4, 2025
View reviewed changes
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Apr 4, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2025
@nodejs-github-bot nodejs-github-bot merged commit 054371d into nodejs:main Apr 4, 2025
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 054371d

@gurgunday gurgunday deleted the sqlite-array branch April 4, 2025 23:20
JonasBa pushed a commit to JonasBa/node that referenced this pull request Apr 11, 2025
@gurgunday @JonasBa
sqlite: add setReturnArrays method to StatementSync
255ce35 
PR-URL: nodejs#57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@gurgunday @RafaelGSS
sqlite: add setReturnArrays method to StatementSync
23b2bc9 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@gurgunday @RafaelGSS
sqlite: add setReturnArrays method to StatementSync
2b2a0bf 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
0b50262 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
4a8b246 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@gurgunday @RafaelGSS
sqlite: add setReturnArrays method to StatementSync
ac6f7d1 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
688a954 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
9a67be8 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
e8ec01d 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
f996ab0 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
ad464f3 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@github-actions github-actions bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@gurgunday @aduh95
sqlite: add setReturnArrays method to StatementSync
5e0503a 
PR-URL: #57542
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@AuntyEms AuntyEms mentioned this pull request Jun 8, 2025
Closed
@tniessen
Copy link
Member

@gurgunday @cjihrig Was there any discussion of how the internal state of a StatementSync (i.e., whether it will return an array or an object) should be reflected in the TypeScript declarations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig left review comments

@geeksilva97 geeksilva97 geeksilva97 left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

node:sqlite to support retrieving rows as arrays instead of objects
6 participants
@gurgunday @nodejs-github-bot @anonrig @cjihrig @tniessen @geeksilva97