Copilot commented on code in PR #4286: URL: https://github.com/apache/arrow-adbc/pull/4286#discussion_r3181891426
########## c/driver/db2/meson.build: ########## @@ -0,0 +1,44 @@ +# 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. + +db2_dep = meson.get_compiler('c').find_library('db2', required: true) + +adbc_db2_driver_lib = library( Review Comment: This Meson file is never reachable from the current build: `c/meson.build` has no `db2` option and never calls `subdir('driver/db2')`. As written, Meson users cannot build the new driver at all despite the PR claiming Meson integration. ########## c/driver/db2/connection.cc: ########## @@ -0,0 +1,60 @@ +// 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. + +#include "connection.h" + +#include "error.h" + +namespace adbc::db2 { + +Status Db2Connection::InitImpl(void* parent) { + database_ = reinterpret_cast<Db2Database*>(parent); + + UNWRAP_RESULT(hdbc_, database_->AllocConnection()); + + // SQLDriverConnect mutates the input string; copy to a stable buffer. + const std::string& conn_str = database_->connection_string(); + SQLCHAR out_str[1024]; + SQLSMALLINT out_len = 0; + SQLRETURN rc = SQLDriverConnect( + hdbc_, /*WindowHandle=*/nullptr, + const_cast<SQLCHAR*>(reinterpret_cast<const SQLCHAR*>(conn_str.c_str())), + static_cast<SQLSMALLINT>(conn_str.size()), out_str, sizeof(out_str), &out_len, Review Comment: This passes `std::string::c_str()` to `SQLDriverConnect` via `const_cast`, even though the comment correctly notes that the API may modify the input buffer. If the CLI writes into that pointer, we're mutating storage that was obtained as `const`, which is undefined behavior and can corrupt the connection string buffer. ########## c/driver/db2/meson.build: ########## @@ -0,0 +1,44 @@ +# 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. + +db2_dep = meson.get_compiler('c').find_library('db2', required: true) + Review Comment: Meson only locates `libdb2` here; it never discovers or adds the Db2 CLI headers. On the common `clidriver` layout from `DB2_HOME`/`IBM_DB_HOME`, `sqlcli1.h` lives outside the compiler's default include path, so Meson builds will fail even when the library is installed correctly. ########## c/driver/db2/database.cc: ########## @@ -0,0 +1,160 @@ +// 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. + +#include "database.h" + +#include <cctype> +#include <sstream> +#include <string> +#include <string_view> + +#include <arrow-adbc/driver/db2.h> + +#include "error.h" + +namespace adbc::db2 { + +bool Db2Database::IsValidPort(std::string_view port) const { + if (port.empty()) return false; + int value = 0; + for (char c : port) { + if (!std::isdigit(static_cast<unsigned char>(c))) return false; + value = value * 10 + (c - '0'); + if (value > 65535) return false; + } + return value > 0; +} + +void Db2Database::BuildConnectionString() { + // If the user supplied a complete CLI/ODBC connection string via "uri", + // honor it as-is and ignore the per-field options. + if (!conn_str_.empty()) return; + + std::ostringstream oss; + if (!database_.empty()) oss << "DATABASE=" << database_ << ";"; + if (!hostname_.empty()) oss << "HOSTNAME=" << hostname_ << ";"; + if (!port_.empty()) oss << "PORT=" << port_ << ";"; + if (!hostname_.empty() || !port_.empty()) oss << "PROTOCOL=TCPIP;"; + if (!uid_.empty()) oss << "UID=" << uid_ << ";"; + if (!pwd_.empty()) oss << "PWD=" << pwd_ << ";"; + conn_str_ = oss.str(); +} + +Status Db2Database::InitImpl() { + BuildConnectionString(); + + if (conn_str_.empty()) { + return status::InvalidArgument( + kErrorPrefix, + " No connection string provided. Set the standard 'uri' option, " + "or the individual options (", + ADBC_DB2_OPTION_DATABASE, ", ", ADBC_DB2_OPTION_HOSTNAME, ", ", + ADBC_DB2_OPTION_PORT, ", ", ADBC_DB2_OPTION_UID, ", ", + ADBC_DB2_OPTION_PWD, ")."); + } + + SQLRETURN rc = SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &henv_); + if (rc != SQL_SUCCESS && rc != SQL_SUCCESS_WITH_INFO) { + return status::Internal(kErrorPrefix, + " Failed to allocate ODBC environment handle (rc=", rc, + ")"); + } + + rc = SQLSetEnvAttr(henv_, SQL_ATTR_ODBC_VERSION, + reinterpret_cast<SQLPOINTER>(SQL_OV_ODBC3), 0); + UNWRAP_STATUS(CheckRc(SQL_HANDLE_ENV, henv_, rc, "SQLSetEnvAttr(ODBC_VERSION)")); + + return status::Ok(); +} + +Status Db2Database::ReleaseImpl() { + if (henv_ != SQL_NULL_HENV) { + SQLFreeHandle(SQL_HANDLE_ENV, henv_); Review Comment: `Db2Database` owns the shared `SQLHENV`, and each `Db2Connection` keeps a raw pointer back to this object. Releasing the database here always frees the environment and deletes the database even if connections are still open, so a later connection release will dereference a freed `Db2Database` and free an HDBC that was allocated from an already-freed environment. ########## docs/source/driver/db2.rst: ########## @@ -0,0 +1,166 @@ +.. 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. + +========== +Db2 Driver +========== + +.. note:: + + This driver is in the early stages of development. The current + release implements **connection management only**: opening and + closing :c:struct:`AdbcDatabase` / :c:struct:`AdbcConnection` + handles, parsing connection options, authenticating against the + server, and mapping Db2 SQLSTATEs to ADBC error codes. + + Statement execution, metadata retrieval (``GetObjects``, + ``GetTableSchema``, ...), transaction management, and bulk + ingestion are **not yet implemented** and will be added in + subsequent pull requests. Calling those entry points returns + ``ADBC_STATUS_NOT_IMPLEMENTED``. Review Comment: This note overstates the current behavior: transaction APIs do not consistently return `ADBC_STATUS_NOT_IMPLEMENTED`. With the current implementation, `Commit`/`Rollback` on a freshly opened connection go through the framework's autocommit path and return `ADBC_STATUS_INVALID_STATE`, so the user-facing docs are inaccurate. ########## c/CMakeLists.txt: ########## @@ -145,6 +151,10 @@ LIBRARY=$<TARGET_FILE:adbc_driver_${TARGET}_shared>" ${Python3_EXECUTABLE} -m pi if(ADBC_DRIVER_SNOWFLAKE) adbc_install_python_package(snowflake) endif() + + if(ADBC_DRIVER_DB2) Review Comment: Enabling both `ADBC_BUILD_PYTHON` and `ADBC_DRIVER_DB2` will now fail because this target tries to `pip install` `python/adbc_driver_db2`, but that package directory does not exist in the repo. The other drivers only add this hook once their Python package is present. ########## c/driver/db2/db2_test.cc: ########## @@ -0,0 +1,357 @@ +// 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. + +#include <cstring> +#include <string> + +#include <arrow-adbc/adbc.h> +#include <arrow-adbc/driver/db2.h> +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include "validation/adbc_validation.h" +#include "validation/adbc_validation_util.h" + +using adbc_validation::IsOkStatus; +using adbc_validation::IsStatus; + +namespace { + +// Read the connection URI from the environment. When unset, all +// tests that need a live Db2 server skip. +const char* GetTestUri() { + const char* uri = std::getenv("ADBC_DB2_TEST_URI"); + return (uri != nullptr && uri[0] != '\0') ? uri : nullptr; +} + +class Db2Quirks : public adbc_validation::DriverQuirks { + public: + AdbcStatusCode SetupDatabase(struct AdbcDatabase* database, + struct AdbcError* error) const override { + const char* uri = GetTestUri(); + if (uri == nullptr) return ADBC_STATUS_INVALID_STATE; + return AdbcDatabaseSetOption(database, "uri", uri, error); + } + + // The driver currently scopes itself to connection management; the + // following capabilities are intentionally disabled until follow-up + // PRs add execution, transactions, ingestion, and metadata. + bool supports_bulk_ingest(const char* /*mode*/) const override { return false; } + bool supports_concurrent_statements() const override { return false; } + bool supports_transactions() const override { return false; } + bool supports_get_sql_info() const override { return false; } + bool supports_get_objects() const override { return false; } + bool supports_metadata_current_catalog() const override { return false; } + bool supports_metadata_current_db_schema() const override { return false; } + bool supports_partitioned_data() const override { return false; } + bool supports_dynamic_parameter_binding() const override { return false; } + bool supports_error_on_incompatible_schema() const override { return false; } + bool supports_ingest_view_types() const override { return false; } + bool supports_ingest_float16() const override { return false; } +}; + +} // namespace + +// --------------------------------------------------------------------------- +// AdbcDatabase lifecycle (matches the standard ADBCV_TEST_DATABASE coverage) +// --------------------------------------------------------------------------- + +class Db2DatabaseTest : public ::testing::Test, public adbc_validation::DatabaseTest { + public: + const adbc_validation::DriverQuirks* quirks() const override { return &quirks_; } + void SetUp() override { + if (GetTestUri() == nullptr) { + GTEST_SKIP() << "ADBC_DB2_TEST_URI not set"; + } + ASSERT_NO_FATAL_FAILURE(SetUpTest()); + } + void TearDown() override { + if (GetTestUri() != nullptr) ASSERT_NO_FATAL_FAILURE(TearDownTest()); + } + + protected: + Db2Quirks quirks_; +}; +ADBCV_TEST_DATABASE(Db2DatabaseTest) + +// --------------------------------------------------------------------------- +// AdbcConnection lifecycle +// +// We deliberately do not invoke ADBCV_TEST_CONNECTION here because the bulk +// of those tests exercise metadata APIs (GetObjects/GetTableSchema/...) and +// transactions that are out of scope for this initial driver. We instead +// reuse the framework's connection-flow helpers directly so behavior stays +// in lock-step with the other ADBC drivers. +// --------------------------------------------------------------------------- + +class Db2ConnectionFlowTest : public ::testing::Test, + public adbc_validation::ConnectionTest { + public: + const adbc_validation::DriverQuirks* quirks() const override { return &quirks_; } + void SetUp() override { + if (GetTestUri() == nullptr) { + GTEST_SKIP() << "ADBC_DB2_TEST_URI not set"; + } + ASSERT_NO_FATAL_FAILURE(SetUpTest()); + } + void TearDown() override { + if (GetTestUri() != nullptr) ASSERT_NO_FATAL_FAILURE(TearDownTest()); + } + + protected: + Db2Quirks quirks_; +}; + +TEST_F(Db2ConnectionFlowTest, NewInit) { TestNewInit(); } +TEST_F(Db2ConnectionFlowTest, Release) { TestRelease(); } +TEST_F(Db2ConnectionFlowTest, Concurrent) { TestConcurrent(); } +TEST_F(Db2ConnectionFlowTest, AutocommitDefault) { TestAutocommitDefault(); } + +// --------------------------------------------------------------------------- +// Db2-specific connection-management tests (option parsing, error mapping) +// --------------------------------------------------------------------------- + +class Db2ConnectionOptionsTest : public ::testing::Test { + protected: + void SetUp() override { + if (GetTestUri() == nullptr) { + GTEST_SKIP() << "ADBC_DB2_TEST_URI not set"; + } Review Comment: This fixture skips **all** option tests when `ADBC_DB2_TEST_URI` is unset, including the purely local validation cases (`UnknownOptionIsRejected`, `MalformedPortIsRejected`, `EmptyUidOrPwdIsRejected`, etc.) that do not need a live Db2 server. That means most CI environments will never exercise the new option-parsing/validation logic. -- 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]
