Applied changes according to review

man-group · Nov 20, 2023 · e60a987 · e60a987
1 parent ae0a882
commit e60a987
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 8 deletions.
diff --git a/cpp/arcticdb/processing/clause.cpp b/cpp/arcticdb/processing/clause.cpp
@@ -309,6 +309,7 @@ Composite<ProcessingUnit> AggregationClause::process(std::shared_ptr<Store> stor
                                                 using optional_iter_type = std::optional<decltype(input_data.bit_vector()->first())>;
                                                 optional_iter_type iter = std::nullopt;
                                                 size_t previous_value_index = 0;
+                                                constexpr size_t missing_value_group_id = 0;
 
                                                 if (is_sparse)
                                                 {
@@ -340,10 +341,10 @@ Composite<ProcessingUnit> AggregationClause::process(std::shared_ptr<Store> stor
                                                             val = *ptr;
                                                         }
                                                         if (is_sparse) {
-                                                            for (size_t j = previous_value_index; j != *(iter.value()); ++j) {
-                                                                row_to_group.emplace_back(size_t(0));
+                                                            for (size_t j = previous_value_index + 1; j != *(iter.value()); ++j) {
+                                                                row_to_group.emplace_back(missing_value_group_id);
                                                             }
-                                                            previous_value_index = *(iter.value()) + 1;
+                                                            previous_value_index = *(iter.value());
                                                             ++(iter.value());
                                                         }
 

diff --git a/cpp/arcticdb/processing/test/test_clause.cpp b/cpp/arcticdb/processing/test/test_clause.cpp
@@ -264,6 +264,8 @@ TEST(Clause, AggregationSparseGroupby) {
     std::shared_ptr<Store> empty_store;
     size_t num_rows{100};
     size_t unique_grouping_values{10};
+    // 1 more group because of missing values
+    size_t unique_groups{unique_grouping_values + 1};
     auto seg = generate_sparse_groupby_testing_segment(num_rows, unique_grouping_values);
 
     ProcessingUnit processing_unit(std::move(seg), pipelines::FrameSlice{});
@@ -277,21 +279,21 @@ TEST(Clause, AggregationSparseGroupby) {
     ASSERT_EQ(1, slice_and_keys.size());
 
     using aggregation_test::check_column;
-    check_column<int64_t>(slice_and_keys[0], "sum_int", unique_grouping_values + 1, [&](size_t idx) -> int64_t {
+    check_column<int64_t>(slice_and_keys[0], "sum_int", unique_groups, [unique_groups](size_t idx) -> int64_t {
         if (idx == 0) {
             return 495;
         } else {
             return 450 - static_cast<int64_t>(idx % unique_grouping_values);
         }
     });
-    check_column<int64_t>(slice_and_keys[0], "min_int", unique_grouping_values + 1, [](size_t idx) -> int64_t {
+    check_column<int64_t>(slice_and_keys[0], "min_int", unique_groups, [](size_t idx) -> int64_t {
         if (idx == 0) {
             return 0;
         } else {
             return idx;
         }
     });
-    check_column<int64_t>(slice_and_keys[0], "max_int", unique_grouping_values + 1, [&](size_t idx) -> int64_t {
+    check_column<int64_t>(slice_and_keys[0], "max_int", unique_groups, [unique_groups](size_t idx) -> int64_t {
         if (idx == 0) {
             return 99;
         }
@@ -301,14 +303,14 @@ TEST(Clause, AggregationSparseGroupby) {
             return 90 + idx % unique_grouping_values;
         }
     });
-    check_column<double>(slice_and_keys[0], "mean_int", unique_grouping_values + 1, [&](size_t idx) -> double {
+    check_column<double>(slice_and_keys[0], "mean_int", unique_groups, [unique_grouping_values](size_t idx) -> double {
         if (idx == 0) {
             return 49.5;
         } else {
             return (450 - static_cast<int64_t>(idx % unique_grouping_values)) / 9.;
         }
     });
-    check_column<uint64_t>(slice_and_keys[0], "count_int", unique_grouping_values + 1, [](size_t idx) -> uint64_t {
+    check_column<uint64_t>(slice_and_keys[0], "count_int", unique_groups, [](size_t idx) -> uint64_t {
         if (idx == 0) {
             return 10;
         } else {