Skip to content

GH-49268: [C++][FlightRPC] Fix ODBC tests for MacOS #49267

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.

Sign up for GitHub

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

Merged
kou merged 4 commits into from
Mar 13, 2026
+1,906 −1,751
Merged

GH-49268: [C++][FlightRPC] Fix ODBC tests for MacOS #49267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e091f57
Fix Mac Tests
alinaliBQ Mar 6, 2026
795a1d0
Resolve Unix-odbc vs. IODBC linking issue
alinaliBQ Mar 6, 2026
5c6228c
Change `GetInfo` disabled tests to start new connection
alinaliBQ Mar 10, 2026
dfe4dc9
Address comments
justing-bq Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/cpp_extra.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,9 @@ jobs:
sudo sysctl -w kern.coredump=1
sudo sysctl -w kern.corefile=/tmp/core.%N.%P
ulimit -c unlimited # must enable within the same shell
# Give CI write access for system DSN
sudo mkdir -p /Library/ODBC
sudo chown -R $USER /Library/ODBC
ci/scripts/cpp_test.sh $(pwd) $(pwd)/build

odbc-msvc:
Expand Down
1 change: 0 additions & 1 deletion ci/scripts/cpp_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ case "$(uname)" in
;;
Darwin)
n_jobs=$(sysctl -n hw.ncpu)
exclude_tests+=("arrow-flight-sql-odbc-test")
# TODO: https://github.com/apache/arrow/issues/40410
exclude_tests+=("arrow-s3fs-test")
;;
Expand Down
54 changes: 32 additions & 22 deletions cpp/src/arrow/flight/sql/example/sqlite_tables_schema_batch_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,33 +65,43 @@ Status SqliteTablesWithSchemaBatchReader::ReadNext(std::shared_ptr<RecordBatch>*

auto* string_array = reinterpret_cast<StringArray*>(table_name_array.get());

std::vector<std::shared_ptr<Field>> column_fields;
std::map<std::string, std::vector<std::shared_ptr<Field>>> table_columns_map;
for (int i = 0; i < table_name_array->length(); i++) {
const std::string& table_name = string_array->GetString(i);
table_columns_map[table_name];
}

while (sqlite3_step(schema_statement->GetSqlite3Stmt()) == SQLITE_ROW) {
std::string sqlite_table_name = std::string(reinterpret_cast<const char*>(
sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 0)));
if (sqlite_table_name == table_name) {
const char* column_name = reinterpret_cast<const char*>(
sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 1));
const char* column_type = reinterpret_cast<const char*>(
sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 2));
int nullable = sqlite3_column_int(schema_statement->GetSqlite3Stmt(), 3);

const ColumnMetadata& column_metadata = GetColumnMetadata(
GetSqlTypeFromTypeName(column_type), sqlite_table_name.c_str());
std::shared_ptr<DataType> arrow_type;
auto status = GetArrowType(column_type).Value(&arrow_type);
if (!status.ok()) {
return Status::NotImplemented("Unknown SQLite type '", column_type,
"' for column '", column_name, "' in table '",
table_name, "': ", status);
}
column_fields.push_back(arrow::field(column_name, arrow_type, nullable == 0,
column_metadata.metadata_map()));
while (sqlite3_step(schema_statement->GetSqlite3Stmt()) == SQLITE_ROW) {
std::string table_name = std::string(reinterpret_cast<const char*>(
sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 0)));

if (table_columns_map.contains(table_name)) {
const char* column_name = reinterpret_cast<const char*>(
sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 1));
const char* column_type = reinterpret_cast<const char*>(
sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 2));
int nullable = sqlite3_column_int(schema_statement->GetSqlite3Stmt(), 3);

const ColumnMetadata& column_metadata =
GetColumnMetadata(GetSqlTypeFromTypeName(column_type), table_name.c_str());

std::shared_ptr<DataType> arrow_type;
auto status = GetArrowType(column_type).Value(&arrow_type);
if (!status.ok()) {
return Status::NotImplemented("Unknown SQLite type '", column_type,
"' for column '", column_name, "' in table '",
table_name, "': ", status);
}
table_columns_map[table_name].push_back(arrow::field(
column_name, arrow_type, nullable == 0, column_metadata.metadata_map()));
}
}

std::vector<std::shared_ptr<Field>> column_fields;
for (int i = 0; i < table_name_array->length(); i++) {
const std::string& table_name = string_array->GetString(i);
column_fields = table_columns_map[table_name];

ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Buffer> schema_buffer,
ipc::SerializeSchema(*arrow::schema(column_fields)));

Expand Down
6 changes: 3 additions & 3 deletions cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ add_library(arrow_odbc_spi_impl STATIC
spi/result_set.h
spi/result_set_metadata.h
spi/statement.h
system_dsn.cc
system_dsn.h
system_trust_store.cc
system_trust_store.h
types.h
Expand All @@ -125,9 +127,7 @@ if(WIN32)
ui/dsn_configuration_window.h
ui/window.cc
ui/window.h
win_system_dsn.cc
system_dsn.cc
system_dsn.h)
win_system_dsn.cc)
endif()

