luis4a0 commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4075624373
##########
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:
No reset is needed here: GoogleTest creates and destroys a fresh fixture for
each `TEST_F`; it explicitly does not reuse the fixture across tests. See
https://google.github.io/googletest/primer.html#same-data-multiple-tests.
In the public dependency, `queryCtx_` is a non-static member initialized by
`QueryCtx::create(...)` for each fixture instance:
https://github.com/IBM/velox/blob/33c4cdb51dfb9df674496a977db1201df727cc70/velox/functions/prestosql/tests/utils/FunctionBaseTest.h#L400-L402.
The override therefore does not leak through this member into subsequent
cases. Keeping the test unchanged; all six `SparkFunctionTest` cases also
passed 20 shuffled iterations (120 executions) with the public BROUND-patched
dependency described in this PR.
##########
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:
Renamed the binding and its references to `bround` in
https://github.com/apache/gluten/commit/60378882795ce94e01204bbeee1b81eb5df10051.
The paired validator test now also distinguishes `Round` and `BRound` across
out-of-range, negative, zero, and positive scales. Behavior is unchanged; the
validator, transformer, and full BROUND integration suites passed on Java 17
and 21.
##########
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:
Renamed the converter binding and both references to `bround` in
https://github.com/apache/gluten/commit/60378882795ce94e01204bbeee1b81eb5df10051.
Dispatch, child order, and the original expression passed to
`genBRoundTransformer` are unchanged. Rebuilt the affected modules and passed
all nine BROUND integration tests on both Java 17 and Java 21.
--
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]
