Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/cpp_extra.yml
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,6 @@ jobs:
- 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
- name: Setup ccache
run: |
ci/scripts/ccache_setup.sh
Expand All @@ -392,6 +390,8 @@ jobs:
# include path. So we disable -Werror to avoid build error
# by warnings from packages installed by Homebrew.
export BUILD_WARNING_LEVEL=PRODUCTION
LIBIODBC_DIR="$(brew --cellar libiodbc)/$(brew list --versions libiodbc | awk '{print $2}')"
export ODBC_INCLUDE_DIR=$LIBIODBC_DIR/include
export CXXFLAGS="$CXXFLAGS -I$ODBC_INCLUDE_DIR"
ci/scripts/cpp_build.sh $(pwd) $(pwd)/build
- name: Test
Expand Down
2 changes: 1 addition & 1 deletion ci/scripts/cpp_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ 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")
exclude_tests+=("arrow-flight-sql-odbc-test")
;;
MINGW*)
n_jobs=${NUMBER_OF_PROCESSORS:-1}
Expand Down
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*>(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just curious, is this a bug that only occurs on mac and not Windows?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The logic for iterating through all the columns happens inside the while (sqlite3_step(schema_statement->GetSqlite3Stmt()) == SQLITE_ROW) loop. On Windows we can go through this loop multiple times which is why the loop was nested inside another loop iterating through the tables. But on MacOS, this loop will only run all the way through once. So I rewrote this logic to only go through the loop once using a map to sort out which columns belong to which table. This new logic works for both Windows and MacOS.

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
25 changes: 23 additions & 2 deletions cpp/src/arrow/flight/sql/odbc/tests/columns_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@

#include <gtest/gtest.h>

// Many tests are disabled for MacOS due to iODBC limitations with ODBC 2.0 APIs and
// identifiers such as SQLColAttributes, SQL_COLUMN_AUTO_INCREMENT,
// SQL_COLUMN_QUALIFIER_NAME.

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

template <typename T>
Expand Down Expand Up @@ -364,6 +368,7 @@ void GetSQLColAttributeNumeric(SQLHSTMT stmt, const std::wstring& wsql, SQLUSMAL
SQLColAttribute(stmt, idx, field_identifier, 0, 0, nullptr, value));
}

#ifndef __APPLE__
Comment thread
alinaliBQ marked this conversation as resolved.
void GetSQLColAttributesNumeric(SQLHSTMT stmt, const std::wstring& wsql, SQLUSMALLINT idx,
SQLUSMALLINT field_identifier, SQLLEN* value) {
// Execute query and check SQLColAttribute numeric attribute
Expand All @@ -377,7 +382,7 @@ void GetSQLColAttributesNumeric(SQLHSTMT stmt, const std::wstring& wsql, SQLUSMA
ASSERT_EQ(SQL_SUCCESS,
SQLColAttributes(stmt, idx, field_identifier, 0, 0, nullptr, value));
}

#endif // __APPLE__
} // namespace

TYPED_TEST(ColumnsTest, SQLColumnsTestInputData) {
Expand Down Expand Up @@ -1387,7 +1392,8 @@ TEST_F(ColumnsMockTest, TestSQLColAttributeAllTypes) {
SQL_FALSE); // expected_unsigned_column
}

TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesAllTypesODBCVer2) {
#ifndef __APPLE__
TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesAllTypes) {
// Tests ODBC 2.0 API SQLColAttributes
this->CreateTableAllDataType();

Expand Down Expand Up @@ -1446,6 +1452,7 @@ TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesAllTypesODBCVer2) {
SQL_PRED_NONE, // expected_searchable
SQL_FALSE); // expected_unsigned_column
}
#endif // __APPLE__

