Replace spdlogs with Arrow Internal Logging#104
Merged
alinaliBQ merged 10 commits intoapache-odbcfrom Sep 23, 2025
Merged
Conversation
Author
|
Closing PR as I found some issues. Will reopen when it is ready for review |
Author
|
Raised apache#47608 for the issues I was running into |
alinaliBQ
commented
Sep 22, 2025
Comment on lines
19
to
23
|
|
||
| #include <functional> | ||
| #include <memory> | ||
| #include <string> | ||
| #include "arrow/util/logging.h" | ||
|
|
||
| #include <spdlog/fmt/fmt.h> | ||
|
|
||
| // The logger using spdlog is deprecated and will be replaced. | ||
| // TODO: mirgate logging to use Arrow's internal logging system | ||
|
|
||
| #define __LAZY_LOG(LEVEL, ...) \ | ||
| do { \ | ||
| driver::odbcabstraction::Logger* logger = \ | ||
| driver::odbcabstraction::Logger::GetInstance(); \ | ||
| if (logger) { \ | ||
| logger->log(driver::odbcabstraction::LogLevel::LogLevel_##LEVEL, \ | ||
| [&]() { return fmt::format(__VA_ARGS__); }); \ | ||
| } \ | ||
| } while (0) | ||
| #define __LAZY_LOG(LEVEL, ...) ARROW_LOG(LEVEL) << __VA_ARGS__; | ||
| #define LOG_DEBUG(...) __LAZY_LOG(DEBUG, __VA_ARGS__) |
Author
There was a problem hiding this comment.
Here is the main code change for this PR. This is where logging is switched to Arrow
Author
|
Let me rework on the Arrow logging usage |
alinaliBQ
force-pushed
the
remove-spd-logs
branch
3 times, most recently
from
7a8324f to
966fbe4
Compare
It is up to Arrow community to enable GLOG on Windows
alinaliBQ
force-pushed
the
remove-spd-logs
branch
from
966fbe4 to
79fa39b
Compare
Resolves errors of kernel function already registered
We can work + test on log file support on macOS/Linux phase
alinaliBQ
commented
Sep 23, 2025
cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/logger.h
Outdated
Show resolved
Hide resolved
* `GetModulePath` can be used to fetch the co-located TLS file
alinaliBQ
commented
Sep 23, 2025
Comment on lines
-85
to
+81
| : false; | ||
| if (!log_enabled.get()) { | ||
| return; | ||
| } | ||
| void FlightSqlDriver::RegisterComputeKernels() { | ||
| auto registry = arrow::compute::GetFunctionRegistry(); | ||
|
|
||
| auto log_path_iterator = propertyMap.find(std::string(SPDLogger::LOG_PATH)); | ||
| auto log_path = log_path_iterator != propertyMap.end() ? log_path_iterator->second : ""; | ||
| if (log_path.empty()) { | ||
| return; | ||
| // strptime is one of the required compute functions | ||
| auto strptime_func = registry->GetFunction("strptime"); | ||
| if (!strptime_func.ok()) { | ||
| // Register Kernel functions to library | ||
| ThrowIfNotOK(arrow::compute::Initialize()); | ||
| } | ||
| } |
Author
There was a problem hiding this comment.
This function resolves the kernel registry issue
justing-bq
approved these changes
Sep 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
spdlogsand use Arrow's internal logs instead. The PR is based on commit: dbt-labs/flightsql-odbc@65554dfSee this file for documentation for enable logging: cpp/src/arrow/flight/sql/odbc/README
Log file path is not supported, because Arrow code disables GLOG on Windows (MSVC) as
Plasma using glog is not fully tested on windows.I found this reason from apache@7104d64