feat: make public headers consumable as C++20 in compat mode by PingLiuPing · Pull Request #936 · apache/iceberg-cpp

PingLiuPing · 2026-09-17T01:13:10Z

Enable C++20 consumers to include and use iceberg-cpp’s installed public headers when the library is built with ICEBERG_CXX20_COMPAT=ON, while keeping iceberg-cpp implementation code and internal workflows on C++23. C++23 remains the default public API.

Restore the vendored iceberg::expected backport — brings back src/iceberg/compat/expected.h from #40 (removed by #139), with its test and LICENSE entry. unexpected's comparison operators now go through error().

cpp20_compatibility_test compiles all public headers as C++20, and the C++20 example links against an installed compatibility package.

Fix #928

zhjwpku · 2026-09-17T02:05:14Z

It seems there are some conflicts, please rebase the main branch.

zhjwpku · 2026-09-17T02:08:39Z


 project(example)

-set(CMAKE_CXX_STANDARD 23)


Can we make C++23 still as default and let it accept user supplied option so that C++20 can be test manually locally.

@zhjwpku Thank you for the comments.

Making it configurable is a good idea. Have different thoughts on making C++23 as default though.

With this patch, it changes the minimum supported C++ standard from C++23 to C++20 for downstream consumers. And C++20 is the interface contract between the consumer and iceberg-cpp library, and the contract should be tested continuously.

Setting the default to C++20 ensures the minimum supported standard (contract) is continuously exercised. Defaulting it to C++23 would let C++20 only breakages slip through.

One refinement is that C++23 compatibility should still be tested separately. C++23 should accepts C++20 code, but we can enhance this by provide an optional example configuration for C++23, for example, expose an ICEBERG_EXAMPLE_CXX_STANDARD cache setting that defaults to 20 and accepts 23; then update CI to build both.
And also refine the document to state clearly that the minimum C++ standard is C++20 for public headers. What do you think?

Happy to make changes either way.

Make sense to me, I think we should build both for compatibility purpose.

@zhjwpku Exposed ICEBERG_EXAMPLE_CXX_STANDARD and updated document accordingly in de7b278.

manuzhang · 2026-09-17T11:04:45Z

