You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "PingLiuPing (via GitHub)" <gi...@apache.org> on 2026/09/17 01:13:13 UTC
[PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
PingLiuPing opened a new pull request, #936:
URL: https://github.com/apache/iceberg-cpp/pull/936
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](https://github.com/PingLiuPing/axiom/issues/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4079533960
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4122976410
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2443 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2024 zeus-cpp
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+/// \file iceberg/expected.h
+/// \brief A C++20 backport of C++23 `std::expected`, used as the storage type
+/// behind `iceberg::Result`.
+///
+/// Purpose: the library itself may be built as C++23, but its public headers
+/// must stay consumable from C++20 translation units. `std::expected` is a
+/// C++23 library feature, so `iceberg::Result<T>` (see iceberg/result.h) is
+/// defined in terms of `iceberg::expected` instead. The API mirrors
+/// `std::expected` (including the monadic `and_then`, `or_else`, `transform`
+/// and `transform_error`), so code can be written as if against the standard
+/// type.
+///
+/// History:
+/// - apache/iceberg-cpp#40 vendored this header, adapted from
+/// https://github.com/zeus-cpp/expected (MIT), while the project targeted
Review Comment:
I think we can pin the latest v1.4.0, no need to stick to v1.2.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119237546
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
+# C++20 is the minimum standard iceberg-cpp's public headers support, so the
+# example builds as C++20 by default to keep that contract exercised. Set this to
+# 23 to also check the headers from a C++23 consumer.
+set(ICEBERG_EXAMPLE_CXX_STANDARD
+ 20
+ 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 "
Review Comment:
Thanks, changed to `if(ICEBERG_EXAMPLE_CXX_STANDARD MATCHES "^(98|11|14|17)$")` to keep compatitable with future c++ standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4033502467
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,65 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#issuecomment-5707328972
It seems there are some conflicts, please rebase the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119093498
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2443 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2024 zeus-cpp
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+/// \file iceberg/expected.h
+/// \brief A C++20 backport of C++23 `std::expected`, used as the storage type
+/// behind `iceberg::Result`.
+///
+/// Purpose: the library itself may be built as C++23, but its public headers
+/// must stay consumable from C++20 translation units. `std::expected` is a
+/// C++23 library feature, so `iceberg::Result<T>` (see iceberg/result.h) is
+/// defined in terms of `iceberg::expected` instead. The API mirrors
+/// `std::expected` (including the monadic `and_then`, `or_else`, `transform`
+/// and `transform_error`), so code can be written as if against the standard
+/// type.
+///
+/// History:
+/// - apache/iceberg-cpp#40 vendored this header, adapted from
+/// https://github.com/zeus-cpp/expected (MIT), while the project targeted
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4084526929
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,65 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
Review Comment:
@zhjwpku Exposed `ICEBERG_EXAMPLE_CXX_STANDARD` and updated document accordingly in de7b278ec124f61defb6fca39fc8e2bfdf6be339.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4090576050
##########
mkdocs/docs/getting-started.md:
##########
@@ -23,10 +23,14 @@
**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
Review Comment:
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.
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2443 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2024 zeus-cpp
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+/// \file iceberg/expected.h
+/// \brief A C++20 backport of C++23 `std::expected`, used as the storage type
+/// behind `iceberg::Result`.
+///
+/// Purpose: the library itself may be built as C++23, but its public headers
+/// must stay consumable from C++20 translation units. `std::expected` is a
+/// C++23 library feature, so `iceberg::Result<T>` (see iceberg/result.h) is
+/// defined in terms of `iceberg::expected` instead. The API mirrors
+/// `std::expected` (including the monadic `and_then`, `or_else`, `transform`
+/// and `transform_error`), so code can be written as if against the standard
+/// type.
+///
+/// History:
+/// - apache/iceberg-cpp#40 vendored this header, adapted from
+/// https://github.com/zeus-cpp/expected (MIT), while the project targeted
Review Comment:
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.
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
+# C++20 is the minimum standard iceberg-cpp's public headers support, so the
+# example builds as C++20 by default to keep that contract exercised. Set this to
+# 23 to also check the headers from a C++23 consumer.
+set(ICEBERG_EXAMPLE_CXX_STANDARD
+ 20
+ 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 "
+ "'${ICEBERG_EXAMPLE_CXX_STANDARD}'")
+endif()
+
+set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD})
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_CXX_EXTENSIONS OFF)
find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest)
+if(TARGET iceberg::iceberg_bundle_shared)
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared)
+else()
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static)
+endif()
+
+if(TARGET iceberg::iceberg_rest_shared)
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared)
+else()
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static)
+endif()
+
add_executable(demo_example demo_example.cc)
-target_link_libraries(demo_example
- PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>"
- "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>"
-)
+target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET}
+ ${ICEBERG_REST_TARGET})
+
+# Compile every installed public header as a consumer using
+# ICEBERG_EXAMPLE_CXX_STANDARD. The installed include
+# tree is the public API contract: iceberg_install_all_headers excludes internal
+# headers before packaging them.
+get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET}
+ INTERFACE_INCLUDE_DIRECTORIES)
+foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS)
+ if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg")
+ set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}")
+ break()
+ endif()
+endforeach()
+
+if(NOT ICEBERG_PUBLIC_INCLUDE_DIR)
+ message(FATAL_ERROR "Could not locate iceberg's installed public headers")
+endif()
+
+file(GLOB_RECURSE
+ ICEBERG_PUBLIC_HEADERS
+ CONFIGURE_DEPENDS
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h"
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp")
Review Comment:
nit: we don't have `.hpp`, do we?
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
+# C++20 is the minimum standard iceberg-cpp's public headers support, so the
+# example builds as C++20 by default to keep that contract exercised. Set this to
+# 23 to also check the headers from a C++23 consumer.
+set(ICEBERG_EXAMPLE_CXX_STANDARD
+ 20
+ 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 "
Review Comment:
Do we really need to specify 20 or 23 here? We need to update this file as well when we support C++26.
##########
src/iceberg/util/error_collector.h:
##########
@@ -101,6 +104,13 @@ class ICEBERG_EXPORT ErrorCollector {
ErrorCollector(const ErrorCollector&) = default;
ErrorCollector& operator=(const ErrorCollector&) = default;
+// C++23 uses deducing `this` so that `return AddError(...)` keeps returning the
Review Comment:
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?
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
+# C++20 is the minimum standard iceberg-cpp's public headers support, so the
+# example builds as C++20 by default to keep that contract exercised. Set this to
+# 23 to also check the headers from a C++23 consumer.
+set(ICEBERG_EXAMPLE_CXX_STANDARD
+ 20
+ 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 "
+ "'${ICEBERG_EXAMPLE_CXX_STANDARD}'")
+endif()
+
+set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD})
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_CXX_EXTENSIONS OFF)
find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest)
+if(TARGET iceberg::iceberg_bundle_shared)
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared)
+else()
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static)
+endif()
+
+if(TARGET iceberg::iceberg_rest_shared)
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared)
+else()
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static)
+endif()
+
add_executable(demo_example demo_example.cc)
-target_link_libraries(demo_example
- PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>"
- "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>"
-)
+target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET}
+ ${ICEBERG_REST_TARGET})
+
+# Compile every installed public header as a consumer using
+# ICEBERG_EXAMPLE_CXX_STANDARD. The installed include
+# tree is the public API contract: iceberg_install_all_headers excludes internal
+# headers before packaging them.
+get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET}
+ INTERFACE_INCLUDE_DIRECTORIES)
+foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS)
+ if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg")
+ set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}")
+ break()
+ endif()
+endforeach()
+
+if(NOT ICEBERG_PUBLIC_INCLUDE_DIR)
+ message(FATAL_ERROR "Could not locate iceberg's installed public headers")
+endif()
+
+file(GLOB_RECURSE
+ ICEBERG_PUBLIC_HEADERS
+ CONFIGURE_DEPENDS
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h"
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp")
+list(SORT ICEBERG_PUBLIC_HEADERS)
+
+set(ICEBERG_PUBLIC_HEADER_CHECK_SOURCE
+ "// Generated from iceberg's installed public headers.\n")
+foreach(ICEBERG_PUBLIC_HEADER IN LISTS ICEBERG_PUBLIC_HEADERS)
+ file(RELATIVE_PATH ICEBERG_PUBLIC_HEADER_RELATIVE_PATH "${ICEBERG_PUBLIC_INCLUDE_DIR}"
+ "${ICEBERG_PUBLIC_HEADER}")
+ string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE
+ "#include <${ICEBERG_PUBLIC_HEADER_RELATIVE_PATH}>\n")
+endforeach()
+string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE "\nint main() { return 0; }\n")
+
+file(GENERATE
+ 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")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4034030795
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,65 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
Review Comment:
Make sense to me, I think we should build both for compatibility purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4062564001
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
@manuzhang Thanks.
Wondering which one you mean: `<repo_root>/src/iceberg/third_lib` or `<repo_root>/third_lib`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119229470
##########
src/iceberg/util/error_collector.h:
##########
@@ -101,6 +104,13 @@ class ICEBERG_EXPORT ErrorCollector {
ErrorCollector(const ErrorCollector&) = default;
ErrorCollector& operator=(const ErrorCollector&) = default;
+// C++23 uses deducing `this` so that `return AddError(...)` keeps returning the
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4063065363
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
I see there's already a `thirdparty` directory, but sure that's the best place. @wgtmac should have more background.
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
I see there's already a `thirdparty` directory, but I'm not sure that's the best place. @wgtmac should have more background.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4067949636
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make public headers consumable as C++20 in compat mode [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#issuecomment-6095953065
@wgtmac @zhjwpku @manuzhang Thank you all for the review. @wgtmac Thank you for the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119045023
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
+# C++20 is the minimum standard iceberg-cpp's public headers support, so the
+# example builds as C++20 by default to keep that contract exercised. Set this to
+# 23 to also check the headers from a C++23 consumer.
+set(ICEBERG_EXAMPLE_CXX_STANDARD
+ 20
+ 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 "
+ "'${ICEBERG_EXAMPLE_CXX_STANDARD}'")
+endif()
+
+set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD})
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_CXX_EXTENSIONS OFF)
find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest)
+if(TARGET iceberg::iceberg_bundle_shared)
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared)
+else()
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static)
+endif()
+
+if(TARGET iceberg::iceberg_rest_shared)
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared)
+else()
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static)
+endif()
+
add_executable(demo_example demo_example.cc)
-target_link_libraries(demo_example
- PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>"
- "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>"
-)
+target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET}
+ ${ICEBERG_REST_TARGET})
+
+# Compile every installed public header as a consumer using
+# ICEBERG_EXAMPLE_CXX_STANDARD. The installed include
+# tree is the public API contract: iceberg_install_all_headers excludes internal
+# headers before packaging them.
+get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET}
+ INTERFACE_INCLUDE_DIRECTORIES)
+foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS)
+ if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg")
+ set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}")
+ break()
+ endif()
+endforeach()
+
+if(NOT ICEBERG_PUBLIC_INCLUDE_DIR)
+ message(FATAL_ERROR "Could not locate iceberg's installed public headers")
+endif()
+
+file(GLOB_RECURSE
+ ICEBERG_PUBLIC_HEADERS
+ CONFIGURE_DEPENDS
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h"
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp")
Review Comment:
Thanks, checked the source code and no .hpp headers, removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4036092905
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
Do we have to put this file here? It will be better placed in a `third_lib` directory and a README to explain its purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4068031021
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
I'm worried about long term maintenance. We may forget what it is, where it comes from and when to drop over time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4036092905
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4068031021
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
I'm worried about long term maintenance. We may forget what it is and where it comes from over time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119391218
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,79 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
+# C++20 is the minimum standard iceberg-cpp's public headers support, so the
+# example builds as C++20 by default to keep that contract exercised. Set this to
+# 23 to also check the headers from a C++23 consumer.
+set(ICEBERG_EXAMPLE_CXX_STANDARD
+ 20
+ 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 "
+ "'${ICEBERG_EXAMPLE_CXX_STANDARD}'")
+endif()
+
+set(CMAKE_CXX_STANDARD ${ICEBERG_EXAMPLE_CXX_STANDARD})
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+set(CMAKE_CXX_EXTENSIONS OFF)
find_package(iceberg CONFIG REQUIRED COMPONENTS bundle rest)
+if(TARGET iceberg::iceberg_bundle_shared)
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_shared)
+else()
+ set(ICEBERG_BUNDLE_TARGET iceberg::iceberg_bundle_static)
+endif()
+
+if(TARGET iceberg::iceberg_rest_shared)
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_shared)
+else()
+ set(ICEBERG_REST_TARGET iceberg::iceberg_rest_static)
+endif()
+
add_executable(demo_example demo_example.cc)
-target_link_libraries(demo_example
- PRIVATE "$<IF:$<TARGET_EXISTS:iceberg::iceberg_bundle_shared>,iceberg::iceberg_bundle_shared,iceberg::iceberg_bundle_static>"
- "$<IF:$<TARGET_EXISTS:iceberg::iceberg_rest_shared>,iceberg::iceberg_rest_shared,iceberg::iceberg_rest_static>"
-)
+target_link_libraries(demo_example PRIVATE ${ICEBERG_BUNDLE_TARGET}
+ ${ICEBERG_REST_TARGET})
+
+# Compile every installed public header as a consumer using
+# ICEBERG_EXAMPLE_CXX_STANDARD. The installed include
+# tree is the public API contract: iceberg_install_all_headers excludes internal
+# headers before packaging them.
+get_target_property(ICEBERG_BUNDLE_INCLUDE_DIRS ${ICEBERG_BUNDLE_TARGET}
+ INTERFACE_INCLUDE_DIRECTORIES)
+foreach(ICEBERG_INCLUDE_DIR IN LISTS ICEBERG_BUNDLE_INCLUDE_DIRS)
+ if(EXISTS "${ICEBERG_INCLUDE_DIR}/iceberg")
+ set(ICEBERG_PUBLIC_INCLUDE_DIR "${ICEBERG_INCLUDE_DIR}")
+ break()
+ endif()
+endforeach()
+
+if(NOT ICEBERG_PUBLIC_INCLUDE_DIR)
+ message(FATAL_ERROR "Could not locate iceberg's installed public headers")
+endif()
+
+file(GLOB_RECURSE
+ ICEBERG_PUBLIC_HEADERS
+ CONFIGURE_DEPENDS
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.h"
+ "${ICEBERG_PUBLIC_INCLUDE_DIR}/iceberg/*.hpp")
+list(SORT ICEBERG_PUBLIC_HEADERS)
+
+set(ICEBERG_PUBLIC_HEADER_CHECK_SOURCE
+ "// Generated from iceberg's installed public headers.\n")
+foreach(ICEBERG_PUBLIC_HEADER IN LISTS ICEBERG_PUBLIC_HEADERS)
+ file(RELATIVE_PATH ICEBERG_PUBLIC_HEADER_RELATIVE_PATH "${ICEBERG_PUBLIC_INCLUDE_DIR}"
+ "${ICEBERG_PUBLIC_HEADER}")
+ string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE
+ "#include <${ICEBERG_PUBLIC_HEADER_RELATIVE_PATH}>\n")
+endforeach()
+string(APPEND ICEBERG_PUBLIC_HEADER_CHECK_SOURCE "\nint main() { return 0; }\n")
+
+file(GENERATE
+ 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")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4128684122
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2443 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2024 zeus-cpp
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+/// \file iceberg/expected.h
+/// \brief A C++20 backport of C++23 `std::expected`, used as the storage type
+/// behind `iceberg::Result`.
+///
+/// Purpose: the library itself may be built as C++23, but its public headers
+/// must stay consumable from C++20 translation units. `std::expected` is a
+/// C++23 library feature, so `iceberg::Result<T>` (see iceberg/result.h) is
+/// defined in terms of `iceberg::expected` instead. The API mirrors
+/// `std::expected` (including the monadic `and_then`, `or_else`, `transform`
+/// and `transform_error`), so code can be written as if against the standard
+/// type.
+///
+/// History:
+/// - apache/iceberg-cpp#40 vendored this header, adapted from
+/// https://github.com/zeus-cpp/expected (MIT), while the project targeted
Review Comment:
@zhjwpku Thanks, makes sense, updated to v1.4.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make public headers consumable as C++20 in compat mode [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4229434349
##########
src/iceberg/update/snapshot_update.h:
##########
@@ -66,96 +77,124 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
///
/// \param reporter The metrics reporter to use.
/// \return Reference to this for method chaining.
+#if ICEBERG_CXX20_COMPAT
Review Comment:
Could we use the C++20 approach instead of the macro with C++23 explicit object parameter? cc @wgtmac
##########
src/iceberg/logging/log_level.h:
##########
@@ -66,7 +67,7 @@ constexpr std::string_view ToString(LogLevel level) noexcept {
case LogLevel::kOff:
return "off";
}
- std::unreachable();
+ ::iceberg::unreachable();
Review Comment:
Moving the `ToString`s from `.h` to `.cc` would let us drop `iceberg/compat/unreachable.h` and use `std::unreachable()`, but I'm fine with the current approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4032441938
##########
example/CMakeLists.txt:
##########
@@ -20,13 +20,65 @@ cmake_minimum_required(VERSION 3.25)
project(example)
-set(CMAKE_CXX_STANDARD 23)
Review Comment:
Can we make C++23 still as default and let it accept user supplied option so that C++20 can be test manually locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4068376270
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make public headers consumable as C++20 in compat mode [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4229588252
##########
src/iceberg/logging/log_level.h:
##########
@@ -66,7 +67,7 @@ constexpr std::string_view ToString(LogLevel level) noexcept {
case LogLevel::kOff:
return "off";
}
- std::unreachable();
+ ::iceberg::unreachable();
Review Comment:
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.
##########
src/iceberg/update/snapshot_update.h:
##########
@@ -66,96 +77,124 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
///
/// \param reporter The metrics reporter to use.
/// \return Reference to this for method chaining.
+#if ICEBERG_CXX20_COMPAT
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make public headers consumable as C++20 in compat mode [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#issuecomment-6079954202
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make public headers consumable as C++20 in compat mode [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4119239461
##########
mkdocs/docs/getting-started.md:
##########
@@ -23,10 +23,14 @@
**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
Review Comment:
Thanks, reversed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#issuecomment-6056823391
@wgtmac @zhjwpku gentle ping, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "PingLiuPing (via GitHub)" <gi...@apache.org>.
PingLiuPing commented on code in PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#discussion_r4078077770
##########
src/iceberg/expected.h:
##########
@@ -0,0 +1,2401 @@
+/*
Review Comment:
@manuzhang @zhjwpku Added comments in expected.h. Let's me know if that works for you.
Happy to make further changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make the public headers consumable as C++20 [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#issuecomment-6078372055
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: make public headers consumable as C++20 in compat mode [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #936:
URL: https://github.com/apache/iceberg-cpp/pull/936#issuecomment-6096229726
@PingLiuPing Please let us know whether this works in your case and feel free to create followup PRs when needed. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org