TEST_F(ColumnsRemoteTest, TestSQLColAttributeAllTypes) {
// Test assumes there is a table $scratch.ODBCTest in remote server
Expand Down Expand Up @@ -1910,6 +1917,7 @@ TYPED_TEST(ColumnsTest, TestSQLColAttributeCaseSensitive) {
ASSERT_EQ(SQL_FALSE, value);
}

#ifndef __APPLE__
TYPED_TEST(ColumnsOdbcV2Test, TestSQLColAttributesCaseSensitive) {
// Arrow limitation: returns SQL_FALSE for case sensitive column
// Tests ODBC 2.0 API SQLColAttributes
Expand All @@ -1924,6 +1932,7 @@ TYPED_TEST(ColumnsOdbcV2Test, TestSQLColAttributesCaseSensitive) {
GetSQLColAttributesNumeric(this->stmt, wsql, 28, SQL_COLUMN_CASE_SENSITIVE, &value);
ASSERT_EQ(SQL_FALSE, value);
}
#endif // __APPLE__

TEST_F(ColumnsMockTest, TestSQLColAttributeUniqueValue) {
// Mock server limitation: returns false for auto-increment column
Expand All @@ -1935,6 +1944,7 @@ TEST_F(ColumnsMockTest, TestSQLColAttributeUniqueValue) {
ASSERT_EQ(SQL_FALSE, value);
}

#ifndef __APPLE__
TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesAutoIncrement) {
// Tests ODBC 2.0 API SQLColAttributes
// Mock server limitation: returns false for auto-increment column
Expand All @@ -1945,6 +1955,7 @@ TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesAutoIncrement) {
GetSQLColAttributeNumeric(this->stmt, wsql, 1, SQL_COLUMN_AUTO_INCREMENT, &value);
ASSERT_EQ(SQL_FALSE, value);
}
#endif // __APPLE__

TEST_F(ColumnsMockTest, TestSQLColAttributeBaseTableName) {
this->CreateTableAllDataType();
Expand All @@ -1955,6 +1966,7 @@ TEST_F(ColumnsMockTest, TestSQLColAttributeBaseTableName) {
ASSERT_EQ(std::wstring(L"AllTypesTable"), value);
}

#ifndef __APPLE__
TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesTableName) {
// Tests ODBC 2.0 API SQLColAttributes
this->CreateTableAllDataType();
Expand All @@ -1964,6 +1976,7 @@ TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesTableName) {
GetSQLColAttributesString(this->stmt, wsql, 1, SQL_COLUMN_TABLE_NAME, value);
ASSERT_EQ(std::wstring(L"AllTypesTable"), value);
}
#endif // __APPLE__

TEST_F(ColumnsMockTest, TestSQLColAttributeCatalogName) {
// Mock server limitattion: mock doesn't return catalog for result metadata,
Expand All @@ -1985,6 +1998,7 @@ TEST_F(ColumnsRemoteTest, TestSQLColAttributeCatalogName) {
ASSERT_EQ(std::wstring(L""), value);
}

#ifndef __APPLE__
TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesQualifierName) {
// Mock server limitattion: mock doesn't return catalog for result metadata,
// and the defautl catalog should be 'main'
Expand All @@ -2005,6 +2019,7 @@ TEST_F(ColumnsOdbcV2RemoteTest, TestSQLColAttributesQualifierName) {
GetSQLColAttributeString(this->stmt, wsql, 1, SQL_COLUMN_QUALIFIER_NAME, value);
ASSERT_EQ(std::wstring(L""), value);
}
#endif // __APPLE__

TYPED_TEST(ColumnsTest, TestSQLColAttributeCount) {
std::wstring wsql = this->GetQueryAllDataTypes();
Expand Down Expand Up @@ -2050,6 +2065,7 @@ TEST_F(ColumnsRemoteTest, TestSQLColAttributeSchemaName) {
ASSERT_EQ(std::wstring(L""), value);
}

#ifndef __APPLE__
TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesOwnerName) {
// Tests ODBC 2.0 API SQLColAttributes
this->CreateTableAllDataType();
Expand All @@ -2071,6 +2087,7 @@ TEST_F(ColumnsOdbcV2RemoteTest, TestSQLColAttributesOwnerName) {
GetSQLColAttributesString(this->stmt, wsql, 1, SQL_COLUMN_OWNER_NAME, value);
ASSERT_EQ(std::wstring(L""), value);
}
#endif // __APPLE__

TEST_F(ColumnsMockTest, TestSQLColAttributeTableName) {
this->CreateTableAllDataType();
Expand Down Expand Up @@ -2119,6 +2136,7 @@ TEST_F(ColumnsRemoteTest, TestSQLColAttributeTypeName) {
ASSERT_EQ(std::wstring(L"TIMESTAMP"), value);
}

#ifndef __APPLE__
TEST_F(ColumnsOdbcV2MockTest, TestSQLColAttributesTypeName) {
// Tests ODBC 2.0 API SQLColAttributes
this->CreateTableAllDataType();
Expand Down Expand Up @@ -2159,6 +2177,7 @@ TEST_F(ColumnsOdbcV2RemoteTest, TestSQLColAttributesTypeName) {
GetSQLColAttributesString(this->stmt, L"", 9, SQL_COLUMN_TYPE_NAME, value);
ASSERT_EQ(std::wstring(L"TIMESTAMP"), value);
}
#endif // __APPLE__

TYPED_TEST(ColumnsTest, TestSQLColAttributeUnnamed) {
std::wstring wsql = this->GetQueryAllDataTypes();
Expand All @@ -2175,6 +2194,7 @@ TYPED_TEST(ColumnsTest, TestSQLColAttributeUpdatable) {
ASSERT_EQ(SQL_ATTR_READWRITE_UNKNOWN, value);
}

#ifndef __APPLE__
TYPED_TEST(ColumnsOdbcV2Test, TestSQLColAttributesUpdatable) {
// Tests ODBC 2.0 API SQLColAttributes
std::wstring wsql = this->GetQueryAllDataTypes();
Expand All @@ -2183,6 +2203,7 @@ TYPED_TEST(ColumnsOdbcV2Test, TestSQLColAttributesUpdatable) {
GetSQLColAttributesNumeric(this->stmt, wsql, 1, SQL_COLUMN_UPDATABLE, &value);
ASSERT_EQ(SQL_ATTR_READWRITE_UNKNOWN, value);
}
#endif // __APPLE__

TEST_F(ColumnsMockTest, SQLDescribeColValidateInput) {
this->CreateTestTables();
Expand Down
10 changes: 10 additions & 0 deletions cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ void GetInfo(SQLHDBC connection, SQLUSMALLINT info_type, SQLWCHAR* value,
}
} // namespace

// Test disabled until we resolve bus error on MacOS
#ifdef DISABLE_TEST
Comment on lines +70 to +71

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you raise an internal Jira for this? If we plan to export this change to the Arrow repo, we need to raise a GitHub issue for this as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoTruncation) {
static constexpr int info_len = 1;
SQLWCHAR value[info_len] = L"";
Expand All @@ -79,6 +81,7 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoTruncation) {
VerifyOdbcErrorState(SQL_HANDLE_DBC, this->conn, kErrorState01004);
EXPECT_GT(message_length, 0);
}
#endif

// Driver Information

Expand Down Expand Up @@ -319,7 +322,11 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoOdbcVer) {
SQLWCHAR value[kOdbcBufferSize] = L"";
GetInfo(this->conn, SQL_ODBC_VER, value);

#ifdef __APPLE__
EXPECT_STREQ(static_cast<const SQLWCHAR*>(L"03.52.0000"), value);
#else
EXPECT_STREQ(static_cast<const SQLWCHAR*>(L"03.80.0000"), value);
#endif // __APPLE__
}

TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoParamArrayRowCounts) {
Expand Down Expand Up @@ -785,6 +792,8 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoIntegrity) {
EXPECT_STREQ(static_cast<const SQLWCHAR*>(L"N"), value);
}

// Test disabled until we resolve bus error on MacOS
#ifdef DISABLE_TEST
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoKeywords) {
// Keyword strings can require 5000 buffer length
static constexpr int info_len = kOdbcBufferSize * 5;
Expand All @@ -793,6 +802,7 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoKeywords) {

EXPECT_GT(wcslen(value), 0);
}
#endif

TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoLikeEscapeClause) {
SQLWCHAR value[kOdbcBufferSize] = L"";
Expand Down
3 changes: 3 additions & 0 deletions cpp/src/arrow/flight/sql/odbc/tests/get_functions_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ using TestTypesOdbcV2 =
::testing::Types<FlightSQLOdbcV2MockTestBase, FlightSQLOdbcV2RemoteTestBase>;
TYPED_TEST_SUITE(GetFunctionsOdbcV2Test, TestTypesOdbcV2);

// MacOS driver manager iODBC does not support SQLGetFunctions for ODBC 3.x or 2.x driver
#ifndef __APPLE__
TYPED_TEST(GetFunctionsTest, TestSQLGetFunctionsAllFunctions) {
// Verify driver manager return values for SQLGetFunctions

Expand Down Expand Up @@ -216,5 +218,6 @@ TYPED_TEST(GetFunctionsOdbcV2Test, TestSQLGetFunctionsUnsupportedSingleAPI) {
api_exists = -1;
}
}
#endif // __APPLE__

} // namespace arrow::flight::sql::odbc
Loading
Loading