GH-46575: [C++][FlightRPC] Add Diagnostic tests #47764

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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+
+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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

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


Suggested change

public:

using List = std::list<T>;

done, removed using List

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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

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


Suggested change

template <typename T>

class ErrorsOdbcV2Test : public T {

public:

using List = std::list<T>;

};

template <typename T>

class ErrorsOdbcV2Test : public T {

};

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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

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


done, removed using List

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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

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


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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

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


Added RAII helper for this (ErrorsHandleTest)

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

cpp/src/arrow/flight/sql/odbc/tests/errors_test.cc

+  // 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

cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h

+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 <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt

Requires SQLDriverConnect Implementation Co-authored-by: rscales <[email protected]>

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.

alinaliBQ mentioned this pull request Oct 8, 2025

GH-46575: [C++][FlightRPC] ODBC Diagnostics Report #47763

Merged

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

lidavidm reviewed Oct 9, 2025

View reviewed changes

github-actions bot 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 4f4bbc0 Compare October 21, 2025 21:55

alinaliBQ commented Oct 21, 2025

View reviewed changes

github-actions bot 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-actions bot 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 99d8ec6 Compare October 22, 2025 22:30

github-actions bot 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 eddea34 Compare 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-actions bot 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 2768502 Compare October 27, 2025 23:51

github-actions bot 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-actions bot 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

cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt Outdated Show resolved Hide resolved

alinaliBQ force-pushed the gh-46575-odbc-diagnostics-tests branch from 2768502 to d808e34 Compare 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 fdaf290 Compare 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 34546b5 Compare December 2, 2025 20:56

Add Diagnostic tests

9a1ebf4

Requires SQLDriverConnect Implementation Co-authored-by: rscales <[email protected]>

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

jduo approved these changes Dec 8, 2025

View reviewed changes

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


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

		TYPED_TEST(FlightSQLODBCTestBase, TestSQLGetDiagFieldWForConnectFailure) {

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

GH-46575: [C++][FlightRPC] Add Diagnostic tests #47764

GH-46575: [C++][FlightRPC] Add Diagnostic tests #47764

Uh oh!

Conversation

alinaliBQ commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alinaliBQ commented Oct 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinaliBQ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinaliBQ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kou commented Dec 9, 2025

Uh oh!

jduo commented Dec 9, 2025

Uh oh!

kou commented Dec 9, 2025

Uh oh!

conbench-apache-arrow bot commented Dec 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

alinaliBQ commented Oct 8, 2025 •

edited

Loading