You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alinaliBQ (via GitHub)" <gi...@apache.org> on 2025/10/08 19:42:33 UTC
[PR] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
alinaliBQ opened a new pull request, #47759:
URL: https://github.com/apache/arrow/pull/47759
### Rationale for this change
ODBC driver needs to handle environment handle and connection handle allocation and deallocation.
### What changes are included in this PR?
- Implement SQLAllocHandle and SQLFreeHandle APIs for allocating and deallocating environment and connection handles.
- Tests
### Are these changes tested?
- Yes, in CI
### Are there any user-facing changes?
No
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47759:
URL: https://github.com/apache/arrow/pull/47759#issuecomment-3382982612
:warning: GitHub issue #46096 **has been automatically assigned in GitHub** to PR creator.
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2449226424
##########
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:
I added a TODO `GH-46574` comment to make it clearer
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2449236960
##########
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:
Yes, added comment for 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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2450440652
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,108 @@
+// 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>;
+};
Review Comment:
```suggestion
template <typename T>
class ConnectionTest : public T {
};
```
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2434242669
##########
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;
+ }
Review Comment:
I think this needs to be noted inline; as-is this looks suspicious to anyone reading the code later
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2434260893
##########
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;
+ }
Review Comment:
sure, added comment
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2446227157
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
+}
+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+ // ODBC Environment
+ SQLHENV env;
Review Comment:
added calls to free the result
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
Review Comment:
sure, changed to use `ASSERT_NE`
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
Review Comment:
fixed
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
+}
+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+}
+
+TEST(SQLAllocHandle, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+
+ // Allocate a connection using alloc handle
+ SQLRETURN return_alloc_handle = SQLAllocHandle(SQL_HANDLE_DBC, env, &conn);
+
+ EXPECT_EQ(SQL_SUCCESS, return_alloc_handle);
+}
+
+TEST(SQLAllocConnect, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+
+ // Allocate a connection using alloc handle
+ SQLRETURN return_alloc_connect = SQLAllocConnect(env, &conn);
Review Comment:
added call to free result
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
+}
+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+}
+
+TEST(SQLAllocHandle, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+
+ // Allocate a connection using alloc handle
+ SQLRETURN return_alloc_handle = SQLAllocHandle(SQL_HANDLE_DBC, env, &conn);
+
+ EXPECT_EQ(SQL_SUCCESS, return_alloc_handle);
+}
+
+TEST(SQLAllocConnect, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
Review Comment:
added call to free env.
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
Review Comment:
remove `ODBC environment` comment
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
Review Comment:
yup, added check and freed the result
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2446258281
##########
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>;
+TYPED_TEST_SUITE(ConnectionTest, TestTypes);
Review Comment:
we added the separate test fixture here
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2434192530
##########
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;
+ }
Review Comment:
The function is creating a connection here so `get` is used, and when `environment->CreateConnection()` is called, the result `shared_ptr` is kept in a `std::vector` of connections inside the environment handle. As long as the parent environment handle is alive, the connection `shared_ptr` will be kept alive unless the user frees the connection.
https://github.com/apache/arrow/blob/541cba859c3cdb4cc6cb3f456aaae79f67c7d57d/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_environment.cc#L66-L73
##########
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:
`ReleaseConnection` does the equivalent of `delete`. `ReleaseConnection` removes the connection shared_ptr from the `std::vector` of connections, and the shared_ptr is automatically destructed afterwards.
https://github.com/apache/arrow/blob/541cba859c3cdb4cc6cb3f456aaae79f67c7d57d/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc#L688-L691
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #47759:
URL: https://github.com/apache/arrow/pull/47759
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2415691878
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
+}
+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+}
+
+TEST(SQLAllocHandle, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+
+ // Allocate a connection using alloc handle
+ SQLRETURN return_alloc_handle = SQLAllocHandle(SQL_HANDLE_DBC, env, &conn);
+
+ EXPECT_EQ(SQL_SUCCESS, return_alloc_handle);
+}
+
+TEST(SQLAllocConnect, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+
+ // Allocate a connection using alloc handle
+ SQLRETURN return_alloc_connect = SQLAllocConnect(env, &conn);
Review Comment:
Should we free this?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
Review Comment:
Do we need to use GMock here?
How about `ASSERT_NE(env, nullptr)`?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
+}
+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+}
+
+TEST(SQLAllocHandle, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_value);
+
+ // Allocate a connection using alloc handle
+ SQLRETURN return_alloc_handle = SQLAllocHandle(SQL_HANDLE_DBC, env, &conn);
+
+ EXPECT_EQ(SQL_SUCCESS, return_alloc_handle);
+}
+
+TEST(SQLAllocConnect, TestSQLAllocHandleConnect) {
+ // ODBC Environment
+ SQLHENV env;
+ SQLHDBC conn;
+
+ // Allocate an environment handle
+ SQLRETURN return_value = SQLAllocEnv(&env);
Review Comment:
Should we free `env`?
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2446365383
##########
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>;
+TYPED_TEST_SUITE(ConnectionTest, TestTypes);
Review Comment:
Ok, I think I have more concerns here now, but I'll leave that on the testing PR.
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2449220237
##########
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:
The reason we have a new class for `ConnectionRemoteTest` is that `connection_test.cc` will have tests specifically for remote test servers (which will be added in future PRs), and in `connection_test.cc`, they don't have tests that are only running on the mock server. The purpose of `ConnectionRemoteTest` is just to give it a specific name
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47759:
URL: https://github.com/apache/arrow/pull/47759#issuecomment-3435424761
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 62afc09cf67cb96b18f2ce5fa93bb03f2c5506db.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/53454437791) has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2452997106
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,108 @@
+// 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>;
+};
Review Comment:
Rebased and removed `using List`
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47759:
URL: https://github.com/apache/arrow/pull/47759#issuecomment-3383021818
@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR
--
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] GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47759:
URL: https://github.com/apache/arrow/pull/47759#discussion_r2415179276
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
Review Comment:
nit: I don't think these comments add much
##########
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:
Don't we need a `delete` here?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
Review Comment:
Should we check the return code? (Should we free the result?)
##########
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;
+ }
Review Comment:
Does this not immediately release the shared_ptr? Shouldn't it be `conn.release()`?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);
+
+ EXPECT_THAT(env, ::testing::NotNull());
+}
+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+ // ODBC Environment
+ SQLHENV env;
Review Comment:
ditto, should we check/free the result?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,170 @@
+// 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"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
Review Comment:
Should be `<>` includes
--
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