Copilot commented on code in PR #4645:
URL: https://github.com/apache/arrow-adbc/pull/4645#discussion_r3717984095
##########
c/driver/framework/base_driver.h:
##########
@@ -54,8 +54,8 @@ enum class LifecycleState {
kInitialized,
};
-/// \brief A typed option value wrapper. It currently does not attempt
-/// conversion (i.e., getting a double option as a string).
+/// \brief A typed option value wrapper. Numeric values can be retrieved as
strings,
+/// but other conversions are not attempted.
Review Comment:
The updated docstring says "Numeric values can be retrieved as strings, but
other conversions are not attempted." However, this class does perform other
conversions (e.g. `CGet(double*)` accepts `int64_t` and converts via
`static_cast<double>`). Please adjust the comment to reflect the actual
(limited) conversion behavior.
##########
c/driver/framework/base_driver.h:
##########
@@ -169,11 +169,26 @@ class Option {
return std::visit(
[&](auto&& value) -> AdbcStatusCode {
using T = std::decay_t<decltype(value)>;
- if constexpr (std::is_same_v<T, std::string>) {
- size_t value_size_with_terminator = value.size() + 1;
+ if constexpr (std::is_same_v<T, std::string> || std::is_same_v<T,
int64_t> ||
+ std::is_same_v<T, double>) {
+ char formatted[24]; // Enough room for double/int64_t
+ std::string_view string_value;
+ if constexpr (std::is_same_v<T, std::string>) {
+ string_value = value;
+ } else {
+ auto result =
+ std::to_chars(formatted, formatted + sizeof(formatted),
value);
+ if (result.ec != std::errc()) {
+ return status::Internal("Could not format numeric option
value")
+ .ToAdbc(error);
+ }
+ string_value = std::string_view(
+ formatted, static_cast<size_t>(result.ptr - formatted));
+ }
Review Comment:
`Option::CGet(char*, ...)` now formats `double` via
`std::to_chars(formatted, ..., value)`, but the 3-argument `std::to_chars`
overload only exists for integral types; floating-point requires specifying
`std::chars_format`. As written, this will fail to compile (or be non-portable)
when the variant holds a `double`.
Use the floating-point overload for `double` and keep the integral overload
for `int64_t` (optionally with a slightly larger buffer to avoid
`value_too_large`).
--
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]
