GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation #47760

alinaliBQ · 2025-10-08T19:49:58Z

Rationale for this change

ODBC driver needs to set and get environment attributes, such as ODBC driver version.

What changes are included in this PR?

Implement SQLGetEnvAttr and SQLSetEnvAttr APIs for retrieving and setting environment attribute values
Tests

Are these changes tested?

Tested locally on Windows MSVC.
Will be tested in CI after #47689 is merged

Are there any user-facing changes?

No

GitHub Issue: [C++] SQLSetEnvAttr & SQLGetEnvAttr implementation #46098

alinaliBQ · 2025-10-08T19:57:09Z

@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR

lidavidm · 2025-10-08T23:19:57Z

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

+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+  // ODBC Environment


nit, but I don't think these comments add anything

lidavidm · 2025-10-08T23:20:20Z

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

+        if (!value_ptr && !str_len_ptr) {
+          throw DriverException("Invalid null pointer for attribute.", "HY000");
+        }


Test this case as well?

Added a test DISABLED_TestSQLGetEnvAttrNullValuePointer for this case. The test case is disabled because call to SQLGetEnvAttr is handled by the driver manager on Windows. And the Windows driver manager doesn't error out when null pointer is passed, even though it should.

Potentially this can be tested on mac/linux depending on the driver manager on these systems

kou · 2025-10-09T05:57:22Z

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

+#ifdef _WIN32
+#  include <windows.h>
+#endif


Should we use cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h instead?

kou · 2025-10-09T06:10:54Z

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

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


Should we use ASSERT_EQ() not EXPECT_EQ()?
Do we need to proceed this test even when this assertion is failed?

fixed to use ASSERT_EQ

kou · 2025-10-09T06:15:08Z

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

+  SQLRETURN return_set =
+      SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(SQL_OV_ODBC2), 0);
+
+  EXPECT_EQ(SQL_SUCCESS, return_set);


Is SQL_SUCCESS expected?

Should we assert set value by SQLGetEnvAttr()?

yes SQL_SUCCESS is expected since the version is supposed to be supported.

Added check for SQLGetEnvAttr

kou · 2025-10-09T06:15:34Z

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

+
+  EXPECT_EQ(SQL_SUCCESS, return_env);
+
+  // Attempt to set to unsupported version


Is this comment correct?

fixed the comment, thanks good catch

kou · 2025-10-09T06:20:48Z

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

  ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
                   << ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
                   << ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);


Can we remove this debug print now?

We generally log the ODBC API calls for debugging so we know which ODBC APIs got passed to the driver, is it ok to keep these logs?

kou · 2025-10-09T06:23:09Z

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

+        if (!value_ptr && !str_len_ptr) {
+          throw DriverException("Invalid null pointer for attribute.", "HY000");
+        }


Should we check this out of this switch?

Probably not in this case. I think attribute needs to be checked first before the pointers, and ODBC needs to error out on attribute value first.
In a case where attribute, value_ptr and str_len_ptr are all invalid, HYC00 (error code for invalid attribute) should be returned first. So we put value_ptr and str_len_ptr check under the switch statement

alinaliBQ

Addressed code review comments

alinaliBQ · 2025-10-20T22:21:36Z

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

  ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
                   << ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
                   << ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);


We generally log the ODBC API calls for debugging so we know which ODBC APIs got passed to the driver, is it ok to keep these logs?

alinaliBQ · 2025-10-20T22:47:04Z

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

+        if (!value_ptr && !str_len_ptr) {
+          throw DriverException("Invalid null pointer for attribute.", "HY000");
+        }


Added a test DISABLED_TestSQLGetEnvAttrNullValuePointer for this case. The test case is disabled because call to SQLGetEnvAttr is handled by the driver manager on Windows. And the Windows driver manager doesn't error out when null pointer is passed, even though it should.

Potentially this can be tested on mac/linux depending on the driver manager on these systems

