refactor(c/driver/framework): Remove fmt as required dependency of the driver framework #2081
Conversation
| class ViewArrayStream { | ||
| public: | ||
| ViewArrayStream(ArrowArrayStream* stream, ArrowErrorCode* code, ArrowError* error) | ||
| : range_{Next{this, stream, UniqueArray()}}, code_{code}, error_{error} {} |
There was a problem hiding this comment.
A less heavy-handed change could work too, but this was the line that caused a problem when building Python wheels:
D:\a\arrow-adbc\arrow-adbc\adbc\c\vendor\nanoarrow\nanoarrow.hpp(829,21): warning C4355: 'this': used in base member initializer list [D:\a\arrow-adbc\arrow-adbc\adbc\build\driver\framework\adbc_driver_framework.vcxproj]
There was a problem hiding this comment.
That warning is spurious; range_ is not a base member. Should be silenced by being slightly more verbose:
@@ -843,7 +843,10 @@ class ViewArrayAsFixedSizeBytes {
class ViewArrayStream {
public:
ViewArrayStream(ArrowArrayStream* stream, ArrowErrorCode* code, ArrowError* error)
- : range_{Next{this, stream, UniqueArray()}}, code_{code}, error_{error} {}
+ : code_{code}, error_{error} {
+ range_.next.self = this;
+ range_.next.stream = stream;
+ }
ViewArrayStream(ArrowArrayStream* stream, ArrowError* error)
: ViewArrayStream{stream, &internal_code_, error} {}
| if constexpr (std::is_same_v<T, adbc::driver::Option::Unset>) { | ||
| return std::string("(NULL)"); | ||
| } else if constexpr (std::is_same_v<T, std::string>) { | ||
| return std::string("'") + value + "'"; |
There was a problem hiding this comment.
| return std::string("'") + value + "'"; | |
| return (std::stringstream() << std::quoted(value)).str(); |
There was a problem hiding this comment.
I thought the general advice is that stringstream is really slow? (It's also another header...) I'd rather keep the concat
c/driver/framework/base_driver.cc
Outdated
| [&](auto&& value) { | ||
| using T = std::decay_t<decltype(value)>; | ||
| if constexpr (std::is_same_v<T, adbc::driver::Option::Unset>) { | ||
| return std::string("(NULL)"); |
There was a problem hiding this comment.
nit: is the explicit string necessary? (You could declare the lambda to return string, and/or use string_literals?)
| #include <string> | ||
| #include <utility> | ||
| #include <variant> | ||
| #include <vector> | ||
|
|
||
| #include <arrow-adbc/adbc.h> | ||
| #if defined(ADBC_FRAMEWORK_USE_FMT) |
There was a problem hiding this comment.
Are we going to keep this around forever? Or port everything away?
There was a problem hiding this comment.
Keep forever, I think (definitely the way to go for drivers in the ADBC repo). Maybe cleaner as different header instead of embedding it here?
This PR removes fmt as a required component of a driver based on the driver framework. This is so that we can eliminate
c/driver/common/driver_base.hin favour of the much betterc/driver/framework.The fmt library is great and easy to use and we should absolutely use it in our own drivers; however, I don't know of any libraries that would want to export an ADBC driver that don't already have a scheme for error generation (e.g., Arrow's Status or GDAL's
cpl_error.h). In the R package we don't generate any fancy errors but we do need to make a driver to check that the wires are plugged in. I think I've done this in such a way that we keep all the benefits for our own drivers but drop the requirement for all users.This PR leans in to the
Status/Result. I take back everything I ever said about it...it's awesome and we should keep it (closes #1663). I think it will be particularly nice for the consuming end as well if or when we get there.This PR also limits the use of nanoarrow to .cc files (i.e., nanoarrow is not exposed in the headers). This mostly just seemed cleaner, if not strictly necessary (/there are other problems that need solving like namespacing if the internal nanoarrow were to be completely isolated). I also removed references to any headers outside
driver/framework(mostly to the old utility library).This is not quite far enough to do what I need to do in the R package, where I basically just want an empty driver that does nothing. We could fence the bits that rely on the other framework components with
#if !defined(ADBC_FRAMEWORK_MINIMAL)or separatedriver::BaseDatabase(contains noXXXImpl()methods) fromdriver::Database(what the current BaseDatabase is).