Copilot commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4073069001
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -37,13 +38,44 @@ import io.substrait.proto.SimpleExtensionDeclaration
import scala.collection.JavaConverters._
import scala.collection.mutable.ArrayBuffer
+import scala.util.Properties
-class VeloxValidatorApi extends ValidatorApi {
+class VeloxValidatorApi extends ValidatorApi with Logging {
import VeloxValidatorApi._
/** For velox backend, key validation is on native side. */
- override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean =
- true
+ override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean = {
+ expr match {
+ case round: BRound =>
+ round.scale match {
Review Comment:
Similarly, binding a `BRound` instance to `round` is ambiguous (especially
given the existing `Round` expression). Renaming to `bround` makes the
validator logic clearer and reduces the chance of mistakes when extending
validation for `Round` vs `BRound`.
##########
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 round: BRound =>
+ BackendsApiManager.getSparkPlanExecApiInstance.genBRoundTransformer(
+ substraitExprName,
+ round.children.map(replaceWithExpressionTransformer0(_,
attributeSeq, expressionsMap)),
+ round)
Review Comment:
The pattern variable name `round` is misleading here because the matched
type is `BRound` (and Spark also has a distinct `Round`). Rename the binding to
something like `bround` to avoid confusion in stack traces and future edits.
##########
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));
+}
+
+TEST_F(SparkFunctionTest, broundIntegralOverflowModes) {
+ auto input =
makeRowVector({makeFlatVector<int64_t>({std::numeric_limits<int64_t>::max()})});
+ queryCtx_->testingOverrideConfigUnsafe({{sparkAnsiEnabledConfigKey(),
"false"}});
+ facebook::velox::test::assertEqualVectors(
+ makeFlatVector<int64_t>({-8'446'744'073'709'551'616LL}),
evaluate("bround(c0, cast(-19 as integer))", input));
+ queryCtx_->testingOverrideConfigUnsafe({{sparkAnsiEnabledConfigKey(),
"true"}});
+ VELOX_ASSERT_THROW(evaluate("bround(c0, cast(-19 as integer))", input),
"overflow");
+}
Review Comment:
`testingOverrideConfigUnsafe` mutates `queryCtx_` and this test leaves ANSI
enabled set to `true` at the end, which can leak into subsequent `TEST_F` cases
that reuse the same fixture. Capture the prior value and restore it (or clear
the override) before returning, so later tests remain isolated and
order-independent.
--
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]
