r-dbi / RPostgres

A DBI-compliant interface to PostgreSQL
https://rpostgres.r-dbi.org
Other
328 stars 78 forks source link

unnamed `Id()` breaks `dbQuoteIdentifier()` #453

Closed dpprdan closed 3 months ago

dpprdan commented 7 months ago

https://github.com/r-dbi/DBI/pull/417, i.e. allowing unnamed components in Id(), breaks dbQuoteIdentifier() (dbQuoteIdentifier_PqConnection_Id specifically), because the latter currently relies (and checks for) on component names.

library(RPostgres)

conn <- postgresDefault()

dbQuoteIdentifier(conn, Id("mytable"))
#> Error in dbQuoteIdentifier(conn, Id("mytable")): all(names(x@name) %in% c("catalog", "schema", "table")) is not TRUE

dbDisconnect(conn)
Session info ``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.3.2 (2023-10-31 ucrt) #> os Windows 11 x64 (build 22631) #> system x86_64, mingw32 #> ui RTerm #> language en #> collate German_Germany.utf8 #> ctype German_Germany.utf8 #> tz Europe/Berlin #> date 2023-12-20 #> pandoc 3.1.11 @ C:/PROGRA~1/Pandoc/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> bit 4.0.5 2022-11-15 [1] CRAN (R 4.3.0) #> bit64 4.0.5 2020-08-30 [1] CRAN (R 4.3.0) #> blob 1.2.4 2023-03-17 [1] CRAN (R 4.3.0) #> cli 3.6.1 2023-03-23 [1] CRAN (R 4.3.0) #> DBI 1.1.3.9017 2023-12-20 [1] Github (r-dbi/DBI@62056e2) #> digest 0.6.33 2023-07-07 [1] CRAN (R 4.3.1) #> evaluate 0.22 2023-09-29 [1] CRAN (R 4.3.1) #> fastmap 1.1.1 2023-02-24 [1] CRAN (R 4.3.0) #> fs 1.6.3 2023-07-20 [1] CRAN (R 4.3.1) #> generics 0.1.3 2022-07-05 [1] CRAN (R 4.3.0) #> glue 1.6.2 2022-02-24 [1] CRAN (R 4.3.0) #> hms 1.1.3 2023-03-21 [1] CRAN (R 4.3.0) #> htmltools 0.5.6.1 2023-10-06 [1] CRAN (R 4.3.1) #> knitr 1.45 2023-10-30 [1] CRAN (R 4.3.1) #> lifecycle 1.0.3 2022-10-07 [1] CRAN (R 4.3.0) #> lubridate 1.9.3 2023-09-27 [1] CRAN (R 4.3.1) #> magrittr 2.0.3 2022-03-30 [1] CRAN (R 4.3.0) #> pkgconfig 2.0.3 2019-09-22 [1] CRAN (R 4.3.0) #> purrr 1.0.2 2023-08-10 [1] CRAN (R 4.3.1) #> R.cache 0.16.0 2022-07-21 [1] CRAN (R 4.3.0) #> R.methodsS3 1.8.2 2022-06-13 [1] CRAN (R 4.3.0) #> R.oo 1.25.0 2022-06-12 [1] CRAN (R 4.3.0) #> R.utils 2.12.2 2022-11-11 [1] CRAN (R 4.3.0) #> reprex 2.0.2 2022-08-17 [1] CRAN (R 4.3.0) #> rlang 1.1.1 2023-04-28 [1] CRAN (R 4.3.0) #> rmarkdown 2.25 2023-09-18 [1] CRAN (R 4.3.1) #> RPostgres * 1.4.6 2023-10-22 [1] CRAN (R 4.3.1) #> rstudioapi 0.15.0 2023-07-07 [1] CRAN (R 4.3.1) #> sessioninfo 1.2.2 2021-12-06 [1] CRAN (R 4.3.0) #> styler 1.10.2 2023-08-29 [1] CRAN (R 4.3.1) #> timechange 0.2.0 2023-01-11 [1] CRAN (R 4.3.0) #> vctrs 0.6.4 2023-10-12 [1] CRAN (R 4.3.1) #> withr 2.5.2 2023-10-30 [1] CRAN (R 4.3.1) #> xfun 0.40 2023-08-09 [1] CRAN (R 4.3.1) #> yaml 2.3.7 2023-01-23 [1] CRAN (R 4.3.0) #> #> [1] C:/Users/Daniel.AK-HAMBURG/AppData/Local/R/win-library/4.3 #> [2] C:/Program Files/R/R-4.3.2/library #> #> ────────────────────────────────────────────────────────────────────────────── ```
krlmlr commented 7 months ago

