Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(c/driver/framework): Remove fmt as required dependency of the driver framework #2081

Merged
merged 15 commits into from
Aug 20, 2024
Merged
35 changes: 18 additions & 17 deletions c/driver/framework/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,21 @@ target_include_directories(adbc_driver_framework
"${REPOSITORY_ROOT}/c/vendor")
target_link_libraries(adbc_driver_framework PUBLIC adbc_driver_common fmt::fmt)

# if(ADBC_BUILD_TESTS)
# add_test_case(driver_framework_test
# PREFIX
# adbc
# EXTRA_LABELS
# driver-framework
# SOURCES
# utils_test.cc
# driver_test.cc
# EXTRA_LINK_LIBS
# adbc_driver_framework
# nanoarrow)
# target_compile_features(adbc-driver-framework-test PRIVATE cxx_std_17)
# target_include_directories(adbc-driver-framework-test
# PRIVATE "${REPOSITORY_ROOT}" "${REPOSITORY_ROOT}/c/vendor")
# adbc_configure_target(adbc-driver-framework-test)
# endif()
if(ADBC_BUILD_TESTS)
add_test_case(driver_framework_test
PREFIX
adbc
EXTRA_LABELS
driver-framework
SOURCES
base_driver_test.cc
EXTRA_LINK_LIBS
adbc_driver_framework
nanoarrow)
target_compile_features(adbc-driver-framework-test PRIVATE cxx_std_17)
target_include_directories(adbc-driver-framework-test
PRIVATE "${REPOSITORY_ROOT}/c/"
"${REPOSITORY_ROOT}/c/include"
"${REPOSITORY_ROOT}/c/vendor")
adbc_configure_target(adbc-driver-framework-test)
endif()
102 changes: 25 additions & 77 deletions c/driver/framework/base_connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@
#include <vector>

#include <arrow-adbc/adbc.h>
#include <nanoarrow/nanoarrow.h>
#include <nanoarrow/nanoarrow.hpp>

