GH-50756: [C++][FlightSQL][ODBC] Fix Clang 20 compilation on macOS 26 - #50757
Merged
Conversation
HuaHuaY
changed the title
|
|
lidavidm
requested changes
Aug 3, 2026
lidavidm
left a comment
lidavidm
left a comment
Member
There was a problem hiding this comment.
I think we should resolve the discussion on the original issue before proceeding here.
github-actions
Bot
added
awaiting changes
HuaHuaY
force-pushed
the
fix_macos_flight_compile
branch
from
August 3, 2026 03:11
f7986d3 to
a4aa4cb
Compare
github-actions
Bot
added
awaiting change review
lidavidm
reviewed
Aug 3, 2026
lidavidm
left a comment
lidavidm
left a comment
Member
There was a problem hiding this comment.
Seems reasonable. CC @alinaliBQ
github-actions
Bot
added
awaiting changes
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Clang 20 (macOS 26) compilation failures in the Flight SQL ODBC driver by addressing deprecated Unicode conversion APIs, narrowing/precision warnings treated as errors, and unused-field warnings.
Changes:
- Replace deprecated
std::wstring_convert/std::codecvt_utf8usage with vendored utfcpp conversions in ODBC encoding helpers. - Resolve narrowing/precision warnings by adding explicit casts for ODBC/Arrow indices and RapidJSON length parameters.
- Remove an unused
FlightSqlConnectionprivate field and fix zero-initialization oftm.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc | Cast result-set column indices to int for BindColumn calls. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc | Fix narrowing warnings for metadata indexing and numeric radix handling; clarify TODOs. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc | Cast column index to int for GetData calls in test/demo code. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/json_converter.cc | Cast lengths to rapidjson::SizeType to avoid implicit narrowing warnings. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_result_set.cc | Cast column index to int for Arrow RecordBatch::column(int) access. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h | Remove unused odbc_version_ member. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc | Remove now-deleted odbc_version_ initializer. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding.h | Replace deprecated codecvt-based conversions with utfcpp-based conversions. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc | Avoid implicit narrowing in byte-length calculations for SQLWCHAR conversion. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/accessors/timestamp_array_accessor.cc | Fix tm initialization to avoid missing-field initializer warnings. |
| cpp/src/arrow/flight/sql/odbc/odbc_api.cc | Remove unused variable; adjust diagnostic record index type. |
Comment on lines
643
to
646
| // Convert from ODBC 1 based record number to internal diagnostics 0 indexed storage | ||
| const size_t record_index = static_cast<size_t>(rec_number - 1); | ||
| const auto record_index = static_cast<uint32_t>(rec_number - 1); | ||
| if (!diagnostics->HasRecord(record_index)) { | ||
| return SQL_NO_DATA; |
Contributor
Author
There was a problem hiding this comment.
- The function whose actual argument is
record_indexhas a parameter type ofuint32_t, so there is no semantic difference to the original code. - This function has already checked
rec_number < 1on line 602, so there is no need to check it again here.
alinaliBQ
reviewed
Aug 4, 2026
alinaliBQ
left a comment
alinaliBQ
left a comment
Collaborator
There was a problem hiding this comment.
Could you trigger the CI for C++ Extra / ODBC debug AMD64 macOS 15-intel (pull_request) again to see if the failure can be reproduced?
It might be an existing flakey failure; just to confirm.
Member
|
I kicked CI again. |
HuaHuaY
force-pushed
the
fix_macos_flight_compile
branch
from
August 5, 2026 03:26
a4aa4cb to
200d825
Compare
github-actions
Bot
added
awaiting change review
Contributor
Author
|
The CI run that was previously re-triggered has passed. I rebased and pushed a new commit to address the comments above. |
lidavidm
approved these changes
Aug 5, 2026
lidavidm
left a comment
lidavidm
left a comment
Member
There was a problem hiding this comment.
Thanks. It seems the failures are unrelated to this PR.
github-actions
Bot
added
awaiting merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Fix Clang 20 compilation on macOS 26.
What changes are included in this PR?
Fix four kinds of compile errors mentioned in the issue GH-50756.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.