Skip to content

sqlite: support db.loadExtension - #53900

Closed
himself65 wants to merge 1 commit into
nodejs:mainfrom
himself65:himself65/20240717/load_extension
Closed

sqlite: support db.loadExtension#53900
himself65 wants to merge 1 commit into
nodejs:mainfrom
himself65:himself65/20240717/load_extension

Conversation

@himself65

@himself65himself65 commented Jul 17, 2024

Copy link
Copy Markdown
Member

Closes#53898

This PR will implement loadExtension API to align with other SQLite3 JS library like better-sqlite3, node-sqlite3, jsr:@db/sqlite, bun:sqlite

Example Code:

import{load}from'sqlite-vec'import{DatabaseSync}from'node:sqlite'constdb=newDatabaseSync(':memory:',{allowExtension: true})load(db)// db.loadExtension() supportconst{ sqlite_version, vec_version }=db.prepare('select sqlite_version() as sqlite_version, vec_version() as vec_version;').get()console.log(`SQLite version: ${sqlite_version}, vector version ${vec_version}`)constitems=[[1,[0.1,0.1,0.1,0.1]],[2,[0.2,0.2,0.2,0.2]],[3,[0.3,0.3,0.3,0.3]],[4,[0.4,0.4,0.4,0.4]],[5,[0.5,0.5,0.5,0.5]]]constquery=[0.3,0.3,0.3,0.3]db.exec('CREATE VIRTUAL TABLE vec_items USING vec0(embedding float[4])')constinsertStmt=db.prepare('INSERT INTO vec_items(rowid, embedding) VALUES (?, ?)')for(const[id,vector]ofitems){constrowId=BigInt(id)constembedding=newUint8Array(Float32Array.from(vector).buffer)insertStmt.run(rowId,embedding)}constrows=db.prepare(` SELECT rowid, distance FROM vec_items WHERE embedding MATCH ? ORDER BY distance LIMIT 3`).all(newUint8Array(Float32Array.from(query).buffer))console.log(rows)
> sqlite-vec /Users/himself65/Code/node/out/Debug/node --experimental-sqlite ./index.mjs
(node:72169) ExperimentalWarning: SQLite is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
SQLite version: 3.46.0, vector version v0.1.1
[
{ rowid: 3, distance: 0 },
{ rowid: 4, distance: 0.19999998807907104 },
{ rowid: 2, distance: 0.20000001788139343 }
]

@nodejs-github-botnodejs-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 Jul 17, 2024
@himself65

Copy link
Copy Markdown
MemberAuthor

Found another small issue: nodejs/core-validate-commit#122

@avivkeller

Copy link
Copy Markdown
Member

Found another small issue: nodejs/core-validate-commit#122

Duplicate of nodejs/core-validate-commit#121

@himself65

Copy link
Copy Markdown
MemberAuthor

I will re start this PR

@himself65
himself65force-pushed the himself65/20240717/load_extension branch from edb5f58 to 8e80ca7CompareAugust 8, 2024 04:31
@himself65himself65 changed the title src(sqlite): sqlite3.loadExtensionsqlite: support sqlite3.loadExtensionAug 8, 2024
@himself65
himself65force-pushed the himself65/20240717/load_extension branch from 8e80ca7 to 200e60bCompareAugust 8, 2024 05:16
@himself65
himself65 marked this pull request as ready for review August 8, 2024 07:53
@himself65himself65 changed the title sqlite: support sqlite3.loadExtensionsqlite: support sqlite.loadExtensionAug 8, 2024
@himself65himself65 changed the title sqlite: support sqlite.loadExtensionsqlite: support db.loadExtensionAug 8, 2024
@himself65
himself65force-pushed the himself65/20240717/load_extension branch from 141f72c to 10faf88CompareAugust 8, 2024 07:54
@himself65
himself65 requested a review from cjihrigAugust 8, 2024 07:54
@himself65
himself65force-pushed the himself65/20240717/load_extension branch 2 times, most recently from 8b1dba0 to 67fe19aCompareAugust 8, 2024 08:34
@himself65

Copy link
Copy Markdown
MemberAuthor

I have no idea how to write a test for load extension now.

@himself65

Copy link
Copy Markdown
MemberAuthor

/cc @nodejs/sqlite (no such group?)

anyway I cc @nodejs/tsc since this is a breaking change

@codecov

