diff --git a/.github/workflows/cpp_extra.yml b/.github/workflows/cpp_extra.yml index ca5a3adb4b7..72d3d559c97 100644 --- a/.github/workflows/cpp_extra.yml +++ b/.github/workflows/cpp_extra.yml @@ -330,9 +330,75 @@ jobs: cd cpp/examples/minimal_build ../minimal_build.build/arrow-example - odbc: + odbc-macos: needs: check-labels - name: ODBC + name: ODBC ${{ matrix.architecture }} macOS ${{ matrix.macos-version }} + runs-on: macos-${{ matrix.macos-version }} + if: >- + needs.check-labels.outputs.force == 'true' || + contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra') || + contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra: C++') + timeout-minutes: 75 + strategy: + fail-fast: false + matrix: + include: + - architecture: AMD64 + macos-version: "15-intel" + - architecture: ARM64 + macos-version: "14" + env: + ARROW_BUILD_TESTS: ON + ARROW_FLIGHT_SQL_ODBC: ON + ARROW_HOME: /tmp/local + steps: + - name: Checkout Arrow + uses: actions/checkout@v6.0.0 + with: + fetch-depth: 0 + submodules: recursive + - name: Install Dependencies + run: | + brew bundle --file=cpp/Brewfile + export LIBIODBC_DIR="$(brew --cellar libiodbc)/$(brew list --versions libiodbc | awk '{print $2}')" + echo ODBC_INCLUDE_DIR="$LIBIODBC_DIR/include" >> $GITHUB_ENV + echo ODBC_LIB_DIR="$LIBIODBC_DIR/lib" >> $GITHUB_ENV + - name: Setup ccache + run: | + ci/scripts/ccache_setup.sh + - name: ccache info + id: ccache-info + run: | + echo "cache-dir=$(ccache --get-config cache_dir)" >> $GITHUB_OUTPUT + - name: Cache ccache + uses: actions/cache@v4 + with: + path: ${{ steps.ccache-info.outputs.cache-dir }} + key: cpp-ccache-macos-${{ matrix.macos-version }}-${{ hashFiles('cpp/**') }} + restore-keys: cpp-ccache-macos-${{ matrix.macos-version }}- + - name: Build + run: | + # Homebrew uses /usr/local as prefix. So packages + # installed by Homebrew also use /usr/local/include. We + # want to include headers for packages installed by + # Homebrew as system headers to ignore warnings in them. + # But "-isystem /usr/local/include" isn't used by CMake + # because /usr/local/include is marked as the default + # include path. So we disable -Werror to avoid build error + # by warnings from packages installed by Homebrew. + export BUILD_WARNING_LEVEL=PRODUCTION + ci/scripts/cpp_build.sh $(pwd) $(pwd)/build + - name: Test + shell: bash + run: | + sudo sysctl -w kern.coredump=1 + sudo sysctl -w kern.corefile=/tmp/core.%N.%P + ulimit -c unlimited # must enable within the same shell + ci/scripts/cpp_test.sh $(pwd) $(pwd)/build + + odbc-msvc: + needs: check-labels + name: ODBC Windows runs-on: windows-2022 if: >- needs.check-labels.outputs.force == 'true' || @@ -442,6 +508,7 @@ jobs: - jni-linux - jni-macos - msvc-arm64 - - odbc + - odbc-macos + - odbc-msvc uses: ./.github/workflows/report_ci.yml secrets: inherit diff --git a/ci/scripts/cpp_build.sh b/ci/scripts/cpp_build.sh index 904b5cccb42..dfad0723c1a 100755 --- a/ci/scripts/cpp_build.sh +++ b/ci/scripts/cpp_build.sh @@ -259,7 +259,7 @@ else -DCMAKE_BUILD_TYPE=${ARROW_BUILD_TYPE:-debug} \ -DCMAKE_VERBOSE_MAKEFILE=${CMAKE_VERBOSE_MAKEFILE:-OFF} \ -DCMAKE_C_FLAGS="${CFLAGS:-}" \ - -DCMAKE_CXX_FLAGS="${CXXFLAGS:-}" \ + -DCMAKE_CXX_FLAGS="${CXXFLAGS:-} -I${ODBC_INCLUDE_DIR:-} -L${ODBC_LIB_DIR:-}" \ -DCMAKE_CXX_STANDARD="${CMAKE_CXX_STANDARD:-20}" \ -DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR:-lib} \ -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX:-${ARROW_HOME}} \ @@ -270,6 +270,7 @@ else -DgRPC_SOURCE=${gRPC_SOURCE:-} \ -DGTest_SOURCE=${GTest_SOURCE:-} \ -Dlz4_SOURCE=${lz4_SOURCE:-} \ + -DODBC_INCLUDE_DIR="${ODBC_INCLUDE_DIR:-}" \ -Dopentelemetry-cpp_SOURCE=${opentelemetry_cpp_SOURCE:-} \ -DORC_SOURCE=${ORC_SOURCE:-} \ -DPARQUET_BUILD_EXAMPLES=${PARQUET_BUILD_EXAMPLES:-OFF} \ diff --git a/ci/scripts/cpp_test.sh b/ci/scripts/cpp_test.sh index 0ad59bc308f..515d981ca81 100755 --- a/ci/scripts/cpp_test.sh +++ b/ci/scripts/cpp_test.sh @@ -61,6 +61,7 @@ case "$(uname)" in n_jobs=$(sysctl -n hw.ncpu) # TODO: https://github.com/apache/arrow/issues/40410 exclude_tests+=("arrow-s3fs-test") + exclude_tests+=("arrow-flight-sql-odbc-test") ;; MINGW*) n_jobs=${NUMBER_OF_PROCESSORS:-1} diff --git a/cpp/Brewfile b/cpp/Brewfile index 4c42607568c..811712516bf 100644 --- a/cpp/Brewfile +++ b/cpp/Brewfile @@ -28,6 +28,7 @@ brew "git" brew "glog" brew "googletest" brew "grpc" +brew "libiodbc" brew "llvm" brew "lz4" brew "mimalloc" diff --git a/cpp/CMakePresets.json b/cpp/CMakePresets.json index e6c2e7e43a6..969f80eb91e 100644 --- a/cpp/CMakePresets.json +++ b/cpp/CMakePresets.json @@ -314,6 +314,17 @@ "displayName": "Debug build with tests and Flight SQL", "cacheVariables": {} }, + { + "name": "ninja-debug-flight-sql-odbc", + "inherits": [ + "features-flight-sql", + "base-debug" + ], + "displayName": "Debug build with tests and Flight SQL ODBC", + "cacheVariables": { + "ARROW_FLIGHT_SQL_ODBC": "ON" + } + }, { "name": "ninja-debug-gandiva", "inherits": [ @@ -510,6 +521,17 @@ "displayName": "Release build with Flight SQL", "cacheVariables": {} }, + { + "name": "ninja-release-flight-sql-odbc", + "inherits": [ + "features-flight-sql", + "base-release" + ], + "displayName": "Release build with Flight SQL ODBC", + "cacheVariables": { + "ARROW_FLIGHT_SQL_ODBC": "ON" + } + }, { "name": "ninja-release-gandiva", "inherits": [ diff --git a/cpp/cmake_modules/DefineOptions.cmake b/cpp/cmake_modules/DefineOptions.cmake index 0f6674c7143..5d34ff50e35 100644 --- a/cpp/cmake_modules/DefineOptions.cmake +++ b/cpp/cmake_modules/DefineOptions.cmake @@ -107,8 +107,8 @@ macro(tsort_bool_option_dependencies) endmacro() macro(resolve_option_dependencies) - # Arrow Flight SQL ODBC is available only for Windows for now. - if(NOT WIN32) + # Arrow Flight SQL ODBC is available only for Windows and macOS for now. + if(NOT WIN32 AND NOT APPLE) set(ARROW_FLIGHT_SQL_ODBC OFF) endif() if(MSVC_TOOLCHAIN) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 50ea395ec4f..25782bf541a 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -855,7 +855,7 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle, } #else // Attempt connection without loading DSN window on macOS/Linux - connection->Connect(dsn, properties, missing_properties); + connection->Connect(dsn_value, properties, missing_properties); #endif // Copy connection string to out_connection_string after connection attempt return ODBC::GetStringAttribute(true, connection_string, false, out_connection_string, diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt b/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt index 2fe9c41e3ce..22531d9638e 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt @@ -45,10 +45,10 @@ add_library(arrow_odbc_spi_impl config/connection_string_parser.h diagnostics.cc diagnostics.h - error_codes.h encoding.cc encoding.h encoding_utils.h + error_codes.h exceptions.cc exceptions.h flight_sql_auth_method.cc diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/address_info.cc b/cpp/src/arrow/flight/sql/odbc/odbc_impl/address_info.cc index 5ee6674c3c2..7bdb4d58cf8 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/address_info.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/address_info.cc @@ -16,6 +16,7 @@ // under the License. #include "arrow/flight/sql/odbc/odbc_impl/address_info.h" +#include namespace driver { @@ -34,7 +35,7 @@ bool AddressInfo::GetAddressInfo(const std::string& host, char* host_name_info, } error = getnameinfo(addrinfo_result_->ai_addr, addrinfo_result_->ai_addrlen, - host_name_info, static_cast(max_host), NULL, 0, 0); + host_name_info, static_cast(max_host), NULL, 0, 0); return error == 0; } diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h index 0390a57e52f..9f1ef589b08 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h @@ -22,8 +22,10 @@ #include "arrow/flight/sql/odbc/odbc_impl/platform.h" #include "arrow/flight/sql/odbc/odbc_impl/spi/connection.h" +#if defined _WIN32 || defined _WIN64 // winuser.h needs to be included after windows.h, which is defined in platform.h -#include +# include +#endif namespace arrow::flight::sql::odbc { namespace config { diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_handle.h b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_handle.h index b3fd6e371a2..9dd8fe37baf 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_handle.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_handle.h @@ -46,7 +46,18 @@ class ODBCHandle { try { GetDiagnostics().Clear(); rc = function(); - } catch (const arrow::flight::sql::odbc::DriverException& ex) { + } catch (const arrow::flight::sql::odbc::AuthenticationException& ex) { + GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException( + ex.GetMessageText(), ex.GetSqlState(), ex.GetNativeError())); + } catch (const arrow::flight::sql::odbc::NullWithoutIndicatorException& ex) { + GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException( + ex.GetMessageText(), ex.GetSqlState(), ex.GetNativeError())); + } + // on mac, DriverException doesn't catch the subclass exceptions hence we added + // the following above. + // GH-48278 TODO investigate if there is a way to catch all the subclass exceptions + // under DriverException + catch (const arrow::flight::sql::odbc::DriverException& ex) { GetDiagnostics().AddError(ex); } catch (const std::bad_alloc&) { GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException( diff --git a/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt b/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt index 39284c750b0..ac7a60511fb 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt +++ b/cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt @@ -15,11 +15,6 @@ # specific language governing permissions and limitations # under the License. -add_custom_target(tests) - -find_package(ODBC REQUIRED) -include_directories(${ODBC_INCLUDE_DIRS}) - find_package(SQLite3Alt REQUIRED) set(ARROW_FLIGHT_SQL_MOCK_SERVER_SRCS @@ -52,5 +47,13 @@ add_arrow_test(flight_sql_odbc_test ${SQLite3_LIBRARIES} arrow_odbc_spi_impl) +# On Windows, cmake uses suffix `DIRS` for ODBC include headers, and +# on unix, cmake uses suffix `DIR`. +if(WIN32) + target_include_directories(arrow-flight-sql-odbc-test PUBLIC ${ODBC_INCLUDE_DIRS}) +else() + target_include_directories(arrow-flight-sql-odbc-test PUBLIC ${ODBC_INCLUDE_DIR}) +endif() + # Disable unity build due to sqlite_sql_info.cc conflict with sql.h and sqlext.h headers. set_target_properties(arrow-flight-sql-odbc-test PROPERTIES UNITY_BUILD OFF) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc index b1081bc1d6a..3ca4a50ef76 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc @@ -442,7 +442,7 @@ TEST_F(ConnectionRemoteTest, TestSQLDriverConnectInvalidUid) { arrow::util::UTF8ToWideString(connect_str)); std::vector connect_str0(wconnect_str.begin(), wconnect_str.end()); - SQLWCHAR out_str[kOdbcBufferSize]; + SQLWCHAR out_str[kOdbcBufferSize] = {0}; SQLSMALLINT out_str_len; // Connecting to ODBC server. diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc index 3f12e35c6d6..470a68b3beb 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc @@ -130,9 +130,9 @@ std::wstring ODBCRemoteTestBase::GetQueryAllDataTypes() { CAST(true AS BOOLEAN) AS bit_true, --Character types - 'Z' AS c_char, '你' AS c_wchar, + 'Z' AS c_char, _utf8'你' AS c_wchar, - '你好' AS c_wvarchar, + _utf8'你好' AS c_wvarchar, 'XYZ' AS c_varchar, @@ -245,7 +245,7 @@ std::string ODBCMockTestBase::GetConnectionString() { std::string connect_str( "driver={Apache Arrow Flight SQL ODBC Driver};HOST=localhost;port=" + std::to_string(port) + ";token=" + std::string(kTestToken) + - ";useEncryption=false;"); + ";useEncryption=false;UseWideChar=true;"); return connect_str; } diff --git a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h index 7dd77d8fa62..aee423451c4 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h +++ b/cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h @@ -237,6 +237,9 @@ static constexpr std::string_view kErrorStateHY114 = "HY114"; static constexpr std::string_view kErrorStateHY118 = "HY118"; static constexpr std::string_view kErrorStateHYC00 = "HYC00"; static constexpr std::string_view kErrorStateS1004 = "S1004"; +static constexpr std::string_view kErrorStateS1002 = "S1002"; +static constexpr std::string_view kErrorStateS1010 = "S1010"; +static constexpr std::string_view kErrorStateS1090 = "S1090"; /// Verify ODBC Error State void VerifyOdbcErrorState(SQLSMALLINT handle_type, SQLHANDLE handle, diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_attr_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_attr_test.cc index 5b6821430a1..0a4e99d33a6 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_attr_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_attr_test.cc @@ -63,6 +63,8 @@ void GetStmtAttr(SQLHSTMT statement, SQLINTEGER attribute, SQLPOINTER* value) { SQLGetStmtAttr(statement, attribute, value, SQL_IS_POINTER, &string_length)); } +#if defined(SQL_ATTR_ASYNC_STMT_EVENT) || defined(SQL_ATTR_ASYNC_STMT_PCALLBACK) || \ + defined(SQL_ATTR_ASYNC_STMT_PCONTEXT) // Validate error return value and code void ValidateGetStmtAttrErrorCode(SQLHSTMT statement, SQLINTEGER attribute, std::string_view error_code) { @@ -74,6 +76,8 @@ void ValidateGetStmtAttrErrorCode(SQLHSTMT statement, SQLINTEGER attribute, VerifyOdbcErrorState(SQL_HANDLE_STMT, statement, error_code); } +#endif // SQL_ATTR_ASYNC_STMT_EVENT || SQL_ATTR_ASYNC_STMT_PCALLBACK || + // SQL_ATTR_ASYNC_STMT_PCONTEXT // Validate return value for call to SQLSetStmtAttr with SQLULEN void ValidateSetStmtAttr(SQLHSTMT statement, SQLINTEGER attribute, SQLULEN new_value) { diff --git a/cpp/src/arrow/vendored/whereami/whereami.cc b/cpp/src/arrow/vendored/whereami/whereami.cc index 945226193f9..94437361ec0 100644 --- a/cpp/src/arrow/vendored/whereami/whereami.cc +++ b/cpp/src/arrow/vendored/whereami/whereami.cc @@ -159,7 +159,7 @@ WAI_NOINLINE WAI_FUNCSPEC int WAI_PREFIX(getModulePath)(char* out, int capacity, return length; } -#elif defined(__linux__) || defined(__CYGWIN__) || defined(__sun) || \ +#elif defined(__APPLE__) || defined(__linux__) || defined(__CYGWIN__) || defined(__sun) || \ defined(WAI_USE_PROC_SELF_EXE) # include