-
Notifications
You must be signed in to change notification settings - Fork 80
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
Remove dependency on RSQLite #165
Conversation
@@ -102,13 +102,11 @@ lowLevelQuerySqlToAndromeda.default <- function(connection, | |||
integerAsNumeric = integerAsNumeric | |||
) | |||
|
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.
Here I just use andromeda's interface for writing and appending to a table.
@@ -239,21 +231,12 @@ querySqlToAndromeda <- function(connection, | |||
integerAsNumeric = integerAsNumeric, | |||
integer64AsNumeric = integer64AsNumeric | |||
) | |||
columnNames <- RSQLite::dbListFields(andromeda, andromedaTableName) | |||
columnNames <- colnames(andromeda[[andromedaTableName]]) |
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.
This should work with the current Andromeda version.
) | ||
lapply(sql, function(x) RSQLite::dbExecute(andromeda, x)) | ||
} | ||
names(andromeda[[andromedaTableName]]) <- newColumnNames |
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.
This required a new Andromeda method and is in the current develop branch of Andromeda.
The R-CMD checks are failing because of this error |
@ablack3 Am I correct in thinking that this branch will mean that any db backend can be used with andromeda, including remote backends? |
@azimov Basically I need to make sure that if other packages use Andromeda they do so through it's interface (functions that are tested) and not rely on implementation details like the fact that it uses SQLite under the hood. I'm working on a new version of Andromeda that uses duckdb instead of Sqlite. I had been thinking of Andromeda as a local database but Martijn's view of it is really as a replacement for ff which is strictly to support on-disk dataframes and not really treat it as a database. I'd love to hear your use cases or thoughts about Andromeda. Have you seen the dm package (https://cynkra.github.io/dm/)? |
I've not seen DM but that looks incredibly useful, thanks!
My use case is simply using FeatureExtraction with a huge number of cohorts where storing results on a disk isn't desirable or efficient, this is sort of supported in FeatureExtraction for a limited subset of functions (and it seems buggy and inconsistent so I just wrote this PR). If Andromeda supported using any Sql db then my thought was that no minimal would need to be made to FE to support this mode of working. I fully accept that this isn't want Andromeda was designed to do and I'm definitely not proposing as my use-case is probably niche, this commit just made me think it would be possible. |
I think the goal of Andromeda is to support larger than memory dataframes/tables (including multiple related tables) in R and writing FeatureExtraction results directly to a remote database should be handled in FeatureExtraction. |
Remove dependency on RSQLite and use Andromeda interface for queryToAndromeda functions.
There is one change that should require the current develop branch of Andromeda (see comment below).
names(andromeda[[andromedaTableName]]) <- newColumnNames
Unfortunately I don't think these functions are covered by tests. @schuemie, If you're ok with these changes conceptually then I'll write some tests as a next step.