Skip to content

Commit d5fa7cb

Browse files
alinaliBQjusting-bqvic-tsang
authored
GH-48575: [C++][FlightRPC] Standalone ODBC macOS CI (#48577)
### Rationale for this change #48575 ### What changes are included in this PR? - Add new ODBC workflow for macOS Intel 15 and 14 arm64. - Added ODBC build fixes to enable build on macOS CI. ### Are these changes tested? Tested in CI and local macOS Intel and M1 environments. ### Are there any user-facing changes? N/A * GitHub Issue: #48575 Lead-authored-by: Alina (Xi) Li <alina.li@improving.com> Co-authored-by: justing-bq <62349012+justing-bq@users.noreply.github.com> Co-authored-by: Victor Tsang <victor.tsang@improving.com> Co-authored-by: Alina (Xi) Li <alinal@bitquilltech.com> Co-authored-by: vic-tsang <victor.tsang@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
1 parent 0dfae70 commit d5fa7cb

File tree

16 files changed

+144
-24
lines changed

16 files changed

+144
-24
lines changed

.github/workflows/cpp_extra.yml

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -336,9 +336,76 @@ jobs:
336336
cd cpp/examples/minimal_build
337337
../minimal_build.build/arrow-example
338338
339-
odbc:
339+
odbc-macos:
340340
needs: check-labels
341-
name: ODBC
341+
name: ODBC ${{ matrix.architecture }} macOS ${{ matrix.macos-version }}
342+
runs-on: macos-${{ matrix.macos-version }}
343+
if: >-
344+
needs.check-labels.outputs.force == 'true' ||
345+
contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra') ||
346+
contains(fromJSON(needs.check-labels.outputs.ci-extra-labels || '[]'), 'CI: Extra: C++')
347+
timeout-minutes: 75
348+
strategy:
349+
fail-fast: false
350+
matrix:
351+
include:
352+
- architecture: AMD64
353+
macos-version: "15-intel"
354+
- architecture: ARM64
355+
macos-version: "14"
356+
env:
357+
ARROW_BUILD_TESTS: ON
358+
ARROW_FLIGHT_SQL_ODBC: ON
359+
ARROW_HOME: /tmp/local
360+
steps:
361+
- name: Checkout Arrow
362+
uses: actions/checkout@v6.0.1
363+
with:
364+
fetch-depth: 0
365+
submodules: recursive
366+
- name: Install Dependencies
367+
run: |
368+
brew bundle --file=cpp/Brewfile
369+
- name: Setup ccache
370+
run: |
371+
ci/scripts/ccache_setup.sh
372+
- name: ccache info
373+
id: ccache-info
374+
run: |
375+
echo "cache-dir=$(ccache --get-config cache_dir)" >> $GITHUB_OUTPUT
376+
- name: Cache ccache
377+
uses: actions/cache@v5.0.2
378+
with:
379+
path: ${{ steps.ccache-info.outputs.cache-dir }}
380+
key: cpp-odbc-ccache-macos-${{ matrix.macos-version }}-${{ hashFiles('cpp/**') }}
381+
restore-keys: cpp-odbc-ccache-macos-${{ matrix.macos-version }}-
382+
- name: Build
383+
run: |
384+
# Homebrew uses /usr/local as prefix. So packages
385+
# installed by Homebrew also use /usr/local/include. We
386+
# want to include headers for packages installed by
387+
# Homebrew as system headers to ignore warnings in them.
388+
# But "-isystem /usr/local/include" isn't used by CMake
389+
# because /usr/local/include is marked as the default
390+
# include path. So we disable -Werror to avoid build error
391+
# by warnings from packages installed by Homebrew.
392+
export BUILD_WARNING_LEVEL=PRODUCTION
393+
LIBIODBC_DIR="$(brew --cellar libiodbc)/$(brew list --versions libiodbc | awk '{print $2}')"
394+
ODBC_INCLUDE_DIR=$LIBIODBC_DIR/include
395+
export ARROW_CMAKE_ARGS="-DODBC_INCLUDE_DIR=$ODBC_INCLUDE_DIR"
396+
export CXXFLAGS="$CXXFLAGS -I$ODBC_INCLUDE_DIR"
397+
ci/scripts/cpp_build.sh $(pwd) $(pwd)/build
398+
- name: Test
399+
shell: bash
400+
run: |
401+
sudo sysctl -w kern.coredump=1
402+
sudo sysctl -w kern.corefile=/tmp/core.%N.%P
403+
ulimit -c unlimited # must enable within the same shell
404+
ci/scripts/cpp_test.sh $(pwd) $(pwd)/build
405+
406+
odbc-msvc:
407+
needs: check-labels
408+
name: ODBC Windows
342409
runs-on: windows-2022
343410
if: >-
344411
needs.check-labels.outputs.force == 'true' ||
@@ -519,6 +586,7 @@ jobs:
519586
- jni-linux
520587
- jni-macos
521588
- msvc-arm64
522-
- odbc
589+
- odbc-macos
590+
- odbc-msvc
523591
uses: ./.github/workflows/report_ci.yml
524592
secrets: inherit

ci/scripts/cpp_test.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ case "$(uname)" in
5959
;;
6060
Darwin)
6161
n_jobs=$(sysctl -n hw.ncpu)
62+
exclude_tests+=("arrow-flight-sql-odbc-test")
6263
# TODO: https://github.com/apache/arrow/issues/40410
6364
exclude_tests+=("arrow-s3fs-test")
6465
;;