#include "driver/common/options.h"
#include "driver/common/utils.h"
#include "driver/framework/base_driver.h"
#include "driver/framework/catalog.h"
#include "driver/framework/objects.h"
Expand Down Expand Up @@ -71,8 +67,8 @@ class ConnectionBase : public ObjectBase {
AdbcStatusCode Commit(AdbcError* error) {
switch (autocommit_) {
case AutocommitState::kAutocommit:
return status::InvalidState("{} No active transaction, cannot commit",
Derived::kErrorPrefix)
return status::InvalidState(Derived::kErrorPrefix,
" No active transaction, cannot commit")
.ToAdbc(error);
case AutocommitState::kTransaction:
return impl().CommitImpl().ToAdbc(error);
Expand All @@ -84,36 +80,14 @@ class ConnectionBase : public ObjectBase {
/// \internal
AdbcStatusCode GetInfo(const uint32_t* info_codes, size_t info_codes_length,
ArrowArrayStream* out, AdbcError* error) {
std::vector<uint32_t> codes(info_codes, info_codes + info_codes_length);
RAISE_RESULT(error, auto infos, impl().InfoImpl(codes));

nanoarrow::UniqueSchema schema;
nanoarrow::UniqueArray array;
RAISE_STATUS(error, AdbcInitConnectionGetInfoSchema(schema.get(), array.get()));

for (const auto& info : infos) {
RAISE_STATUS(
error,
std::visit(
[&](auto&& value) -> Status {
using T = std::decay_t<decltype(value)>;
if constexpr (std::is_same_v<T, std::string>) {
return AdbcConnectionGetInfoAppendString(array.get(), info.code, value);
} else if constexpr (std::is_same_v<T, int64_t>) {
return AdbcConnectionGetInfoAppendInt(array.get(), info.code, value);
} else {
static_assert(!sizeof(T), "info value type not implemented");
}
return status::Ok();
},
info.value));
CHECK_NA(INTERNAL, ArrowArrayFinishElement(array.get()), error);
if (!out) {
RAISE_STATUS(error, status::InvalidArgument("out must be non-null"));
}

struct ArrowError na_error = {0};
CHECK_NA_DETAIL(INTERNAL, ArrowArrayFinishBuildingDefault(array.get(), &na_error),
&na_error, error);
return BatchToArrayStream(array.get(), schema.get(), out, error);
std::vector<uint32_t> codes(info_codes, info_codes + info_codes_length);
RAISE_RESULT(error, auto infos, impl().InfoImpl(codes));
RAISE_STATUS(error, AdbcGetInfo(infos, out));
return ADBC_STATUS_OK;
}

/// \internal
Expand Down Expand Up @@ -152,20 +126,17 @@ class ConnectionBase : public ObjectBase {
depth = GetObjectsDepth::kTables;
break;
default:
return status::InvalidArgument("{} GetObjects: invalid depth {}",
Derived::kErrorPrefix, c_depth)
return status::InvalidArgument(Derived::kErrorPrefix,
" GetObjects: invalid depth ", c_depth)
.ToAdbc(error);
}

RAISE_RESULT(error, auto helper, impl().GetObjectsImpl());
nanoarrow::UniqueSchema schema;
nanoarrow::UniqueArray array;
auto status =
BuildGetObjects(helper.get(), depth, catalog_filter, schema_filter, table_filter,
column_filter, table_type_filter, schema.get(), array.get());
auto status = BuildGetObjects(helper.get(), depth, catalog_filter, schema_filter,
table_filter, column_filter, table_type_filter, out);
RAISE_STATUS(error, helper->Close());
RAISE_STATUS(error, status);
return BatchToArrayStream(array.get(), schema.get(), out, error);
return ADBC_STATUS_OK;
}

/// \internal
Expand Down Expand Up @@ -210,8 +181,8 @@ class ConnectionBase : public ObjectBase {
const char* table_name, ArrowSchema* schema,
AdbcError* error) {
if (!table_name) {
return status::InvalidArgument("{} GetTableSchema: must provide table_name",
Derived::kErrorPrefix)
return status::InvalidArgument(Derived::kErrorPrefix,
" GetTableSchema: must provide table_name")
.ToAdbc(error);
}
std::memset(schema, 0, sizeof(*schema));
Expand All @@ -228,36 +199,13 @@ class ConnectionBase : public ObjectBase {

/// \internal
AdbcStatusCode GetTableTypes(ArrowArrayStream* out, AdbcError* error) {
RAISE_RESULT(error, std::vector<std::string> table_types, impl().GetTableTypesImpl());

nanoarrow::UniqueArray array;
nanoarrow::UniqueSchema schema;
ArrowSchemaInit(schema.get());

CHECK_NA(INTERNAL, ArrowSchemaSetType(schema.get(), NANOARROW_TYPE_STRUCT), error);
CHECK_NA(INTERNAL, ArrowSchemaAllocateChildren(schema.get(), /*num_columns=*/1),
error);
ArrowSchemaInit(schema.get()->children[0]);
CHECK_NA(INTERNAL,
ArrowSchemaSetType(schema.get()->children[0], NANOARROW_TYPE_STRING), error);
CHECK_NA(INTERNAL, ArrowSchemaSetName(schema.get()->children[0], "table_type"),
error);
schema.get()->children[0]->flags &= ~ARROW_FLAG_NULLABLE;

CHECK_NA(INTERNAL, ArrowArrayInitFromSchema(array.get(), schema.get(), NULL), error);
CHECK_NA(INTERNAL, ArrowArrayStartAppending(array.get()), error);

for (std::string const& table_type : table_types) {
CHECK_NA(
INTERNAL,
ArrowArrayAppendString(array->children[0], ArrowCharView(table_type.c_str())),
error);
CHECK_NA(INTERNAL, ArrowArrayFinishElement(array.get()), error);
if (!out) {
RAISE_STATUS(error, status::InvalidArgument("out must be non-null"));
}

CHECK_NA(INTERNAL, ArrowArrayFinishBuildingDefault(array.get(), NULL), error);

return BatchToArrayStream(array.get(), schema.get(), out, error);
RAISE_RESULT(error, std::vector<std::string> table_types, impl().GetTableTypesImpl());
RAISE_STATUS(error, AdbcGetTableTypes(table_types, out));
return ADBC_STATUS_OK;
}

/// \internal
Expand All @@ -276,8 +224,8 @@ class ConnectionBase : public ObjectBase {
AdbcStatusCode Rollback(AdbcError* error) {
switch (autocommit_) {
case AutocommitState::kAutocommit:
return status::InvalidState("{} No active transaction, cannot rollback",
Derived::kErrorPrefix)
return status::InvalidState(Derived::kErrorPrefix,
" No active transaction, cannot rollback")
.ToAdbc(error);
case AutocommitState::kTransaction:
return impl().RollbackImpl().ToAdbc(error);
Expand Down Expand Up @@ -352,12 +300,12 @@ class ConnectionBase : public ObjectBase {
}
return status::Ok();
}
return status::NotImplemented("{} Unknown connection option {}={}",
Derived::kErrorPrefix, key, value);
return status::NotImplemented(Derived::kErrorPrefix, " Unknown connection option ",
key, "=", value.Format());
}

Status ToggleAutocommitImpl(bool enable_autocommit) {
return status::NotImplemented("{} Cannot change autocommit", Derived::kErrorPrefix);
return status::NotImplemented(Derived::kErrorPrefix, " Cannot change autocommit");
}

protected:
Expand Down
4 changes: 2 additions & 2 deletions c/driver/framework/base_database.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ class DatabaseBase : public ObjectBase {

/// \brief Set an option. May be called prior to InitImpl.
virtual Status SetOptionImpl(std::string_view key, Option value) {
return status::NotImplemented("{} Unknown database option {}={}",
Derived::kErrorPrefix, key, value);
return status::NotImplemented(Derived::kErrorPrefix, " Unknown database option ", key,
"=", value.Format());
}

private:
Expand Down
32 changes: 25 additions & 7 deletions c/driver/framework/base_driver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Result<bool> Option::AsBool() const {
return false;
}
}
return status::InvalidArgument("Invalid boolean value {}", *this);
return status::InvalidArgument("Invalid boolean value ", this->Format());
},
value_);
}
Expand All @@ -46,15 +46,15 @@ Result<int64_t> Option::AsInt() const {
auto end = value.data() + value.size();
auto result = std::from_chars(begin, end, parsed);
if (result.ec != std::errc()) {
return status::InvalidArgument("Invalid integer value '{}': not an integer",
value);
return status::InvalidArgument("Invalid integer value '", value,
"': not an integer", value);
} else if (result.ptr != end) {
return status::InvalidArgument("Invalid integer value '{}': trailing data",
value);
return status::InvalidArgument("Invalid integer value '", value,
"': trailing data", value);
}
return parsed;
}
return status::InvalidArgument("Invalid integer value {}", *this);
return status::InvalidArgument("Invalid integer value ", this->Format());
},
value_);
}
Expand All @@ -66,7 +66,24 @@ Result<std::string_view> Option::AsString() const {
if constexpr (std::is_same_v<T, std::string>) {
return value;
}
return status::InvalidArgument("Invalid string value {}", *this);
return status::InvalidArgument("Invalid string value {}", this->Format());
},
value_);
}

