Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
duckdb 1.29.0; self-host extensions #1734
duckdb 1.29.0; self-host extensions #1734
Changes from 39 commits
c70e1bc
0029c8c
feeaad8
33aa5cb
543f823
7475589
47b6bd0
abd0380
7ac5d1d
0adcb36
5365371
1fdf717
bc712c3
2fb2878
bc49674
9a13f2a
e2c8b6c
4a5128d
13f892c
8bb2866
43ef6eb
6764969
d72f0c3
d6fc020
69f25a2
30788e3
710f36a
4f58100
a8cfdcd
490d969
aaff8f8
bc39bbe
8bd0972
b90c22a
b37be07
9abaf57
ccc0073
26c7a6f
4416dd3
7704416
1dde616
0491966
be26385
a23d3e4
c753728
6e828c9
365dbe3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
First, why do we need special hashing behavior for DuckDB files? And second, if we do, we should have a separate function or inline it instead of putting this into
applyHash
. The only place we currently callapplyHash
with a path that starts with/_duckdb/
is on L151, so we can just do the special behavior there. But I’d like to avoid the special behavior unless it’s necessary.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.
We need this because when you
INSTALL foo FROM host
DuckDB will load it from${host}/v1.1.1/wasm_{p}/parquet.duckdb_extension.wasm
and not accept anything else.(If we could give the full path I would have gone with
${host}/v1.1.1/wasm_{p}/parquet.duckdb_extension.${hash}.wasm
but that doesn't seem to be supported. Maybe I missed something, though.)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.
Yep makes sense once I grokked DuckDB’s requirements on the custom repositories for extensions. But I still think we should break out this into a separate function rather than overloading
applyHash
, and I’m not sure we need content hashes on anything under_duckdb/
assuming that they don’t change the contents of extensions independently of the DuckDB version. If they do, then we’ll need a separate custom repository per extension since they’ll all have different hashes…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.
Yes—currently I don't rely on any constancy from the extensions repository. Even if duckdb's hosted extensions are indeed changed only when the duckdb version changes, it won't necessarily be the case for custom extensions. Which is why each extension gets its own INSTALL FROM command with a specific hashed path.
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 DatabaseClient.dialect isn’t used for anything in Framework. I think it’s used in notebooks for ejecting from a SQL cell, but there’s no analogous concept in Framework so maybe we should consider dropping it. Not directly related to this PR though!