lidavidm commented on code in PR #3694:
URL: https://github.com/apache/arrow-adbc/pull/3694#discussion_r2520258720
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1691,16 +1691,45 @@ AdbcStatusCode AdbcDatabaseSetOption(struct
AdbcDatabase* database, const char*
TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
if (std::strcmp(key, "driver") == 0) {
std::string_view v{value};
- std::string::size_type pos = v.find("://");
+ std::string::size_type pos = v.find(":");
if (pos != std::string::npos) {
std::string_view d = v.substr(0, pos);
args->driver = std::string{d};
- args->options["uri"] = std::string{v};
+
+ auto next = v.at(pos + 1);
Review Comment:
Won't this throw if `pos == v.len() - 1`?
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
Review Comment:
This doesn't test the :: case
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1691,16 +1691,45 @@ AdbcStatusCode AdbcDatabaseSetOption(struct
AdbcDatabase* database, const char*
TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
if (std::strcmp(key, "driver") == 0) {
std::string_view v{value};
- std::string::size_type pos = v.find("://");
+ std::string::size_type pos = v.find(":");
if (pos != std::string::npos) {
std::string_view d = v.substr(0, pos);
args->driver = std::string{d};
- args->options["uri"] = std::string{v};
+
+ auto next = v.at(pos + 1);
+ if (next == '/' || next == ':') {
+ // uri is like 'driver://...' or 'driver:file::...'
Review Comment:
What kind of `file` URI starts with two colons?
##########
rust/driver_manager/src/lib.rs:
##########
Review Comment:
Same comment here; ideally the URI parsing/mangling would be factored out
and unit tested
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1691,16 +1691,45 @@ AdbcStatusCode AdbcDatabaseSetOption(struct
AdbcDatabase* database, const char*
TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
if (std::strcmp(key, "driver") == 0) {
std::string_view v{value};
- std::string::size_type pos = v.find("://");
+ std::string::size_type pos = v.find(":");
if (pos != std::string::npos) {
std::string_view d = v.substr(0, pos);
args->driver = std::string{d};
- args->options["uri"] = std::string{v};
+
+ auto next = v.at(pos + 1);
+ if (next == '/' || next == ':') {
+ // uri is like 'driver://...' or 'driver:file::...'
Review Comment:
(Should we special case this detection to _only_ file URIs?)
##########
rust/driver_manager/src/lib.rs:
##########
@@ -890,6 +890,63 @@ pub struct ManagedDatabase {
}
impl ManagedDatabase {
+ pub fn from_uri(
+ uri: &str,
+ entrypoint: Option<&[u8]>,
+ version: AdbcVersion,
+ load_flags: LoadFlags,
+ additional_search_paths: Option<Vec<PathBuf>>,
+ ) -> Result<Self> {
+ Self::from_uri_with_opts(
+ uri,
+ entrypoint,
+ version,
+ load_flags,
+ additional_search_paths,
+ std::iter::empty(),
+ )
+ }
+
+ pub fn from_uri_with_opts(
+ uri: &str,
+ entrypoint: Option<&[u8]>,
+ version: AdbcVersion,
+ load_flags: LoadFlags,
+ additional_search_paths: Option<Vec<PathBuf>>,
+ opts: impl IntoIterator<Item = (<Self as Optionable>::Option,
OptionValue)>,
+ ) -> Result<Self> {
+ let idx = uri.find(":");
Review Comment:
Could use `ok_or_else(...)?` to avoid the `is_none` and `unwrap`
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1691,16 +1691,45 @@ AdbcStatusCode AdbcDatabaseSetOption(struct
AdbcDatabase* database, const char*
TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
if (std::strcmp(key, "driver") == 0) {
std::string_view v{value};
- std::string::size_type pos = v.find("://");
+ std::string::size_type pos = v.find(":");
if (pos != std::string::npos) {
std::string_view d = v.substr(0, pos);
args->driver = std::string{d};
- args->options["uri"] = std::string{v};
+
+ auto next = v.at(pos + 1);
Review Comment:
Also I think it might be better to break this out into a helper that parses
out the driver/uri and can be unit-tested directly (something like
`optional<pair<optional<string_view>, optional<string_view>>>` though you may
want to define a helper struct just for convenience)
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1691,16 +1691,45 @@ AdbcStatusCode AdbcDatabaseSetOption(struct
AdbcDatabase* database, const char*
TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
if (std::strcmp(key, "driver") == 0) {
std::string_view v{value};
- std::string::size_type pos = v.find("://");
+ std::string::size_type pos = v.find(":");
if (pos != std::string::npos) {
std::string_view d = v.substr(0, pos);
args->driver = std::string{d};
- args->options["uri"] = std::string{v};
+
+ auto next = v.at(pos + 1);
+ if (next == '/' || next == ':') {
+ // uri is like 'driver://...' or 'driver:file::...'
Review Comment:
Looks like sqlite below. I would much rather special-case `file` URIs.
##########
rust/driver_manager/src/lib.rs:
##########
@@ -890,6 +890,63 @@ pub struct ManagedDatabase {
}
impl ManagedDatabase {
+ pub fn from_uri(
+ uri: &str,
+ entrypoint: Option<&[u8]>,
+ version: AdbcVersion,
+ load_flags: LoadFlags,
+ additional_search_paths: Option<Vec<PathBuf>>,
+ ) -> Result<Self> {
+ Self::from_uri_with_opts(
+ uri,
+ entrypoint,
+ version,
+ load_flags,
+ additional_search_paths,
+ std::iter::empty(),
+ )
+ }
+
+ pub fn from_uri_with_opts(
+ uri: &str,
+ entrypoint: Option<&[u8]>,
+ version: AdbcVersion,
+ load_flags: LoadFlags,
+ additional_search_paths: Option<Vec<PathBuf>>,
+ opts: impl IntoIterator<Item = (<Self as Optionable>::Option,
OptionValue)>,
+ ) -> Result<Self> {
+ let idx = uri.find(":");
+ if idx.is_none() {
+ return Err(Error::with_message_and_status(
+ format!("Invalid URI: {uri}"),
+ Status::InvalidArguments,
+ ));
+ }
+
+ let colon_pos = idx.unwrap();
+ let driver = &uri[..colon_pos];
+ let mut final_uri = uri;
+
+ let next = &uri[colon_pos..colon_pos + 1];
Review Comment:
I think we need to be doing bounds checks before slicing so we don't panic
at runtime
--
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]
