GH-46575: [C++][FlightRPC] Add Diagnostic tests by alinaliBQ · Pull Request #47764 · apache/arrow

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ · 2025-10-08T23:57:51Z

Rationale for this change

Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics.

What changes are included in this PR?

Add tests for ODBC diagnostics

Are these changes tested?

PR depends on #47971 for tests to work.

Tested locally.

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL][ODBC] Develop basic diagnostic record handling: SQLGetDiagField, SQLGetDiagRec #46575

alinaliBQ · 2025-10-08T23:59:56Z

@lidavidm @kou Please review this draft ODBC API PR. The implementation PR is at #47763.

lidavidm · 2025-10-09T00:15:23Z

+
+namespace arrow::flight::sql::odbc {
+
+TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {


Same comment here, we can subclass the base fixture and create a specific fixture for this file.

lidavidm · 2025-10-09T00:15:47Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


The test will continue even if EXPECT_ fails. Usually these kinds of checks should be ASSERT.

It would be more concise to just ASSERT_EQ(SQL_SUCCESS, SQLAllocEnv(&env)) and so on.

yup, updated the code to use ASSERT_EQ

lidavidm · 2025-10-09T00:17:00Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


If the test name starts with DISABLED_, GTest will ignore the test automatically

changed to use DISABLED_ and removed GTEST_SKIP

alinaliBQ

In-progress of addressing comments. The team is still working on improving the subclass logic for separate test fixture (discussion is in #47788)

alinaliBQ · 2025-10-21T21:48:16Z

+ // Allocate an environment handle
+ SQLRETURN ret = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, ret);


yup, updated the code to use ASSERT_EQ

alinaliBQ · 2025-10-21T21:48:30Z

+ // Test is disabled because driver manager on Windows does not pass through SQL_NTS
+ // This test case can be potentially used on macOS/Linux
+ GTEST_SKIP();


changed to use DISABLED_ and removed GTEST_SKIP

lidavidm · 2025-10-22T00:32:12Z

+ public:
+ using List = std::list<T>;


Suggested change
public:
using List = std::list<T>;

done, removed using List

lidavidm · 2025-10-22T00:32:19Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


Suggested change
template <typename T>
classErrorsOdbcV2Test : publicT {
public:
using List = std::list<T>;
};
template <typename T>
classErrorsOdbcV2Test : publicT {
};

lidavidm · 2025-10-22T00:37:10Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


maybe consider an RAII helper to free resources in tests

Added RAII helper for this (ErrorsHandleTest)

lidavidm · 2025-10-22T00:37:32Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


EXPECT_FALSE?

Yup good catch, fixed

alinaliBQ

Addressed comments

alinaliBQ · 2025-10-22T21:16:20Z

+ public:
+ using List = std::list<T>;


done, removed using List

alinaliBQ · 2025-10-22T21:16:29Z

+template <typename T>
+class ErrorsOdbcV2Test : public T {
+ public:
+ using List = std::list<T>;
+};


alinaliBQ · 2025-10-22T22:31:05Z

+ // Free connection handle
+ EXPECT_EQ(SQL_SUCCESS, SQLFreeHandle(SQL_HANDLE_DBC, conn));


Added RAII helper for this (ErrorsHandleTest)

alinaliBQ · 2025-10-22T22:33:39Z

+ // API not implemented error from driver manager
+ EXPECT_EQ(std::wstring(L"IM001"), std::wstring(sql_state));
+
+ EXPECT_TRUE(!std::wstring(message).empty());


Yup good catch, fixed

lidavidm · 2025-10-27T00:25:51Z

+static constexpr std::string_view authorization_header = "authorization";
+static constexpr std::string_view bearer_prefix = "Bearer ";
+static constexpr std::string_view test_token = "t0k3n";


Can we keep to the naming scheme? kAuthorizationHeader etc.

Yes, the change was added by accident. Fixed.
The RAII helper logic will also be in #47971, after #47971 is merged, I will rebase this PR.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see #47764) ### Are these changes tested? Tests will be in a separate PR (see #47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: #46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

kou · 2025-12-09T02:34:48Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

jduo · 2025-12-09T17:42:23Z

@jduo Could you use our merge script https://github.com/apache/arrow/tree/main/dev#how-to-merge-a-pull-request in this repository?

Sure @kou . Do you mean to re-do the last two merges using the scripts or use the merge script going forward?

Thanks

kou · 2025-12-09T21:48:16Z

The latter. (We can't re-do merges in public repositories because it changes commit hashes...)

conbench-apache-arrow · 2025-12-21T02:06:53Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 6fed4f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change ODBC needs to provide diagnostic information so users can debug the error ### What changes are included in this PR? - Implementation of SQLGetDiagField and SQLGetDiagRec Tests are included in separate PR (see apache#47764) ### Are these changes tested? Tests will be in a separate PR (see apache#47764). Other APIs depend on SQLGetDiagField and SQLGetDiagRec to get error reporting functionality, and tests for SQLGetDiagField and SQLGetDiagRec depend on other APIs for creating errors, as these diagnostic APIs alone do not initiate any errors. Changes tested locally ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>

alinaliBQ mentioned this pull request Oct 8, 2025
GH-46575: [C++][FlightRPC] ODBC Diagnostics Report #47763
Merged

github-actionsBot added Component: C++ Component: FlightRPC awaiting review Awaiting review labels Oct 8, 2025

lidavidm reviewed Oct 9, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 9, 2025

alinaliBQ mentioned this pull request Oct 10, 2025
GH-46734: [C++] Arrow Flight SQL ODBC layer #46099
Open

alinaliBQ force-pushed the gh-46575-odbc-diagnostics-tests branch from 19da64d to 4f4bbc0Compare October 21, 2025 21:55

alinaliBQ commented Oct 21, 2025
View reviewed changes

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 21, 2025

lidavidm reviewed Oct 22, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 22, 2025

alinaliBQ force-pushed the gh-46575-odbc-diagnostics-tests branch from 4f4bbc0 to 99d8ec6Compare October 22, 2025 22:30

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 22, 2025

alinaliBQ force-pushed the gh-46575-odbc-diagnostics-tests branch from 99d8ec6 to eddea34Compare October 22, 2025 22:34

alinaliBQ commented Oct 22, 2025
View reviewed changes

alinaliBQ requested a review from lidavidm October 22, 2025 22:34

lidavidm reviewed Oct 27, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 27, 2025

alinaliBQ force-pushed the gh-46575-odbc-diagnostics-tests branch from eddea34 to 2768502Compare October 27, 2025 23:51

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 27, 2025

lidavidm approved these changes Oct 31, 2025
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 31, 2025

justing-bq reviewed Nov 6, 2025
View reviewed changes

Comment threadcpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt Outdated

alinaliBQ force-pushed the gh-46575-odbc-diagnostics-tests branch from 2768502 to d808e34Compare November 7, 2025 00:41

alinaliBQ mentioned this pull request Nov 11, 2025
GH-47707: [C++][FlightRPC] Add tests for descriptor handle allocation #48053
Merged

justing-bq force-pushed the gh-46575-odbc-diagnostics-tests branch 2 times, most recently from 3bdd474 to fdaf290Compare November 17, 2025 21:46

alinaliBQ marked this pull request as ready for review November 21, 2025 00:23

alinaliBQ requested a review from lidavidm December 1, 2025 19:12

justing-bq force-pushed the gh-46575-odbc-diagnostics-tests branch from fdaf290 to 34546b5Compare December 2, 2025 20:56

Add Diagnostic tests
9a1ebf4
Requires SQLDriverConnect Implementation Co-authored-by: rscales <robscales@icloud.com>

justing-bq force-pushed the gh-46575-odbc-diagnostics-tests branch from 34546b5 to 9a1ebf4Compare December 3, 2025 18:25

jduo approved these changes Dec 8, 2025
View reviewed changes

jduo merged commit 6fed4f3 into apache:mainDec 8, 2025
45 checks passed