You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/06/26 16:23:30 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #852: feat(c/driver/postgresql): implement StatementExecuteSchema
lidavidm opened a new pull request, #852:
URL: https://github.com/apache/arrow-adbc/pull/852
A draft implementation of the new StatementExecuteSchema method for the PostgreSQL driver.
See #318.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#issuecomment-1648506975
Will merge once I get green CI on my branch (https://github.com/lidavidm/arrow-adbc/tree/spec-1.1.0-execute-schema-cpp)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1260242953
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_CATALOG", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_CATALOG'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_DB_SCHEMA) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_SCHEMA", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_SCHEMA'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_AUTOCOMMIT) == 0) {
+ output = autocommit_ ? ADBC_OPTION_VALUE_ENABLED : ADBC_OPTION_VALUE_DISABLED;
+ } else {
+ return ADBC_STATUS_NOT_FOUND;
+ }
+
+ if (output.size() + 1 <= *length) {
Review Comment:
No, the idea is that you can check the out length, then call it again with a larger buffer if necessary
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1260242616
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
Review Comment:
Hmm,
> The following functions return parameter values established at connection. These values are fixed for the life of the connection.
It looks like we might not want to use this then, since you can change the current database at runtime
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] WillAyd commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1255033912
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
Review Comment:
I think can simplify this the same way as https://github.com/apache/arrow-adbc/pull/880
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_CATALOG", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_CATALOG'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_DB_SCHEMA) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_SCHEMA", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_SCHEMA'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_AUTOCOMMIT) == 0) {
+ output = autocommit_ ? ADBC_OPTION_VALUE_ENABLED : ADBC_OPTION_VALUE_DISABLED;
+ } else {
+ return ADBC_STATUS_NOT_FOUND;
+ }
+
+ if (output.size() + 1 <= *length) {
Review Comment:
Should this raise an error when not true?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm merged pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#issuecomment-1610168897
TODOs
- [ ] database/statement options
- [ ] error_details
- [ ] ConnectionCancel
- [ ] StatementCancel
- [ ] ConnectionGetStatistics
- [ ] new ingestion modes
- [ ] port to sqlite, too
- [ ] port to Go FFI template
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#issuecomment-1610166550
Note this can't build until SQLite and all the Go FFI is updated too (which is a pain)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] WillAyd commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1261717104
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_CATALOG", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_CATALOG'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_DB_SCHEMA) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_SCHEMA", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_SCHEMA'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_AUTOCOMMIT) == 0) {
+ output = autocommit_ ? ADBC_OPTION_VALUE_ENABLED : ADBC_OPTION_VALUE_DISABLED;
+ } else {
+ return ADBC_STATUS_NOT_FOUND;
+ }
+
+ if (output.size() + 1 <= *length) {
Review Comment:
Ah OK - guessing that is already handled in the ADBC class already then? It isn't the most obvious error if the caller's buffer is too small especially since we return ADBC_STATUS_OK
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#issuecomment-1648506402
- Going to punt on error_details and statistics since those are more involved, will file separate PRs.
- Going to punt on SQLite; I think I'll keep it as a 1.0.0-only driver for now, and #924 can be used to bring it up to 1.1.0 feature level.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1261744564
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_CATALOG", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_CATALOG'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_DB_SCHEMA) == 0) {
+ PqResultHelper result_helper{conn_, "SELECT CURRENT_SCHEMA", {}, error};
+ RAISE_ADBC(result_helper.Prepare());
+ RAISE_ADBC(result_helper.Execute());
+ auto it = result_helper.begin();
+ if (it == result_helper.end()) {
+ SetError(error, "[libpq] PostgreSQL returned no rows for 'SELECT CURRENT_SCHEMA'");
+ return ADBC_STATUS_INTERNAL;
+ }
+ output = (*it)[0].data;
+ } else if (std::strcmp(option, ADBC_CONNECTION_OPTION_AUTOCOMMIT) == 0) {
+ output = autocommit_ ? ADBC_OPTION_VALUE_ENABLED : ADBC_OPTION_VALUE_DISABLED;
+ } else {
+ return ADBC_STATUS_NOT_FOUND;
+ }
+
+ if (output.size() + 1 <= *length) {
Review Comment:
The idea is you can check the returned length; if it's too awkward we can consider something else but I dislike trying to overload the return code to mean anything except an error (and I don't think this is strictly an error)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] WillAyd commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1261706061
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
Review Comment:
I think postgres requires you to disconnect/connect if you want to change databases? When using psql as an example you would have to do `\connect <other_db>` to make that switch
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #852: feat(c/driver/postgresql): implement ADBC 1.1.0 features
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #852:
URL: https://github.com/apache/arrow-adbc/pull/852#discussion_r1261707606
##########
c/driver/postgresql/connection.cc:
##########
@@ -844,6 +849,55 @@ AdbcStatusCode PostgresConnection::GetObjects(
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnection::GetOption(const char* option, char* value,
+ size_t* length, struct AdbcError* error) {
+ std::string output;
+ if (std::strcmp(option, ADBC_CONNECTION_OPTION_CURRENT_CATALOG) == 0) {
Review Comment:
Ah! You're right. I'll fix this up then.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org