if(APPLE)
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ void ODBCStatement::SetStmtAttr(SQLINTEGER statement_attribute, SQLPOINTER value
switch (statement_attribute) {
case SQL_ATTR_APP_PARAM_DESC: {
ODBCDescriptor* desc = static_cast<ODBCDescriptor*>(value);
if (current_apd_ != desc) {
if (desc && current_apd_ != desc) {
if (current_apd_ != built_in_apd_.get()) {
current_apd_->DetachFromStatement(this, true);
}
Expand All @@ -567,7 +567,7 @@ void ODBCStatement::SetStmtAttr(SQLINTEGER statement_attribute, SQLPOINTER value
}
case SQL_ATTR_APP_ROW_DESC: {
ODBCDescriptor* desc = static_cast<ODBCDescriptor*>(value);
if (current_ard_ != desc) {
if (desc && current_ard_ != desc) {
if (current_ard_ != built_in_ard_.get()) {
current_ard_->DetachFromStatement(this, false);
}
Expand Down
15 changes: 6 additions & 9 deletions cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,20 @@

#include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h"

#include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h"
#include "arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h"
#include "arrow/flight/sql/odbc/odbc_impl/ui/dsn_configuration_window.h"
#include "arrow/flight/sql/odbc/odbc_impl/ui/window.h"
#include "arrow/flight/sql/odbc/odbc_impl/util.h"
#include "arrow/result.h"
#include "arrow/util/utf8.h"

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

system_dsn.cc uses boost::iequals in RegisterDsn(...) but the Boost predicate header is no longer included after this change, which will break compilation. Add the appropriate Boost include (e.g., the predicate header) in this file or in a header that guarantees it for all build targets.

Suggested change
#include <boost/algorithm/string/predicate.hpp>

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@justing-bq justing-bq Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#include <odbcinst.h>
#include <boost/algorithm/string/predicate.hpp>
#include <sstream>

namespace arrow::flight::sql::odbc {

using config::Configuration;

void PostError(DWORD error_code, LPCWSTR error_msg) {
void PostError(DWORD error_code, LPWSTR error_msg) {
#if defined _WIN32
MessageBox(NULL, error_msg, L"Error!", MB_ICONEXCLAMATION | MB_OK);
#endif // _WIN32
SQLPostInstallerError(error_code, error_msg);
}

Expand All @@ -42,7 +39,7 @@ void PostArrowUtilError(arrow::Status error_status) {
std::wstring werror_msg = arrow::util::UTF8ToWideString(error_msg).ValueOr(
L"Error during utf8 to wide string conversion");

PostError(ODBC_ERROR_GENERAL_ERR, werror_msg.c_str());
PostError(ODBC_ERROR_GENERAL_ERR, const_cast<LPWSTR>(werror_msg.c_str()));
}

void PostLastInstallerError() {
Expand All @@ -55,7 +52,7 @@ void PostLastInstallerError() {
buf << L"Message: \"" << msg << L"\", Code: " << code;
std::wstring error_msg = buf.str();

PostError(code, error_msg.c_str());
PostError(code, const_cast<LPWSTR>(error_msg.c_str()));
}
Comment on lines 30 to 56
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostError/callers cast away constness from std::wstring::c_str() to pass an LPWSTR. If the installer API ever writes into this buffer, that becomes undefined behavior. Prefer keeping the parameter const (e.g., LPCWSTR) when possible, or pass a truly mutable, null-terminated buffer (e.g., std::vector<wchar_t> / std::wstring with data() in C++17+ and ensuring null termination) instead of casting.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@justing-bq justing-bq Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now using const_cast.


/**
Expand Down
5 changes: 4 additions & 1 deletion cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@
#include "arrow/flight/sql/odbc/odbc_impl/platform.h"

#include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h"
#include "arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h"
#include "arrow/status.h"

#include <odbcinst.h>

namespace arrow::flight::sql::odbc {

#if defined _WIN32
Expand Down Expand Up @@ -64,7 +67,7 @@ bool RegisterDsn(const config::Configuration& config, LPCWSTR driver);
*/
bool UnregisterDsn(const std::wstring& dsn);

void PostError(DWORD error_code, LPCWSTR error_msg);
void PostError(DWORD error_code, LPWSTR error_msg);

void PostArrowUtilError(arrow::Status error_status);
} // namespace arrow::flight::sql::odbc
2 changes: 1 addition & 1 deletion cpp/src/arrow/flight/sql/odbc/odbc_impl/win_system_dsn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ BOOL INSTAPI ConfigDSNW(HWND hwnd_parent, WORD req, LPCWSTR wdriver,
std::wstring werror_msg =
arrow::util::UTF8ToWideString(error_msg).ValueOr(L"Error during DSN load");

PostError(err.GetNativeError(), werror_msg.c_str());
PostError(err.GetNativeError(), const_cast<LPWSTR>(werror_msg.c_str()));
return FALSE;
}

Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ else()
${ARROW_TEST_SHARED_LINK_LIBS})
endif()

set(ARROW_FLIGHT_SQL_ODBC_TEST_LIBS ${ODBC_LIBRARIES} ${ODBCINST} ${SQLite3_LIBRARIES})
# On macOS, link `ODBCINST` first to ensure iodbc take precedence over unixodbc
set(ARROW_FLIGHT_SQL_ODBC_TEST_LIBS ${ODBCINST} ${ODBC_LIBRARIES} ${SQLite3_LIBRARIES})

# On Windows, dynamic linking ODBC is supported, tests link libraries dynamically.
# On unix systems, static linking ODBC is supported, thus tests link libraries statically.
Expand Down
Loading
Loading