Code Clean up and enable ODBC tests in CI

Still need to modify ODBCUtilEnvironment if we decide to use it. Still need to add ODBC V2 support so a different env and conn is used for ODBC 2 tests.

Author: alinaliBQ

.github/workflows/cpp_windows.yml

@@ -164,7 +164,3 @@ jobs:
           for /f "usebackq delims=" %%I in (`bash -c "cygpath -u \"$VCPKG_ROOT_KEEP\""` ) do set VCPKG_ROOT=%%I
 
           bash -c "ci/scripts/cpp_test.sh $(pwd) $(pwd)/build"
-
-          # Run ODBC test
-          ${{ github.workspace }}\build\cpp\%ARROW_BUILD_TYPE%\arrow-flight-sql-odbc-test.exe
-

ci/scripts/cpp_test.sh

@@ -82,7 +82,6 @@ case "$(uname)" in
     exclude_tests+=("gandiva-precompiled-test")
     exclude_tests+=("gandiva-projector-test")
     exclude_tests+=("gandiva-utf8-test")
-    exclude_tests+=("arrow-flight-sql-odbc-test")
     ;;
   *)
     n_jobs=${NPROC:-1}

cpp/src/arrow/compute/registry.cc

@@ -30,31 +30,14 @@
 #include "arrow/util/config.h"  // For ARROW_COMPUTE
 #include "arrow/util/logging.h"
 
