GH-49552: [C++][FlightRPC][ODBC] Enable ODBC test build on Linux#49668
GH-49552: [C++][FlightRPC][ODBC] Enable ODBC test build on Linux#49668justing-bq wants to merge 2 commits intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
|
|
71ade9f to
ccf583a
Compare
There was a problem hiding this comment.
Pull request overview
Enables building the Flight SQL ODBC C++ test suite on Linux by removing the platform gate in CMake and updating tests/helpers to better handle platform differences in SQLWCHAR and wide-string handling.
Changes:
- Always add the ODBC tests subdirectory when
ARROW_BUILD_TESTSis enabled (including Linux). - Refactor many tests to avoid relying on
L""/wcslenassumptions and to use helper macros/utilities forSQLWCHAR. - Adjust CI to exclude running the ODBC test binary on Linux while still building it.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/arrow/flight/sql/odbc/tests/type_info_test.cc | Switches expected column name handling and replaces NULL sentinel usage with numeric values. |
| cpp/src/arrow/flight/sql/odbc/tests/tables_test.cc | Updates SQLWCHAR parameter initialization and column-name comparisons for portability. |
| cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc | Introduces ASSIGN_SQLWCHAR_ARR_AND_LEN usage and adjusts string comparisons/conversions. |
| cpp/src/arrow/flight/sql/odbc/tests/statement_attr_test.cc | Uses the new SQLWCHAR assignment helper macro for query strings. |
| cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h | Adds Linux-specific SQLWCHAR assignment macros and declares new conversion helpers. |
| cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc | Implements new SQLWCHAR length/conversion helpers and tweaks DSN registration call. |
| cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc | Avoids constructing std::wstring directly from SQLWCHAR* by converting to UTF-8 first. |
| cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc | Makes output buffers consistently zero-initialized. |
| cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc | Renames helper and converts many wide-string assertions to use ConvertToWString. |
| cpp/src/arrow/flight/sql/odbc/tests/columns_test.cc | Updates SQLWCHAR parameter handling and metadata assertions for Linux builds. |
| cpp/src/arrow/flight/sql/odbc/CMakeLists.txt | Removes Windows/macOS-only gating so tests are built on Linux too. |
| ci/scripts/cpp_test.sh | Excludes arrow-flight-sql-odbc-test from Linux test execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string driver = config.Get(FlightSqlConnection::DRIVER); | ||
| std::wstring w_driver = arrow::util::UTF8ToWideString(driver).ValueOr(L""); | ||
| return RegisterDsn(config, w_driver.c_str()); | ||
| return RegisterDsn(config, reinterpret_cast<LPCWSTR>(w_driver.c_str())); | ||
| } |
There was a problem hiding this comment.
WriteDSN converts the driver name to std::wstring and then reinterpret_casts w_driver.c_str() to LPCWSTR. On Linux, the driver manager expects UTF-16 SQLWCHAR (see system_dsn.cc ConvertToSQLWCHAR), so casting wchar_t* (UTF-32) to LPCWSTR is likely incorrect and can mis-encode the driver name or cause misaligned reads. Convert the UTF-8 driver string to the platform’s expected SQLWCHAR representation before calling RegisterDsn (e.g., UTF8StringToUTF16 on Linux, UTF8ToWideString elsewhere), and pass the resulting buffer pointer without reinterpret_casting between incompatible character types.
Rationale for this change
The test suite needs to be updated so it will build on Linux in addition to Windows & Mac.
Resolves #49552
What changes are included in this PR?
Miscellaneous changes to get the tests building on Linux.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.