lidavidm commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2479899317
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/system_dsn.h:
##########
@@ -19,6 +19,7 @@
#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
#include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h"
+#include "arrow/result.h"
Review Comment:
nit: if you wanted arrow::Status, just include status.h
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h:
##########
@@ -28,6 +28,13 @@
namespace arrow::flight::sql::odbc {
+/// \brief Case insensitive comparator that takes string_view
+struct CaseInsensitiveComparatorStrView {
Review Comment:
Why is this defined here? It seems to only be used inside the implementation
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -40,15 +39,15 @@ using arrow::flight::sql::odbc::WcsToUtf8;
// Return the number of bytes required for the conversion.
template <typename CHAR_TYPE>
-inline size_t ConvertToSqlWChar(const std::string& str, SQLWCHAR* buffer,
+inline size_t ConvertToSqlWChar(const std::string_view& str, SQLWCHAR* buffer,
Review Comment:
string_view can (and should) be passed by value.
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -86,7 +85,7 @@ inline size_t ConvertToSqlWChar(const std::string& str,
SQLWCHAR* buffer,
/// \param[in] msg_len Number of characters in wchar_msg
/// \return wchar_msg in std::string format
inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len =
SQL_NTS) {
- if (msg_len == 0 || !wchar_msg || wchar_msg[0] == 0) {
+ if (!wchar_msg || wchar_msg[0] == 0 || msg_len == 0) {
Review Comment:
Shouldn't you check length before dereferencing the pointer?
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/spi/connection.h:
##########
@@ -32,13 +32,13 @@ namespace arrow::flight::sql::odbc {
/// \brief Case insensitive comparator
struct CaseInsensitiveComparator {
- bool operator()(const std::string_view& s1, const std::string_view& s2)
const {
+ bool operator()(const std::string& s1, const std::string& s2) const {
return boost::lexicographical_compare(s1, s2, boost::is_iless());
}
};
// PropertyMap is case-insensitive for keys.
-typedef std::map<std::string_view, std::string, CaseInsensitiveComparator>
PropertyMap;
+typedef std::map<std::string, std::string, CaseInsensitiveComparator>
PropertyMap;
Review Comment:
With the right definitions, you should be able to avoid wrapping every
lookup in `std::string`. Can we avoid that? (See the note about 'transparent'
comparator in https://en.cppreference.com/w/cpp/container/map/find)
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h:
##########
@@ -75,8 +75,12 @@ class ODBCConnection : public ODBCHandle<ODBCConnection> {
inline bool IsOdbc2Connection() const { return is_2x_connection_; }
- /// @return the DSN or empty string if Driver was used.
- static std::string GetPropertiesFromConnString(
+ /// @return the DSN or an empty string if the DSN is not found or is found
after the
+ /// driver
+ static std::string GetDsnIfExists(const std::string& conn_str);
Review Comment:
Would `std::optional<std::string>` be better?
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -31,6 +31,11 @@
#include "arrow/flight/sql/odbc/odbc_impl/spi/connection.h"
#include "arrow/util/logging.h"
+#if defined _WIN32
+// For displaying DSN Window
+# include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h"
+#endif
Review Comment:
```suggestion
#endif // defined(_WIN32)
```
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h:
##########
@@ -17,16 +17,15 @@
#pragma once
-#include <arrow/flight/sql/odbc/odbc_impl/diagnostics.h>
-#include <arrow/flight/sql/odbc/odbc_impl/exceptions.h>
-#include <arrow/flight/sql/odbc/odbc_impl/platform.h>
#include <sql.h>
#include <sqlext.h>
#include <algorithm>
#include <cstring>
#include <memory>
-
-#include <arrow/flight/sql/odbc/odbc_impl/encoding_utils.h>
+#include "arrow/flight/sql/odbc/odbc_impl/diagnostics.h"
+#include "arrow/flight/sql/odbc/odbc_impl/encoding_utils.h"
+#include "arrow/flight/sql/odbc/odbc_impl/exceptions.h"
+#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
namespace ODBC {
Review Comment:
(maybe for a separate issue but) can we fix this namespace?
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.h:
##########
@@ -75,8 +75,12 @@ class ODBCConnection : public ODBCHandle<ODBCConnection> {
inline bool IsOdbc2Connection() const { return is_2x_connection_; }
- /// @return the DSN or empty string if Driver was used.
- static std::string GetPropertiesFromConnString(
+ /// @return the DSN or an empty string if the DSN is not found or is found
after the
+ /// driver
Review Comment:
We use `\return` not `@return`.
--
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]