@@ -0,0 +1,2401 @@
+/*


Do we have to put this file here? It will be better to be placed in a third_lib directory and have a README to explain its purpose.

@manuzhang Thanks.

Wondering which one you mean: <repo_root>/src/iceberg/third_lib or <repo_root>/third_lib?

I see there's already a thirdparty directory, but I'm not sure that's the best place. @wgtmac should have more background.

I think it's fine to put expected.h here. We already have murmurhash3_internal.h in src/iceberg/util. Similarly, the thrift idls in thirdparty generate code in src/iceberg/catalog/hive/gen-cpp, and those generated files are also tracked in git.

I'm worried about long term maintenance. We may forget what it is, where it comes from and when to drop over time.

Would it help to add a few comments to the file header (after the license) explaining the purpose of this file and providing a brief history?
Or I can update the commit message or separate it to another PR and explain in the PR description.

@manuzhang @zhjwpku Added comments in expected.h. Let's me know if that works for you.
Happy to make further changes.

Work for me, I think the zeus-cpp section in LICENSE should tell people the history of expected.h, but no objection for the comments in expected.h.

wgtmac · 2026-09-23T15:57:34Z

+     ICEBERG_PUBLIC_HEADERS
+     CONFIGURE_DEPENDS
+     "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h"
+     "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp")


nit: we don't have .hpp, do we?

Thanks, checked the source code and no .hpp headers, removed.

wgtmac · 2026-09-24T06:17:28Z

+///
+/// History:
+/// - apache/iceberg-cpp#40 vendored this header, adapted from
+///   https://github.com/zeus-cpp/expected (MIT), while the project targeted


Could we pin the vendored source to a specific upstream tag/commit and record the local delta here? The current history only links the repository, so future audits and upstream syncs will not know which version this 2.4k-line file came from.

Thanks for the suggestion, I cannot find the detail commit ID in #40. My agent compares the code and suggest it is https://github.com/zeus-cpp/expected/releases/tag/v1.2.0. Have a quick check, the timeline match. @zhjwpku not sure if you still remember this.

I think we can pin the latest v1.4.0, no need to stick to v1.2.0.

@zhjwpku Thanks, makes sense, updated to v1.4.0.

wgtmac · 2026-09-24T06:17:33Z

  ErrorCollector(const ErrorCollector&) = default;
  ErrorCollector& operator=(const ErrorCollector&) = default;

+// C++23 uses deducing `this` so that `return AddError(...)` keeps returning the


Both this class and SnapshotUpdate now duplicate the full API and comment blocks across the C++20/C++23 branches. Could we centralize the deducing-this feature test and avoid maintaining two implementations, or at least keep one API shape unless derived-type fluent chaining is a required compatibility guarantee?

Thanks, this is intended actually as I want to limit the scope to the header only.
But agree with you, there is duplication here.

My latest commit go with one API shape: ErrorCollector and SnapshotUpdate now expose plain member functions returning the base type in both C++20 and C++23. This removes the duplicated blocks, and the class definition no longer differs between C++20 and C++23 translation units.

For derived-type fluent chaining: inside the repo, nothing relies on derived-type fluent chaining. However, this is a breaking API change for C++23 users, so I'd like your call on whether it's acceptable here:

AddError(...) now returns ErrorCollector&. A user builder deriving from ErrorCollector that does return AddError(...); in a method returning its own type no longer compiles.
SnapshotUpdate setters now return SnapshotUpdate&, so a derived-class method can no longer be chained after them.

wgtmac · 2026-09-24T06:22:53Z

+    CACHE STRING "C++ standard used to build the example (20 or 23)")
+set_property(CACHE ICEBERG_EXAMPLE_CXX_STANDARD PROPERTY STRINGS 20 23)
+if(NOT ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(20|23)$")
+  message(FATAL_ERROR "ICEBERG_EXAMPLE_CXX_STANDARD must be 20 or 23, got "


Do we really need to specify 20 or 23 here? We need to update this file as well when we support C++26.

Thanks, changed to if(ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(98|11|14|17)$") to keep compatitable with future c++ standard.

wgtmac · 2026-09-24T06:25:14Z

 **Required:**

- C++23 compliant compiler (GCC 14+, Clang 18+, MSVC 2022+)
+- C++23 compliant compiler (GCC 14+, Clang 18+, MSVC 2022+) to build iceberg-cpp itself


It seems better to keep here unchanged to indicate that we officially support C++23. Then we can add a dedicated section below for the contract of C++20 compatibility.

Thanks, reversed.

PingLiuPing · 2026-10-08T09:24:51Z

@wgtmac @zhjwpku gentle ping, thank you!

…expected v1.4.0 Co-authored-by: Junwang Zhao <zhjwpku@gmail.com>

… minimum standard for public headers

wgtmac · 2026-10-09T09:39:30Z

Thanks for the work on this, @PingLiuPing! The PR was already largely working, so I went ahead and pushed a revised version directly to the branch to save a few review round trips.

The main change is that C++23 stays as the default API, while C++20 is now an explicit opt-in mode via ICEBERG_CXX20_COMPAT. This avoids switching public types based on __cplusplus, which could create ODR problems or leave the library and downstream application with different assumptions. The compatibility code is grouped under iceberg/compat; the original C++23 fluent APIs are preserved, while C++20 gets clear fallbacks. I also simplified the tests, examples, docs, and CI setup, and made the build scripts use named parameters instead of long positional ON/OFF lists.

Keep C++23 as the default public API and build mode while adding an opt-in ICEBERG_CXX20_COMPAT package for C++20 consumers. - Add compatibility headers and generated build configuration for compiler detection, byteswap, unreachable, and expected. - Preserve the default C++23 fluent API and provide explicit C++20-compatible fallbacks for SnapshotUpdate and ErrorCollector. - Keep one API shape per package so the library and consumers agree at compile time and avoid ODR issues. - Add and simplify the C++20 compatibility compile/API test. - Cover compatibility mode on Linux, macOS, and Windows while retaining a default C++23 Ubuntu RelWithDebInfo job. - Update package configuration, examples, documentation, and CI helper scripts, including named parameters for build scripts.

zhjwpku · 2026-10-09T10:53:42Z

      return "off";
  }
-  std::unreachable();
+  ::iceberg::unreachable();


Moving the ToStrings from .h to .cc would let us drop iceberg/compat/unreachable.h and use std::unreachable(), but I'm fine with the current approach.

Thanks. I checked the remaining uses: not all of them are in non-template functions. Several public templates, such as those in string_util.h and visit_type.h, still need a C++20-compatible unreachable. Also, LogLevel::ToString is constexpr, so moving it to .cc would remove constant evaluation. I would keep the compat wrapper for now and treat this as a possible later cleanup.

zhjwpku · 2026-10-09T11:05:57Z

  ///
  /// \param reporter The metrics reporter to use.
  /// \return Reference to this for method chaining.
+#if ICEBERG_CXX20_COMPAT


Could we use the C++20 approach instead of the macro with C++23 explicit object parameter? cc @wgtmac

Thanks. Using the C++20 form in C++23 would change these setters to return SnapshotUpdate&, which breaks existing derived fluent calls such as append.ToBranch(...).AppendFile(...). The macro selects a package-level API shape: the default C++23 package keeps deducing this, while the compat package uses base references. I would keep it unless we decide to break the existing C++23 fluent API.

wgtmac · 2026-10-09T11:28:13Z

Thanks @PingLiuPing for the contribution and @zhjwpku @manuzhang for the review! I'll merge this to move forward. Feel free to create followup PRs if you think worth doing.

PingLiuPing mentioned this pull request Sep 17, 2026

Public headers require C++23, forcing the same on every consumer #928

Closed

zhjwpku reviewed Sep 17, 2026

View reviewed changes

PingLiuPing force-pushed the cxx20-public-headers branch from 797da4f to 4a66df4 Compare September 17, 2026 03:48

wgtmac requested a review from manuzhang September 17, 2026 06:50

manuzhang reviewed Sep 17, 2026

View reviewed changes

PingLiuPing force-pushed the cxx20-public-headers branch 3 times, most recently from 044b27b to de7b278 Compare September 23, 2026 14:16

wgtmac reviewed Sep 24, 2026

View reviewed changes

PingLiuPing force-pushed the cxx20-public-headers branch 4 times, most recently from 01d07dd to 31e9943 Compare September 29, 2026 03:40

PingLiuPing and others added 14 commits October 9, 2026 17:35

restore the vendored iceberg::expected backport, updated to zeus-cpp/…

0e6fbd3

…expected v1.4.0 Co-authored-by: Junwang Zhao <zhjwpku@gmail.com>

let iceberg::expected hold a value type with no default constructor

e0e54d3

make Result portable by aliasing it to iceberg::expected

ca1a06a

replace the remaining C++23 library features in public headers

3c4473d

drop deducing this from TaskGroup

4b54836

give ErrorCollector::AddError a C++20 form

92ba9f5

give SnapshotUpdate's chaining setters a C++20 form

f8ffbc2

build the example as C++20

f692d97

Fix pre-commit

e0ee232

Removed invalid e1.error() access on a successful expected<void, int>

c217e89

Fix windows build

a75609f

expose ICEBERG_EXAMPLE_CXX_STANDARD and update docs to state C++20 as…

1e02e7e

… minimum standard for public headers

add C++20 public header compatibility check

dcc4af0

keep a single API shape for ErrorCollector and SnapshotUpdate

7256edf

wgtmac force-pushed the cxx20-public-headers branch from 31e9943 to fbfa2f2 Compare October 9, 2026 09:37

wgtmac changed the title ~~feat: make the public headers consumable as C++20~~ feat: make public headers consumable as C++20 in compat mode Oct 9, 2026

wgtmac force-pushed the cxx20-public-headers branch from fbfa2f2 to 79e341e Compare October 9, 2026 09:49

zhjwpku reviewed Oct 9, 2026

View reviewed changes

wgtmac approved these changes Oct 9, 2026

View reviewed changes

wgtmac merged commit dab8664 into apache:main Oct 9, 2026
19 checks passed

Conversation

PingLiuPing commented Sep 17, 2026 • edited by wgtmac Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhjwpku commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuzhang Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuzhang Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuzhang Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PingLiuPing commented Oct 8, 2026

Uh oh!

wgtmac commented Oct 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Oct 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

PingLiuPing commented Sep 17, 2026 •

edited by wgtmac

Loading

manuzhang Sep 17, 2026 •

edited

Loading

manuzhang Sep 21, 2026 •

edited

Loading

manuzhang Sep 22, 2026 •

edited

Loading