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

Add extensions via function in upgrade.R #389

Merged
merged 24 commits into from
Mar 7, 2022
Merged

Conversation

mgirlich
Copy link
Contributor

@mgirlich mgirlich commented Nov 3, 2021

Fixes #274.

  • I'm not sure whether we also need patch files for every extension
  • The current naming for loading extensions is in my opinion a bit confusing. initExtension() to load the math functions and initRegExp() to load the regular expressions. I think it would be nice to always have "extension" somewhere in the name.
  • I didn't manage to compile the ICU extension. It always complained "'unicode/utypes.h' file not found" even though I installed icu4c with homebrew... (also see stackoverflow)
db <- RSQLite::datasetsDb()
RSQLite::initSeries(db)
RSQLite::dbGetQuery(db, "SELECT value FROM generate_series(5,20,5);")
#>   value
#> 1     5
#> 2    10
#> 3    15
#> 4    20

Created on 2021-11-03 by the reprex package (v2.0.1)

@krlmlr
Copy link
Member

krlmlr commented Nov 3, 2021

Thanks for working on this! I think we can soft-deprecate initExtension() in favor of something clearer.

sqlite is plain C, do we need to compile differently with extensions that are C++?

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Another thing: should extensions live in src/vendor/extensions, with different compilation rules?

@mgirlich
Copy link
Contributor Author

mgirlich commented Nov 8, 2021

I put the extensions in their own folder src/vendor/extensions as suggested. They now have their own compilation rules in MAKEVARS. I don't know much about make files so you might have some improvement suggestions.
Currently, the MAKEVARS file is changed by hand for every extension but this could also be automated.

  • I am not sure what the regexp.patch file is for. Do we also need to patch the other extension files?
  • Should we compile the extensions as shared library? At least that's what it says on https://www.sqlite.org/loadext.html? I don't know what the pros and cons would be.

@krlmlr
Copy link
Member

krlmlr commented Dec 6, 2021

Thanks for working on this. What are the compilation failures in the CSV extension about? Should we remove it for now and investigate separately?

@krlmlr
Copy link
Member

krlmlr commented Jan 3, 2022

I tweaked a bit. It's weird that compilation of the csv extension works on some systems, is it relying on system header files? That would be undesirable, we're vendoring everything.

Let's avoid the extra complexity of shared libraries.

@krlmlr
Copy link
Member

krlmlr commented Jan 3, 2022

Added an explicit include path, let's see if the build behaves now.

Instead of multiple initXXX() functions, should we keep only initExtensions() and add a name argument? The default extension would be "math", the others would be called by their short name. I don't mind soft-deprecating initRegExp() .

@krlmlr krlmlr merged commit 7747b1d into r-dbi:main Mar 7, 2022
@krlmlr
Copy link
Member

krlmlr commented Mar 7, 2022

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support/configuration for SQLite3's generate_series
2 participants