r-dbi / RPostgres

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

fix: Fix `dbQuoteIdentifier()` for `Id()` objects to no longer rely on names #460

Closed krlmlr closed 3 months ago

aviator-app[bot] commented 3 months ago

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes. Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.
aviator-app[bot] commented 3 months ago

This pull request failed to merge: some CI status(es) failed. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Failed CI(s): Smoke test: stock R

aviator-app[bot] commented 3 months ago

This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts).

aviator-app[bot] commented 3 months ago

This pull request failed to merge: PR has a blocked label, remove to re-queue. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

iangow commented 1 month ago

Fix dbQuoteIdentifier() for Id() objects to no longer rely on names.

More like "no longer even uses names" … this was a "breaking change" for me, as I was using Id(table = "y", schema = "x") in places here. (I hadn't realized it hadn't been implemented already, so I thought I was safe when sending in my book.) All good now, it seems.

Kjir commented 1 month ago

I confirm that this is a breaking change. It breaks this code: https://github.com/mbannert/timeseriesdb/blob/main/R/read_time_series.R#L44

Before it would produce "timeseries".ts_read_raw(...) but now produces "timeseries"ts_read_raw(...) because there is only the schema in DBI::Id(schema="timeseries") and thus the collapse character would not be added, whereas it was added before.

krlmlr commented 1 month ago

Ian, I'm getting:

DBI::Id(table = "y", schema = "x")
#> <Id> "x"."y"

Created on 2024-06-08 with reprex v2.1.0

Do you expect a different result?

Anyway, thanks for the feedback, apologies for the inconvenience.

iangow commented 1 month ago

I thought I was

Ian, I'm getting:

DBI::Id(table = "y", schema = "x")
#> <Id> "x"."y"

Created on 2024-06-08 with reprex v2.1.0

Do you expect a different result?

Anyway, thanks for the feedback, apologies for the inconvenience.

I thought I was seeing Id(table = "y", schema = "x") generating 'table "y"."x" not found' errors or similar, but now I can't remember precisely where I saw them and I can't reproduce them. I have reverted my code to Id(table = "y", schema = "x") and I'm not seeing any issues; (<Id> "x"."y") is exactly what I want.