GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support #47971

alinaliBQ · 2025-10-27T23:32:09Z

Rationale for this change

Add ODBC driver connection support for the driver, so the driver can connect to servers.

What changes are included in this PR?

Implementation of unicode APIs:

SQLDriverConnect
SQLDisconnect
SQLConnect
DSN Window
Test refactoring

Are these changes tested?

Tested locally on MSVC Windows.

Are there any user-facing changes?

N/A

GitHub Issue: [C++][Arrow Flight SQL ODBC] Develop basic connectivity: SQLDriverConnect, SQLConnect, and SQLDisconnect, and DSN window #46574

github-actions · 2025-10-27T23:32:34Z

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

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

alinaliBQ · 2025-10-28T22:31:55Z

@lidavidm @kou Changes in this PR are ready for review, please let me know if you have any comments. Thank you! This PR is in draft because it depends on #47763 to get 2 of its tests passing.

* use suggestion from James for one-liner change to return `Status::OK` directly * fix for iterating endpoints, it is based on JDBC's fix for the same bug * save value of `FlightClientOptions` * Use `TestFlightServer` Add `arrow_flight_sql_shared` to enable usages for `TestFlightServer` * Fix build errors from missing `gmock` * Add checks for array data Update flight_sql_stream_chunk_buffer_test.cc Add `FlightStreamChunkBufferTest` unit test Draft test with `FlightSQLODBCMockEndpointTestBase` * Reference apacheGH-47117 and Clean up code * Add driver and DSN to built-in properties to not pass driver/dsn as attributes to server * Allow `=` in values inside connection strings, fixes connectivity problems * Address comments from Rob

alinaliBQ · 2025-10-29T18:36:31Z

Rebased onto master to get changes from #47763

Edit: getting segfault error, looking into it.

lidavidm · 2025-10-31T01:09:13Z

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

 #include "arrow/flight/sql/odbc/odbc_impl/platform.h"

 #include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h"
+#include "arrow/result.h"


nit: if you wanted arrow::Status, just include status.h

lidavidm · 2025-10-31T01:10:13Z

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

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

