You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "amoeba (via GitHub)" <gi...@apache.org> on 2026/02/18 02:23:32 UTC
[PR] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
amoeba opened a new pull request, #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006
This is just a draft to verify my test fails on CI like it does locally. Will update PR body before merge.
Ref https://github.com/apache/arrow-adbc/issues/3970
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2825614516
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
The problem is that paths aren't UTF-8, so decoding it seems very wrong.
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2825636920
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
Is there a codepath I'm not seeing where we use a `Path` object? I'm assuming that's what you're referring to when you say paths are bytestrings. All I see are `str` involved in the code paths here and I guess I'm assuming those turn into UTF-8 encoded `const char*` on the C side... I'll look again tomorrow
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2825616044
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
Paths should be bytestrings (with some restrictions), so if Python provided UTF-8 that we have to decode into the native codepage, then presumably Python should've provided the raw bytestring in the first place
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2825559653
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
For some reason I wasn't able to breakpoint GetDriverInfo so when I laboriously stepped my way into GetDriverInfo, I can see the code in the patch gives me what I expect. Notice `driver_name` is the garbled form and `driver_path` is correct:
<img width="501" height="467" alt="image" src="https://github.com/user-attachments/assets/714cd438-bc21-41fb-8f0a-31887cb3bf11" />
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2820007793
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
So the issue is that Python is treating the path as a string when it shouldn't?
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2825645081
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
Talking about the Python side. I'm assuming this should not be converting to str, but should be using bytes?
https://github.com/apache/arrow-adbc/blob/7b38cf4543330592a40a5023b4e6b93f8f34d7ff/python/adbc_driver_sqlite/adbc_driver_sqlite/__init__.py#L63-L93
--
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] fix(c): handle non-ASCII driver paths in GetDriverInfo on Windows [arrow-adbc]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #4006:
URL: https://github.com/apache/arrow-adbc/pull/4006#discussion_r2825550475
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -797,6 +797,48 @@ TEST_F(DriverManifest, LoadNonAsciiPath) {
UnsetConfigPath();
}
+// Test loading a driver DLL with a path that contains non-ASCII chars
+// See https://github.com/apache/arrow-adbc/issues/3970
+TEST_F(DriverManifest, LoadDriverDllFromNonAsciiPath) {
+ // Create a directory with original characters from the issue (项目 = "project")
+#ifdef _WIN32
+ std::filesystem::path non_ascii_dir = temp_dir / L"\u9879\u76ee";
+#else
+ std::filesystem::path non_ascii_dir = temp_dir / "\u9879\u76ee";
+#endif
+ std::filesytem::create_directories(non_ascii_dir);
+
+ // Create a dummy driver DLL, we'll just check th error message later to confirm
+ std::filesystem::path test_file = non_ascii_dir / "dummy.dll";
+ std::ofstream(test_file) << "not a real DLL";
+
+ // Create a UTF-8 encoded string to simulate what Python or another caller would
+ // pass
Review Comment:
I'm not sure it's that.
I initially started out Claude Code on it and it zero'd in on,
```patch
diff --git a/c/driver_manager/adbc_driver_manager.cc b/c/driver_manager/adbc_driver_manager.cc
index 15d98c00b..5bb4a34d1 100644
--- a/c/driver_manager/adbc_driver_manager.cc
+++ b/c/driver_manager/adbc_driver_manager.cc
@@ -704,7 +704,12 @@ struct ManagedLibrary {
}
// First try to treat the given driver name as a path to a manifest or shared library
+#ifdef _WIN32
+ // On Windows, decode UTF-8 to wide string to properly handle non-ASCII paths
+ std::filesystem::path driver_path(Utf8Decode(std::string(driver_name)));
+#else
std::filesystem::path driver_path(driver_name);
+#endif
const bool allow_relative_paths = load_options & ADBC_LOAD_FLAG_ALLOW_RELATIVE_PATHS;
if (driver_path.has_extension()) {
if (driver_path.is_relative() && !allow_relative_paths) {
```
So I started to see if I could prove it. I looked at the sequence of calls that leads there from Python and it looks like we pass a Python str() and treat it as a C string in the C++ side. I also built adbc_driver_manager with debug symbols and when I run,
```python
>>> dbapi.connect(driver="项目\some.dll")
```
and stop on a breakpoint in the C++ driver manager's AdbcDatabaseInit, I see,
<img width="334" height="218" alt="image" src="https://github.com/user-attachments/assets/f5f78a70-65d6-403a-8fc9-cb536b348ec0" />
--
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