cpp/Brewfile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ brew "git"
2828
brew "glog"
2929
brew "googletest"
3030
brew "grpc"
31+
brew "libiodbc"
3132
brew "llvm"
3233
brew "lz4"
3334
brew "mimalloc"

cpp/CMakePresets.json

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,17 @@
315315
"displayName": "Debug build with tests and Flight SQL",
316316
"cacheVariables": {}
317317
},
318+
{
319+
"name": "ninja-debug-flight-sql-odbc",
320+
"inherits": [
321+
"features-flight-sql",
322+
"base-debug"
323+
],
324+
"displayName": "Debug build with tests and Flight SQL ODBC",
325+
"cacheVariables": {
326+
"ARROW_FLIGHT_SQL_ODBC": "ON"
327+
}
328+
},
318329
{
319330
"name": "ninja-debug-gandiva",
320331
"inherits": [
@@ -511,6 +522,17 @@
511522
"displayName": "Release build with Flight SQL",
512523
"cacheVariables": {}
513524
},
525+
{
526+
"name": "ninja-release-flight-sql-odbc",
527+
"inherits": [
528+
"features-flight-sql",
529+
"base-release"
530+
],
531+
"displayName": "Release build with Flight SQL ODBC",
532+
"cacheVariables": {
533+
"ARROW_FLIGHT_SQL_ODBC": "ON"
534+
}
535+
},
514536
{
515537
"name": "ninja-release-gandiva",
516538
"inherits": [

cpp/cmake_modules/DefineOptions.cmake

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,8 @@ macro(tsort_bool_option_dependencies)
107107
endmacro()
108108

109109
macro(resolve_option_dependencies)
110-
# Arrow Flight SQL ODBC is available only for Windows for now.
111-
if(NOT WIN32)
110+
# Arrow Flight SQL ODBC is available only for Windows and macOS for now.
111+
if(NOT WIN32 AND NOT APPLE)
112112
set(ARROW_FLIGHT_SQL_ODBC OFF)
113113
endif()
114114
if(MSVC_TOOLCHAIN)

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -855,7 +855,7 @@ SQLRETURN SQLDriverConnect(SQLHDBC conn, SQLHWND window_handle,
855855
}
856856
#else
857857
// Attempt connection without loading DSN window on macOS/Linux
858-
connection->Connect(dsn, properties, missing_properties);
858+
connection->Connect(dsn_value, properties, missing_properties);
859859
#endif
860860
// Copy connection string to out_connection_string after connection attempt
861861
return ODBC::GetStringAttribute(true, connection_string, false, out_connection_string,

cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,10 @@ add_library(arrow_odbc_spi_impl
4545
config/connection_string_parser.h
4646
diagnostics.cc
4747
diagnostics.h
48-
error_codes.h
4948
encoding.cc
5049
encoding.h
5150
encoding_utils.h
51+
error_codes.h
5252
exceptions.cc
5353
exceptions.h
5454
flight_sql_auth_method.cc
@@ -130,9 +130,18 @@ if(WIN32)
130130
system_dsn.h)
131131
endif()
132132

133-
target_link_libraries(arrow_odbc_spi_impl
134-
PUBLIC arrow_flight_sql_shared arrow_compute_shared Boost::locale
135-
${ODBCINST})
133+
if(APPLE)
134+
target_include_directories(arrow_odbc_spi_impl SYSTEM BEFORE PUBLIC ${ODBC_INCLUDE_DIR})
135+
target_link_libraries(arrow_odbc_spi_impl
136+
PUBLIC arrow_flight_sql_shared arrow_compute_shared Boost::locale
137+
iodbc)
138+
else()
139+
find_package(ODBC REQUIRED)
140+
target_include_directories(arrow_odbc_spi_impl PUBLIC ${ODBC_INCLUDE_DIR})
141+
target_link_libraries(arrow_odbc_spi_impl
142+
PUBLIC arrow_flight_sql_shared arrow_compute_shared Boost::locale
143+
${ODBCINST})
144+
endif()
136145

137146
set_target_properties(arrow_odbc_spi_impl
138147
PROPERTIES ARCHIVE_OUTPUT_DIRECTORY

cpp/src/arrow/flight/sql/odbc/odbc_impl/address_info.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717

1818
#include "arrow/flight/sql/odbc/odbc_impl/address_info.h"
19+
#include <cstdint>
1920

2021
namespace driver {
2122

@@ -34,7 +35,7 @@ bool AddressInfo::GetAddressInfo(const std::string& host, char* host_name_info,
3435
}
3536

3637
error = getnameinfo(addrinfo_result_->ai_addr, addrinfo_result_->ai_addrlen,
37-
host_name_info, static_cast<DWORD>(max_host), NULL, 0, 0);
38+
host_name_info, static_cast<uint32_t>(max_host), NULL, 0, 0);
3839
return error == 0;
3940
}
4041

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,10 @@
2222
#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
2323
#include "arrow/flight/sql/odbc/odbc_impl/spi/connection.h"
2424

25+
#if defined _WIN32
2526
// winuser.h needs to be included after windows.h, which is defined in platform.h
26-
#include <winuser.h>
27+
# include <winuser.h>
28+
#endif
2729

2830
namespace arrow::flight::sql::odbc {
2931
namespace config {

cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_handle.h

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,18 @@ class ODBCHandle {
4646
try {
4747
GetDiagnostics().Clear();
4848
rc = function();
49-
} catch (const arrow::flight::sql::odbc::DriverException& ex) {
49+
} catch (const arrow::flight::sql::odbc::AuthenticationException& ex) {
50+
GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException(
51+
ex.GetMessageText(), ex.GetSqlState(), ex.GetNativeError()));
52+
} catch (const arrow::flight::sql::odbc::NullWithoutIndicatorException& ex) {
53+
GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException(
54+
ex.GetMessageText(), ex.GetSqlState(), ex.GetNativeError()));
55+
}
56+
// on mac, DriverException doesn't catch the subclass exceptions hence we added
57+
// the following above.
58+
// GH-48278 TODO investigate if there is a way to catch all the subclass exceptions
59+
// under DriverException
60+
catch (const arrow::flight::sql::odbc::DriverException& ex) {
5061
GetDiagnostics().AddError(ex);
5162
} catch (const std::bad_alloc&) {
5263
GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException(

0 commit comments

Comments
 (0)