luis4a0 commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4082235861
##########
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 bround: BRound =>
+ bround.scale match {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) =>
+ if (scale < MIN_BROUND_SCALE || scale > MAX_BROUND_SCALE) {
+ logDebug(
+ s"bround scale $scale is outside the native " +
+ s"[$MIN_BROUND_SCALE, $MAX_BROUND_SCALE] interval; " +
+ "falling back to Spark.")
+ false
+ } else if (
+ scale != 0 &&
+ (bround.child.dataType == FloatType || bround.child.dataType ==
DoubleType) &&
+ !Properties.isJavaAtLeast(MIN_BROUND_FLOATING_JAVA_VERSION)
+ ) {
Review Comment:
Addressed in
https://github.com/apache/gluten/commit/d4bff51fe14c485fc7f883464ac70ee7b31e66bb.
The validator companion now computes a private immutable JVM-qualification
Boolean once and reuses it across validator instances. The regression fails
with the repeated-check implementation and verifies the cached behavior for
both an existing and a newly constructed validator, restoring the temporarily
changed property in `finally`.
The 12 validator/transformer tests and all 9 BROUND integration tests pass
on Java 17 and Java 21. Scale bounds, NULL/zero-scale handling, and native
eligibility are unchanged.
##########
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:
`queryCtx_` is not shared between these test cases. GoogleTest creates and
destroys a fresh fixture for each `TEST_F`, and `FunctionBaseTest` initializes
this non-static member with a separate `QueryCtx::create(...)` for each
instance:
https://google.github.io/googletest/primer.html#same-data-multiple-tests
https://github.com/IBM/velox/blob/33c4cdb51dfb9df674496a977db1201df727cc70/velox/functions/prestosql/tests/utils/FunctionBaseTest.h#L400-L402
This is the same isolation concern addressed previously at
https://github.com/apache/gluten/pull/13047#discussion_r4073068953. I re-ran
all six `SparkFunctionTest` cases for 20 shuffled iterations (120 executions);
all passed. Keeping this test unchanged rather than resetting a context that is
destroyed before the next fixture is constructed.
--
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]
