From c8a21c14c5b85f2d016967c818e176335056efea Mon Sep 17 00:00:00 2001 From: Giulio Eulisse <10544+ktf@users.noreply.github.com> Date: Thu, 15 May 2025 16:58:29 +0200 Subject: [PATCH] DPL: Drop obsolete API The new plugin based mechanism does not need the bulk insertion anymore. --- .../Core/include/Framework/TableBuilder.h | 106 +----------------- .../Core/test/benchmark_TableBuilder.cxx | 33 ------ Framework/Core/test/test_TableBuilder.cxx | 34 ------ 3 files changed, 4 insertions(+), 169 deletions(-) diff --git a/Framework/Core/include/Framework/TableBuilder.h b/Framework/Core/include/Framework/TableBuilder.h index 8d7601cefc634..0b35d5be083e4 100644 --- a/Framework/Core/include/Framework/TableBuilder.h +++ b/Framework/Core/include/Framework/TableBuilder.h @@ -42,12 +42,6 @@ class Table; class Array; } // namespace arrow -template -struct BulkInfo { - const T ptr; - size_t size; -}; - extern template class arrow::NumericBuilder; extern template class arrow::NumericBuilder; extern template class arrow::NumericBuilder; @@ -200,34 +194,6 @@ struct BuilderUtils { } } - template - static arrow::Status bulkAppend(HolderType& holder, size_t bulkSize, const PTR ptr) - { - return holder.builder->AppendValues(ptr, bulkSize, nullptr); - } - - template - static arrow::Status bulkAppendChunked(HolderType& holder, BulkInfo info) - { - // Appending nullptr is a no-op. - if (info.ptr == nullptr) { - return arrow::Status::OK(); - } - if constexpr (std::is_same_v>) { - if (appendToList>(holder.builder, info.ptr, info.size).ok() == false) { - throw runtime_error("Unable to append to column"); - } else { - return arrow::Status::OK(); - } - } else { - if (holder.builder->AppendValues(info.ptr, info.size, nullptr).ok() == false) { - throw runtime_error("Unable to append to column"); - } else { - return arrow::Status::OK(); - } - } - } - template static arrow::Status append(HolderType& holder, std::pair ip) { @@ -518,14 +484,6 @@ struct TableBuilderHelpers { return {BuilderTraits::make_datatype()...}; } - template - static std::vector> makeFields(std::array const& names) - { - char const* const* names_ptr = names.data(); - return { - std::make_shared(*names_ptr++, BuilderMaker::make_datatype(), true, nullptr)...}; - } - /// Invokes the append method for each entry in the tuple template static bool append(std::tuple& holders, VALUES&& values) @@ -542,19 +500,6 @@ struct TableBuilderHelpers { (BuilderUtils::unsafeAppend(std::get(holders), std::get(values)), ...); } - template - static bool bulkAppend(std::tuple& holders, size_t bulkSize, PTRS ptrs) - { - return (BuilderUtils::bulkAppend(std::get(holders), bulkSize, std::get(ptrs)).ok() && ...); - } - - /// Return true if all columns are done. - template - static bool bulkAppendChunked(std::tuple& holders, INFOS infos) - { - return (BuilderUtils::bulkAppendChunked(std::get(holders), std::get(infos)).ok() && ...); - } - /// Invokes the append method for each entry in the tuple template static bool finalize(std::vector>& arrays, std::tuple& holders) @@ -575,15 +520,9 @@ constexpr auto tuple_to_pack(std::tuple&&) return framework::pack{}; } -template -concept BulkInsertable = (std::integral> && !std::same_as>); - template struct InsertionTrait { - static consteval DirectInsertion policy() - requires(!BulkInsertable); - static consteval CachedInsertion policy() - requires(BulkInsertable); + static consteval DirectInsertion policy(); using Policy = decltype(policy()); }; @@ -658,7 +597,9 @@ class TableBuilder template auto makeBuilders(std::array const& columnNames, size_t nRows) { - mSchema = std::make_shared(TableBuilderHelpers::makeFields(columnNames)); + char const* const* names_ptr = columnNames.data(); + mSchema = std::make_shared( + std::vector>({std::make_shared(*names_ptr++, BuilderMaker::make_datatype(), true, nullptr)...})); mHolders = makeHolders(mMemoryPool, nRows); mFinalizer = [](std::vector>& arrays, void* holders) -> bool { @@ -768,45 +709,6 @@ class TableBuilder }(typename T::table_t::persistent_columns_t{}); } - template - auto preallocatedPersist(std::array const& columnNames, int nRows) - { - constexpr size_t nColumns = NCOLUMNS; - validate(); - mArrays.resize(nColumns); - makeBuilders(columnNames, nRows); - - // Callback used to fill the builders - return [holders = mHolders](unsigned int /*slot*/, typename BuilderMaker::FillType... args) -> void { - TableBuilderHelpers::unsafeAppend(*(HoldersTupleIndexed*)holders, std::forward_as_tuple(args...)); - }; - } - - template - auto bulkPersist(std::array const& columnNames, size_t nRows) - { - validate(); - // Should not be called more than once - mArrays.resize(NCOLUMNS); - makeBuilders(columnNames, nRows); - - return [holders = mHolders](unsigned int /*slot*/, size_t batchSize, typename BuilderMaker::FillType const*... args) -> void { - TableBuilderHelpers::bulkAppend(*(HoldersTupleIndexed*)holders, batchSize, std::forward_as_tuple(args...)); - }; - } - - template - auto bulkPersistChunked(std::array const& columnNames, size_t nRows) - { - validate(); - mArrays.resize(NCOLUMNS); - makeBuilders(columnNames, nRows); - - return [holders = mHolders](unsigned int /*slot*/, BulkInfo::STLValueType const*>... args) -> bool { - return TableBuilderHelpers::bulkAppendChunked(*(HoldersTupleIndexed*)holders, std::forward_as_tuple(args...)); - }; - } - /// Reserve method to expand the columns as needed. template auto reserveArrays(std::tuple& holders, int s) diff --git a/Framework/Core/test/benchmark_TableBuilder.cxx b/Framework/Core/test/benchmark_TableBuilder.cxx index 59d1450e895bd..5b9dee866c8a3 100644 --- a/Framework/Core/test/benchmark_TableBuilder.cxx +++ b/Framework/Core/test/benchmark_TableBuilder.cxx @@ -62,39 +62,6 @@ static void BM_TableBuilderScalarReserved(benchmark::State& state) BENCHMARK(BM_TableBuilderScalarReserved)->Arg(1 << 21); BENCHMARK(BM_TableBuilderScalarReserved)->Range(8, 8 << 16); -static void BM_TableBuilderScalarPresized(benchmark::State& state) -{ - using namespace o2::framework; - for (auto _ : state) { - TableBuilder builder; - auto rowWriter = builder.preallocatedPersist({"x"}, state.range(0)); - for (auto i = 0; i < state.range(0); ++i) { - rowWriter(0, 0.f); - } - auto table = builder.finalize(); - } -} - -BENCHMARK(BM_TableBuilderScalarPresized)->Arg(1 << 20); -BENCHMARK(BM_TableBuilderScalarPresized)->Range(8, 8 << 16); - -static void BM_TableBuilderScalarBulk(benchmark::State& state) -{ - using namespace o2::framework; - auto chunkSize = state.range(0) / 256; - std::vector buffer(chunkSize, 0.); // We assume data is chunked in blocks 256th of the total size - for (auto _ : state) { - TableBuilder builder; - auto bulkWriter = builder.bulkPersist({"x"}, state.range(0)); - for (auto i = 0; i < state.range(0) / chunkSize; ++i) { - bulkWriter(0, chunkSize, buffer.data()); - } - auto table = builder.finalize(); - } -} - -BENCHMARK(BM_TableBuilderScalarBulk)->Range(256, 1 << 20); - static void BM_TableBuilderSimple(benchmark::State& state) { using namespace o2::framework; diff --git a/Framework/Core/test/test_TableBuilder.cxx b/Framework/Core/test/test_TableBuilder.cxx index b08fee5ad4e6a..00cbbbc59b725 100644 --- a/Framework/Core/test/test_TableBuilder.cxx +++ b/Framework/Core/test/test_TableBuilder.cxx @@ -162,30 +162,6 @@ TEST_CASE("TestTableBuilderStruct") } } -TEST_CASE("TestTableBuilderBulk") -{ - using namespace o2::framework; - TableBuilder builder; - auto bulkWriter = builder.bulkPersist({"x", "y"}, 10); - int x[] = {0, 1, 2, 3, 4, 5, 6, 7}; - int y[] = {0, 1, 2, 3, 4, 5, 6, 7}; - - bulkWriter(0, 8, x, y); - - auto table = builder.finalize(); - REQUIRE(table->num_columns() == 2); - REQUIRE(table->num_rows() == 8); - REQUIRE(table->schema()->field(0)->name() == "x"); - REQUIRE(table->schema()->field(1)->name() == "y"); - REQUIRE(table->schema()->field(0)->type()->id() == arrow::int32()->id()); - REQUIRE(table->schema()->field(1)->type()->id() == arrow::int32()->id()); - - for (int64_t i = 0; i < 8; ++i) { - auto p = std::dynamic_pointer_cast>(table->column(0)->chunk(0)); - REQUIRE(p->Value(i) == i); - } -} - TEST_CASE("TestTableBuilderMore") { using namespace o2::framework; @@ -288,13 +264,3 @@ TEST_CASE("TestColumnCount") int count2 = TableBuilder::countColumns(); REQUIRE(count2 == 3); } - -TEST_CASE("TestMakeFields") -{ - auto fields = TableBuilderHelpers::makeFields({"i", "f"}); - REQUIRE(fields.size() == 2); - REQUIRE(fields[0]->name() == "i"); - REQUIRE(fields[1]->name() == "f"); - REQUIRE(fields[0]->type()->name() == "int32"); - REQUIRE(fields[1]->type()->name() == "float"); -}