lidavidm commented on code in PR #714:
URL: https://github.com/apache/arrow-adbc/pull/714#discussion_r1209313186
##########
c/driver/postgresql/connection.cc:
##########
@@ -72,12 +72,44 @@ class PqResultRow {
// as expected prior to iterating
class PqResultHelper {
public:
- PqResultHelper(PGconn* conn, const char* query) : conn_(conn) {
+ PqResultHelper(PGconn* conn, const char* query, std::vector<std::string>
param_values,
+ struct AdbcError* error)
+ : conn_(conn), param_values_(param_values), error_(error) {
Review Comment:
```suggestion
: conn_(conn), param_values_(std::move(param_values)), error_(error) {
```
##########
c/driver/postgresql/connection.cc:
##########
@@ -146,15 +185,17 @@ class PqGetObjectsHelper {
}
AdbcStatusCode GetObjects() {
- PqResultHelper curr_db_helper = PqResultHelper{conn_, "SELECT
current_database()"};
- if (curr_db_helper.Status() == PGRES_TUPLES_OK) {
- assert(curr_db_helper.NumRows() == 1);
- auto curr_iter = curr_db_helper.begin();
- PqResultRow db_row = *curr_iter;
- current_db_ = std::string(db_row[0].data);
- } else {
- return ADBC_STATUS_INTERNAL;
- }
+ std::vector<std::string> _;
Review Comment:
You can write this as `PqResultHelper helper(conn_, "QUERY", {}, error_};`
##########
c/driver/postgresql/connection.cc:
##########
@@ -72,12 +72,44 @@ class PqResultRow {
// as expected prior to iterating
class PqResultHelper {
public:
- PqResultHelper(PGconn* conn, const char* query) : conn_(conn) {
+ PqResultHelper(PGconn* conn, const char* query, std::vector<std::string>
param_values,
+ struct AdbcError* error)
+ : conn_(conn), param_values_(param_values), error_(error) {
Review Comment:
I guess this may eventually need to be more flexible than `std::string`? We
can re-evaluate when needed.
(Unfortunately, R places the restriction that we can't use C++17 for the
next few years. So we can't use `std::variant` to handle this, we would have to
backport.)
##########
c/driver/postgresql/connection.cc:
##########
@@ -146,15 +185,17 @@ class PqGetObjectsHelper {
}
AdbcStatusCode GetObjects() {
- PqResultHelper curr_db_helper = PqResultHelper{conn_, "SELECT
current_database()"};
- if (curr_db_helper.Status() == PGRES_TUPLES_OK) {
- assert(curr_db_helper.NumRows() == 1);
- auto curr_iter = curr_db_helper.begin();
- PqResultRow db_row = *curr_iter;
- current_db_ = std::string(db_row[0].data);
- } else {
- return ADBC_STATUS_INTERNAL;
- }
+ std::vector<std::string> _;
Review Comment:
Though yeah, just add a second constructor for `PqResultHelper`.
##########
c/driver/postgresql/connection.cc:
##########
@@ -72,12 +72,44 @@ class PqResultRow {
// as expected prior to iterating
class PqResultHelper {
public:
- PqResultHelper(PGconn* conn, const char* query) : conn_(conn) {
+ PqResultHelper(PGconn* conn, const char* query, std::vector<std::string>
param_values,
Review Comment:
```suggestion
explicit PqResultHelper(PGconn* conn, const char* query,
std::vector<std::string> param_values,
```
Should've caught this earlier but might as well now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
