GH-46096: [C++][FlightRPC] Environment and Connection Handle Allocation by alinaliBQ · Pull Request #47759 · apache/arrow

alinaliBQ · 2025-10-08T19:42:31Z

Rationale for this change

ODBC driver needs to handle environment handle and connection handle allocation and deallocation.

What changes are included in this PR?

Implement SQLAllocHandle and SQLFreeHandle APIs for allocating and deallocating environment and connection handles.
Tests

Are these changes tested?

Tested locally on Windows MSVC.
Will be tested in CI after GH-46004: [C++][FlightRPC] Enable ODBC Build In C++ Workflows #47689 is merged

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightRPC][ODBC] SQLAllocEnv & SQLFreeEnv implementation #46096
GitHub Issue: [C++] SQLAllocConnect & SQLFreeConnect implementation #46097

github-actions · 2025-10-08T19:43:00Z

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

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

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

lidavidm · 2025-10-08T22:42:53Z

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

+  SQLHENV env;
+
+  // Allocate an environment handle
+  SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);


Should we check the return code? (Should we free the result?)

yup, added check and freed the result

lidavidm · 2025-10-08T22:43:03Z

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

+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+  // ODBC Environment


nit: I don't think these comments add much

remove ODBC environment comment

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

lidavidm · 2025-10-08T22:43:38Z

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

+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+  // ODBC Environment
+  SQLHENV env;


ditto, should we check/free the result?

added calls to free the result

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

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

alinaliBQ

We are working on the comments

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

Add comment from apache#47759 (comment)

alinaliBQ

Addressed all code review comments

alinaliBQ · 2025-10-20T21:57:32Z

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

+  SQLHENV env;
+
+  // Allocate an environment handle
+  SQLAllocHandle(SQL_HANDLE_ENV, SQL_NULL_HANDLE, &env);


yup, added check and freed the result

alinaliBQ · 2025-10-20T21:57:46Z

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

+namespace arrow::flight::sql::odbc {
+
+TEST(SQLAllocHandle, TestSQLAllocHandleEnv) {
+  // ODBC Environment


remove ODBC environment comment

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

alinaliBQ · 2025-10-20T21:58:27Z

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

+
+TEST(SQLAllocEnv, TestSQLAllocEnv) {
+  // ODBC Environment
+  SQLHENV env;


added calls to free the result

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

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

lidavidm

Please rebase!

lidavidm · 2025-10-22T04:58:32Z

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

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


Suggested change

template <typename T>

class ConnectionTest : public T {

public:

using List = std::list<T>;

};

template <typename T>

class ConnectionTest : public T {

};

Rebased and removed using List

* Tests will be added in separate PRs Add tests for SQLAllocEnv and SQLAllocConnect * Other test structure code will be in a separate PR Add comment Add comment from apache#47759 (comment) Work on code review comments Add test fixture for file Co-Authored-By: justing-bq <justin.gossett@improving.com> Add comment for `ConnectionRemoteTest` Add comment for `ReleaseConnection`

address comment

alinaliBQ

Addressed comment. I ran the tests locally and they passed. Marking PR as ready for review. Changes in this PR will be tested in CI after #47689 is merged

alinaliBQ · 2025-10-22T18:30:06Z

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

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


Rebased and removed using List

conbench-apache-arrow · 2025-10-23T06:52:40Z

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

There were no benchmark performance regressions. 🎉

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

…location (apache#47759) ### Rationale for this change ODBC driver needs to handle environment handle and connection handle allocation and deallocation. ### What changes are included in this PR? - Implement SQLAllocHandle and SQLFreeHandle APIs for allocating and deallocating environment and connection handles. - Tests ### Are these changes tested? - Tested locally on Windows MSVC. - Will be tested in CI after apache#47689 is merged ### Are there any user-facing changes? No * GitHub Issue: apache#46096 * GitHub Issue: apache#46097 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>

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

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

cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Outdated Show resolved Hide resolved

This was referenced Oct 10, 2025

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

Open

Address general test code review comments Bit-Quill/arrow#122

Merged

alinaliBQ commented Oct 15, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/odbc_api.cc Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_api.cc Show resolved Hide resolved

alinaliBQ added a commit to Bit-Quill/arrow that referenced this pull request Oct 16, 2025

Add comment

f764198

Add comment from apache#47759 (comment)

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

alinaliBQ added a commit to Bit-Quill/arrow that referenced this pull request Oct 20, 2025

Add comment

50846f5

Add comment from apache#47759 (comment)

alinaliBQ force-pushed the gh-46096-sql-alloc-env branch 2 times, most recently from 85b1b11 to 21ba2a1 Compare October 20, 2025 22:17

alinaliBQ commented Oct 20, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Show resolved Hide resolved

alinaliBQ requested review from kou and lidavidm October 20, 2025 22:18

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

lidavidm reviewed Oct 20, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_api.cc Show resolved Hide resolved

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

alinaliBQ force-pushed the gh-46096-sql-alloc-env branch from abc38d9 to 6d0570a Compare October 21, 2025 18:00

lidavidm reviewed Oct 22, 2025

View reviewed changes

alinaliBQ added 2 commits October 22, 2025 11:22

Remove using List = std::list<T>;

eb37961

address comment

Fix test errors

906f413

alinaliBQ force-pushed the gh-46096-sql-alloc-env branch from 6d0570a to 906f413 Compare October 22, 2025 18:29

alinaliBQ commented Oct 22, 2025

View reviewed changes

alinaliBQ marked this pull request as ready for review October 22, 2025 18:33

alinaliBQ requested a review from lidavidm October 22, 2025 23:09

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

lidavidm merged commit 62afc09 into apache:main Oct 23, 2025
47 of 49 checks passed

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

lidavidm mentioned this pull request Oct 23, 2025

[C++][FlightRPC][ODBC] SQLAllocEnv & SQLFreeEnv implementation #46096

Closed

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!

github-actions bot commented Oct 8, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm 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!

alinaliBQ left a comment

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 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alinaliBQ commented Oct 8, 2025 •

edited

Loading