[VL] Support Decimal type in Gluten #1176

JkSelf · 2023-03-22T02:20:48Z

What changes were proposed in this pull request?

Co-working by @JkSelf @liujiayi771 @jinchengchenghh @rui-mo

How was this patch tested?

Verified TPC-DS 103 queries locally.

test by CH[[378]]

github-actions · 2023-03-22T02:21:08Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[Gluten-${ISSUES_ID}] ${detailed message}

See also:

Other pull requests

jinchengchenghh · 2023-03-23T02:04:53Z

gluten-core/src/main/scala/io/glutenproject/expression/DecimalExpressionsTransformer.scala

+
+import org.apache.spark.sql.catalyst.expressions.Expression
+
+class UnscaledValueTransformer(


Please move this transformer to UnaryExpressionTransformer.scala

it can reuse UnaryExpressionTransformer and don't need to create a new one, right ?

Yes

case class UnscaledValue(child: Expression) extends UnaryExpression

@liujiayi771 Can you help to resolve this comment? Thanks.

I have removed these codes.

zhouyuan · 2023-03-23T03:35:37Z

@zzcclp This patch enabled several decimal related expressions, CK backend ran into seg faults in some tests. Can you take a look?

23/03/23 01:10:40 ERROR Executor: Exception in task 1.0 in stage 56.0 (TID 113)
java.lang.RuntimeException: check_overflow function requires two args.
0. Poco::Exception::Exception(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, int) @ 0x16758bfa in /tmp/libch.so
1. DB::Exception::Exception(DB::Exception::MessageMasked&&, int, bool) @ 0x11c06a95 in /tmp/libch.so
2. DB::Exception::Exception<char const (&) [43], void>(int, char const (&) [43]) @ 0xac96e16 in /tmp/libch.so
3. local_engine::SerializedPlanParser::getFunctionName(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, substrait::Expression_ScalarFunction const&) @ 0xaca0a11 in /tmp/libch.so
4. local_engine::SerializedPlanParser::parseFunctionWithDAG(substrait::Expression const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>&, std::__1::shared_ptr<DB::ActionsDAG>, bool) @ 0xaca9f08 in /tmp/libch.so
5. local_engine::SerializedPlanParser::parseFunctionArgument(std::__1::shared_ptr<DB::ActionsDAG>&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, substrait::FunctionArgument const&) @ 0xacbba4d in /tmp/libch.so
6. local_engine::SerializedPlanParser::parseFunctionArguments(std::__1::shared_ptr<DB::ActionsDAG>&, std::__1::vector<DB::ActionsDAG::Node const*, std::__1::allocator<DB::ActionsDAG::Node const*>>&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, substrait::Expression_ScalarFunction const&) @ 0xacb87ef in /tmp/libch.so
7. local_engine::SerializedPlanParser::parseFunctionWithDAG(substrait::Expression const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>&, std::__1::shared_ptr<DB::ActionsDAG>, bool) @ 0xaca9f46 in /tmp/libch.so
8. local_engine::SerializedPlanParser::parseFunction(DB::Block const&, substrait::Expression const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>&, std::__1::shared_ptr<DB::ActionsDAG>, bool) @ 0xaca1047 in /tmp/libch.so
9. local_engine::SerializedPlanParser::expressionsToActionsDAG(std::__1::vector<substrait::Expression, std::__1::allocator<substrait::Expression>> const&, DB::Block const&, DB::Block const&) @ 0xac9e81d in /tmp/libch.so
10. local_engine::SerializedPlanParser::parseOp(substrait::Rel const&, std::__1::list<substrait::Rel const*, std::__1::allocator<substrait::Rel const*>>&) @ 0xacadf8e in /tmp/libch.so
11. local_engine::SerializedPlanParser::parse(std::__1::unique_ptr<substrait::Plan, std::__1::default_delete<substrait::Plan>>) @ 0xacac73e in /tmp/libch.so
12. local_engine::SerializedPlanParser::parse(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) @ 0xacbc89c in /tmp/libch.so

zzcclp · 2023-03-23T04:14:57Z

@taiyang-li please help to review this pr, it's about decimal type support

zzcclp · 2023-03-23T04:15:42Z

gluten-core/src/main/scala/io/glutenproject/expression/ConverterUtils.scala

+ val decimalType = datatype.asInstanceOf[DecimalType]
+ val precision = decimalType.precision
+ val scale = decimalType.scale
+ typedFuncName.concat("dec<" + precision + "," + scale + ">")


It seems like needs to support this name for ch backend. @taiyang-li

ok, let's do it

taiyang-li · 2023-03-23T04:36:54Z

@JkSelf there are two issues related to clickhouse backend AFAIK:

the new adding functions make_decimal/unscaled_value is not supported by clickhouse yet, we can insert them into CH_EXPR_BLACKLIST_TYPE_EXISTS in CHExpressionUtil.scala
CheckOverflowTransformer is not actually working in ExpressionConverter, maybe we can extract it out of UnaryExpressionTransformer.apply and add it directly in ExpressionConverter. May confirm if velox need it ? cc @loneylee

loneylee · 2023-03-23T05:36:34Z

gluten-core/src/main/scala/io/glutenproject/expression/UnaryExpressionTransformer.scala

 val expressionNodes =
- Lists.newArrayList(childNode, new BooleanLiteralNode(original.nullOnOverflow))
+ Lists.newArrayList(childNode, toTypeNodes, new BooleanLiteralNode(original.nullOnOverflow))


Why do you need to pass the parameter toTypeNodes, there is already a return type in the substrait plan.

@jinchengchenghh Can you help to check whether we can use the return type directly in the substrait plan?

This is from CheckOverflow constructor, velox should use it to match function signature, and decide if it is ShortDecimal or LongDecimal, so as MakeDecimal. This is a special case, the input argument is same with return type, but I think we should not do some exceptional thing when convert substrait plan to velox plan

@JkSelf @jinchengchenghh Maybe we can do this special logic only when backend is velox, or just append toTypeNodes after nullOnOverflow instead before it. Currently CH didn't need toTypeNodes, those changes would cause an incompatiability issue as in CH uts.

OK, I will move it after nullOnOverflow

Please add comment 'test by CH[[378]]'.
Kyligence/ClickHouse#378

Can you merge this one Kyligence/ClickHouse#378? So this PR can pass CH CI

JkSelf · 2023-03-23T06:01:33Z

CheckOverflowTransformer is not actually working in ExpressionConverter, maybe we can extract it out of UnaryExpressionTransformer.apply and add it directly in ExpressionConverter. May confirm if velox need it ? cc @loneylee

@jinchengchenghh Can you help to look this suggestion about CheckOverflowTransformer . Thanks.

JkSelf · 2023-03-23T06:06:05Z

backends-clickhouse/src/main/scala/io/glutenproject/utils/CHExpressionUtil.scala

@@ -41,7 +41,9 @@ object CHExpressionUtil {
 SPLIT_PART -> Set(EMPTY_TYPE),
 TO_UNIX_TIMESTAMP -> Set(DATE_TYPE),
 UNIX_TIMESTAMP -> Set(DATE_TYPE),
- MIGHT_CONTAIN -> Set(EMPTY_TYPE)
+ MIGHT_CONTAIN -> Set(EMPTY_TYPE),


@taiyang-li Please help to review. Thanks.

jinchengchenghh · 2023-03-23T06:06:08Z

@JkSelf there are two issues related to clickhouse backend AFAIK:

the new adding functions make_decimal/unscaled_value is not supported by clickhouse yet, we can insert them into CH_EXPR_BLACKLIST_TYPE_EXISTS in CHExpressionUtil.scala

CheckOverflowTransformer is not actually working in ExpressionConverter, maybe we can extract it out of UnaryExpressionTransformer.apply and add it directly in ExpressionConverter. May confirm if velox need it ? cc @loneylee

For the 2,

case class CheckOverflow(
    child: Expression,
    dataType: DecimalType,
    nullOnOverflow: Boolean) extends UnaryExpression

It extends UnaryExpression, so it should place here, and it is really common used in velox

JkSelf · 2023-03-28T02:28:03Z

@zzcclp @taiyang-li
There are three failed uts in CH backend, can you help to take a look? Thanks.

loneylee · 2023-03-28T02:41:37Z

@zzcclp @taiyang-li There are three failed uts in CH backend, can you help to take a look? Thanks.

Add comment 'test by CH[[378]]'

JkSelf · 2023-03-28T03:26:36Z

test by CH[[378]]

loneylee · 2023-03-28T08:33:57Z

@JkSelf
CH backend fixed errors, Please try test again.

zhouyuan

👍

zhouyuan

👍 if CI is green

JkSelf force-pushed the decimal-test-latest-velox branch from 4151b91 to 44e6e3c Compare March 23, 2023 00:52

jinchengchenghh reviewed Mar 23, 2023

View reviewed changes

zzcclp reviewed Mar 23, 2023

View reviewed changes

loneylee reviewed Mar 23, 2023

View reviewed changes

JkSelf commented Mar 23, 2023

View reviewed changes

zhouyuan added the velox backend works for Velox backend label Mar 23, 2023

JkSelf force-pushed the decimal-test-latest-velox branch 2 times, most recently from bde4e51 to 14e89e0 Compare March 27, 2023 13:58

JkSelf and others added 3 commits March 28, 2023 09:45

add decimal support

d7b1307

support decimal avg and sum

5188606

disable failed decimal uts temporary

630cba9

JkSelf force-pushed the decimal-test-latest-velox branch from 14e89e0 to ee887b3 Compare March 28, 2023 02:03

JkSelf force-pushed the decimal-test-latest-velox branch from ee887b3 to 9a5fbdd Compare March 28, 2023 06:19

zhouyuan previously approved these changes Mar 28, 2023

View reviewed changes

JkSelf dismissed zhouyuan’s stale review via 630cba9 March 28, 2023 12:20

JkSelf force-pushed the decimal-test-latest-velox branch from 9a5fbdd to 630cba9 Compare March 28, 2023 12:20

zhouyuan approved these changes Mar 28, 2023

View reviewed changes

JkSelf merged commit 59a6f3f into apache:main Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VL] Support Decimal type in Gluten #1176

[VL] Support Decimal type in Gluten #1176

JkSelf commented Mar 22, 2023 •

edited by zzcclp

Loading

github-actions bot commented Mar 22, 2023

jinchengchenghh Mar 23, 2023

zzcclp Mar 23, 2023

jinchengchenghh Mar 23, 2023

JkSelf Mar 23, 2023

liujiayi771 Mar 27, 2023

zhouyuan commented Mar 23, 2023

zzcclp commented Mar 23, 2023

zzcclp Mar 23, 2023

taiyang-li Mar 23, 2023

taiyang-li commented Mar 23, 2023

loneylee Mar 23, 2023

JkSelf Mar 23, 2023

jinchengchenghh Mar 23, 2023

taiyang-li Mar 23, 2023

jinchengchenghh Mar 23, 2023

loneylee Mar 24, 2023

jinchengchenghh Mar 28, 2023

JkSelf commented Mar 23, 2023

JkSelf Mar 23, 2023

taiyang-li Mar 23, 2023

jinchengchenghh commented Mar 23, 2023

JkSelf commented Mar 28, 2023

loneylee commented Mar 28, 2023

JkSelf commented Mar 28, 2023

loneylee commented Mar 28, 2023

zhouyuan left a comment

zhouyuan left a comment


		import org.apache.spark.sql.catalyst.expressions.Expression

		class UnscaledValueTransformer(

[VL] Support Decimal type in Gluten #1176

[VL] Support Decimal type in Gluten #1176

Conversation

JkSelf commented Mar 22, 2023 • edited by zzcclp Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyuan commented Mar 23, 2023

zzcclp commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taiyang-li commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JkSelf commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Mar 23, 2023

JkSelf commented Mar 28, 2023

loneylee commented Mar 28, 2023

JkSelf commented Mar 28, 2023

loneylee commented Mar 28, 2023

zhouyuan left a comment

Choose a reason for hiding this comment

zhouyuan left a comment

Choose a reason for hiding this comment

JkSelf commented Mar 22, 2023 •

edited by zzcclp

Loading