From 763e2a9f131b90f784d591ed41eba24fdb0f8e3d Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Fri, 4 Jul 2025 14:46:39 -0700 Subject: [PATCH 01/12] SQLBindCol Initial Impl --- cpp/src/arrow/flight/sql/odbc/entry_points.cc | 14 +++++--------- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 10 ++++++++++ cpp/src/arrow/flight/sql/odbc/odbc_api.h | 2 ++ .../arrow/flight/sql/odbc/tests/statement_test.cc | 5 +++++ 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/entry_points.cc b/cpp/src/arrow/flight/sql/odbc/entry_points.cc index e7c55d5a73a1..ec30405870f6 100644 --- a/cpp/src/arrow/flight/sql/odbc/entry_points.cc +++ b/cpp/src/arrow/flight/sql/odbc/entry_points.cc @@ -150,15 +150,11 @@ SQLRETURN SQL_API SQLPrepare(SQLHSTMT stmt, SQLWCHAR* queryText, SQLINTEGER text SQLRETURN SQL_API SQLExecute(SQLHSTMT stmt) { return arrow::SQLExecute(stmt); } -SQLRETURN SQL_API SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT columnNumber, - SQLSMALLINT targetType, SQLPOINTER targetValuePtr, - SQLLEN bufferLength, SQLLEN* strLen_or_IndPtr) { - LOG_DEBUG( - "SQLBindCol called with stmt: {}, columnNumber: {}, targetType: {}, " - "targetValuePtr: {}, bufferLength: {}, strLen_or_IndPtr: {}", - stmt, columnNumber, targetType, targetValuePtr, bufferLength, - fmt::ptr(strLen_or_IndPtr)); - return SQL_ERROR; +SQLRETURN SQL_API SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, + SQLPOINTER dataPtr, SQLLEN bufferLength, + SQLLEN* indicatorPtr) { + return arrow::SQLBindCol(stmt, recordNumber, cType, dataPtr, bufferLength, + indicatorPtr); } SQLRETURN SQL_API SQLCancel(SQLHSTMT stmt) { diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 91121726e8f1..de033a5bfbf3 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -976,6 +976,16 @@ SQLRETURN SQLFetch(SQLHSTMT stmt) { }); } +SQLRETURN SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, + SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr) { + LOG_DEBUG( + "SQLBindCol called with stmt: {}, recordNumber: {}, cType: {}, " + "dataPtr: {}, bufferLength: {}, strLen_or_IndPtr: {}", + stmt, recordNumber, cType, dataPtr, bufferLength, fmt::ptr(indicatorPtr)); + return SQL_ERROR; + //-AL - TODO: implement SQLBindCol functionality +} + SQLRETURN SQLGetData(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr) { // GH-46979: support SQL_C_GUID data type diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.h b/cpp/src/arrow/flight/sql/odbc/odbc_api.h index 504a8f545f88..91c45b887389 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.h @@ -68,6 +68,8 @@ SQLRETURN SQLExecDirect(SQLHSTMT stmt, SQLWCHAR* queryText, SQLINTEGER textLengt SQLRETURN SQLPrepare(SQLHSTMT stmt, SQLWCHAR* queryText, SQLINTEGER textLength); SQLRETURN SQLExecute(SQLHSTMT stmt); SQLRETURN SQLFetch(SQLHSTMT stmt); +SQLRETURN SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, + SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr); SQLRETURN SQLGetData(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr); SQLRETURN SQLMoreResults(SQLHSTMT stmt); diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index 3335d5f72469..74f131baa4e0 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1432,4 +1432,9 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExecDirectIgnoreInvalidBufLen) { this->disconnect(); } +//-AL- TODO: add tests that use SQLBindCol to fetch all data . +// TODO: add tests that replicate queries for 1) varibinary (mock server) +// +// and 2) time (remote server) + } // namespace arrow::flight::sql::odbc From e41ef228c4fe0752546063f54fb6923868522d45 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Fri, 4 Jul 2025 15:47:52 -0700 Subject: [PATCH 02/12] Draft SQLBindCol impl + tests --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 10 +- .../flight/sql/odbc/tests/statement_test.cc | 318 +++++++++++++++++- 2 files changed, 325 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index de033a5bfbf3..d748c0f99c3f 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -982,8 +982,14 @@ SQLRETURN SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType "SQLBindCol called with stmt: {}, recordNumber: {}, cType: {}, " "dataPtr: {}, bufferLength: {}, strLen_or_IndPtr: {}", stmt, recordNumber, cType, dataPtr, bufferLength, fmt::ptr(indicatorPtr)); - return SQL_ERROR; - //-AL - TODO: implement SQLBindCol functionality + using ODBC::ODBCDescriptor; + using ODBC::ODBCStatement; + return ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() { + ODBCStatement* statement = reinterpret_cast(stmt); + ODBCDescriptor* ard = statement->GetARD(); + ard->BindCol(recordNumber, cType, dataPtr, bufferLength, indicatorPtr); + return SQL_SUCCESS; + }); } SQLRETURN SQLGetData(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index 74f131baa4e0..6e35cb1925d2 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1432,9 +1432,325 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExecDirectIgnoreInvalidBufLen) { this->disconnect(); } -//-AL- TODO: add tests that use SQLBindCol to fetch all data . +//-AL- TODOs for tests // TODO: add tests that replicate queries for 1) varibinary (mock server) // // and 2) time (remote server) +// TODO: add tests for indicator pointer for null data etc +// TODO: add SQLBindCol tests for row fetching + +TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColSimpleQuery) { + // -AL- TODO can perhaps delete this test later after TestSQLBindColDataQuery is done, + // this is just proof of concept + this->connect(); + + // Numeric Types + + // Signed Tiny Int + int8_t stiny_int_val; + SQLLEN buf_len = sizeof(stiny_int_val); + SQLLEN ind; + + SQLRETURN ret = + SQLBindCol(this->stmt, 1, SQL_C_STINYINT, &stiny_int_val, buf_len, &ind); + + std::wstring wsql = L"SELECT 1;"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = + SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Check that 1 is fetched + EXPECT_EQ(stiny_int_val, 1); +} + +TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColDataQuery) { + this->connect(); + + // Numeric Types + + // Signed Tiny Int + int8_t stiny_int_val_min; + int8_t stiny_int_val_max; + SQLLEN buf_len = 0; + SQLLEN ind; + + SQLRETURN ret = + SQLBindCol(this->stmt, 1, SQL_C_STINYINT, &stiny_int_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 2, SQL_C_STINYINT, &stiny_int_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Unsigned Tiny Int + uint8_t utiny_int_val_min; + uint8_t utiny_int_val_max; + + ret = SQLBindCol(this->stmt, 3, SQL_C_UTINYINT, &utiny_int_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 4, SQL_C_UTINYINT, &utiny_int_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Signed Small Int + int16_t ssmall_int_val_min; + int16_t ssmall_int_val_max; + + ret = SQLBindCol(this->stmt, 5, SQL_C_SSHORT, &ssmall_int_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 6, SQL_C_SSHORT, &ssmall_int_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Unsigned Small Int + uint16_t usmall_int_val_min; + uint16_t usmall_int_val_max; + + ret = SQLBindCol(this->stmt, 7, SQL_C_USHORT, &usmall_int_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 8, SQL_C_USHORT, &usmall_int_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Signed Integer + SQLINTEGER slong_val_min; + SQLINTEGER slong_val_max; + + ret = SQLBindCol(this->stmt, 9, SQL_C_SLONG, &slong_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 10, SQL_C_SLONG, &slong_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Unsigned Integer + SQLUINTEGER ulong_val_min; + SQLUINTEGER ulong_val_max; + + ret = SQLBindCol(this->stmt, 11, SQL_C_ULONG, &ulong_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 12, SQL_C_ULONG, &ulong_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Signed Big Int + SQLBIGINT sbig_int_val_min; + SQLBIGINT sbig_int_val_max; + + ret = SQLBindCol(this->stmt, 13, SQL_C_SBIGINT, &sbig_int_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 14, SQL_C_SBIGINT, &sbig_int_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Unsigned Big Int + SQLUBIGINT ubig_int_val_min; + SQLUBIGINT ubig_int_val_max; + + ret = SQLBindCol(this->stmt, 15, SQL_C_UBIGINT, &ubig_int_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 16, SQL_C_UBIGINT, &ubig_int_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Decimal + SQL_NUMERIC_STRUCT decimal_val_neg; + SQL_NUMERIC_STRUCT decimal_val_pos; + memset(&decimal_val_neg, 0, sizeof(decimal_val_neg)); + memset(&decimal_val_pos, 0, sizeof(decimal_val_pos)); + + ret = SQLBindCol(this->stmt, 17, SQL_C_NUMERIC, &decimal_val_neg, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 18, SQL_C_NUMERIC, &decimal_val_pos, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Float + float float_val_min; + float float_val_max; + + ret = SQLBindCol(this->stmt, 19, SQL_C_FLOAT, &float_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 20, SQL_C_FLOAT, &float_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Double + SQLDOUBLE double_val_min; + SQLDOUBLE double_val_max; + + ret = SQLBindCol(this->stmt, 21, SQL_C_DOUBLE, &double_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 22, SQL_C_DOUBLE, &double_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Bit + bool bit_val_false; + bool bit_val_true; + + ret = SQLBindCol(this->stmt, 23, SQL_C_BIT, &bit_val_false, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 24, SQL_C_BIT, &bit_val_true, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Characters + SQLCHAR char_val[2]; + buf_len = sizeof(SQLCHAR) * 2; + + ret = SQLBindCol(this->stmt, 25, SQL_C_CHAR, &char_val, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + SQLWCHAR wchar_val[2]; + constexpr size_t wchar_size = driver::odbcabstraction::GetSqlWCharSize(); + buf_len = wchar_size * 2; + + ret = SQLBindCol(this->stmt, 26, SQL_C_WCHAR, &wchar_val, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + SQLWCHAR wvarchar_val[3]; + buf_len = wchar_size * 3; + + ret = SQLBindCol(this->stmt, 27, SQL_C_WCHAR, &wvarchar_val, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + SQLCHAR varchar_val[4]; + buf_len = sizeof(SQLCHAR) * 4; + + ret = SQLBindCol(this->stmt, 28, SQL_C_CHAR, &varchar_val, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Date and Timestamp + SQL_DATE_STRUCT date_val_min{}, date_val_max{}; + buf_len = 0; + + ret = SQLBindCol(this->stmt, 29, SQL_C_TYPE_DATE, &date_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 30, SQL_C_TYPE_DATE, &date_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + SQL_TIMESTAMP_STRUCT timestamp_val_min{}, timestamp_val_max{}; + + ret = + SQLBindCol(this->stmt, 31, SQL_C_TYPE_TIMESTAMP, ×tamp_val_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = + SQLBindCol(this->stmt, 32, SQL_C_TYPE_TIMESTAMP, ×tamp_val_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Execute query and fetch data once since there is only 1 row. + std::wstring wsql = this->getQueryAllDataTypes(); + std::vector sql0(wsql.begin(), wsql.end()); + + ret = + SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Data verification + + // Signed Tiny Int + EXPECT_EQ(stiny_int_val_min, std::numeric_limits::min()); + EXPECT_EQ(stiny_int_val_max, std::numeric_limits::max()); + + // Unsigned Tiny Int + EXPECT_EQ(utiny_int_val_min, std::numeric_limits::min()); + EXPECT_EQ(utiny_int_val_max, std::numeric_limits::max()); + + // Signed Small Int + EXPECT_EQ(ssmall_int_val_min, std::numeric_limits::min()); + EXPECT_EQ(ssmall_int_val_max, std::numeric_limits::max()); + + // Unsigned Small Int + EXPECT_EQ(usmall_int_val_min, std::numeric_limits::min()); + EXPECT_EQ(usmall_int_val_max, std::numeric_limits::max()); + + // Signed Long + EXPECT_EQ(slong_val_min, std::numeric_limits::min()); + EXPECT_EQ(slong_val_max, std::numeric_limits::max()); + + // Unsigned Long + EXPECT_EQ(ulong_val_min, std::numeric_limits::min()); + EXPECT_EQ(ulong_val_max, std::numeric_limits::max()); + + // Signed Big Int + EXPECT_EQ(sbig_int_val_min, std::numeric_limits::min()); + EXPECT_EQ(sbig_int_val_max, std::numeric_limits::max()); + + // Unsigned Big Int + EXPECT_EQ(ubig_int_val_min, std::numeric_limits::min()); + EXPECT_EQ(ubig_int_val_max, std::numeric_limits::max()); + + // Decimal + EXPECT_EQ(decimal_val_neg.sign, 0); + EXPECT_EQ(decimal_val_neg.scale, 0); + EXPECT_EQ(decimal_val_neg.precision, 38); + EXPECT_THAT(decimal_val_neg.val, ::testing::ElementsAre(0xFF, 0xC9, 0x9A, 0x3B, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0)); + + EXPECT_EQ(decimal_val_pos.sign, 1); + EXPECT_EQ(decimal_val_pos.scale, 0); + EXPECT_EQ(decimal_val_pos.precision, 38); + EXPECT_THAT(decimal_val_pos.val, ::testing::ElementsAre(0xFF, 0xC9, 0x9A, 0x3B, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0)); + + // Float + EXPECT_EQ(float_val_min, -std::numeric_limits::max()); + EXPECT_EQ(float_val_max, std::numeric_limits::max()); + + // Double + EXPECT_EQ(double_val_min, -std::numeric_limits::max()); + EXPECT_EQ(double_val_max, std::numeric_limits::max()); + + // Bit + EXPECT_EQ(bit_val_false, false); + EXPECT_EQ(bit_val_true, true); + + // Characters + EXPECT_EQ(char_val[0], 'Z'); + EXPECT_EQ(wchar_val[0], L'你'); + EXPECT_EQ(wvarchar_val[0], L'你'); + EXPECT_EQ(wvarchar_val[1], L'好'); + + EXPECT_EQ(varchar_val[0], 'X'); + EXPECT_EQ(varchar_val[1], 'Y'); + EXPECT_EQ(varchar_val[2], 'Z'); + + // Date + EXPECT_EQ(date_val_min.day, 1); + EXPECT_EQ(date_val_min.month, 1); + EXPECT_EQ(date_val_min.year, 1400); + + EXPECT_EQ(date_val_max.day, 31); + EXPECT_EQ(date_val_max.month, 12); + EXPECT_EQ(date_val_max.year, 9999); + + // Timestamp + EXPECT_EQ(timestamp_val_min.day, 1); + EXPECT_EQ(timestamp_val_min.month, 1); + EXPECT_EQ(timestamp_val_min.year, 1400); + EXPECT_EQ(timestamp_val_min.hour, 0); + EXPECT_EQ(timestamp_val_min.minute, 0); + EXPECT_EQ(timestamp_val_min.second, 0); + EXPECT_EQ(timestamp_val_min.fraction, 0); + + EXPECT_EQ(timestamp_val_max.day, 31); + EXPECT_EQ(timestamp_val_max.month, 12); + EXPECT_EQ(timestamp_val_max.year, 9999); + EXPECT_EQ(timestamp_val_max.hour, 23); + EXPECT_EQ(timestamp_val_max.minute, 59); + EXPECT_EQ(timestamp_val_max.second, 59); + EXPECT_EQ(timestamp_val_max.fraction, 0); + + this->disconnect(); +} } // namespace arrow::flight::sql::odbc From da0e2c110b41f670e5d32238547da920a0bdd24c Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 7 Jul 2025 10:32:10 -0700 Subject: [PATCH 03/12] Add more tests for sqlbindcol --- .../flight/sql/odbc/tests/statement_test.cc | 223 ++++++++++++++---- 1 file changed, 180 insertions(+), 43 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index 6e35cb1925d2..6370e56f64db 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1077,8 +1077,8 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExecDirectFloatTruncation) { } TEST_F(FlightSQLODBCRemoteTestBase, TestSQLExecDirectNullQuery) { - // Limitation on mock test server prevents null from working properly. - // Mock server has type `DENSE_UNION` for null column data. + // Limitation on mock test server prevents null from working properly, so use remote + // server instead. Mock server has type `DENSE_UNION` for null column data. this->connect(); std::wstring wsql = L"SELECT null as null_col;"; @@ -1166,8 +1166,8 @@ TEST_F(FlightSQLODBCMockTestBase, TestSQLExecDirectTruncationQueryNullIndicator) } TEST_F(FlightSQLODBCRemoteTestBase, TestSQLExecDirectNullQueryNullIndicator) { - // Limitation on mock test server prevents null from working properly. - // Mock server has type `DENSE_UNION` for null column data. + // Limitation on mock test server prevents null from working properly, so use remote + // server instead. Mock server has type `DENSE_UNION` for null column data. this->connect(); std::wstring wsql = L"SELECT null as null_col;"; @@ -1432,42 +1432,6 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExecDirectIgnoreInvalidBufLen) { this->disconnect(); } -//-AL- TODOs for tests -// TODO: add tests that replicate queries for 1) varibinary (mock server) -// -// and 2) time (remote server) -// TODO: add tests for indicator pointer for null data etc -// TODO: add SQLBindCol tests for row fetching - -TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColSimpleQuery) { - // -AL- TODO can perhaps delete this test later after TestSQLBindColDataQuery is done, - // this is just proof of concept - this->connect(); - - // Numeric Types - - // Signed Tiny Int - int8_t stiny_int_val; - SQLLEN buf_len = sizeof(stiny_int_val); - SQLLEN ind; - - SQLRETURN ret = - SQLBindCol(this->stmt, 1, SQL_C_STINYINT, &stiny_int_val, buf_len, &ind); - - std::wstring wsql = L"SELECT 1;"; - std::vector sql0(wsql.begin(), wsql.end()); - - ret = - SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); - EXPECT_EQ(ret, SQL_SUCCESS); - - ret = SQLFetch(this->stmt); - EXPECT_EQ(ret, SQL_SUCCESS); - - // Check that 1 is fetched - EXPECT_EQ(stiny_int_val, 1); -} - TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColDataQuery) { this->connect(); @@ -1482,7 +1446,7 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColDataQuery) { SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_STINYINT, &stiny_int_val_min, buf_len, &ind); EXPECT_EQ(ret, SQL_SUCCESS); - + ret = SQLBindCol(this->stmt, 2, SQL_C_STINYINT, &stiny_int_val_max, buf_len, &ind); EXPECT_EQ(ret, SQL_SUCCESS); @@ -1648,8 +1612,7 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColDataQuery) { std::wstring wsql = this->getQueryAllDataTypes(); std::vector sql0(wsql.begin(), wsql.end()); - ret = - SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); EXPECT_EQ(ret, SQL_SUCCESS); ret = SQLFetch(this->stmt); @@ -1753,4 +1716,178 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColDataQuery) { this->disconnect(); } +TEST_F(FlightSQLODBCRemoteTestBase, TestSQLBindColTimeQuery) { + // Mock server test is skipped due to limitation on the mock server. + // Time type from mock server does not include the fraction + this->connect(); + + SQL_TIME_STRUCT time_var_min{}; + SQL_TIME_STRUCT time_var_max{}; + SQLLEN buf_len = sizeof(time_var_min); + SQLLEN ind; + + SQLRETURN ret = + SQLBindCol(this->stmt, 1, SQL_C_TYPE_TIME, &time_var_min, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLBindCol(this->stmt, 2, SQL_C_TYPE_TIME, &time_var_max, buf_len, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + std::wstring wsql = + LR"( + SELECT CAST(TIME '00:00:00' AS TIME) AS time_min, + CAST(TIME '23:59:59' AS TIME) AS time_max; + )"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Check min values for time. + EXPECT_EQ(time_var_min.hour, 0); + EXPECT_EQ(time_var_min.minute, 0); + EXPECT_EQ(time_var_min.second, 0); + + // Check max values for time. + EXPECT_EQ(time_var_max.hour, 23); + EXPECT_EQ(time_var_max.minute, 59); + EXPECT_EQ(time_var_max.second, 59); + + this->disconnect(); +} + +TEST_F(FlightSQLODBCMockTestBase, TestSQLBindColVarbinaryQuery) { + // Have binary test on mock test base as remote test servers tend to have different + // formats for binary data + this->connect(); + + // varbinary + std::vector varbinary_val(3); + SQLLEN buf_len = varbinary_val.size(); + SQLLEN ind; + SQLRETURN ret = + SQLBindCol(this->stmt, 1, SQL_C_BINARY, &varbinary_val[0], buf_len, &ind); + + std::wstring wsql = L"SELECT X'ABCDEF' AS c_varbinary;"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Check varbinary values + EXPECT_EQ(varbinary_val[0], '\xAB'); + EXPECT_EQ(varbinary_val[1], '\xCD'); + EXPECT_EQ(varbinary_val[2], '\xEF'); + + this->disconnect(); +} + +TEST_F(FlightSQLODBCRemoteTestBase, TestSQLBindColNullQuery) { + // Limitation on mock test server prevents null from working properly, so use remote + // server instead. Mock server has type `DENSE_UNION` for null column data. + this->connect(); + + SQLINTEGER val; + SQLLEN ind; + + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, &val, 0, &ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + std::wstring wsql = L"SELECT null as null_col;"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Verify SQL_NULL_DATA is returned for indicator + EXPECT_EQ(ind, SQL_NULL_DATA); + + this->disconnect(); +} + +TEST_F(FlightSQLODBCRemoteTestBase, TestSQLBindColNullQueryNullIndicator) { + // Limitation on mock test server prevents null from working properly, so use remote + // server instead. Mock server has type `DENSE_UNION` for null column data. + this->connect(); + + SQLINTEGER val; + + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, &val, 0, 0); + + std::wstring wsql = L"SELECT null as null_col;"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + + EXPECT_EQ(ret, SQL_ERROR); + // Verify invalid null indicator is reported, as it is required + VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, error_state_22002); + + this->disconnect(); +} + +TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColRowFetching) { + this->connect(); + + SQLINTEGER val; + SQLLEN buf_len = sizeof(val); + SQLLEN ind; + + // Same variable will be used for column 1, the value of `val` + // should be updated after every SQLFetch call. + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, &val, buf_len, &ind); + + std::wstring wsql = + LR"( + SELECT 1 AS small_table + UNION ALL + SELECT 2 + UNION ALL + SELECT 3; + )"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Fetch row 1 + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Verify 1 is returned + EXPECT_EQ(val, 1); + + // Fetch row 2 + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Verify 2 is returned + EXPECT_EQ(val, 2); + + // Fetch row 3 + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Verify 3 is returned + EXPECT_EQ(val, 3); + + // Verify result set has no more data beyond row 3 + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_NO_DATA); + + this->disconnect(); +} + } // namespace arrow::flight::sql::odbc From 5b529f90654110641c1012088397eb6c38491914 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 7 Jul 2025 11:25:54 -0700 Subject: [PATCH 04/12] SQL_UNBIND implementation + tests --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 14 ++- .../flight/sql/odbc/tests/statement_test.cc | 89 +++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index d748c0f99c3f..15b26ebaf6bc 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -197,9 +197,18 @@ SQLRETURN SQLFreeStmt(SQLHSTMT handle, SQLUSMALLINT option) { return SQLFreeHandle(SQL_HANDLE_STMT, handle); } - // TODO Implement SQLBindCol + // TODO Implement SQLBindCol -AL- + // Implement SQL_UNBIND in SQLFreeStmt case SQL_UNBIND: { - return SQL_SUCCESS; + using ODBC::ODBCDescriptor; + using ODBC::ODBCStatement; + return ODBCStatement::ExecuteWithDiagnostics(handle, SQL_ERROR, [=]() { + ODBCStatement* statement = reinterpret_cast(handle); + ODBCDescriptor* ard = statement->GetARD(); + // Unbind columns + ard->SetHeaderField(SQL_DESC_COUNT, (void*)0, 0); + return SQL_SUCCESS; + }); } // SQLBindParameter is not supported @@ -985,6 +994,7 @@ SQLRETURN SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType using ODBC::ODBCDescriptor; using ODBC::ODBCStatement; return ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() { + // GH-47021: implement driver to return indicator value when data pointer is null ODBCStatement* statement = reinterpret_cast(stmt); ODBCDescriptor* ard = statement->GetARD(); ard->BindCol(recordNumber, cType, dataPtr, bufferLength, indicatorPtr); diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index 6370e56f64db..90491206806c 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1890,4 +1890,93 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColRowFetching) { this->disconnect(); } +TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColIndicatorOnly) { + // GH-47021: implement driver to return indicator value when data pointer is null + GTEST_SKIP(); + // Verify driver supports null data pointer with valid indicator pointer + this->connect(); + + // Numeric Types + + // Signed Tiny Int + SQLLEN stiny_int_ind; + + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_STINYINT, 0, 0, &stiny_int_ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Characters + SQLLEN buf_len = sizeof(SQLCHAR) * 2; + SQLLEN char_val_ind; + + ret = SQLBindCol(this->stmt, 25, SQL_C_CHAR, 0, buf_len, &char_val_ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Execute query and fetch data once since there is only 1 row. + std::wstring wsql = this->getQueryAllDataTypes(); + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Verify values for indicator pointer + // Signed Tiny Int + EXPECT_EQ(stiny_int_ind, 1); + + // Char array + EXPECT_EQ(char_val_ind, 1); + this->disconnect(); +} + +TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColIndicatorOnlySQLUnbind) { + // Verify driver supports valid indicator pointer after unbinding all columns + this->connect(); + + // Numeric Types + + // Signed Tiny Int + int8_t stiny_int_val; + SQLLEN stiny_int_ind; + + SQLRETURN ret = + SQLBindCol(this->stmt, 1, SQL_C_STINYINT, &stiny_int_val, 0, &stiny_int_ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Characters + SQLCHAR char_val[2]; + SQLLEN buf_len = sizeof(SQLCHAR) * 2; + SQLLEN char_val_ind; + + ret = SQLBindCol(this->stmt, 25, SQL_C_CHAR, &char_val, buf_len, &char_val_ind); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Driver should still be able to execute queries after unbinding columns + ret = SQLFreeStmt(this->stmt, SQL_UNBIND); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Execute query and fetch data once since there is only 1 row. + std::wstring wsql = this->getQueryAllDataTypes(); + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + ret = SQLFetch(this->stmt); + EXPECT_EQ(ret, SQL_SUCCESS); + + // GH-47021: implement driver to return indicator value when data pointer is null and + // uncomment the checks Verify values for indicator pointer Signed Tiny Int + // EXPECT_EQ(stiny_int_ind, 1); + + // Char array + // EXPECT_EQ(char_val_ind, 1); + + this->disconnect(); +} + +// TODO: -AL- Add tests for SQL_ATTR_ROW_ARRAY_SIZE +// after SQLSetStmtAttr is implemented + } // namespace arrow::flight::sql::odbc From 7fb4afc3fe43a92ea92477d59a318c56156ed45a Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 7 Jul 2025 13:30:54 -0700 Subject: [PATCH 05/12] Remove todo comments --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 15b26ebaf6bc..8a823ee20866 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -197,8 +197,6 @@ SQLRETURN SQLFreeStmt(SQLHSTMT handle, SQLUSMALLINT option) { return SQLFreeHandle(SQL_HANDLE_STMT, handle); } - // TODO Implement SQLBindCol -AL- - // Implement SQL_UNBIND in SQLFreeStmt case SQL_UNBIND: { using ODBC::ODBCDescriptor; using ODBC::ODBCStatement; From e44d950dd280abe3d8f24e1b5c87b672a50af91d Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 8 Jul 2025 11:58:43 -0700 Subject: [PATCH 06/12] SQLExtendedFetch initial impl Note that commits prior to this one are for SQLBindCol --- cpp/src/arrow/flight/sql/odbc/entry_points.cc | 7 ++++ cpp/src/arrow/flight/sql/odbc/odbc.def | 1 + cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 32 +++++++++++++++++++ cpp/src/arrow/flight/sql/odbc/odbc_api.h | 3 ++ .../flight/sql/odbc/tests/statement_test.cc | 5 +++ 5 files changed, 48 insertions(+) diff --git a/cpp/src/arrow/flight/sql/odbc/entry_points.cc b/cpp/src/arrow/flight/sql/odbc/entry_points.cc index ec30405870f6..b9e73728d2ac 100644 --- a/cpp/src/arrow/flight/sql/odbc/entry_points.cc +++ b/cpp/src/arrow/flight/sql/odbc/entry_points.cc @@ -137,6 +137,13 @@ SQLRETURN SQL_API SQLExecDirect(SQLHSTMT stmt, SQLWCHAR* queryText, SQLRETURN SQL_API SQLFetch(SQLHSTMT stmt) { return arrow::SQLFetch(stmt); } +SQLRETURN SQL_API SQLExtendedFetch(SQLHSTMT stmt, SQLUSMALLINT fetchOrientation, + SQLLEN fetchOffset, SQLULEN* rowCountPtr, + SQLUSMALLINT* rowStatusArray) { + return arrow::SQLExtendedFetch(stmt, fetchOrientation, fetchOffset, rowCountPtr, + rowStatusArray); +} + SQLRETURN SQL_API SQLGetData(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr) { diff --git a/cpp/src/arrow/flight/sql/odbc/odbc.def b/cpp/src/arrow/flight/sql/odbc/odbc.def index d5767c5b5c4f..a0149b67dfb9 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc.def +++ b/cpp/src/arrow/flight/sql/odbc/odbc.def @@ -33,6 +33,7 @@ EXPORTS SQLErrorW SQLExecDirectW SQLExecute + SQLExtendedFetch SQLFetch SQLForeignKeysW SQLFreeEnv diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index 8a823ee20866..a225d96dfed4 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -983,6 +983,38 @@ SQLRETURN SQLFetch(SQLHSTMT stmt) { }); } +SQLRETURN SQLExtendedFetch(SQLHSTMT stmt, SQLUSMALLINT fetchOrientation, + SQLLEN fetchOffset, SQLULEN* rowCountPtr, + SQLUSMALLINT* rowStatusArray) { + LOG_DEBUG( + "SQLExtendedFetch called with stmt: {}, fetchOrientation: {}, fetchOffset: {}, " + "rowCountPtr: {}, rowStatusArray: {}", + stmt, fetchOrientation, fetchOffset, fmt::ptr(rowCountPtr), + fmt::ptr(rowStatusArray)); + using ODBC::ODBCDescriptor; + using ODBC::ODBCStatement; + return ODBCStatement::ExecuteWithDiagnostics(stmt, SQL_ERROR, [=]() { + if (fetchOrientation != SQL_FETCH_NEXT) { + throw DriverException("Optional feature not supported.", "HYC00"); + } + // fetchOffset is ignored as only SQL_FETCH_NEXT is supported + + ODBCStatement* statement = reinterpret_cast(stmt); + + // The SQL_ROWSET_SIZE statement attribute specifies the number of rows in the + // rowset. + SQLULEN rowSetSize = statement->GetRowsetSize(); + LOG_DEBUG( + "SQL_ROWSET_SIZE value for SQLExtendedFetch: {}", rowSetSize); + if (statement->Fetch(static_cast(rowSetSize))) { + return SQL_SUCCESS; + } else { + // Reached the end of rowset + return SQL_NO_DATA; + } + }); +} + SQLRETURN SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr) { LOG_DEBUG( diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.h b/cpp/src/arrow/flight/sql/odbc/odbc_api.h index 91c45b887389..101793541d1a 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.h +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.h @@ -68,6 +68,9 @@ SQLRETURN SQLExecDirect(SQLHSTMT stmt, SQLWCHAR* queryText, SQLINTEGER textLengt SQLRETURN SQLPrepare(SQLHSTMT stmt, SQLWCHAR* queryText, SQLINTEGER textLength); SQLRETURN SQLExecute(SQLHSTMT stmt); SQLRETURN SQLFetch(SQLHSTMT stmt); +SQLRETURN SQLExtendedFetch(SQLHSTMT stmt, SQLUSMALLINT fetchOrientation, + SQLLEN fetchOffset, SQLULEN* rowCountPtr, + SQLUSMALLINT* rowStatusArray); SQLRETURN SQLBindCol(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, SQLPOINTER dataPtr, SQLLEN bufferLength, SQLLEN* indicatorPtr); SQLRETURN SQLGetData(SQLHSTMT stmt, SQLUSMALLINT recordNumber, SQLSMALLINT cType, diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index 90491206806c..de9bcaf95c08 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1979,4 +1979,9 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColIndicatorOnlySQLUnbind) { // TODO: -AL- Add tests for SQL_ATTR_ROW_ARRAY_SIZE // after SQLSetStmtAttr is implemented +// -AL- TODO: add test for SQLExtendedFetch to validate that missing indicator results in +// error_state_22002 state but SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO according to the spec. +// +// TODO: after SQLSetStmtAttr, add tests for SQL_ROWSET_SIZE which is used by SQLExtendedFetch + } // namespace arrow::flight::sql::odbc From 9bd2e14066f995434b0da42ebf27ce7b75fcf2c5 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 8 Jul 2025 13:49:11 -0700 Subject: [PATCH 07/12] Implement rowCountPtr and rowStatusArray for SQLExtendedFetch --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 5 ++--- .../odbc_impl/odbc_statement.h | 4 +++- .../odbc_impl/odbc_statement.cc | 22 +++++++++++++++---- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index a225d96dfed4..ececff191155 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -1004,9 +1004,8 @@ SQLRETURN SQLExtendedFetch(SQLHSTMT stmt, SQLUSMALLINT fetchOrientation, // The SQL_ROWSET_SIZE statement attribute specifies the number of rows in the // rowset. SQLULEN rowSetSize = statement->GetRowsetSize(); - LOG_DEBUG( - "SQL_ROWSET_SIZE value for SQLExtendedFetch: {}", rowSetSize); - if (statement->Fetch(static_cast(rowSetSize))) { + LOG_DEBUG("SQL_ROWSET_SIZE value for SQLExtendedFetch: {}", rowSetSize); + if (statement->Fetch(static_cast(rowSetSize), rowCountPtr, rowStatusArray)) { return SQL_SUCCESS; } else { // Reached the end of rowset diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h index df5ca5e34abe..73cdc2448f8f 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/odbc_impl/odbc_statement.h @@ -66,8 +66,10 @@ class ODBCStatement : public ODBCHandle { /** * @brief Returns true if the number of rows fetch was greater than zero. + * rowCountPtr and rowStatusArray are optional arguments, they are only needed for + * SQLExtendedFetch */ - bool Fetch(size_t rows); + bool Fetch(size_t rows, SQLULEN* rowCountPtr = 0, SQLUSMALLINT* rowStatusArray = 0); bool isPrepared() const; void GetStmtAttr(SQLINTEGER statementAttribute, SQLPOINTER output, diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc index 1714850323a7..a996b0dc91b2 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc @@ -317,7 +317,8 @@ void ODBCStatement::ExecuteDirect(const std::string& query) { m_isPrepared = false; } -bool ODBCStatement::Fetch(size_t rows) { +bool ODBCStatement::Fetch(size_t rows, SQLULEN* rowCountPtr, + SQLUSMALLINT* rowStatusArray) { if (m_hasReachedEndOfResult) { m_ird->SetRowsProcessed(0); return false; @@ -350,11 +351,24 @@ bool ODBCStatement::Fetch(size_t rows) { m_currentArd->NotifyBindingsHavePropagated(); } - size_t rowsFetched = m_currenResult->Move(rows, m_currentArd->GetBindOffset(), - m_currentArd->GetBoundStructOffset(), - m_ird->GetArrayStatusPtr()); + uint16_t* row_status_array = 0; + if (rowStatusArray) { + // For SQLExtendedFetch only + row_status_array = rowStatusArray; + } else { + row_status_array = m_ird->GetArrayStatusPtr(); + } + + size_t rowsFetched = + m_currenResult->Move(rows, m_currentArd->GetBindOffset(), + m_currentArd->GetBoundStructOffset(), row_status_array); m_ird->SetRowsProcessed(static_cast(rowsFetched)); + if (rowCountPtr) { + // For SQLExtendedFetch only + *rowCountPtr = rowsFetched; + } + m_rowNumber += rowsFetched; m_hasReachedEndOfResult = rowsFetched != rows; return rowsFetched != 0; From 2b7d751ba6023855b318fbf1982b98aefcc34cf2 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Tue, 8 Jul 2025 16:29:43 -0700 Subject: [PATCH 08/12] Add tests for SQLExtendedFetch * `SQLExtendedFetch` doesn't return `SQL_SUCCESS_WITH_INFO` for error state 22002. --- .../flight/sql/odbc/tests/statement_test.cc | 103 +++++++++++++++++- 1 file changed, 99 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index de9bcaf95c08..40de574fd4df 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1979,9 +1979,104 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColIndicatorOnlySQLUnbind) { // TODO: -AL- Add tests for SQL_ATTR_ROW_ARRAY_SIZE // after SQLSetStmtAttr is implemented -// -AL- TODO: add test for SQLExtendedFetch to validate that missing indicator results in -// error_state_22002 state but SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO according to the spec. -// -// TODO: after SQLSetStmtAttr, add tests for SQL_ROWSET_SIZE which is used by SQLExtendedFetch +// -AL- TODO: after SQLSetStmtAttr, add tests for SQL_ROWSET_SIZE which is used by +// SQLExtendedFetch and fetch all 3 rows at once +TYPED_TEST(FlightSQLODBCTestBase, TestSQLExtendedFetchRowFetching) { + this->connect(); + + SQLINTEGER val; + SQLLEN buf_len = sizeof(val); + SQLLEN ind; + + // Same variable will be used for column 1, the value of `val` + // should be updated after every SQLFetch call. + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, &val, buf_len, &ind); + + std::wstring wsql = + LR"( + SELECT 1 AS small_table + UNION ALL + SELECT 2 + UNION ALL + SELECT 3; + )"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + // Fetch row 1. + // row_count* and row_status* returns information on the SQLExtendedFetch call + SQLULEN row_count1; + SQLUSMALLINT row_status1[1]; + + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count1, row_status1); + EXPECT_EQ(ret, SQL_SUCCESS); + EXPECT_EQ(row_status1[0], SQL_SUCCESS); + + // Verify 1 is returned + EXPECT_EQ(val, 1); + EXPECT_EQ(row_count1, 1); + + // Fetch row 2 + SQLULEN row_count2; + SQLUSMALLINT row_status2[1]; + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count2, row_status2); + EXPECT_EQ(ret, SQL_SUCCESS); + EXPECT_EQ(row_status2[0], SQL_SUCCESS); + + // Verify 2 is returned + EXPECT_EQ(val, 2); + EXPECT_EQ(row_count2, 1); + + // Fetch row 3 + SQLULEN row_count3; + SQLUSMALLINT row_status3[1]; + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count3, row_status3); + EXPECT_EQ(ret, SQL_SUCCESS); + EXPECT_EQ(row_status3[0], SQL_SUCCESS); + + // Verify 3 is returned + EXPECT_EQ(val, 3); + EXPECT_EQ(row_count3, 1); + + // Verify result set has no more data beyond row 3 + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count3, row_status3); + EXPECT_EQ(ret, SQL_NO_DATA); + + this->disconnect(); +} + +// -AL- TODO: added test for SQLExtendedFetch to validate that missing indicator results in +// error_state_22002 state but SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO +// according to the spec. . But this doesn't work, +// need to ask James if we should raise an issue for this. +TEST_F(FlightSQLODBCRemoteTestBase, TestSQLExtendedFetchQueryNullIndicator) { + // Limitation on mock test server prevents null from working properly, so use remote + // server instead. Mock server has type `DENSE_UNION` for null column data. + GTEST_SKIP(); + this->connect(); + + SQLINTEGER val; + + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, &val, 0, 0); + + std::wstring wsql = L"SELECT null as null_col;"; + std::vector sql0(wsql.begin(), wsql.end()); + + ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); + EXPECT_EQ(ret, SQL_SUCCESS); + + SQLULEN row_count1; + SQLUSMALLINT row_status1[1]; + + // SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO for 22002 state + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count1, row_status1); + EXPECT_EQ(ret, SQL_SUCCESS_WITH_INFO); //-AL- driver is currently returning SQL_ERROR + // Verify invalid null indicator is reported, as it is required + VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, error_state_22002); + + this->disconnect(); +} } // namespace arrow::flight::sql::odbc From 3a9a4f4234bd457763b9668ca0bd184020961ade Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Mon, 14 Jul 2025 14:12:36 -0700 Subject: [PATCH 09/12] Add tests for `SQL_ROWSET_SIZE` --- cpp/src/arrow/flight/sql/odbc/odbc_api.cc | 2 + .../flight/sql/odbc/tests/statement_test.cc | 68 +++++++------------ 2 files changed, 28 insertions(+), 42 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc index ececff191155..cad6fdb4fff0 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbc_api.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbc_api.cc @@ -986,6 +986,8 @@ SQLRETURN SQLFetch(SQLHSTMT stmt) { SQLRETURN SQLExtendedFetch(SQLHSTMT stmt, SQLUSMALLINT fetchOrientation, SQLLEN fetchOffset, SQLULEN* rowCountPtr, SQLUSMALLINT* rowStatusArray) { + // GH-47110: SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO for certain diag + // states LOG_DEBUG( "SQLExtendedFetch called with stmt: {}, fetchOrientation: {}, fetchOffset: {}, " "rowCountPtr: {}, rowStatusArray: {}", diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index 40de574fd4df..b25ab5f71d39 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1979,18 +1979,20 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLBindColIndicatorOnlySQLUnbind) { // TODO: -AL- Add tests for SQL_ATTR_ROW_ARRAY_SIZE // after SQLSetStmtAttr is implemented -// -AL- TODO: after SQLSetStmtAttr, add tests for SQL_ROWSET_SIZE which is used by -// SQLExtendedFetch and fetch all 3 rows at once TYPED_TEST(FlightSQLODBCTestBase, TestSQLExtendedFetchRowFetching) { + // Set SQL_ROWSET_SIZE to fetch 3 rows at once this->connect(); - SQLINTEGER val; + constexpr SQLULEN rows = 3; + SQLINTEGER val[rows]; SQLLEN buf_len = sizeof(val); - SQLLEN ind; + SQLLEN ind[rows]; // Same variable will be used for column 1, the value of `val` // should be updated after every SQLFetch call. - SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, &val, buf_len, &ind); + SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, val, buf_len, ind); + + ret = SQLSetStmtAttr(this->stmt, SQL_ROWSET_SIZE, reinterpret_cast(rows), 0); std::wstring wsql = LR"( @@ -2005,53 +2007,36 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExtendedFetchRowFetching) { ret = SQLExecDirect(this->stmt, &sql0[0], static_cast(sql0.size())); EXPECT_EQ(ret, SQL_SUCCESS); - // Fetch row 1. - // row_count* and row_status* returns information on the SQLExtendedFetch call - SQLULEN row_count1; - SQLUSMALLINT row_status1[1]; + // Fetch row 1-3. + SQLULEN row_count; + SQLUSMALLINT row_status[rows]; - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count1, row_status1); + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count, row_status); EXPECT_EQ(ret, SQL_SUCCESS); - EXPECT_EQ(row_status1[0], SQL_SUCCESS); + EXPECT_EQ(row_count, 3); - // Verify 1 is returned - EXPECT_EQ(val, 1); - EXPECT_EQ(row_count1, 1); - - // Fetch row 2 - SQLULEN row_count2; - SQLUSMALLINT row_status2[1]; - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count2, row_status2); - EXPECT_EQ(ret, SQL_SUCCESS); - EXPECT_EQ(row_status2[0], SQL_SUCCESS); + for (int i = 0; i < rows; i++) { + EXPECT_EQ(row_status[i], SQL_SUCCESS); + } - // Verify 2 is returned - EXPECT_EQ(val, 2); - EXPECT_EQ(row_count2, 1); - - // Fetch row 3 - SQLULEN row_count3; - SQLUSMALLINT row_status3[1]; - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count3, row_status3); - EXPECT_EQ(ret, SQL_SUCCESS); - EXPECT_EQ(row_status3[0], SQL_SUCCESS); - - // Verify 3 is returned - EXPECT_EQ(val, 3); - EXPECT_EQ(row_count3, 1); + // Verify 1 is returned for row 1 + EXPECT_EQ(val[0], 1); + // Verify 2 is returned for row 2 + EXPECT_EQ(val[1], 2); + // Verify 3 is returned for row 3 + EXPECT_EQ(val[2], 3); // Verify result set has no more data beyond row 3 - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count3, row_status3); + SQLULEN row_count2; + SQLUSMALLINT row_status2[rows]; + ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count2, row_status2); EXPECT_EQ(ret, SQL_NO_DATA); this->disconnect(); } -// -AL- TODO: added test for SQLExtendedFetch to validate that missing indicator results in -// error_state_22002 state but SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO -// according to the spec. . But this doesn't work, -// need to ask James if we should raise an issue for this. TEST_F(FlightSQLODBCRemoteTestBase, TestSQLExtendedFetchQueryNullIndicator) { + // GH-47110: SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO for 22002 // Limitation on mock test server prevents null from working properly, so use remote // server instead. Mock server has type `DENSE_UNION` for null column data. GTEST_SKIP(); @@ -2072,8 +2057,7 @@ TEST_F(FlightSQLODBCRemoteTestBase, TestSQLExtendedFetchQueryNullIndicator) { // SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO for 22002 state ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count1, row_status1); - EXPECT_EQ(ret, SQL_SUCCESS_WITH_INFO); //-AL- driver is currently returning SQL_ERROR - // Verify invalid null indicator is reported, as it is required + EXPECT_EQ(ret, SQL_SUCCESS_WITH_INFO); VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, error_state_22002); this->disconnect(); From dab4665f7d600b177ede10db551a2170825d3f9b Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 16 Jul 2025 12:04:57 -0700 Subject: [PATCH 10/12] Address comments from James --- .../sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc index a996b0dc91b2..3940bdccc775 100644 --- a/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc +++ b/cpp/src/arrow/flight/sql/odbc/odbcabstraction/odbc_impl/odbc_statement.cc @@ -351,17 +351,17 @@ bool ODBCStatement::Fetch(size_t rows, SQLULEN* rowCountPtr, m_currentArd->NotifyBindingsHavePropagated(); } - uint16_t* row_status_array = 0; + uint16_t* arrayStatusPtr; if (rowStatusArray) { // For SQLExtendedFetch only - row_status_array = rowStatusArray; + arrayStatusPtr = rowStatusArray; } else { - row_status_array = m_ird->GetArrayStatusPtr(); + arrayStatusPtr = m_ird->GetArrayStatusPtr(); } size_t rowsFetched = m_currenResult->Move(rows, m_currentArd->GetBindOffset(), - m_currentArd->GetBoundStructOffset(), row_status_array); + m_currentArd->GetBoundStructOffset(), arrayStatusPtr); m_ird->SetRowsProcessed(static_cast(rowsFetched)); if (rowCountPtr) { From ef95af60278f69d751615b37268d83f27fe175a6 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 16 Jul 2025 12:05:53 -0700 Subject: [PATCH 11/12] Trigger deploy or CI From a6ac9a167adf830219e30bc001102e83c8813411 Mon Sep 17 00:00:00 2001 From: "Alina (Xi) Li" Date: Wed, 16 Jul 2025 13:04:43 -0700 Subject: [PATCH 12/12] Fix CI build errors --- cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc index d962f3964206..ce67909896bb 100644 --- a/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc +++ b/cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc @@ -1064,7 +1064,6 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExecDirectFloatTruncation) { EXPECT_EQ(ret, SQL_SUCCESS); int16_t ssmall_int_val; - SQLLEN buf_len = sizeof(ssmall_int_val); ret = SQLGetData(this->stmt, 1, SQL_C_SSHORT, &ssmall_int_val, 0, 0); EXPECT_EQ(ret, SQL_SUCCESS_WITH_INFO); @@ -2042,7 +2041,8 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExtendedFetchRowFetching) { // should be updated after every SQLFetch call. SQLRETURN ret = SQLBindCol(this->stmt, 1, SQL_C_LONG, val, buf_len, ind); - ret = SQLSetStmtAttr(this->stmt, SQL_ROWSET_SIZE, reinterpret_cast(rows), 0); + ret = + SQLSetStmtAttr(this->stmt, SQL_ROWSET_SIZE, reinterpret_cast(rows), 0); std::wstring wsql = LR"( @@ -2061,7 +2061,7 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExtendedFetchRowFetching) { SQLULEN row_count; SQLUSMALLINT row_status[rows]; - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count, row_status); + ret = SQLExtendedFetch(this->stmt, SQL_FETCH_NEXT, 0, &row_count, row_status); EXPECT_EQ(ret, SQL_SUCCESS); EXPECT_EQ(row_count, 3); @@ -2079,7 +2079,7 @@ TYPED_TEST(FlightSQLODBCTestBase, TestSQLExtendedFetchRowFetching) { // Verify result set has no more data beyond row 3 SQLULEN row_count2; SQLUSMALLINT row_status2[rows]; - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count2, row_status2); + ret = SQLExtendedFetch(this->stmt, SQL_FETCH_NEXT, 0, &row_count2, row_status2); EXPECT_EQ(ret, SQL_NO_DATA); this->disconnect(); @@ -2106,7 +2106,7 @@ TEST_F(FlightSQLODBCRemoteTestBase, TestSQLExtendedFetchQueryNullIndicator) { SQLUSMALLINT row_status1[1]; // SQLExtendedFetch should return SQL_SUCCESS_WITH_INFO for 22002 state - ret = SQLExtendedFetch(stmt, SQL_FETCH_NEXT, 0, &row_count1, row_status1); + ret = SQLExtendedFetch(this->stmt, SQL_FETCH_NEXT, 0, &row_count1, row_status1); EXPECT_EQ(ret, SQL_SUCCESS_WITH_INFO); VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, error_state_22002);