From bd5ebc7b123598698257c93c0a37451553a364b2 Mon Sep 17 00:00:00 2001 From: yan ma Date: Mon, 29 Apr 2024 20:03:53 +0800 Subject: [PATCH] address comments --- .../gluten/utils/VeloxIntermediateData.scala | 3 + cpp/velox/CMakeLists.txt | 1 - .../functions/RowConstructorWithAllNull.cc | 63 ------------------- .../functions/RowConstructorWithAllNull.h | 17 ++--- .../operators/functions/RowFunctionWithNull.h | 10 +-- 5 files changed, 14 insertions(+), 80 deletions(-) delete mode 100644 cpp/velox/operators/functions/RowConstructorWithAllNull.cc 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 index 8f95afe8d0e8..a00bcae1ce70 100644 --- 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 . 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 index 146b094c5cbb..55f45881b108 100644 --- 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 deleted file mode 100644 index 9b9da2d8bf66..000000000000 --- a/cpp/velox/operators/functions/RowConstructorWithAllNull.cc +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "RowConstructorWithAllNull.h" -#include "velox/expression/VectorFunction.h" - -namespace gluten { -facebook::velox::TypePtr RowConstructorWithAllNullCallToSpecialForm::resolveType( - const std::vector& argTypes) { - auto numInput = argTypes.size(); - std::vector names(numInput); - std::vector types(numInput); - for (auto i = 0; i < numInput; i++) { - types[i] = argTypes[i]; - names[i] = fmt::format("c{}", i + 1); - } - return facebook::velox::ROW(std::move(names), std::move(types)); -} - -facebook::velox::exec::ExprPtr RowConstructorWithAllNullCallToSpecialForm::constructSpecialForm( - const std::string& name, - const facebook::velox::TypePtr& type, - std::vector&& compiledChildren, - bool trackCpuUsage, - const facebook::velox::core::QueryConfig& config) { - auto [function, metadata] = facebook::velox::exec::vectorFunctionFactories().withRLock( - [&config, &name](auto& functionMap) -> std::pair< - std::shared_ptr, - facebook::velox::exec::VectorFunctionMetadata> { - auto functionIterator = functionMap.find(name); - if (functionIterator != functionMap.end()) { - return {functionIterator->second.factory(name, {}, config), functionIterator->second.metadata}; - } else { - VELOX_FAIL("Function {} is not registered.", name); - } - }); - - return std::make_shared( - type, std::move(compiledChildren), function, metadata, name, trackCpuUsage); -} - -facebook::velox::exec::ExprPtr RowConstructorWithAllNullCallToSpecialForm::constructSpecialForm( - const facebook::velox::TypePtr& type, - std::vector&& compiledChildren, - bool trackCpuUsage, - const facebook::velox::core::QueryConfig& config) { - return constructSpecialForm(kRowConstructorWithAllNull, type, std::move(compiledChildren), trackCpuUsage, config); -} -} // namespace gluten diff --git a/cpp/velox/operators/functions/RowConstructorWithAllNull.h b/cpp/velox/operators/functions/RowConstructorWithAllNull.h index 186498d8b30c..dfc79e1a977b 100644 --- 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& argTypes) override; - - facebook::velox::exec::ExprPtr constructSpecialForm( - const facebook::velox::TypePtr& type, - std::vector&& 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&& 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 index 9ee1828785f2..4131fb472ddd 100644 --- 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++; + } } } });