From 6af0565c38dfed2cc03ed29a486b192853da9e40 Mon Sep 17 00:00:00 2001 From: Rohan Jain Date: Mon, 5 Oct 2026 12:52:51 -0700 Subject: [PATCH 1/2] fix: build with MSVC 14.42 (VS 2022 17.12) The README lists MSVC 2022+ as supported, but the sources do not compile with MSVC 14.42. Newer MSVC versions are unaffected. This change makes no functional change: - ParallelCollect used a lambda in its requires-clause and lambdas expanded over a parameter pack, which 14.42 rejects (C3546/C2326/C3520). Replace them with a class-template trait and helper function templates. - ManifestGroup used a conditional operator over two move-only Result prvalues, which 14.42 tries to copy (C2280). Select the stream with if/else in a small helper instead. - Replace the C++23 0UZ literal, which 14.42 does not support (C3688), with size_t{0}. - Add the empty parameter list to a lambda with a trailing return type in cache_test.cc. Omitting it is C++23 syntax (P1102) that 14.42 rejects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/iceberg/manifest/manifest_group.cc | 20 ++- src/iceberg/test/cache_test.cc | 2 +- .../test/merging_snapshot_update_test.cc | 2 +- src/iceberg/update/snapshot_update.cc | 2 +- src/iceberg/util/executor_util_internal.h | 138 ++++++++++-------- 5 files changed, 95 insertions(+), 69 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 87ee857e5..ccb82f0e1 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -49,6 +49,16 @@ namespace iceberg { namespace { +// Selected with if/else rather than a conditional operator because older MSVC versions +// (e.g. 14.42) try to copy the move-only Result operands of the conditional operator. +Result OpenEntriesStream(ManifestReader& reader, + bool ignore_deleted) { + if (ignore_deleted) { + return reader.LiveEntriesStream(); + } + return reader.EntriesStream(); +} + std::shared_ptr DataFileFilterSchema() { auto empty_partition_type = std::make_shared(std::vector{}); return std::make_shared(std::vector{ @@ -341,9 +351,8 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream { } ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest, columns_)); - ICEBERG_ASSIGN_OR_RAISE(entry_stream_, group_->ignore_deleted_ - ? reader->LiveEntriesStream() - : reader->EntriesStream()); + ICEBERG_ASSIGN_OR_RAISE(entry_stream_, + OpenEntriesStream(*reader, group_->ignore_deleted_)); current_spec_id_ = manifest.partition_spec_id; return true; } @@ -376,9 +385,8 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream { [this](const ManifestFile* manifest) -> Result> { ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(*manifest, columns_)); - ICEBERG_ASSIGN_OR_RAISE(auto stream, group_->ignore_deleted_ - ? reader->LiveEntriesStream() - : reader->EntriesStream()); + ICEBERG_ASSIGN_OR_RAISE( + auto stream, OpenEntriesStream(*reader, group_->ignore_deleted_)); std::vector tagged_streams; tagged_streams.emplace_back(manifest->partition_spec_id, std::move(stream)); diff --git a/src/iceberg/test/cache_test.cc b/src/iceberg/test/cache_test.cc index fbc29ab9c..ee50d1b0e 100644 --- a/src/iceberg/test/cache_test.cc +++ b/src/iceberg/test/cache_test.cc @@ -95,7 +95,7 @@ TEST(MemoizeLruTest, Threads) { group.SetExecutor(std::ref(executor)); for (int32_t thread_id = 0; thread_id < 4; ++thread_id) { - group.Submit([&] -> Status { + group.Submit([&]() -> Status { for (int32_t i = 0; i < 100; ++i) { const int32_t key = i % 8; if (memoized(key) != key * 2) { diff --git a/src/iceberg/test/merging_snapshot_update_test.cc b/src/iceberg/test/merging_snapshot_update_test.cc index d31a1eb29..355b5db5e 100644 --- a/src/iceberg/test/merging_snapshot_update_test.cc +++ b/src/iceberg/test/merging_snapshot_update_test.cc @@ -1028,7 +1028,7 @@ TEST_F(MergingSnapshotUpdateTest, WriteDeleteGroups) { op->WriteManifestsWith(executor, 3); constexpr size_t kFileCount = 15'000; - auto files = std::views::iota(0UZ, kFileCount) | + auto files = std::views::iota(size_t{0}, kFileCount) | std::views::transform([this](size_t index) { return MakeDeleteFile(std::format("/delete/group_{}.parquet", index), static_cast(index % 2)); diff --git a/src/iceberg/update/snapshot_update.cc b/src/iceberg/update/snapshot_update.cc index d6a1066ac..cdf8a6578 100644 --- a/src/iceberg/update/snapshot_update.cc +++ b/src/iceberg/update/snapshot_update.cc @@ -103,7 +103,7 @@ Result> WriteManifestGroups(OptionalExecutor executor, // TODO(zehua): Replace the manual offset calculation with `std::views::chunk` // once the supported libc++ provides it. auto groups = - std::views::iota(0UZ, group_count) | + std::views::iota(size_t{0}, group_count) | std::views::transform([files, group_size](size_t group_index) { const size_t offset = group_index * group_size; return files.subspan(offset, std::min(group_size, files.size() - offset)); diff --git a/src/iceberg/util/executor_util_internal.h b/src/iceberg/util/executor_util_internal.h index 9e8ac3782..61c03dd70 100644 --- a/src/iceberg/util/executor_util_internal.h +++ b/src/iceberg/util/executor_util_internal.h @@ -82,6 +82,19 @@ concept ParallelCollectible = requires ParallelReducible, Options...>; }; +// Checked through a class template rather than a lambda in the requires-clause because +// older MSVC versions (e.g. 14.42) cannot expand the Args pack inside such a lambda. +template +struct ParallelCollectibleArgs : std::false_type {}; + +template +struct ParallelCollectibleArgs, std::tuple, Options...> + : std::bool_constant<( + ParallelCollectible::input_type, + typename ParallelCollectTraits::task_type, + Options...> && + ...)> {}; + } // namespace internal template @@ -157,82 +170,87 @@ struct ParallelReduce> { } }; +// The helpers below replace lambdas that were expanded over the pair index pack, which +// older MSVC versions (e.g. 14.42) cannot compile. +namespace internal { + +template +auto MakeParallelCollectValues(ArgsTuple& args_tuple, std::index_sequence) { + return std::tuple{ + std::vector, + std::tuple_element_t>>( + std::ranges::size(std::get(args_tuple)))...}; +} + +template +auto ReduceParallelCollectValues(ValuesTuple& values_tuple, std::index_sequence) { + if constexpr (sizeof...(I) == 1) { + return ParallelReduce::value_type, + Options...>::Reduce(std::get<0>(values_tuple)); + } else { + return std::tuple{ + ParallelReduce::value_type, + Options...>::Reduce(std::get(values_tuple))...}; + } +} + +template +void SubmitParallelCollectPair(Group& group, ArgsTuple& args_tuple, + ValuesTuple& values_tuple) { + using item_ref = std::ranges::range_reference_t>; + + for (auto&& [item, value] : + std::views::zip(std::get(args_tuple), std::get(values_tuple))) { + if constexpr (std::is_lvalue_reference_v) { + group.Submit([&]() -> Status { + ICEBERG_ASSIGN_OR_RAISE(value, + std::invoke(std::get(args_tuple), item)); + return {}; + }); + } else { + group.Submit([&, item = std::move(item)]() mutable -> Status { + ICEBERG_ASSIGN_OR_RAISE( + value, std::invoke(std::get(args_tuple), std::move(item))); + return {}; + }); + } + } +} + +template +void SubmitParallelCollectTasks(Group& group, ArgsTuple& args_tuple, + ValuesTuple& values_tuple, std::index_sequence) { + (SubmitParallelCollectPair(group, args_tuple, values_tuple), ...); +} + +} // namespace internal + template - requires(sizeof...(Args) >= 2 && sizeof...(Args) % 2 == 0 && - [](std::index_sequence) consteval { - return (internal::ParallelCollectible< - typename internal::ParallelCollectTraits::input_type, - typename internal::ParallelCollectTraits::task_type, - Options...> && - ...); - }(std::make_index_sequence{})) + requires( + sizeof...(Args) >= 2 && sizeof...(Args) % 2 == 0 && + internal::ParallelCollectibleArgs, + std::tuple, Options...>::value) auto ParallelCollect(OptionalExecutor executor, Args&&... args) { constexpr std::size_t pair_count = sizeof...(Args) / 2; using indices = std::make_index_sequence; auto args_tuple = std::forward_as_tuple(std::forward(args)...); + auto values_tuple = internal::MakeParallelCollectValues(args_tuple, indices{}); - auto values_tuple = [&](std::index_sequence) { - return std::tuple{[&] { - using traits = internal::ParallelCollectTraits; - - return std::vector( - std::ranges::size(std::get(args_tuple))); - }()...}; - }(indices{}); - - auto reduce_all = [&](std::index_sequence) { - auto reduce_one = [&] { - using traits = internal::ParallelCollectTraits; - using value_type = typename traits::value_type; - return ParallelReduce::Reduce( - std::get(values_tuple)); - }; - - if constexpr (pair_count == 1) { - return reduce_one.template operator()<0>(); - } else { - return std::tuple{reduce_one.template operator()()...}; - } - }; - - using result_type = decltype(reduce_all(indices{})); + using result_type = decltype(internal::ReduceParallelCollectValues( + values_tuple, indices{})); TaskGroup group; group.SetExecutor(executor); - - [&](std::index_sequence) { - ( - [&] { - using item_ref = std::ranges::range_reference_t< - typename internal::ParallelCollectTraits::input_type>; - - for (auto&& [item, value] : - std::views::zip(std::get(args_tuple), std::get(values_tuple))) { - if constexpr (std::is_lvalue_reference_v) { - group.Submit([&]() -> Status { - ICEBERG_ASSIGN_OR_RAISE( - value, std::invoke(std::get(args_tuple), item)); - return {}; - }); - } else { - group.Submit([&, item = std::move(item)]() mutable -> Status { - ICEBERG_ASSIGN_OR_RAISE( - value, std::invoke(std::get(args_tuple), std::move(item))); - return {}; - }); - } - } - }(), - ...); - }(indices{}); + internal::SubmitParallelCollectTasks(group, args_tuple, values_tuple, indices{}); auto status = std::move(group).Run(); if (!status.has_value()) { return Result(std::unexpected(status.error())); } - return Result(reduce_all(indices{})); + return Result( + internal::ReduceParallelCollectValues(values_tuple, indices{})); } } // namespace iceberg From 3c95ca8c5fb796c351aa5ab076c2e96a793a85b8 Mon Sep 17 00:00:00 2001 From: Rohan Jain Date: Mon, 5 Oct 2026 14:08:59 -0700 Subject: [PATCH 2/2] ci: retrigger CI after runner infrastructure failures Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>