Copilot commented on code in PR #49786:
URL: https://github.com/apache/arrow/pull/49786#discussion_r3285537900
##########
cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc:
##########
@@ -1986,8 +1986,13 @@ TYPED_TEST(StatementTest, TestSQLMoreResultsNoData) {
TYPED_TEST(StatementTest, TestSQLMoreResultsInvalidFunctionSequence) {
// Verify function sequence error state is reported when SQLMoreResults is
called
// without executing any queries
+#ifdef __linux__
+ ASSERT_EQ(SQL_NO_DATA, SQLMoreResults(this->stmt));
+ VerifyOdbcErrorState(SQL_HANDLE_STMT, this->stmt, kErrorState00000);
Review Comment:
`VerifyOdbcErrorState` unconditionally calls `SQLGetDiagRec` and expects a
SQLSTATE string, but successful calls (including `SQL_NO_DATA`) typically have
no diagnostic record, so this check is likely to fail (sql_state stays empty).
Instead, explicitly assert that `SQLGetDiagRec(..., rec=1, ...)` returns
`SQL_NO_DATA` (no diagnostics) or adjust the helper to handle this case.
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc:
##########
@@ -117,31 +124,34 @@ 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.
std::wstring trace_file = L"invalid/file/path";
Review Comment:
This comment says the placeholder trace file path is used because "we want
the call to fail", but on the non-Windows branch the test now expects
`SQL_SUCCESS`. Please update the comment to match the platform-specific
behavior (or remove the rationale).
##########
cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc:
##########
@@ -1043,7 +1043,7 @@ TEST_F(StatementMockTest,
TestSQLExecDirectVarbinaryTruncation) {
ASSERT_EQ(SQL_SUCCESS,
SQLGetData(this->stmt, 1, SQL_C_BINARY, &varbinary_val2[0],
buf_len, &ind));
- EXPECT_EQ('\xAB', varbinary_val[0]);
+ EXPECT_EQ(static_cast<char>('\xAB'), static_cast<char>(varbinary_val[0]));
Review Comment:
In the second SQLGetData fetch, the buffer is `varbinary_val2` but the
assertion still checks `varbinary_val[0]`. This means the test isn't validating
the second fetch result and can pass even if the second fetch is wrong.
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc:
##########
@@ -331,9 +335,11 @@ TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoOdbcVer) {
std::wstring result = ConvertToWString(value);
-#ifdef __APPLE__
+#if defined(__APPLE__)
EXPECT_EQ(std::wstring(L"03.52.0000"), result);
-#else
+#elif defined(__linux__)
+ EXPECT_EQ(std::wstring(L"03.52"), result);
+#else // WINDOWS
EXPECT_EQ(std::wstring(L"03.80.0000"), result);
#endif // __APPLE__
Review Comment:
The preprocessor block now uses `#if defined(__APPLE__) / #elif
defined(__linux__) / #else`, but the trailing `#endif` comment still says
`__APPLE__`, which is misleading when reading the code.
--
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]
