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

alinaliBQ · 2025-09-24T22:22:52Z

Rationale for this change

Replace spdlogs with Arrow's Internal Logging.

What changes are included in this PR?

removed spdlogs fetch and build
replaced odbc logging framework with Arrow's internal logging
Build fixes:
- use C++ 20
- dsn window minor static cast fix

Are these changes tested?

yes, on local Windows environment

Are there any user-facing changes?

No

GitHub Issue: [C++][FlightSQL] Use Arrow's internal logging system for the ODBC #47518

* Add logging README * register kernel function conditionally * Resolves errors of kernel function already registered Logging files are not supported since GLOG is not enabled on Windows in Arrow repo. We can work + test on log file support on macOS/Linux phase * Added minor changes to fix the build

github-actions · 2025-09-24T22:23:15Z

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

alinaliBQ · 2025-09-24T22:24:16Z

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

+# Use C++ 20 for ODBC and its subdirectory
+# GH-44792: Arrow will switch to C++ 20
+set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)


This change is duplicated at #47517. But it is required to fix the build. After this PR is merged, I will remove duplication at #47517

alinaliBQ · 2025-09-24T22:24:24Z

cpp/src/arrow/flight/sql/odbc/flight_sql/ui/dsn_configuration_window.cc

      ChildId::DISABLE_CERT_VERIFICATION_CHECKBOX, disableCertVerification);

-  rowPos += INTERVAL + static_cast<int>(1.5 * ROW_HEIGHT);
+  rowPos += INTERVAL + static_cast<int>(1.5 * static_cast<int>(ROW_HEIGHT));


This change is duplicated at #47517. But it is required to fix the build. After this PR is merged, I will remove duplication at #47517

alinaliBQ · 2025-09-24T22:25:50Z

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc

+  // Enable driver logging. Log files are not supported on Windows yet, since GLOG is not
+  // tested fully on Windows.
+  arrow::util::ArrowLog::StartArrowLog("arrow-flight-sql-odbc", log_level);


Logging documentation is available at #46099 (see here). We plan to add the docs in future PRs

alinaliBQ · 2025-09-24T22:28:46Z

@lidavidm @kou Please have a look at this PR. It addresses code review comment at #47517 (comment). I separated the change in a different PR as the change is big to make it easier to review.

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc

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

alinaliBQ

Addressed comments from Sutou

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc

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

kou · 2025-09-26T02:02:08Z

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc

-  odbcabstraction::Logger::SetInstance(std::move(logger));
+  // Enable driver logging. Log files are not supported on Windows yet, since GLOG is not
+  // tested fully on Windows.
+  arrow::util::ArrowLog::StartArrowLog("arrow-flight-sql-odbc", log_level);


Can we call paired arrow::util::ArrowLog::ShutdownArrowLog()?

Yes. I have added call to it in the FlightSqlDriver destructor. Currently calling ShutdownArrowLog will have no impact as the function shuts down glog, and the driver doesn't support glog logging yet. I think it makes sense to call ShutdownArrowLog since arrow community might make logging changes in the future

This reverts commit 93f095c. The logic is flawed.

The default log level is INFO

kou

+1

kou · 2025-09-27T09:01:24Z

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc

+  // Info log level is enabled by default.
+  if (log_level != ArrowLogLevel::ARROW_INFO) {


We don't want to enable logging only on INFO log level, right?
I think removing this condition (why do we need to disable?) or using log_level <= ArrowLogLevel::ARROW_INFO (disable all not important logs) is better. But we can revisit this later if it's needed.

I agree that let's revisit this later. We do want to enable all kinds of logging. This condition is actually more for tests, not for disabling logging. I found that the enable log command was called multiple times, so I added this condition in hopes to reduce the number of times the logging starts.

INFO logging is enabled in Arrow logging framework by default. I have tested this. So to ensure logging is enabled, we only need to call StartArrowLog if the log level is not INFO.

conbench-apache-arrow · 2025-09-27T15:52:12Z

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

There were no benchmark performance regressions. 🎉

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

…nal Logging (apache#47645) ### Rationale for this change Replace `spdlogs` with Arrow's Internal Logging. ### What changes are included in this PR? - removed `spdlogs` fetch and build - replaced odbc logging framework with Arrow's internal logging - Build fixes: - use C++ 20 - dsn window minor static cast fix ### Are these changes tested? - yes, on local Windows environment ### Are there any user-facing changes? No * GitHub Issue: apache#47518 Lead-authored-by: Alina (Xi) Li <[email protected]> Co-authored-by: Alina (Xi) Li <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

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

alinaliBQ marked this pull request as ready for review September 24, 2025 22:23

alinaliBQ requested a review from lidavidm as a code owner September 24, 2025 22:23

alinaliBQ commented Sep 24, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 24, 2025

alinaliBQ mentioned this pull request Sep 24, 2025

GH-47516: [C++][FlightRPC] Initial ODBC driver framework #47517

Merged

alinaliBQ mentioned this pull request Sep 24, 2025

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

Open

lidavidm approved these changes Sep 25, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 25, 2025

lidavidm requested a review from kou September 25, 2025 07:34

kou reviewed Sep 25, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/CMakeLists.txt Outdated Show resolved Hide resolved

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

Address comments from Kou

74594cf

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

alinaliBQ commented Sep 25, 2025

View reviewed changes

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/CMakeLists.txt Outdated Show resolved Hide resolved

kou reviewed Sep 26, 2025

View reviewed changes

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

Call ShutDownArrowLog

90d0a72

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

alinaliBQ added 3 commits September 26, 2025 16:52

Register log confditionally

93f095c

Revert "Register log confditionally"

116ae2d

This reverts commit 93f095c. The logic is flawed.

Register log if log level is not info

044079b

The default log level is INFO

lidavidm approved these changes Sep 27, 2025

View reviewed changes

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

kou approved these changes Sep 27, 2025

View reviewed changes

kou changed the title ~~GH-47518: [C++][FlightRPC] Replace spdlogs with Arrow's Internal Logging~~ GH-47518: [C++][FlightRPC] Replace spdlogs with Arrow's Internal Logging Sep 27, 2025

kou merged commit 9b96bdb into apache:main Sep 27, 2025
42 of 43 checks passed

kou removed the awaiting merge Awaiting merge label Sep 27, 2025

kou mentioned this pull request Sep 27, 2025

[C++][FlightSQL] Use Arrow's internal logging system for the ODBC #47518

Closed

github-actions bot added the awaiting committer review Awaiting committer review label Sep 29, 2025

alinaliBQ mentioned this pull request Oct 1, 2025

Fix seg fault from register log Bit-Quill/arrow#111

Merged

alinaliBQ deleted the gh-47518-odbc-remove-spd-logs branch October 27, 2025 20:27

		// Info log level is enabled by default.
		if (log_level != ArrowLogLevel::ARROW_INFO) {

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

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

Uh oh!

Conversation

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

Uh oh!

alinaliBQ Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

alinaliBQ Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

alinaliBQ Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

alinaliBQ commented Sep 24, 2025

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!

kou Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

alinaliBQ Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

kou Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

alinaliBQ Sep 29, 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!

alinaliBQ Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Sep 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

alinaliBQ commented Sep 24, 2025 •

edited by github-actions bot

Loading

alinaliBQ Sep 29, 2025 •

edited

Loading