Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 27, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
602fea4
Replace `spdlogs` with Arrow's Internal Logging
alinaliBQ Sep 23, 2025
74594cf
Address comments from Kou
alinaliBQ Sep 25, 2025
90d0a72
Call `ShutDownArrowLog`
alinaliBQ Sep 26, 2025
93f095c
Register log confditionally
alinaliBQ Sep 26, 2025
116ae2d
Revert "Register log confditionally"
alinaliBQ Sep 27, 2025
044079b
Register log if log level is not info
alinaliBQ Sep 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cpp/src/arrow/flight/sql/odbc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,9 @@

add_custom_target(arrow_flight_sql_odbc)

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

add_subdirectory(flight_sql)
add_subdirectory(odbcabstraction)
97 changes: 39 additions & 58 deletions cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,41 +18,31 @@
#include "arrow/flight/sql/odbc/flight_sql/include/flight_sql/flight_sql_driver.h"
#include "arrow/flight/sql/odbc/flight_sql/flight_sql_connection.h"
#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h"
#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spd_logger.h"
#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/utils.h"
#include "arrow/util/io_util.h"
#include "arrow/util/logging.h"
#include "arrow/util/string.h"

#define DEFAULT_MAXIMUM_FILE_SIZE 16777216
#define CONFIG_FILE_NAME "arrow-odbc.ini"
using arrow::util::ArrowLogLevel;

