feat: make the public headers consumable as C++20 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 while keeping iceberg-cpp implementation code and internal workflows on C++23.

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

Built and ran public_headers_cxx20 as an external C++20 package consumer; all 199 installed public headers compiled and linked.

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.

Co-authored-by: Junwang Zhao <zhjwpku@gmail.com>

…ctible result types

… minimum standard for public headers

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.

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.

wgtmac · 2026-09-24T06:31:31Z

+     OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/public_headers_check.cc"
+     CONTENT "${ICEBERG_PUBLIC_HEADER_CHECK_SOURCE}")
+
+add_executable(public_headers_check "${CMAKE_CURRENT_BINARY_DIR}/public_headers_check.cc")


IMO, a better alternative is to add a dedicated test executable built with C++20. It takes extra steps to install iceberg libraries and then build the example. We can add a non-installed header file (e.g. src/iceberg/cpp20_compatibility_internal.h) to include all public headers and then use it in the test case. The challenge is to make this header file in sync when we add new header files. We can update AGENTS.md to add this as an advice.

Thanks, added src/iceberg/cpp20_compatibility_internal.h and src/iceberg/test/cpp20_compatibility_test.cc. src/iceberg/cpp20_compatibility_internal.h is generated by a new python file ci/generate_public_headers.py, and this script is used in pre-commit to check that no public headers is missed in cpp20_compatibility_internal.h. It follows the same rule with function(iceberg_install_all_headers PATH) with one diff: dropping .hpp since repo has no .hpp files today.

PingLiuPing mentioned this pull request Sep 17, 2026

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

Open

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 2 times, most recently from 4554b71 to a07e2fa Compare September 21, 2026 09:06

PingLiuPing and others added 14 commits September 23, 2026 21:55

restore the vendored iceberg::expected backport

c01f40f

Co-authored-by: Junwang Zhao <zhjwpku@gmail.com>

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

4e9b719

make Result portable by aliasing it to iceberg::expected

16520aa

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

5ee79e6

drop deducing this from TaskGroup

7b30963

give ErrorCollector::AddError a C++20 form

152ebc4

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

2551261

build the example as C++20

869795d

Fix pre-commit

c3fe6f5

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

8a7fc01

prevents MSVC from trying to construct private or non default constru…

4f9675f

…ctible result types

Fix windows build

1f1da6a

Add comments and history to exptected.h

d005280

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

de7b278

… minimum standard for public headers

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

wgtmac reviewed Sep 24, 2026

View reviewed changes

PingLiuPing added 2 commits September 28, 2026 14:06

add C++20 public header compatibility check

fe43b73

keep a single API shape for ErrorCollector and SnapshotUpdate

9f34259

PingLiuPing force-pushed the cxx20-public-headers branch from d49cfc8 to 9f34259 Compare September 28, 2026 06:08

Conversation

PingLiuPing commented Sep 17, 2026

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

manuzhang Sep 17, 2026 •

edited

Loading

manuzhang Sep 21, 2026 •

edited

Loading

manuzhang Sep 22, 2026 •

edited

Loading