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

driver-snowflake: Class confusion #861

Open
detule opened this issue Nov 10, 2024 · 1 comment · May be fixed by #865
Open

driver-snowflake: Class confusion #861

detule opened this issue Nov 10, 2024 · 1 comment · May be fixed by #865

Comments

@detule
Copy link
Collaborator

detule commented Nov 10, 2024

We have

setClass("Snowflake", contains = "OdbcConnection")
...
snowflake <- function() {
  new("Snowflake")
}
...
setMethod(
  "dbConnect", "Snowflake",
   function(drv,

but the DBI::dbConnect API is formulated relative to OdbcDriver, not OdbcConnection. I don't think this is causing issues, right now, however if nobody objects, i plan on changing the snowflake() call to return an object that inherits from OdbcDriver. This will also align the Snowflake and Databricks API.

@simonpcouch
Copy link
Collaborator

Ah, interesting. Nice catch! Just talking myself through this:

Backends that don't have a connection helper just have e.g. setClass("DBMSName", contains = "OdbcConnection") which kicks in from dbConnect(odbc()) output and the methods are inherited via DBMSName. The driver class is generic to odbc(), the connection class is DBMS-specific.

Databricks has databricks() which returns an DatabricksOdbcDriver < OdbcDriver. A dbConnect() method is defined relative to it, otherwise Databricks just inherits its methods from setClass("Spark SQL", contains = "OdbcConnection") (i.e. based on the output of dbConnect()). The driver class is DBMS-specific, the connection class kind of is even though it doesn't use the DBMS name explicitly.

Snowflake has snowflake() which returns a new("Snowflake") as setClass("Snowflake", contains = "OdbcConnection"). The resulting connection object from dbConnect() also has class "Snowflake" and methods are defined relative to that class. The driver and connection classes are DBMS-specific but, unfortunately, identical. We can thus split off the driver class to be DBMS-specific but also be disambiguated from the connection class.

So we do make snowflake() return new("SnowflakeOdbcDriver") which is just setClass("SnowflakeOdbcDriver", contains = "OdbcDriver"), as you suggest, and transition dbConnect from working on Snowflake instead to SnowflakeOdbcDriver, but then keep all of the existing "Snowflake" class and methods code as-is, since it defines behavior on the connection rather than the driver?

@detule detule linked a pull request Dec 7, 2024 that will close this issue
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 a pull request may close this issue.

2 participants