abonander opened a new issue, #4428:
URL: https://github.com/apache/arrow-adbc/issues/4428
### What feature or improvement would you like to see?
In #4413 I lamented that passing booleans through the current API causes a
lot of unnecessary allocations because they have to be converted to `String` to
be passed as `OptionValue::String`.
For the same reason, FFI _must_ unconditionally copy string and binary
option values to a new allocation because the API doesn't allow passing a
borrowed string/slice.
# Backwards-Compatible: Add provided methods to `Optionable`
Add a new provided methods to `Optionable` that take borrowed data:
```rust
pub trait Optionable {
// ...
/// Set an option by key from a borrowed string.
///
/// This should be called when converting to [`OptionValue::String`]
would require copying to a new `String`,
/// as the implementation may be able to avoid it, e.g. if parsing as a
`bool` anyway.
///
/// Implementors are **recommended but not required** to override this
method if they can provide
/// a more efficient implementation.
fn set_option_str(&self, key: Self::Option, value: &str) -> Result<()> {
self.set_option(key, value.into())
}
/// Set an option by key from a borrowed slice.
///
/// This should be called when converting to [`OptionValue::Bytes`]
would require copying to a new `Vec<u8>`,
/// as the implementation may be able to avoid it, e.g. if parsing to
another type anyway.
///
/// Implementors are **recommended but not required** to override this
method if they can provide
/// a more efficient implementation.
fn set_option_bytes(&self, key: Self::Option, value: &[u8]) ->
Result<()> {
self.set_option(key, value.into())
}
}
```
The FFI could trivially be updated to call these methods directly, which
gives implementations the possibility of bypassing the allocation if they don't
need it. It's a backwards-compatible addition because it falls back to the
existing
required method, so implementors don't _need_ to be aware of it.
However, there's another possible source of unnecessary allocations here
that bothers me, which we could eliminate if we allowed a _small_ breaking
change that's unlikely to affect most downstream crates.
# Minimally Breaking: Allow Borrowed Keys in `Optionable`
For driver-specific keys, `OptionDatabase`, `OptionConnection` and
`OptionStatement` all have to convert the key to an owned `String` which is
another source of generally unnecessary allocations, as the key strings are
often going to be hardcoded on both sides of the API.
We can adapt the above approach to also take a borrowed string for the
_key_, so that driver-specific keys don't need to be copied to a new allocation:
```rust
pub trait Optionable {
// ...
fn set_option_str(&self, key: &str, value: &str) -> Result<()>;
fn set_option_bytes(&self, key: &str, value: &str) ->
```
However, this cannot be a provided method, because the `Optionable::Option`
associated type only requires `AsRef<str>`, so there's no generic way for a
provided method to convert `&str` to `Self::Option`.
So the possibilities are to either:
* Add these as provided methods returning `Status::NotImplemented`.
* Backwards-compatible, but very suboptimal because downstream consumers
may not know if these methods are not implemented beforehand, so they risk
encountering errors at runtime or having to check for the error and implement
the fallback themselves.
* Add these as required methods.
* Breaking change for all implementors of `Optionable`, but loudly
signals new methods.
* Add `From<&str>` for `Optionable::Option`.
* Breaking change, but **unlikely** to affect downstream consumers.
The last option is the most interesting, because all the existing option
types _already_ implement `From<&str>`. It's actually only a breaking change
because the `Optionable` trait is not limited to the pre-defined option types,
and _theoretically_ a downstream consumer could be using a completely different
type for `Self::Option`. This seems incredibly unlikely in practice, however,
because there is no reason to do so: the need to implement `Optionable` is
driven by the rest of the API, which also forces the pre-defined `Option*`
types to be used.
In addition to this change, I would add a method like the following to all
the `Option*` types:
```rust
impl OptionDatabase {
/// Parse `key` as one of the pre-defined database options, ignoring
unknown or custom keys.
pub fn try_predefined(key: &str) -> Option<Self> {
match key {
ADBC_OPTION_URI => Some(Self::Uri),
ADBC_OPTION_USERNAME => Some(Self::Username),
ADBC_OPTION_PASSWORD => Some(Self::Password),
_ => None,
}
}
```
This serves as a convenient building block that allows drivers to bypass the
allocation for custom keys.
I chose to return `None` instead of `Error` to avoid the internal
allocations in `Error`, since it's up to the caller to decide whether the key
is actually invalid or not.
--
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]
