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 2026/09/11 03:42:27 UTC
[PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
lidavidm opened a new pull request, #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775
- Ship a hardcoded type resolver, eliminating queries on the first connect
- [ ] Add tests that this resolves the same entries as before
- [ ] Document the option
- [ ] Add ability to build the table when needed
Closes #4749.
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#discussion_r4005861893
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -581,6 +586,8 @@ class PostgresTypeResolver {
}
private:
+ friend class PostgresTypeTest_BuiltinResolver_Test;
+
std::unordered_map<uint32_t, PostgresType> mapping_;
// We can't use PostgresTypeId as an unordered map key because there is no
// built-in hasher for an enum on gcc 4.8 (i.e., R 3.6 on Windows).
Review Comment:
FWIW R 3.6 on Windows is no longer in our matrix. Unless gcc 4.8 is interesting for other reasons, you can probably remove this workaround.
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -581,6 +586,8 @@ class PostgresTypeResolver {
}
private:
+ friend class PostgresTypeTest_BuiltinResolver_Test;
Review Comment:
Either way. Visiting all oid -> PostgresType mappings is a pretty reasonable thing to do and keeps the test details out of this.
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#issuecomment-5693497555
Added the getter.
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#issuecomment-5634272034
I still need to figure out what's going on with CrateDB but otherwise this is working.
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#discussion_r4023360177
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -581,6 +586,8 @@ class PostgresTypeResolver {
}
private:
+ friend class PostgresTypeTest_BuiltinResolver_Test;
+
std::unordered_map<uint32_t, PostgresType> mapping_;
// We can't use PostgresTypeId as an unordered map key because there is no
// built-in hasher for an enum on gcc 4.8 (i.e., R 3.6 on Windows).
Review Comment:
I'll fix this separately
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -581,6 +586,8 @@ class PostgresTypeResolver {
}
private:
+ friend class PostgresTypeTest_BuiltinResolver_Test;
Review Comment:
Added a getter
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#discussion_r4001997033
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -581,6 +586,8 @@ class PostgresTypeResolver {
}
private:
+ friend class PostgresTypeTest_BuiltinResolver_Test;
Review Comment:
Perhaps, but this is finer-grained since we're only exposing it to the test itself
--
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
Re: [PR] feat(c/driver/postgresql): use preinitialized type resolver [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #4775:
URL: https://github.com/apache/arrow-adbc/pull/4775#discussion_r4001774670
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -24,8 +24,13 @@
#include <utility>
#include <vector>
+#include <libpq-fe.h>
#include <nanoarrow/nanoarrow.hpp>
+#include "driver/framework/status.h"
+
+using adbc::driver::Status;
Review Comment:
If it's all the same, it's probably better to move this to its own header/cc pair. The postgres_type and postgres_copy are otherwise nicely self-contained and oblivious of libpq or the driver framework.
##########
c/driver/postgresql/codegen/pgtype.py:
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Parse pg_type.dat and generate C++ code.
+"""
Review Comment:
This would benefit from a bit more detail (where does pg_type.dat come from, roughly what does it look like, and roughly what does this generate?)
##########
c/driver/postgresql/database.cc:
##########
@@ -112,6 +130,23 @@ AdbcStatusCode PostgresDatabase::SetOption(const char* key, const char* value,
InternalAdbcSetError(error, "[libpq] Invalid value for option %s=%s", key, value);
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ } else if (strcmp(key, "adbc.postgresql.internal_rebuild_type_resolver") == 0) {
+ // TODO:
+ return Init(error);
+ } else if (strcmp(key, "adbc.postgresql.type_resolver_mode") == 0) {
+ if (std::strcmp(value, "auto") == 0) {
+ // TODO(lidavidm): implement fallback-based modes
+ // type_resolver_mode_ = TypeResolverMode::kAuto;
+ InternalAdbcSetError(error, "[libpq] %s=%s not yet supported", key, value);
Review Comment:
Issue reference?
##########
c/driver/postgresql/codegen/pgtype.cc:
##########
@@ -0,0 +1,889 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// !! DO NOT EDIT!!
+// Auto-generated by pgtype.py.
+
+#include "postgresql/postgres_type.h"
+
+#include <nanoarrow/nanoarrow.hpp>
+
+#include "driver/framework/status.h"
+
+using adbc::driver::Status;
+
+struct ArrowError;
+
+namespace adbcpq {
+
+Status InitializeTypeResolver(PostgresTypeResolver& resolver) {
+ PostgresTypeResolver::Item type_item;
+ type_item.class_oid = 0;
+ type_item.base_oid = 0;
+ ArrowError na_error = {};
+
+ type_item.oid = 16;
+ type_item.typname = "bool";
+ type_item.typreceive = "boolrecv";
+ type_item.child_oid = 0;
+ UNWRAP_NANOARROW(na_error, Internal, resolver.Insert(type_item, &na_error));
+ type_item.oid = 1000;
+ type_item.typname = "_bool";
+ type_item.typreceive = "array_recv";
+ type_item.child_oid = 16;
+ UNWRAP_NANOARROW(na_error, Internal, resolver.Insert(type_item, &na_error));
Review Comment:
Do you need to generate this or can you generate something more compact like
```c
std::array<PostgresTypeResolver::Item, XX> kItems = {{...}, {...}, ...};
```
and then loop in non-generated code
```c
Status InitializeTypeResolver(PostgresTypeResolver& resolver) {
for (const auto& item : kItems) {
// insert
}
}
```
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -1149,4 +1156,27 @@ static inline std::vector<PostgresTypeId> PostgresTypeIdAll(bool nested) {
return base;
}
+enum class TypeResolverMode {
+ // (Not yet implemented) start as kBuiltin, and query the database if a type
+ // is not found. Eventually, only query the database for types that are
+ // unknown instead of rebuilding the entire resolver.
Review Comment:
It may be less confusing to leave out auto mode until you actually implement it
##########
c/driver/postgresql/postgres_type.h:
##########
@@ -581,6 +586,8 @@ class PostgresTypeResolver {
}
private:
+ friend class PostgresTypeTest_BuiltinResolver_Test;
Review Comment:
Can you also just make public the pieces you need for this?
##########
c/driver/postgresql/codegen/pgtype.py:
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Parse pg_type.dat and generate C++ code.
+"""
+
+import argparse
+import ast
+import dataclasses
+import types
+from pathlib import Path
+
+
+@dataclasses.dataclass(frozen=True)
+class PgTypeDef:
+ oid: int
+ typalign: str
+ typbyval: str
+ typcategory: str
+ typinput: str
+ typlen: str
+ typname: str
+ typoutput: str
+ typreceive: str
+ typsend: str
+ descr: str = ""
+ array_type_oid: int | None = None
+ typanalyze: str | None = None
+ typarray: str | None = None
+ typcollation: str | None = None
+ typdelim: str | None = None
+ typelem: str | None = None
+ typispreferred: str | None = None
+ typmodin: str | None = None
+ typmodout: str | None = None
+ typrelid: str | None = None
+ typstorage: str | None = None
+ typsubscript: str | None = None
+ typtype: str | None = None
+
+
+template = """// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// !! DO NOT EDIT!!
+// Auto-generated by pgtype.py.
+
+#include "postgresql/postgres_type.h"
+
+#include <nanoarrow/nanoarrow.hpp>
+
+#include "driver/framework/status.h"
+
+using adbc::driver::Status;
+
+struct ArrowError;
+
+namespace adbcpq {{
+
+Status InitializeTypeResolver(PostgresTypeResolver& resolver) {{
+ PostgresTypeResolver::Item type_item;
+ type_item.class_oid = 0;
+ type_item.base_oid = 0;
+ ArrowError na_error = {{}};
+
+{func_body}
+ return Status::Ok();
+}}
+
+}} // namespace adbcpq
+""" # noqa:E501
+
+
+def parse(pgtypedat):
+ # Not a real parser, just kitbashing
Review Comment:
Maybe either remove this or document the constraints of this parser?
--
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