abnobdoss commented on PR #853: URL: https://github.com/apache/iceberg-cpp/pull/853#issuecomment-5577820619
Thanks for the review, @wgtmac. Apologies for the delay getting back to this. The branch has been rebased onto Arrow 25.0.0, and I've incorporated the Hive and C++ linter conversions from #854. Builds that meet the intended outcome (build types are unchanged): | Build (type) | Main | This PR | |---|---|---| | AMD64 Ubuntu 26.04 (Debug) | Vendored | Conda | | AMD64 Ubuntu 26.04 (RelWithDebInfo) | Vendored | Conda | | AArch64 macOS 26 (Debug) | Vendored | Conda | | AMD64 Ubuntu 26.04 Hive (Debug) | Vendored | Conda + system Thrift | | ASAN and UBSAN Tests (Debug) | Vendored | Conda; Iceberg remains instrumented, Arrow is prebuilt without sanitizer instrumentation | | SQL Catalog (AMD64 Ubuntu 26.04) (Debug) | Vendored | Conda | | SQL Catalog (AArch64 macOS 26) (Debug) | Vendored | Conda | | AWS (Ubuntu 26.04, S3 + SigV4, system AWS SDK) (Debug) | Vendored Arrow + vcpkg AWS SDK | Conda Arrow + AWS SDK | | cpp-linter (Debug) | Vendored | Conda + system Thrift | | Meson - AMD64 Ubuntu 26.04 (Debug) | No Arrow | No Arrow | | Meson - AArch64 macOS 26 (Debug) | No Arrow | No Arrow | | Meson - AMD64 Windows 2025 (Debug) | No Arrow | No Arrow | | Verify (macos-26) (Release) | Vendored | Vendored | | Verify (ubuntu-26.04) (Release) | Vendored | Vendored | Open exceptions: | Build (type) | Main | This PR | Question | |---|---|---|---| | AMD64 Windows 2025 (Debug) | Vendored | Vendored | See question 1. | | SQL Catalog (AMD64 Windows 2025) (Debug) | Vendored | Vendored | See question 1. | | AWS (Ubuntu 26.04, S3 + SigV4, bundled AWS SDK) (Debug) | Vendored | Vendored | See question 2. | | AWS (macOS 26 ARM64, S3, bundled AWS SDK) (Debug) | Vendored | Vendored | See question 2. | 1. If I understand correctly the only way to use conda's prebuilt Arrow on Windows, we would have to change our Windows CMake builds from Debug to [RelWithDebInfo or Release](https://arrow.apache.org/docs/developers/cpp/windows.html#using-conda-forge-for-build-dependencies:~:text=If%20you%20intend%20to%20use%20%2DDCMAKE_BUILD_TYPE%3Ddebug%20then%20you%20must%20build%20the%20packages%20from%20source). Is that something we would want to pursue? 2. Do we want to retain both bundled AWS jobs in PR CI, or keep one and convert the other to conda with a system AWS SDK? 3. Both Hive-enabled jobs now use system Thrift. Given your point about bundled coverage, should we add bundled Hive/Thrift coverage to release verification, or revert one of those jobs to bundled dependencies? More crucially, although this speeds up builds and reduces reliance on warm caches, the extra exceptions above feel like they make CI harder to maintain. I'm leaning toward dropping this change for now because I don't see a simple way to preserve coverage without adding additional complexity to the CI which is already slightly complex. What do you think: should we continue pursuing this change or drop it? -- 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]
