Add <dbplyr_table_ident> #1284

mgirlich · 2023-05-12T09:54:17Z

Currently, there are multiple classes that can be used to create a sql tbl:

<ident>
<dbplyr_schema>
<dbplyr_catalog>
<Id> (from DBI)
<sql>
<SQL>

This is quite messy, needs lots of testing and lots of extra code to support every case. Further, these classes have some annoying properties (e.g. see #1205). This PR improves the situation by adding a new class <dbplyr_table_ident> which replaces these classes (at least internally for now).

Its using vctrs::rcrd() under the hood. In most cases we don't really need (or want) anything but a single table but it might still be nice to have the vector properties.

There are a couple of questions though:

in_schema() and in_catalog() support <ident_q>, e.g. in_schema(ident_q("a b"), "table") gives <SCHEMA> a b.`table`. I don't think it is used often and I don't really see the need for it. Supporting this would make the implementation more complicated.
Should we deprecate <dbplyr_schema> and <dbplyr_catalog>? We use it internally anyway, so package authors might need to adapt to it anyway. Also, they would likely face the same issues we had with the schema and catalog classes.
I would love to get rid of the sql field in the table ident class. Unfortunately, it is needed to support <ident_q>. Could we deprecate <ident_q> for table identifiers? I don't really see a good use case for them. I think they rather make problems than provide actual benefit.

Closes #1204. Closes #1205. Closes #1261. Closes #1139. Closes #1264.

mgirlich · 2023-05-12T09:56:01Z

@hadley What's your opinion here?
@krlmlr This might be interesting for you and dm. Feel free to provide feedback.

hadley · 2023-05-12T18:31:58Z

Basic idea seems reasonable to me. Answer to questions below:

As long as there's someway to opt-out of quoting, no need to keep ident_q()
I'm fine with it; I've never found the idea of giving special names to various levels of namespacing terribly compelling.
No strong feelings and no recollection of why it was set up this was in the first place, so as long as (1) is handled, then I'm happy.

mgirlich · 2023-05-15T12:18:13Z

As long as there's someway to opt-out of quoting, no need to keep ident_q()

At which detail level to you want to be able to opt out of quoting?

Either quote everything (i.e. catalog, schema, and table) or nothing, e.g.

new_table_ident("catalog", "schema", "table", quote = TRUE)
#> "catalog"."schema"."table"
new_table_ident("catalog", "schema", "table", quote = FALSE)
#> catalog.schema.table

Can toggle quoting for catalog, schema, and table separately, e.g.

new_table_ident("catalog", "schema", "table")
#> "catalog"."schema"."table"
new_table_ident("catalog", "schema", "table", quote_schema = FALSE)
#> "catalog".schema."table"

Should it be possible to provide a qualified table name, e.g.

new_table_ident(sql("catalog.schema.table"))

The 3rd case doesn't really need to be implemented separately as it can be covered in both cases, but I think it might affect the user interface.

By the way, what's your use case to provide already quoted identifiers?

hadley · 2023-05-15T13:53:43Z

All I want is someway to provide a string that dbplyr will leave as is — this is just a useful escape hatch if we hit some weird database or weird table name where our normal approach doesn't work. So in terms of the user interface, option 3 is what I was thinking of (although I'm not tied to sql()).

mgirlich · 2023-05-25T11:12:45Z

@hadley You can skim over this PR to get a rough idea on how as_table_ident() affects the code.
My main questions are:

do we want to export the <dbplyr_table_ident> class? (together with new/as_table_ident())
It would be nice to get rid of the classes <dbplyr_catalog>, <dbplyr_schema()> and <ident_q> in the future. But its not easy to get rid of a class. Maybe we could do that by deprecating escape.dbplyr_schema/dbplyr_catalog/ident_q(). What do you think?

R/table-ident.R

mgirlich · 2023-05-25T11:20:00Z

