GH-47726 : [C++][FlightRPC] ODBC Unicode Support #47771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

lidavidm merged 7 commits into apache:main from Bit-Quill:gh-47726-unicode-support

Oct 16, 2025

Contributor

alinaliBQ commented Oct 9, 2025 •

edited by github-actions bot

Loading

Rationale for this change

Enable ODBC unicode support, so ODBC can handle wide characters.

What changes are included in this PR?

Enable ODBC unicode support by setting -DUNICODE to 1
DSN is read and retrieved in unicode format, and then converted to regular strings for driver
Fix a string reading bug in GetAttributeSQLWCHAR

Are these changes tested?

Build is tested locally

Are there any user-facing changes?

No

GitHub Issue: [C++][Arrow Flight SQL ODBC Driver] Unicode support #47726

alinaliBQ and others added 2 commits

October 9, 2025 10:29


          Unicode support for DSN ODBC APIs

0a88aeb

* Let compiler append `W` to ODBC APIs where applicable.
* pending fixes for changing namespaces


          Fix for GetAttributeSQLWCHAR for truncation error

4c2306a

Co-authored-by: rscales <[email protected]>

Length should be divided by number of characters in a wide string

github-actions bot commented Oct 9, 2025

⚠️ GitHub issue #47726 has been automatically assigned in GitHub to PR creator.

github-actions bot added Component: C++ Component: FlightRPC awaiting review labels


          Fix lint

e8a262c

alinaliBQ marked this pull request as ready for review

October 9, 2025 20:25

alinaliBQ requested a review from lidavidm as a code owner

October 9, 2025 20:25


          Fix variable names

89d72ee

lidavidm reviewed

View reviewed changes

Member

lidavidm left a comment

Would this make future porting to other platforms more difficult?

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.h Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_impl/system_trust_store.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_impl/system_trust_store.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting review awaiting changes and removed awaiting review labels

alinaliBQ mentioned this pull request

GH-47786: [C++][FlightRPC] Establish ODBC tests #47788

Merged


          Address David's comments

004b470

alinaliBQ commented

View reviewed changes

Contributor Author

alinaliBQ left a comment

Addressed David's comments

cpp/src/arrow/flight/sql/odbc/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_impl/system_trust_store.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting review awaiting committer review and removed awaiting review awaiting changes labels

Contributor Author

alinaliBQ commented Oct 10, 2025 •

edited

Loading

Would this make future porting to other platforms more difficult?

@lidavidm Are you referring to porting the unicode support to macOS/Linux platforms? Could you please elaborate on this point?

Member

lidavidm commented Oct 12, 2025

Would this make future porting to other platforms more difficult?

@lidavidm Are you referring to porting the unicode support to macOS/Linux platforms? Could you please elaborate on this point?

Yes, would std::wstring cause issues on Linux/macOS? (I suppose the type is supported, but might be annoying since most OS APIs don't use wchar there...)

lidavidm reviewed

View reviewed changes

Member

lidavidm left a comment

Now that I look at it there's a lot of ValueOr that I ignored earlier...I would expect the errors need to be propagated?

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Outdated

    
                                        const std::string& dflt = "") {

              #define BUFFER_SIZE (1024)

                std::vector<char> buf(BUFFER_SIZE);

                std::wstring wdsn = arrow::util::UTF8ToWideString(dsn).ValueOr(L"");

Member

lidavidm Oct 13, 2025

Why are we ignoring errors here?

I suppose the return type doesn't allow for errors. But I'd expect that we update the return type to Result<std::string>.

Contributor Author

alinaliBQ Oct 15, 2025

You raise a good point, these errors should not be ignored. I have added throws for the Result errors.
The caller will catch the exceptions thrown

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Outdated

    
                return std::string(buf.data(), ret);

                std::wstring wresult = std::wstring(buf.data(), ret);

                std::string result = arrow::util::WideStringToUTF8(wresult).ValueOr("");

Member

lidavidm Oct 13, 2025

Same here.

Contributor Author

alinaliBQ Oct 15, 2025

Same as above comment, I have added throws

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Show resolved Hide resolved

github-actions bot added awaiting review awaiting changes and removed awaiting review awaiting committer review labels


          Address David's comments

1f241a2

Throw error if conversion fails

Remove DSN window

Leave enabling DSN window to apacheGH-46574

alinaliBQ commented

View reviewed changes

Contributor Author

alinaliBQ left a comment

Addressing David's comments

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Outdated

    
                                        const std::string& dflt = "") {

              #define BUFFER_SIZE (1024)

                std::vector<char> buf(BUFFER_SIZE);

                std::wstring wdsn = arrow::util::UTF8ToWideString(dsn).ValueOr(L"");

Contributor Author

alinaliBQ Oct 15, 2025

You raise a good point, these errors should not be ignored. I have added throws for the Result errors.
The caller will catch the exceptions thrown

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Outdated

    
                return std::string(buf.data(), ret);

                std::wstring wresult = std::wstring(buf.data(), ret);

                std::string result = arrow::util::WideStringToUTF8(wresult).ValueOr("");

Contributor Author

alinaliBQ Oct 15, 2025

Same as above comment, I have added throws

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Show resolved Hide resolved

cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc Show resolved Hide resolved

github-actions bot added awaiting review awaiting committer review and removed awaiting review awaiting changes labels

Contributor Author

alinaliBQ commented Oct 15, 2025

Yes, would std::wstring cause issues on Linux/macOS? (I suppose the type is supported, but might be annoying since most OS APIs don't use wchar there...)

I will look into this and get back to you


          add throws

08bf224

Continue adding places where exceptions need to be thrown or caught

alinaliBQ force-pushed the gh-47726-unicode-support branch from b378900 to 08bf224 Compare

October 15, 2025 00:13

lidavidm approved these changes

View reviewed changes

github-actions bot added awaiting merge and removed awaiting committer review labels

Contributor Author

alinaliBQ commented Oct 15, 2025

@lidavidm To follow up on the usage of std::wstring on macOS/Linux, we will be using std::wstring to hold UTF-32 data on Linux/Mac and UTF-16 on Windows. The ODBC uses wchar / std::wstring when communicating with ODBC APIs to support unicode, and the ODBC will be converting to use std::string internally when communicating with any OS API that uses char. I think it should be okay

lidavidm merged commit fc5fd48 into apache:main

44 checks passed

lidavidm removed the awaiting merge label

lidavidm mentioned this pull request

[C++][Arrow Flight SQL ODBC Driver] Unicode support #47726

Closed

conbench-apache-arrow bot commented Oct 16, 2025

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit fc5fd48.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

alinaliBQ deleted the gh-47726-unicode-support branch

October 27, 2025 20:28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: C++ Component: FlightRPC