-
Notifications
You must be signed in to change notification settings - Fork 138
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.
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
Add OPFS (Origin Private File System) Support #1856
Conversation
This is very very welcome, thanks. I am in the process of releasing a new version of duckdb-wasm, connected to duckdb |
@carlopi |
One comment would be if you could have a look at the failing test |
bbb2067
to
383ff1b
Compare
@carlopi https://github.com/AKABANAKK/duckdb-wasm/actions/runs/11016245572 |
Thanks, I gave a proper look, this looks solid, thanks a lot for the contribution. This PR does introduces some API changes that might be somewhat unexpected, so I think the proper way forward would be:
|
@carlopi |
886b161
to
54b82e9
Compare
ad9b192
to
dedc2aa
Compare
I am planning to submit a follow-up PR after this one is merged, which will address the following:
These features are already implemented and are awaiting the merge of this PR. |
1da9a46
to
eb6ec89
Compare
@e1arikawa to build your branch I just need to clone your repo and build from source?
Then I imagine there's somewhere I can find something like |
@e1arikawa love to hear this! I made a fork of your feature branch where I've implemented #1 for our team, but including #2 and #3 is above and beyond. Are you able to you share your working branch on your fork? I'd like to pull it down and test it with our use cases. Obviously no need to PR it yet, but I'd prefer to work off of a single fork while we all wait for maintainer approval.
|
Hello @carlopi or team! I’ve been following this PR and wanted to mention how valuable it would be for our work. Just wondering if there’s an update or anything I can assist with to help get it merged? Thanks! |
I don't want to beat a dead horse, but we are extremely interested in this feature as well. |
/** Register a file object URL */ | ||
public registerFileHandle<HandleType>( | ||
public async registerFileHandle<HandleType>( | ||
name: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest, possibly only problem I am seeing for merning this is that I think it's not a good idea to move from a syncronous call to an asyncronous call.
Would it be possible to add a registerFileHandleAsync
, and use that in the relevant places, while leaving the option (that can't support OPFS) for a simpler and backward compatible registerFileHandle
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also another note: I tried (single threaded version) and got:
COPY (SELECT 32) TO 'opfs://myfile.csv';
to show a waring in console (No OPFS access handle registered with name: opfs://myfile.csv
) but also trigger a Maximum call stack size exceeded
.
Here having this either fail with a better failure mode would be enough.
Actually, thanks A LOT @e1arikawa and congrats on pushing this in. I might send a couple of PR to adapt some minor things, but it's a great step in the right direction. Thanks a lot |
Thank you @e1arikawa, @carlopi et.al.! That’s a great new feature! |
Needs first to avoid throwing in destructor (big no), AND to convert JS exception in C++ exception More iteration on comments to duckdb#1856
Needs first to avoid throwing in destructor (big no), AND to convert JS exception in C++ exception More iteration on comments to duckdb#1856
Needs first to avoid throwing in destructor (big no), AND to convert JS exception in C++ exception More iteration on comments to duckdb#1856
Needs first to avoid throwing in destructor (big no), AND to convert JS exception in C++ exception More iteration on comments to #1856
@carlopi @e1arikawa any progress around supporting adding support for |
Thanks for working on the OPFS support, it's going to be super useful! I'm trying to use it with
when running this code
|
Hello there, I too am very interested in OPFS support. My use case is to download a db file created on the server and work with it in the browser. I have simulated this as @ilyabo above and see the same issue. Just to be sure, I exported the file from OPFS back into the file system and opened it in duckdb cli and the file is OK. Many thanks, really look forward to getting this working, great effort - love this repo :) |
Description:
This PR implements OPFS (Origin Private File System) support in the latest version of duckdb-wasm based on PR #1490.
This allows database files to be read and written to the OPFS.
API:
When opening a database file, you can now use the following API:
Changes: