You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alinaliBQ (via GitHub)" <gi...@apache.org> on 2025/10/27 23:32:11 UTC

[PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

alinaliBQ opened a new pull request, #47971:
URL: https://github.com/apache/arrow/pull/47971

   
   ### Rationale for this change
   Add ODBC driver connection support for the driver, so the driver can connect to servers.
   
   ### What changes are included in this PR?
   Implementation of unicode APIs:
   - SQLDriverConnect
   - SQLDisconnect
   - SQLConnect
   - DSN Window
   
   ### Are these changes tested?
   Tested locally on Windows.
   Note that this PR depends on https://github.com/apache/arrow/pull/47763 to get tests passing.
   
   ### Are there any user-facing changes?
   N/A
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2505564959


##########
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:
   Done.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2505572184


##########
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:
   Done.



##########
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:
   Done.



##########
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:
   Done.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47971:
URL: https://github.com/apache/arrow/pull/47971#issuecomment-3458823590

   @lidavidm @kou Changes in this PR are ready for review, please let me know if you have any comments. Thank you! This PR is in draft because it depends on https://github.com/apache/arrow/pull/47763 to get 2 of its tests passing.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47971:
URL: https://github.com/apache/arrow/pull/47971#issuecomment-3532912310

   After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 94b9bb66aae66043b790b69219dde1e0f6d32441.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/55411421437) has more details.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #47971:
URL: https://github.com/apache/arrow/pull/47971


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2504807359


##########
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:
   I have created issue https://github.com/apache/arrow/issues/48083, @justing-bq please link this issue in the code, thanks!
   ```
   GH-48083 TODO: replace `namespace ODBC` with `namespace arrow::flight::sql::odbc`
   ```



##########
.github/workflows/cpp.yml:
##########
@@ -310,7 +310,7 @@ jobs:
       ARROW_DATASET: ON
       ARROW_FLIGHT: ON
       ARROW_FLIGHT_SQL: ON
-      ARROW_FLIGHT_SQL_ODBC: ON
+      ARROW_FLIGHT_SQL_ODBC: OFF
       ARROW_GANDIVA: ON

Review Comment:
   As discussed in https://github.com/apache/arrow/issues/48019, we will try to enable ODBC on MSVC, so I am removing ODBC build from MinGW here



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on PR #47971:
URL: https://github.com/apache/arrow/pull/47971#issuecomment-3505019995

   Please take another look @lidavidm 


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47971:
URL: https://github.com/apache/arrow/pull/47971#issuecomment-3453769455

   :warning: GitHub issue #46574 **has been automatically assigned in GitHub** to PR creator.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2505571508


##########
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:
   I've changed it to a transparent comparator and adjusted all the unnecessary std::string() wrapping.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2505574435


##########
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:
   Added a link to the github issue.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2505566182


##########
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:
   Moved to the .cc file.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2508413354


##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -101,4 +100,19 @@ inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len = SQ
   return std::string(utf8_str.begin(), utf8_str.end());
 }
 
+inline std::string SqlStringToString(const unsigned char* sql_str,
+                                     int32_t sql_str_len = SQL_NTS) {
+  std::string res;
+
+  const char* sql_str_c = reinterpret_cast<const char*>(sql_str);
+
+  if (!sql_str) return res;
+
+  if (sql_str_len == SQL_NTS)
+    res.assign(sql_str_c);
+  else if (sql_str_len > 0)
+    res.assign(sql_str_c, sql_str_len);

Review Comment:
   Ah, nit, but please use braces for this. (I'm surprised none of the linters complained.)



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2485107709


##########
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:
   Yes.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "justing-bq (via GitHub)" <gi...@apache.org>.
justing-bq commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2505573132


##########
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:
   Done.



##########
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:
   Done.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2467480515


##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -144,11 +148,14 @@ std::wstring FlightSQLODBCRemoteTestBase::GetQueryAllDataTypes() {
   return wsql;
 }
 
-void FlightSQLODBCRemoteTestBase::SetUp() {
+void ODBCRemoteTestBase::SetUp() {
   if (arrow::internal::GetEnvVar(kTestConnectStr.data()).ValueOr("").empty()) {
     GTEST_SKIP() << "Skipping test: kTestConnectStr not set";
   }
+}
 
+void FlightSQLODBCRemoteTestBase::SetUp() {
+  ODBCRemoteTestBase::SetUp();
   this->Connect();

Review Comment:
   This change causes test failure because having `GTEST_SKIP` in `SetUp` doesn't cause the subsequent `Connect` call to be skipped. We are looking into it.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47971:
URL: https://github.com/apache/arrow/pull/47971#issuecomment-3463159882

   Rebased onto `master` to get changes from https://github.com/apache/arrow/pull/47763


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2482225152


##########
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:
   yea we can do it for a separate issue. Are we thinking of replacing `ODBC` with `namespace arrow::flight::sql::odbc`?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47971:
URL: https://github.com/apache/arrow/pull/47971#discussion_r2512335262


##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -101,4 +100,19 @@ inline std::string SqlWcharToString(SQLWCHAR* wchar_msg, SQLINTEGER msg_len = SQ
   return std::string(utf8_str.begin(), utf8_str.end());
 }
 
+inline std::string SqlStringToString(const unsigned char* sql_str,
+                                     int32_t sql_str_len = SQL_NTS) {
+  std::string res;
+
+  const char* sql_str_c = reinterpret_cast<const char*>(sql_str);
+
+  if (!sql_str) return res;
+
+  if (sql_str_len == SQL_NTS)
+    res.assign(sql_str_c);
+  else if (sql_str_len > 0)
+    res.assign(sql_str_c, sql_str_len);

Review Comment:
   Sound good, addressed the comment



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support [arrow]

Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47971:
URL: https://github.com/apache/arrow/pull/47971#issuecomment-3524172130

   @lidavidm All comments have been addressed, please let me know if you have other feedback, thank you!
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org