alinaliBQ · 2025-10-20T22:51:03Z

cpp/src/arrow/flight/sql/odbc/odbc_api.cc

+        if (!value_ptr && !str_len_ptr) {
+          throw DriverException("Invalid null pointer for attribute.", "HY000");
+        }


Probably not in this case. I think attribute needs to be checked first before the pointers, and ODBC needs to error out on attribute value first.
In a case where attribute, value_ptr and str_len_ptr are all invalid, HYC00 (error code for invalid attribute) should be returned first. So we put value_ptr and str_len_ptr check under the switch statement

alinaliBQ · 2025-10-20T22:52:19Z

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

+#ifdef _WIN32
+#  include <windows.h>
+#endif


alinaliBQ · 2025-10-20T22:52:23Z

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

+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+  // ODBC Environment


alinaliBQ · 2025-10-20T22:52:30Z

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

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


fixed to use ASSERT_EQ

alinaliBQ · 2025-10-20T22:53:19Z

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

+
+  EXPECT_EQ(SQL_SUCCESS, return_env);
+
+  // Attempt to set to unsupported version


fixed the comment, thanks good catch

alinaliBQ · 2025-10-20T22:53:54Z

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

+  SQLRETURN return_set =
+      SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(SQL_OV_ODBC2), 0);
+
+  EXPECT_EQ(SQL_SUCCESS, return_set);


yes SQL_SUCCESS is expected since the version is supposed to be supported.

Added check for SQLGetEnvAttr

Co-authored-by: rscales <[email protected]> Add tests for setting and getting environment attributes Address code review comments Co-Authored-By: justing-bq <[email protected]>

alinaliBQ · 2025-10-23T19:30:28Z

Rebased on top of main branch to resolve merge conflicts. This PR is ready for review

conbench-apache-arrow · 2025-10-24T09:22:51Z

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

There were no benchmark performance regressions. 🎉

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

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

alinaliBQ force-pushed the gh-46098-sql-env-attr branch from 8d6f722 to 5da7b4f Compare October 8, 2025 19:56

lidavidm reviewed Oct 8, 2025

View reviewed changes

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

kou reviewed Oct 9, 2025

View reviewed changes

alinaliBQ mentioned this pull request Oct 10, 2025

GH-46734: [C++] Arrow Flight SQL ODBC layer #46099

Open

alinaliBQ force-pushed the gh-46098-sql-env-attr branch from 5da7b4f to 65391da Compare October 20, 2025 23:00

alinaliBQ commented Oct 20, 2025

View reviewed changes

alinaliBQ requested review from kou and lidavidm October 20, 2025 23:00

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

alinaliBQ force-pushed the gh-46098-sql-env-attr branch from 65391da to 1395cff Compare October 22, 2025 18:54

Extract implementation for SQLGetEnvAttr and SQLSetEnvAttr

c4de836

Co-authored-by: rscales <[email protected]> Add tests for setting and getting environment attributes Address code review comments Co-Authored-By: justing-bq <[email protected]>

alinaliBQ force-pushed the gh-46098-sql-env-attr branch from 1395cff to c4de836 Compare October 23, 2025 19:28

alinaliBQ marked this pull request as ready for review October 23, 2025 19:29

alinaliBQ mentioned this pull request Oct 23, 2025

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

Draft

lidavidm approved these changes Oct 24, 2025

View reviewed changes

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

lidavidm merged commit 56e3836 into apache:main Oct 24, 2025
46 of 47 checks passed

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

lidavidm mentioned this pull request Oct 24, 2025

[C++] SQLSetEnvAttr & SQLGetEnvAttr implementation #46098

Closed


		EXPECT_EQ(SQL_SUCCESS, return_env);

		// Attempt to set to unsupported version

Uh oh!

GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation #47760

GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation #47760

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinaliBQ commented Oct 23, 2025

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alinaliBQ commented Oct 8, 2025 •

edited

Loading