Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CORE] Remove an extra argument of make_decimal function #3754

Merged
merged 2 commits into from
Nov 24, 2023
Merged

[CORE] Remove an extra argument of make_decimal function #3754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5077382
Integrate with Velox make_decimal function
rui-mo Nov 16, 2023
86aac8f
Update make decimal args.
loneylee Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cpp-ch/local-engine/Parser/SerializedPlanParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,10 +733,10 @@ SerializedPlanParser::getFunctionName(const std::string & function_signature, co
}
else if (function_name == "make_decimal")
{
if (args.size() < 3)
throw Exception(ErrorCodes::BAD_ARGUMENTS, "make_decimal function requires at least 3 args.");
if (args.size() < 2)
throw Exception(ErrorCodes::BAD_ARGUMENTS, "make_decimal function requires at least 2 args.");
ch_function_name = SCALAR_FUNCTIONS.at(function_name);
auto null_on_overflow = args.at(2).value().literal().boolean();
auto null_on_overflow = args.at(1).value().literal().boolean();
if (null_on_overflow)
ch_function_name = ch_function_name + "OrNull";
}
Expand Down Expand Up @@ -1006,8 +1006,8 @@ const ActionsDAG::Node * SerializedPlanParser::parseFunctionWithDAG(
}
else if (startsWith(function_signature, "make_decimal:"))
{
if (scalar_function.arguments().size() < 3)
throw Exception(ErrorCodes::BAD_ARGUMENTS, "make_decimal function requires at least three args.");
if (scalar_function.arguments().size() < 2)
throw Exception(ErrorCodes::BAD_ARGUMENTS, "make_decimal function requires at least 2 args.");

ActionsDAG::NodeRawConstPtrs new_args;
new_args.reserve(3);
Expand Down
1 change: 0 additions & 1 deletion cpp/velox/substrait/SubstraitParser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,6 @@ std::unordered_map<std::string, std::string> SubstraitParser::substraitVeloxFunc
{"bit_and_merge", "bitwise_and_agg_merge"},
{"collect_set", "array_distinct"},
{"murmur3hash", "hash_with_seed"},
{"make_decimal", "make_decimal_by_unscaled_value"},
{"modulus", "mod"} /*Presto functions.*/
};

Expand Down
6 changes: 1 addition & 5 deletions gluten-core/src/main/scala/io/glutenproject/expression/UnaryExpressionTransformer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,8 @@ case class MakeDecimalTransformer(
Seq(original.dataType, BooleanType),
FunctionConfig.OPT))

// use fake decimal literal, because velox function signature need to get return type
// scale and precision by input type variable
val toTypeNodes =
ExpressionBuilder.makeDecimalLiteral(new Decimal().set(0, original.precision, original.scale))
val expressionNodes =
Lists.newArrayList(childNode, toTypeNodes, new BooleanLiteralNode(original.nullOnOverflow))
Lists.newArrayList(childNode, new BooleanLiteralNode(original.nullOnOverflow))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzcclp Is toTypeNodes needed for CH backend?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loneylee please help to check this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not need, but the function parameters have been changed, and the ch backend needs to update too.

    else if (function_name == "make_decimal")
    {
        if (args.size() < 3)
            throw Exception(ErrorCodes::BAD_ARGUMENTS, "make_decimal function requires at least 3 args.");
        ch_function_name = SCALAR_FUNCTIONS.at(function_name);
        auto null_on_overflow = args.at(2).value().literal().boolean();
        if (null_on_overflow)
            ch_function_name = ch_function_name + "OrNull";
    }

CI test is passed. Maybe ut is not open. I will commit a pr to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loneylee Modified in this PR. Could you help review? Thanks.

val typeNode = ConverterUtils.getTypeNode(original.dataType, original.nullable)
ExpressionBuilder.makeScalarFunction(functionId, expressionNodes, typeNode)
}
Expand Down
2 changes: 1 addition & 1 deletion ...-ut/spark32/src/test/scala/io/glutenproject/utils/clickhouse/ClickHouseTestSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ class ClickHouseTestSettings extends BackendTestSettings {
.exclude("Gluten - TIMESTAMP_MICROS")
.exclude("Gluten - unix_timestamp")
.exclude("Gluten - to_unix_timestamp")
enableSuite[GlutenDecimalExpressionSuite].exclude("MakeDecimal")
enableSuite[GlutenDecimalExpressionSuite]
enableSuite[GlutenHashExpressionsSuite]
.exclude("sha2")
.exclude("murmur3/xxHash64/hive hash: struct<null:void,boolean:boolean,byte:tinyint,short:smallint,int:int,long:bigint,float:float,double:double,bigDecimal:decimal(38,18),smallDecimal:decimal(10,0),string:string,binary:binary,date:date,timestamp:timestamp,udt:examplepoint>")
Expand Down
2 changes: 1 addition & 1 deletion ...-ut/spark33/src/test/scala/io/glutenproject/utils/clickhouse/ClickHouseTestSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,7 @@ class ClickHouseTestSettings extends BackendTestSettings {
.exclude("Gluten - TIMESTAMP_MICROS")
.exclude("Gluten - unix_timestamp")
.exclude("Gluten - to_unix_timestamp")
enableSuite[GlutenDecimalExpressionSuite].exclude("MakeDecimal")
enableSuite[GlutenDecimalExpressionSuite]
enableSuite[GlutenHashExpressionsSuite]
.exclude("sha2")
.exclude("murmur3/xxHash64/hive hash: struct<null:void,boolean:boolean,byte:tinyint,short:smallint,int:int,long:bigint,float:float,double:double,bigDecimal:decimal(38,18),smallDecimal:decimal(10,0),string:string,binary:binary,date:date,timestamp:timestamp,udt:examplepoint>")
Expand Down
Loading