lidavidm commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2446392617
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -36,26 +36,173 @@ SQLRETURN SQLAllocHandle(SQLSMALLINT type, SQLHANDLE
parent, SQLHANDLE* result)
ARROW_LOG(DEBUG) << "SQLAllocHandle called with type: " << type
<< ", parent: " << parent
<< ", result: " << static_cast<const void*>(result);
- // GH-46096 TODO: Implement SQLAllocEnv
- // GH-46097 TODO: Implement SQLAllocConnect, pre-requisite requires
SQLAllocEnv
- // implementation
-
- // GH-47706 TODO: Implement SQLAllocStmt, pre-requisite requires
+ // GH-47706 TODO: Add tests for SQLAllocStmt, pre-requisite requires
// SQLDriverConnect implementation
- // GH-47707 TODO: Implement SQL_HANDLE_DESC for
+ // GH-47707 TODO: Add tests for SQL_HANDLE_DESC implementation for
// descriptor handle, pre-requisite requires SQLAllocStmt
- return SQL_INVALID_HANDLE;
+
+ *result = nullptr;
+
+ switch (type) {
+ case SQL_HANDLE_ENV: {
+ using driver::flight_sql::FlightSqlDriver;
+ using ODBC::ODBCEnvironment;
+
+ *result = SQL_NULL_HENV;
+
+ try {
+ static std::shared_ptr<FlightSqlDriver> odbc_driver =
+ std::make_shared<FlightSqlDriver>();
+ *result = reinterpret_cast<SQLHENV>(new ODBCEnvironment(odbc_driver));
+
+ return SQL_SUCCESS;
+ } catch (const std::bad_alloc&) {
+ // allocating environment failed so cannot log diagnostic error here
+ return SQL_ERROR;
+ }
+ }
+
+ case SQL_HANDLE_DBC: {
+ using ODBC::ODBCConnection;
+ using ODBC::ODBCEnvironment;
+
+ *result = SQL_NULL_HDBC;
+
+ ODBCEnvironment* environment =
reinterpret_cast<ODBCEnvironment*>(parent);
+
+ return ODBCEnvironment::ExecuteWithDiagnostics(environment, SQL_ERROR,
[=]() {
+ std::shared_ptr<ODBCConnection> conn = environment->CreateConnection();
+
+ if (conn) {
+ *result = reinterpret_cast<SQLHDBC>(conn.get());
+
+ return SQL_SUCCESS;
+ }
+
+ return SQL_ERROR;
+ });
+ }
+
+ case SQL_HANDLE_STMT: {
+ using ODBC::ODBCConnection;
+ using ODBC::ODBCStatement;
+
+ *result = SQL_NULL_HSTMT;
+
+ ODBCConnection* connection = reinterpret_cast<ODBCConnection*>(parent);
+
+ return ODBCConnection::ExecuteWithDiagnostics(connection, SQL_ERROR,
[=]() {
+ std::shared_ptr<ODBCStatement> statement =
connection->CreateStatement();
+
+ if (statement) {
+ *result = reinterpret_cast<SQLHSTMT>(statement.get());
+
+ return SQL_SUCCESS;
+ }
+
+ return SQL_ERROR;
+ });
+ }
+
+ case SQL_HANDLE_DESC: {
+ using ODBC::ODBCConnection;
+ using ODBC::ODBCDescriptor;
+
+ *result = SQL_NULL_HDESC;
+
+ ODBCConnection* connection = reinterpret_cast<ODBCConnection*>(parent);
+
+ return ODBCConnection::ExecuteWithDiagnostics(connection, SQL_ERROR,
[=]() {
+ std::shared_ptr<ODBCDescriptor> descriptor =
connection->CreateDescriptor();
+
+ if (descriptor) {
+ *result = reinterpret_cast<SQLHDESC>(descriptor.get());
+
+ return SQL_SUCCESS;
+ }
+
+ return SQL_ERROR;
+ });
+ }
+
+ default:
+ break;
+ }
+
+ return SQL_ERROR;
}
SQLRETURN SQLFreeHandle(SQLSMALLINT type, SQLHANDLE handle) {
ARROW_LOG(DEBUG) << "SQLFreeHandle called with type: " << type
<< ", handle: " << handle;
- // GH-46096 TODO: Implement SQLFreeEnv
- // GH-46097 TODO: Implement SQLFreeConnect
- // GH-47706 TODO: Implement SQLFreeStmt
- // GH-47707 TODO: Implement SQL_HANDLE_DESC for descriptor handle
- return SQL_INVALID_HANDLE;
+ // GH-47706 TODO: Add tests for SQLFreeStmt, pre-requisite requires
+ // SQLAllocStmt tests
+
+ // GH-47707 TODO: Add tests for SQL_HANDLE_DESC implementation for
+ // descriptor handle
+ switch (type) {
+ case SQL_HANDLE_ENV: {
+ using ODBC::ODBCEnvironment;
+
+ ODBCEnvironment* environment =
reinterpret_cast<ODBCEnvironment*>(handle);
+
+ if (!environment) {
+ return SQL_INVALID_HANDLE;
+ }
+
+ delete environment;
+
+ return SQL_SUCCESS;
+ }
+
+ case SQL_HANDLE_DBC: {
+ using ODBC::ODBCConnection;
+
+ ODBCConnection* conn = reinterpret_cast<ODBCConnection*>(handle);
+
+ if (!conn) {
+ return SQL_INVALID_HANDLE;
+ }
+
+ conn->ReleaseConnection();
+
+ return SQL_SUCCESS;
Review Comment:
Can we also explain this inline as with the other comment?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,107 @@
+// 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 "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include <gtest/gtest.h>
+
+namespace arrow::flight::sql::odbc {
+
+template <typename T>
+class ConnectionTest : public T {
+ public:
+ using List = std::list<T>;
+};
+
+class ConnectionRemoteTest : public FlightSQLODBCRemoteTestBase {};
+using TestTypes = ::testing::Types<FlightSQLODBCMockTestBase,
ConnectionRemoteTest>;
Review Comment:
Shouldn't this still just be parametrized on `FlightSQLODBCRemoteTestBase`
and `FlightSQLODBCMockTestBase`? I don't see why one needs a new class but not
the other.
--
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]
