Skip to content
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

feat: Include rfuns extension #144

Merged
merged 3 commits into from
Apr 29, 2024
Merged

feat: Include rfuns extension #144

merged 3 commits into from
Apr 29, 2024

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Apr 26, 2024

@krlmlr krlmlr requested review from hannes and samansmink April 26, 2024 20:02
@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 27, 2024

@samansmink: I'd like to statically link the rfuns extension with the duckdb R package. Do you see downsides in the way this PR solves it?

  • Expose an entry point for the R code to register the linked extension
  • Do not list the extension as part of the built-in extensions
  • Vendor all *.cpp files in the extension as a single source file in this package

The alternative I see is to just register the functions this extension provides, without registering the extension itself.

Copy link

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

Hey @krlmlr!

First of all; I haven't really looked in to duckdb-r nor the rfunctions extension, so i'm not super familiar here.

In general I think this looks fine. Inlining like this may not be super scalable from a maintainability perspective, but if the requirement of linking an extension statically here is expected to be a one-off thing, I think its ok.

Do not list the extension as part of the built-in extensions
Do you mean this as an intended requirement or as a consequence of the current implementation? If the goal is to have the rfuns extension show up in from duckdb_extensions() I think having it marked as a statically linked extension would be the cleanest. Note that this PR duckdb/duckdb#11677 should ensure it will be correctly marked as a statically linked extension

I think in general to inline the rfuns in duckdb-r it is probably cleanest do this as an extension as you have done now: In the future we may improve duckdb's internal documention functionality and then it would be nice to be able to communicate to users that these functions come from an extension called rfuns that is statically loaded.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 29, 2024

Thanks, Sam. The extension is in development, there will be a script that updates the vendored files.

I'm not even sure if we want it to show in from duckdb_extensions() . Are you suggesting that duckdb/duckdb#11677 will also have the side effect of marking this extension as a statically linked extension?

I wonder what happens if the statically linked extension is loaded and the user attempts to install it from the repository.

@samansmink
Copy link

samansmink commented Apr 29, 2024

@krlmlr installing will still happen, loading should be a NOP if the extension is loaded in the way you do in this PR where it would get added to DatabaseInstance::loaded_extensions

@krlmlr krlmlr merged commit 9fcd063 into main Apr 29, 2024
22 of 23 checks passed
@krlmlr krlmlr deleted the f-rfuns branch April 29, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline extension into duckdb R package
2 participants