You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2023/04/28 19:49:43 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #628: refactor(c/driver/postgresql): Remove utils duplication
WillAyd opened a new pull request, #628:
URL: https://github.com/apache/arrow-adbc/pull/628
- Use the new SetError with variadic arguments
- Replaced StringBuilder with some std::string concatenation where applicable
- Replaced any CHECK_ macro with the variant from either nanoarrow or existing util
- Renamed util.h -> postgres_util.h to disambiguate; right now this seems just scoped to utils for network byte swapping
--
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 #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628
--
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] paleolimbot commented on a diff in pull request #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#discussion_r1180898868
##########
c/driver/postgresql/database.cc:
##########
@@ -42,7 +42,8 @@ AdbcStatusCode PostgresDatabase::Init(struct AdbcError* error) {
AdbcStatusCode PostgresDatabase::Release(struct AdbcError* error) {
if (open_connections_ != 0) {
- SetError(error, "Database released with ", open_connections_, " open connections");
+ SetError(error, "%s%d%s", "[libpq] Database released with ", open_connections_,
Review Comment:
Is `open_connections_` an `int`? I forget the details, but I think `-Wpedantic` on gcc will fire if it is not and 32-bit windows may segfault. I usually do `static_cast<int>()` for `%d` or `static_cast<long>()` for `%ld` to make sure since most of our C++ uses `int32_t` and `int64_t`.
--
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 #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#discussion_r1180806139
##########
c/driver/postgresql/connection.cc:
##########
@@ -23,18 +23,18 @@
#include <adbc.h>
#include "database.h"
-#include "util.h"
+#include "utils.h"
namespace adbcpq {
AdbcStatusCode PostgresConnection::Commit(struct AdbcError* error) {
if (autocommit_) {
- SetError(error, "Cannot commit when autocommit is enabled");
+ SetError(error, "%s", "[libpq] Cannot commit when autocommit is enabled");
Review Comment:
Instead of hard-coding libpq or SQLite another option would be to wrap this in a macro, use `__FILE__` and traverse up to the parent directory to get either `postgresql` or `sqlite`, etc...For now just stuck with hard coding libpq in this
##########
c/driver/postgresql/connection.cc:
##########
@@ -23,18 +23,18 @@
#include <adbc.h>
#include "database.h"
-#include "util.h"
+#include "utils.h"
namespace adbcpq {
AdbcStatusCode PostgresConnection::Commit(struct AdbcError* error) {
if (autocommit_) {
- SetError(error, "Cannot commit when autocommit is enabled");
+ SetError(error, "%s", "[libpq] Cannot commit when autocommit is enabled");
Review Comment:
Instead of hard-coding libpq or SQLite another option would be to wrap this in a macro, use `__FILE__` and traverse up to the parent directory to get either `postgresql` or `sqlite`, etc...
--
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 #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#discussion_r1180899569
##########
c/driver/postgresql/database.cc:
##########
@@ -42,7 +42,8 @@ AdbcStatusCode PostgresDatabase::Init(struct AdbcError* error) {
AdbcStatusCode PostgresDatabase::Release(struct AdbcError* error) {
if (open_connections_ != 0) {
- SetError(error, "Database released with ", open_connections_, " open connections");
+ SetError(error, "%s%d%s", "[libpq] Database released with ", open_connections_,
Review Comment:
Nice catch this is a mistake
--
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] paleolimbot commented on pull request #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#issuecomment-1529655286
Yes, the postgres driver's bootstrap.R will need to be updated to (1) copy the additional utils.h file and (2) apply the rename of util.h to postgres_util.h (I think!).
--
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 #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#issuecomment-1529330744
I think the R build does need updating from the 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] lidavidm commented on pull request #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#issuecomment-1528983527
Kicked the R jobs since they failed
--
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 pull request #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#issuecomment-1530039465
Out of my element with the R stuff but think I got it. Windows build still failing - assuming unrelated to this PR
--
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 #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#discussion_r1180933373
##########
c/driver/postgresql/database.cc:
##########
@@ -42,7 +42,8 @@ AdbcStatusCode PostgresDatabase::Init(struct AdbcError* error) {
AdbcStatusCode PostgresDatabase::Release(struct AdbcError* error) {
if (open_connections_ != 0) {
- SetError(error, "Database released with ", open_connections_, " open connections");
+ SetError(error, "%s%d%s", "[libpq] Database released with ", open_connections_,
Review Comment:
Its an int32 so replaced with the appropriate inttypes macro
--
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 #628: refactor(c/driver/postgresql): Remove utils duplication
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #628:
URL: https://github.com/apache/arrow-adbc/pull/628#discussion_r1181759240
##########
c/driver/postgresql/statement.cc:
##########
@@ -206,15 +211,17 @@ struct BindStream {
param_lengths[i] = 0;
break;
default:
- SetError(error, "Field #", i + 1, " ('", bind_schema->children[i]->name,
+ SetError(error, "%s%" PRIu64 "%s%s%s%s", "[libpq] Field #",
Review Comment:
The R builds using mingw32 didn't like the %z specifier `statement.cc:214:31: warning: unknown conversion type character 'z' in format [-Wformat=]`
--
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