Skip to content

sqlite: use cached strings for property keys - #65902

Open
kyungrae2002 wants to merge 1 commit into
nodejs:mainfrom
kyungrae2002:sqlite-use-cached-property-strings
Open

kyungrae2002 wants to merge 1 commit into
nodejs:mainfrom
kyungrae2002:sqlite-use-cached-property-strings

Conversation

@kyungrae2002

Copy link
Copy Markdown
Contributor

Use strings cached in Environment when accessing commonly used node:sqlite property keys.
Replace FIXED_ONE_BYTE_STRING() calls in the SQLite bindings with the corresponding strings cached by Environment.

This avoids creating the same V8 strings at each call site and makes the SQLite binding consistent with other Node.js core modules that use Environment string getters.

No intended behavioral changes.

@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 8, 2026
@codecov

codecov Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.37%. Comparing base (66f26d3) to head (bcae997).

Files with missing lines Patch % Lines
src/node_sqlite.cc 33.33% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65902      +/-   ##
==========================================
- Coverage   90.38%   90.37%   -0.01%     
==========================================
  Files         790      790              
  Lines      274497   274488       -9     
  Branches    52557    52559       +2     
==========================================
- Hits       248100   248076      -24     
  Misses      16879    16879              
- Partials     9518     9533      +15     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.92% <33.33%> (-0.08%) ⬇️

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

@addaleax addaleax added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@github-actions github-actions Bot removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts with its base branch, removing the author ready label.
Please rebase your branch onto the latest base branch, resolve the conflicts locally, and force-push.
Afterwards the pull request needs a fresh collaborator approval, and a collaborator will add the label back once it is author ready again.

Signed-off-by: Kyungrae Kang <kyungrae2002@gmail.com>
Assisted-by: Codex
@kyungrae2002
kyungrae2002 force-pushed the sqlite-use-cached-property-strings branch from 551eeb6 to bcae997 Compare September 26, 2026 10:18
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.

3 participants