From 768436ff2ce5f15e18b437b67652cdbed56d4403 Mon Sep 17 00:00:00 2001
From: Giulio Eulisse <10544+ktf@users.noreply.github.com>
Date: Wed, 3 Mar 2021 14:12:04 +0100
Subject: [PATCH] DPL: simplify TableBuilder API

* De-templatise TableBuilder::validate
* Move a few throw statements out of line
---
 .../Core/include/Framework/TableBuilder.h     | 33 +++++++------------
 Framework/Core/src/TableBuilder.cxx           | 20 ++++++++++-
 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/Framework/Core/include/Framework/TableBuilder.h b/Framework/Core/include/Framework/TableBuilder.h
index 43e45b9d79bb0..9836d90399dfc 100644
--- a/Framework/Core/include/Framework/TableBuilder.h
+++ b/Framework/Core/include/Framework/TableBuilder.h
@@ -556,6 +556,8 @@ struct HolderTrait<int64_t> {
 /// to build an arrow::Table from a TDataFrame.
 class TableBuilder
 {
+  static void throwError(RuntimeErrorRef const& ref);
+
   template <typename... ARGS>
   using HoldersTuple = typename std::tuple<typename HolderTrait<ARGS>::Holder...>;
 
@@ -567,17 +569,7 @@ class TableBuilder
     return (HoldersTuple<ARGS...>*)mHolders;
   }
 
-  template <typename... ARGS>
-  void validate(std::vector<std::string> const& columnNames)
-  {
-    constexpr int nColumns = sizeof...(ARGS);
-    if (nColumns != columnNames.size()) {
-      throw runtime_error("Mismatching number of column types and names");
-    }
-    if (mHolders != nullptr) {
-      throw runtime_error("TableBuilder::persist can only be invoked once per instance");
-    }
-  }
+  void validate(const int nColumns, std::vector<std::string> const& columnNames) const;
 
   template <typename... ARGS>
   auto makeBuilders(std::vector<std::string> const& columnNames, size_t nRows)
@@ -595,11 +587,8 @@ class TableBuilder
   template <typename... ARGS>
   auto makeFinalizer()
   {
-    mFinalizer = [schema = mSchema, &arrays = mArrays, holders = mHolders]() -> void {
-      auto status = TableBuilderHelpers::finalize(arrays, *(HoldersTuple<ARGS...>*)holders, std::make_index_sequence<sizeof...(ARGS)>{});
-      if (status == false) {
-        throw runtime_error("Unable to finalize");
-      }
+    mFinalizer = [schema = mSchema, &arrays = mArrays, holders = mHolders]() -> bool {
+      return TableBuilderHelpers::finalize(arrays, *(HoldersTuple<ARGS...>*)holders, std::make_index_sequence<sizeof...(ARGS)>{});
     };
   }
 
@@ -650,7 +639,7 @@ class TableBuilder
   auto persistTuple(framework::pack<ARGS...>, std::vector<std::string> const& columnNames)
   {
     constexpr int nColumns = sizeof...(ARGS);
-    validate<ARGS...>(columnNames);
+    validate(nColumns, columnNames);
     mArrays.resize(nColumns);
     makeBuilders<ARGS...>(columnNames, 1000);
     makeFinalizer<ARGS...>();
@@ -660,7 +649,7 @@ class TableBuilder
     return [holders = mHolders](unsigned int slot, FillTuple const& t) -> void {
       auto status = TableBuilderHelpers::append(*(HoldersTuple<ARGS...>*)holders, std::index_sequence_for<ARGS...>{}, t);
       if (status == false) {
-        throw runtime_error("Unable to append");
+        throwError(runtime_error("Unable to append"));
       }
     };
   }
@@ -687,7 +676,7 @@ class TableBuilder
   auto preallocatedPersist(std::vector<std::string> const& columnNames, int nRows)
   {
     constexpr int nColumns = sizeof...(ARGS);
-    validate<ARGS...>(columnNames);
+    validate(nColumns, columnNames);
     mArrays.resize(nColumns);
     makeBuilders<ARGS...>(columnNames, nRows);
     makeFinalizer<ARGS...>();
@@ -702,7 +691,7 @@ class TableBuilder
   auto bulkPersist(std::vector<std::string> const& columnNames, size_t nRows)
   {
     constexpr int nColumns = sizeof...(ARGS);
-    validate<ARGS...>(columnNames);
+    validate(nColumns, columnNames);
     mArrays.resize(nColumns);
     makeBuilders<ARGS...>(columnNames, nRows);
     makeFinalizer<ARGS...>();
@@ -716,7 +705,7 @@ class TableBuilder
   auto bulkPersistChunked(std::vector<std::string> const& columnNames, size_t nRows)
   {
     constexpr int nColumns = sizeof...(ARGS);
-    validate<ARGS...>(columnNames);
+    validate(nColumns, columnNames);
     mArrays.resize(nColumns);
     makeBuilders<ARGS...>(columnNames, nRows);
     makeFinalizer<ARGS...>();
@@ -764,7 +753,7 @@ class TableBuilder
     return this->template persist<E>(columnNames);
   }
 
-  std::function<void(void)> mFinalizer;
+  std::function<bool(void)> mFinalizer;
   void* mHolders;
   arrow::MemoryPool* mMemoryPool;
   std::shared_ptr<arrow::Schema> mSchema;
diff --git a/Framework/Core/src/TableBuilder.cxx b/Framework/Core/src/TableBuilder.cxx
index f05604c365f38..31c87218744e7 100644
--- a/Framework/Core/src/TableBuilder.cxx
+++ b/Framework/Core/src/TableBuilder.cxx
@@ -50,9 +50,27 @@ namespace o2::framework
 std::shared_ptr<arrow::Table>
   TableBuilder::finalize()
 {
-  mFinalizer();
+  bool status = mFinalizer();
+  if (status == false) {
+    throwError(runtime_error("Unable to finalize"));
+  }
   assert(mSchema->num_fields() > 0 && "Schema needs to be non-empty");
   return arrow::Table::Make(mSchema, mArrays);
 }
 
+void TableBuilder::throwError(RuntimeErrorRef const& ref)
+{
+  throw ref;
+}
+
+void TableBuilder::validate(const int nColumns, std::vector<std::string> const& columnNames) const
+{
+  if (nColumns != columnNames.size()) {
+    throwError(runtime_error("Mismatching number of column types and names"));
+  }
+  if (mHolders != nullptr) {
+    throwError(runtime_error("TableBuilder::persist can only be invoked once per instance"));
+  }
+}
+
 } // namespace o2::framework