DatabaseConnector 7

[schuemie]

When DatabaseConnector was first created in April 2014 DBI was still in its infancy, with no drivers for most platforms, and dbplyr did not exist yet. In the 10 years since then, the landscape has changed dramatically, also forcing us to re-evaluate the role of DatatabaseConnector.

I believe there still is a place for DatabaseConnector. DBI is a rather loose interface, leaving many per-platform idiosyncrasies. I propose DatabaseConnector is the place where those idiosyncrasies are resolved, so that anybody wanting to write analytics packages that run on all platforms we support in OHDSI, they only need to know about DatabaseConnector, and not have to worry about each individual platform. In other words, if DatabaseConnector does its job, no other OHDS package should have code that looks like if dbms == 'postgresql' do this else do this. All that should be handled by DatabaseConnector. In that sense, I think of DatabaseConnector as the 'last mile' solver: it standardizes what DBI doesn't. Hopefully in the future we won't need DatabaseConnector anymore, and we should work towards that.

Translation mechanisms

Each database platform speaks its own SQL dialect. We initially developed SqlRender so OHDSI packages would write SQL once (in what we now call OhdsiSql), and we can translate that to all other dialects. Since then, dbplyr has been developed, which allows constructing platform-specific SQL by chaining R commands. Each has its pros and cons:

SqlRender pros:

• Easier for people who know SQL
• Allows date operations (e.g. DATE_ADD(), DATE_DIFF(), YEAR())
• Closer link between SQL you write and SQL that gets executed
• Allows translation of SQL not generated in R (e.g. allows translation of Circe cohort SQL).

SqlRender cons:

