This is an automated email from the ASF dual-hosted git repository. ulyssesyou pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/incubator-gluten.git
The following commit(s) were added to refs/heads/main by this push: new 33f993554 [GLUTEN-4652][VL] Fix min_by/max_by result mismatch when RDD partition num > 1 (#5711) 33f993554 is described below commit 33f993554bebc388c7011dd91b86eaadc729f0d5 Author: zhouyifan279 <88070094+zhouyifan...@users.noreply.github.com> AuthorDate: Mon May 13 19:03:41 2024 +0800 [GLUTEN-4652][VL] Fix min_by/max_by result mismatch when RDD partition num > 1 (#5711) --- .../execution/VeloxAggregateFunctionsSuite.scala | 18 ++++------- .../functions/RegistrationAllFunctions.cc | 23 ++++++++------ .../functions/RowConstructorWithAllNull.h | 37 ---------------------- .../operators/functions/RowConstructorWithNull.cc | 10 +----- .../operators/functions/RowConstructorWithNull.h | 8 +++++ 5 files changed, 28 insertions(+), 68 deletions(-) diff --git a/backends-velox/src/test/scala/org/apache/gluten/execution/VeloxAggregateFunctionsSuite.scala b/backends-velox/src/test/scala/org/apache/gluten/execution/VeloxAggregateFunctionsSuite.scala index 398f5e05e..faa361edf 100644 --- a/backends-velox/src/test/scala/org/apache/gluten/execution/VeloxAggregateFunctionsSuite.scala +++ b/backends-velox/src/test/scala/org/apache/gluten/execution/VeloxAggregateFunctionsSuite.scala @@ -194,18 +194,12 @@ abstract class VeloxAggregateFunctionsSuite extends VeloxWholeStageTransformerSu } test("min_by/max_by") { - withTempPath { - path => - Seq((5: Integer, 6: Integer), (null: Integer, 11: Integer), (null: Integer, 5: Integer)) - .toDF("a", "b") - .write - .parquet(path.getCanonicalPath) - spark.read - .parquet(path.getCanonicalPath) - .createOrReplaceTempView("test") - runQueryAndCompare("select min_by(a, b), max_by(a, b) from test") { - checkGlutenOperatorMatch[HashAggregateExecTransformer] - } + withSQLConf(("spark.sql.leafNodeDefaultParallelism", "2")) { + runQueryAndCompare( + "select min_by(a, b), max_by(a, b) from " + + "values (5, 6), (null, 11), (null, 5) test(a, b)") { + checkGlutenOperatorMatch[HashAggregateExecTransformer] + } } } diff --git a/cpp/velox/operators/functions/RegistrationAllFunctions.cc b/cpp/velox/operators/functions/RegistrationAllFunctions.cc index c77fa47e5..5a6b0f6aa 100644 --- a/cpp/velox/operators/functions/RegistrationAllFunctions.cc +++ b/cpp/velox/operators/functions/RegistrationAllFunctions.cc @@ -15,11 +15,10 @@ * limitations under the License. */ #include "operators/functions/RegistrationAllFunctions.h" + #include "operators/functions/Arithmetic.h" -#include "operators/functions/RowConstructorWithAllNull.h" #include "operators/functions/RowConstructorWithNull.h" #include "operators/functions/RowFunctionWithNull.h" - #include "velox/expression/SpecialFormRegistry.h" #include "velox/expression/VectorFunction.h" #include "velox/functions/lib/RegistrationHelpers.h" @@ -45,29 +44,32 @@ void registerFunctionOverwrite() { velox::registerFunction<RoundFunction, double, double, int32_t>({"round"}); velox::registerFunction<RoundFunction, float, float, int32_t>({"round"}); + auto kRowConstructorWithNull = RowConstructorWithNullCallToSpecialForm::kRowConstructorWithNull; velox::exec::registerVectorFunction( - "row_constructor_with_null", + kRowConstructorWithNull, std::vector<std::shared_ptr<velox::exec::FunctionSignature>>{}, std::make_unique<RowFunctionWithNull</*allNull=*/false>>(), RowFunctionWithNull</*allNull=*/false>::metadata()); velox::exec::registerFunctionCallToSpecialForm( - RowConstructorWithNullCallToSpecialForm::kRowConstructorWithNull, - std::make_unique<RowConstructorWithNullCallToSpecialForm>()); + kRowConstructorWithNull, std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithNull)); + + auto kRowConstructorWithAllNull = RowConstructorWithNullCallToSpecialForm::kRowConstructorWithAllNull; velox::exec::registerVectorFunction( - "row_constructor_with_all_null", + kRowConstructorWithAllNull, std::vector<std::shared_ptr<velox::exec::FunctionSignature>>{}, std::make_unique<RowFunctionWithNull</*allNull=*/true>>(), RowFunctionWithNull</*allNull=*/true>::metadata()); velox::exec::registerFunctionCallToSpecialForm( - RowConstructorWithAllNullCallToSpecialForm::kRowConstructorWithAllNull, - std::make_unique<RowConstructorWithAllNullCallToSpecialForm>()); + kRowConstructorWithAllNull, + std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull)); velox::functions::sparksql::registerBitwiseFunctions("spark_"); } } // namespace void registerAllFunctions() { // The registration order matters. Spark sql functions are registered after - // presto sql functions to overwrite the registration for same named functions. + // presto sql functions to overwrite the registration for same named + // functions. velox::functions::prestosql::registerAllScalarFunctions(); velox::functions::sparksql::registerFunctions(""); velox::aggregate::prestosql::registerAllAggregateFunctions( @@ -76,7 +78,8 @@ void registerAllFunctions() { "", true /*registerCompanionFunctions*/, true /*overwrite*/); velox::window::prestosql::registerAllWindowFunctions(); velox::functions::window::sparksql::registerWindowFunctions(""); - // Using function overwrite to handle function names mismatch between Spark and Velox. + // Using function overwrite to handle function names mismatch between Spark + // and Velox. registerFunctionOverwrite(); } diff --git a/cpp/velox/operators/functions/RowConstructorWithAllNull.h b/cpp/velox/operators/functions/RowConstructorWithAllNull.h deleted file mode 100644 index dfc79e1a9..000000000 --- a/cpp/velox/operators/functions/RowConstructorWithAllNull.h +++ /dev/null @@ -1,37 +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. - */ - -#pragma once - -#include "RowConstructorWithNull.h" - -namespace gluten { -class RowConstructorWithAllNullCallToSpecialForm : public RowConstructorWithNullCallToSpecialForm { - public: - static constexpr const char* kRowConstructorWithAllNull = "row_constructor_with_all_null"; - - protected: - facebook::velox::exec::ExprPtr constructSpecialForm( - const std::string& name, - const facebook::velox::TypePtr& type, - std::vector<facebook::velox::exec::ExprPtr>&& 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/RowConstructorWithNull.cc b/cpp/velox/operators/functions/RowConstructorWithNull.cc index 955d957e2..e8b8a2883 100644 --- a/cpp/velox/operators/functions/RowConstructorWithNull.cc +++ b/cpp/velox/operators/functions/RowConstructorWithNull.cc @@ -32,11 +32,11 @@ facebook::velox::TypePtr RowConstructorWithNullCallToSpecialForm::resolveType( } facebook::velox::exec::ExprPtr RowConstructorWithNullCallToSpecialForm::constructSpecialForm( - const std::string& name, const facebook::velox::TypePtr& type, std::vector<facebook::velox::exec::ExprPtr>&& compiledChildren, bool trackCpuUsage, const facebook::velox::core::QueryConfig& config) { + auto name = this->rowFunctionName; auto [function, metadata] = facebook::velox::exec::vectorFunctionFactories().withRLock( [&config, &name](auto& functionMap) -> std::pair< std::shared_ptr<facebook::velox::exec::VectorFunction>, @@ -52,12 +52,4 @@ facebook::velox::exec::ExprPtr RowConstructorWithNullCallToSpecialForm::construc return std::make_shared<facebook::velox::exec::Expr>( type, std::move(compiledChildren), function, metadata, name, trackCpuUsage); } - -facebook::velox::exec::ExprPtr RowConstructorWithNullCallToSpecialForm::constructSpecialForm( - const facebook::velox::TypePtr& type, - std::vector<facebook::velox::exec::ExprPtr>&& compiledChildren, - bool trackCpuUsage, - const facebook::velox::core::QueryConfig& config) { - return constructSpecialForm(kRowConstructorWithNull, type, std::move(compiledChildren), trackCpuUsage, config); -} } // namespace gluten diff --git a/cpp/velox/operators/functions/RowConstructorWithNull.h b/cpp/velox/operators/functions/RowConstructorWithNull.h index 6cfeaee37..66b745e3e 100644 --- a/cpp/velox/operators/functions/RowConstructorWithNull.h +++ b/cpp/velox/operators/functions/RowConstructorWithNull.h @@ -23,6 +23,10 @@ namespace gluten { class RowConstructorWithNullCallToSpecialForm : public facebook::velox::exec::FunctionCallToSpecialForm { public: + RowConstructorWithNullCallToSpecialForm(const std::string& rowFunctionName) { + this->rowFunctionName = rowFunctionName; + } + facebook::velox::TypePtr resolveType(const std::vector<facebook::velox::TypePtr>& argTypes) override; facebook::velox::exec::ExprPtr constructSpecialForm( @@ -32,6 +36,7 @@ class RowConstructorWithNullCallToSpecialForm : public facebook::velox::exec::Fu const facebook::velox::core::QueryConfig& config) override; static constexpr const char* kRowConstructorWithNull = "row_constructor_with_null"; + static constexpr const char* kRowConstructorWithAllNull = "row_constructor_with_all_null"; protected: facebook::velox::exec::ExprPtr constructSpecialForm( @@ -40,5 +45,8 @@ class RowConstructorWithNullCallToSpecialForm : public facebook::velox::exec::Fu std::vector<facebook::velox::exec::ExprPtr>&& compiledChildren, bool trackCpuUsage, const facebook::velox::core::QueryConfig& config); + + private: + std::string rowFunctionName; }; } // namespace gluten --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@gluten.apache.org For additional commands, e-mail: commits-h...@gluten.apache.org