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 + + [email protected](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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