R/db-io.R

  check_character(types, allow_null = TRUE)
  check_named(types)
  check_bool(temporary)

  tryCatch(
    dbWriteTable(
      con,
-      name = dbi_quote(table, con),
+      name = table_ident_to_id(table),


Before we passed a quoted string of class <SQL>. It has benefits to instead pass an <Id> object (see #1261). As I had to adapt this anyway, I thought I might just change it directly.

mgirlich · 2023-05-25T11:20:18Z

R/db-io.R

@@ -187,11 +187,6 @@ db_write_table.DBIConnection <- function(con,

 # Utility functions ------------------------------------------------------------

-dbi_quote <- function(x, con) UseMethod("dbi_quote")


dbi_quote() basically wasn't used anywhere.

mgirlich · 2023-05-25T11:21:35Z

R/table-ident.R

+  check_character(catalog, call = error_call)
+  check_logical(quoted, call = error_call)
+
+  n <- vctrs::vec_size_common(


It is not really necessary to support vectors but it might be handy at some point.

mgirlich · 2023-05-25T11:35:11Z

R/table-ident.R

+    ),
+    .frequency = "regularly",
+    .frequency_id = paste0("dbplyr_", f)
+  )


This should hopefully push people to avoid using ident_q() and sql() unless necessary.

R/tbl-sql.R

mgirlich · 2023-06-07T14:43:21Z

@hadley do you think you find time in the next 2 weeks to skim over this PR? It's okay if you're too busy just wanted to know whether I should wait for this PR before tackling some other issues or not 😄

hadley · 2023-06-07T16:07:34Z

Sorry, I lost the notification. I'll add it to my to do list and try and take a look this week.

krlmlr

Great idea! I'll watch this space to make the necessary changes in dm. Is there a fallback that works in CRAN and dev dplyr?

R/table-ident.R

krlmlr · 2023-06-07T16:14:57Z

tests/testthat/_snaps/verb-compute.md

@@ -21,7 +21,7 @@
      df %>% compute(name = in_schema("main", "db1"), temporary = FALSE)
    Condition
      Error in `db_save_query.DBIConnection()`:
-      ! Can't save query to table `main`.`db1`.
+      ! Can't save query to table main.db1.


Is this intended?

Yes, this is on purpose:

this might help to disencourage people to use format() when they should actually use escape()

the backticks aren't the correct way to escape names in many databases. So, the error message might confuse people that dbplyr didn't escape the table identifier correctly.

We do generally prefer to use some indicator of the start and end of the string because it's not uncommon to accidentally add a leading or trailing space, which is then really hard to discover.

R/table-ident.R

R/tbl-sql.R

R/verb-joins.R

hadley · 2023-06-12T21:03:03Z

tests/testthat/_snaps/verb-compute.md

@@ -21,7 +21,7 @@
      df %>% compute(name = in_schema("main", "db1"), temporary = FALSE)
    Condition
      Error in `db_save_query.DBIConnection()`:
-      ! Can't save query to table `main`.`db1`.
+      ! Can't save query to table main.db1.


We do generally prefer to use some indicator of the start and end of the string because it's not uncommon to accidentally add a leading or trailing space, which is then really hard to discover.

hadley · 2023-06-12T21:07:02Z

R/backend-postgres-old.R

  if (!isFALSE(temporary)) {
    cli_abort(c(
      "RPostgreSQL backend does not support creation of temporary tables",
      i = "Either set {.code temporary = FALSE} or switch to {.pkg RPostgres}"
    ))
  }

+  # RPostgreSQL doesn't handle `Id()` or `SQL()` correctly, so we can only pass


Do we want to warn about this? Or do we think so few people still use RPostgreSQL that it's not worth it?

R/build-sql.R

R/db-sql.R

R/table-ident.R

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>

mgirlich added 8 commits May 12, 2023 08:37

Add table ident class

2e6fbb3

Use table ident for <base_query>

e0cd623

Use table ident in glue_sql2()

e8729ca

Use table ident in sql_table_index()

4957a4f

Use table ident in db_write_table()

61cbcf7

Use table ident in compute()

ed1fda8

Use table ident in copy_to.src_sql()

bcff30f

Use table ident in get_col_types()

fac030d

mgirlich added 5 commits May 12, 2023 09:58

Remove unused dbi_quote()

f506d23

Fix db_table_temporary() for MS SQL

71a731b

Pass Id to db_write_table()

73f060a

Workaround bug in RPostgreSQL

2807d53

Workaround bug in MS SQL?

533c462

mgirlich added 11 commits May 25, 2023 06:16

Switch from field sql to quoted

3ea6296

Avoid unnecessary alias

621571b

Fix temporary table for MS SQL

5caef01

Remove db_write_table() method for MS SQL

6388f0a

Improve error message for unknown names in <Id>

41e8c79

Inform on ident_q() and sql() as table ident

84d42f6

Remove unnecessary check for <ident> and <sql>

812ecd1

Remove as_subquery_name()

f3dfa21

Use as_table_ident() in sql_table_prefix()

55d88fe

Remove unnecessary ident()

48627d4

Use sql_star() where possible

b80eb78

mgirlich marked this pull request as ready for review May 25, 2023 11:01

mgirlich commented May 25, 2023

View reviewed changes

R/table-ident.R Outdated Show resolved Hide resolved

mgirlich commented May 25, 2023

View reviewed changes

mgirlich added 2 commits May 25, 2023 11:28

Inform regularly, not always

67ccec6

Handle inform frequency

0b6f4fb

mgirlich commented May 25, 2023

View reviewed changes

R/tbl-sql.R Show resolved Hide resolved

mgirlich requested a review from hadley May 25, 2023 11:36

mgirlich mentioned this pull request May 26, 2023

dm should use dbplyr::in_catalog() or DBI::Id() for dbplyr tables cynkra/dm#1788

Closed

krlmlr reviewed Jun 7, 2023

View reviewed changes

mgirlich mentioned this pull request Jun 9, 2023

chore: Compatibility with dbplyr > 2.3.2 cynkra/dm#1894

Merged

check_table_ident() accepts glue

b6366c9

hadley approved these changes Jun 12, 2023

View reviewed changes

mgirlich and others added 8 commits June 13, 2023 07:48

Apply suggestions from code review

48a542f

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>

Switch from names to alias

753512e

Handle canonical names in quote_table_ident()

c013bb7

Use unclass() instead of vec_data()

6384155

Switch from current_env() to caller_env()

81a6565

Remove todo note

eb72edc

Also escape table in format() method

fe0f8a8

Merge commit '05c5317b7faae24722e307f61d0d405663ed9153'

0df8671

mgirlich merged commit 17980ed into main Jun 13, 2023

mgirlich deleted the new-table-ident branch June 13, 2023 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add <dbplyr_table_ident> #1284

Add <dbplyr_table_ident> #1284

mgirlich commented May 12, 2023 •

edited

Loading

mgirlich commented May 12, 2023 •

edited

Loading

hadley commented May 12, 2023

mgirlich commented May 15, 2023

hadley commented May 15, 2023

mgirlich commented May 25, 2023

mgirlich May 25, 2023

mgirlich May 25, 2023

mgirlich May 25, 2023

mgirlich May 25, 2023

mgirlich commented Jun 7, 2023

hadley commented Jun 7, 2023

krlmlr left a comment

krlmlr Jun 7, 2023

mgirlich Jun 9, 2023

hadley Jun 12, 2023

hadley Jun 12, 2023

hadley Jun 12, 2023

		@@ -187,11 +187,6 @@ db_write_table.DBIConnection <- function(con,

		# Utility functions ------------------------------------------------------------

		dbi_quote <- function(x, con) UseMethod("dbi_quote")

Add <dbplyr_table_ident> #1284

Add <dbplyr_table_ident> #1284

Conversation

mgirlich commented May 12, 2023 • edited Loading

mgirlich commented May 12, 2023 • edited Loading

hadley commented May 12, 2023

mgirlich commented May 15, 2023

hadley commented May 15, 2023

mgirlich commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgirlich commented Jun 7, 2023

hadley commented Jun 7, 2023

krlmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgirlich commented May 12, 2023 •

edited

Loading

mgirlich commented May 12, 2023 •

edited

Loading