From 53395a88df64f5ba52b836507b170c7d5ab69fb9 Mon Sep 17 00:00:00 2001
From: yan ma <yan.ma@intel.com>
Date: Fri, 22 Mar 2024 23:11:19 +0800
Subject: [PATCH] fix min_by/max_by null handle in final aggregation

---
 .../lib/aggregates/MinMaxByAggregatesBase.h   | 46 ++++---------------
 1 file changed, 10 insertions(+), 36 deletions(-)

diff --git a/velox/functions/lib/aggregates/MinMaxByAggregatesBase.h b/velox/functions/lib/aggregates/MinMaxByAggregatesBase.h
index 6776a071b379..e2c4db609262 100644
--- a/velox/functions/lib/aggregates/MinMaxByAggregatesBase.h
+++ b/velox/functions/lib/aggregates/MinMaxByAggregatesBase.h
@@ -354,26 +354,17 @@ class MinMaxByAggregateBase : public exec::Aggregate {
 
     decodedValue_.decode(*baseRowVector->childAt(0), rows);
     decodedComparison_.decode(*baseRowVector->childAt(1), rows);
-
-    if (decodedIntermediateResult_.isConstantMapping() &&
-        decodedIntermediateResult_.isNullAt(0)) {
-      return;
-    }
-    if (decodedIntermediateResult_.mayHaveNulls()) {
-      rows.applyToSelected([&](vector_size_t i) {
-        if (decodedIntermediateResult_.isNullAt(i)) {
-          return;
-        }
-        const auto decodedIndex = decodedIntermediateResult_.index(i);
-        updateValues(
-            groups[i],
-            decodedValue_,
-            decodedComparison_,
-            decodedIndex,
-            decodedValue_.isNullAt(decodedIndex),
-            mayUpdate);
-      });
+    if (decodedIntermediateResult_.isConstantMapping()) {
+      const auto decodedIndex = decodedIntermediateResult_.index(0);
+      updateValues(
+          groups[0],
+          decodedValue_,
+          decodedComparison_,
+          decodedIndex,
+          decodedValue_.isNullAt(decodedIndex),
+          mayUpdate);
     } else {
+      // We don't need special null value handle in final aggregation
       rows.applyToSelected([&](vector_size_t i) {
         const auto decodedIndex = decodedIntermediateResult_.index(i);
         updateValues(
@@ -457,9 +448,6 @@ class MinMaxByAggregateBase : public exec::Aggregate {
     decodedComparison_.decode(*baseRowVector->childAt(1), rows);
 
     if (decodedIntermediateResult_.isConstantMapping()) {
-      if (decodedIntermediateResult_.isNullAt(0)) {
-        return;
-      }
       const auto decodedIndex = decodedIntermediateResult_.index(0);
       updateValues(
           group,
@@ -468,20 +456,6 @@ class MinMaxByAggregateBase : public exec::Aggregate {
           decodedIndex,
           decodedValue_.isNullAt(decodedIndex),
           mayUpdate);
-    } else if (decodedIntermediateResult_.mayHaveNulls()) {
-      rows.applyToSelected([&](vector_size_t i) {
-        if (decodedIntermediateResult_.isNullAt(i)) {
-          return;
-        }
-        const auto decodedIndex = decodedIntermediateResult_.index(i);
-        updateValues(
-            group,
-            decodedValue_,
-            decodedComparison_,
-            decodedIndex,
-            decodedValue_.isNullAt(decodedIndex),
-            mayUpdate);
-      });
     } else {
       rows.applyToSelected([&](vector_size_t i) {
         const auto decodedIndex = decodedIntermediateResult_.index(i);