-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48575: [C++][FlightRPC] Standalone ODBC macOS CI #48577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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/[email protected] | ||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}" \ | ||
|
Comment on lines
261
to
263
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am open to suggestions for other locations for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use |
||
| -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} \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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") | ||
|
Comment on lines
63
to
+64
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you keep this list in alphabetical order? |
||
| ;; | ||
| MINGW*) | ||
| n_jobs=${NUMBER_OF_PROCESSORS:-1} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -314,6 +314,17 @@ | |
| "displayName": "Debug build with tests and Flight SQL", | ||
| "cacheVariables": {} | ||
| }, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ODBC can be built on Windows and macOS Intel/ARM but it is not available on Linux yet (we will add Linux support later), let me know if this change should be removed until Linux is also supported. |
||
| { | ||
| "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": [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to check both of
https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
|
||
| // winuser.h needs to be included after windows.h, which is defined in platform.h | ||
| #include <winuser.h> | ||
| # include <winuser.h> | ||
| #endif | ||
|
|
||
| namespace arrow::flight::sql::odbc { | ||
| namespace config { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Comment on lines
+49
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't happen...maybe clang is stricter than MSVC?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this is odd, it does seem like undefined behavior. clang being more strict is a possibility. |
||
| GetDiagnostics().AddError(ex); | ||
| } catch (const std::bad_alloc&) { | ||
| GetDiagnostics().AddError(arrow::flight::sql::odbc::DriverException( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
|
Comment on lines
+50
to
+56
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use find_package(ODBC REQUIRED)
target_link_libraries(arrow-flight-sql-odbc-test PRIVATE ODBC::ODBC)https://cmake.org/cmake/help/latest/module/FindODBC.html#imported-targets |
||
|
|
||
| # 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) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use
-DODBC_ROOTinstead?https://cmake.org/cmake/help/latest/module/FindODBC.html#example-finding-a-custom-odbc-installation