-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes from 7 commits
a032259
1bfc506
b96d107
7692975
cf0dce6
1987ac6
44d7abc
a443106
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -336,9 +336,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}')" | ||||||
|
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. We don't need
Contributor
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. No longer exporting LIBIODBC_DIR |
||||||
| echo ODBC_INCLUDE_DIR="$LIBIODBC_DIR/include" >> $GITHUB_ENV | ||||||
|
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. How about moving them to the
Contributor
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. Done. |
||||||
| - 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 | ||||||
|
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.
Suggested change
Contributor
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. Done. |
||||||
| 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 }}- | ||||||
|
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 use different cache keys with existing keys?
Contributor
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. Done. |
||||||
| - 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 | ||||||
| export CXXFLAGS="$CXXFLAGS -I$ODBC_INCLUDE_DIR" | ||||||
| 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' || | ||||||
|
|
@@ -465,6 +531,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 |
|---|---|---|
|
|
@@ -272,6 +272,7 @@ else | |
| -DgRPC_SOURCE=${gRPC_SOURCE:-} \ | ||
| -DGTest_SOURCE=${GTest_SOURCE:-} \ | ||
| -Dlz4_SOURCE=${lz4_SOURCE:-} \ | ||
| -DODBC_INCLUDE_DIR="${ODBC_INCLUDE_DIR:-}" \ | ||
|
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 revert this?
Contributor
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. Build breaks without this line.
Collaborator
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. @kou I think
Collaborator
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. @justing-bq Can you move this line above line 277
Contributor
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. Done. |
||
| -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 |
|---|---|---|
|
|
@@ -315,6 +315,17 @@ | |
| "displayName": "Debug build with tests and Flight SQL", | ||
| "cacheVariables": {} | ||
| }, | ||
|
Collaborator
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": [ | ||
|
|
@@ -511,6 +522,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 |
|---|---|---|
|
|
@@ -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?
Collaborator
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 |
|---|---|---|
|
|
@@ -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"; | ||
|
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 sort them?
Contributor
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. Done. |
||
|
|
||
| /// Verify ODBC Error State | ||
| void VerifyOdbcErrorState(SQLSMALLINT handle_type, SQLHANDLE handle, | ||
|
|
||
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.
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.
Done.