Repository: man-group/ArcticDB (language: python)

Issue:
Groupby on float columns containing NaNs includes NaNs in the returned dataframe
This is in contrast to Pandas, which does not include `NaN` as a group
Performance optimisations discovered during benchmarking of analytics functionality



Discussion / hints:

Fix patch:
diff --git a/cpp/arcticdb/CMakeLists.txt b/cpp/arcticdb/CMakeLists.txt
index f9301973ae..823a400b73 100644
--- a/cpp/arcticdb/CMakeLists.txt
+++ b/cpp/arcticdb/CMakeLists.txt
@@ -145,6 +145,17 @@ ENDIF()
 
 set(CMAKE_INSTALL_RPATH "\\$ORIGIN/lib")
 
+if (NOT WIN32)
+    if(NOT ${ARCTICDB_USING_CONDA})
+        # Compilers on conda-forge are relatively strict.
+        # For now, we do not want to treat warnings as errors for those builds.
+        add_compile_options("-Werror")
+    endif()
+    add_compile_options("-ggdb")
+    add_compile_options("-fvisibility=hidden")
+    add_compile_options("-fno-omit-frame-pointer")
+endif ()
+
 ## Core library without python bindings ##
 set(arcticdb_srcs
         # header files
@@ -567,17 +578,6 @@ target_link_libraries(arcticdb_core PRIVATE
 
 GENERATE_EXPORT_HEADER(arcticdb_core)
 
-if (NOT WIN32)
-    if(NOT ${ARCTICDB_USING_CONDA})
-        # Compilers on conda-forge are relatively strict.
-        # For now, we do not want to treat warnings as errors for those builds.
-        add_compile_options("-Werror")
-    endif()
-    add_compile_options("-ggdb")
-    add_compile_options("-fvisibility=hidden")
-    add_compile_options("-fno-omit-frame-pointer")
-endif ()
-
 ## Core python bindings, private only ##
 set(arcticdb_python_srcs
         async/python_bindings.cpp
diff --git a/cpp/arcticdb/column_store/memory_segment.hpp b/cpp/arcticdb/column_store/memory_segment.hpp
index da45dc36fb..4cca2d93da 100644
--- a/cpp/arcticdb/column_store/memory_segment.hpp
+++ b/cpp/arcticdb/column_store/memory_segment.hpp
@@ -403,6 +403,16 @@ class SegmentInMemory {
         return SegmentInMemory(impl_->filter(filter_bitset, filter_down_stringpool, validate));
     }
 
+    std::vector<SegmentInMemory> partition(const std::vector<std::optional<uint8_t>>& row_to_segment,
+                           const std::vector<uint64_t>& segment_counts) const{
+        std::vector<SegmentInMemory> res;
+        auto impls = impl_->partition(row_to_segment, segment_counts);
+        for (auto&& impl: impls) {
+            res.emplace_back(SegmentInMemory(std::move(impl)));
+        }
+        return res;
+    }
+
     bool empty() const {
         return is_null() || impl_->empty();
     }
diff --git a/cpp/arcticdb/column_store/memory_segment_impl.hpp b/cpp/arcticdb/column_store/memory_segment_impl.hpp
index f49982d548..eb172b19ca 100644
--- a/cpp/arcticdb/column_store/memory_segment_impl.hpp
+++ b/cpp/arcticdb/column_store/memory_segment_impl.hpp
@@ -1111,6 +1111,66 @@ class SegmentInMemoryImpl {
         set_metadata(std::move(any));
     }
 
+    // Partitions the segment into n new segments. Each row in the starting segment is mapped to one of the output segments
+    // by the row_to_segment vector (std::nullopt means the row is not included in any output segment).
+    // segment_counts is the length of the number of output segments, and should be greater than or equal to the max value
+    // in row_to_segment
+    inline std::vector<std::shared_ptr<SegmentInMemoryImpl>> partition(const std::vector<std::optional<uint8_t>>& row_to_segment,
+                                                       const std::vector<uint64_t>& segment_counts) const {
+        schema::check<ErrorCode::E_UNSUPPORTED_COLUMN_TYPE>(!is_sparse(),
+                                                            "SegmentInMemory::partition not supported with sparse columns");
+        internal::check<ErrorCode::E_ASSERTION_FAILURE>(row_count() == row_to_segment.size(),
+                    "row_to_segment size does not match segment row count: {} != {}", row_to_segment.size(), row_count());
+        std::vector<std::shared_ptr<SegmentInMemoryImpl>> output(segment_counts.size());
+        if(std::all_of(segment_counts.begin(), segment_counts.end(), [](const size_t& segment_count) { return segment_count == 0; })) {
+            return output;
+        }
+
+        for (const auto& segment_count: folly::enumerate(segment_counts)) {
+            if (*segment_count > 0) {
+                auto& seg = output.at(segment_count.index);
+                seg = get_output_segment(*segment_count);
+                seg->set_row_data(*segment_count - 1);
+                seg->set_string_pool(string_pool_);
+                seg->set_compacted(compacted_);
+                if (metadata_) {
+                    google::protobuf::Any metadata;
+                    metadata.CopyFrom(*metadata_);
+                    seg->set_metadata(std::move(metadata));
+                }
+            }
+        }
+
+        for(const auto& column : folly::enumerate(columns())) {
+            (*column)->type().visit_tag([&] (auto type_desc_tag){
+                using TypeDescriptorTag = decltype(type_desc_tag);
+                using ColumnTagType = typename TypeDescriptorTag::DataTypeTag;
+                using RawType = typename ColumnTagType::raw_type;
+
+                auto output_col_idx = column.index;
+                std::vector<RawType*> output_ptrs{output.size(), nullptr};
+                for (const auto& segment: folly::enumerate(output)) {
+                    if (static_cast<bool>(*segment)) {
+                        output_ptrs.at(segment.index) = reinterpret_cast<RawType*>((*segment)->column(output_col_idx).ptr());
+                    }
+                }
+
+                auto input_data =  (*column)->data();
+                size_t overall_idx = 0;
+                while(auto block = input_data.next<TypeDescriptorTag>()) {
+                    auto input_ptr = reinterpret_cast<const RawType*>(block.value().data());
+                    for (size_t block_idx = 0; block_idx < block.value().row_count(); ++block_idx, ++input_ptr, ++overall_idx) {
+                        auto opt_output_segment_idx = row_to_segment[overall_idx];
+                        if (opt_output_segment_idx.has_value()) {
+                            *(output_ptrs[*opt_output_segment_idx]++) = *input_ptr;
+                        }
+                    }
+                }
+            });
+        }
+        return output;
+    }
+
     std::vector<std::shared_ptr<SegmentInMemoryImpl>> split(size_t rows) const{
         std::vector<std::shared_ptr<SegmentInMemoryImpl>> output;
         util::check(rows > 0, "rows supplied in SegmentInMemoryImpl.split() is non positive");
diff --git a/cpp/arcticdb/pipeline/filter_segment.hpp b/cpp/arcticdb/pipeline/filter_segment.hpp
index f43303f727..8edd88e157 100644
--- a/cpp/arcticdb/pipeline/filter_segment.hpp
+++ b/cpp/arcticdb/pipeline/filter_segment.hpp
@@ -21,4 +21,10 @@ inline SegmentInMemory filter_segment(const SegmentInMemory& input,
     return input.filter(filter_bitset, filter_down_stringpool, validate);
 }
 
+inline std::vector<SegmentInMemory> partition_segment(const SegmentInMemory& input,
+                                      const std::vector<std::optional<uint8_t>>& row_to_segment,
+                                      const std::vector<uint64_t>& segment_counts) {
+    return input.partition(row_to_segment, segment_counts);
+}
+
 } //namespace arcticdb
diff --git a/cpp/arcticdb/processing/clause.cpp b/cpp/arcticdb/processing/clause.cpp
index 9634d8ccf3..8c243567b1 100644
--- a/cpp/arcticdb/processing/clause.cpp
+++ b/cpp/arcticdb/processing/clause.cpp
@@ -29,17 +29,17 @@ std::vector<Composite<ProcessingSegment>> single_partition(std::vector<Composite
 class GroupingMap {
     using NumericMapType = std::variant<
             std::monostate,
-            std::shared_ptr<std::unordered_map<bool, size_t>>,
-            std::shared_ptr<std::unordered_map<uint8_t, size_t>>,
-            std::shared_ptr<std::unordered_map<uint16_t, size_t>>,
-            std::shared_ptr<std::unordered_map<uint32_t, size_t>>,
-            std::shared_ptr<std::unordered_map<uint64_t, size_t>>,
-            std::shared_ptr<std::unordered_map<int8_t, size_t>>,
-            std::shared_ptr<std::unordered_map<int16_t, size_t>>,
-            std::shared_ptr<std::unordered_map<int32_t, size_t>>,
-            std::shared_ptr<std::unordered_map<int64_t, size_t>>,
-            std::shared_ptr<std::unordered_map<float, size_t>>,
-            std::shared_ptr<std::unordered_map<double, size_t>>>;
+            std::shared_ptr<emilib::HashMap<bool, size_t>>,
+            std::shared_ptr<emilib::HashMap<uint8_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<uint16_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<uint32_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<uint64_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<int8_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<int16_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<int32_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<int64_t, size_t>>,
+            std::shared_ptr<emilib::HashMap<float, size_t>>,
+            std::shared_ptr<emilib::HashMap<double, size_t>>>;
 
     NumericMapType map_;
 
@@ -55,16 +55,16 @@ class GroupingMap {
     }
 
     template<typename T>
-    std::shared_ptr<std::unordered_map<T, size_t>> get() {
+    std::shared_ptr<emilib::HashMap<T, size_t>> get() {
         return util::variant_match(map_,
                                    [that = this](const std::monostate &) {
-                                       that->map_ = std::make_shared<std::unordered_map<T, size_t>>();
-                                       return std::get<std::shared_ptr<std::unordered_map<T, size_t>>>(that->map_);
+                                       that->map_ = std::make_shared<emilib::HashMap<T, size_t>>();
+                                       return std::get<std::shared_ptr<emilib::HashMap<T, size_t>>>(that->map_);
                                    },
-                                   [](const std::shared_ptr<std::unordered_map<T, size_t>> &ptr) {
+                                   [](const std::shared_ptr<emilib::HashMap<T, size_t>> &ptr) {
                                        return ptr;
                                    },
-                                   [](const auto &) -> std::shared_ptr<std::unordered_map<T, size_t>> {
+                                   [](const auto &) -> std::shared_ptr<emilib::HashMap<T, size_t>> {
                                        schema::raise<ErrorCode::E_UNSUPPORTED_COLUMN_TYPE>(
                                                "GroupBy does not support the grouping column type changing with dynamic schema");
                                    });
@@ -245,26 +245,45 @@ Composite<ProcessingSegment> AggregationClause::process(std::shared_ptr<Store> s
                                                 row_to_group.reserve(col.column_->row_count());
                                                 auto input_data = col.column_->data();
                                                 auto hash_to_group = grouping_map.get<RawType>();
+                                                // For string grouping columns, keep a local map within this ProcessingSegment
+                                                // from offsets to groups, to avoid needless calls to col.string_at_offset and
+                                                // string_pool->get
+                                                // This could be slower in cases where there aren't many repeats in string
+                                                // grouping columns. Maybe track hit ratio of finds and stop using it if it is
+                                                // too low?
+                                                // Tested with 100,000,000 row dataframe with 100,000 unique values in the grouping column. Timings:
+                                                // 11.14 seconds without caching
+                                                // 11.01 seconds with caching
+                                                // Not worth worrying about right now
+                                                emilib::HashMap<RawType, size_t> offset_to_group;
                                                 while (auto block = input_data.next<ScalarTagType<DataTypeTagType>>()) {
                                                     const auto row_count = block->row_count();
                                                     auto ptr = block->data();
                                                     for (size_t i = 0; i < row_count; ++i, ++ptr) {
                                                         RawType val;
                                                         if constexpr(is_sequence_type(data_type)) {
-                                                            std::optional<std::string_view> str = col.string_at_offset(*ptr);
-                                                            if (str.has_value()) {
-                                                                val = string_pool->get(*str, true).offset();
+                                                            auto offset = *ptr;
+                                                            if (auto it = offset_to_group.find(offset); it != offset_to_group.end()) {
+                                                                val = it->second;
                                                             } else {
-                                                                val = *ptr;
+                                                                std::optional<std::string_view> str = col.string_at_offset(offset);
+                                                                if (str.has_value()) {
+                                                                    val = string_pool->get(*str, true).offset();
+                                                                } else {
+                                                                    val = offset;
+                                                                }
+                                                                RawType val_copy(val);
+                                                                offset_to_group.insert_unique(std::move(offset), std::move(val_copy));
                                                             }
                                                         } else {
                                                             val = *ptr;
                                                         }
                                                         if (auto it = hash_to_group->find(val); it == hash_to_group->end()) {
-                                                            row_to_group.push_back(next_group_id);
-                                                            hash_to_group->try_emplace(val, next_group_id++);
+                                                            row_to_group.emplace_back(next_group_id);
+                                                            auto group_id = next_group_id++;
+                                                            hash_to_group->insert_unique(std::move(val), std::move(group_id));
                                                         } else {
-                                                            row_to_group.push_back(it->second);
+                                                            row_to_group.emplace_back(it->second);
                                                         }
                                                     }
                                                 }
diff --git a/cpp/arcticdb/processing/grouper.hpp b/cpp/arcticdb/processing/grouper.hpp
index 2c8d1c1fea..69a43ba0d3 100644
--- a/cpp/arcticdb/processing/grouper.hpp
+++ b/cpp/arcticdb/processing/grouper.hpp
@@ -23,6 +23,7 @@
 #include <folly/Poly.h>
 #include <arcticdb/column_store/string_pool.hpp>
 #include <arcticdb/util/hash.hpp>
+#include <arcticdb/util/third_party/emilib_map.hpp>
 
 namespace arcticdb::grouping {
 
@@ -35,47 +36,69 @@ class HashingGroupers {
         using DataTypeTag = typename GrouperDescriptor::DataTypeTag;
         using RawType = typename DataTypeTag::raw_type;
 
-        std::optional<size_t> group(RawType key, std::shared_ptr<StringPool> sp) const {
+        // The multiple return statements in here are ugly, but avoids creating temporaries as far as possible
+        std::optional<size_t> group(RawType key, const std::shared_ptr<StringPool>& sp) {
             constexpr DataType dt = DataTypeTag::data_type;
-            std::optional<HashedValue> hash_result;
-            if constexpr(dt == DataType::ASCII_FIXED64 || dt == DataType::ASCII_DYNAMIC64 || dt == DataType::UTF_FIXED64 || dt == DataType::UTF_DYNAMIC64) {
-                if (is_a_string(key)) {
-                    hash_result = hash(sp->get_view(key));
+            if constexpr (dt == DataType::ASCII_FIXED64 || dt == DataType::ASCII_DYNAMIC64 ||
+                          dt == DataType::UTF_FIXED64 || dt == DataType::UTF_DYNAMIC64) {
+                if (auto it = cache_.find(key); it != cache_.end()) {
+                    return it->second;
                 } else {
-                    hash_result = std::nullopt;
+                    if (is_a_string(key)) {
+                        auto hashed_value = hash(sp->get_view(key));
+                        cache_.insert_unique(std::move(key), std::move(hashed_value));
+                        return hashed_value;
+                    } else {
+                        cache_.insert(key, std::nullopt);
+                        return std::nullopt;
+                    }
+                }
+            } else if constexpr(dt == DataType::FLOAT32 || dt == DataType::FLOAT64) {
+                if (std::isnan(key)) {
+                    return std::nullopt;
+                } else {
+                    return hash<RawType>(&key, 1);
                 }
             } else {
-                hash_result = hash<RawType>(&key, 1);
+                return hash<RawType>(&key, 1);
             }
-
-            return hash_result;
         }
+    private:
+        // Only use a cache for grouping on string columns to avoid getting and hashing the same string view repeatedly
+        // No point for numeric types, as we would have to hash it to look it up in this map anyway
+        // This will be slower in cases where there aren't many repeats in string grouping columns
+        // Maybe track cache hit ratio and stop using it if it is too low?
+        // Tested with 100,000,000 row dataframe with 100,000 unique values in the grouping column. Timings:
+        // 10.39 seconds without caching
+        // 11.01 seconds with caching
+        // Not worth worrying about right now
+        emilib::HashMap<RawType, std::optional<size_t>> cache_;
     };
 };
 
 class Bucketizer {
     public:
-    virtual size_t bucket(size_t group) const = 0;
+    virtual uint8_t bucket(size_t group) const = 0;
 
-    virtual size_t num_buckets() const = 0;
+    virtual uint8_t num_buckets() const = 0;
 
     virtual ~Bucketizer() {};
 };
 
 class ModuloBucketizer : Bucketizer {
-    size_t mod_;
+    uint8_t mod_;
 public:
-    ModuloBucketizer(size_t mod) :
+    ModuloBucketizer(uint8_t mod) :
         mod_(mod) {
     }
 
     ARCTICDB_MOVE_COPY_DEFAULT(ModuloBucketizer)
 
-    size_t bucket(size_t group) const {
+    uint8_t bucket(size_t group) const {
         return group % mod_;
     }
 
-    virtual size_t num_buckets() const {
+    virtual uint8_t num_buckets() const {
         return mod_;
     }
 
@@ -84,11 +107,11 @@ class ModuloBucketizer : Bucketizer {
 
 class IdentityBucketizer : Bucketizer {
 public:
-    size_t bucket(size_t group) const {
+    uint8_t bucket(size_t group) const {
         return group;
     }
 
-    virtual size_t num_buckets() const {
+    virtual uint8_t num_buckets() const {
         return 0;
     }
 };
diff --git a/cpp/arcticdb/processing/processing_segment.hpp b/cpp/arcticdb/processing/processing_segment.hpp
index 7a326e179f..b160e2a5f8 100644
--- a/cpp/arcticdb/processing/processing_segment.hpp
+++ b/cpp/arcticdb/processing/processing_segment.hpp
@@ -104,7 +104,9 @@ namespace arcticdb {
         }
 
         size_t get_bucket(){
-            return bucket_.value();
+            internal::check<ErrorCode::E_ASSERTION_FAILURE>(bucket_.has_value(),
+                                                            "ProcessingSegment::get_bucket called, but bucket_ has no value");
+            return *bucket_;
         }
 
         void set_bucket(size_t bucket) {
@@ -137,11 +139,21 @@ namespace arcticdb {
     }
 
     template<typename Grouper, typename Bucketizer>
-    std::vector<std::optional<size_t>> get_buckets(const ColumnWithStrings& col, std::shared_ptr<Grouper> grouper, std::shared_ptr<Bucketizer> bucketizer) {
+    std::pair<std::vector<std::optional<uint8_t>>, std::vector<uint64_t>> get_buckets(
+            const ColumnWithStrings& col,
+            Grouper& grouper,
+            Bucketizer& bucketizer) {
+        schema::check<ErrorCode::E_UNSUPPORTED_COLUMN_TYPE>(!col.column_->is_sparse(),
+                                                            "GroupBy not supported with sparse columns");
         auto input_data = col.column_->data();
+        // Mapping from row to bucket
+        // std::nullopt only for Nones and NaNs in string/float columns
+        std::vector<std::optional<uint8_t>> row_to_bucket;
+        row_to_bucket.reserve(col.column_->row_count());
+        // Tracks how many rows are in each bucket
+        // Use to skip empty buckets, and presize columns in the output ProcessingSegments
+        std::vector<uint64_t> bucket_counts(bucketizer.num_buckets(), 0);
 
-        std::vector<std::optional<size_t>> output;
-        output.reserve(col.column_->row_count());
         using TypeDescriptorTag = typename Grouper::GrouperDescriptor;
         using RawType = typename TypeDescriptorTag::DataTypeTag::raw_type;
 
@@ -150,16 +162,18 @@ namespace arcticdb {
             auto ptr = reinterpret_cast<const RawType*>(block->data());
             for(auto i = 0u; i < row_count; ++i, ++ptr){
                 if constexpr(std::is_same_v<typename Grouper::GrouperDescriptor, TypeDescriptorTag>) {
-                    auto opt_group = grouper->group(*ptr, col.string_pool_);
+                    auto opt_group = grouper.group(*ptr, col.string_pool_);
                     if (opt_group.has_value()) {
-                        output.emplace_back(bucketizer->bucket(*opt_group));
+                        auto bucket = bucketizer.bucket(*opt_group);
+                        row_to_bucket.emplace_back(bucket);
+                        ++bucket_counts[bucket];
                     } else {
-                        output.emplace_back(std::nullopt);
+                        row_to_bucket.emplace_back(std::nullopt);
                     }
                 }
             }
         }
-        return output;
+        return {std::move(row_to_bucket), std::move(bucket_counts)};
     }
 
     template<typename GrouperType, typename BucketizerType>
@@ -168,6 +182,7 @@ namespace arcticdb {
             ProcessingSegment& input,
             const ColumnName& grouping_column_name,
             bool dynamic_schema) {
+
         Composite<ProcessingSegment> output;
         auto get_result = input.get(ColumnName(grouping_column_name), store);
         if (std::holds_alternative<ColumnWithStrings>(get_result)) {
@@ -176,37 +191,32 @@ namespace arcticdb {
                 using TypeDescriptorTag = decltype(type_desc_tag);
                 using ResolvedGrouperType = typename GrouperType::template Grouper<TypeDescriptorTag>;
 
-                auto grouper = std::make_shared<ResolvedGrouperType>(ResolvedGrouperType());
+                ResolvedGrouperType grouper;
                 auto num_buckets = ConfigsMap::instance()->get_int("Partition.NumBuckets", async::TaskScheduler::instance()->cpu_thread_count());
-                auto bucketizer = std::make_shared<BucketizerType>(num_buckets);
-                auto bucket_vec = get_buckets(partitioning_column, grouper, bucketizer);
-                std::vector<util::BitSet> bitsets;
-                bitsets.resize(bucketizer->num_buckets());
-                std::vector<util::BitSet::bulk_insert_iterator> iterators;
-                for(auto& bitset : bitsets)
-                    iterators.emplace_back(util::BitSet::bulk_insert_iterator(bitset));
-
-                for(auto val : folly::enumerate(bucket_vec))
-                    if (val->has_value())
-                        iterators[val->value()] = val.index;
-
-                for(auto& iterator : iterators)
-                    iterator.flush();
-
-                for(auto bitset : folly::enumerate(bitsets)) {
-                    if(bitset->count() != 0) {
-                        ProcessingSegment proc;
-                        proc.set_bucket(bitset.index);
-                        for (auto& seg_slice_and_key : input.data()) {
-                            const SegmentInMemory& seg = seg_slice_and_key.segment(store);
-                            bitset->resize(seg.row_count());
-                            auto new_seg = filter_segment(seg, *bitset);
+                if (num_buckets > std::numeric_limits<uint8_t>::max()) {
+                    log::version().warn("GroupBy partitioning buckets capped at {} (received {})",
+                                        std::numeric_limits<uint8_t>::max(),
+                                        num_buckets);
+                    num_buckets = std::numeric_limits<uint8_t>::max();
+                }
+                std::vector<ProcessingSegment> procs{static_cast<size_t>(num_buckets)};
+                BucketizerType bucketizer(num_buckets);
+                auto [row_to_bucket, bucket_counts] = get_buckets(partitioning_column, grouper, bucketizer);
+                for (auto& seg_slice_and_key : input.data()) {
+                    const SegmentInMemory& seg = seg_slice_and_key.segment(store);
+                    auto new_segs = partition_segment(seg, row_to_bucket, bucket_counts);
+                    for (auto&& new_seg: folly::enumerate(new_segs)) {
+                        if (bucket_counts.at(new_seg.index) > 0) {
                             pipelines::FrameSlice new_slice{seg_slice_and_key.slice()};
-                            new_slice.adjust_rows(new_seg.row_count());
-
-                            proc.data().emplace_back(pipelines::SliceAndKey{std::move(new_seg), std::move(new_slice)});
+                            new_slice.adjust_rows(new_seg->row_count());
+                            procs.at(new_seg.index).data().emplace_back(pipelines::SliceAndKey{std::move(*new_seg), std::move(new_slice)});
                         }
-                        output.push_back(std::move(proc));
+                    }
+                }
+                for (auto&& proc: folly::enumerate(procs)) {
+                    if (bucket_counts.at(proc.index) > 0) {
+                        proc->set_bucket(proc.index);
+                        output.push_back(std::move(*proc));
                     }
                 }
             });
diff --git a/cpp/arcticdb/util/offset_string.hpp b/cpp/arcticdb/util/offset_string.hpp
index 1c1e279021..83f05a0af5 100644
--- a/cpp/arcticdb/util/offset_string.hpp
+++ b/cpp/arcticdb/util/offset_string.hpp
@@ -36,7 +36,7 @@ inline constexpr OffsetString::offset_t not_a_string() { return std::numeric_lim
 inline constexpr OffsetString::offset_t nan_placeholder() { return not_a_string() - 1; }
 
 // Returns true if the provided offset does not represent None or NaN
-inline bool is_a_string(OffsetString::offset_t offset) {
+inline constexpr bool is_a_string(OffsetString::offset_t offset) {
     return offset < nan_placeholder();
 }
 


Test patch:
diff --git a/python/tests/unit/arcticdb/version_store/test_aggregation.py b/python/tests/unit/arcticdb/version_store/test_aggregation.py
index 1411d3fce5..cbfdfddb2e 100644
--- a/python/tests/unit/arcticdb/version_store/test_aggregation.py
+++ b/python/tests/unit/arcticdb/version_store/test_aggregation.py
@@ -23,6 +23,24 @@
 from hypothesis.extra.pandas import column, data_frames, range_indexes
 
 
+def test_group_on_float_column_with_nans(lmdb_version_store):
+    lib = lmdb_version_store
+    sym = "test_group_on_float_column_with_nans"
+    df = pd.DataFrame(
+        {
+            "grouping_column": [1.0, 2.0, np.nan, 1.0, 2.0, 2.0],
+            "agg_column": [1, 2, 3, 4, 5, 6],
+        }
+    )
+    lib.write(sym, df)
+    expected = df.groupby("grouping_column").agg({"agg_column": "sum"})
+    q = QueryBuilder()
+    q = q.groupby("grouping_column").agg({"agg_column": "sum"})
+    received = lib.read(sym, query_builder=q).data
+    received.sort_index(inplace=True)
+    assert_frame_equal(expected, received)
+
+
 @use_of_function_scoped_fixtures_in_hypothesis_checked
 @settings(deadline=None)
 @given(