You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "zhli1142015 (via GitHub)" <gi...@apache.org> on 2024/06/04 23:26:12 UTC
[PR] [VL] Handle try_subtract [incubator-gluten]
zhli1142015 opened a new pull request, #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985
## What changes were proposed in this pull request?
Follow up to https://github.com/apache/incubator-gluten/pull/5356
## How was this patch tested?
UT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2149450704
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627369020
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.TRY_ADD
+ )
+ case tryEval @ TryEval(a: Subtract) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_SUBTRACT
+ )
+ case tryEval @ TryEval(a: Divide) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_DIVIDE
+ )
+ case tryEval @ TryEval(a: Multiply) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_MULTIPLY
)
case a: Add =>
- BackendsApiManager.getSparkPlanExecApiInstance.genAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- a
+ a,
+ ExpressionNames.TRY_ADD
Review Comment:
I wonder why we map Add to try_add. Is it to solve some semantic difference?
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"try_add"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"try_subtract"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"try_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"try_divide"});
Review Comment:
I notice they are registered in Velox Presto functions. Can we also register them in Velox Spark functions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628607266
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -123,42 +123,50 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
original)
}
- override def genTryAddTransformer(
+ override def genTryArithmeticTransformer(
substraitExprName: String,
left: ExpressionTransformer,
right: ExpressionTransformer,
- original: TryEval): ExpressionTransformer = {
+ original: TryEval,
+ checkArithmeticExprName: String): ExpressionTransformer = {
if (SparkShimLoader.getSparkShims.withAnsiEvalMode(original.child)) {
- throw new GlutenNotSupportException(s"add with ansi mode is not supported")
+ throw new GlutenNotSupportException(
+ s"${original.child.prettyName} with ansi mode is not supported")
}
original.child.dataType match {
case LongType | IntegerType | ShortType | ByteType =>
- case _ => throw new GlutenNotSupportException(s"try_add is not supported")
+ case _ => throw new GlutenNotSupportException(s"$substraitExprName is not supported")
}
// Offload to velox for only IntegralTypes.
GenericExpressionTransformer(
substraitExprName,
- Seq(GenericExpressionTransformer(ExpressionNames.TRY_ADD, Seq(left, right), original)),
+ Seq(GenericExpressionTransformer(checkArithmeticExprName, Seq(left, right), original)),
original)
}
- override def genAddTransformer(
+ /**
+ * Map arithmetic expr to different functions: substraitExprName or try(checkArithmeticExprName)
+ * based on EvalMode.
+ */
+ override def genArithmeticTransformer(
Review Comment:
Could you explain the bit the difference of `genTryArithmeticTransformer` and `genArithmeticTransformer`? I notice genTryArithmeticTransformer does not generate a try function but genArithmeticTransformer does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2148778006
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628603742
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.CHECK_ADD
+ )
Review Comment:
For this case, is the substraitExprName 'try', and are we generating a try(check_add) function? I notice in Velox, try means returning null when exception happens, which seems to be different with TryEval in Spark which requires failing on error.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -123,42 +123,50 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
original)
}
- override def genTryAddTransformer(
+ override def genTryArithmeticTransformer(
substraitExprName: String,
left: ExpressionTransformer,
right: ExpressionTransformer,
- original: TryEval): ExpressionTransformer = {
+ original: TryEval,
+ checkArithmeticExprName: String): ExpressionTransformer = {
if (SparkShimLoader.getSparkShims.withAnsiEvalMode(original.child)) {
- throw new GlutenNotSupportException(s"add with ansi mode is not supported")
+ throw new GlutenNotSupportException(
+ s"${original.child.prettyName} with ansi mode is not supported")
}
original.child.dataType match {
case LongType | IntegerType | ShortType | ByteType =>
- case _ => throw new GlutenNotSupportException(s"try_add is not supported")
+ case _ => throw new GlutenNotSupportException(s"$substraitExprName is not supported")
}
// Offload to velox for only IntegralTypes.
GenericExpressionTransformer(
substraitExprName,
- Seq(GenericExpressionTransformer(ExpressionNames.TRY_ADD, Seq(left, right), original)),
+ Seq(GenericExpressionTransformer(checkArithmeticExprName, Seq(left, right), original)),
original)
}
- override def genAddTransformer(
+ /**
+ * Map arithmetic expr to different functions: substraitExprName or try(checkArithmeticExprName)
+ * based on EvalMode.
+ */
+ override def genArithmeticTransformer(
Review Comment:
Could you explain the bit the difference of `genTryArithmeticTransformer` and `genArithmeticTransformer`? I notice genTryArithmeticTransformer does not gen a try function but genArithmeticTransformer does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627460879
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -123,42 +123,44 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
original)
}
- override def genTryAddTransformer(
+ override def genTryArithmeticTransformer(
substraitExprName: String,
left: ExpressionTransformer,
right: ExpressionTransformer,
- original: TryEval): ExpressionTransformer = {
+ original: TryEval,
+ funcName: String): ExpressionTransformer = {
Review Comment:
Updated, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628716297
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.CHECK_ADD
+ )
Review Comment:
I see. Thanks for the explain!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628697272
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.CHECK_ADD
+ )
Review Comment:
Yes, here is `try`, I think it has same behavior in spark and velox: return null when error happens.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala#L40C1-L46C6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627335359
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"check_plus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"check_minus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"check_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"check_divide"});
Review Comment:
Make sense, updated. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628696612
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -123,42 +123,50 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
original)
}
- override def genTryAddTransformer(
+ override def genTryArithmeticTransformer(
substraitExprName: String,
left: ExpressionTransformer,
right: ExpressionTransformer,
- original: TryEval): ExpressionTransformer = {
+ original: TryEval,
+ checkArithmeticExprName: String): ExpressionTransformer = {
if (SparkShimLoader.getSparkShims.withAnsiEvalMode(original.child)) {
- throw new GlutenNotSupportException(s"add with ansi mode is not supported")
+ throw new GlutenNotSupportException(
+ s"${original.child.prettyName} with ansi mode is not supported")
}
original.child.dataType match {
case LongType | IntegerType | ShortType | ByteType =>
- case _ => throw new GlutenNotSupportException(s"try_add is not supported")
+ case _ => throw new GlutenNotSupportException(s"$substraitExprName is not supported")
}
// Offload to velox for only IntegralTypes.
GenericExpressionTransformer(
substraitExprName,
- Seq(GenericExpressionTransformer(ExpressionNames.TRY_ADD, Seq(left, right), original)),
+ Seq(GenericExpressionTransformer(checkArithmeticExprName, Seq(left, right), original)),
original)
}
- override def genAddTransformer(
+ /**
+ * Map arithmetic expr to different functions: substraitExprName or try(checkArithmeticExprName)
+ * based on EvalMode.
+ */
+ override def genArithmeticTransformer(
Review Comment:
Yes, All the try_xx expressions are `RuntimeReplaceable `, they are converted to some other expressions, Before spark 3.4, they are converted to `TryEval(XX(failOnError = true))`, https://github.com/apache/spark/blob/branch-3.3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala#L81, `genTryArithmeticTransformer ` is added to handle this case.
After spark 3.4, for numeric types, they are converted to `XX(EvalMode.TRY)`, `genArithmeticTransformer` is added to handle this case, for velox we still convert it to `TryEval(XX(failOnError = true))`.
Both are converted to `try(check_xx)` in velox.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628706746
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.CHECK_ADD
+ )
Review Comment:
We notice failOnError is set true for EvalMode.TRY, so I'm a bit confused here.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L201-L206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627410680
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.TRY_ADD
+ )
+ case tryEval @ TryEval(a: Subtract) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_SUBTRACT
+ )
+ case tryEval @ TryEval(a: Divide) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_DIVIDE
+ )
+ case tryEval @ TryEval(a: Multiply) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_MULTIPLY
)
case a: Add =>
- BackendsApiManager.getSparkPlanExecApiInstance.genAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- a
+ a,
+ ExpressionNames.TRY_ADD
Review Comment:
After spark 3.4 `try_add` is converted to to `add(EvalMode.TRY)`, here may generate wrong results as we don't map it to correct velox functions. `genArithmeticTransformer` would map add to correct functions based on the `EvalMode`.
Thanks.
https://github.com/apache/incubator-gluten/pull/5356#issue-2234987326
https://github.com/apache/spark/blob/c4f720dfb41919dade7002b49462b3ff6b91eb22/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala#L82C6-L82C78
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627674762
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"check_plus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"check_minus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"check_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"check_divide"});
Review Comment:
I think it's better to use names: `check_xx`. As we translate `try_xx` to `try(check_xx)`. if we use name try_xx, then they may be confusing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628706746
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.CHECK_ADD
+ )
Review Comment:
We notice failOnError is set true for `EvalMode.TRY`, so I'm a bit confused here.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L201-L206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 merged PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627417541
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"try_add"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"try_subtract"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"try_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"try_divide"});
Review Comment:
Yes, if we can support them in Velox Spark functions, we could remove the custom registration in Gluten.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627430941
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"try_add"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"try_subtract"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"try_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"try_divide"});
Review Comment:
We can firstly merge this pr. Then remove the registering after velox pr is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2148572159
<!--
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/LICEGJ-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.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on Github Issues?
https://github.com/apache/incubator-gluten/issues
Then could you also rename ***commit message*** and ***pull request title*** in the following format?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1628713141
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.CHECK_ADD
+ )
Review Comment:
Before spark 3.4 try_add is convert to `TryEval(Add(failOnError = true))`, after 3.4 it is converted to Add(EvalMode.TRY), and try-catch logic is executed in https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L365. I think both have same semantic: return null when error happens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627410680
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.TRY_ADD
+ )
+ case tryEval @ TryEval(a: Subtract) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_SUBTRACT
+ )
+ case tryEval @ TryEval(a: Divide) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_DIVIDE
+ )
+ case tryEval @ TryEval(a: Multiply) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_MULTIPLY
)
case a: Add =>
- BackendsApiManager.getSparkPlanExecApiInstance.genAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- a
+ a,
+ ExpressionNames.TRY_ADD
Review Comment:
After spark 3.4 `try_add` is converted to to `add(EvalMode.TRY)`, here may generate wrong results as we don't map `add()` to correct velox functions. `genArithmeticTransformer` would map add to correct functions based on the `EvalMode`.
Thanks.
https://github.com/apache/incubator-gluten/pull/5356#issue-2234987326
https://github.com/apache/spark/blob/c4f720dfb41919dade7002b49462b3ff6b91eb22/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TryEval.scala#L82C6-L82C78
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627315319
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"check_plus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"check_minus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"check_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"check_divide"});
Review Comment:
Can we just register these functions with spark-aligned names: try_add, try_subtract, try_divide, try_multiply?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627399732
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"try_add"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"try_subtract"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"try_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"try_divide"});
Review Comment:
Do you mean register them here: https://github.com/facebookincubator/velox/blob/main/velox/functions/sparksql/RegisterArithmetic.cpp#L39?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2149020428
cc @rui-mo and @PHILO-HE , thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2149297479
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2149696778
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627435656
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -123,42 +123,44 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
original)
}
- override def genTryAddTransformer(
+ override def genTryArithmeticTransformer(
substraitExprName: String,
left: ExpressionTransformer,
right: ExpressionTransformer,
- original: TryEval): ExpressionTransformer = {
+ original: TryEval,
+ funcName: String): ExpressionTransformer = {
Review Comment:
It is better to add a 'try_' prefix on substraitExprName if the try-function is needed to avoid introducing two functions names?
If we want to keep this implementation, could we rename funcName maybe like tryFunctionName and also document it to avoid misunderstanding? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627436223
##########
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -563,18 +563,68 @@ object ExpressionConverter extends SQLConfHelper with Logging {
arrayTransform
)
case tryEval @ TryEval(a: Add) =>
- BackendsApiManager.getSparkPlanExecApiInstance.genTryAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- tryEval
+ tryEval,
+ ExpressionNames.TRY_ADD
+ )
+ case tryEval @ TryEval(a: Subtract) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_SUBTRACT
+ )
+ case tryEval @ TryEval(a: Divide) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_DIVIDE
+ )
+ case tryEval @ TryEval(a: Multiply) =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genTryArithmeticTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
+ replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
+ tryEval,
+ ExpressionNames.TRY_MULTIPLY
)
case a: Add =>
- BackendsApiManager.getSparkPlanExecApiInstance.genAddTransformer(
+ BackendsApiManager.getSparkPlanExecApiInstance.genArithmeticTransformer(
substraitExprName,
replaceWithExpressionTransformerInternal(a.left, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(a.right, attributeSeq, expressionsMap),
- a
+ a,
+ ExpressionNames.TRY_ADD
Review Comment:
I understand. Thanks for the explain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "GlutenPerfBot (via GitHub)" <gi...@apache.org>.
GlutenPerfBot commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2151520949
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
<table border="1" cellpadding="0" cellspacing="0">
<tbody>
<tr>
<td><font color=#000000>query</font></td>
<td><font color=#000000>log/native_5985_time.csv</font></td>
<td><font color=#000000>log/native_master_06_05_2024_54aeb010d_time.csv</font></td>
<td><font color=#000000>difference</font></td>
<td><font color=#000000>percentage</font></td>
</tr>
<tr>
<td><font color=#6F9915>q1</font></td>
<td><font color=#6F9915>35.32</font></td>
<td><font color=#6F9915>34.06</font></td>
<td><font color=#6F9915>-1.266</font></td>
<td><font color=#6F9915>96.42%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q2</font></td>
<td><font color=#6F9915>23.73</font></td>
<td><font color=#6F9915>23.10</font></td>
<td><font color=#6F9915>-0.627</font></td>
<td><font color=#6F9915>97.36%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q3</font></td>
<td><font color=#6F9915>37.62</font></td>
<td><font color=#6F9915>36.59</font></td>
<td><font color=#6F9915>-1.033</font></td>
<td><font color=#6F9915>97.25%</font></td>
</tr>
<tr>
<td><font color=#F92663>q4</font></td>
<td><font color=#F92663>31.56</font></td>
<td><font color=#F92663>34.70</font></td>
<td><font color=#F92663>3.138</font></td>
<td><font color=#F92663>109.94%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q5</font></td>
<td><font color=#6F9915>69.91</font></td>
<td><font color=#6F9915>69.24</font></td>
<td><font color=#6F9915>-0.676</font></td>
<td><font color=#6F9915>99.03%</font></td>
</tr>
<tr>
<td><font color=#F92663>q6</font></td>
<td><font color=#F92663>7.53</font></td>
<td><font color=#F92663>7.84</font></td>
<td><font color=#F92663>0.307</font></td>
<td><font color=#F92663>104.08%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q7</font></td>
<td><font color=#6F9915>82.50</font></td>
<td><font color=#6F9915>81.42</font></td>
<td><font color=#6F9915>-1.082</font></td>
<td><font color=#6F9915>98.69%</font></td>
</tr>
<tr>
<td><font color=#F92663>q8</font></td>
<td><font color=#F92663>87.04</font></td>
<td><font color=#F92663>87.21</font></td>
<td><font color=#F92663>0.163</font></td>
<td><font color=#F92663>100.19%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q9</font></td>
<td><font color=#6F9915>124.78</font></td>
<td><font color=#6F9915>119.35</font></td>
<td><font color=#6F9915>-5.429</font></td>
<td><font color=#6F9915>95.65%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q10</font></td>
<td><font color=#6F9915>45.74</font></td>
<td><font color=#6F9915>45.23</font></td>
<td><font color=#6F9915>-0.509</font></td>
<td><font color=#6F9915>98.89%</font></td>
</tr>
<tr>
<td><font color=#F92663>q11</font></td>
<td><font color=#F92663>20.38</font></td>
<td><font color=#F92663>22.65</font></td>
<td><font color=#F92663>2.267</font></td>
<td><font color=#F92663>111.12%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q12</font></td>
<td><font color=#6F9915>25.56</font></td>
<td><font color=#6F9915>25.04</font></td>
<td><font color=#6F9915>-0.520</font></td>
<td><font color=#6F9915>97.97%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q13</font></td>
<td><font color=#6F9915>40.29</font></td>
<td><font color=#6F9915>36.70</font></td>
<td><font color=#6F9915>-3.595</font></td>
<td><font color=#6F9915>91.08%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q14</font></td>
<td><font color=#6F9915>17.81</font></td>
<td><font color=#6F9915>17.35</font></td>
<td><font color=#6F9915>-0.464</font></td>
<td><font color=#6F9915>97.40%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q15</font></td>
<td><font color=#6F9915>31.24</font></td>
<td><font color=#6F9915>29.97</font></td>
<td><font color=#6F9915>-1.270</font></td>
<td><font color=#6F9915>95.94%</font></td>
</tr>
<tr>
<td><font color=#F92663>q16</font></td>
<td><font color=#F92663>14.42</font></td>
<td><font color=#F92663>15.12</font></td>
<td><font color=#F92663>0.692</font></td>
<td><font color=#F92663>104.80%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q17</font></td>
<td><font color=#6F9915>104.54</font></td>
<td><font color=#6F9915>104.42</font></td>
<td><font color=#6F9915>-0.114</font></td>
<td><font color=#6F9915>99.89%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q18</font></td>
<td><font color=#6F9915>145.45</font></td>
<td><font color=#6F9915>144.82</font></td>
<td><font color=#6F9915>-0.629</font></td>
<td><font color=#6F9915>99.57%</font></td>
</tr>
<tr>
<td><font color=#F92663>q19</font></td>
<td><font color=#F92663>13.48</font></td>
<td><font color=#F92663>14.62</font></td>
<td><font color=#F92663>1.143</font></td>
<td><font color=#F92663>108.48%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q20</font></td>
<td><font color=#6F9915>26.73</font></td>
<td><font color=#6F9915>26.48</font></td>
<td><font color=#6F9915>-0.250</font></td>
<td><font color=#6F9915>99.06%</font></td>
</tr>
<tr>
<td><font color=#F92663>q21</font></td>
<td><font color=#F92663>260.73</font></td>
<td><font color=#F92663>266.79</font></td>
<td><font color=#F92663>6.061</font></td>
<td><font color=#F92663>102.32%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q22</font></td>
<td><font color=#6F9915>14.62</font></td>
<td><font color=#6F9915>12.29</font></td>
<td><font color=#6F9915>-2.333</font></td>
<td><font color=#6F9915>84.04%</font></td>
</tr>
<tr>
<td><font color=#6F9915>total</font></td>
<td><font color=#6F9915>1260.98</font></td>
<td><font color=#6F9915>1254.96</font></td>
<td><font color=#6F9915>-6.025</font></td>
<td><font color=#6F9915>99.52%</font></td>
</tr>
</tbody>
</table>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2148796528
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627437914
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"try_add"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"try_subtract"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"try_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"try_divide"});
Review Comment:
Makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2148572326
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#discussion_r1627674762
##########
cpp/velox/operators/functions/RegistrationAllFunctions.cc:
##########
@@ -67,6 +68,10 @@ void registerFunctionOverwrite() {
kRowConstructorWithAllNull,
std::make_unique<RowConstructorWithNullCallToSpecialForm>(kRowConstructorWithAllNull));
velox::functions::sparksql::registerBitwiseFunctions("spark_");
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedPlusFunction>({"check_plus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMinusFunction>({"check_minus"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedMultiplyFunction>({"check_multiply"});
+ velox::functions::registerBinaryIntegral<velox::functions::CheckedDivideFunction>({"check_divide"});
Review Comment:
I think it's better to use names: `check_xx`. As we translate `try_xx` to `try(check_xx)`. if we use name try_xx, then they may be confusing. Please let me know if this makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Handle try_subtract, try_multiply, try_divide [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5985:
URL: https://github.com/apache/incubator-gluten/pull/5985#issuecomment-2149725950
Run Gluten Clickhouse CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org