abonander opened a new issue, #4413: URL: https://github.com/apache/arrow-adbc/issues/4413
### What feature or improvement would you like to see? # Problem It's very non-obvious how boolean options are meant to be handled in the ADBC Rust API. There is no specific `OptionValue` variant for booleans, nor are there [defined constants for enabled/disabled](https://docs.rs/adbc_core/0.23.0/adbc_core/constants/index.html) (in the Rust API). I initially assumed it would be `OptionValue::Int(0)` for `false` and `OptionValue::Int(1)` for `true` like is conventional in C APIs, which also maps to "truthiness" semantics in other languages like JavaScript: zero is false, one (or any nonzero value) is true. However, I was surprised to learn that it's actually supposed to be the string values `"true"` and `"false"`, which only became clear after talking to @lidavidm and checking for myself how the Postgres ADBC driver uses it: https://github.com/apache/arrow-adbc/blob/698f2161977c85cd6bcd71dd622046f749fc69ca/c/driver/postgresql/statement.cc#L848 This is not very intuitive or easy to discover, especially with limited examples to go off of. The only other example of an ADBC driver in Rust doesn't actually implement any boolean options: https://github.com/apache/arrow-adbc/blob/698f2161977c85cd6bcd71dd622046f749fc69ca/rust/driver/datafusion/src/lib.rs#L271 This is also means setting or retrieving boolean options requires an allocation every time, because we need to convert to a `String` when creating the `OptionValue`. The FFI layer also has to copy to a new owned `String` because it's unable to borrow from the original string, since its lifetime is only valid for the duration of the call, even though in this case it can only be one of two strings. Most drivers probably don't have to set a whole lot of options anyway, but it's suboptimal. # Solutions ## Option 1: `OptionValue::Bool(bool)` The obvious answer is to add a new variant for `bool`s. They would then have a direct representation in the _Rust_ API, but this would still cause issues: ### Drivers still need to handle string `"true"`/`"false"` values passed by older clients or over FFI. Automagically converting these to `bool`s would be a breaking behavior change because existing drivers would have to update to expect the new variant, but there isn't an accompanying breaking API change to signal this. The `#[non_exhaustive]` attribute on `OptionValue` _requires_ driver authors to provide a wildcard fallback, so driver authors would have to read the CHANGELOG to know that a new variant has been added and that it _needs_ to be handled. In my experience, not everyone reads the CHANGELOG, so this would likely cause unexpected new errors at runtime. ## Option 2: implement boolean conversions Implement `From<bool> for OptionValue` and `TryFrom<&OptionValue> for bool`. This is completely backwards compatible and doesn't change any existing semantics, but makes it more clear how booleans are supposed to be handled, and convenient for implementing options that expect them. For this reason, I favor this approach. This doesn't eliminate the allocations for `"true"` and `"false"` strings, however. One possibility is to change the `String(String)` variant to `String(Cow<'static, str>)` and use `Cow::Borrowed` with static strings. It's arguable whether that's necessary, though. ## Option 3: add booleans to the ADBC C API I think this is the only way that adding `OptionValue::Bool` makes sense, is to add booleans to the C API as well, i.e. adding `AdbcGetOptionBool` and `AdbcSetOptionBool`. This is obviously a much more sweeping, and breaking, change that would affect all languages and drivers, but would eliminate any ambiguities and unnecessary allocations. It may be something to consider for a future revision of the API. -- 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]
