DyfanJones / noctua

Connect R to Athena using paws SDK (DBI Interface)
https://dyfanjones.github.io/noctua/
Other
45 stars 5 forks source link

The 'statement' argument to dbGetQuery and dbSendQuery methods isn't exposed in older versions of R #170

Closed tyner closed 2 years ago

tyner commented 2 years ago

Issue Description

The 'statement' argument to dbGetQuery and dbSendQuery methods isn't exposed in older versions of R, resulting in an error that it cannot find the 'statement' object.

Reproducible Example

First, install version 2.3.0 of the noctua package under R version 3.4.4. Then take a look at

library(noctua)
showMethods("dbGetQuery", includeDefs=TRUE)
showMethods("dbSendQuery", includeDefs=TRUE)

It will say:

Function: dbGetQuery (package DBI)
conn="AthenaConnection", statement="character"
function (conn, ...)
{
    .local <- function (conn, statement = NULL, statistics = FALSE,
        unload = FALSE, ...)
    {
        con_error_msg(conn, msg = "Connection already closed.")
        stopifnot(is.logical(statistics), is.logical(unload))
        rs <- dbSendQuery(conn, statement = statement, unload = unload)
        if (statistics)
            print(dbStatistics(rs))
        out <- dbFetch(res = rs, n = -1, ...)
        dbClearResult(rs)
        return(out)
    }
    .local(conn, statement, ...)
}

and

Function: dbSendQuery (package DBI)
conn="AthenaConnection", statement="character"
function (conn, ...)
{
    .local <- function (conn, statement = NULL, unload = FALSE,
        ...)
    {
        con_error_msg(conn, msg = "Connection already closed.")
        stopifnot(is.logical(unload))
        res <- AthenaResult(conn = conn, statement = statement,
            s3_staging_dir = conn@info$s3_staging, unload = unload)
        return(res)
    }
    .local(conn, statement, ...)
}

Whereas, under R version 3.5.0 (and later), the methods do expose the statement argument, and there is no error when using them. So naturally I am wondering if this is due to R-core correcting a bug going from version 3.4.4 to 3.5.0 of R, or could it be considered a bug in noctua itself? If the former, should noctua require R version >= 3.5.0? In any case, curious to hear your thoughts or suggestions.

Session Info ```r devtools::session_info() #> output ``` ``` Session info ------------------------------------------------------------------- setting value version R version 3.4.4 (2018-03-15) system x86_64, linux-gnu ui X11 language (EN) collate en_US.UTF-8 tz America/New_York date 2021-11-04 Packages ----------------------------------------------------------------------- package * version date source data.table 1.12.6 2019-10-18 CRAN (R 3.4.4) DBI 1.1.0 2019-12-15 CRAN (R 3.4.4) devtools 1.12.0 2016-12-05 CRAN (R 3.4.0) digest 0.6.27 2020-10-24 CRAN (R 3.4.4) memoise 1.1.0 2017-04-21 CRAN (R 3.4.0) noctua * 2.3.0 2021-10-26 CRAN (R 3.4.4) paws 0.1.12 2021-09-03 CRAN (R 3.4.1) rstudioapi 0.11 2020-02-07 CRAN (R 3.4.4) withr 2.1.2 2018-03-15 CRAN (R 3.4.4) ```
DyfanJones commented 2 years ago

Hi @tyner, thanks for identify this issue. I will look into this to see if it is a bug within the package.

DyfanJones commented 2 years ago

@tyner just added a fix to the current PR branch bump-dev. I have tested with R-3.3.3 so I am hoping it works for you 😄 Please let me know.

remotes::install_github("dyfanjones/noctua", ref="bump-dev")
tyner commented 2 years ago

Thanks for the quick turnaround! I ran into a bit of a snag when it asked about upgrading xml2. I tried two different ways and got the same error each time. I also tried upgrading to the latest version of withr, which didn't resolve the issue. Alternatively, is there a way to directly download a .tar.gz version of the updated noctua package?

remotes::install_github("dyfanjones/noctua", ref="bump-dev")
Downloading GitHub repo dyfanjones/noctua@bump-dev
These packages have more recent versions available.
It is recommended to update all of them.
Which would you like to update?

1: All                               
2: CRAN packages only                
3: None                              
4: xml2 (268387bb1... -> NA) [GitHub]

Enter one or more numbers, or an empty line to skip updates: 3
Error: Failed to install 'noctua' from GitHub:
  'local_makevars' is not an exported object from 'namespace:withr'
remotes::install_github("dyfanjones/noctua", ref="bump-dev")
Downloading GitHub repo dyfanjones/noctua@bump-dev
These packages have more recent versions available.
It is recommended to update all of them.
Which would you like to update?

1: All                               
2: CRAN packages only                
3: None                              
4: xml2 (268387bb1... -> NA) [GitHub]

Enter one or more numbers, or an empty line to skip updates: 1
xml2 (268387bb1... -> NA) [GitHub]
Downloading GitHub repo hadley/xml2@master
Error: Failed to install 'noctua' from GitHub:
  Failed to install 'xml2' from GitHub:
  'local_makevars' is not an exported object from 'namespace:withr'
DyfanJones commented 2 years ago

Intersting, remotes install worked for me. However if you have all the dependencies you can try

remotes::install_github("dyfanjones/noctua", ref="bump-dev", dependencies = FALSE)

Note if you don't have the dependencies this will fail.

tyner commented 2 years ago

That did the trick. I have tested the new version of noctua under a version of R prior to 3.5.0 and it works splendidly.

I am still curious to learn if R-core had intentionally made setMethod more robust in this regard for version 3.5.0, or if the observed behavior was a happy accident associated with some other change. I may inquire about that on the mailing list.

In any case, thank you Dyfan!

DyfanJones commented 2 years ago

No worries, I will push these changes to cran next week. If you need them sooner please let me know.

DyfanJones commented 2 years ago

This has now been pushed to the cran in release 2.4.0.