GH-47516: [C++][FlightRPC] Initial ODBC driver framework by alinaliBQ · Pull Request #47517 · apache/arrow

alinaliBQ · 2025-09-05T19:30:49Z

Rationale for this change

Add initial framework for building an ODBC driver dll

What changes are included in this PR?

initial Flight SQL ODBC framework

Are these changes tested?

Yes, on local Windows environment

Are there any user-facing changes?

None

PR is extracted from PR GH-46734: [C++] Arrow Flight SQL ODBC layer #46099
GitHub Issue: [C++][FlightSQL] Initial ODBC framework #47516

github-actions · 2025-09-05T19:31:12Z

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

alinaliBQ · 2025-09-05T21:37:44Z

@lidavidm @kou Please review this PR, it is extracted from #46099
Note: I don't think I have access to add anyone as reviewer to the PR, looks like the reviewers are automatically added?

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

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

kou · 2025-09-07T08:33:44Z

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

 add_custom_target(arrow_flight_sql_odbc)

+# Ensure fmt is loaded as header only
+add_compile_definitions(FMT_HEADER_ONLY)


Could you use target_compile_definitions() because add_compile_definitions()'s scope is directory? We should not use directory scope commands such as add_compile_definitions() and include_directories() for newly written code.

Sure, I changed to use target_compile_definitions

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

kou · 2025-09-07T08:37:41Z

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

+include(FetchContent)
+fetchcontent_declare(spdlog
+                     URL https://github.com/gabime/spdlog/archive/refs/tags/v1.15.3.zip
+                         CONFIGURE_COMMAND
+                         ""
+                         BUILD_COMMAND
+                         "")
+fetchcontent_makeavailable(spdlog)


Can we use arrow/util/logging.h instead?

Yes. Let me look into this

@kou I have raised PR: #47645 for this change

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

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

kou · 2025-09-07T08:44:04Z

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

+SQLRETURN SQL_API SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) {
+  return SQL_INVALID_HANDLE;
+}


Do we need both of the top-level SQLAllocHandle() and arrow::SQLAllocHandle()?

Yes. In upcoming PRs, we plan to change it to look like:

SQLRETURN SQL_API SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) { return arrow::flight::sql::odbc::SQLAllocHandle(...); }

I have separate the actual implementation of SQLAllocHandle to make this PR smaller

alinaliBQ

Checked all comments. Let me look into replacing spdlogs with arrow/util/logging.h

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

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

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

alinaliBQ · 2025-09-15T20:51:48Z

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

 add_custom_target(arrow_flight_sql_odbc)

+# Ensure fmt is loaded as header only
+add_compile_definitions(FMT_HEADER_ONLY)


Sure, I changed to use target_compile_definitions

alinaliBQ · 2025-09-15T21:46:57Z

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

+include(FetchContent)
+fetchcontent_declare(spdlog
+                     URL https://github.com/gabime/spdlog/archive/refs/tags/v1.15.3.zip
+                         CONFIGURE_COMMAND
+                         ""
+                         BUILD_COMMAND
+                         "")
+fetchcontent_makeavailable(spdlog)


Yes. Let me look into this

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

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

alinaliBQ · 2025-09-15T21:55:48Z

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

+SQLRETURN SQL_API SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) {
+  return SQL_INVALID_HANDLE;
+}


Yes. In upcoming PRs, we plan to change it to look like:

SQLRETURN SQL_API SQLAllocHandle(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* result) { return arrow::flight::sql::odbc::SQLAllocHandle(...); }

I have separate the actual implementation of SQLAllocHandle to make this PR smaller

cpp/submodules/parquet-testing

Create odbc.def Remove SQLAllocHandle definitions In-progress - fix build errors In progress fix Remove unneeded code Fix build issues Continue build fixes Fix build issues Address review comments Follow-up changes Fix Lint issues

kou

+1

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

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

kou · 2025-09-30T01:49:48Z

cpp/src/arrow/flight/sql/odbc/odbc_api_internal.h

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


Should we use #include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h" instead? It seems that platform.h defines some macros before including windows.h.

Sure, that's reasonable to use. I fixed it

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

alinaliBQ

Addressed comments from @kou

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

alinaliBQ · 2025-10-01T18:42:48Z

cpp/src/arrow/flight/sql/odbc/odbc_api_internal.h

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


Sure, that's reasonable to use. I fixed it

conbench-apache-arrow · 2025-10-02T04:37:06Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…e#47517) ### Rationale for this change Add initial framework for building an ODBC driver dll ### What changes are included in this PR? - initial Flight SQL ODBC framework ### Are these changes tested? - Yes, on local Windows environment ### Are there any user-facing changes? - None * PR is extracted from PR apache#46099 * GitHub Issue: apache#47516 Authored-by: Alina (Xi) Li <alina.li@improving.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

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

alinaliBQ marked this pull request as ready for review September 5, 2025 21:36

alinaliBQ requested a review from lidavidm as a code owner September 5, 2025 21:36

alinaliBQ mentioned this pull request Sep 5, 2025

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

Open

lidavidm reviewed Sep 6, 2025

View reviewed changes

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

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

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

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

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 6, 2025

kou reviewed Sep 7, 2025

View reviewed changes

alinaliBQ commented Sep 15, 2025

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Sep 15, 2025

alinaliBQ mentioned this pull request Sep 24, 2025

GH-47518: [C++][FlightRPC] Replace spdlogs with Arrow's Internal Logging #47645

Merged

lidavidm approved these changes Sep 27, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Sep 27, 2025

alinaliBQ force-pushed the flight-sql-odbc-first-pr branch 2 times, most recently from f3f3c59 to 587050a Compare September 29, 2025 20:44

alinaliBQ commented Sep 29, 2025

View reviewed changes

cpp/submodules/parquet-testing Show resolved Hide resolved

alinaliBQ force-pushed the flight-sql-odbc-first-pr branch from 587050a to 677d1f1 Compare September 29, 2025 21:02

ODBC initial framework

478dc97

Create odbc.def Remove SQLAllocHandle definitions In-progress - fix build errors In progress fix Remove unneeded code Fix build issues Continue build fixes Fix build issues Address review comments Follow-up changes Fix Lint issues

alinaliBQ force-pushed the flight-sql-odbc-first-pr branch from 677d1f1 to 478dc97 Compare September 29, 2025 21:03

alinaliBQ requested a review from kou September 29, 2025 21:03

kou approved these changes Sep 30, 2025

View reviewed changes

Address comments from Sutou

cf0f39c

alinaliBQ commented Oct 1, 2025

View reviewed changes

kou merged commit 16ceade into apache:main Oct 1, 2025
41 checks passed

kou removed the awaiting merge Awaiting merge label Oct 1, 2025

kou mentioned this pull request Oct 1, 2025

[C++][FlightSQL] Initial ODBC framework #47516

Closed

Conversation

alinaliBQ commented Sep 5, 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 Sep 5, 2025

Uh oh!

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

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!

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

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!

alinaliBQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Oct 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alinaliBQ commented Sep 5, 2025 •

edited

Loading

alinaliBQ commented Sep 5, 2025 •

edited

Loading