codecovBot commented Aug 8, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 69.44444% with 22 lines in your changes missing coverage. Please review.

Project coverage is 88.52%. Comparing base (3c2da4b) to head (08280ba).
Report is 233 commits behind head on main.

Files with missing linesPatch %Lines
src/node_sqlite.cc69.44%8 Missing and 14 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #53900 +/- ##
==========================================
+ Coverage 87.99% 88.52% +0.53% 
==========================================
Files 656 657 +1 Lines 188999 189929 +930 Branches 35981 36472 +491 ==========================================
+ Hits 166301 168133 +1832 + Misses 15865 14992 -873 + Partials 6833 6804 -29 
Files with missing linesCoverage Δ
src/node_errors.h85.00% <ø> (ø)
src/node_sqlite.h70.00% <ø> (ø)
src/node_sqlite.cc80.25% <69.44%> (-0.93%)⬇️

... and 108 files with indirect coverage changes

@benjamingr

Copy link
Copy Markdown
Member

cc @cjihrig

Comment threadsrc/node_sqlite.cc Outdated
@cjihrig

Copy link
Copy Markdown
Contributor

since this is a breaking change

I took a quick look at the code. I'm not sure what makes this a breaking change though.

I do think the TSC needs to decide if they want to support loading arbitrary native code in this manner. It wouldn't be the first way that Node supports loading native code, so maybe it's fine.

I have no idea how to write a test for load extension now.

Is it possible to create a very trivial extension that can be accessed as a test fixture?

@tniessen

Copy link
Copy Markdown
Member

I do think the TSC needs to decide if they want to support loading arbitrary native code in this manner.

It definitely must be disabled when the experimental permission model is enabled, just like native add-ons etc.

@himself65

Copy link
Copy Markdown
MemberAuthor

For the native test, I'm not sure if we should build an SQLite binding from scratch or if we should just copy some community dll/so/lib to fixtures.

@targos

Copy link
Copy Markdown
Member

We also need a test that tries to load an unexpected file (that is not a sqlite extension). It should throw, not crash the process.

Comment threadtest/fixtures/sqlite/vec0.aarch64.dylib Outdated
@himself65
himself65force-pushed the himself65/20240717/load_extension branch from c01c585 to 1bf194bCompareAugust 10, 2024 08:22
@jakecastelli

Copy link
Copy Markdown
Member

I'm not sure if we should build an SQLite binding from scratch or if we should just copy some community dll/so/lib to fixtures.

IMO we should build from scratch to avoid any potential attack and make update tests fixture easier if we ever need to update them.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollinamcollina 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

@cjihrig

Copy link
Copy Markdown
Contributor

FYI - I think test/parallel/test-permission-sqlite-load-extension.js is legitimately failing on Windows. I'm not sure if it's related to the use of common.escapePOSIXShell in that test or what.

@himself65
himself65force-pushed the himself65/20240717/load_extension branch from 6ee1209 to 50715efCompareDecember 7, 2024 04:38
@himself65himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@himself65

Copy link
Copy Markdown
MemberAuthor

not sure how to fix it right now. setting up local env on my local windows pc

@himself65

Copy link
Copy Markdown
MemberAuthor

ok the error is legitimately falling, testing on my windows locally, I think because wrong usage of the cp api

@himself65
himself65force-pushed the himself65/20240717/load_extension branch from 50715ef to 08280baCompareDecember 10, 2024 21:58
@himself65

Copy link
Copy Markdown
MemberAuthor

OK, I found because \n on windows will truncate the script code, remove it

@himself65himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@himself65

Copy link
Copy Markdown
MemberAuthor

will merge this today

himself65 added a commit that referenced this pull request Dec 11, 2024
PR-URL: #53900
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@himself65

Copy link
Copy Markdown
MemberAuthor

Landed in 5c2f599

return path.join(path.dirname(process.execPath), targetFile);
}

const binary = resolveBuiltBinary('libsqlite_extension');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems this doesn't work when node is built with --shared-sqlite

Sign up for freeto 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-ciPRs that need a full CI run.sqliteIssues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support sqlite.loadExtension

16 participants

@himself65@avivkeller@benjamingr@cjihrig@tniessen@targos@jakecastelli@nodejs-github-bot@asg017@richardlau@RafaelGSS@mcollina@jasnell@anonrig@aduh95@marco-ippolito