luis4a0 commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4092180105
##########
cpp/velox/tests/SparkFunctionTest.cc:
##########
@@ -125,6 +127,21 @@ TEST_F(SparkFunctionTest, roundWithDecimal) {
runRoundWithDecimalTest<int8_t>(testRoundWithDecIntegralData<int8_t>());
}
+TEST_F(SparkFunctionTest, bround) {
+ auto input = makeRowVector({makeNullableFlatVector<double>({2.5, 3.5, -2.5,
-3.5, std::nullopt})});
+ facebook::velox::test::assertEqualVectors(
+ makeNullableFlatVector<double>({2.0, 4.0, -2.0, -4.0, std::nullopt}),
evaluate("bround(c0)", input));
Review Comment:
Agreed. This is a real merge prerequisite, already recorded in the PR
description's **Dependency** section, not an infrastructure flake.
The required sequence is:
1. Merge the native implementation:
https://github.com/facebookincubator/velox/pull/19072
2. Incorporate it into a compatible IBM Velox tag and update Gluten's pin.
3. Rebuild and pass normal native/Spark CI before merging this companion.
The native PR is still open and unmerged, so the current stock pin cannot
satisfy these tests. Local paired qualification used that public dependency
plus the companion implementation; it was not a claim that the unpatched pin
passes. I will not disable the offload assertions or insert a temporary
unmerged-PR patch to hide this dependency.
Leaving this prerequisite thread open until the compatible dependency update
is available and validated.
##########
cpp/velox/substrait/SubstraitParser.cc:
##########
@@ -284,8 +284,8 @@ std::string SubstraitParser::mapToVeloxFunction(const
std::string& substraitFunc
substraitFunction == "gte" || substraitFunction == "equal") {
return "decimal_" + it->second;
}
- if (substraitFunction == "round") {
- return "decimal_round";
+ if (substraitFunction == "round" || substraitFunction == "bround") {
+ return "decimal_" + substraitFunction;
}
Review Comment:
Agreed. This is a real merge prerequisite, already recorded in the PR
description's **Dependency** section, not an infrastructure flake.
The required sequence is:
1. Merge the native implementation:
https://github.com/facebookincubator/velox/pull/19072
2. Incorporate it into a compatible IBM Velox tag and update Gluten's pin.
3. Rebuild and pass normal native/Spark CI before merging this companion.
The native PR is still open and unmerged, so the current stock pin cannot
satisfy these tests. Local paired qualification used that public dependency
plus the companion implementation; it was not a claim that the unpatched pin
passes. I will not disable the offload assertions or insert a temporary
unmerged-PR patch to hide this dependency.
Leaving this prerequisite thread open until the compatible dependency update
is available and validated.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -656,6 +656,11 @@ object ExpressionConverter extends SQLConfHelper with
Logging {
throw new GlutenNotSupportException(
"CheckOverflowInTableInsert is used in ANSI mode, but Gluten does
not support ANSI mode."
)
+ case bround: BRound =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genBRoundTransformer(
+ substraitExprName,
+ bround.children.map(replaceWithExpressionTransformer0(_,
attributeSeq, expressionsMap)),
+ bround)
Review Comment:
Agreed. This is a real merge prerequisite, already recorded in the PR
description's **Dependency** section, not an infrastructure flake.
The required sequence is:
1. Merge the native implementation:
https://github.com/facebookincubator/velox/pull/19072
2. Incorporate it into a compatible IBM Velox tag and update Gluten's pin.
3. Rebuild and pass normal native/Spark CI before merging this companion.
The native PR is still open and unmerged, so the current stock pin cannot
satisfy these tests. Local paired qualification used that public dependency
plus the companion implementation; it was not a claim that the unpatched pin
passes. I will not disable the offload assertions or insert a temporary
unmerged-PR patch to hide this dependency.
Leaving this prerequisite thread open until the compatible dependency update
is available and validated.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