• Harder to debug in the context of R code
• SQL itself is not very structured (e.g. can't define functions in a cross-platform compatible way), leading to long code chunks.
• Requires a set of translation rules per dialect
• Not included in unit test coverage statistcs

dbplyr pros:

• Easier for people who know R (especially dplyr)
• Easier to write and debug in the context of R code
• Easier to structure, e.g. move parts into functions
• No mixing of languages in a package

dbplyr cons:

• No consistent support for date operations
• More separation between code you write and SQL that gets executed. (SQL could be highly suboptimal)
• Doesn't help if you already have SQL (E.g. Circe cohort definitions)

I therefore think for the foreseeable future we want to support both SqlRender and dbplyr.

Database connections

Currently most connections are supported via JDBC drivers, although SQLite and DuckDB now use their respective DBI drivers. I think we should be agnostic to the type of driver used, and perhaps we can switch some of the JDBC drivers to DBI. I do still think that

1. DatabaseConnector should wrap the other drivers. That will allow DatabaseConnector to shield the user from database-specific idiosyncrasies.
2. DatabaseConnector should specify the exact driver to use for each platform. For example, there are multiple DBI drivers and a JDBC driver for Postgres, but we should pick only one we support. If we don't do that, we won't be able to ensure DatabaseConnector behaves in a consistent manner, because every driver (like every platform) has its own weird things (sometimes even changing the SQL dialect).

Proposed high-level changes

For DatabaseConnector 7 I suggest we lean more into the role of last-mile solver. DatabaseConnector should get leaner, offloading as much as possible to other packages (e.g. dbplyr support should be done entirely by back-ends not in DatabaseConnector). I also suggest we more closely align with DBI, and some of its unwritten conventions. (e.g. DatabaseConnector uses databaseSchema, whereas DBI unofficially uses catalog and schema).

I'm unsure what to do with renderTranslateQuerySql() and renderTranslateExecuteSql(). I use these functions all the time, because they (a) translate on the fly, (b) throw nice shareable error reports, and (c) have progress bars. However, they are not part of the DBI specification.

[ablack3]

Very nice!

I'll add a few links and ideas.

Consider supporting Boolean datatypes which are available on the following dbms

This is nice because users sometimes want to move data from R to the database and other drivers will preserve the datatype as boolean. So if a user changes drivers their code will not need to change.

• Oracle >=23: https://docs.oracle.com/en/database/oracle/oracle-database/23/sqlrf/Data-Types.html#GUID-285FFCA8-390D-4FA9-9A51-47B84EF5F83A
• BigQuery: https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#boolean_type
• Postgres: https://www.postgresql.org/docs/current/datatype-boolean.html
• Redshift: https://docs.aws.amazon.com/redshift/latest/dg/r_Boolean_type.html
• Snowflake: https://docs.snowflake.com/en/sql-reference/data-types-logical
• Databricks/Spark: https://docs.databricks.com/en/sql/language-manual/data-types/boolean-type.html
• duckdb: https://duckdb.org/docs/sql/data_types/boolean.html
• Sql Server: https://learn.microsoft.com/en-us/sql/t-sql/language-elements/comparison-operators-transact-sql?view=sql-server-ver16#boolean-data-type

SQLite does not have a bool type but apparently will store the keyword TRUE as 1 and FALSE as 0. It also does not have date types. duckdb is now past v1 and might be better than sqlite for many of our use cases.

Hive, netezza, impala, and pdw, (and possibly synapse?) aren't really supported since we don't have test servers so it seems like all OHDSI databases except sqlite have boolean datatypes.

DBITest

We could try to improve DatabaseConnectors DBI support by implementing more tests from the DBITest package. https://dbitest.r-dbi.org/

dbplyr integration

This PR might work with dbplyr. Since DatabaseConnector supports so many dialects I guess we need to set the class of the connection type dynamically when the connection is created.

I think we should keep renderTranslateQuerySql() and renderTranslateExecuteSql() simply because they are used a lot in ohdsi code. To make the package leaner we could make the dependency on SqlRender optional (Suggests rather than Depends).

Possible issues to fix:

I got this when I add the PqConnection class to a databaseconnector connection

[schuemie]

@ablack3 and I discussed the following:

Plan for DatabaseConnector version 7:

1. Swap out dbplyr backends. The current implementation uses the SQL Server backend, after which we use SqlRender to translate to the various SQL dialects. The new implementation will use each DBMS's respective backend instead, without using SqlRender. This probably requires the DatabaseConnector connections to inherit from the ODBC classes.
2. Make sure all top-level functions (e.g. executeSql, connect) talk DBI to lower-level functions. Currently these functions have tight integration with some of the driver implementations, which makes it harder to switch out drivers when needed.
3. Adopt DBI's approach for catalog & schema, deprecate databaseSchema. These are two solutions to the same problem, but the DBI approach keeps the two separate, for example making it easier to add quotes.
4. Make sure all drivers in DatabaseConnector (JDBC + wrapped DBI drivers) conform to DBI standard. No automatic translation.
5. Add support for logical data types. As described in Adam's post. SQLite doesn't support logical types, so we will stick to the current behavior there (convert to INT and throw a warning). We suspect we will phase out SQLite in favor of DuckDB in OHDSI in the future.

At a higher level, the idea is to define some standard we'll call DBI+, which is DBI plus additional constraints currently not specified in DBI, for example:

• Support for temp tables, even on platforms that don't have them
• Bigint handling
• DATETIME timezone handling (when timezone not specified on server, use local timezone, server timezone, or UTC?)

DatabaseConnector will enforce DBI+ for its drivers, truly allowing the same code to be used for all platforms. At some point, we may convince Posit to adopt DBI+ themselves, at which point DatabaseConnector will become mostly obsolete.

The higher-level connect() function will keep its two important features:

1. Allow connectionDetails input. This is necessary for more complex use cases where multiple connections need to be spawned (in multiple threads).
2. Select a single driver per platform. This will mean that packages that enforce the use of connect() (e.g. all packages in HADES) have only one driver per platform they need to test against.

Figure 1 General architecture overview. The higher-level functions will interact with the drivers through DBI+. The drivers will implement DBI+. If a DBI driver doesn't already implement DBI+, it will need to be wrapped. Otherwise, we can use it as is.

@ablack3 : could you check if I represented what we discussed correctly?

[ablack3]

Yes @schuemie this is what we discussed and seems like a good plan to me. I actually think we could use odbc, RPostgres, and bigrquery DBI driver packages possibly removing the need to use JDBC at all. I do like having the option to use JDBC or odbc with DatabaseConnector since I have found that one or the other works better in certain environments (sometimes there are weird technical issues so having a backup is nice). I think that we agreed that DatabaseConnector 7 could support both the current JDBC drivers that are used by the package as well as add wrappers around other DBI packages. However in Hades we would use one set of drivers that are well tested. I will start working on "1. Swap out dbplyr backends" since this is important for the use of DatabaseConnector with some of the tools developed in Darwin. For this I will define a new class for each connection type "DatabaseConnectorConnectionPostgres" for example that inherits from both "DatabaseConnectorConnection" and "PqConnection" so that the dbplyr translation environment will be handled correctly. https://github.com/tidyverse/dbplyr/blob/860bd6adf988a2b039485030d234eba05ee46a3c/R/backend-postgres.R#L79

Also I'll note that with the help of @edward-burn we are close to having all the date manipulation functionality we need in dplyr. So one can write dplyr code that will work on dataframes or any ohdsi dbms that does date manipulations correctly.
These are done using functions in the clock package. https://github.com/tidyverse/dbplyr/blob/860bd6adf988a2b039485030d234eba05ee46a3c/R/backend-postgres.R#L239

[schuemie]

I've added the steps we agreed on as issues that are part of the v7 milestone. If possible, I'd like to have this done by end of Q7, but currently I'm waiting on step 1 (swapping out the back ends).

[chrisknoll]

This approach using dplyr...this will make the api to sql translation an R-exclusive function then? What about those parties that are producing sql via python or Java?

[ablack3]

You can still write SQL. The dplyr backend change is about using dbplyr to generate the correct sql dialect when dplyr is used instead of the current approach. The current approach is to make dplyr think that all DatabaseConnector connections are Sql Server and then pass the SQL server code that dbplyr generates (which is not always valid OHDSI-SQL) to SqlRender.

[chrisknoll]

Ok, i wasn't clear if the end result would be we would retire SqlRender and depend on each dplyr backend to perform translation.

If that's not the case, then I see how we would continue producing OHDSI-Sql through non-R contexts and translating through SqlRender.

[azimov]

Swap out dbplyr backends. The current implementation uses the SQL Server backend, after which we use SqlRender to translate to the various SQL dialects. The new implementation will use each DBMS's respective backend instead, without using SqlRender. This probably requires the DatabaseConnector connections to inherit from the ODBC classes.

My experience of dbplyr translations has been that they're almost always inefficient and I have had more than one situation where I've had to do a complete re-write of code.

In general, my opinion on ORM style systems (from experience with Python, php, C# and Java based mechanisms) is that you should write SQL. Dbplyr might be fine for quick, user friendly scripts but I don't think it should be used in production ready situations and not for big studies. This should especially be a consideration in 'pay-per-query-credit' databases such as databricks and snowflake.

[ablack3]

Thanks for your input @azimov, @mdlavallee92, and @chrisknoll. It is appreciated and well received. A major motivation for me is also to avoid the somewhat awkward situation of requiring Darwin data partners to use one driver for the tools developed using dplyr and a different driver for Hades tools using OHDSI-SQL. I'd like to use DatabaseConnector for all the tools we're using in Darwin if possible.

I do understand that writing OHDSI-SQL might be better for production code but given time and resource constraints sometimes it's just not possible to do. One thing I would push hard for is IDE support for OHDSI-SQL meaning a developer could get syntax checking without actually needing to run their code on all dbms. The dplyr approach is not for everyone but some people really like using it for some use cases. Using R objects that are references to tables in the database is actually quite nice. You can almost forget your are using a database (similar to Andromeda) and your objects can have associated attributes and methods.

It looks like the data is all being pulled down into local memory and then manipulated to R functions. I thought the objective here was to take a dplyr expression, get an 'intermediate abstraction of the query' that could then be executed on the server. That's not what Ed's code is doing, is it? And if not, does that mean a 100M person dataset gets pulled into memory before the dplyr functions can work?

In this file I linked above all of the dplyr code is used to create SQL that is executed on the database. No data is pulled into R.

Here is the end of the function. In the code below studyPopDb is a lazy query that has not yet been executed. compute takes the "lazy query", generates the SQL, runs it in the database and stores the result in a new table in the database. It returns a reference to this table. No data has been pulled into R. Developers can use collect to run their queries and pull the results into R or compute to run their query and save the result in the database. And since the dplyr code works on dataframes as well as references to database tables you can move collect around to the appropriate place in the pipeline without changing the rest of your code. In principle you can run this package on a cdm object that is all dataframes in R or in a database.

  # run the query and save the result in a new database table
  studyPopDb <- studyPopDb %>%
    dplyr::compute(
      name = intermediateTable,
      temporary = FALSE
    )

  # remove intermediate tables
  CDMConnector::dropTable(
    cdm = cdm,
    name = dplyr::starts_with(paste0(intermediateTable, "_"))
  )

  # return list with population and attrition
  dpop <- list()
  dpop[["denominator_population"]] <- studyPopDb
  dpop[["attrition"]] <- attrition

  return(dpop)

I do think that the DatabaseConnector package should be mainly concerned with on the job of running the SQL it is given and not really care how that SQL is generated though.

[ablack3]

Since we're talking about sql, have you all seen https://sqlglot.com/sqlglot.html?

[chrisknoll]

Here is the end of the function. In the code below studyPopDb is a lazy query that has not yet been executed. compute takes the "lazy query", generates the SQL, runs it in the database and stores the result in a new table in the database.

That's good, and in line with what I was hoping it was: you're just constructing a query expression using some sort of API (compute = create table as..., etc), but was confused by this part:

  # finalise population (if we still have people)
  if ((studyPopDb %>% dplyr::count() %>% dplyr::pull()) > 0) {
    minAge <- as.integer(minAge)
    maxAge <- as.integer(maxAge)
    daysPriorObservation <- as.integer(daysPriorObservation)

    # for each min age, add the date at which they reach it
    # for each max age, add the date at which they reach it
    # the day before their next birthday
    maxAgePlusOne <- as.integer(maxAge + 1)
    minAgeDates <- addDaysQuery(cdm = cdm,
                             variable = "dob",
                             number = minAge[seq_along(minAge)],
                             type = "year",
                             name_style = "date_min_age_{number}")
    maxAgeDates <- addDaysQuery(cdm = cdm,
                                variable = "dob",
                                number = maxAgePlusOne,
                                type = "year",
                                name_style = "date_max_age_{number}")

I tried to understand what's going on here, but haven't used it so forgive the stupid question but: isnt dplyr::pull() pulling data from the server and then the if() statement is determining if it needs to apply additional queries (addDaysQuery) based on if thre is values...I'm not sure if somehow the if statements are 'deferred executed' until the query expression is invoked against the target database,...could you describe what's happening here?

My perspective is that when you build the query expression, this is completely detached from any connection to data (which is why later I'll want to talk about why the translation layer is being bound to DatabaseConnector at all, but we'll get to that one later). But the way I'm reading it above, it feels like once you get to that if statement, it needs to mateterilize something from the data in order to count the values form the column (the last one on the right which is typically the last one created (from the docs))...but if it's not really doing that and it's somehow deferred, I'd really like to know how the if() is being evaluated after the call to getDenominatorCohorts returns.

[azimov]

I do understand that writing OHDSI-SQL might be better for production code but given time and resource constraints sometimes it's just not possible to do.

I appreciate this perspective, I don't really mind what individuals do to get their tasks done and I'm fine to support this. But for HADES/Ohdsi package I would really be hesitant to support an ORM or any R to SQL mapping, I think the potential risk of it blowing up is not worth it, where we can easily fix an inefficient SQL query or broken translation. I would also imagine its easy to have copilot or ChatGPT convert dplyr translations to SQL too, should users wish to make code HADES ready.

Since we're talking about sql, have you all seen https://sqlglot.com/sqlglot.html?

Yes, I'm a fan of this package. I actually wrote some code to test it as a drop in replacement for SqlRender last year and it would be fairly straightforward, however I see little advantage forcing a python dependency instead of/as well as JRE.
This would be useful if moving towards or using a python stack for OHDSI though.

Some of the core advantages of this package are that you can test SQL against its internal engine (so, unit test your sql without the need for an actual database engine) as well as automatic optimizations if you cant provide a dictionary of types for the column names. This could speed up anything that hits the CDM as we obviously know these types in advance.

[ablack3]

could you describe what's happening here?

@chrisknoll you are correct that dplyr::pull() does pull data into R and force computation of the SQL query contained in studyPopDb. If we look a few lines above the if statement we see that compute was called so studyPopDb essentially contains the query "select * from table;". The code studyPopDb %>% dplyr::count() %>% dplyr::pull() is used to get the number of rows from the table. I think there is code similar to this in Hades used to get the row counts for Andromeda tables.

The if statement is not deferred and executes as soon as the control flow reaches it.

# run the SQL query and store the result in a table in the database
studyPopDb <- studyPopDb %>%
    dplyr::compute(
      name = paste0(intermediateTable, "_i_3"),
      temporary = FALSE
    )

  attrition <- recordAttrition(
    table = studyPopDb,
    id = "person_id",
    reasonId = 5,
    reason = "No observation time available during study period",
    existingAttrition = attrition
  )

  # Run "select count(*) as n from table" and pull the result into R
  if ((studyPopDb %>% dplyr::count() %>% dplyr::pull()) > 0) {
...

[ablack3]

I was also wondering if we could discuss the approach to casing of column names (uppercase, lowercase) that is used in DatabaseConnector.

I think the way this work is that you can pass any case into insertTable (upper, lower, or mixed). That might end up in the database as uppercase or lowercase depending on the dbms. When you use querySql you will always read the column names out as uppercase.

What if we had an option to return column names as upper, lowercase, or whatever they are in the database without casting the case?

[schuemie]

I think all HADES packages use the conversion from snake case to camel case, so then the case will be unambiguously defined. When not using that, I think the behavior should be in line with other DBI interfaces.

[ablack3]

I think column casing can be a tricky issue that DatabaseConnector should be responsible for.

Source case
We casing in R dataframes that get uploaded to a database with insertTable or dbWriteTable.
We also have the case in the OHDSI-SQL code where I think at least sometimes case insensitivity is assumed.

Database Case
We have the case when the data is stored in the database.
Here it depends on if we quote column names (to get case sensitivity) or leave them unquoted in which case the database case will depend on the dbms.

Case when data is returned from the database
Do we cast the case to lower or uppercase when reading from the database?
I think most DBI drivers do not change the case.

Different drivers and database systems have different behavior. For example duckdb will preserve case which caused this issue. OHDSI/Achilles#766

lower case columns in ddl: https://github.com/OHDSI/Achilles/blob/ea478a34179f52c13bb798abd41d7ad96d1b1fc2/inst/sql/sql_server/analyses/achilles_analysis_ddl.sql#L7
But the column names in the dataframe that gets uploaded to this table are uppercase: https://github.com/OHDSI/Achilles/blob/main/inst/csv/achilles/achilles_analysis_details.csv

I'd like to try unambiguously defining and document how casing is handled in network studies.

[chrisknoll]

Let's be careful here. For the most parts, RDBMS column names (vs. column values) work case insensitive. The platforms that do not are not cases we care about (Like Sybase and MS Access).

I believe the issue comes form when you access a result set in R, R accesses data frame column names as case-sensitive. For example:

> x <- list(a="1", B="2", c="3")
> x$a
[1] "1"
> x$A
NULL
> 

So maybe the default behavior of DBI is to return column names in upper-case. If that's the case, that's what we should stick with. I don't think we should be automagically converting columns to snake-case, or automagicaly to-lowercase on these. If columns come from databases in a pre-specified standard (and I'd value DBI standards over our own home-grown one), then we should write our code around that assumption and not make some middle-layer transformation that 1) we need to know we're an exception and 2) if we decide we like to do something differently later, we have to go back and change all of our code around that new rule.

[schuemie]

The conversion to camelCase is not automatic, but requires requesting this explicitly, which I do everywhere in my code (e.g. here in CohortMethod). In my humble opinion, If we don't convert to camelCase the code will be mess, because our guidelines prescribes camelCase for R, but any variable that at some point was pulled from a database will be snake_case.

I'm recommending that the default behavior of DatabaseConnector, including the behavior of dbplyr support, should be in line with whatever behavior DBI and dbplyr use elsewhere, although that does mean there won't be an elegant way to maintain code style.

[chrisknoll]

Going to disagree with you on two points:

One: SnakeCase conversion has definitely been automatic, even tho there's a parameter that you need to set to make it happen, there have been several circumstances (Read CSV for example) where I'm looking at a CSV column heading that says 'cohort_id', and for some unknown, unspecified reason it's showing up in my dataframe as 'cohortId'. So while you may think you're correct that it's not 'automatic' we're writing code that will make it automatic on behalf of some other caller (in fact, your code demonstrates that exactly: you are forcing camelcase not accepting it as a user choice Edit this is probably fine if it's just an internal column-name conversion that is only seen by your own function in that call, but if you're returning these fields that the user expects to have come from a database/csv and the user expects one form but this function returns another, then I think my point stands; if that's not the case, then this is not a valid example).

Two: it's not a mess. I agree that we should keep camelcase on programatic elements such as function params, API fields, function names, etc. snake-case comes from database/data source level contexts. So, when I am reading from a dataframe from a sql query, I can expect to see labels in one form, and when I'm assigning it to an api it takes another form. This is just context cues about the source of information and the target it's being applied. You can call it style, you can call it cumbersome, but I rarely have seen successful code where you don't need to know the context of the data you're dealing with.

I'm recommending that the default behavior of DatabaseConnector, including the behavior of dbplyr support, should be in line with whatever behavior DBI and dbplyr use elsewhere, although that does mean there won't be an elegant way to maintain code style.

I fully support this, and I'd go a step further that if you're pulling data out with one specification (like read a csv with certain column headings) we should get those headings out the way we see them in the source (or we should get the columns out of a database query in the same form we specified them). Transforming it in some way to make it feel like we are avoiding a 'mess' completely ignores the pain and suffering that is happening when we are pulilng the rug out beneath developers that just want something to make sense ie:

df <- executeQuery(con, "select count(*) as person_count from person")
personCount <- df[["person_count"]]  # field matches the query and not df[["personCount"]] where no one asked for that column

Update: I recognize that the DBI interface (or rJava JDBC results set conversion) may return that column as PERSON_COUNT, and therefore my code above would not find the column, but I don't want to let that "case-insensitivity in DB world vs. case sensitivity in R world" distract from the snake-case/camel-case topic I'm talking about.

[ablack3]

I'm making some progress on the boolean type support #285 . I'm putting all my changes on one branch which is a lot easier for me since I can just focus on getting a single branch in a good state. The PR (work in progress) is here #286.

I found it easier to pass integers between R and Java than logical types. The reason is that Java's native boolean type does not have an NA/null value while R's logical type does. Java has a Boolean class that does support NA/null but that does not get automatically converted to R's logical type by RJava. The int type seems to be working fine though. The data is still stored as boolean in the database and in R but is temporarily represented as integers when being passed between java and R.

So far I've only been testing on postgres.

[ablack3]

@schuemie what version of java should I be using to compile the DatabaseConnector java code? 1.8?

[chrisknoll]

yes, for now we are sticking with JDK 8

[ablack3]

Thanks @chrisknoll.

[chrisknoll]

seems to be a problem running on JDK9+ even to you compiled to 1.8 compatability, it seems the later JDKs broke compatability:
https://stackoverflow.com/questions/48693695/java-nio-buffer-not-loading-clear-method-on-runtime

And this may give more information:
https://www.morling.dev/blog/bytebuffer-and-the-dreaded-nosuchmethoderror/

I noticed in the above it references using the --release option, but.....doesn't seem to be work...or you the release switch needs to specify --release=8, which I'm not sure if your IDE is doing that.

[ablack3]

I also realized we can't yet remove OracleTempSchema because FeatureExtraction still requires it. It would be nice to remove this in the next major release. OHDSI/FeatureExtraction#126