Copilot commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4091991824
##########
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:
This adds a native `bround` mapping, but the repository's checked-in Velox
build still leaves `UPSTREAM_VELOX_PR_ID` empty
(`ep/build-velox/src/get-velox.sh:28`) and only applies an upstream patch when
that variable is set (`:136-148`). Since the companion Velox change is not part
of this PR, a standard build can reach this transformer with no registered
`bround`/`decimal_bround` functions and fail at execution. Please land the
dependency pin/update (or otherwise enforce the prerequisite) before enabling
this native path.
##########
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:
This mapping makes native plans resolve BROUND to `bround`/`decimal_bround`,
but the repository's pinned source in `ep/build-velox/src/get-velox.sh` is
still `dft-2026_09_14`, whose stock tag does not provide these registrations. A
clean Gluten build therefore reaches an unavailable native function unless the
companion Velox change is pinned before this patch is merged; please include
that dependency update in the merge sequence or keep this integration gated
until it is available.
##########
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:
These tests invoke the native `bround` function, but the repository's
default Velox build still checks out `dft-2026_09_14` and leaves
`UPSTREAM_VELOX_PR_ID` empty (`ep/build-velox/src/get-velox.sh:20-28`), while
the companion change is only applied when that ID is supplied
(`ep/build-velox/src/get-velox.sh:136-150`). Without updating the pinned
dependency (or wiring the companion patch into the build), the normal native
build/CI cannot register this function and these tests will fail at runtime;
please land that dependency change before enabling this suite.
--
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]