Thanks for the heads-up. What do you mean by "breaks"? Is this different from "a feature that is not yet implemented here"?

dpprdan commented 7 months ago

https://github.com/r-dbi/RPostgres/blob/73dbac8d37f7435ea26eef8c42ba387ba3a507f4/NEWS.md?plain=1#L287 https://github.com/r-dbi/DBI/issues/173

Not a contradiction per se, but 🤔

dpprdan commented 7 months ago

What do you mean by "breaks"? Is this different from "a feature that is not yet implemented here"?

Aren't all bugs just features not yet implemented? 😄

Just saw, so I guess: "Great minds..." 😉

krlmlr commented 7 months ago

The NEWS item you mentioned is not about component names, but about the names of the resulting vector.

dpprdan commented 7 months ago

The NEWS item you mentioned is not about component names, but about the names of the resulting vector.

Technically true, but don't the names for the resulting vector usually stem from the component names (if x is an Id())?

krlmlr commented 7 months ago
list(
  A = c(a = 1, b = 2),
  B = c(a = 3, c = 4)
)

This is about the outer names A and B, not the inner (component) names a and b.

dpprdan commented 7 months ago

What do you mean by "breaks"? Is this different from "a feature that is not yet implemented here"?

Sorry, I may have misunderstood. Did you mean whether I think this might become a bigger issue beyond implementing the change here?

Well, I was vaguely sceptical of the unnamed Id change at first, because Hadley's assertion "we never actually use them, never need to care about them" is obviously not true (see above - though replacing "never" with "rarely" makes it true again) and without the names it can be hard to tell which database structure a component is referring to. It is comparable to matching arguments by name and by position and we're essentially losing the option to match by name. That said, it is rarely necessary to tell which database structure a component is referring to and it's still possible to order Id() components by name (including columns 🚀 ). So do I have a concrete example where something breaks beyond "not yet implemented"? No, I don't.


Anyway, re "not yet implemented": #372 should fix this issue now.

dpprdan commented 6 months ago

That said, it is rarely necessary to tell which database structure a component is referring to

Not so rarely after all:

https://github.com/r-dbi/RPostgres/blob/f15f8f445def9e24ff98e4bfc910efcf705c85aa/R/tables.R#L135

https://github.com/r-dbi/RPostgres/blob/f15f8f445def9e24ff98e4bfc910efcf705c85aa/R/tables.R#L170

https://github.com/r-dbi/RPostgres/blob/f15f8f445def9e24ff98e4bfc910efcf705c85aa/R/dbListObjects_PqConnection_ANY.R#L25-L27

So right now I am more sceptical again, but I will have to look at this again with a clear head.

dpprdan commented 6 months ago

I assume https://github.com/r-dbi/DBI/pull/422 will have to be implemented here as well, @krlmlr?

Do we still need this right now, if the user has a way to create Id objects without naming the components? Why is it important that dbUnquoteIdentifier() returns unnamed Id objects?

What was the answer to your question? Why would we throw away information?

RPostgres currently relies on this here

https://github.com/r-dbi/RPostgres/blob/f15f8f445def9e24ff98e4bfc910efcf705c85aa/R/dbExistsTable_PqConnection_character.R#L8

and here

https://github.com/r-dbi/RPostgres/blob/f15f8f445def9e24ff98e4bfc910efcf705c85aa/R/dbListFields_PqConnection_character.R#L5

and then in the places mentioned one post up ☝🏻.

(I also do in #413 and #414)

krlmlr commented 3 months ago

Dealt with most of the fallout. I suspect dbExistsTable(con, Id(...)) doesn't currently work.

Issues for tests: https://github.com/r-dbi/DBItest/issues/340, https://github.com/r-dbi/DBItest/issues/367.

Please open a new issue linking here if needed.