Copilot commented on code in PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#discussion_r4001603940
##########
c/driver/postgresql/database.cc:
##########
@@ -85,7 +85,25 @@ AdbcStatusCode PostgresDatabase::Init(struct AdbcError*
error) {
return status.ToAdbc(error);
}
- status = RebuildTypeResolver(conn);
+ auto resolver = std::make_shared<PostgresTypeResolver>();
+ switch (type_resolver_mode_) {
+ case TypeResolverMode::kAuto:
+ case TypeResolverMode::kBuiltin: {
+ status = InitializeTypeResolver(*resolver);
+ break;
Review Comment:
The setter accepts `auto`, but initialization handles it exactly like
`builtin`. This contradicts the enum's stated fallback semantics: callers
selecting `auto` silently lose server resolution for unknown/composite types.
Please either implement the fallback before accepting this value or reject
`auto` until it is supported.
##########
c/driver/postgresql/postgres_type_test.cc:
##########
@@ -553,4 +555,41 @@ TEST(PostgresTypeTest, PostgresTypeResolveInt2vector) {
EXPECT_EQ(0, type.n_children());
}
+TEST(PostgresTypeTest, BuiltinResolver) {
+ const char* uri = std::getenv("ADBC_POSTGRESQL_TEST_URI");
+ if (!uri) {
+ FAIL() << "Must provide env var ADBC_POSTGRESQL_TEST_URI";
+ }
+
+ auto* conn = PQconnectdb(uri);
+ if (PQstatus(conn) != CONNECTION_OK) {
+ std::string message = PQerrorMessage(conn);
+ PQfinish(conn);
+ ASSERT_EQ(CONNECTION_OK, PQstatus(conn)) << message;
+ }
Review Comment:
`PQfinish` invalidates `conn`, so the following `PQstatus(conn)`
dereferences freed memory on the connection-failure path (and ASAN may report a
use-after-free instead of the useful connection error). Capture the message,
finish the connection, and fail directly.
--
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]
