Copilot commented on code in PR #13047:
URL: https://github.com/apache/gluten/pull/13047#discussion_r4075771722
##########
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:
This calls `Properties.isJavaAtLeast(...)` on every validation, which can be
invoked many times during planning. Consider caching the computed boolean once
(e.g., `private val isJavaQualifiedForFloatingBround =
Properties.isJavaAtLeast(MIN_BROUND_FLOATING_JAVA_VERSION)`) and referencing it
here, to avoid repeated parsing/check overhead and keep the condition easier to
read.
##########
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 the shared `queryCtx_` and the test
leaves it in ANSI=true at the end, which can cause order-dependent failures if
later tests rely on defaults. Please restore the prior config (or explicitly
set back to the suite default) at the end of the test, or use a scoped/RAII
helper if one exists in the test framework.
--
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]