-#include <iostream> // -AL- TEMP remove later
-
 namespace arrow {
 namespace compute {
 
 class FunctionRegistry::FunctionRegistryImpl {
  public:
   explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
-      : parent_(parent) {// -AL- TEMP remove later
-    std::cout << "-AL- FunctionRegistryImpl constructor called\n";
-  }
-  ~FunctionRegistryImpl() { // -AL- TEMP remove later
-    size_t name_size = name_to_function_.size();
-    std::cout << "-AL- ~FunctionRegistryImpl name_to_function_ size:" << name_size <<
-      "\n";
-
-    //auto it = name_to_function_.find(std::string("split_pattern"));
-    //auto func = it->second;
-    //for (auto& kv : name_to_function_) {
-    //  kv.second.reset();
-    //}
-    //name_to_function_.clear();
-
-    std::cout << "-AL- ~FunctionRegistryImpl destructor called\n";
-  }
+      : parent_(parent) {}
+  ~FunctionRegistryImpl() {}
 
   Status CanAddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
     if (parent_ != NULLPTR) {
@@ -144,8 +127,7 @@ class FunctionRegistry::FunctionRegistryImpl {
 
   const Function* cast_function() { return cast_function_; }
 
-  void ClearFunctioRegistry() { name_to_function_.clear();
-  }
+  void ClearFunctioRegistry() { name_to_function_.clear(); }
 
  private:
   // must not acquire mutex
@@ -235,7 +217,6 @@ class FunctionRegistry::FunctionRegistryImpl {
 };
 
 std::unique_ptr<FunctionRegistry> FunctionRegistry::Make() {
-  std::cout << "-AL- Make is called once TEMP \n";
   return std::unique_ptr<FunctionRegistry>(new FunctionRegistry());
 }
 
@@ -298,14 +279,11 @@ int FunctionRegistry::num_functions() const { return impl_->num_functions(); }
 
 const Function* FunctionRegistry::cast_function() const { return impl_->cast_function(); }
 
-void FunctionRegistry::ClearFunctioRegistry() {
-  impl_->ClearFunctioRegistry();
-}
+void FunctionRegistry::ClearFunctioRegistry() { impl_->ClearFunctioRegistry(); }
 
 namespace internal {
 
 static std::unique_ptr<FunctionRegistry> CreateBuiltInRegistry() {
-  std::cout << "-AL- CreateBuiltInRegistry is called once TEMP \n";
   auto registry = FunctionRegistry::Make();
 
   // Register core kernels

cpp/src/arrow/compute/registry.h

@@ -111,8 +111,10 @@ class ARROW_EXPORT FunctionRegistry {
   ///
   /// Helpful for get cast function as needed.
   const Function* cast_function() const;
-  
-  // -AL- todo docs?
+
+  /// \brief Clear function registry
+  ///
+  /// Helpful to avoid segfault from race condition. Call this function before DLL unload.
   void ClearFunctioRegistry();
 
  private:

cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc

@@ -29,11 +29,10 @@
 
 namespace arrow::flight::sql::odbc {
 
-// -AL- todo rename later
-auto alinatest_env = ::testing::AddGlobalTestEnvironment(new AlinaTestEnvironment);
+auto odbc_util_env = ::testing::AddGlobalTestEnvironment(new OdbcUtilEnvironment);
 
-AlinaTestEnvironment* GetAlinaTestEnv() {
-  return ::arrow::internal::checked_cast<AlinaTestEnvironment*>(alinatest_env);
+OdbcUtilEnvironment* GetOdbcUtilEnv() {
+  return ::arrow::internal::checked_cast<OdbcUtilEnvironment*>(odbc_util_env);
 }
 
 void ODBCRemoteTestBase::AllocEnvConnHandles(SQLINTEGER odbc_ver) {
@@ -63,15 +62,15 @@ void ODBCRemoteTestBase::ConnectWithString(std::string connect_str) {
   SQLSMALLINT out_str_len;
 
   // -AL- once local conn handles are removed,can rename back to conn
-   SQLHDBC global_conn = GetAlinaTestEnv()->getConnHandle();
-  //SQLHDBC global_conn = conn;  //-AL- TEMP
+  SQLHDBC global_conn = GetOdbcUtilEnv()->getConnHandle();
+  // SQLHDBC global_conn = conn;  //-AL- TEMP
 
   // Connecting to ODBC server.
   ASSERT_EQ(SQL_SUCCESS,
             SQLDriverConnect(global_conn, NULL, &connect_str0[0],
                              static_cast<SQLSMALLINT>(connect_str0.size()), out_str,
                              kOdbcBufferSize, &out_str_len, SQL_DRIVER_NOPROMPT))
-       << GetOdbcErrorMessage(SQL_HANDLE_DBC, global_conn);
+      << GetOdbcErrorMessage(SQL_HANDLE_DBC, global_conn);
 
   // GH-47710: TODO Allocate a statement using alloc handle
   // ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_STMT, conn, &stmt));
@@ -81,8 +80,8 @@ void ODBCRemoteTestBase::Disconnect() {
   // GH-47710: TODO Close statement
   // EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_STMT, stmt));
 
-    // -AL- once local conn handles are removed,can rename back to conn
-  SQLHDBC global_conn = GetAlinaTestEnv()->getConnHandle();
+  // -AL- once local conn handles are removed,can rename back to conn
+  SQLHDBC global_conn = GetOdbcUtilEnv()->getConnHandle();
 
   // Disconnect from ODBC
   EXPECT_EQ(SQL_SUCCESS, SQLDisconnect(global_conn))

cpp/src/arrow/flight/sql/odbc/tests/odbc_test_util.h

@@ -19,7 +19,7 @@
 
 #include "arrow/flight/sql/odbc/odbc_impl/platform.h"
 
-#include "arrow/compute/api.h" // -AL- attempt to keep registry alive
+#include "arrow/compute/api.h"
 
 #include <sql.h>
 #include <sqlucode.h>
@@ -29,10 +29,9 @@
 namespace arrow::flight::sql::odbc {
 
 // -AL- todo update to actual ODBC allocation
-class AlinaTestEnvironment : public ::testing::Environment {
+class OdbcUtilEnvironment : public ::testing::Environment {
  public:
   void SetUp() override {
-    int x = 1;
     // -AL- todo add env_v2 for v2 after global setup/teardown works.
 
     // Allocate an environment handle
@@ -45,27 +44,20 @@ class AlinaTestEnvironment : public ::testing::Environment {
 
     // Allocate a connection using alloc handle
     ASSERT_EQ(SQL_SUCCESS, SQLAllocHandle(SQL_HANDLE_DBC, env, &conn));
-    std::cout << "-AL- AlinaTestEnvironment::SetUp\n";
+    std::cout << "-AL- OdbcUtilEnvironment::SetUp\n";
   }
 
   void TearDown() override {
-
-
-    // -AL- this doesn't work
-    //// Remove function registry before test exits
+    // Clear function registry before test exits
     auto reg = arrow::compute::GetFunctionRegistry();
     reg->ClearFunctioRegistry();
-    // delete reg;
 
-
-    int y = 1;
     // Free connection handle
     EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));
 
     // Free environment handle
     EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_ENV, env));
-    std::cout << "-AL- AlinaTestEnvironment::TearDown\n";
-
+    std::cout << "-AL- OdbcUtilEnvironment::TearDown\n";
   }
 
   SQLHENV getEnvHandle() { return env; }