From 2a644c8b08248b6a195255f3bc6f2f34397a9efc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= <git@rmr.ninja>
Date: Wed, 28 Feb 2024 18:28:38 +0100
Subject: [PATCH] Improvements based on PR review

---
 .../AggregateFunctionAnyRespectNulls.cpp           | 14 ++++++++------
 src/AggregateFunctions/SingleValueData.h           |  5 +++--
 src/Common/findExtreme.cpp                         |  2 ++
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/AggregateFunctions/AggregateFunctionAnyRespectNulls.cpp b/src/AggregateFunctions/AggregateFunctionAnyRespectNulls.cpp
index f87e3deb6bc..7275409c151 100644
--- a/src/AggregateFunctions/AggregateFunctionAnyRespectNulls.cpp
+++ b/src/AggregateFunctions/AggregateFunctionAnyRespectNulls.cpp
@@ -19,7 +19,7 @@ namespace
 {
 struct AggregateFunctionAnyRespectNullsData
 {
-    enum Status : UInt8
+    enum class Status : UInt8
     {
         NotSet = 1,
         SetNull = 2,
@@ -142,17 +142,17 @@ public:
     void serialize(ConstAggregateDataPtr __restrict place, WriteBuffer & buf, std::optional<size_t> /* version */) const override
     {
         auto & d = this->data(place);
-        UInt8 k = d.status;
+        UInt8 k = static_cast<UInt8>(d.status);
 
         writeBinaryLittleEndian<UInt8>(k, buf);
-        if (k == Data::Status::SetOther)
+        if (d.status == Data::Status::SetOther)
             serialization->serializeBinary(d.value, buf, {});
     }
 
     void deserialize(AggregateDataPtr place, ReadBuffer & buf, std::optional<size_t> /* version */, Arena *) const override
     {
         auto & d = this->data(place);
-        UInt8 k = Data::Status::NotSet;
+        UInt8 k = 0;
         readBinaryLittleEndian<UInt8>(k, buf);
         d.status = static_cast<Data::Status>(k);
         if (d.status == Data::Status::NotSet)
@@ -164,9 +164,11 @@ public:
             return;
         }
         else if (d.status == Data::Status::SetOther)
+        {
             serialization->deserializeBinary(d.value, buf, {});
-        else
-            throw Exception(ErrorCodes::INCORRECT_DATA, "Incorrect type ({}) in {}State", static_cast<Int8>(k), getName());
+            return;
+        }
+        throw Exception(ErrorCodes::INCORRECT_DATA, "Incorrect type ({}) in {}State", static_cast<Int8>(k), getName());
     }
 
     void insertResultInto(AggregateDataPtr __restrict place, IColumn & to, Arena *) const override
diff --git a/src/AggregateFunctions/SingleValueData.h b/src/AggregateFunctions/SingleValueData.h
index 4e8256b20ef..2ba98f8a9c3 100644
--- a/src/AggregateFunctions/SingleValueData.h
+++ b/src/AggregateFunctions/SingleValueData.h
@@ -186,6 +186,7 @@ struct SingleValueDataNumeric final : public SingleValueDataBase
     using Base = SingleValueDataFixed<T>;
 
 private:
+    /// 32 bytes for types of 256 bits, + 8 bytes for the virtual table pointer.
     static constexpr size_t base_memory_reserved_size = 40;
     struct alignas(alignof(Base)) PrivateMemory
     {
@@ -269,10 +270,10 @@ struct SingleValueDataString final : public SingleValueDataBase
     static constexpr bool is_compilable = false;
     using Self = SingleValueDataString;
 
-    /// 0 size indicates that there is no value. Empty string must has terminating '\0' and, therefore, size of empty string is 1
+    /// 0 size indicates that there is no value. Empty string must have terminating '\0' and, therefore, size of empty string is 1
     UInt32 size = 0;
     UInt32 capacity = 0; /// power of two or zero
-    char * large_data;
+    char * large_data; /// Always allocated in an arena
 
     //// TODO: Maybe instead of a virtual class we need to go with a std::variant of the 3 to avoid reserving space for the vtable
     static constexpr UInt32 MAX_SMALL_STRING_SIZE
diff --git a/src/Common/findExtreme.cpp b/src/Common/findExtreme.cpp
index 2d131cfd524..ea8c1bcb57e 100644
--- a/src/Common/findExtreme.cpp
+++ b/src/Common/findExtreme.cpp
@@ -90,6 +90,7 @@ MULTITARGET_FUNCTION_AVX2_SSE42(
                 }
                 else if constexpr (is_min)
                 {
+                    /// Double negation is necessary to help the compiler vectorize the operation
                     bool keep_number = add_if_cond_zero ? !condition_map[i] : !!condition_map[i];
                     /// If keep_number = ptr[i] * 1 + 0 * max = ptr[i]
                     /// If not keep_number = ptr[i] * 0 + 1 * max = max
@@ -99,6 +100,7 @@ MULTITARGET_FUNCTION_AVX2_SSE42(
                 else
                 {
                     static_assert(std::same_as<ComparatorClass, MaxComparator<T>>);
+                    /// Double negation is necessary to help the compiler vectorize the operation
                     bool keep_number = add_if_cond_zero ? !condition_map[i] : !!condition_map[i];
                     /// If keep_number = ptr[i] * 1 + 0 * lowest = ptr[i]
                     /// If not keep_number = ptr[i] * 0 + 1 * lowest = lowest