Skip to content

Commit 6af0565

Browse files
Rohan JainCopilot
andcommitted
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<ManifestEntryStreamPtr> 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>
1 parent 85f8a84 commit 6af0565

5 files changed

Lines changed: 95 additions & 69 deletions

File tree

‎src/iceberg/manifest/manifest_group.cc‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,16 @@ namespace iceberg {
4949

5050
namespace {
5151

52+
// Selected with if/else rather than a conditional operator because older MSVC versions
53+
// (e.g. 14.42) try to copy the move-only Result operands of the conditional operator.
54+
Result<ManifestEntryStreamPtr> OpenEntriesStream(ManifestReader& reader,
55+
bool ignore_deleted) {
56+
if (ignore_deleted) {
57+
return reader.LiveEntriesStream();
58+
}
59+
return reader.EntriesStream();
60+
}
61+
5262
std::shared_ptr<Schema> DataFileFilterSchema() {
5363
auto empty_partition_type = std::make_shared<StructType>(std::vector<SchemaField>{});
5464
return std::make_shared<Schema>(std::vector<SchemaField>{
@@ -341,9 +351,8 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream {
341351
}
342352

343353
ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest, columns_));
344-
ICEBERG_ASSIGN_OR_RAISE(entry_stream_, group_->ignore_deleted_
345-
? reader->LiveEntriesStream()
346-
: reader->EntriesStream());
354+
ICEBERG_ASSIGN_OR_RAISE(entry_stream_,
355+
OpenEntriesStream(*reader, group_->ignore_deleted_));
347356
current_spec_id_ = manifest.partition_spec_id;
348357
return true;
349358
}
@@ -376,9 +385,8 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream {
376385
[this](const ManifestFile* manifest) -> Result<std::vector<TaggedStream>> {
377386
ICEBERG_ASSIGN_OR_RAISE(auto reader,
378387
group_->MakeReader(*manifest, columns_));
379-
ICEBERG_ASSIGN_OR_RAISE(auto stream, group_->ignore_deleted_
380-
? reader->LiveEntriesStream()
381-
: reader->EntriesStream());
388+
ICEBERG_ASSIGN_OR_RAISE(
389+
auto stream, OpenEntriesStream(*reader, group_->ignore_deleted_));
382390

383391
std::vector<TaggedStream> tagged_streams;
384392
tagged_streams.emplace_back(manifest->partition_spec_id, std::move(stream));

‎src/iceberg/test/cache_test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ TEST(MemoizeLruTest, Threads) {
9595
group.SetExecutor(std::ref(executor));
9696

9797
for (int32_t thread_id = 0; thread_id < 4; ++thread_id) {
98-
group.Submit([&] -> Status {
98+
group.Submit([&]() -> Status {
9999
for (int32_t i = 0; i < 100; ++i) {
100100
const int32_t key = i % 8;
101101
if (memoized(key) != key * 2) {

‎src/iceberg/test/merging_snapshot_update_test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1028,7 +1028,7 @@ TEST_F(MergingSnapshotUpdateTest, WriteDeleteGroups) {
10281028
op->WriteManifestsWith(executor, 3);
10291029

10301030
constexpr size_t kFileCount = 15'000;
1031-
auto files = std::views::iota(0UZ, kFileCount) |
1031+
auto files = std::views::iota(size_t{0}, kFileCount) |
10321032
std::views::transform([this](size_t index) {
10331033
return MakeDeleteFile(std::format("/delete/group_{}.parquet", index),
10341034
static_cast<int64_t>(index % 2));

‎src/iceberg/update/snapshot_update.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ Result<std::vector<ManifestFile>> WriteManifestGroups(OptionalExecutor executor,
103103
// TODO(zehua): Replace the manual offset calculation with `std::views::chunk`
104104
// once the supported libc++ provides it.
105105
auto groups =
106-
std::views::iota(0UZ, group_count) |
106+
std::views::iota(size_t{0}, group_count) |
107107
std::views::transform([files, group_size](size_t group_index) {
108108
const size_t offset = group_index * group_size;
109109
return files.subspan(offset, std::min(group_size, files.size() - offset));

‎src/iceberg/util/executor_util_internal.h‎

Lines changed: 78 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,19 @@ concept ParallelCollectible =
8282
requires ParallelReducible<ParallelCollectValueT<InputRange, Task>, Options...>;
8383
};
8484

85+
// Checked through a class template rather than a lambda in the requires-clause because
86+
// older MSVC versions (e.g. 14.42) cannot expand the Args pack inside such a lambda.
87+
template <typename Indices, typename ArgsTuple, auto... Options>
88+
struct ParallelCollectibleArgs : std::false_type {};
89+
90+
template <std::size_t... I, typename... Args, auto... Options>
91+
struct ParallelCollectibleArgs<std::index_sequence<I...>, std::tuple<Args...>, Options...>
92+
: std::bool_constant<(
93+
ParallelCollectible<typename ParallelCollectTraits<I, Args...>::input_type,
94+
typename ParallelCollectTraits<I, Args...>::task_type,
95+
Options...> &&
96+
...)> {};
97+
8598
} // namespace internal
8699

87100
template <typename... Args>
@@ -157,82 +170,87 @@ struct ParallelReduce<std::tuple<Ts...>> {
157170
}
158171
};
159172

173+
// The helpers below replace lambdas that were expanded over the pair index pack, which
174+
// older MSVC versions (e.g. 14.42) cannot compile.
175+
namespace internal {
176+
177+
template <typename ArgsTuple, std::size_t... I>
178+
auto MakeParallelCollectValues(ArgsTuple& args_tuple, std::index_sequence<I...>) {
179+
return std::tuple{
180+
std::vector<ParallelCollectValueT<std::tuple_element_t<I * 2, ArgsTuple>,
181+
std::tuple_element_t<I * 2 + 1, ArgsTuple>>>(
182+
std::ranges::size(std::get<I * 2>(args_tuple)))...};
183+
}
184+
185+
template <auto... Options, typename ValuesTuple, std::size_t... I>
186+
auto ReduceParallelCollectValues(ValuesTuple& values_tuple, std::index_sequence<I...>) {
187+
if constexpr (sizeof...(I) == 1) {
188+
return ParallelReduce<typename std::tuple_element_t<0, ValuesTuple>::value_type,
189+
Options...>::Reduce(std::get<0>(values_tuple));
190+
} else {
191+
return std::tuple{
192+
ParallelReduce<typename std::tuple_element_t<I, ValuesTuple>::value_type,
193+
Options...>::Reduce(std::get<I>(values_tuple))...};
194+
}
195+
}
196+
197+
template <std::size_t I, typename Group, typename ArgsTuple, typename ValuesTuple>
198+
void SubmitParallelCollectPair(Group& group, ArgsTuple& args_tuple,
199+
ValuesTuple& values_tuple) {
200+
using item_ref = std::ranges::range_reference_t<std::tuple_element_t<I * 2, ArgsTuple>>;
201+
202+
for (auto&& [item, value] :
203+
std::views::zip(std::get<I * 2>(args_tuple), std::get<I>(values_tuple))) {
204+
if constexpr (std::is_lvalue_reference_v<item_ref>) {
205+
group.Submit([&]() -> Status {
206+
ICEBERG_ASSIGN_OR_RAISE(value,
207+
std::invoke(std::get<I * 2 + 1>(args_tuple), item));
208+
return {};
209+
});
210+
} else {
211+
group.Submit([&, item = std::move(item)]() mutable -> Status {
212+
ICEBERG_ASSIGN_OR_RAISE(
213+
value, std::invoke(std::get<I * 2 + 1>(args_tuple), std::move(item)));
214+
return {};
215+
});
216+
}
217+
}
218+
}
219+
220+
template <typename Group, typename ArgsTuple, typename ValuesTuple, std::size_t... I>
221+
void SubmitParallelCollectTasks(Group& group, ArgsTuple& args_tuple,
222+
ValuesTuple& values_tuple, std::index_sequence<I...>) {
223+
(SubmitParallelCollectPair<I>(group, args_tuple, values_tuple), ...);
224+
}
225+
226+
} // namespace internal
227+
160228
template <auto... Options, typename... Args>
161-
requires(sizeof...(Args) >= 2 && sizeof...(Args) % 2 == 0 &&
162-
[]<std::size_t... I>(std::index_sequence<I...>) consteval {
163-
return (internal::ParallelCollectible<
164-
typename internal::ParallelCollectTraits<I, Args...>::input_type,
165-
typename internal::ParallelCollectTraits<I, Args...>::task_type,
166-
Options...> &&
167-
...);
168-
}(std::make_index_sequence<sizeof...(Args) / 2>{}))
229+
requires(
230+
sizeof...(Args) >= 2 && sizeof...(Args) % 2 == 0 &&
231+
internal::ParallelCollectibleArgs<std::make_index_sequence<sizeof...(Args) / 2>,
232+
std::tuple<Args...>, Options...>::value)
169233
auto ParallelCollect(OptionalExecutor executor, Args&&... args) {
170234
constexpr std::size_t pair_count = sizeof...(Args) / 2;
171235
using indices = std::make_index_sequence<pair_count>;
172236

173237
auto args_tuple = std::forward_as_tuple(std::forward<Args>(args)...);
238+
auto values_tuple = internal::MakeParallelCollectValues(args_tuple, indices{});
174239

175-
auto values_tuple = [&]<std::size_t... I>(std::index_sequence<I...>) {
176-
return std::tuple{[&] {
177-
using traits = internal::ParallelCollectTraits<I, Args...>;
178-
179-
return std::vector<typename traits::value_type>(
180-
std::ranges::size(std::get<I * 2>(args_tuple)));
181-
}()...};
182-
}(indices{});
183-
184-
auto reduce_all = [&]<std::size_t... I>(std::index_sequence<I...>) {
185-
auto reduce_one = [&]<std::size_t PairIndex> {
186-
using traits = internal::ParallelCollectTraits<PairIndex, Args...>;
187-
using value_type = typename traits::value_type;
188-
return ParallelReduce<value_type, Options...>::Reduce(
189-
std::get<PairIndex>(values_tuple));
190-
};
191-
192-
if constexpr (pair_count == 1) {
193-
return reduce_one.template operator()<0>();
194-
} else {
195-
return std::tuple{reduce_one.template operator()<I>()...};
196-
}
197-
};
198-
199-
using result_type = decltype(reduce_all(indices{}));
240+
using result_type = decltype(internal::ReduceParallelCollectValues<Options...>(
241+
values_tuple, indices{}));
200242

201243
TaskGroup group;
202244
group.SetExecutor(executor);
203-
204-
[&]<std::size_t... I>(std::index_sequence<I...>) {
205-
(
206-
[&] {
207-
using item_ref = std::ranges::range_reference_t<
208-
typename internal::ParallelCollectTraits<I, Args...>::input_type>;
209-
210-
for (auto&& [item, value] :
211-
std::views::zip(std::get<I * 2>(args_tuple), std::get<I>(values_tuple))) {
212-
if constexpr (std::is_lvalue_reference_v<item_ref>) {
213-
group.Submit([&]() -> Status {
214-
ICEBERG_ASSIGN_OR_RAISE(
215-
value, std::invoke(std::get<I * 2 + 1>(args_tuple), item));
216-
return {};
217-
});
218-
} else {
219-
group.Submit([&, item = std::move(item)]() mutable -> Status {
220-
ICEBERG_ASSIGN_OR_RAISE(
221-
value, std::invoke(std::get<I * 2 + 1>(args_tuple), std::move(item)));
222-
return {};
223-
});
224-
}
225-
}
226-
}(),
227-
...);
228-
}(indices{});
245+
internal::SubmitParallelCollectTasks(group, args_tuple, values_tuple, indices{});
229246

230247
auto status = std::move(group).Run();
231248
if (!status.has_value()) {
232249
return Result<result_type>(std::unexpected<Error>(status.error()));
233250
}
234251

235-
return Result<result_type>(reduce_all(indices{}));
252+
return Result<result_type>(
253+
internal::ReduceParallelCollectValues<Options...>(values_tuple, indices{}));
236254
}
237255

238256
} // namespace iceberg

0 commit comments

Comments
 (0)