+/// \brief Case insensitive comparator that takes string_view
+struct CaseInsensitiveComparatorStrView {


Why is this defined here? It seems to only be used inside the implementation

Moved to the .cc file.

lidavidm · 2025-10-31T01:13:52Z

cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h


 // PropertyMap is case-insensitive for keys.
-typedef std::map<std::string_view, std::string, CaseInsensitiveComparator> PropertyMap;
+typedef std::map<std::string, std::string, CaseInsensitiveComparator> PropertyMap;


With the right definitions, you should be able to avoid wrapping every lookup in std::string. Can we avoid that? (See the note about 'transparent' comparator in https://en.cppreference.com/w/cpp/container/map/find)

I've changed it to a transparent comparator and adjusted all the unnecessary std::string() wrapping.

lidavidm · 2025-10-31T01:19:12Z

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

+#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h"
+#include "arrow/flight/sql/odbc/odbc_impl/platform.h"

 namespace ODBC {


(maybe for a separate issue but) can we fix this namespace?

yea we can do it for a separate issue. Are we thinking of replacing ODBC with namespace arrow::flight::sql::odbc?

I have created issue #48083, @justing-bq please link this issue in the code, thanks!

GH-48083 TODO: replace `namespace ODBC` with `namespace arrow::flight::sql::odbc`

Added a link to the github issue.

lidavidm · 2025-10-31T01:22:04Z

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

 // Return the number of bytes required for the conversion.
 template <typename CHAR_TYPE>
-inline size_t ConvertToSqlWChar(const std::string& str, SQLWCHAR* buffer,
+inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer,


string_view can (and should) be passed by value.

lidavidm · 2025-10-31T01:22:49Z

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

 /// \return wchar_msg in std::string format
 inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len = SQL_NTS) {
-  if (msg_len == 0 || !wchar_msg || wchar_msg[0] == 0) {
+  if (!wchar_msg || wchar_msg[0] == 0 || msg_len == 0) {


Shouldn't you check length before dereferencing the pointer?

lidavidm · 2025-10-31T01:23:57Z

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

-  static std::string GetPropertiesFromConnString(
+  /// @return the DSN or an empty string if the DSN is not found or is found after the
+  /// driver
+  static std::string GetDsnIfExists(const std::string& conn_str);


Would std::optional<std::string> be better?

lidavidm · 2025-10-31T01:27:28Z

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

+  /// @return the DSN or an empty string if the DSN is not found or is found after the
+  /// driver


We use \return not @return.

lidavidm · 2025-10-31T01:27:57Z

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

+#if defined _WIN32
+// For displaying DSN Window
+#  include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h"
+#endif


Suggested change

#endif

#endif // defined(_WIN32)

alinaliBQ

Will let my teammate Justin help out with the comments cc @justing-bq

alinaliBQ · 2025-11-07T17:50:55Z

.github/workflows/cpp.yml

      ARROW_FLIGHT_SQL: ON
-      ARROW_FLIGHT_SQL_ODBC: ON
+      ARROW_FLIGHT_SQL_ODBC: OFF
      ARROW_GANDIVA: ON


As discussed in #48019, we will try to enable ODBC on MSVC, so I am removing ODBC build from MinGW here

alinaliBQ · 2025-11-07T17:53:09Z

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

+#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h"
+#include "arrow/flight/sql/odbc/odbc_impl/platform.h"

 namespace ODBC {


I have created issue #48083, @justing-bq please link this issue in the code, thanks!

GH-48083 TODO: replace `namespace ODBC` with `namespace arrow::flight::sql::odbc`

justing-bq · 2025-11-07T21:19:17Z

Please take another look @lidavidm

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

* SQLDriverConnect requires SQLGetInfo to be implemented for tests to pass, because driver manager requires SQLGetInfo Co-Authored-By: alinalibq <[email protected]>

Co-authored-by: justing-bq <[email protected]>

Iirc, there is segfault error associated with `SQLGetStmtAttrW` Attempt to fix segfault error by adding impl for `SQLSetConnectAttr` Part-fix remove `using` that isn't needed

See apache#48019

std::string_view doesn't work well with std::map because once the std::string_view object is out of scope, the key is no longer valid. The map then has undefined behavior. Enabling transparent comparing only helps with the `find` function, not inserts. For inserts we still need to use a std::string constructor. Ran ODBC tests on MSVC locally and they passed.

…4 GLib & Ruby>

alinaliBQ · 2025-11-12T22:28:42Z

@lidavidm All comments have been addressed, please let me know if you have other feedback, thank you!

conbench-apache-arrow · 2025-11-14T13:52:31Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

### 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: #46575 Co-authored-by: rscales <[email protected]>

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

alinaliBQ mentioned this pull request Oct 27, 2025

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

Merged

alinaliBQ commented Oct 27, 2025

View reviewed changes

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

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

alinaliBQ force-pushed the gh-46574-driver-connectivity branch 2 times, most recently from f28ed1f to 8ad5b6a Compare October 28, 2025 22:29

alinaliBQ force-pushed the gh-46574-driver-connectivity branch from b5159a7 to fc95da6 Compare October 29, 2025 18:35

alinaliBQ marked this pull request as ready for review October 29, 2025 18:36

alinaliBQ requested a review from lidavidm as a code owner October 29, 2025 18:36

alinaliBQ marked this pull request as draft October 29, 2025 20:42

lidavidm reviewed Oct 31, 2025

View reviewed changes

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

alinaliBQ mentioned this pull request Oct 31, 2025

[C++] Segmentation fault at SparseUnionType destructor when arrow.dll is unloaded via ODBC driver manager #48019

Open

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 5, 2025

alinaliBQ force-pushed the gh-46574-driver-connectivity branch from f8f48a3 to 1c34565 Compare November 6, 2025 18:41

alinaliBQ commented Nov 7, 2025

View reviewed changes

alinaliBQ marked this pull request as ready for review November 7, 2025 17:56

alinaliBQ requested review from assignUser, jonkeane, kou and raulcd as code owners November 7, 2025 17:56

lidavidm approved these changes Nov 10, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h Outdated Show resolved Hide resolved

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Nov 10, 2025

alinaliBQ requested a review from lidavidm November 10, 2025 23:42

alinaliBQ and others added 10 commits November 12, 2025 11:07

Extract SQLDriverConnect implementation

c276d40

* SQLDriverConnect requires SQLGetInfo to be implemented for tests to pass, because driver manager requires SQLGetInfo Co-Authored-By: alinalibq <[email protected]>

Add connection tests and refactoring

6f296a8

Co-authored-by: justing-bq <[email protected]>

Port test fixture changes

6d260ad

Co-authored-by: justing-bq <[email protected]>

Fix connectivity issues from headers

fec523c

Attempt to fix segfault error

3d95eee

Iirc, there is segfault error associated with `SQLGetStmtAttrW` Attempt to fix segfault error by adding impl for `SQLSetConnectAttr` Part-fix remove `using` that isn't needed

Remove ODBC from MinGW build

c23ff57

See apache#48019

Transparent Comparator

2f88ba4

Address remaining feedback

58cb05a

Add braces

e06a5e0

justing-bq force-pushed the gh-46574-driver-connectivity branch from a9eae88 to cfcc956 Compare November 12, 2025 19:33

Empty commit to trigger CI for <C GLib & Ruby / AMD64 Windows MinGW 6…

f37d7e3

…4 GLib & Ruby>

lidavidm approved these changes Nov 14, 2025

View reviewed changes

lidavidm merged commit 94b9bb6 into apache:main Nov 14, 2025
42 of 45 checks passed

lidavidm removed the awaiting merge Awaiting merge label Nov 14, 2025

lidavidm mentioned this pull request Nov 14, 2025

[C++][Arrow Flight SQL ODBC] Develop basic connectivity: SQLDriverConnect, SQLConnect, and SQLDisconnect, and DSN window #46574

Closed

		/// @return the DSN or an empty string if the DSN is not found or is found after the
		/// driver

GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support #47971

GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support #47971

Uh oh!

Conversation

alinaliBQ commented Oct 27, 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 27, 2025

Uh oh!

Uh oh!

alinaliBQ commented Oct 28, 2025

Uh oh!

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

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!

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!

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!

justing-bq commented Nov 7, 2025

Uh oh!

Uh oh!

alinaliBQ commented Nov 12, 2025

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

alinaliBQ commented Oct 27, 2025 •

edited

Loading

alinaliBQ commented Oct 29, 2025 •

edited

Loading