odbc icon indicating copy to clipboard operation
odbc copied to clipboard

Add support for naming tables with `I()`

Open hadley opened this issue 1 year ago • 4 comments

And eliminate double dispatch for dbExistsTable() and friends

setMethod(
  "dbExistsTable", c("OdbcConnection"),
  function(conn, name, ...) {
    
    if (is(name, "SQL") || inherits(x, "AsIs")) {
      # converts to an Id object
      name <- dbUnquoteIdentifier(conn, name)[[1]]
    }
     
    if (is(name, "Id")) {
      df <- odbcConnectionTables(
        conn,
        name = id_field(name, "table"),
        catalog_name = id_field(name, "catalog"),
        schema_name = id_field(name, "schema")
      )
    } else if (is.character(name) && length(name) == 1) {
      df <- odbcConnectionTables(conn, name = name, ..., exact = TRUE)
    } else {
      abort("`name` must be a string, Id(), SQL() object.")
    }
    
    NROW(df) > 0
  }
)

Need to check if we need c("OdbcConnection", "ANY") in the signature to avoid ambiguous dispatch, and if we can make dbWriteTable() work the same way.

hadley avatar Dec 22 '23 13:12 hadley

Also seems worth checking for arguments that would be ignored, e.g. database = in the issue linked above.

simonpcouch avatar Dec 22 '23 14:12 simonpcouch

@simonpcouch maaaaaybe. I don't think the named Id() idea is particularly well thought out so I'm not sure how much effort we want to put into mitigating using it incorrectly. But it's probably worth having something to check that the Id is well formed (i.e. it has two or three components).

hadley avatar Dec 22 '23 14:12 hadley

The fact that several folks came across #423 after its submission and a similar, well-voted-on SO Post that made the same mistake in its answer (I just submitted an edit) makes me feel like helping folks out with incorrect usage is worth the effort.

simonpcouch avatar Dec 22 '23 14:12 simonpcouch

Ok, lets add some simple checks when we tackle this.

hadley avatar Dec 22 '23 19:12 hadley