Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(c/driver/postgresql): Support queries that bind parameters and return a result #2065

Merged

Conversation

paleolimbot
Copy link
Member

@paleolimbot paleolimbot commented Aug 7, 2024

This PR allows statements with a bind stream to return a result. This is not trivial because it requires concatenating results from multiple rows in a bind stream into one output stream. The existing bind stream had been wired up to pull and loop in one big function, but we need something with that knows which array/row it's on and can "bind and execute" the next one. I think I did this successfully without modifying the existing binding behaviour (which could maybe use the copy writer at some point in the future for better type support).

The behaviour of the "result reader" now incorporates the bind stream:

  • When the result reader is being created/initialized, it executes eagerly (the query if there are no bind parameters, and the query for all the bind parameters that return zero-row results or until the bind stream is exhausted otherwise). If there is no out array stream, it will exhaust the bind stream before returning. This mirrors what currently happens and I think is more of what a user would expect; however, it does mean that a user would have to remember to fully consume the output array stream to ensure that the full bind stream was exhausted (if they specified an output stream in the first place).
  • On each call to GetNext() of the result, we check for an existing PGresult to convert, and try to regenerate it if it's absent (== already been converted to an array and returned) using a new value from the bind stream. When pulling from the bind stream we pull everything we can until we get something with at least one row.

Much existing behaviour (update, create table, insert, delete) flows through the first bullet, and I believe this has some more correct behaviour for the case where an insert or update or delete updates a number of rows that isn't one and/or there is more than one value in the bind stream.

library(adbcdrivermanager)

con <- adbc_database_init(
  adbcpostgresql::adbcpostgresql(), 
  uri = "postgresql://localhost:5432/postgres?user=postgres&password=password"
) |> 
  adbc_connection_init()

read_adbc(con, "SELECT $1 as x", bind = data.frame(x = c("a", NA, "b", "c"))) |> 
  as.data.frame()
#>      x
#> 1    a
#> 2 <NA>
#> 3    b
#> 4    c

execute_adbc(con, "DROP TABLE IF EXISTS integers")
write_adbc(data.frame(x = 1:10), con, "integers")

read_adbc(con, "SELECT * from integers where x > $1", bind = data.frame(x = 8:10)) |> 
  as.data.frame()
#>    x
#> 1  9
#> 2 10
#> 3 10

Created on 2024-08-13 with reprex v2.0.2

Closes #2024.

@paleolimbot paleolimbot force-pushed the c-driver-postgresql-bind-stream-read branch from 130e81d to 0937125 Compare August 13, 2024 12:37
@paleolimbot paleolimbot marked this pull request as ready for review August 13, 2024 15:17
@github-actions github-actions bot added this to the ADBC Libraries 14 milestone Aug 13, 2024
Comment on lines 250 to 251
// For the case where we don't need a result, we either need to exhaust the bind
// stream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we either need to exhaust the bind stream, or ...?

@paleolimbot paleolimbot merged commit 83f0768 into apache:main Aug 14, 2024
79 of 80 checks passed
@paleolimbot paleolimbot deleted the c-driver-postgresql-bind-stream-read branch August 15, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

c/driver/postgresql: Support paramterized queries that return a result set
2 participants