namespace driver {
namespace flight_sql {
static constexpr const char* kODBCLogLevel = "ARROW_ODBC_LOG_LEVEL";

using odbcabstraction::Connection;
using odbcabstraction::LogLevel;
using odbcabstraction::OdbcVersion;
using odbcabstraction::SPDLogger;

namespace {
LogLevel ToLogLevel(int64_t level) {
switch (level) {
case 0:
return LogLevel::LogLevel_TRACE;
case 1:
return LogLevel::LogLevel_DEBUG;
case 2:
return LogLevel::LogLevel_INFO;
case 3:
return LogLevel::LogLevel_WARN;
case 4:
return LogLevel::LogLevel_ERROR;
default:
return LogLevel::LogLevel_OFF;
}
FlightSqlDriver::FlightSqlDriver()
: diagnostics_("Apache Arrow", "Flight SQL", OdbcVersion::V_3), version_("0.9.0.0") {
RegisterLog();
}
} // namespace

FlightSqlDriver::FlightSqlDriver()
: diagnostics_("Apache Arrow", "Flight SQL", OdbcVersion::V_3), version_("0.9.0.0") {}
FlightSqlDriver::~FlightSqlDriver() {
// Unregister log if logging is enabled
if (arrow::internal::GetEnvVar(kODBCLogLevel).ValueOr("").empty()) {
return;
}
arrow::util::ArrowLog::ShutDownArrowLog();
}

std::shared_ptr<Connection> FlightSqlDriver::CreateConnection(OdbcVersion odbc_version) {
return std::make_shared<FlightSqlConnection>(odbc_version, version_);
Expand All @@ -63,45 +53,36 @@ odbcabstraction::Diagnostics& FlightSqlDriver::GetDiagnostics() { return diagnos
void FlightSqlDriver::SetVersion(std::string version) { version_ = std::move(version); }

void FlightSqlDriver::RegisterLog() {
odbcabstraction::PropertyMap propertyMap;
driver::odbcabstraction::ReadConfigFile(propertyMap, CONFIG_FILE_NAME);

auto log_enable_iterator = propertyMap.find(SPDLogger::LOG_ENABLED);
auto log_enabled = log_enable_iterator != propertyMap.end()
? odbcabstraction::AsBool(log_enable_iterator->second)
: false;
if (!log_enabled) {
std::string log_level_str = arrow::internal::GetEnvVar(kODBCLogLevel)
.Map(arrow::internal::AsciiToLower)
.Map(arrow::internal::TrimString)
.ValueOr("");
if (log_level_str.empty()) {
return;
}

auto log_path_iterator = propertyMap.find(SPDLogger::LOG_PATH);
auto log_path = log_path_iterator != propertyMap.end() ? log_path_iterator->second : "";
if (log_path.empty()) {
return;
auto log_level = ArrowLogLevel::ARROW_DEBUG;

if (log_level_str == "fatal") {
log_level = ArrowLogLevel::ARROW_FATAL;
} else if (log_level_str == "error") {
log_level = ArrowLogLevel::ARROW_ERROR;
} else if (log_level_str == "warning") {
log_level = ArrowLogLevel::ARROW_WARNING;
} else if (log_level_str == "info") {
log_level = ArrowLogLevel::ARROW_INFO;
} else if (log_level_str == "debug") {
log_level = ArrowLogLevel::ARROW_DEBUG;
} else if (log_level_str == "trace") {
log_level = ArrowLogLevel::ARROW_TRACE;
}

auto log_level_iterator = propertyMap.find(SPDLogger::LOG_LEVEL);
auto log_level = ToLogLevel(log_level_iterator != propertyMap.end()
? std::stoi(log_level_iterator->second)
: 1);
if (log_level == odbcabstraction::LogLevel_OFF) {
return;
// Enable driver logging. Log files are not supported on Windows yet, since GLOG is not
// tested fully on Windows.
// Info log level is enabled by default.
if (log_level != ArrowLogLevel::ARROW_INFO) {
Comment on lines +82 to +83
Copy link
Member

@kou kou Sep 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alinaliBQ alinaliBQ Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alinaliBQ alinaliBQ Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

arrow::util::ArrowLog::StartArrowLog("arrow-flight-sql-odbc", log_level);
}

auto maximum_file_size_iterator = propertyMap.find(SPDLogger::MAXIMUM_FILE_SIZE);
auto maximum_file_size = maximum_file_size_iterator != propertyMap.end()
? std::stoi(maximum_file_size_iterator->second)
: DEFAULT_MAXIMUM_FILE_SIZE;

auto maximum_file_quantity_iterator = propertyMap.find(SPDLogger::FILE_QUANTITY);
auto maximum_file_quantity = maximum_file_quantity_iterator != propertyMap.end()
? std::stoi(maximum_file_quantity_iterator->second)
: 1;

std::unique_ptr<odbcabstraction::SPDLogger> logger(new odbcabstraction::SPDLogger());

logger->init(maximum_file_quantity, maximum_file_size, log_path, log_level);
odbcabstraction::Logger::SetInstance(std::move(logger));
}

} // namespace flight_sql
Expand Down
1 change: 1 addition & 0 deletions cpp/src/arrow/flight/sql/odbc/flight_sql/include/flight_sql/flight_sql_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class FlightSqlDriver : public odbcabstraction::Driver {

public:
FlightSqlDriver();
~FlightSqlDriver();

std::shared_ptr<odbcabstraction::Connection> CreateConnection(
odbcabstraction::OdbcVersion odbc_version) override;
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/flight/sql/odbc/flight_sql/ui/dsn_configuration_window.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ int DsnConfigurationWindow::CreateEncryptionSettingsGroup(int posX, int posY, in
rightCheckPosX, rowPos - 2, 20, 2 * ROW_HEIGHT, "",
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));
Copy link
Contributor Author

@alinaliBQ alinaliBQ Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


encryptionSettingsGroupBox =
CreateGroupBox(posX, posY, sizeX, rowPos - posY, "Encryption settings",
Expand Down
21 changes: 0 additions & 21 deletions cpp/src/arrow/flight/sql/odbc/odbcabstraction/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ add_library(odbcabstraction
include/odbcabstraction/diagnostics.h
include/odbcabstraction/error_codes.h
include/odbcabstraction/exceptions.h
include/odbcabstraction/logger.h
include/odbcabstraction/platform.h
include/odbcabstraction/spd_logger.h
include/odbcabstraction/types.h
include/odbcabstraction/utils.h
include/odbcabstraction/odbc_impl/attribute_utils.h
Expand All @@ -47,8 +45,6 @@ add_library(odbcabstraction
diagnostics.cc
encoding.cc
exceptions.cc
logger.cc
spd_logger.cc
utils.cc
../../../../vendored/whereami/whereami.cc
odbc_impl/odbc_connection.cc
Expand All @@ -65,20 +61,3 @@ set_target_properties(odbcabstraction
${CMAKE_BINARY_DIR}/$<CONFIG>/lib
RUNTIME_OUTPUT_DIRECTORY
${CMAKE_BINARY_DIR}/$<CONFIG>/lib)

include(FetchContent)
fetchcontent_declare(spdlog
URL https://github.com/gabime/spdlog/archive/76fb40d95455f249bd70824ecfcae7a8f0930fa3.zip
CONFIGURE_COMMAND
""
BUILD_COMMAND
"")
fetchcontent_getproperties(spdlog)
if(NOT spdlog_POPULATED)
fetchcontent_populate(spdlog)
endif()

add_library(spdlog INTERFACE)
target_include_directories(spdlog INTERFACE ${spdlog_SOURCE_DIR}/include)

target_link_libraries(odbcabstraction PUBLIC spdlog)
67 changes: 0 additions & 67 deletions cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h
View file
Open in desktop

This file was deleted.

54 changes: 0 additions & 54 deletions cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spd_logger.h
View file
Open in desktop

This file was deleted.

7 changes: 1 addition & 6 deletions cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,8 @@

#pragma once

#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h>
#include <arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h>
#include <boost/algorithm/string.hpp>
#include <string>
#include "arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/spi/connection.h"

namespace driver {
namespace odbcabstraction {
Expand Down Expand Up @@ -51,8 +49,5 @@ boost::optional<bool> AsBool(const Connection::ConnPropertyMap& connPropertyMap,
boost::optional<int32_t> AsInt32(int32_t min_value,
const Connection::ConnPropertyMap& connPropertyMap,
const std::string_view& property_name);

void ReadConfigFile(PropertyMap& properties, const std::string& configFileName);

} // namespace odbcabstraction
} // namespace driver
32 changes: 0 additions & 32 deletions cpp/src/arrow/flight/sql/odbc/odbcabstraction/logger.cc
View file
Open in desktop

This file was deleted.

Loading
Loading