Skip to content

Add extensions via function in upgrade.R - #389

Merged
krlmlr merged 24 commits into
r-dbi:mainfrom
mgirlich:extensions
Mar 7, 2022
Merged

Add extensions via function in upgrade.R#389
krlmlr merged 24 commits into
r-dbi:mainfrom
mgirlich:extensions

Conversation

@mgirlich

@mgirlich mgirlich commented Nov 3, 2021

Copy link
Copy Markdown

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

krlmlr commented Nov 3, 2021

Copy link
Copy Markdown
Member

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++?

@krlmlr krlmlr 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.

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

@mgirlich

mgirlich commented Nov 8, 2021

Copy link
Copy Markdown
Author

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

krlmlr commented Dec 6, 2021

Copy link
Copy Markdown
Member

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

krlmlr commented Jan 3, 2022

Copy link
Copy Markdown
Member

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

krlmlr commented Jan 3, 2022

Copy link
Copy Markdown
Member

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

krlmlr commented Mar 7, 2022

Copy link
Copy Markdown
Member

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