GH-47786: [C++][FlightRPC] Establish ODBC tests #47788

justing-bq · 2025-10-10T20:54:58Z

Rationale for this change

What changes are included in this PR?

Created a new directory/subproject for tests.
Added a testing utility class called odbc_test_suite.
Created new test file with one simple test.

Are these changes tested?

Yes

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL] Add tests for arrow::flight::sql::odbc #47786

github-actions · 2025-10-10T20:55:23Z

⚠️ GitHub issue #47786 has been automatically assigned in GitHub to PR creator.

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

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

alinaliBQ · 2025-10-10T22:13:26Z

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

+// TODO: once RegisterDsn is implemented in Mac and Linux, the following can be
+// re-enabled.
+#if defined _WIN32 || defined _WIN64
+bool WriteDSN(std::string connection_str) {


Could you please raise a GitHub issue for this todo?

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

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

alinaliBQ · 2025-10-15T18:21:57Z

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

+
+void FlightSQLODBCRemoteTestBase::SetUp() {
+  if (arrow::internal::GetEnvVar(TEST_CONNECT_STR).ValueOr("").empty()) {
+    GTEST_SKIP() << "Skipping FlightSQLODBCRemoteTestBase test: TEST_CONNECT_STR not set";


Can we change to use arrow log for this?

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

cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h

cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h

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

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

alinaliBQ

LGTM

cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h

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

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

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

lidavidm · 2025-10-20T23:37:51Z

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

+template <typename T>
+class FlightSQLODBCTestBase : public T {
+ public:
+  using List = std::list<T>;
+};
+
+using TestTypes =
+    ::testing::Types<FlightSQLODBCMockTestBase, FlightSQLODBCRemoteTestBase>;
+TYPED_TEST_SUITE(FlightSQLODBCTestBase, TestTypes);


Why is the parametrization so complex here? Subclassing a type parameter looks suspicious...Is the idea to test with a mocked server and an in process server?

As mentioned in the other PR I would prefer we don't scatter tests using this fixture across different files. So instead of having FlightSQLODBCTestBase defined here I would rather we have proper docstrings for FlightSQLODBCMockTestBase and FlightSQLODBCRemoteTestBase that explain what they're for and how you're expected to parametrize them, and define the equivalent of FlightSQLODBCTestBase in individual test files where necessary.

For parametrization, we followed Google's instruction for creating typed tests (https://google.github.io/googletest/advanced.html#typed-tests), and their example used using List = std::list<T>; etc.

Is the idea to test with a mocked server and an in process server?

Yes that is right. For majority of ODBC tests, we want to run the tests on both a local mock server and a remote server. I agree that FlightSQLODBCTestBase can be removed cc @justing-bq.

their example used using List = std::list<T>

Um, but do you need to use that? If not, please remove it. It's not required.

Note the justification for typed tests:

Suppose you have multiple implementations of the same interface and want to make sure that all of them satisfy some common requirements. Or, you may have defined several types that are supposed to conform to the same “concept” and you want to verify it. In both cases, you want the same test logic repeated for different types.

While it's not like you can't use the mechanism in other ways, it's a little surprising to see it used in this way.

FlightSQLODBCTestBase is now removed. Future PRs will be adding separate equivalent text fixtures for each file.

lidavidm · 2025-10-22T00:30:48Z

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

+};
+
+/** ODBC read buffer size. */
+constexpr int ODBC_BUFFER_SIZE = 1024;


Suggested change

constexpr int ODBC_BUFFER_SIZE = 1024;

static constexpr int kOdbcBufferSize = 1024;

lidavidm · 2025-10-22T00:31:23Z

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

+#define TEST_CONNECT_STR "ARROW_FLIGHT_SQL_ODBC_CONN"
+#define TEST_DSN "Apache Arrow Flight SQL Test DSN"


While we're at it, these could be static constexpr std::string_view (maybe that's inconvenient though)

Co-Authored-By: rscales <[email protected]> Co-Authored-By: justing-bq <[email protected]> Co-Authored-By: alinalibq <[email protected]>

conbench-apache-arrow · 2025-10-22T10:11:42Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0a0ee3e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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

justing-bq force-pushed the startTest branch from fc65d84 to 32b25bc Compare October 10, 2025 21:36

alinaliBQ reviewed Oct 10, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 10, 2025

justing-bq force-pushed the startTest branch from 32b25bc to 71b3fcd Compare October 14, 2025 17:16

justing-bq marked this pull request as ready for review October 14, 2025 18:01

justing-bq requested a review from lidavidm as a code owner October 14, 2025 18:01

alinaliBQ reviewed Oct 15, 2025

View reviewed changes

lidavidm reviewed Oct 16, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 16, 2025

justing-bq force-pushed the startTest branch from 71b3fcd to 7ac3278 Compare October 16, 2025 21:03

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 16, 2025

alinaliBQ reviewed Oct 16, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h Outdated Show resolved Hide resolved

alinaliBQ approved these changes Oct 17, 2025

View reviewed changes

justing-bq requested a review from lidavidm October 17, 2025 23:07

lidavidm approved these changes Oct 19, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 19, 2025

justing-bq force-pushed the startTest branch from 7ac3278 to 9197678 Compare October 20, 2025 16:50

justing-bq requested a review from lidavidm October 20, 2025 20:02

lidavidm reviewed Oct 20, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 20, 2025

justing-bq force-pushed the startTest branch from 9197678 to 80cfba9 Compare October 21, 2025 17:43

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

justing-bq requested a review from lidavidm October 21, 2025 17:46

justing-bq force-pushed the startTest branch from 80cfba9 to 4f320ff Compare October 21, 2025 17:49

This was referenced Oct 21, 2025

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

Draft

GH-47708: [C++][FlightRPC] Connection Attribute Support for ODBC #47772

Draft

justing-bq force-pushed the startTest branch 2 times, most recently from 36903c7 to 1653d29 Compare October 21, 2025 22:47

lidavidm approved these changes Oct 22, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 22, 2025

justing-bq force-pushed the startTest branch from 1653d29 to 5fe79d9 Compare October 22, 2025 01:12

Establish ODBC tests

da625e0

Co-Authored-By: rscales <[email protected]> Co-Authored-By: justing-bq <[email protected]> Co-Authored-By: alinalibq <[email protected]>

justing-bq force-pushed the startTest branch from 5fe79d9 to da625e0 Compare October 22, 2025 01:23

lidavidm approved these changes Oct 22, 2025

View reviewed changes

lidavidm merged commit 0a0ee3e into apache:main Oct 22, 2025
44 of 46 checks passed

lidavidm removed the awaiting merge Awaiting merge label Oct 22, 2025

lidavidm mentioned this pull request Oct 22, 2025

[C++][FlightSQL] Add tests for arrow::flight::sql::odbc #47786

Closed

	constexpr int ODBC_BUFFER_SIZE = 1024;
	static constexpr int kOdbcBufferSize = 1024;

		#define TEST_CONNECT_STR "ARROW_FLIGHT_SQL_ODBC_CONN"
		#define TEST_DSN "Apache Arrow Flight SQL Test DSN"

GH-47786: [C++][FlightRPC] Establish ODBC tests #47788

GH-47786: [C++][FlightRPC] Establish ODBC tests #47788

Uh oh!

Conversation

justing-bq commented Oct 10, 2025 • edited by github-actions bot 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!

github-actions bot commented Oct 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alinaliBQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

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!

conbench-apache-arrow bot commented Oct 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justing-bq commented Oct 10, 2025 •

edited by github-actions bot

Loading

lidavidm Oct 20, 2025 •

edited

Loading