std::string Option::Format() const {
return std::visit(
[&](auto&& value) {
using T = std::decay_t<decltype(value)>;
if constexpr (std::is_same_v<T, adbc::driver::Option::Unset>) {
return std::string("(NULL)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is the explicit string necessary? (You could declare the lambda to return string, and/or use string_literals?)

} else if constexpr (std::is_same_v<T, std::string>) {
return std::string("'") + value + "'";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return std::string("'") + value + "'";
return (std::stringstream() << std::quoted(value)).str();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the general advice is that stringstream is really slow? (It's also another header...) I'd rather keep the concat

} else if constexpr (std::is_same_v<T, std::vector<uint8_t>>) {
return std::string("(") + std::to_string(value.size()) + " bytes)";
} else {
return std::to_string(value);
}
},
value_);
}
Expand Down Expand Up @@ -157,4 +174,5 @@ AdbcStatusCode Option::CGet(double* out, AdbcError* error) const {
},
value_);
}

} // namespace adbc::driver
29 changes: 3 additions & 26 deletions c/driver/framework/base_driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@
#include <vector>

#include <arrow-adbc/adbc.h>
#include <fmt/core.h>
#include <fmt/format.h>

#include "driver/common/utils.h"
#include "driver/framework/status.h"

/// \file base.h ADBC Driver Framework
Expand Down Expand Up @@ -91,6 +88,9 @@ class Option {
/// \brief Get the value if it is a string.
Result<std::string_view> AsString() const;

/// \brief Provide a human-readable summary of the value
std::string Format() const;

private:
Value value_;

Expand Down Expand Up @@ -623,26 +623,3 @@ class Driver {
};

} // namespace adbc::driver

/// \brief Formatter for Option values.
template <>
struct fmt::formatter<adbc::driver::Option> : fmt::nested_formatter<std::string_view> {
auto format(const adbc::driver::Option& option, fmt::format_context& ctx) const {
return write_padded(ctx, [=](auto out) {
return std::visit(
[&](auto&& value) {
using T = std::decay_t<decltype(value)>;
if constexpr (std::is_same_v<T, adbc::driver::Option::Unset>) {
return fmt::format_to(out, "(NULL)");
} else if constexpr (std::is_same_v<T, std::string>) {
return fmt::format_to(out, "'{}'", value);
} else if constexpr (std::is_same_v<T, std::vector<uint8_t>>) {
return fmt::format_to(out, "({} bytes)", value.size());
} else {
return fmt::format_to(out, "{}", value);
}
},
option.value());
});
}
};
Loading
Loading