Addressing feedback from @robertbindar

TileDB-Inc · Sep 16, 2023 · f5f4ac8 · f5f4ac8
1 parent 0e2bef1
commit f5f4ac8
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 51 deletions.
diff --git a/tiledb/sm/query/readers/aggregators/aggregate_with_count.h b/tiledb/sm/query/readers/aggregators/aggregate_with_count.h
@@ -60,6 +60,7 @@ class AggregateWithCount {
    *
    * @tparam SUM_T Sum type.
    * @tparam BITMAP_T Bitmap type.
+   * @tparam AggPolicy Aggregation policy.
    * @param input_data Input data for the sum.
    *
    * @return {Sum for the cells, number of cells, optional validity value}.

diff --git a/tiledb/sm/query/readers/aggregators/safe_sum.h b/tiledb/sm/query/readers/aggregators/safe_sum.h
@@ -58,10 +58,14 @@ class SafeSum {
    */
   template <typename SUM_T>
   static void safe_sum(SUM_T value, std::atomic<SUM_T>& sum) {
-    SUM_T cur_sum = sum;
+    // Start by saving the current sum value from the atomic to operate on in
+    // 'cur_sum'. Then compute the new sum in 'new_sum'.
+    // std::atomic_compare_exchange_weak will only update the value and return
+    // true if the value hasn't changed since we saved it in 'cur_sum'.
+    SUM_T cur_sum;
     SUM_T new_sum;
     do {
-      new_sum = cur_sum;
+      new_sum = cur_sum = sum;
       op(value, new_sum);
     } while (!std::atomic_compare_exchange_weak(&sum, &cur_sum, new_sum));
   }

diff --git a/tiledb/sm/query/readers/aggregators/test/unit_aggregate_with_count.cc b/tiledb/sm/query/readers/aggregators/test/unit_aggregate_with_count.cc
@@ -97,13 +97,7 @@ TEMPLATE_LIST_TEST_CASE(
     // Regular attribute.
     std::vector<uint8_t> bitmap = {1, 1, 0, 0, 0, 1, 1, 0, 1, 0};
     AggregateBuffer input_data{
-        2,
-        10,
-        fixed_data.data(),
-        nullopt,
-        nullopt,
-        false,
-        bitmap.data()};
+        2, 10, fixed_data.data(), nullopt, nullopt, false, bitmap.data()};
     auto res = aggregator.template aggregate<
         typename sum_type_data<T>::sum_type,
         uint8_t,
@@ -124,13 +118,7 @@ TEMPLATE_LIST_TEST_CASE(
 
     // Nullable attribute.
     AggregateBuffer input_data3{
-        0,
-        2,
-        fixed_data.data(),
-        nullopt,
-        validity_data.data(),
-        false,
-        nullopt};
+        0, 2, fixed_data.data(), nullopt, validity_data.data(), false, nullopt};
     auto res_nullable = aggregator_nullable.template aggregate<
         typename sum_type_data<T>::sum_type,
         uint8_t,
@@ -160,13 +148,7 @@ TEMPLATE_LIST_TEST_CASE(
     // Regular attribute.
     std::vector<uint64_t> bitmap_count = {1, 2, 4, 0, 0, 1, 2, 0, 1, 2};
     AggregateBuffer input_data{
-        2,
-        10,
-        fixed_data.data(),
-        nullopt,
-        nullopt,
-        true,
-        bitmap_count.data()};
+        2, 10, fixed_data.data(), nullopt, nullopt, true, bitmap_count.data()};
     auto res = aggregator.template aggregate<
         typename sum_type_data<T>::sum_type,
         uint64_t,
@@ -176,13 +158,7 @@ TEMPLATE_LIST_TEST_CASE(
     CHECK(std::get<2>(res) == nullopt);
 
     AggregateBuffer input_data2{
-        0,
-        2,
-        fixed_data.data(),
-        nullopt,
-        nullopt,
-        true,
-        bitmap_count.data()};
+        0, 2, fixed_data.data(), nullopt, nullopt, true, bitmap_count.data()};
     auto res2 = aggregator.template aggregate<
         typename sum_type_data<T>::sum_type,
         uint64_t,

diff --git a/tiledb/sm/query/readers/aggregators/test/unit_sum_mean.cc b/tiledb/sm/query/readers/aggregators/test/unit_sum_mean.cc
@@ -255,13 +255,7 @@ void basic_aggregation_test(std::vector<RES> expected_results) {
 
     // Nullable attribute.
     AggregateBuffer input_data3{
-        0,
-        2,
-        fixed_data.data(),
-        nullopt,
-        validity_data.data(),
-        false,
-        nullopt};
+        0, 2, fixed_data.data(), nullopt, validity_data.data(), false, nullopt};
     aggregator_nullable.aggregate_data(input_data3);
     aggregator_nullable.copy_to_user_buffer("Agg2", buffers);
     if (is_nan(expected_results[4])) {
@@ -289,25 +283,13 @@ void basic_aggregation_test(std::vector<RES> expected_results) {
     // Regular attribute.
     std::vector<uint64_t> bitmap_count = {1, 2, 4, 0, 0, 1, 2, 0, 1, 2};
     AggregateBuffer input_data{
-        2,
-        10,
-        fixed_data.data(),
-        nullopt,
-        nullopt,
-        true,
-        bitmap_count.data()};
+        2, 10, fixed_data.data(), nullopt, nullopt, true, bitmap_count.data()};
     aggregator.aggregate_data(input_data);
     aggregator.copy_to_user_buffer("Agg", buffers);
     CHECK(res == expected_results[6]);
 
     AggregateBuffer input_data2{
-        0,
-        2,
-        fixed_data.data(),
-        nullopt,
-        nullopt,
-        true,
-        bitmap_count.data()};
+        0, 2, fixed_data.data(), nullopt, nullopt, true, bitmap_count.data()};
     aggregator.aggregate_data(input_data2);
     aggregator.copy_to_user_buffer("Agg", buffers);
     CHECK(res == expected_results[7]);