kou commented on code in PR #47645: URL: https://github.com/apache/arrow/pull/47645#discussion_r2378053522
########## cpp/src/arrow/flight/sql/odbc/CMakeLists.txt: ########## @@ -17,5 +17,10 @@ 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) +set(CMAKE_CXX_STANDARD_REQUIRED ON) Review Comment: This is redundant. We already have it: https://github.com/apache/arrow/blob/235032ad245030c6364a9c8ec02066c0aa0bb18d/cpp/cmake_modules/SetupCxxFlags.cmake#L150 ```suggestion ``` ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc: ########## @@ -18,41 +18,45 @@ #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" -#define DEFAULT_MAXIMUM_FILE_SIZE 16777216 -#define CONFIG_FILE_NAME "arrow-odbc.ini" +#define ODBC_LOG_LEVEL "ARROW_ODBC_LOG_LEVEL" -namespace driver { -namespace flight_sql { - -using odbcabstraction::Connection; -using odbcabstraction::LogLevel; -using odbcabstraction::OdbcVersion; -using odbcabstraction::SPDLogger; +using arrow::util::ArrowLogLevel; namespace { -LogLevel ToLogLevel(int64_t level) { +/// Return the corresponding ArrowLogLevel. Debug level is returned by default. +ArrowLogLevel ToLogLevel(int64_t level) { Review Comment: How about using name not number for log level? See also: https://github.com/apache/arrow/blob/235032ad245030c6364a9c8ec02066c0aa0bb18d/cpp/src/arrow/filesystem/s3fs.cc#L3567-L3585 ########## cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_driver.cc: ########## @@ -18,41 +18,45 @@ #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" -#define DEFAULT_MAXIMUM_FILE_SIZE 16777216 -#define CONFIG_FILE_NAME "arrow-odbc.ini" +#define ODBC_LOG_LEVEL "ARROW_ODBC_LOG_LEVEL" Review Comment: Could you use `const char *kODBCLogLevel = "ARROW_ODBC_LOG_LEVEL"` in a namespace? See also: https://google.github.io/styleguide/cppguide.html#Constant_Names -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
