Copilot commented on code in PR #49786:
URL: https://github.com/apache/arrow/pull/49786#discussion_r3293542163
##########
cpp/src/arrow/flight/sql/odbc/tests/tables_test.cc:
##########
@@ -138,6 +141,8 @@ TEST_F(TablesMockTest, SQLTablesTestGetSchemaHasNoData) {
ValidateFetch(this->stmt, SQL_NO_DATA);
}
+// GH-49702: TODO Disabled on Linux due to BlockingQueue issue
+#ifndef __linux__
TEST_F(TablesRemoteTest, SQLTablesTestGetMetadataForAllSchemas) {
Review Comment:
The `#ifndef __linux__` guard that starts here isn't closed until much later
(line ~350), which ends up disabling a large block of subsequent tests
(including several `TablesMockTest` cases) on Linux. If only specific tests are
impacted by GH-49702, the guard should be narrowed so unrelated tests still run
on Linux.
##########
cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc:
##########
@@ -1986,8 +1986,12 @@ TYPED_TEST(StatementTest, TestSQLMoreResultsNoData) {
TYPED_TEST(StatementTest, TestSQLMoreResultsInvalidFunctionSequence) {
// Verify function sequence error state is reported when SQLMoreResults is
called
// without executing any queries
Review Comment:
This test-level comment says we're verifying a function-sequence error
state, but the Linux branch now expects `SQL_NO_DATA` and does not verify any
diagnostic state. Please update the comment (or add platform-specific wording)
so the intent matches the assertions.
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc:
##########
@@ -117,31 +124,33 @@ TYPED_TEST(ConnectionAttributeTest,
TestSQLSetConnectAttrTraceDMOnly) {
}
#endif // __APPLE__
-TYPED_TEST(ConnectionAttributeTest, TestSQLSetConnectAttrTracefileDMOnly) {
+TYPED_TEST(ConnectionAttributePreConnectTest,
TestSQLSetConnectAttrTracefileDMOnly) {
// Verify DM-only attribute is handled by Driver Manager
- // Use placeholder value as we want the call to fail, or else
- // the driver manager will produce a trace file.
+ // Use placeholder value to avoid the driver manager from producing a trace
file.
std::wstring trace_file = L"invalid/file/path";
std::vector<SQLWCHAR> trace_file0(trace_file.begin(), trace_file.end());
+
+#ifdef _WIN32
ASSERT_EQ(SQL_ERROR, SQLSetConnectAttr(this->conn, SQL_ATTR_TRACEFILE,
&trace_file0[0],
static_cast<SQLINTEGER>(trace_file0.size())));
-#ifdef __APPLE__
- VerifyOdbcErrorState(SQL_HANDLE_DBC, this->conn, kErrorStateHYC00);
-#else
VerifyOdbcErrorState(SQL_HANDLE_DBC, this->conn, kErrorStateHY000);
-#endif // __APPLE__
+#else // Mac & Linux
+ ASSERT_EQ(SQL_SUCCESS,
+ SQLSetConnectAttr(this->conn, SQL_ATTR_TRACEFILE, &trace_file0[0],
+ static_cast<SQLINTEGER>(trace_file0.size())));
Review Comment:
`trace_file0` is built from a `std::wstring` without an explicit null
terminator, but it is passed to `SQLSetConnectAttr` as a string pointer.
Elsewhere in the test suite (e.g. `ASSIGN_SQLWCHAR_ARR` on Linux) wide-string
buffers are explicitly null-terminated before being passed to ODBC APIs.
Consider appending a terminating 0 (and using `SQL_NTS` or an explicit length
excluding the terminator) to avoid driver manager implementations reading past
the buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
