Uh oh!
There was an error while loading. Please reload this page.
GH-46574: [C++][FlightRPC] ODBC Driver Connectivity support - #47971
Conversation
Uh oh!
There was an error while loading. Please reload this page.
f28ed1f to
8ad5b6aComparealinaliBQ
commented
Oct 28, 2025
* use suggestion from James for one-liner change to return `Status::OK` directly * fix for iterating endpoints, it is based on JDBC's fix for the same bug * save value of `FlightClientOptions` * Use `TestFlightServer` Add `arrow_flight_sql_shared` to enable usages for `TestFlightServer` * Fix build errors from missing `gmock` * Add checks for array data Update flight_sql_stream_chunk_buffer_test.cc Add `FlightStreamChunkBufferTest` unit test Draft test with `FlightSQLODBCMockEndpointTestBase` * Reference apacheGH-47117 and Clean up code * Add driver and DSN to built-in properties to not pass driver/dsn as attributes to server * Allow `=` in values inside connection strings, fixes connectivity problems * Address comments from Rob
b5159a7 to
fc95da6CompareRebased onto Edit: getting segfault error, looking into it. |
| #include "arrow/flight/sql/odbc/odbc_impl/platform.h" | ||
| #include "arrow/flight/sql/odbc/odbc_impl/config/configuration.h" | ||
| #include "arrow/result.h" |
There was a problem hiding this comment.
nit: if you wanted arrow::Status, just include status.h
| namespace arrow::flight::sql::odbc { | ||
| /// \brief Case insensitive comparator that takes string_view | ||
| struct CaseInsensitiveComparatorStrView { |
There was a problem hiding this comment.
Why is this defined here? It seems to only be used inside the implementation
| // 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; |
There was a problem hiding this 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)
There was a problem hiding this comment.
I've changed it to a transparent comparator and adjusted all the unnecessary std::string() wrapping.
| #include "arrow/flight/sql/odbc/odbc_impl/exceptions.h" | ||
| #include "arrow/flight/sql/odbc/odbc_impl/platform.h" | ||
| namespace ODBC { |
There was a problem hiding this comment.
(maybe for a separate issue but) can we fix this namespace?
There was a problem hiding this comment.
yea we can do it for a separate issue. Are we thinking of replacing ODBC with namespace arrow::flight::sql::odbc?
There was a problem hiding this comment.
I have created issue #48083, @justing-bq please link this issue in the code, thanks!
GH-48083 TODO: replace `namespace ODBC` with `namespace arrow::flight::sql::odbc`
There was a problem hiding this comment.
Added a link to the github issue.
| // 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, |
There was a problem hiding this comment.
string_view can (and should) be passed by value.
| /// \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) { |
There was a problem hiding this comment.
Shouldn't you check length before dereferencing the pointer?
| 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); |
There was a problem hiding this comment.
Would std::optional<std::string> be better?
| /// @return the DSN or an empty string if the DSN is not found or is found after the | ||
| /// driver |
| #if defined _WIN32 | ||
| // For displaying DSN Window | ||
| # include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h" | ||
| #endif |
There was a problem hiding this comment.
| #endif | |
| #endif// defined(_WIN32) |
f8f48a3 to
1c34565Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Will let my teammate Justin help out with the comments cc @justing-bq
| ARROW_FLIGHT_SQL: ON | ||
| ARROW_FLIGHT_SQL_ODBC: ON | ||
| ARROW_FLIGHT_SQL_ODBC: OFF | ||
| ARROW_GANDIVA: ON |
There was a problem hiding this comment.
As discussed in #48019, we will try to enable ODBC on MSVC, so I am removing ODBC build from MinGW here
| #include "arrow/flight/sql/odbc/odbc_impl/exceptions.h" | ||
| #include "arrow/flight/sql/odbc/odbc_impl/platform.h" | ||
| namespace ODBC { |
There was a problem hiding this comment.
I have created issue #48083, @justing-bq please link this issue in the code, thanks!
GH-48083 TODO: replace `namespace ODBC` with `namespace arrow::flight::sql::odbc`
justing-bq
commented
Nov 7, 2025
Please take another look @lidavidm |
Uh oh!
There was an error while loading. Please reload this page.
* SQLDriverConnect requires SQLGetInfo to be implemented for tests to pass, because driver manager requires SQLGetInfo Co-Authored-By: alinalibq <alina.li@improving.com>
Co-authored-by: justing-bq <justin.gossett@improving.com>
Co-authored-by: justing-bq <justin.gossett@improving.com>
Iirc, there is segfault error associated with `SQLGetStmtAttrW` Attempt to fix segfault error by adding impl for `SQLSetConnectAttr` Part-fix remove `using` that isn't needed
std::string_view doesn't work well with std::map because once the std::string_view object is out of scope, the key is no longer valid. The map then has undefined behavior. Enabling transparent comparing only helps with the `find` function, not inserts. For inserts we still need to use a std::string constructor. Ran ODBC tests on MSVC locally and they passed.
a9eae88 to
cfcc956ComparealinaliBQ
commented
Nov 12, 2025
@lidavidm All comments have been addressed, please let me know if you have other feedback, thank you! |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 94b9bb6. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
### Rationale for this change Putting the tests for diagnostics in a separate PR from #47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on #47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: #46575 Co-authored-by: rscales <robscales@icloud.com>
…ache#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 - Test refactoring ### Are these changes tested? Tested locally on MSVC Windows. ### Are there any user-facing changes? N/A * GitHub Issue: apache#46574 Lead-authored-by: Alina (Xi) Li <alina.li@improving.com> Co-authored-by: justing-bq <62349012+justing-bq@users.noreply.github.com> Co-authored-by: justing-bq <justin.gossett@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>
### Rationale for this change Putting the tests for diagnostics in a separate PR from apache#47763, because non-diagnostic ODBC APIs are required to get diagnostics. ### What changes are included in this PR? - Add tests for ODBC diagnostics ### Are these changes tested? PR depends on apache#47971 for tests to work. Tested locally. ### Are there any user-facing changes? No * GitHub Issue: apache#46575 Co-authored-by: rscales <robscales@icloud.com>
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:
Are these changes tested?
Tested locally on MSVC Windows.
Are there any user-facing changes?
N/A