wgtmac commented on code in PR #989:
URL: https://github.com/apache/iceberg-cpp/pull/989#discussion_r4272701739
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -258,6 +299,9 @@ function(resolve_arrow_dependency)
# Arrow's exported static target interface may reference system libraries
# (e.g. Threads, OpenSSL, CURL, ZLIB) that consumers need to find.
list(APPEND ICEBERG_SYSTEM_DEPENDENCIES Threads ZLIB)
+ if(ICEBERG_ENCRYPTION)
Review Comment:
Please register OpenSSL outside the vendored-only branch. The bundle exports
OpenSSL::Crypto for both dependency modes, but shared-only system Arrow/Parquet
does not create that target for installed consumers. A clean consumer with
ICEBERG_ENCRYPTION=ON and REST=OFF can therefore fail during CMake generation.
##########
src/iceberg/iceberg-config.cmake.in:
##########
@@ -78,6 +78,11 @@ macro(iceberg_find_components components)
endforeach()
endmacro()
+# OpenSSL used at build time (table encryption), e.g. Homebrew's keg-only
OpenSSL.
+if(NOT DEFINED OPENSSL_ROOT_DIR AND NOT "@ICEBERG_OPENSSL_ROOT_DIR@" STREQUAL
"")
Review Comment:
Please avoid exporting the build machine's OpenSSL root in the installed
package. After relocation, this stale hint can prevent FindOpenSSL from
searching the consumer's prefix, and it can override an environment-provided
root. Resolve OpenSSL from the consumer's toolchain or environment instead.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -178,12 +201,18 @@ function(resolve_arrow_dependency)
set(ARROW_S3 ${ICEBERG_S3})
set(ARROW_JSON ON)
set(ARROW_PARQUET ON)
+ if(ICEBERG_ENCRYPTION)
+ # Parquet modular encryption, used for encrypted Iceberg data and delete
files.
+ set(PARQUET_REQUIRE_ENCRYPTION ON)
+ endif()
set(ARROW_ENABLE_THREADING ON)
set(ARROW_SIMD_LEVEL "NONE")
set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
set(ARROW_POSITION_INDEPENDENT_CODE ON)
set(ARROW_DEPENDENCY_SOURCE "BUNDLED")
set(ARROW_WITH_ZLIB ON)
+ # zstd is the default Parquet codec of Iceberg (Java and C++).
+ set(ARROW_WITH_ZSTD ON)
Review Comment:
ZSTD is not required by Parquet modular encryption. Since this changes the
vendored dependency surface independently, could we move it to a separate
change, or document the existing writer default
(write.parquet.compression-codec=zstd) and add a focused compatibility test?
Keeping the scope explicit will make this encryption change easier to validate.
##########
CMakeLists.txt:
##########
@@ -41,6 +41,8 @@ option(ICEBERG_BUILD_SHARED "Build shared library" OFF)
option(ICEBERG_BUILD_TESTS "Build tests" ON)
option(ICEBERG_BUILD_BENCHMARKS "Build benchmarks" OFF)
option(ICEBERG_BUILD_BUNDLE "Build the battery included library" ON)
+option(ICEBERG_ENCRYPTION
+ "Build table encryption support (Parquet modular encryption, AES GCM
streams)" ON)
Review Comment:
Could this option default to OFF? This PR only enables the crypto build
capability; table encryption is not wired yet, while ON makes the default
bundle require OpenSSL and increases downstream build and packaging
requirements for users who do not use encryption. Users can opt in with
-DICEBERG_ENCRYPTION=ON until the end-to-end feature is available.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -216,6 +245,18 @@ function(resolve_arrow_dependency)
fetchcontent_makeavailable(VendoredArrow)
+ # Arrow's Parquet targets do not inherit the OpenSSL include directory when
+ # Arrow is vendored; add it for the Parquet encryption sources. Depending on
the
+ # platform and build type, they are compiled in the object library or
directly in
+ # the static/shared library. The bundle library links OpenSSL::Crypto.
+ if(ICEBERG_ENCRYPTION)
+ foreach(parquet_target parquet_objlib parquet_static parquet_shared)
+ if(TARGET ${parquet_target})
+ target_include_directories(${parquet_target} PRIVATE
${OPENSSL_INCLUDE_DIR})
Review Comment:
Please restrict this include injection to vendored Arrow targets. If a
parent project has already loaded system Parquet, parquet_static/parquet_shared
can be compatibility ALIAS targets, and target_include_directories cannot
modify an ALIAS. This can make an otherwise valid parent-project configuration
fail.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
