address comments

apache · Apr 29, 2024 · bd5ebc7 · bd5ebc7
1 parent 758c372
commit bd5ebc7
Show file tree

Hide file tree

Showing 5 changed files with 14 additions and 80 deletions.
diff --git a/backends-velox/src/main/scala/org/apache/gluten/utils/VeloxIntermediateData.scala b/backends-velox/src/main/scala/org/apache/gluten/utils/VeloxIntermediateData.scala
@@ -161,6 +161,9 @@ object VeloxIntermediateData {
   def getRowConstructFuncName(aggFunc: AggregateFunction): String = aggFunc match {
     case _: Average | _: Sum if aggFunc.dataType.isInstanceOf[DecimalType] =>
       "row_constructor"
+    // For agg function min_by/max_by, it needs to keep rows with null value but non-null
+    // comparison, such as <null, 5>. So we set the struct to null when all of the arguments
+    // are null
     case _: MaxMinBy =>
       "row_constructor_with_all_null"
     case _ => "row_constructor_with_null"

diff --git a/cpp/velox/CMakeLists.txt b/cpp/velox/CMakeLists.txt
@@ -306,7 +306,6 @@ set(VELOX_SRCS
     memory/VeloxMemoryManager.cc
     operators/functions/RegistrationAllFunctions.cc
     operators/functions/RowConstructorWithNull.cc
-    operators/functions/RowConstructorWithAllNull.cc
     operators/functions/SparkTokenizer.cc
     operators/serializer/VeloxColumnarToRowConverter.cc
     operators/serializer/VeloxColumnarBatchSerializer.cc

diff --git a/cpp/velox/operators/functions/RowConstructorWithAllNull.cc b/cpp/velox/operators/functions/RowConstructorWithAllNull.cc
diff --git a/cpp/velox/operators/functions/RowConstructorWithAllNull.h b/cpp/velox/operators/functions/RowConstructorWithAllNull.h
@@ -17,20 +17,11 @@
 
 #pragma once
 
-#include "velox/expression/FunctionCallToSpecialForm.h"
-#include "velox/expression/SpecialForm.h"
+#include "RowConstructorWithNull.h"
 
 namespace gluten {
-class RowConstructorWithAllNullCallToSpecialForm : public facebook::velox::exec::FunctionCallToSpecialForm {
+class RowConstructorWithAllNullCallToSpecialForm : public RowConstructorWithNullCallToSpecialForm {
  public:
-  facebook::velox::TypePtr resolveType(const std::vector<facebook::velox::TypePtr>& argTypes) override;
-
-  facebook::velox::exec::ExprPtr constructSpecialForm(
-      const facebook::velox::TypePtr& type,
-      std::vector<facebook::velox::exec::ExprPtr>&& compiledChildren,
-      bool trackCpuUsage,
-      const facebook::velox::core::QueryConfig& config) override;
-
   static constexpr const char* kRowConstructorWithAllNull = "row_constructor_with_all_null";
 
  protected:
@@ -39,6 +30,8 @@ class RowConstructorWithAllNullCallToSpecialForm : public facebook::velox::exec:
       const facebook::velox::TypePtr& type,
       std::vector<facebook::velox::exec::ExprPtr>&& compiledChildren,
       bool trackCpuUsage,
-      const facebook::velox::core::QueryConfig& config);
+      const facebook::velox::core::QueryConfig& config) {
+    return constructSpecialForm(kRowConstructorWithAllNull, type, std::move(compiledChildren), trackCpuUsage, config);
+  }
 };
 } // namespace gluten
diff --git a/cpp/velox/operators/functions/RowFunctionWithNull.h b/cpp/velox/operators/functions/RowFunctionWithNull.h
@@ -50,7 +50,7 @@ class RowFunctionWithNull final : public facebook::velox::exec::VectorFunction {
           if (arg->mayHaveNulls() && arg->isNullAt(i)) {
             // For row_constructor_with_null, if any argument of the struct is null,
             // set the struct as null.
-            if (!allNull) {
+            if constexpr (!allNull) {
               facebook::velox::bits::setNull(nullsPtr, i, true);
               cntNull++;
               break;
@@ -60,9 +60,11 @@ class RowFunctionWithNull final : public facebook::velox::exec::VectorFunction {
           }
         }
         // For row_constructor_with_all_null, set the struct to be null when all arguments are all
-        if (allNull && argsNullCnt == argsCopy.size()) {
-          facebook::velox::bits::setNull(nullsPtr, i, true);
-          cntNull++;
+        if constexpr (allNull) {
+          if (argsNullCnt == argsCopy.size()) {
+            facebook::velox::bits::setNull(nullsPtr, i, true);
+            cntNull++;
+          }
         }
       }
     });