Copilot commented on code in PR #49784:
URL: https://github.com/apache/arrow/pull/49784#discussion_r3150968996
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h:
##########
@@ -41,12 +41,18 @@
static constexpr std::string_view kTestConnectStr =
"ARROW_FLIGHT_SQL_ODBC_CONN";
static constexpr std::string_view kTestDsn = "Apache Arrow Flight SQL Test
DSN";
-inline SQLHENV env = 0;
-inline SQLHDBC conn = 0;
-inline SQLHSTMT stmt = 0;
+inline std::string remote_test_connect_str = "";
-inline bool skipping_test = false;
-inline bool connected = false;
+struct OdbcHandles {
+ SQLHENV env = SQL_NULL_HENV;
+ SQLHDBC conn = SQL_NULL_HDBC;
+ SQLHSTMT stmt = SQL_NULL_HSTMT;
+};
+
+inline OdbcHandles remote_odbcv3_handles;
+inline OdbcHandles remote_odbcv2_handles;
+inline OdbcHandles mock_odbcv3_handles;
+inline OdbcHandles mock_odbcv2_handles;
Review Comment:
`OdbcHandles` includes a `stmt` field, but it is never initialized anywhere
(connections are established in `OdbcTestEnvironment`, while statements are
allocated per-test in `ODBCTestBase::SetUp()`). Keeping an always-null `stmt`
in the shared handle bundle is misleading; consider removing it (and the
related assignments) or populating it explicitly if you intend to reuse a
single statement handle too.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h:
##########
@@ -61,17 +67,19 @@ namespace arrow::flight::sql::odbc {
class ODBCTestBase : public ::testing::Test {
public:
/// \brief Allocate environment and connection handles
- static void AllocEnvConnHandles(SQLINTEGER odbc_ver = SQL_OV_ODBC3);
+ static void AllocEnvConnHandles(SQLHENV& env_handle, SQLHDBC& conn_handle,
+ SQLINTEGER odbc_ver = SQL_OV_ODBC3);
/// \brief Free environment and connection handles
- static void FreeEnvConnHandles();
+ static void FreeEnvConnHandles(SQLHENV& env_handle, SQLHDBC& conn_handle);
/// \brief Connect to Arrow Flight SQL server using connection string
defined in
/// environment variable "ARROW_FLIGHT_SQL_ODBC_CONN", allocate statement
handle.
/// Connects using ODBC Ver 3 by default
- static void Connect(std::string connect_str, SQLINTEGER odbc_ver =
SQL_OV_ODBC3);
+ static void Connect(std::string connect_str, SQLHENV& env_handle, SQLHDBC&
conn_handle,
+ SQLINTEGER odbc_ver = SQL_OV_ODBC3);
/// \brief Connect to Arrow Flight SQL server using connection string
Review Comment:
The doc comment for `Connect` says it allocates a statement handle, but the
updated signature only sets up env/conn and `ConnectWithString` only calls
`SQLDriverConnect`. Update the comment (and/or function name/docs) to reflect
that statement handles are allocated per-test in `ODBCTestBase::SetUp()` now.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -400,20 +445,24 @@ void ODBCMockTestBase::DropUnicodeTable() {
}
void FlightSQLODBCMockTestBase::SetUpTestSuite() {
- std::string connect_str = GetConnectionString();
- Connect(connect_str, SQL_OV_ODBC3);
- connected = true;
+ env = mock_odbcv3_handles.env;
+ conn = mock_odbcv3_handles.conn;
+ stmt = mock_odbcv3_handles.stmt;
}
Review Comment:
`mock_odbcv3_handles.stmt` is never set (statements are allocated in
`ODBCTestBase::SetUp()`), so assigning `stmt = mock_odbcv3_handles.stmt` is
redundant/misleading. Consider dropping the `stmt` assignment here (and the
`stmt` field from `OdbcHandles`) so the reuse model is clearly env/conn-only.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -168,68 +228,53 @@ std::wstring ODBCTestBase::GetQueryAllDataTypes() {
}
void ODBCTestBase::SetUp() {
- if (connected) {
- ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, conn, &stmt));
- }
+ ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, conn, &stmt));
}
void ODBCTestBase::TearDown() {
- if (connected) {
- ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
- }
-}
-
-void ODBCTestBase::TearDownTestSuite() {
- if (connected) {
- Disconnect();
- connected = false;
- }
-}
-
-void FlightSQLODBCRemoteTestBase::CheckForRemoteTest() {
- if (arrow::internal::GetEnvVar(kTestConnectStr.data()).ValueOr("").empty()) {
- skipping_test = true;
- GTEST_SKIP() << "Skipping test: kTestConnectStr not set";
- }
+ ASSERT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
}
void FlightSQLODBCRemoteTestBase::SetUpTestSuite() {
- CheckForRemoteTest();
- if (skipping_test) {
+ if (!RunningRemoteTests()) {
+ GTEST_SKIP() << "Skipping Test Suite: Environment Variable " <<
kTestConnectStr.data()
+ << " is not set";
return;
}
- std::string connect_str = GetConnectionString();
- Connect(connect_str, SQL_OV_ODBC3);
- connected = true;
+ env = remote_odbcv3_handles.env;
+ conn = remote_odbcv3_handles.conn;
+ stmt = remote_odbcv3_handles.stmt;
}
Review Comment:
`remote_odbcv3_handles.stmt` is never set (statements are allocated in
`ODBCTestBase::SetUp()`), so assigning `stmt = remote_odbcv3_handles.stmt` just
forces `ODBCTestBase::stmt` to `SQL_NULL_HSTMT` at suite setup. Consider
removing the `stmt` assignment here (and the `stmt` field from `OdbcHandles`)
to avoid confusion about what is actually being reused.
--
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]
