-
Notifications
You must be signed in to change notification settings - Fork 124
Public headers require C++23, forcing the same on every consumer #928
Description
CMakeLists.txt:35 sets CMAKE_CXX_STANDARD 23, which is a fine choice for the
library's own sources. The requirement is not confined to them, though.
src/iceberg/result.h includes <expected> and <format>, and defines:
template <typename T, typename E = typename DefaultError<T>::type> using Result = std::expected<T, E>; using Status = Result<void>;
Result<T> is the return type of most public entry points —
Catalog::ListNamespaces, ListTables, LoadTable, CreateTable,
StageCreateTable and so on. Every consumer translation unit that calls them
must therefore compile as C++23 as well. std::expected needs libstdc++ 12 or
libc++ 16 and <format> needs libstdc++ 13, so a consumer on an older but still
widely deployed toolchain cannot include the headers at all.
This matters for the engines the library is meant to be embedded in: Velox
builds as C++20, Arrow as C++17, DuckDB as C++11. Integrating leaves two
options — move the whole engine to C++23, or add an isolation layer whose only
purpose is keeping iceberg-cpp headers out of the rest of the build. We are
looking at iceberg-cpp for an Iceberg connector in
Axiom, which builds on Velox at
C++20, and would rather do neither.
Would you consider making the public error type portable while keeping the API
shape unchanged?
#if defined(__cpp_lib_expected) template <typename T, typename E = typename DefaultError<T>::type> using Result = std::expected<T, E>; #else // vendored fallback with the same interface #endif
arrow::Result, absl::StatusOr and tl::expected all exist for this reason.
The library's own sources could keep building as C++23; only the public headers
would need to hold a lower baseline.
Happy to send a patch if the direction is agreeable.
cc @PingLiuPing, who is proposing the Iceberg connector for Axiom.